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. -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
