Message-ID: <CAKMK7uG=vdaTo25b3gSaD4T2naA51VeaVL0+5YAJ9HMrBziTWQ@mail.gmail.com>
Date:   Thu, 20 Dec 2018 11:47:14 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Tomasz Figa <tfiga@...omium.org>
Cc:     Helen Koike <helen.koike@...labora.com>, 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] drm: add capability DRM_CAP_ASYNC_UPDATE

On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@...omium.org> wrote:
>
> Hi Helen,
>
> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@...labora.com> wrote:
> >
> > Hi Tomasz,
> >
> > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@...labora.com> wrote:
> > >>
> > >> Hi Ville
> > >>
> > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > >>>> Allow userspace to identify if the driver supports async update.
> > >>>
> > >>> And what exactly is an "async update"?
> > >>
> > >> I agree we are lacking docs on this, I'll send in the next version as
> > >> soon as we agree on a name (please see below).
> > >>
> > >> For reference:
> > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > >>
> > >>>
> > >>> I keep asking people to come up with the a better name for this, and to
> > >>> document what it actually means. Recently I've been think we should
> > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > >>> avoid introducing yet another set of names for the same thing. We'd
> > >>> still want to document things properly though.
> > >>
> > >> Another name it was suggested was "atomic amend", this feature basically
> > >> allows userspace to complement an update previously sent (i.e. its in
> > >> the queue and wasn't commited yet), it allows adding a plane update to
> > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > >
> > > Note that it doesn't seem to be what the code currently is doing. For
> > > example, for cursor updates, it doesn't seem to be working on the
> > > currently pending commit, but just directly issues an atomic async
> > > update call to the planes. The code actually seems to fall back to a
> > > normal sync commit, if there is an already pending commit touching the
> > > same plane or including a modeset.
> >
> > It should fail as discussed at:
> > https://patchwork.freedesktop.org/patch/243088/
> >
> > There was the following code inside the drm_atomic_helper_async_check()
> > in the previous patch which would fallback to a normal commit if there
> > isn't any pending commit to amend:
> >
> > +       if (!old_plane_state->commit)
> > +               return -EINVAL;
> >
> > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > this got removed, but which means that async update will be enabled
> > anyway. So the following code is wrong:
> >
> > -       if (state->legacy_cursor_update)
> > +       if (state->async_update || state->legacy_cursor_update)
> >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> >
> > Does it make sense? If yes I'll fix this in the next version of the
> > Atomic IOCTL patch (and also those two patches should be in the same
> > series, I'll send them together next time).
> >
> > Thanks for pointing this out.
> >
> > Please let me know if you still don't agree on the name "atomic amend",
> > or if I am missing something.
>
> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> need a way to commit the cursor plane asynchronously from other
> commits any time the cursor changes its position or framebuffer. As
> long as the new API allows that and the maintainers are fine with it,
> I think I should be okay with it too.

If this is just about the cursor, why is the current legacy cursor
ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
if we want to support having a normal atomic commit and a cursor
update in the same atomic ioctl, coming up with reasonable semantics
for that will be complicated.

Pointer to code that uses this would be better ofc.
-Daniel

> Best regards,
> Tomasz
>
> >
> > Helen
> >
> > >
> > > Best regards,
> > > Tomasz
> > >
> > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > >>
> > >>
> > >> Thanks
> > >> Helen
> > >>
> > >>>
> > >>>>
> > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> > >>>> [prepared for upstream]
> > >>>> Signed-off-by: Helen Koike <helen.koike@...labora.com>
> > >>>>
> > >>>> ---
> > >>>> Hi,
> > >>>>
> > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > >>>> figure that async_update exists.
> > >>>>
> > >>>> 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].
> > >>>>
> > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > >>>>
> > >>>> 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
> > >>>>
> > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > >>>>
> > >>>> Thanks
> > >>>> Helen
> > >>>>
> > >>>>
> > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > >>>>  include/uapi/drm/drm.h      |  1 +
> > >>>>  2 files changed, 12 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > >>>> @@ -31,6 +31,7 @@
> > >>>>  #include <drm/drm_ioctl.h>
> > >>>>  #include <drm/drmP.h>
> > >>>>  #include <drm/drm_auth.h>
> > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > >>>>  #include "drm_legacy.h"
> > >>>>  #include "drm_internal.h"
> > >>>>  #include "drm_crtc_internal.h"
> > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > >>>>  {
> > >>>>      struct drm_get_cap *req = data;
> > >>>>      struct drm_crtc *crtc;
> > >>>> +    struct drm_plane *plane;
> > >>>>
> > >>>>      req->value = 0;
> > >>>>
> > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > >>>>              req->value = 1;
> > >>>>              break;
> > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > >>>> +            req->value = 1;
> > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > >>>> +                            req->value = 0;
> > >>>> +                            break;
> > >>>> +                    }
> > >>>> +            }
> > >>>> +            break;
> > >>>>      default:
> > >>>>              return -EINVAL;
> > >>>>      }
> > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > >>>> index 300f336633f2..ff01540cbb1d 100644
> > >>>> --- a/include/uapi/drm/drm.h
> > >>>> +++ b/include/uapi/drm/drm.h
> > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > >>>>
> > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > >>>>  struct drm_get_cap {
> > >>>> --
> > >>>> 2.19.1
> > >>>>
> > >>>> _______________________________________________
> > >>>> dri-devel mailing list
> > >>>> dri-devel@...ts.freedesktop.org
> > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Powered by blists - more mailing lists