[Intel-gfx] [PATCH 02/15] drm/i915: Handle cursor updating active_planes correctly.

[Matt Roper]

On Wed, Sep 19, 2018 at 03:56:31PM +0200, Maarten Lankhorst wrote:
> While we may not update new_crtc_state, we may clear active_planes
> if the new cursor update state will disable the cursor, but we fail
> after. If this is immediately followed by a modeset disable, we may
> soon not disable the planes correctly when we start depending on
> active_planes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

Fixes: b2b55502d683c ("drm/i915: Pass proper old/new states to
intel_plane_atomic_check_with_state()")

I think?  So basically we were clobbering the current cstate's
active_planes field (and anything else in cstate that
intel_plane_atomic_check_with_state() might touch in the future) if the
atomic transaction failed.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ac381eb8103..7a7ff1d76031 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13517,14 +13517,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *old_fb;
> -	struct drm_crtc_state *crtc_state = crtc->state;
> +	struct intel_crtc_state *crtc_state =
> +		to_intel_crtc_state(crtc->state);
> +	struct intel_crtc_state *new_crtc_state;
>  
>  	/*
>  	 * When crtc is inactive or there is a modeset pending,
>  	 * wait for it to complete in the slowpath
>  	 */
> -	if (!crtc_state->active || needs_modeset(crtc_state) ||
> -	    to_intel_crtc_state(crtc_state)->update_pipe)
> +	if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
> +	    crtc_state->update_pipe)
>  		goto slow;
>  
>  	old_plane_state = plane->state;
> @@ -13554,6 +13556,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	if (!new_plane_state)
>  		return -ENOMEM;
>  
> +	new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
> +	if (!new_crtc_state) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
>  	drm_atomic_set_fb_for_plane(new_plane_state, fb);
>  
>  	new_plane_state->src_x = src_x;
> @@ -13565,9 +13573,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	new_plane_state->crtc_w = crtc_w;
>  	new_plane_state->crtc_h = crtc_h;
>  
> -	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> -						  to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
> -						  to_intel_plane_state(plane->state),
> +	ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
> +						  to_intel_plane_state(old_plane_state),
>  						  to_intel_plane_state(new_plane_state));
>  	if (ret)
>  		goto out_free;
> @@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  
>  	/* Swap plane state */
>  	plane->state = new_plane_state;
> +	crtc_state->active_planes = new_crtc_state->active_planes;

At the moment this is the only crtc_state field that our plane check
might update for a cursor, but is there a reason why we don't just swap
in the new state and destroy the old one farther down?  If our driver
ever did start updating other fields, it would be really easy to
overlook the need to copy those new fields over here.  I realize a lot
of the likely candidates for fields that plane check might want to
update (e.g., watermark stuff) would also disqualify us from taking this
legacy cursor fastpath in the first place, but I'm a bit nervous about
assuming that we'll never be updating any other fields at all.


Matt

>  
>  	if (plane->state->visible) {
>  		trace_intel_update_plane(plane, to_intel_crtc(crtc));
> -		intel_plane->update_plane(intel_plane,
> -					  to_intel_crtc_state(crtc->state),
> +		intel_plane->update_plane(intel_plane, crtc_state,
>  					  to_intel_plane_state(plane->state));
>  	} else {
>  		trace_intel_disable_plane(plane, to_intel_crtc(crtc));
> @@ -13604,6 +13611,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  out_free:
> +	if (new_crtc_state)
> +		intel_crtc_destroy_state(crtc, &new_crtc_state->base);
>  	if (ret)
>  		intel_plane_destroy_state(plane, new_plane_state);
>  	else
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795