Quoting Hogander, Jouni (2025-07-18 07:46:31-03:00)
>On Fri, 2025-07-18 at 09:05 +0300, Hogander, Jouni wrote:
>> On Thu, 2025-07-17 at 10:31 -0300, Gustavo Sousa wrote:
>> > Quoting Jouni Högander (2025-07-17 03:32:58-03:00)
>> > > We are seeing "dmesg-warn/abort - *ERROR* PHY * failed after 3
>> > > retries"
>> > > since we started configuring LFPS sending. According to Bspec
>> > > Configuring
>> > > LFPS sending is needed only when using AUXLess ALPM. This patch
>> > > avoids
>> > > these failures by configuring LFPS sending only when using
>> > > AUXLess
>> > > ALPM.
>> >
>> > Hm... But then with this patch we are missing writing zero to that
>> > bit
>> > when necessary, no?
>>
>> That shouldn't be necessary as 0 is the reset value.
>>
>> >
>> > Could the timeouts be happening because intel_cx0_rmw() is getting
>> > called without calling
>> > intel_cx0_phy_transaction_begin()/intel_cx0_phy_transaction_end()?
>>
>> I wasn't aware about these. I will try them.
>
>I tested this and it doesn't help:
Okay. Well, I still find it weird that this would time out for one case
and not time out for another... Do we have confirmation that this is
working fine for the AUX-Less ALPM case?
I wonder if we should rather do this step together with
intel_c10_pll_program(). Note that, for C10, there is also a required
step to set PHY_C10_VDR_CONTROL1[2] before accessing the msgbus.
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index ed8e640b96b0..e6ff7f07b2e3 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -3239,6 +3239,7 @@ void intel_lnl_mac_transmit_lfps(struct
>intel_encoder *encoder,
> const struct intel_crtc_state
>*crtc_state)
> {
> struct intel_display *display = to_intel_display(encoder);
>+ intel_wakeref_t wakeref;
> u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(encoder);
> bool enable =
>intel_alpm_is_alpm_aux_less(enc_to_intel_dp(encoder),
> crtc_state);
>@@ -3247,6 +3248,8 @@ void intel_lnl_mac_transmit_lfps(struct
>intel_encoder *encoder,
> if (DISPLAY_VER(display) < 20)
> return;
>
>+ wakeref = intel_cx0_phy_transaction_begin(encoder);
>+
> for (i = 0; i < 4; i++) {
> int tx = i % 2 + 1;
> u8 lane_mask = i < 2 ? INTEL_CX0_LANE0 :
>INTEL_CX0_LANE1;
>@@ -3259,6 +3262,8 @@ void intel_lnl_mac_transmit_lfps(struct
>intel_encoder *encoder,
> enable ? CONTROL0_MAC_TRANSMIT_LFPS : 0,
> MB_WRITE_COMMITTED);
> }
>+
>+ intel_cx0_phy_transaction_end(encoder, wakeref);
> }
>
>Do you think I should still add this change as well?
If we are still going with this function instead of doing it in
intel_c10_pll_program(), then yes.
--
Gustavo Sousa
>
>>
>> BR,
>>
>> Jouni Högander
>>
>> >
>> > >
>> > > Fixes: 9dc619680de4 ("drm/i915/display: Add function to configure
>> > > LFPS sending")
>> > > Signed-off-by: Jouni Högander <jouni.hogan...@intel.com>
>> > > ---
>> > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 11 +++++------
>> > > 1 file changed, 5 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > index ed8e640b96b0..9cfc3187aeab 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > @@ -3239,14 +3239,14 @@ void intel_lnl_mac_transmit_lfps(struct
>> > > intel_encoder *encoder,
>> > > const struct intel_crtc_state
>> > > *crtc_state)
>> > > {
>> > > struct intel_display *display =
>> > > to_intel_display(encoder);
>> > > - u8 owned_lane_mask =
>> > > intel_cx0_get_owned_lane_mask(encoder);
>> > > - bool enable =
>> > > intel_alpm_is_alpm_aux_less(enc_to_intel_dp(encoder),
>> > > - crtc_state);
>> > > + u8 owned_lane_mask;
>> > > int i;
>> > >
>> > > - if (DISPLAY_VER(display) < 20)
>> > > + if (DISPLAY_VER(display) < 20 ||
>> > > +
>> > > !intel_alpm_is_alpm_aux_less(enc_to_intel_dp(encoder),
>> > > crtc_state))
>> > > return;
>> > >
>> > > + owned_lane_mask =
>> > > intel_cx0_get_owned_lane_mask(encoder);
>> >
>> > This optimization could be on it's own patch.
>
>Ok, maybe I leave that out or add own patch.
>
>BR,
>
>Jouni Högander
>
>
>> >
>> > --
>> > Gustavo Sousa
>> >
>> > > for (i = 0; i < 4; i++) {
>> > > int tx = i % 2 + 1;
>> > > u8 lane_mask = i < 2 ? INTEL_CX0_LANE0 :
>> > > INTEL_CX0_LANE1;
>> > > @@ -3256,8 +3256,7 @@ void intel_lnl_mac_transmit_lfps(struct
>> > > intel_encoder *encoder,
>> > >
>> > > intel_cx0_rmw(encoder, lane_mask,
>> > > PHY_CMN1_CONTROL(tx, 0),
>> > > CONTROL0_MAC_TRANSMIT_LFPS,
>> > > - enable ?
>> > > CONTROL0_MAC_TRANSMIT_LFPS
>> > > : 0,
>> > > - MB_WRITE_COMMITTED);
>> > > + CONTROL0_MAC_TRANSMIT_LFPS,
>> > > MB_WRITE_COMMITTED);
>> > > }
>> > > }
>> > >
>> > > --
>> > > 2.43.0
>> > >
>>
>
