[PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[Helen Koike]

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. Note that for now it's only enabled for cursor
planes.

DRM_MODE_ATOMIC_AMEND 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>
[updated for upstream]
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
Hi,

This is the second attempt to introduce the new ATOMIC_AMEND flag for atomic
operations, see the commit message for a more detailed description.

This was tested using a small program that exercises the uAPI for easy
sanity testing. The program was created by Alexandros and modified by
Enric to test the capability flag [2].

To test, just build the program and use the --atomic flag to use the cursor
plane with the ATOMIC_AMEND flag. E.g.

  drm_cursor --atomic

The test worked on a rockchip Ficus v1.1 board on top of mainline plus
the patch to update cursors asynchronously through atomic for the
drm/rockchip driver plus the DRM_CAP_ASYNC_UPDATE patch.

Alexandros also did a proof-of-concept to use this flag and draw cursors
using atomic if possible on ozone [1].

Thanks
Helen

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
[2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability


Changes in v2:
- rebase tree
- do not fall back to a non-async update if if there isn't any
pending commit to amend

Changes in v1:
- https://patchwork.freedesktop.org/patch/243088/
- Only enable it if userspace requests it.
- Only allow async update for cursor type planes.
- Rename ASYNC_UPDATE for ATOMIC_AMEND.

 drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
 drivers/gpu/drm/drm_atomic_uapi.c   | 6 ++++++
 include/uapi/drm/drm_mode.h         | 4 +++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 269f1a74de38..333190c6a0a4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -934,7 +934,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;
@@ -1602,6 +1602,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 	if (new_plane_state->fence)
 		return -EINVAL;
 
+	/* Only allow async update for cursor type planes. */
+	if (plane->type != DRM_PLANE_TYPE_CURSOR)
+		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
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f315098c..d5d26fe85ecf 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -28,6 +28,7 @@
 
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_atomic.h>
+#include <drm/drm_atomic_uapi.h>
 #include <drm/drm_print.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_writeback.h>
@@ -1275,6 +1276,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_AMEND))
+		return -EINVAL;
+
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 
 	state = drm_atomic_state_alloc(dev);
@@ -1283,6 +1288,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_AMEND);
 
 retry:
 	copied_objs = 0;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index d3e0fe31efc5..0adb28d27e9e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -733,13 +733,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_AMEND 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_AMEND)
 
 struct drm_mode_atomic {
 	__u32 flags;
-- 
2.19.1

Re: [PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[Tomasz Figa]

Hi Helen,

On Sat, Nov 24, 2018 at 6:54 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> 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.

First of all, thanks for the patch. Please see my comments below.

If the description above applies (and AFAICT that's what the existing
code does indeed), then this doesn't sound like "amend" to me. It
sounds exactly as the kernel code calls it - "async update" or perhaps
"instantaneous commit" could better describe it?

>
> 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. Note that for now it's only enabled for cursor
> planes.
>
> DRM_MODE_ATOMIC_AMEND 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>
> [updated for upstream]
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> Hi,
>
> This is the second attempt to introduce the new ATOMIC_AMEND flag for atomic
> operations, see the commit message for a more detailed description.
>
> This was tested using a small program that exercises the uAPI for easy
> sanity testing. The program was created by Alexandros and modified by
> Enric to test the capability flag [2].
>
> To test, just build the program and use the --atomic flag to use the cursor
> plane with the ATOMIC_AMEND flag. E.g.
>
>   drm_cursor --atomic
>
> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> the patch to update cursors asynchronously through atomic for the
> drm/rockchip driver plus the DRM_CAP_ASYNC_UPDATE patch.
>
> Alexandros also did a proof-of-concept to use this flag and draw cursors
> using atomic if possible on ozone [1].
>
> Thanks
> Helen
>
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>
>
> Changes in v2:
> - rebase tree
> - do not fall back to a non-async update if if there isn't any
> pending commit to amend
>
> Changes in v1:
> - https://patchwork.freedesktop.org/patch/243088/
> - Only enable it if userspace requests it.
> - Only allow async update for cursor type planes.
> - Rename ASYNC_UPDATE for ATOMIC_AMEND.
>
>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
>  drivers/gpu/drm/drm_atomic_uapi.c   | 6 ++++++
>  include/uapi/drm/drm_mode.h         | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 269f1a74de38..333190c6a0a4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -934,7 +934,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;
> @@ -1602,6 +1602,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>         if (new_plane_state->fence)
>                 return -EINVAL;
>
> +       /* Only allow async update for cursor type planes. */
> +       if (plane->type != DRM_PLANE_TYPE_CURSOR)
> +               return -EINVAL;
> +

So the existing upstream code already allowed this for any planes and
we're restricting this to cursor planes only. Is this expected? No
potential users that already started using this for other plane types?

Best regards,
Tomasz

Re: [PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[Daniel Vetter]

On Thu, Dec 13, 2018 at 04:43:57PM +0900, Tomasz Figa wrote:
> Hi Helen,
> 
> On Sat, Nov 24, 2018 at 6:54 AM Helen Koike <helen.koike@collabora.com> wrote:
> >
> > 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.
> 
> First of all, thanks for the patch. Please see my comments below.
> 
> If the description above applies (and AFAICT that's what the existing
> code does indeed), then this doesn't sound like "amend" to me. It
> sounds exactly as the kernel code calls it - "async update" or perhaps
> "instantaneous commit" could better describe it?
> 
> >
> > 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. Note that for now it's only enabled for cursor
> > planes.
> >
> > DRM_MODE_ATOMIC_AMEND 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>
> > [updated for upstream]
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > ---
> > Hi,
> >
> > This is the second attempt to introduce the new ATOMIC_AMEND flag for atomic
> > operations, see the commit message for a more detailed description.
> >
> > This was tested using a small program that exercises the uAPI for easy
> > sanity testing. The program was created by Alexandros and modified by
> > Enric to test the capability flag [2].
> >
> > To test, just build the program and use the --atomic flag to use the cursor
> > plane with the ATOMIC_AMEND flag. E.g.
> >
> >   drm_cursor --atomic
> >
> > The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > the patch to update cursors asynchronously through atomic for the
> > drm/rockchip driver plus the DRM_CAP_ASYNC_UPDATE patch.
> >
> > Alexandros also did a proof-of-concept to use this flag and draw cursors
> > using atomic if possible on ozone [1].
> >
> > Thanks
> > Helen
> >
> > [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
> > [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> >
> >
> > Changes in v2:
> > - rebase tree
> > - do not fall back to a non-async update if if there isn't any
> > pending commit to amend
> >
> > Changes in v1:
> > - https://patchwork.freedesktop.org/patch/243088/
> > - Only enable it if userspace requests it.
> > - Only allow async update for cursor type planes.
> > - Rename ASYNC_UPDATE for ATOMIC_AMEND.
> >
> >  drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
> >  drivers/gpu/drm/drm_atomic_uapi.c   | 6 ++++++
> >  include/uapi/drm/drm_mode.h         | 4 +++-
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 269f1a74de38..333190c6a0a4 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -934,7 +934,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;
> > @@ -1602,6 +1602,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> >         if (new_plane_state->fence)
> >                 return -EINVAL;
> >
> > +       /* Only allow async update for cursor type planes. */
> > +       if (plane->type != DRM_PLANE_TYPE_CURSOR)
> > +               return -EINVAL;
> > +
> 
> So the existing upstream code already allowed this for any planes and
> we're restricting this to cursor planes only. Is this expected? No
> potential users that already started using this for other plane types?

The backend supports it for anything right now (if the driver implements
it, that is). We do expose it through the legacy cursor api, and the
legacy page_flip api, but not through atomic itself. It also has the
problem that the not all drivers who support the async legacy cursor mode
in atomic use this new infrastructure, so there's a few problems. Plus
semantics are very ill-defined (we'd definitely need igt testcases for
this stuff, especially all the new combinations with events, fences, ...).

I think what we'd need here to make sure we're not digging an uapi hole:

1. Entirely remove the legacy_cursor_update hack. There's some patches
floating around, but would need to be polished.

2. Make sure all drivers supporting legacy async cursor updates do through
the async_plane update infrastructure.

3. Get the async plane update stuff merged for amdgpu. Iirc that's still
stuck somewhere (but I'm not 100% sure what exactly they're doing).

4. Pile of igt testcases for all the new corner cases exposing this in
atomic opens up. Many cases we might want to simply reject it.

5. Userspace. Big one I have is whether we need a flag like ALLOW_MODESET,
since the current code transparently falls back to vblank-synced updates
if async updates aren't available.

tldr; lots of work. Also maybe:

0. Dump this todo into Documentation/gpu/todo.rst so it won't get lost.

Cheers, Daniel


> 
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[Helen Koike]

Hello,

On 12/13/18 7:01 AM, Daniel Vetter wrote:
> On Thu, Dec 13, 2018 at 04:43:57PM +0900, Tomasz Figa wrote:
>> Hi Helen,
>>
>> On Sat, Nov 24, 2018 at 6:54 AM Helen Koike <helen.koike@collabora.com> wrote:
>>>
>>> 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.
>>
>> First of all, thanks for the patch. Please see my comments below.
>>
>> If the description above applies (and AFAICT that's what the existing
>> code does indeed), then this doesn't sound like "amend" to me. It
>> sounds exactly as the kernel code calls it - "async update" or perhaps
>> "instantaneous commit" could better describe it?

There is an error in this patch (please, see below).
Async should fail if there is no pending commit, at least is what I
understand from the discussion at
https://patchwork.freedesktop.org/patch/243088/

>>
>>>
>>> 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. Note that for now it's only enabled for cursor
>>> planes.
>>>
>>> DRM_MODE_ATOMIC_AMEND 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>
>>> [updated for upstream]
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>> Hi,
>>>
>>> This is the second attempt to introduce the new ATOMIC_AMEND flag for atomic
>>> operations, see the commit message for a more detailed description.
>>>
>>> This was tested using a small program that exercises the uAPI for easy
>>> sanity testing. The program was created by Alexandros and modified by
>>> Enric to test the capability flag [2].
>>>
>>> To test, just build the program and use the --atomic flag to use the cursor
>>> plane with the ATOMIC_AMEND flag. E.g.
>>>
>>>   drm_cursor --atomic
>>>
>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
>>> the patch to update cursors asynchronously through atomic for the
>>> drm/rockchip driver plus the DRM_CAP_ASYNC_UPDATE patch.
>>>
>>> Alexandros also did a proof-of-concept to use this flag and draw cursors
>>> using atomic if possible on ozone [1].
>>>
>>> Thanks
>>> Helen
>>>
>>> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>>>
>>>
>>> Changes in v2:
>>> - rebase tree
>>> - do not fall back to a non-async update if if there isn't any
>>> pending commit to amend
>>>
>>> Changes in v1:
>>> - https://patchwork.freedesktop.org/patch/243088/
>>> - Only enable it if userspace requests it.
>>> - Only allow async update for cursor type planes.
>>> - Rename ASYNC_UPDATE for ATOMIC_AMEND.
>>>
>>>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
>>>  drivers/gpu/drm/drm_atomic_uapi.c   | 6 ++++++
>>>  include/uapi/drm/drm_mode.h         | 4 +++-
>>>  3 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 269f1a74de38..333190c6a0a4 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -934,7 +934,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);

I just realized this is wrong, drm_atomic_helper_async_check() should
return error if there is a pending old_plane_state->commit (this v2
patch is not doing this, but v1 was), if drm_atomic_helper_async_check()
returned because of it, then we should return error here to scale this
failure to userspace. Make sense? Tomasz, do you agree?

>>>
>>>         return ret;
>>> @@ -1602,6 +1602,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>>         if (new_plane_state->fence)
>>>                 return -EINVAL;
>>>
>>> +       /* Only allow async update for cursor type planes. */
>>> +       if (plane->type != DRM_PLANE_TYPE_CURSOR)
>>> +               return -EINVAL;
>>> +
>>
>> So the existing upstream code already allowed this for any planes and
>> we're restricting this to cursor planes only. Is this expected? No
>> potential users that already started using this for other plane types?
> 
> The backend supports it for anything right now (if the driver implements
> it, that is). We do expose it through the legacy cursor api, and the
> legacy page_flip api, but not through atomic itself. It also has the
> problem that the not all drivers who support the async legacy cursor mode
> in atomic use this new infrastructure, so there's a few problems. Plus
> semantics are very ill-defined (we'd definitely need igt testcases for
> this stuff, especially all the new combinations with events, fences, ...).
> 
> I think what we'd need here to make sure we're not digging an uapi hole:
> 
> 1. Entirely remove the legacy_cursor_update hack. There's some patches
> floating around, but would need to be polished.
> 
> 2. Make sure all drivers supporting legacy async cursor updates do through
> the async_plane update infrastructure.
> 
> 3. Get the async plane update stuff merged for amdgpu. Iirc that's still
> stuck somewhere (but I'm not 100% sure what exactly they're doing).
> 
> 4. Pile of igt testcases for all the new corner cases exposing this in
> atomic opens up. Many cases we might want to simply reject it.
> 
> 5. Userspace. Big one I have is whether we need a flag like ALLOW_MODESET,
> since the current code transparently falls back to vblank-synced updates
> if async updates aren't available.
> 
> tldr; lots of work. Also maybe:
> 
> 0. Dump this todo into Documentation/gpu/todo.rst so it won't get lost.
> 
> Cheers, Daniel
> 

Thanks Daniel for pointing those out, I'll start to take a look on them.

> 
>>
>> Best regards,
>> Tomasz
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 


Thanks
Helen

Re: [PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[Daniel Vetter]

On Thu, Dec 13, 2018 at 11:35:33PM -0200, Helen Koike wrote:
> Hello,
> 
> On 12/13/18 7:01 AM, Daniel Vetter wrote:
> > On Thu, Dec 13, 2018 at 04:43:57PM +0900, Tomasz Figa wrote:
> >> Hi Helen,
> >>
> >> On Sat, Nov 24, 2018 at 6:54 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>>
> >>> 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.
> >>
> >> First of all, thanks for the patch. Please see my comments below.
> >>
> >> If the description above applies (and AFAICT that's what the existing
> >> code does indeed), then this doesn't sound like "amend" to me. It
> >> sounds exactly as the kernel code calls it - "async update" or perhaps
> >> "instantaneous commit" could better describe it?
> 
> There is an error in this patch (please, see below).
> Async should fail if there is no pending commit, at least is what I
> understand from the discussion at
> https://patchwork.freedesktop.org/patch/243088/

Hm, that's not really my takeaway from that discussion. There's lots of
open issues with this uapi (too many real-world corner cases that aren't
thought through yet), but that conclusion I didn't find anywhere ...
-Daniel

> 
> >>
> >>>
> >>> 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. Note that for now it's only enabled for cursor
> >>> planes.
> >>>
> >>> DRM_MODE_ATOMIC_AMEND 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>
> >>> [updated for upstream]
> >>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>> ---
> >>> Hi,
> >>>
> >>> This is the second attempt to introduce the new ATOMIC_AMEND flag for atomic
> >>> operations, see the commit message for a more detailed description.
> >>>
> >>> This was tested using a small program that exercises the uAPI for easy
> >>> sanity testing. The program was created by Alexandros and modified by
> >>> Enric to test the capability flag [2].
> >>>
> >>> To test, just build the program and use the --atomic flag to use the cursor
> >>> plane with the ATOMIC_AMEND flag. E.g.
> >>>
> >>>   drm_cursor --atomic
> >>>
> >>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> >>> the patch to update cursors asynchronously through atomic for the
> >>> drm/rockchip driver plus the DRM_CAP_ASYNC_UPDATE patch.
> >>>
> >>> Alexandros also did a proof-of-concept to use this flag and draw cursors
> >>> using atomic if possible on ozone [1].
> >>>
> >>> Thanks
> >>> Helen
> >>>
> >>> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
> >>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> >>>
> >>>
> >>> Changes in v2:
> >>> - rebase tree
> >>> - do not fall back to a non-async update if if there isn't any
> >>> pending commit to amend
> >>>
> >>> Changes in v1:
> >>> - https://patchwork.freedesktop.org/patch/243088/
> >>> - Only enable it if userspace requests it.
> >>> - Only allow async update for cursor type planes.
> >>> - Rename ASYNC_UPDATE for ATOMIC_AMEND.
> >>>
> >>>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
> >>>  drivers/gpu/drm/drm_atomic_uapi.c   | 6 ++++++
> >>>  include/uapi/drm/drm_mode.h         | 4 +++-
> >>>  3 files changed, 14 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>> index 269f1a74de38..333190c6a0a4 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>> @@ -934,7 +934,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);
> 
> I just realized this is wrong, drm_atomic_helper_async_check() should
> return error if there is a pending old_plane_state->commit (this v2
> patch is not doing this, but v1 was), if drm_atomic_helper_async_check()
> returned because of it, then we should return error here to scale this
> failure to userspace. Make sense? Tomasz, do you agree?
> 
> >>>
> >>>         return ret;
> >>> @@ -1602,6 +1602,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> >>>         if (new_plane_state->fence)
> >>>                 return -EINVAL;
> >>>
> >>> +       /* Only allow async update for cursor type planes. */
> >>> +       if (plane->type != DRM_PLANE_TYPE_CURSOR)
> >>> +               return -EINVAL;
> >>> +
> >>
> >> So the existing upstream code already allowed this for any planes and
> >> we're restricting this to cursor planes only. Is this expected? No
> >> potential users that already started using this for other plane types?
> > 
> > The backend supports it for anything right now (if the driver implements
> > it, that is). We do expose it through the legacy cursor api, and the
> > legacy page_flip api, but not through atomic itself. It also has the
> > problem that the not all drivers who support the async legacy cursor mode
> > in atomic use this new infrastructure, so there's a few problems. Plus
> > semantics are very ill-defined (we'd definitely need igt testcases for
> > this stuff, especially all the new combinations with events, fences, ...).
> > 
> > I think what we'd need here to make sure we're not digging an uapi hole:
> > 
> > 1. Entirely remove the legacy_cursor_update hack. There's some patches
> > floating around, but would need to be polished.
> > 
> > 2. Make sure all drivers supporting legacy async cursor updates do through
> > the async_plane update infrastructure.
> > 
> > 3. Get the async plane update stuff merged for amdgpu. Iirc that's still
> > stuck somewhere (but I'm not 100% sure what exactly they're doing).
> > 
> > 4. Pile of igt testcases for all the new corner cases exposing this in
> > atomic opens up. Many cases we might want to simply reject it.
> > 
> > 5. Userspace. Big one I have is whether we need a flag like ALLOW_MODESET,
> > since the current code transparently falls back to vblank-synced updates
> > if async updates aren't available.
> > 
> > tldr; lots of work. Also maybe:
> > 
> > 0. Dump this todo into Documentation/gpu/todo.rst so it won't get lost.
> > 
> > Cheers, Daniel
> > 
> 
> Thanks Daniel for pointing those out, I'll start to take a look on them.
> 
> > 
> >>
> >> Best regards,
> >> Tomasz
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> 
> Thanks
> Helen
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch