Re: [PATCH] drm/atomic: Check old_plane_state->crtc in drm_atomic_helper_async_check()

From: Sean Paul <seanpaul@chromium.org>
To: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/atomic: Check old_plane_state->crtc in drm_atomic_helper_async_check()
Date: Mon, 23 Jul 2018 10:08:42 -0400	[thread overview]
Message-ID: <20180723140842.GH20359@art_vandelay> (raw)
In-Reply-To: <20180723134354.26298-1-boris.brezillon@bootlin.com>

On Mon, Jul 23, 2018 at 03:43:54PM +0200, Boris Brezillon wrote:
> Async plane update is supposed to work only when updating the FB or FB
> position of an already enabled plane. That does not apply to requests
> where the plane was previously disabled or assigned to a different
> CTRC.
> 
> Check old_plane_state->crtc value to make sure async plane update is
> allowed.
> 
> Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update")
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Hello,
> 
> As discussed on IRC, I'm not sure this "plane should already be enabled
> and assigned to the same CRTC to allow async updates" limitation
> applies to all drivers, but it's required for VC4 to work properly.
> 
> Just let me know if you think I should move this check to
> vc4_plane_atomic_async_check().

I don't _think_ there's any hardware that can switch planes without first
disabling the plane for a frame and then re-enabling it, so this passes the
smell test for me. I'd be very curious to be proven wrong, though :)

Sean

> 
> Thanks,
> 
> Boris
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 866a2cc72ef6..f7ccfebd3ca8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1555,7 +1555,8 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  	if (n_planes != 1)
>  		return -EINVAL;
>  
> -	if (!new_plane_state->crtc)
> +	if (!new_plane_state->crtc ||
> +	    old_plane_state->crtc != new_plane_state->crtc)
>  		return -EINVAL;
>  
>  	funcs = plane->helper_private;
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel