Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH] drm/atomic: add ASYNC_UPDATE flag to the Atomic
 IOCTL.
From:   Enric Balletbo i Serra <enric.balletbo@collabora.com>
To:     David Airlie <airlied@linux.ie>
Cc:     dnicoara@chromium.org, alexandros.frantzis@collabora.com,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        Gustavo Padovan <gustavo.padovan@collabora.com>,
        tomasz Figa <tfiga@chromium.org>,
        Sean Paul <seanpaul@google.com>, kernel@collabora.com,
        =?UTF-8?Q?St=c3=a9phane_Marchesin?= <marcheu@google.com>,
        Gustavo Padovan <gustavo@padovan.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>
References: <20180627212506.24061-1-enric.balletbo@collabora.com>
Message-ID: <d13bdaa4-4663-5d2e-fdf6-ec9794e2a809@collabora.com>
Date:   Wed, 27 Jun 2018 23:36:30 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <20180627212506.24061-1-enric.balletbo@collabora.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 27/06/18 23:25, Enric Balletbo i Serra wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> This flag tells core to jump ahead the queued update if the conditions
> in drm_atomic_async_check() are met. That means we are only able to do an
> async update if no modeset is pending and update for the same plane is
> not queued.
> 
> It uses the already in place infrastructure for async updates.
> 
> It is useful for cursor updates and async PageFlips over the atomic
> ioctl, otherwise in some cases updates may be delayed to the point the
> user will notice it.
> 
> DRM_MODE_ATOMIC_ASYNC_UPDATE should be passed to the Atomic IOCTL to use
> this feature.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Hi,
> 
> This is an attempt to introduce the new ASYNC_UPDATE flag for atomic
> operations, see the commit message for a more detailed description.
> 
> To test this patch we have created an IGT test that we plan to send to
> the ML but also was tested using a small program that exercises the uAPI
> for easy sanity testing. The program created by Alexandros can be found here
> [2]. To test, just build the program and use the --atomic flag to use the
> cursor plane in normal (blocking mode), and --atomic-async to use the cursor
> plane with the ASYNC_UPDATE flag.E.g.
> 
>   drm_cursor --atomic
> 
> or
> 
>   drm_cursor --atomic-async
> 
> The test worked on a Samsung Chromebook Plus on top of mainline plus
> the patch to update cursors asynchronously through atomic for the
> drm/rockchip driver [3].
> 
> Alexandros also did a proof-of-concept to use this flag and draw cursors
> using atomic if possible on ozone [1].
> 
> Best regards,
>  Enric
> 
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
> [2] https://gitlab.collabora.com/alf/drm-cursor
> [3] https://patchwork.kernel.org/patch/10492693/
> 
> 
>  drivers/gpu/drm/drm_atomic.c        | 6 ++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++---
>  include/uapi/drm/drm_mode.h         | 4 +++-
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c825c76edc1d..15b799f46982 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -80,6 +80,7 @@ drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
>  	 * setting this appropriately?
>  	 */
>  	state->allow_modeset = true;
> +	state->async_update = true;
>  
>  	state->crtcs = kcalloc(dev->mode_config.num_crtc,
>  			       sizeof(*state->crtcs), GFP_KERNEL);
> @@ -2320,6 +2321,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
>  		return -EINVAL;
>  
> +	if ((arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET) &&
> +			(arg->flags & DRM_MODE_ATOMIC_ASYNC_UPDATE))
> +		return -EINVAL;
> +
>  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>  
>  	state = drm_atomic_state_alloc(dev);
> @@ -2328,6 +2333,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  
>  	state->acquire_ctx = &ctx;
>  	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
> +	state->async_update = !!(arg->flags & DRM_MODE_ATOMIC_ASYNC_UPDATE);
>  
>  retry:
>  	plane_mask = 0;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c35654591c12..aeb0523d3bcf 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -891,7 +891,7 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	if (state->legacy_cursor_update)
> +	if (state->async_update || state->legacy_cursor_update)
>  		state->async_update = !drm_atomic_helper_async_check(dev, state);
>  
>  	return ret;
> @@ -1526,13 +1526,16 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  	if (new_plane_state->fence)
>  		return -EINVAL;
>  
> +	/* Only do an async update if there is a pending commit. */
> +	if (!old_plane_state->commit)
> +		return -EINVAL;
> +

I noticed that this change triggers a compiler warning, not sure why, but if I
add the above lines the compiler (gcc version 7.3.0) shows this warning.

drivers/gpu/drm/drm_atomic_helper.c: In function ‘drm_atomic_helper_async_check’:
drivers/gpu/drm/drm_atomic_helper.c:1541:9: warning: ‘new_plane_state’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
  return funcs->atomic_async_check(plane, new_plane_state);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/drm_atomic_helper.c:1541:9: warning: ‘plane’ may be used
uninitialized in this function [-Wmaybe-uninitialized]

Looks a false positive but I'll take a deep look after receive some feedback.

Best regards,
 Enric

>  	/*
>  	 * Don't do an async update if there is an outstanding commit modifying
>  	 * the plane.  This prevents our async update's changes from getting
>  	 * overridden by a previous synchronous update's state.
>  	 */
> -	if (old_plane_state->commit &&
> -	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +	if (!try_wait_for_completion(&old_plane_state->commit->hw_done))
>  		return -EBUSY;
>  
>  	return funcs->atomic_async_check(plane, new_plane_state);
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 50bcf4214ff9..772e84f0edeb 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -718,13 +718,15 @@ struct drm_mode_destroy_dumb {
>  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> +#define DRM_MODE_ATOMIC_ASYNC_UPDATE  0x0800
>  
>  #define DRM_MODE_ATOMIC_FLAGS (\
>  		DRM_MODE_PAGE_FLIP_EVENT |\
>  		DRM_MODE_PAGE_FLIP_ASYNC |\
>  		DRM_MODE_ATOMIC_TEST_ONLY |\
>  		DRM_MODE_ATOMIC_NONBLOCK |\
> -		DRM_MODE_ATOMIC_ALLOW_MODESET)
> +		DRM_MODE_ATOMIC_ALLOW_MODESET |\
> +		DRM_MODE_ATOMIC_ASYNC_UPDATE)
>  
>  struct drm_mode_atomic {
>  	__u32 flags;
>