diff mbox

[RFC] drm/atomic: add ASYNC_UPDATE flag to the Atomic IOCTL.

Message ID 20180627212506.24061-1-enric.balletbo@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Enric Balletbo i Serra June 27, 2018, 9:25 p.m. UTC
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(-)

Comments

Enric Balletbo i Serra June 27, 2018, 9:36 p.m. UTC | #1
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;
>
Daniel Vetter June 28, 2018, 8:30 a.m. UTC | #2
On Wed, Jun 27, 2018 at 11:25:06PM +0200, 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>

I think this should also be used to emulate async updates for legacy
page_flip ioctls. That would even avoid the need for userspace, at least
for the internal stuff :-)
-Daniel

> ---
> 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;
> +
>  	/*
>  	 * 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;
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maarten Lankhorst June 28, 2018, 8:31 a.m. UTC | #3
Op 27-06-18 om 23:25 schreef Enric Balletbo i Serra:
> 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;
Do we really want this by default?

Wouldn't it make more sense to only enable it if userspace requests it?
>  
>  	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);
We should probably move it to be a flag only enabled when requested, and reject with the error returned by drm_atomic_helper_async_check() when things fail.

~Maarten
> @@ -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;
> +
>  	/*
>  	 * 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;
Gustavo Padovan June 28, 2018, 12:31 p.m. UTC | #4
Hi Maarten,

On Thu, 2018-06-28 at 10:31 +0200, Maarten Lankhorst wrote:
> Op 27-06-18 om 23:25 schreef Enric Balletbo i Serra:
> > 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/+/10927
> > 11
> > [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;
> 
> Do we really want this by default?
> 
> Wouldn't it make more sense to only enable it if userspace requests
> it?

This value is rewritten when we check for the ASYNC_UPDATE flag, so it 
disable by default. I guess this line could be removed then.

> >  
> >  	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);
> 
> We should probably move it to be a flag only enabled when requested,
> and reject with the error returned by drm_atomic_helper_async_check()
> when things fail.
> 
> ~Maarten
> > @@ -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;
> > +
> >  	/*
> >  	 * 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;
> 
>
Ville Syrjälä June 28, 2018, 1:35 p.m. UTC | #5
On Wed, Jun 27, 2018 at 11:25:06PM +0200, 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.

I still dislike the name. On Intel hw "async flip" means "flip
asap and tear". Whereas the legcay cursor thing i915 has is a normal
sync flip. "unthrottled" or something like that would be less confusing.

As far as introducing this flag, at least i915 totally lacks a
mechanism for deferring the buffer unpinning after the vblank. Hence
this can't be used by i915 currently. I think the only reason we get
away with the cursor hack is that we unbind the vma lazily and it's
unlikely that the small cursor vma is going to get knocked out before
the next vblank. For larger buffers that risk grows. We would probably
want a stress test that smashes the gtt hard while doing "async
updates" to catch this.

There's also the question of how out fences work with the async
updates. I see that you disallow such an update if there's a previous
sync update still pending. That simplifies things a little bit I
suppose. But still you would need to track the fences per-plane and
signal the old fence ones as soon as the new update overrides the
pending update. And if you want to allow multiple planes in one async
update then I think the uapi would need to be changed to have
per-plane fences as well because subsequent updates could override 
only a part of a previous update.

> 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;
> +
>  	/*
>  	 * 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;
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gustavo Padovan July 6, 2018, 10:54 a.m. UTC | #6
Hi Ville,

On Thu, 2018-06-28 at 16:35 +0300, Ville Syrjälä wrote:
> On Wed, Jun 27, 2018 at 11:25:06PM +0200, 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.
> 
> I still dislike the name. On Intel hw "async flip" means "flip
> asap and tear". Whereas the legcay cursor thing i915 has is a normal
> sync flip. "unthrottled" or something like that would be less
> confusing.

Maybe "amend"? However if we submit an amend commit when no commit is
pending it will become a regular atomic commit. 

> 
> As far as introducing this flag, at least i915 totally lacks a
> mechanism for deferring the buffer unpinning after the vblank. Hence
> this can't be used by i915 currently. I think the only reason we get
> away with the cursor hack is that we unbind the vma lazily and it's
> unlikely that the small cursor vma is going to get knocked out before
> the next vblank. For larger buffers that risk grows. We would
> probably
> want a stress test that smashes the gtt hard while doing "async
> updates" to catch this.

Right. We'll look into more detail to this in i915.

> 
> There's also the question of how out fences work with the async
> updates. I see that you disallow such an update if there's a previous
> sync update still pending. That simplifies things a little bit I
> suppose. But still you would need to track the fences per-plane and
> signal the old fence ones as soon as the new update overrides the
> pending update. And if you want to allow multiple planes in one async
> update then I think the uapi would need to be changed to have
> per-plane fences as well because subsequent updates could override 
> only a part of a previous update.

The only usecase (and userspace) we have on our side for now is for
cursors. I'm not sure an uAPI that only work for cursor in the first
stage would be acceptable. That would make things easier.

For updates on other planes or multiples planes I agree with you on
having per-plane fences - but I guess we need a userspace usecase for
it first.

> 
> > 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/+/10927
> > 11
> > [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;
> > +
> >  	/*
> >  	 * 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;
> > -- 
> > 2.18.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
>
Enric Balletbo i Serra July 23, 2018, 7:56 a.m. UTC | #7
Hi Ville,

On 06/07/18 12:54, Gustavo Padovan wrote:
> Hi Ville,
> 
> On Thu, 2018-06-28 at 16:35 +0300, Ville Syrjälä wrote:
>> On Wed, Jun 27, 2018 at 11:25:06PM +0200, 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.
>>
>> I still dislike the name. On Intel hw "async flip" means "flip
>> asap and tear". Whereas the legcay cursor thing i915 has is a normal
>> sync flip. "unthrottled" or something like that would be less
>> confusing.
> 
> Maybe "amend"? However if we submit an amend commit when no commit is
> pending it will become a regular atomic commit. 
> 

This has been following the names we introduced one year ago [1], basically to
solve the legacy cursor hack, some platforms are already using the async funcs
and there is another patch at least in the ML. I agree that async is not the
best name,

>>
>> As far as introducing this flag, at least i915 totally lacks a
>> mechanism for deferring the buffer unpinning after the vblank. Hence
>> this can't be used by i915 currently. I think the only reason we get
>> away with the cursor hack is that we unbind the vma lazily and it's
>> unlikely that the small cursor vma is going to get knocked out before
>> the next vblank. For larger buffers that risk grows. We would
>> probably
>> want a stress test that smashes the gtt hard while doing "async
>> updates" to catch this.
> 
> Right. We'll look into more detail to this in i915.
> 

We only have one use case for this api/flag, it is used to get cursor updates to
bypass pending atomic updates without having to rely on the legacy API for that.
The current behaviour is to amend the pending atomic update to get the cursor on
the next vblank. I believe intel on intel we would do the same despite having
the chance update right away and tear? Or do you see we doing this differently?
Do you have an usecase for flip asap and tear?

Do you see here a need for 2 apis/flags and have a usecase for both? (one for
updating the plane on the hw asap and tear and another one to wait the next vblank).

>>
>> There's also the question of how out fences work with the async
>> updates. I see that you disallow such an update if there's a previous
>> sync update still pending. That simplifies things a little bit I
>> suppose. But still you would need to track the fences per-plane and
>> signal the old fence ones as soon as the new update overrides the
>> pending update. And if you want to allow multiple planes in one async
>> update then I think the uapi would need to be changed to have
>> per-plane fences as well because subsequent updates could override 
>> only a part of a previous update.
> 
> The only usecase (and userspace) we have on our side for now is for
> cursors. I'm not sure an uAPI that only work for cursor in the first
> stage would be acceptable. That would make things easier.
> 
> For updates on other planes or multiples planes I agree with you on
> having per-plane fences - but I guess we need a userspace usecase for
> it first.
> 

<snip>

Best regards,
  Enric

[1] fef9df8b59453 ("drm/atomic: initial support for asynchronous plane update")
diff mbox

Patch

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;
+
 	/*
 	 * 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;