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

[Gustavo Sousa <gustavo.sousa@intel.com>]

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