On Fri, 2025-07-18 at 10:45 -0300, Gustavo Sousa wrote:
> Quoting Hogander, Jouni (2025-07-18 10:08:52-03:00)
> > On Fri, 2025-07-18 at 09:58 -0300, Gustavo Sousa wrote:
> > > 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 can reproduce this issue only with setup having two 4k monitors
> > in DP
> > ports and then one eDP panel. The issue is triggered on DP port.
> > I.e.
> > something that doesn't have AUX-Less ALPM. As it's clearly
> > unnecessary
> > to write this register on that failing port and not mentioned in
> > HAS I
> > was thinking just removing the failing write would work in this
> > 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.
> >
> > Configuring this is a own step listed in HAS (6.k.ii). I can try if
> > this works. I have no idea if it's ok to move it.
>
> Hm... Well, not sure. Reading the spec on MacTransmitLFPS field, it
> appears that this should be programmed after lane reset sequence is
> done. By the time we call intel_c10_pll_program(), the lane reset
> step
> is already done.
>
> Another thing I think we should be doing, if we want to keep this as
> a
> separate step and be aligned with Bspec, is to set
> C10_VDR_CTRL_MSGBUS_ACCESS, just like it is done for other C10
> progamming in the code. That is a requirement for programming C10 via
> msg bus.
Tested this as well and still I'm seeing this:
[ 118.454525] xe 0000:00:02.0: [drm] *ERROR* PHY I Read 0800 failed
after 3 retries.
I will add your suggestions as a separate patches to my set but I will
keep the patch avoiding lfps configuration when it's not needed.
BR,
Jouni Högander
>
> --
> Gustavo Sousa
>
> >
> > >
> > > >
> > > > 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
> > > > > > >
> > > > >
> > > >
> >
