+ 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
