| Message ID | 20241030192313.4030617-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/dp_mst: Fix DDI function/DP2 config programming | expand |
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? In that case, wouldn't it be better to do this first so the ADLP+ change could be more easily backported? 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. -- Cheers, Luca.
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). > -- > Cheers, > Luca.
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.
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);
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(-)