drm/i915: Don't check modeset state in the hw state force restore path

Submitted by Ander Conselvan de Oliveira on June 1, 2015, 12:41 p.m.

Details

Message ID 1433162483-25476-1-git-send-email-ander.conselvan.de.oliveira@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Ander Conselvan de Oliveira June 1, 2015, 12:41 p.m.
Since the force restore logic will restore the CRTCs state one at a
time, it is possible that the state will be inconsistent until the whole
operation finishes. A call to intel_modeset_check_state() is done once
it's over, so don't check the state multiple times in between. This
regression was introduced in:

commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Wed Nov 5 14:26:06 2014 -0800

    drm/i915: factor out compute_config from __intel_set_mode v3

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---

Hi,

This patch applies on top of nightly, but it is only relevant without
Maarten's "drm/i915: Convert to atomic, part 2" series, because of the
changes to the hw state read out and force restore logic.

The regression exists since 3.19.

---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16e159d..24fb7ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -87,7 +87,8 @@  static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config);
 
 static int intel_set_mode(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state);
+			  struct drm_atomic_state *state,
+			  bool check);
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -10282,7 +10283,7 @@  retry:
 
 	drm_mode_copy(&crtc_state->base.mode, mode);
 
-	if (intel_set_mode(crtc, state)) {
+	if (intel_set_mode(crtc, state, true)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
@@ -10356,7 +10357,7 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (ret)
 			goto fail;
 
-		ret = intel_set_mode(crtc, state);
+		ret = intel_set_mode(crtc, state, true);
 		if (ret)
 			goto fail;
 
@@ -12832,20 +12833,22 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 }
 
 static int intel_set_mode_with_config(struct drm_crtc *crtc,
-				      struct intel_crtc_state *pipe_config)
+				      struct intel_crtc_state *pipe_config,
+				      bool check)
 {
 	int ret;
 
 	ret = __intel_set_mode(crtc, pipe_config);
 
-	if (ret == 0)
+	if (ret == 0 && check)
 		intel_modeset_check_state(crtc->dev);
 
 	return ret;
 }
 
 static int intel_set_mode(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state)
+			  struct drm_atomic_state *state,
+			  bool check)
 {
 	struct intel_crtc_state *pipe_config;
 	int ret = 0;
@@ -12856,7 +12859,7 @@  static int intel_set_mode(struct drm_crtc *crtc,
 		goto out;
 	}
 
-	ret = intel_set_mode_with_config(crtc, pipe_config);
+	ret = intel_set_mode_with_config(crtc, pipe_config, check);
 	if (ret)
 		goto out;
 
@@ -12933,7 +12936,7 @@  void intel_crtc_restore_mode(struct drm_crtc *crtc)
 	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
 					crtc->primary->fb, crtc->x, crtc->y);
 
-	ret = intel_set_mode(crtc, state);
+	ret = intel_set_mode(crtc, state, false);
 	if (ret)
 		drm_atomic_state_free(state);
 }
@@ -13133,7 +13136,7 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 
 	primary_plane_was_visible = primary_plane_visible(set->crtc);
 
-	ret = intel_set_mode_with_config(set->crtc, pipe_config);
+	ret = intel_set_mode_with_config(set->crtc, pipe_config, true);
 
 	if (ret == 0 &&
 	    pipe_config->base.enable &&

Comments

On Mon, 2015-06-01 at 15:41 +0300, Ander Conselvan de Oliveira wrote:
> Since the force restore logic will restore the CRTCs state one at a

> time, it is possible that the state will be inconsistent until the whole

> operation finishes. A call to intel_modeset_check_state() is done once

> it's over, so don't check the state multiple times in between. This

> regression was introduced in:

> 

> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f

> Author: Jesse Barnes <jbarnes@virtuousgeek.org>

> Date:   Wed Nov 5 14:26:06 2014 -0800

> 

>     drm/i915: factor out compute_config from __intel_set_mode v3

> 

> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431

> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>

> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

> ---

> 

> Hi,

> 

> This patch applies on top of nightly, but it is only relevant without

> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the

> changes to the hw state read out and force restore logic.

> 

> The regression exists since 3.19.


Same patch adapated for 4.1-rc6 here:
https://bugzilla.kernel.org/attachment.cgi?id=178461

Ander

> ---

>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------

>  1 file changed, 12 insertions(+), 9 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c

> index 16e159d..24fb7ce 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -87,7 +87,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,

>  				   struct intel_crtc_state *pipe_config);

>  

>  static int intel_set_mode(struct drm_crtc *crtc,

> -			  struct drm_atomic_state *state);

> +			  struct drm_atomic_state *state,

> +			  bool check);

>  static int intel_framebuffer_init(struct drm_device *dev,

>  				  struct intel_framebuffer *ifb,

>  				  struct drm_mode_fb_cmd2 *mode_cmd,

> @@ -10282,7 +10283,7 @@ retry:

>  

>  	drm_mode_copy(&crtc_state->base.mode, mode);

>  

> -	if (intel_set_mode(crtc, state)) {

> +	if (intel_set_mode(crtc, state, true)) {

>  		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");

>  		if (old->release_fb)

>  			old->release_fb->funcs->destroy(old->release_fb);

> @@ -10356,7 +10357,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,

>  		if (ret)

>  			goto fail;

>  

> -		ret = intel_set_mode(crtc, state);

> +		ret = intel_set_mode(crtc, state, true);

>  		if (ret)

>  			goto fail;

>  

> @@ -12832,20 +12833,22 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,

>  }

>  

>  static int intel_set_mode_with_config(struct drm_crtc *crtc,

> -				      struct intel_crtc_state *pipe_config)

> +				      struct intel_crtc_state *pipe_config,

> +				      bool check)

>  {

>  	int ret;

>  

>  	ret = __intel_set_mode(crtc, pipe_config);

>  

> -	if (ret == 0)

> +	if (ret == 0 && check)

>  		intel_modeset_check_state(crtc->dev);

>  

>  	return ret;

>  }

>  

>  static int intel_set_mode(struct drm_crtc *crtc,

> -			  struct drm_atomic_state *state)

> +			  struct drm_atomic_state *state,

> +			  bool check)

>  {

>  	struct intel_crtc_state *pipe_config;

>  	int ret = 0;

> @@ -12856,7 +12859,7 @@ static int intel_set_mode(struct drm_crtc *crtc,

>  		goto out;

>  	}

>  

> -	ret = intel_set_mode_with_config(crtc, pipe_config);

> +	ret = intel_set_mode_with_config(crtc, pipe_config, check);

>  	if (ret)

>  		goto out;

>  

> @@ -12933,7 +12936,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)

>  	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,

>  					crtc->primary->fb, crtc->x, crtc->y);

>  

> -	ret = intel_set_mode(crtc, state);

> +	ret = intel_set_mode(crtc, state, false);

>  	if (ret)

>  		drm_atomic_state_free(state);

>  }

> @@ -13133,7 +13136,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)

>  

>  	primary_plane_was_visible = primary_plane_visible(set->crtc);

>  

> -	ret = intel_set_mode_with_config(set->crtc, pipe_config);

> +	ret = intel_set_mode_with_config(set->crtc, pipe_config, true);

>  

>  	if (ret == 0 &&

>  	    pipe_config->base.enable &&


---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6514
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                 -1              321/321              320/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW  igt@gem_flink@basic      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> Since the force restore logic will restore the CRTCs state one at a
> time, it is possible that the state will be inconsistent until the whole
> operation finishes. A call to intel_modeset_check_state() is done once
> it's over, so don't check the state multiple times in between. This
> regression was introduced in:
>
> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Wed Nov 5 14:26:06 2014 -0800
>
>     drm/i915: factor out compute_config from __intel_set_mode v3
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>
> Hi,
>
> This patch applies on top of nightly, but it is only relevant without
> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the
> changes to the hw state read out and force restore logic.
>
> The regression exists since 3.19.

Sooo, I think this should be applied to fixes, with cc: stable v3.19+,
and IIUC Maarten's series makes this obsolete in dinq?

Now we just need review... Maarten?

BR,
Jani.




>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 16e159d..24fb7ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -87,7 +87,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
>  				   struct intel_crtc_state *pipe_config);
>  
>  static int intel_set_mode(struct drm_crtc *crtc,
> -			  struct drm_atomic_state *state);
> +			  struct drm_atomic_state *state,
> +			  bool check);
>  static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct intel_framebuffer *ifb,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> @@ -10282,7 +10283,7 @@ retry:
>  
>  	drm_mode_copy(&crtc_state->base.mode, mode);
>  
> -	if (intel_set_mode(crtc, state)) {
> +	if (intel_set_mode(crtc, state, true)) {
>  		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
>  		if (old->release_fb)
>  			old->release_fb->funcs->destroy(old->release_fb);
> @@ -10356,7 +10357,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  		if (ret)
>  			goto fail;
>  
> -		ret = intel_set_mode(crtc, state);
> +		ret = intel_set_mode(crtc, state, true);
>  		if (ret)
>  			goto fail;
>  
> @@ -12832,20 +12833,22 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  }
>  
>  static int intel_set_mode_with_config(struct drm_crtc *crtc,
> -				      struct intel_crtc_state *pipe_config)
> +				      struct intel_crtc_state *pipe_config,
> +				      bool check)
>  {
>  	int ret;
>  
>  	ret = __intel_set_mode(crtc, pipe_config);
>  
> -	if (ret == 0)
> +	if (ret == 0 && check)
>  		intel_modeset_check_state(crtc->dev);
>  
>  	return ret;
>  }
>  
>  static int intel_set_mode(struct drm_crtc *crtc,
> -			  struct drm_atomic_state *state)
> +			  struct drm_atomic_state *state,
> +			  bool check)
>  {
>  	struct intel_crtc_state *pipe_config;
>  	int ret = 0;
> @@ -12856,7 +12859,7 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  		goto out;
>  	}
>  
> -	ret = intel_set_mode_with_config(crtc, pipe_config);
> +	ret = intel_set_mode_with_config(crtc, pipe_config, check);
>  	if (ret)
>  		goto out;
>  
> @@ -12933,7 +12936,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
>  					crtc->primary->fb, crtc->x, crtc->y);
>  
> -	ret = intel_set_mode(crtc, state);
> +	ret = intel_set_mode(crtc, state, false);
>  	if (ret)
>  		drm_atomic_state_free(state);
>  }
> @@ -13133,7 +13136,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  
>  	primary_plane_was_visible = primary_plane_visible(set->crtc);
>  
> -	ret = intel_set_mode_with_config(set->crtc, pipe_config);
> +	ret = intel_set_mode_with_config(set->crtc, pipe_config, true);
>  
>  	if (ret == 0 &&
>  	    pipe_config->base.enable &&
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 02-06-15 om 09:12 schreef Jani Nikula:
> On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
>> Since the force restore logic will restore the CRTCs state one at a
>> time, it is possible that the state will be inconsistent until the whole
>> operation finishes. A call to intel_modeset_check_state() is done once
>> it's over, so don't check the state multiple times in between. This
>> regression was introduced in:
>>
>> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f
>> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Date:   Wed Nov 5 14:26:06 2014 -0800
>>
>>     drm/i915: factor out compute_config from __intel_set_mode v3
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>
>> Hi,
>>
>> This patch applies on top of nightly, but it is only relevant without
>> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the
>> changes to the hw state read out and force restore logic.
>>
>> The regression exists since 3.19.
> Sooo, I think this should be applied to fixes, with cc: stable v3.19+,
> and IIUC Maarten's series makes this obsolete in dinq?
>
> Now we just need review... Maarten?
>
> BR,
> Jani.
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Looks good to me, but it will conflict with my own patch series. :(

~Maarten
On Tue, 2015-06-02 at 09:27 +0200, Maarten Lankhorst wrote:
> Op 02-06-15 om 09:12 schreef Jani Nikula:
> > On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> >> Since the force restore logic will restore the CRTCs state one at a
> >> time, it is possible that the state will be inconsistent until the whole
> >> operation finishes. A call to intel_modeset_check_state() is done once
> >> it's over, so don't check the state multiple times in between. This
> >> regression was introduced in:
> >>
> >> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f
> >> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Date:   Wed Nov 5 14:26:06 2014 -0800
> >>
> >>     drm/i915: factor out compute_config from __intel_set_mode v3
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431
> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >> ---
> >>
> >> Hi,
> >>
> >> This patch applies on top of nightly, but it is only relevant without
> >> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the
> >> changes to the hw state read out and force restore logic.
> >>
> >> The regression exists since 3.19.
> > Sooo, I think this should be applied to fixes, with cc: stable v3.19+,
> > and IIUC Maarten's series makes this obsolete in dinq?
> >
> > Now we just need review... Maarten?
> >
> > BR,
> > Jani.
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Looks good to me, but it will conflict with my own patch series. :(

I think it's fine to skip this for dinq, since we move to a single
modeset in the force restore path with your patch series. I just got
confused with what branch to base this on. Thanks for reviewing.

Ander
On Tue, 02 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> On Tue, 2015-06-02 at 09:27 +0200, Maarten Lankhorst wrote:
>> Op 02-06-15 om 09:12 schreef Jani Nikula:
>> > On Mon, 01 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
>> >> Since the force restore logic will restore the CRTCs state one at a
>> >> time, it is possible that the state will be inconsistent until the whole
>> >> operation finishes. A call to intel_modeset_check_state() is done once
>> >> it's over, so don't check the state multiple times in between. This
>> >> regression was introduced in:
>> >>
>> >> commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f
>> >> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>> >> Date:   Wed Nov 5 14:26:06 2014 -0800
>> >>
>> >>     drm/i915: factor out compute_config from __intel_set_mode v3
>> >>
>> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431
>> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> >> ---
>> >>
>> >> Hi,
>> >>
>> >> This patch applies on top of nightly, but it is only relevant without
>> >> Maarten's "drm/i915: Convert to atomic, part 2" series, because of the
>> >> changes to the hw state read out and force restore logic.
>> >>
>> >> The regression exists since 3.19.
>> > Sooo, I think this should be applied to fixes, with cc: stable v3.19+,
>> > and IIUC Maarten's series makes this obsolete in dinq?
>> >
>> > Now we just need review... Maarten?
>> >
>> > BR,
>> > Jani.
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> 
>> Looks good to me, but it will conflict with my own patch series. :(
>
> I think it's fine to skip this for dinq, since we move to a single
> modeset in the force restore path with your patch series. I just got
> confused with what branch to base this on. Thanks for reviewing.

Looks like this conflicts even more between v4.1-rc and what's queued
for v4.2. I'll pass this for now, I'm think it may be easier to fix this
for v4.2 and backport from there.

BR,
Jani.





>
> Ander
>
>
>
>