[Intel-gfx] [PATCH] drm/i915: Don't clobber plane state on internal disables

[Daniel Vetter]

On Wed, Mar 04, 2015 at 10:49:04AM -0800, Matt Roper wrote:
> We need to disable all sprite planes when disabling the CRTC.  We had
> been using the top-level atomic 'disable' entrypoint to accomplish this,
> which was wrong.  Not only can this lead to various locking issues, it
> also modifies the actual plane state, making it impossible to restore
> the plane properly later.  For example, a DPMS off followed by a DPMS on
> will result in any sprite planes in use not being restored properly.
> 
> The proper solution here is to call directly into our 'commit plane'
> hook with a copy of the plane's current state that has 'visible' set to
> false.  Committing this dummy state will turn off the plane, but will
> not touch the actual plane->state pointer, allowing us to properly
> restore the plane state later.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> This was a pretty big problem so I suspect this is probably tied to some of our
> existing bugzilla's but I'm not sure exactly which ones.  It seems that we lack
> an i-g-t test for plane status across DPMS, so I'll send along a new i-g-t test
> for that shortly.
> 
>  drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92cb2ff..6277701 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4201,6 +4201,24 @@ static void intel_enable_sprite_planes(struct drm_crtc *crtc)
>  	}
>  }
>  
> +/*
> + * Disable a plane internally without actually modifying the plane's state.
> + * This will allow us to easily restore the plane later by just reprogramming
> + * its state.
> + */
> +static void disable_plane_internal(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_plane_state *state =
> +		plane->funcs->atomic_duplicate_state(plane);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	intel_state->visible = false;
> +	intel_plane->commit_plane(plane, intel_state);
> +
> +	intel_plane_destroy_state(plane, state);

I wonder whether long-term we should do a full atomic commit including the
begin/end dance here. But this is more than good enough for now, so merged
to dinq.

Thanks, Daniel

> +}
> +
>  static void intel_disable_sprite_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4210,8 +4228,8 @@ static void intel_disable_sprite_planes(struct drm_crtc *crtc)
>  
>  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>  		intel_plane = to_intel_plane(plane);
> -		if (intel_plane->pipe == pipe)
> -			plane->funcs->disable_plane(plane);
> +		if (plane->fb && intel_plane->pipe == pipe)
> +			disable_plane_internal(plane);
>  	}
>  }
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch