Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE

From: Alex Deucher
Date: Thu Dec 20 2018 - 11:40:51 EST

Next message: Michael Mueller: "Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()"
Previous message: Lars Ellenberg: "[PATCH 11/17] drbd: avoid spurious self-outdating with concurrent disconnect / down"
In reply to: Daniel Vetter: "Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE"
Next in thread: Daniel Vetter: "Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

+ Nicholas

On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> >
> > Hi Helen,
> >
> > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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.

I haven't followed this thread too closely, but we ended up needing to
add a fast patch for cursor updates to amdgpu's atomic support to
avoid stuttering issues. Other drivers may end up being affected by
this as well. See:
https://bugs.freedesktop.org/show_bug.cgi?id=106175
Unfortunately, the fast path requires some hacks to handle the ref
counting properly on cursor fbs:
https://patchwork.freedesktop.org/patch/266138/
https://patchwork.freedesktop.org/patch/268298/
It looks like gamma may need similar treatment:
https://bugs.freedesktop.org/show_bug.cgi?id=108917

Alex

> -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@xxxxxxxxxxxxx>
> > > >>>> [prepared for upstream]
> > > >>>> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
> > > >>>>
> > > >>>> ---
> > > >>>> 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@xxxxxxxxxxxxxxxxxxxxx
> > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >>>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Next message: Michael Mueller: "Re: [PATCH v5 05/15] KVM: s390: unify pending_irqs() and pending_irqs_no_gisa()"
Previous message: Lars Ellenberg: "[PATCH 11/17] drbd: avoid spurious self-outdating with concurrent disconnect / down"
In reply to: Daniel Vetter: "Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE"
Next in thread: Daniel Vetter: "Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]