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

[Helen Koike <helen.koike@collabora.com>]

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