Re: [PATCH v2 2/5] drm/i915/adlp+/dp_mst: Align slave transcoder enabling with spec wrt. DDI function

Luca Coelho
2024-11-06T12:40:24Z

On Thu, 2024-10-31 at 13:40 +0200, Imre Deak wrote:
> On Thu, Oct 31, 2024 at 12:49:22PM +0200, Luca Coelho wrote:
> > On Wed, 2024-10-30 at 21:23 +0200, Imre Deak wrote:
> > > On ADLP+ during modeset enabling configure the DDI function without
> > > enabling it for MST slave transcoders before programming the data and
> > > link M/N values. The DDI function gets enabled separately later in the
> > > transcoder enabling sequence.
> > >
> > > Align the code with the spec based on the above.
> > >
> > > v2: Move this patch earlier in the series, addressing the DP2
> > > config fixes for all ADLP+ platforms later.
> > >
> > > Bspec: 55424, 54128, 65448, 68849
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index 7c16406883594..bf264bd1881b5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -1224,7 +1224,7 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
> > > if (DISPLAY_VER(dev_priv) < 12 || !first_mst_stream)
> > > intel_ddi_enable_transcoder_clock(encoder, pipe_config);
> > >
> > > - if (DISPLAY_VER(dev_priv) >= 30 && !first_mst_stream)
> > > + if (DISPLAY_VER(dev_priv) >= 13 && !first_mst_stream)
> > > intel_ddi_config_transcoder_func(encoder, pipe_config);
> > >
> > > intel_dsc_dp_pps_write(&dig_port->base, pipe_config);
> >
> > You are essentially modifying the first patch in the series here, is it
> > because you want to keep the ADLP+ change separate?
>
> Yes, the first patch clearly fixes an issue on PTL. On other ADLP+
> platforms this (and the follow-up DP2 config changes) is just what bspec
> says to do, the lack of which never having caused a problem. Based on
> all this I wanted to keep the PTL fix separate and first, making it
> easier to revert any of the other changes if they caused a problem
> (turning out that the spec was incorrect).
>
> > In that case, wouldn't it be better to do this first so the ADLP+
> > change could be more easily backported?
>
> If that would be required - I'd only backport if there was an evidence
> that it fixes things - I'd either backport it along with the first patch
> as a dependency, or just send this one patch separately rebased on the
> stable trees (which would be the first two patches combined).
>
> > IMHO it's better to first fix stuff for older HW that is not correct
> > and then add new changes for new HW on top of that.
>
> Agreed for actual fixes, but in this case the only clear fix is the
> first one, the rest being just alignment with the spec (and as such
> better kept them easy to revert).

Fair enough. Sorry for not responding before, for some reason it seems
that I didn't get this email in my inbox, only in the list.

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.