diff mbox series

[1/2] drm/i915: Fix fastset vs. pfit on/off on HSW EDP transcoder

Message ID 20190425162906.5242-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Fix fastset vs. pfit on/off on HSW EDP transcoder | expand

Commit Message

Ville Syrjälä April 25, 2019, 4:29 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On HSW the pipe A panel fitter lives inside the display power well,
and the input MUX for the EDP transcoder needs to be configured
appropriately to route the data through the power well as needed.
Changing the MUX setting is not allowed while the pipe is active,
so we need to force a full modeset whenever we need to change it.

Currently we may end up doing a fastset which won't change the
MUX settings, but it will drop the power well reference, and that
kills the pipe.

Cc: stable@vger.kernel.org
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  9 +++++++++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 13 ++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä May 7, 2019, 6:33 p.m. UTC | #1 
On Thu, Apr 25, 2019 at 07:29:05PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On HSW the pipe A panel fitter lives inside the display power well,
> and the input MUX for the EDP transcoder needs to be configured
> appropriately to route the data through the power well as needed.
> Changing the MUX setting is not allowed while the pipe is active,
> so we need to force a full modeset whenever we need to change it.
> 
> Currently we may end up doing a fastset which won't change the
> MUX settings, but it will drop the power well reference, and that
> kills the pipe.
> 
> Cc: stable@vger.kernel.org
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Probably
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104838

and maybe
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108672

> ---
>  drivers/gpu/drm/i915/intel_display.c  |  9 +++++++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 13 ++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c67f165b466c..691c9a929164 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12133,6 +12133,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  			  struct intel_crtc_state *pipe_config,
>  			  bool adjust)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
>  	bool ret = true;
>  	bool fixup_inherited = adjust &&
>  		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> @@ -12354,6 +12355,14 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		PIPE_CONF_CHECK_X(gmch_pfit.pgm_ratios);
>  	PIPE_CONF_CHECK_X(gmch_pfit.lvds_border_bits);
>  
> +	/*
> +	 * Changing the EDP transcoder input mux
> +	 * (A_ONOFF vs. A_ON) requires a full modeset.
> +	 */
> +	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A &&
> +	    current_config->cpu_transcoder == TRANSCODER_EDP)
> +		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);
> +
>  	if (!adjust) {
>  		PIPE_CONF_CHECK_I(pipe_src_w);
>  		PIPE_CONF_CHECK_I(pipe_src_h);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index e94b5b1bc1b7..e7c7be4911c1 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -311,10 +311,17 @@ intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
>  	pipe_config->base.mode_changed = pipe_config->has_psr;
>  	pipe_config->crc_enabled = enable;
>  
> -	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> +	if (IS_HASWELL(dev_priv) &&
> +	    pipe_config->base.active && crtc->pipe == PIPE_A &&
> +	    pipe_config->cpu_transcoder == TRANSCODER_EDP) {
> +		bool old_need_power_well = pipe_config->pch_pfit.enabled ||
> +			pipe_config->pch_pfit.force_thru;
> +		bool new_need_power_well = pipe_config->pch_pfit.enabled ||
> +			enable;
> +
>  		pipe_config->pch_pfit.force_thru = enable;
> -		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -		    pipe_config->pch_pfit.enabled != enable)
> +
> +		if (old_need_power_well != new_need_power_well)
>  			pipe_config->base.connectors_changed = true;
>  	}
>  
> -- 
> 2.21.0
Maarten Lankhorst May 8, 2019, 10:46 a.m. UTC | #2 
Op 25-04-2019 om 18:29 schreef Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On HSW the pipe A panel fitter lives inside the display power well,
> and the input MUX for the EDP transcoder needs to be configured
> appropriately to route the data through the power well as needed.
> Changing the MUX setting is not allowed while the pipe is active,
> so we need to force a full modeset whenever we need to change it.
>
> Currently we may end up doing a fastset which won't change the
> MUX settings, but it will drop the power well reference, and that
> kills the pipe.
>
> Cc: stable@vger.kernel.org
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  9 +++++++++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 13 ++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c67f165b466c..691c9a929164 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12133,6 +12133,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  			  struct intel_crtc_state *pipe_config,
>  			  bool adjust)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
>  	bool ret = true;
>  	bool fixup_inherited = adjust &&
>  		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> @@ -12354,6 +12355,14 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  		PIPE_CONF_CHECK_X(gmch_pfit.pgm_ratios);
>  	PIPE_CONF_CHECK_X(gmch_pfit.lvds_border_bits);
>  
> +	/*
> +	 * Changing the EDP transcoder input mux
> +	 * (A_ONOFF vs. A_ON) requires a full modeset.
> +	 */
> +	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A &&
> +	    current_config->cpu_transcoder == TRANSCODER_EDP)
> +		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);

I guess it depends if we want to make it a blocker or not..

> +
>  	if (!adjust) {
>  		PIPE_CONF_CHECK_I(pipe_src_w);
>  		PIPE_CONF_CHECK_I(pipe_src_h);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index e94b5b1bc1b7..e7c7be4911c1 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -311,10 +311,17 @@ intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
>  	pipe_config->base.mode_changed = pipe_config->has_psr;
>  	pipe_config->crc_enabled = enable;
>  
> -	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> +	if (IS_HASWELL(dev_priv) &&
> +	    pipe_config->base.active && crtc->pipe == PIPE_A &&
> +	    pipe_config->cpu_transcoder == TRANSCODER_EDP) {
> +		bool old_need_power_well = pipe_config->pch_pfit.enabled ||
> +			pipe_config->pch_pfit.force_thru;
> +		bool new_need_power_well = pipe_config->pch_pfit.enabled ||
> +			enable;
> +
>  		pipe_config->pch_pfit.force_thru = enable;
> -		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -		    pipe_config->pch_pfit.enabled != enable)
> +
> +		if (old_need_power_well != new_need_power_well)
>  			pipe_config->base.connectors_changed = true;

Could we get rid of this logic and set mode_changed instead?

Ah, I see that is done in 2/2, much less surprises then. :)

In that case, for both:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Ville Syrjälä May 9, 2019, 1:03 p.m. UTC | #3 
On Wed, May 08, 2019 at 12:46:09PM +0200, Maarten Lankhorst wrote:
> Op 25-04-2019 om 18:29 schreef Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On HSW the pipe A panel fitter lives inside the display power well,
> > and the input MUX for the EDP transcoder needs to be configured
> > appropriately to route the data through the power well as needed.
> > Changing the MUX setting is not allowed while the pipe is active,
> > so we need to force a full modeset whenever we need to change it.
> >
> > Currently we may end up doing a fastset which won't change the
> > MUX settings, but it will drop the power well reference, and that
> > kills the pipe.
> >
> > Cc: stable@vger.kernel.org
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  |  9 +++++++++
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 13 ++++++++++---
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c67f165b466c..691c9a929164 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12133,6 +12133,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  			  struct intel_crtc_state *pipe_config,
> >  			  bool adjust)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
> >  	bool ret = true;
> >  	bool fixup_inherited = adjust &&
> >  		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> > @@ -12354,6 +12355,14 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  		PIPE_CONF_CHECK_X(gmch_pfit.pgm_ratios);
> >  	PIPE_CONF_CHECK_X(gmch_pfit.lvds_border_bits);
> >  
> > +	/*
> > +	 * Changing the EDP transcoder input mux
> > +	 * (A_ONOFF vs. A_ON) requires a full modeset.
> > +	 */
> > +	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A &&
> > +	    current_config->cpu_transcoder == TRANSCODER_EDP)
> > +		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);
> 
> I guess it depends if we want to make it a blocker or not..
> 
> > +
> >  	if (!adjust) {
> >  		PIPE_CONF_CHECK_I(pipe_src_w);
> >  		PIPE_CONF_CHECK_I(pipe_src_h);
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index e94b5b1bc1b7..e7c7be4911c1 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -311,10 +311,17 @@ intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
> >  	pipe_config->base.mode_changed = pipe_config->has_psr;
> >  	pipe_config->crc_enabled = enable;
> >  
> > -	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> > +	if (IS_HASWELL(dev_priv) &&
> > +	    pipe_config->base.active && crtc->pipe == PIPE_A &&
> > +	    pipe_config->cpu_transcoder == TRANSCODER_EDP) {
> > +		bool old_need_power_well = pipe_config->pch_pfit.enabled ||
> > +			pipe_config->pch_pfit.force_thru;
> > +		bool new_need_power_well = pipe_config->pch_pfit.enabled ||
> > +			enable;
> > +
> >  		pipe_config->pch_pfit.force_thru = enable;
> > -		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> > -		    pipe_config->pch_pfit.enabled != enable)
> > +
> > +		if (old_need_power_well != new_need_power_well)
> >  			pipe_config->base.connectors_changed = true;
> 
> Could we get rid of this logic and set mode_changed instead?
> 
> Ah, I see that is done in 2/2, much less surprises then. :)

Yeah, wanted to keep the fix itself minimal for backporting.

> 
> In that case, for both:
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks. Series pushed.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c67f165b466c..691c9a929164 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12133,6 +12133,7 @@  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 			  struct intel_crtc_state *pipe_config,
 			  bool adjust)
 {
+	struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
 	bool ret = true;
 	bool fixup_inherited = adjust &&
 		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
@@ -12354,6 +12355,14 @@  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 		PIPE_CONF_CHECK_X(gmch_pfit.pgm_ratios);
 	PIPE_CONF_CHECK_X(gmch_pfit.lvds_border_bits);
 
+	/*
+	 * Changing the EDP transcoder input mux
+	 * (A_ONOFF vs. A_ON) requires a full modeset.
+	 */
+	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A &&
+	    current_config->cpu_transcoder == TRANSCODER_EDP)
+		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);
+
 	if (!adjust) {
 		PIPE_CONF_CHECK_I(pipe_src_w);
 		PIPE_CONF_CHECK_I(pipe_src_h);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index e94b5b1bc1b7..e7c7be4911c1 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -311,10 +311,17 @@  intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
 	pipe_config->base.mode_changed = pipe_config->has_psr;
 	pipe_config->crc_enabled = enable;
 
-	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
+	if (IS_HASWELL(dev_priv) &&
+	    pipe_config->base.active && crtc->pipe == PIPE_A &&
+	    pipe_config->cpu_transcoder == TRANSCODER_EDP) {
+		bool old_need_power_well = pipe_config->pch_pfit.enabled ||
+			pipe_config->pch_pfit.force_thru;
+		bool new_need_power_well = pipe_config->pch_pfit.enabled ||
+			enable;
+
 		pipe_config->pch_pfit.force_thru = enable;
-		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-		    pipe_config->pch_pfit.enabled != enable)
+
+		if (old_need_power_well != new_need_power_well)
 			pipe_config->base.connectors_changed = true;
 	}