[Intel-gfx] [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2.

[Matt Roper]

On Thu, Sep 20, 2018 at 12:27:05PM +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.
> 
> Changes since v1:
> - Clarify why we cannot swap crtc_state. (Matt)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 58c188482c78..078cdcca88e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13515,14 +13515,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;
> @@ -13552,6 +13554,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;
> @@ -13563,9 +13571,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;
> @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	/* Swap plane state */
>  	plane->state = new_plane_state;
>  
> +	/*
> +	 * We cannot swap crtc_state as it may be in use by an atomic commit or
> +	 * page flip that's running simultaneously. If we swap crtc_state and
> +	 * destroy the old state, we will cause a use-after-free there.

Just to confirm, the commit running simultaneously would have to have
already dropped locks (specifically the crtc lock) and returned to
userspace for us to have this problem, right?  So it's either a
non-blocking commit in the process of doing its programming via
workqueue, or a blocking commit that's done everything except
intel_atomic_cleanup_work?

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

> +	 *
> +	 * Only update active_planes, which is needed for our internal
> +	 * bookkeeping. Either value will do the right thing when updating
> +	 * planes atomically. If the cursor was part of the atomic update then
> +	 * we would have taken the slowpath.
> +	 */
> +	crtc_state->active_planes = new_crtc_state->active_planes;
> +
>  	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));
> @@ -13602,6 +13620,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