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 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:

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?

> 
> 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
> > > 
>