diff mbox series

[6/7] drm/i915/display/tgl: Fix the order of the step to turn transcoder clock off

Message ID 20191123005459.155383-6-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/7] drm/i915/display: Refactor intel_commit_modeset_disables() | expand

Commit Message

Souza, Jose Nov. 23, 2019, 12:54 a.m. UTC 
For TGL the step to turn off the transcoder clock was moved to after
the complete shutdown of DDI. Only the MST slave transcoders should
disable the clock before that.

BSpec: 49190
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c    |  9 ++++++++-
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Nov. 28, 2019, 6:40 p.m. UTC | #1 
On Fri, Nov 22, 2019 at 04:54:58PM -0800, José Roberto de Souza wrote:
> For TGL the step to turn off the transcoder clock was moved to after
> the complete shutdown of DDI. Only the MST slave transcoders should
> disable the clock before that.
> 
> BSpec: 49190
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c    |  9 ++++++++-
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index cfcaa7c81575..aa0249333175 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3813,7 +3813,7 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
>  	 */
>  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>  
> -	if (!is_mst)
> +	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
>  		intel_ddi_disable_pipe_clock(old_crtc_state);
>  
>  	intel_disable_ddi_buf(encoder, old_crtc_state);
> @@ -3826,6 +3826,13 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
>  		intel_display_power_put_unchecked(dev_priv,
>  						  dig_port->ddi_io_power_domain);
>  
> +	/*
> +	 * From TGL BSpec "If single stream or multi-stream master transcoder:
> +	 * Configure Transcoder Clock select to direct no clock to the
> +	 * transcoder"
> +	 */

Not really convinced these comments add anything the code isn't already saying.

> +	if (INTEL_GEN(dev_priv) >= 12)
> +		intel_ddi_disable_pipe_clock(old_crtc_state);

That's much later than the bspec sequence suggests.

>  	intel_ddi_clk_disable(encoder);
>  	tgl_clear_psr2_transcoder_exitline(old_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 94549848653a..53afe3e179f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -369,8 +369,19 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct intel_connector *connector =
>  		to_intel_connector(old_conn_state->connector);
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  
> -	intel_ddi_disable_pipe_clock(old_crtc_state);
> +	intel_dp->active_mst_links--;
> +
> +	/*
> +	 * From TGL BSpec "If multi-stream slave transcoder: Configure
> +	 * Transcoder Clock Select to direct no clock to the transcoder"
> +	 *
> +	 * From older GENs BSpec "Configure Transcoder Clock Select to direct
> +	 * no clock to the transcoder"
> +	 */
> +	if (INTEL_GEN(dev_priv) < 12 || intel_dp->active_mst_links)

Maybe we should add 'last_mst_stream' to mirror the 'first_mst_stream'
in the enable code?

> +		intel_ddi_disable_pipe_clock(old_crtc_state);
>  
>  	/* this can fail */
>  	drm_dp_check_act_status(&intel_dp->mst_mgr);
> @@ -386,8 +397,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
>  				     false);
>  
> -	intel_dp->active_mst_links--;
> -
>  	intel_mst->connector = NULL;
>  	if (intel_dp->active_mst_links == 0) {
>  		intel_dig_port->base.post_disable(&intel_dig_port->base,
> -- 
> 2.24.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Dec. 3, 2019, 11:29 p.m. UTC | #2 
On Thu, 2019-11-28 at 20:40 +0200, Ville Syrjälä wrote:
> On Fri, Nov 22, 2019 at 04:54:58PM -0800, José Roberto de Souza
> wrote:
> > For TGL the step to turn off the transcoder clock was moved to
> > after
> > the complete shutdown of DDI. Only the MST slave transcoders should
> > disable the clock before that.
> > 
> > BSpec: 49190
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c    |  9 ++++++++-
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index cfcaa7c81575..aa0249333175 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3813,7 +3813,7 @@ static void intel_ddi_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	 */
> >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  
> > -	if (!is_mst)
> > +	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
> >  		intel_ddi_disable_pipe_clock(old_crtc_state);
> >  
> >  	intel_disable_ddi_buf(encoder, old_crtc_state);
> > @@ -3826,6 +3826,13 @@ static void intel_ddi_post_disable_dp(struct
> > intel_encoder *encoder,
> >  		intel_display_power_put_unchecked(dev_priv,
> >  						  dig_port-
> > >ddi_io_power_domain);
> >  
> > +	/*
> > +	 * From TGL BSpec "If single stream or multi-stream master
> > transcoder:
> > +	 * Configure Transcoder Clock select to direct no clock to the
> > +	 * transcoder"
> > +	 */
> 
> Not really convinced these comments add anything the code isn't
> already saying.

We have added similar comments in tgl_ddi_pre_enable_dp(), it helps
tracks the code with BSpec steps.

> 
> > +	if (INTEL_GEN(dev_priv) >= 12)
> > +		intel_ddi_disable_pipe_clock(old_crtc_state);
> 
> That's much later than the bspec sequence suggests.

Oh, thanks.
Yeah it should right after intel_disable_ddi_buf()

> 
> >  	intel_ddi_clk_disable(encoder);
> >  	tgl_clear_psr2_transcoder_exitline(old_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 94549848653a..53afe3e179f7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -369,8 +369,19 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  	struct intel_connector *connector =
> >  		to_intel_connector(old_conn_state->connector);
> > +	struct drm_i915_private *dev_priv = to_i915(connector-
> > >base.dev);
> >  
> > -	intel_ddi_disable_pipe_clock(old_crtc_state);
> > +	intel_dp->active_mst_links--;
> > +
> > +	/*
> > +	 * From TGL BSpec "If multi-stream slave transcoder: Configure
> > +	 * Transcoder Clock Select to direct no clock to the
> > transcoder"
> > +	 *
> > +	 * From older GENs BSpec "Configure Transcoder Clock Select to
> > direct
> > +	 * no clock to the transcoder"
> > +	 */
> > +	if (INTEL_GEN(dev_priv) < 12 || intel_dp->active_mst_links)
> 
> Maybe we should add 'last_mst_stream' to mirror the
> 'first_mst_stream'
> in the enable code?

Sounds good

> 
> > +		intel_ddi_disable_pipe_clock(old_crtc_state);
> >  
> >  	/* this can fail */
> >  	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > @@ -386,8 +397,6 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > >port,
> >  				     false);
> >  
> > -	intel_dp->active_mst_links--;
> > -
> >  	intel_mst->connector = NULL;
> >  	if (intel_dp->active_mst_links == 0) {
> >  		intel_dig_port->base.post_disable(&intel_dig_port-
> > >base,
> > -- 
> > 2.24.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Dec. 4, 2019, 10:56 a.m. UTC | #3 
On Tue, Dec 03, 2019 at 11:29:49PM +0000, Souza, Jose wrote:
> On Thu, 2019-11-28 at 20:40 +0200, Ville Syrjälä wrote:
> > On Fri, Nov 22, 2019 at 04:54:58PM -0800, José Roberto de Souza
> > wrote:
> > > For TGL the step to turn off the transcoder clock was moved to
> > > after
> > > the complete shutdown of DDI. Only the MST slave transcoders should
> > > disable the clock before that.
> > > 
> > > BSpec: 49190
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c    |  9 ++++++++-
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
> > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index cfcaa7c81575..aa0249333175 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3813,7 +3813,7 @@ static void intel_ddi_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	 */
> > >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > >  
> > > -	if (!is_mst)
> > > +	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
> > >  		intel_ddi_disable_pipe_clock(old_crtc_state);
> > >  
> > >  	intel_disable_ddi_buf(encoder, old_crtc_state);
> > > @@ -3826,6 +3826,13 @@ static void intel_ddi_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  		intel_display_power_put_unchecked(dev_priv,
> > >  						  dig_port-
> > > >ddi_io_power_domain);
> > >  
> > > +	/*
> > > +	 * From TGL BSpec "If single stream or multi-stream master
> > > transcoder:
> > > +	 * Configure Transcoder Clock select to direct no clock to the
> > > +	 * transcoder"
> > > +	 */
> > 
> > Not really convinced these comments add anything the code isn't
> > already saying.
> 
> We have added similar comments in tgl_ddi_pre_enable_dp(), it helps
> tracks the code with BSpec steps.

Just noise IMO. If the code isn't self explanatory then it should be
fixed instead.

> 
> > 
> > > +	if (INTEL_GEN(dev_priv) >= 12)
> > > +		intel_ddi_disable_pipe_clock(old_crtc_state);
> > 
> > That's much later than the bspec sequence suggests.
> 
> Oh, thanks.
> Yeah it should right after intel_disable_ddi_buf()
> 
> > 
> > >  	intel_ddi_clk_disable(encoder);
> > >  	tgl_clear_psr2_transcoder_exitline(old_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 94549848653a..53afe3e179f7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -369,8 +369,19 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > >  	struct intel_connector *connector =
> > >  		to_intel_connector(old_conn_state->connector);
> > > +	struct drm_i915_private *dev_priv = to_i915(connector-
> > > >base.dev);
> > >  
> > > -	intel_ddi_disable_pipe_clock(old_crtc_state);
> > > +	intel_dp->active_mst_links--;
> > > +
> > > +	/*
> > > +	 * From TGL BSpec "If multi-stream slave transcoder: Configure
> > > +	 * Transcoder Clock Select to direct no clock to the
> > > transcoder"
> > > +	 *
> > > +	 * From older GENs BSpec "Configure Transcoder Clock Select to
> > > direct
> > > +	 * no clock to the transcoder"
> > > +	 */
> > > +	if (INTEL_GEN(dev_priv) < 12 || intel_dp->active_mst_links)
> > 
> > Maybe we should add 'last_mst_stream' to mirror the
> > 'first_mst_stream'
> > in the enable code?
> 
> Sounds good
> 
> > 
> > > +		intel_ddi_disable_pipe_clock(old_crtc_state);
> > >  
> > >  	/* this can fail */
> > >  	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > > @@ -386,8 +397,6 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > > >port,
> > >  				     false);
> > >  
> > > -	intel_dp->active_mst_links--;
> > > -
> > >  	intel_mst->connector = NULL;
> > >  	if (intel_dp->active_mst_links == 0) {
> > >  		intel_dig_port->base.post_disable(&intel_dig_port-
> > > >base,
> > > -- 
> > > 2.24.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index cfcaa7c81575..aa0249333175 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3813,7 +3813,7 @@  static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 	 */
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 
-	if (!is_mst)
+	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
 		intel_ddi_disable_pipe_clock(old_crtc_state);
 
 	intel_disable_ddi_buf(encoder, old_crtc_state);
@@ -3826,6 +3826,13 @@  static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 		intel_display_power_put_unchecked(dev_priv,
 						  dig_port->ddi_io_power_domain);
 
+	/*
+	 * From TGL BSpec "If single stream or multi-stream master transcoder:
+	 * Configure Transcoder Clock select to direct no clock to the
+	 * transcoder"
+	 */
+	if (INTEL_GEN(dev_priv) >= 12)
+		intel_ddi_disable_pipe_clock(old_crtc_state);
 	intel_ddi_clk_disable(encoder);
 	tgl_clear_psr2_transcoder_exitline(old_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 94549848653a..53afe3e179f7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -369,8 +369,19 @@  static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(old_conn_state->connector);
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 
-	intel_ddi_disable_pipe_clock(old_crtc_state);
+	intel_dp->active_mst_links--;
+
+	/*
+	 * From TGL BSpec "If multi-stream slave transcoder: Configure
+	 * Transcoder Clock Select to direct no clock to the transcoder"
+	 *
+	 * From older GENs BSpec "Configure Transcoder Clock Select to direct
+	 * no clock to the transcoder"
+	 */
+	if (INTEL_GEN(dev_priv) < 12 || intel_dp->active_mst_links)
+		intel_ddi_disable_pipe_clock(old_crtc_state);
 
 	/* this can fail */
 	drm_dp_check_act_status(&intel_dp->mst_mgr);
@@ -386,8 +397,6 @@  static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
 				     false);
 
-	intel_dp->active_mst_links--;
-
 	intel_mst->connector = NULL;
 	if (intel_dp->active_mst_links == 0) {
 		intel_dig_port->base.post_disable(&intel_dig_port->base,