| Message ID | 20241030192313.4030617-2-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 PTL 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. > > This fixes a slave transcoder getting stuck during enabling, leading > to page flip timeout errors on the corresponding pipe. > > The spec requires the same programming step for ADLP+ platforms, that > will be addressed separately (on those platforms the above transcoder > getting stuck issue was not observed). > > Bspec: 68849 > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- > drivers/gpu/drm/i915/display/intel_ddi.h | 2 ++ > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +++ > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 2bd14e2134be9..069cca4b38b2e 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -619,7 +619,7 @@ void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, > * Same as intel_ddi_enable_transcoder_func(), but it does not set the enable > * bit. > */ > -static void > +void > intel_ddi_config_transcoder_func(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state) The void fits on the same line, and I think it looks better? > { > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h > index 6d85422bdefef..1aa2e3a190aee 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.h > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h > @@ -57,6 +57,8 @@ void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > void intel_ddi_init(struct intel_display *display, > const struct intel_bios_encoder_data *devdata); > bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); > +void intel_ddi_config_transcoder_func(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state); > void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state); > void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state); > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 1a2ff3e1cb68f..7c16406883594 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -1224,6 +1224,9 @@ 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) > + intel_ddi_config_transcoder_func(encoder, pipe_config); > + > intel_dsc_dp_pps_write(&dig_port->base, pipe_config); > intel_ddi_set_dp_msa(pipe_config, conn_state); > } I don't see the part where you changed the code to _not_ enable this, since you're calling intel_ddi_config_transcoder_func() here now... Did I miss something? -- Cheers, Luca.
On Thu, Oct 31, 2024 at 12:44:38PM +0200, Luca Coelho wrote: > On Wed, 2024-10-30 at 21:23 +0200, Imre Deak wrote: > > On PTL 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. > > > > This fixes a slave transcoder getting stuck during enabling, leading > > to page flip timeout errors on the corresponding pipe. > > > > The spec requires the same programming step for ADLP+ platforms, that > > will be addressed separately (on those platforms the above transcoder > > getting stuck issue was not observed). > > > > Bspec: 68849 > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- > > drivers/gpu/drm/i915/display/intel_ddi.h | 2 ++ > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +++ > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 2bd14e2134be9..069cca4b38b2e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -619,7 +619,7 @@ void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, > > * Same as intel_ddi_enable_transcoder_func(), but it does not set the enable > > * bit. > > */ > > -static void > > +void > > intel_ddi_config_transcoder_func(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state) > > The void fits on the same line, and I think it looks better? Ok, will change that. > > { > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h > > index 6d85422bdefef..1aa2e3a190aee 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.h > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h > > @@ -57,6 +57,8 @@ void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > > void intel_ddi_init(struct intel_display *display, > > const struct intel_bios_encoder_data *devdata); > > bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); > > +void intel_ddi_config_transcoder_func(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state); > > void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state); > > void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state); > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index 1a2ff3e1cb68f..7c16406883594 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -1224,6 +1224,9 @@ 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) > > + intel_ddi_config_transcoder_func(encoder, pipe_config); > > + > > intel_dsc_dp_pps_write(&dig_port->base, pipe_config); > > intel_ddi_set_dp_msa(pipe_config, conn_state); > > } > > I don't see the part where you changed the code to _not_ enable this, > since you're calling intel_ddi_config_transcoder_func() here now... > > Did I miss something? This only _configures_ the DDI function - which was missing so far - the enabling and disabling happens separately later, in intel_mst_enable_dp() / intel_mst_post_disable_dp() - which are done already as expected. > -- > Cheers, > Luca.
On Thu, 2024-10-31 at 12:57 +0200, Imre Deak wrote: > On Thu, Oct 31, 2024 at 12:44:38PM +0200, Luca Coelho wrote: > > On Wed, 2024-10-30 at 21:23 +0200, Imre Deak wrote: > > > On PTL 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. > > > > > > This fixes a slave transcoder getting stuck during enabling, leading > > > to page flip timeout errors on the corresponding pipe. > > > > > > The spec requires the same programming step for ADLP+ platforms, that > > > will be addressed separately (on those platforms the above transcoder > > > getting stuck issue was not observed). > > > > > > Bspec: 68849 > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_ddi.h | 2 ++ > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +++ > > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index 2bd14e2134be9..069cca4b38b2e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -619,7 +619,7 @@ void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, > > > * Same as intel_ddi_enable_transcoder_func(), but it does not set the enable > > > * bit. > > > */ > > > -static void > > > +void > > > intel_ddi_config_transcoder_func(struct intel_encoder *encoder, > > > const struct intel_crtc_state *crtc_state) > > > > The void fits on the same line, and I think it looks better? > > Ok, will change that. > > > > { > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h > > > index 6d85422bdefef..1aa2e3a190aee 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.h > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h > > > @@ -57,6 +57,8 @@ void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > > > void intel_ddi_init(struct intel_display *display, > > > const struct intel_bios_encoder_data *devdata); > > > bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); > > > +void intel_ddi_config_transcoder_func(struct intel_encoder *encoder, > > > + const struct intel_crtc_state *crtc_state); > > > void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, > > > const struct intel_crtc_state *crtc_state); > > > void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state); > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index 1a2ff3e1cb68f..7c16406883594 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -1224,6 +1224,9 @@ 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) > > > + intel_ddi_config_transcoder_func(encoder, pipe_config); > > > + > > > intel_dsc_dp_pps_write(&dig_port->base, pipe_config); > > > intel_ddi_set_dp_msa(pipe_config, conn_state); > > > } > > > > I don't see the part where you changed the code to _not_ enable this, > > since you're calling intel_ddi_config_transcoder_func() here now... > > > > Did I miss something? > > This only _configures_ the DDI function - which was missing so far - the > enabling and disabling happens separately later, in > intel_mst_enable_dp() / intel_mst_post_disable_dp() - which are done > already as expected. Okay, that explains it. Thanks. Reviewed-by: Luca Coelho <luciano.coelho@intel.com> -- Cheers, Luca.
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 2bd14e2134be9..069cca4b38b2e 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -619,7 +619,7 @@ void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, * Same as intel_ddi_enable_transcoder_func(), but it does not set the enable * bit. */ -static void +void intel_ddi_config_transcoder_func(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state) { diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h index 6d85422bdefef..1aa2e3a190aee 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.h +++ b/drivers/gpu/drm/i915/display/intel_ddi.h @@ -57,6 +57,8 @@ void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, void intel_ddi_init(struct intel_display *display, const struct intel_bios_encoder_data *devdata); bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); +void intel_ddi_config_transcoder_func(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state); void intel_ddi_enable_transcoder_func(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state); void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state); diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 1a2ff3e1cb68f..7c16406883594 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -1224,6 +1224,9 @@ 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) + intel_ddi_config_transcoder_func(encoder, pipe_config); + intel_dsc_dp_pps_write(&dig_port->base, pipe_config); intel_ddi_set_dp_msa(pipe_config, conn_state); }
On PTL 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. This fixes a slave transcoder getting stuck during enabling, leading to page flip timeout errors on the corresponding pipe. The spec requires the same programming step for ADLP+ platforms, that will be addressed separately (on those platforms the above transcoder getting stuck issue was not observed). Bspec: 68849 Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- drivers/gpu/drm/i915/display/intel_ddi.h | 2 ++ drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +++ 3 files changed, 6 insertions(+), 1 deletion(-)