Message-ID: <20181217093209.GX21184@phenom.ffwll.local>
Date:   Mon, 17 Dec 2018 10:32:09 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Helen Koike <helen.koike@...labora.com>
Cc:     Tomasz Figa <tfiga@...omium.org>, dnicoara@...omium.org,
        Stéphane Marchesin <marcheu@...gle.com>,
        Sean Paul <seanpaul@...gle.com>,
        Alexandros Frantzis <alexandros.frantzis@...labora.com>,
        David Airlie <airlied@...ux.ie>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Gustavo Padovan <gustavo.padovan@...labora.com>,
        kernel@...labora.com
Subject: Re: [PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

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@...labora.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@...labora.com>
> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> >>> [updated for upstream]
> >>> Signed-off-by: Helen Koike <helen.koike@...labora.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@...ts.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> 
> Thanks
> Helen
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Powered by blists - more mailing lists