Re: [PATCH] drm/i915/display: Write PHY_CMN1_CONTROL only when using AUXLess ALPM

["Hogander, Jouni" <jouni.hogander@intel.com>]

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.hogander@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
> > > > > > > 
> > > > > 
> > > > 
> >