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

[Matt Roper <matthew.d.roper@intel.com>]

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