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

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