diff mbox

drm/i915: Fix all pipe->plane mappings before sanitizing crtc

Message ID 1421135983-1581-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ander Conselvan de Oliveira Jan. 13, 2015, 7:59 a.m. UTC
If QUIRK_PIPEA_FORCE is necessary, intel_sanitize_crtc() might trigger
a mode set. In that case, if pipe A is disabled and pipe B is mapped to
plane B, that mode set happens before the mapping is fixed. Due to the
wrong state, the call to disable pipe B disables plane A (which is
already disabled) and later an assertion for plane B being enabled
(while it should have been disabled) is triggered.

References: https://bugs.freedesktop.org/show_bug.cgi?id=72782
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 50 +++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Comments

Shuang He Jan. 13, 2015, 3:53 p.m. UTC | #1
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  354/354              354/354
ILK                                  201/201              201/201
SNB              +1-1              401/424              401/424
IVB                                  488/488              488/488
BYT                                  278/278              278/278
HSW                                  529/529              529/529
BDW                 -1              405/405              404/405
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt_kms_flip_flip-vs-dpms-off-vs-modeset-interruptible      NSPT(1, M35)PASS(10, M35M22)      PASS(1, M35)
*SNB  igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible      PASS(10, M35M22)      DMESG_WARN(1, M35)
*BDW  igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible      PASS(9, M30M28)      DMESG_WARN(1, M30)
Note: You need to pay more attention to line start with '*'
Jani Nikula Jan. 14, 2015, 10:47 a.m. UTC | #2
On Tue, 13 Jan 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> If QUIRK_PIPEA_FORCE is necessary, intel_sanitize_crtc() might trigger
> a mode set. In that case, if pipe A is disabled and pipe B is mapped to
> plane B, that mode set happens before the mapping is fixed. Due to the
> wrong state, the call to disable pipe B disables plane A (which is
> already disabled) and later an assertion for plane B being enabled
> (while it should have been disabled) is triggered.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=72782

If the patch is expected to fix the bug, please use Bugzilla:. If you're
referencing a bug because it's otherwise relevant, please use
References:.

Which one is this?

BR,
Jani.


> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 50 +++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d01db1b..cf87528 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13127,30 +13127,22 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>  	return true;
>  }
>  
> -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> +static void intel_fix_plane_mapping(struct drm_device *dev)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 reg;
> -
> -	/* Clear any frame start delays used for debugging left by the BIOS */
> -	reg = PIPECONF(crtc->config.cpu_transcoder);
> -	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *crtc;
>  
> -	/* restore vblank interrupts to correct state */
> -	if (crtc->active) {
> -		update_scanline_offset(crtc);
> -		drm_vblank_on(dev, crtc->pipe);
> -	} else
> -		drm_vblank_off(dev, crtc->pipe);
> +	/* gen4+ has a fixed plane -> pipe mapping.*/
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		return;
>  
> -	/* We need to sanitize the plane -> pipe mapping first because this will
> -	 * disable the crtc (and hence change the state) if it is wrong. Note
> -	 * that gen4+ has a fixed plane -> pipe mapping.  */
> -	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
> +	for_each_intel_crtc(dev, crtc) {
>  		struct intel_connector *connector;
>  		bool plane;
>  
> +		if (intel_check_plane_mapping(crtc))
> +			continue;
> +
>  		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
>  			      crtc->base.base.id);
>  
> @@ -13184,6 +13176,24 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		WARN_ON(crtc->active);
>  		crtc->base.enabled = false;
>  	}
> +}
> +
> +static void intel_sanitize_crtc(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 reg;
> +
> +	/* Clear any frame start delays used for debugging left by the BIOS */
> +	reg = PIPECONF(crtc->config.cpu_transcoder);
> +	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
> +
> +	/* restore vblank interrupts to correct state */
> +	if (crtc->active) {
> +		update_scanline_offset(crtc);
> +		drm_vblank_on(dev, crtc->pipe);
> +	} else
> +		drm_vblank_off(dev, crtc->pipe);
>  
>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
>  	    crtc->pipe == PIPE_A && !crtc->active) {
> @@ -13442,6 +13452,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		intel_sanitize_encoder(encoder);
>  	}
>  
> +	/* We need to sanitize the plane -> pipe mapping first because this will
> +	 * disable the crtc (and hence change the state) if it is wrong. */
> +	intel_fix_plane_mapping(dev);
> +
>  	for_each_pipe(dev_priv, pipe) {
>  		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  		intel_sanitize_crtc(crtc);
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ander Conselvan de Oliveira Jan. 14, 2015, 10:50 a.m. UTC | #3
On 01/14/2015 12:47 PM, Jani Nikula wrote:
> On Tue, 13 Jan 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
>> If QUIRK_PIPEA_FORCE is necessary, intel_sanitize_crtc() might trigger
>> a mode set. In that case, if pipe A is disabled and pipe B is mapped to
>> plane B, that mode set happens before the mapping is fixed. Due to the
>> wrong state, the call to disable pipe B disables plane A (which is
>> already disabled) and later an assertion for plane B being enabled
>> (while it should have been disabled) is triggered.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=72782
>
> If the patch is expected to fix the bug, please use Bugzilla:. If you're
> referencing a bug because it's otherwise relevant, please use
> References:.
>
> Which one is this?

This showed up in the dmesg attached to the referenced bug, but is not 
the root cause of the reported problem. As far as I know, there is no 
open bug for this particular issue.

Ander

>
> BR,
> Jani.
>
>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 50 +++++++++++++++++++++++-------------
>>   1 file changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index d01db1b..cf87528 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13127,30 +13127,22 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>>   	return true;
>>   }
>>
>> -static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> +static void intel_fix_plane_mapping(struct drm_device *dev)
>>   {
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	u32 reg;
>> -
>> -	/* Clear any frame start delays used for debugging left by the BIOS */
>> -	reg = PIPECONF(crtc->config.cpu_transcoder);
>> -	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct intel_crtc *crtc;
>>
>> -	/* restore vblank interrupts to correct state */
>> -	if (crtc->active) {
>> -		update_scanline_offset(crtc);
>> -		drm_vblank_on(dev, crtc->pipe);
>> -	} else
>> -		drm_vblank_off(dev, crtc->pipe);
>> +	/* gen4+ has a fixed plane -> pipe mapping.*/
>> +	if (INTEL_INFO(dev)->gen >= 4)
>> +		return;
>>
>> -	/* We need to sanitize the plane -> pipe mapping first because this will
>> -	 * disable the crtc (and hence change the state) if it is wrong. Note
>> -	 * that gen4+ has a fixed plane -> pipe mapping.  */
>> -	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
>> +	for_each_intel_crtc(dev, crtc) {
>>   		struct intel_connector *connector;
>>   		bool plane;
>>
>> +		if (intel_check_plane_mapping(crtc))
>> +			continue;
>> +
>>   		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
>>   			      crtc->base.base.id);
>>
>> @@ -13184,6 +13176,24 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>   		WARN_ON(crtc->active);
>>   		crtc->base.enabled = false;
>>   	}
>> +}
>> +
>> +static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	u32 reg;
>> +
>> +	/* Clear any frame start delays used for debugging left by the BIOS */
>> +	reg = PIPECONF(crtc->config.cpu_transcoder);
>> +	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
>> +
>> +	/* restore vblank interrupts to correct state */
>> +	if (crtc->active) {
>> +		update_scanline_offset(crtc);
>> +		drm_vblank_on(dev, crtc->pipe);
>> +	} else
>> +		drm_vblank_off(dev, crtc->pipe);
>>
>>   	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
>>   	    crtc->pipe == PIPE_A && !crtc->active) {
>> @@ -13442,6 +13452,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>>   		intel_sanitize_encoder(encoder);
>>   	}
>>
>> +	/* We need to sanitize the plane -> pipe mapping first because this will
>> +	 * disable the crtc (and hence change the state) if it is wrong. */
>> +	intel_fix_plane_mapping(dev);
>> +
>>   	for_each_pipe(dev_priv, pipe) {
>>   		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>>   		intel_sanitize_crtc(crtc);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Chris Wilson Jan. 14, 2015, 11 a.m. UTC | #4
On Wed, Jan 14, 2015 at 12:50:46PM +0200, Ander Conselvan de Oliveira wrote:
> On 01/14/2015 12:47 PM, Jani Nikula wrote:
> >On Tue, 13 Jan 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> >>If QUIRK_PIPEA_FORCE is necessary, intel_sanitize_crtc() might trigger
> >>a mode set. In that case, if pipe A is disabled and pipe B is mapped to
> >>plane B, that mode set happens before the mapping is fixed. Due to the
> >>wrong state, the call to disable pipe B disables plane A (which is
> >>already disabled) and later an assertion for plane B being enabled
> >>(while it should have been disabled) is triggered.
> >>
> >>References: https://bugs.freedesktop.org/show_bug.cgi?id=72782
> >
> >If the patch is expected to fix the bug, please use Bugzilla:. If you're
> >referencing a bug because it's otherwise relevant, please use
> >References:.
> >
> >Which one is this?
> 
> This showed up in the dmesg attached to the referenced bug, but is
> not the root cause of the reported problem. As far as I know, there
> is no open bug for this particular issue.

But that machine should not be using QUIRK_PIPEA_FORCE... What I expect
is happening is that the BIOS sets up the LVDS on the "wrong" pipe.
-Chris
Jani Nikula Jan. 14, 2015, 11:06 a.m. UTC | #5
On Wed, 14 Jan 2015, Ander Conselvan de Oliveira <conselvan2@gmail.com> wrote:
> On 01/14/2015 12:47 PM, Jani Nikula wrote:
>> On Tue, 13 Jan 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
>>> If QUIRK_PIPEA_FORCE is necessary, intel_sanitize_crtc() might trigger
>>> a mode set. In that case, if pipe A is disabled and pipe B is mapped to
>>> plane B, that mode set happens before the mapping is fixed. Due to the
>>> wrong state, the call to disable pipe B disables plane A (which is
>>> already disabled) and later an assertion for plane B being enabled
>>> (while it should have been disabled) is triggered.
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=72782
>>
>> If the patch is expected to fix the bug, please use Bugzilla:. If you're
>> referencing a bug because it's otherwise relevant, please use
>> References:.
>>
>> Which one is this?
>
> This showed up in the dmesg attached to the referenced bug, but is not 
> the root cause of the reported problem. As far as I know, there is no 
> open bug for this particular issue.

Ah, so it's correct as it is. Sorry for the noise. Please carry on.

BR,
Jani.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d01db1b..cf87528 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13127,30 +13127,22 @@  intel_check_plane_mapping(struct intel_crtc *crtc)
 	return true;
 }
 
-static void intel_sanitize_crtc(struct intel_crtc *crtc)
+static void intel_fix_plane_mapping(struct drm_device *dev)
 {
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 reg;
-
-	/* Clear any frame start delays used for debugging left by the BIOS */
-	reg = PIPECONF(crtc->config.cpu_transcoder);
-	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *crtc;
 
-	/* restore vblank interrupts to correct state */
-	if (crtc->active) {
-		update_scanline_offset(crtc);
-		drm_vblank_on(dev, crtc->pipe);
-	} else
-		drm_vblank_off(dev, crtc->pipe);
+	/* gen4+ has a fixed plane -> pipe mapping.*/
+	if (INTEL_INFO(dev)->gen >= 4)
+		return;
 
-	/* We need to sanitize the plane -> pipe mapping first because this will
-	 * disable the crtc (and hence change the state) if it is wrong. Note
-	 * that gen4+ has a fixed plane -> pipe mapping.  */
-	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
+	for_each_intel_crtc(dev, crtc) {
 		struct intel_connector *connector;
 		bool plane;
 
+		if (intel_check_plane_mapping(crtc))
+			continue;
+
 		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
 			      crtc->base.base.id);
 
@@ -13184,6 +13176,24 @@  static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		WARN_ON(crtc->active);
 		crtc->base.enabled = false;
 	}
+}
+
+static void intel_sanitize_crtc(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg;
+
+	/* Clear any frame start delays used for debugging left by the BIOS */
+	reg = PIPECONF(crtc->config.cpu_transcoder);
+	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
+
+	/* restore vblank interrupts to correct state */
+	if (crtc->active) {
+		update_scanline_offset(crtc);
+		drm_vblank_on(dev, crtc->pipe);
+	} else
+		drm_vblank_off(dev, crtc->pipe);
 
 	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
 	    crtc->pipe == PIPE_A && !crtc->active) {
@@ -13442,6 +13452,10 @@  void intel_modeset_setup_hw_state(struct drm_device *dev,
 		intel_sanitize_encoder(encoder);
 	}
 
+	/* We need to sanitize the plane -> pipe mapping first because this will
+	 * disable the crtc (and hence change the state) if it is wrong. */
+	intel_fix_plane_mapping(dev);
+
 	for_each_pipe(dev_priv, pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 		intel_sanitize_crtc(crtc);