[PATCH 0/3] Couple of bug fixes for drm-intel-next-fixes

[PATCH 0/3] Couple of bug fixes for drm-intel-next-fixes

[Ander Conselvan de Oliveira]

Hi,

These three patches solve two issues in drm-intel-next-fixes. The most
important one is the fail to restore modes properly in the force_restore
path, after a suspend/resume cycle, fixed by the last two patches. The
other is a fix a sent before, for the multiple checks during force
restore.

Thanks,
Ander

Ander Conselvan de Oliveira (3):
  drm/i915: Don't check modeset state in the hw state force restore path
  drm/i915: Don't update staged config in during force restore modesets
  drm/i915: Don't set enabled value of all CRTCs when restoring the mode

 drivers/gpu/drm/i915/intel_display.c | 54 ++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Don't check modeset state in the hw state force restore path

[Ander Conselvan de Oliveira]

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>
---
 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 4e3f302..6ef57e6 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,
@@ -10096,7 +10097,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);
@@ -10170,7 +10171,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;
 
@@ -12646,20 +12647,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;
@@ -12670,7 +12673,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;
 
@@ -12747,7 +12750,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);
 }
@@ -12947,7 +12950,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

[PATCH 2/3] drm/i915: Don't update staged config during force restore modesets

[Ander Conselvan de Oliveira]

The force restore path relies on the staged config to preserve the
configuration used before a suspend/resume cycle. The update done to it
in intel_modeset_fixup_state() would cause that information to be lost
after the first modeset, making it impossible to restore the modes for
pipes B and C.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6ef57e6..49c6698 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11386,10 +11386,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state)
 		crtc->base.enabled = crtc->base.state->enable;
 		crtc->config = to_intel_crtc_state(crtc->base.state);
 	}
-
-	/* Copy the new configuration to the staged state, to keep the few
-	 * pieces of code that haven't been converted yet happy */
-	intel_modeset_update_staged_output_state(state->dev);
 }
 
 static void
@@ -12654,8 +12650,10 @@ static int intel_set_mode_with_config(struct drm_crtc *crtc,
 
 	ret = __intel_set_mode(crtc, pipe_config);
 
-	if (ret == 0 && check)
+	if (ret == 0 && check) {
+		intel_modeset_update_staged_output_state(crtc->dev);
 		intel_modeset_check_state(crtc->dev);
+	}
 
 	return ret;
 }
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Don't set enabled value of all CRTCs when restoring the mode

[Ander Conselvan de Oliveira]

The code in intel_crtc_restore_mode() sets the enabled value of all the
CRTCs when restoring the mode after a suspend/resume cycle. When more
than one CRTC is enabled, that causes drm_atomic_helper_check_modeset()
to fail if there is more than one pipe enabled, since only one CRTC has
valid connector data. Instead, set only the enabled value for the CRTC
passed as an argument.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49c6698..736e653 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12683,7 +12683,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_atomic_state *state;
-	struct intel_crtc *intel_crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 	struct drm_connector_state *connector_state;
@@ -12726,24 +12725,18 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 		}
 	}
 
-	for_each_intel_crtc(dev, intel_crtc) {
-		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
-			continue;
-
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state)) {
-			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
-				      intel_crtc->base.base.id,
-				      PTR_ERR(crtc_state));
-			continue;
-		}
+	crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
+	if (IS_ERR(crtc_state)) {
+		DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
+			      crtc->base.id, PTR_ERR(crtc_state));
+		/* FIXME: leaking drm atomic state */
+		return;
+	}
 
-		crtc_state->base.active = crtc_state->base.enable =
-			intel_crtc->new_enabled;
+	crtc_state->base.active = crtc_state->base.enable =
+		to_intel_crtc(crtc)->new_enabled;
 
-		if (&intel_crtc->base == crtc)
-			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
-	}
+	drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
 
 	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
 					crtc->primary->fb, crtc->x, crtc->y);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Don't check modeset state in the hw state force restore path

[Matt Roper]

On Fri, Jun 12, 2015 at 10:19:40AM +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>
> ---
>  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 4e3f302..6ef57e6 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,
> @@ -10096,7 +10097,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);
> @@ -10170,7 +10171,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;
>  
> @@ -12646,20 +12647,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)

This parameter just controls whether you check the state or not in this
patch, but in patch #2 it also starts having more of a behavioral impact
(i.e., "don't updated staged output configuration").  I wonder if
picking a different name for this parameter would help avoid any
confusion?

Otherwise, this patch looks good:
   Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Off-topic, but speaking of 'check' I wonder whether we should also
rename the intel_modeset_check_state function and related functions at
some point in the future.  Every time I see that name in the code it
bothers me because I expect it to be related to atomic check (i.e.,
something that runs before we touch hardware) rather than confirming
that hardware programming was successful.  It feels more like an
assert/confirm/verify function to me.


Matt

>  {
>  	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;
> @@ -12670,7 +12673,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;
>  
> @@ -12747,7 +12750,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);
>  }
> @@ -12947,7 +12950,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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Don't update staged config during force restore modesets

[Matt Roper]

On Fri, Jun 12, 2015 at 10:19:41AM +0300, Ander Conselvan de Oliveira wrote:
> The force restore path relies on the staged config to preserve the
> configuration used before a suspend/resume cycle. The update done to it
> in intel_modeset_fixup_state() would cause that information to be lost
> after the first modeset, making it impossible to restore the modes for
> pipes B and C.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=90468
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6ef57e6..49c6698 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11386,10 +11386,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state)
>  		crtc->base.enabled = crtc->base.state->enable;
>  		crtc->config = to_intel_crtc_state(crtc->base.state);
>  	}
> -
> -	/* Copy the new configuration to the staged state, to keep the few
> -	 * pieces of code that haven't been converted yet happy */
> -	intel_modeset_update_staged_output_state(state->dev);
>  }
>  
>  static void
> @@ -12654,8 +12650,10 @@ static int intel_set_mode_with_config(struct drm_crtc *crtc,
>  
>  	ret = __intel_set_mode(crtc, pipe_config);
>  
> -	if (ret == 0 && check)
> +	if (ret == 0 && check) {
> +		intel_modeset_update_staged_output_state(crtc->dev);
>  		intel_modeset_check_state(crtc->dev);
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Don't set enabled value of all CRTCs when restoring the mode

[Matt Roper]

On Fri, Jun 12, 2015 at 10:19:42AM +0300, Ander Conselvan de Oliveira wrote:
> The code in intel_crtc_restore_mode() sets the enabled value of all the
> CRTCs when restoring the mode after a suspend/resume cycle. When more
> than one CRTC is enabled, that causes drm_atomic_helper_check_modeset()
> to fail if there is more than one pipe enabled, since only one CRTC has
> valid connector data. Instead, set only the enabled value for the CRTC
> passed as an argument.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90468
> References: https://bugs.freedesktop.org/show_bug.cgi?id=90396
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 49c6698..736e653 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12683,7 +12683,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_atomic_state *state;
> -	struct intel_crtc *intel_crtc;
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
>  	struct drm_connector_state *connector_state;
> @@ -12726,24 +12725,18 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  		}
>  	}
>  
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
> -			continue;
> -
> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> -		if (IS_ERR(crtc_state)) {
> -			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
> -				      intel_crtc->base.base.id,
> -				      PTR_ERR(crtc_state));
> -			continue;
> -		}
> +	crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> +	if (IS_ERR(crtc_state)) {
> +		DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
> +			      crtc->base.id, PTR_ERR(crtc_state));
> +		/* FIXME: leaking drm atomic state */

I'm not sure I understand why we need to leak here?  Can't we just call
drm_atomic_state_free() before returning?

Aside from that,
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> +		return;
> +	}
>  
> -		crtc_state->base.active = crtc_state->base.enable =
> -			intel_crtc->new_enabled;
> +	crtc_state->base.active = crtc_state->base.enable =
> +		to_intel_crtc(crtc)->new_enabled;
>  
> -		if (&intel_crtc->base == crtc)
> -			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
> -	}
> +	drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
>  
>  	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
>  					crtc->primary->fb, crtc->x, crtc->y);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Don't check modeset state in the hw state force restore path

[Ander Conselvan De Oliveira]

On Mon, 2015-06-15 at 19:00 -0700, Matt Roper wrote:
> On Fri, Jun 12, 2015 at 10:19:40AM +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>
> > ---
> >  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 4e3f302..6ef57e6 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,
> > @@ -10096,7 +10097,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);
> > @@ -10170,7 +10171,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;
> >  
> > @@ -12646,20 +12647,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)
> 
> This parameter just controls whether you check the state or not in this
> patch, but in patch #2 it also starts having more of a behavioral impact
> (i.e., "don't updated staged output configuration").  I wonder if
> picking a different name for this parameter would help avoid any
> confusion?

Thanks for reviewing! I just sent a v2 with the parameter name changed
to force_restore and the memory leak fixed.

> Otherwise, this patch looks good:
>    Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Off-topic, but speaking of 'check' I wonder whether we should also
> rename the intel_modeset_check_state function and related functions at
> some point in the future.  Every time I see that name in the code it
> bothers me because I expect it to be related to atomic check (i.e.,
> something that runs before we touch hardware) rather than confirming
> that hardware programming was successful.  It feels more like an
> assert/confirm/verify function to me.

I'm not sure what would be a better name, but I agree the current one is
confusing.

Ander

> 
> Matt
> 
> >  {
> >  	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;
> > @@ -12670,7 +12673,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;
> >  
> > @@ -12747,7 +12750,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);
> >  }
> > @@ -12947,7 +12950,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
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx