Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp1055866imu; Thu, 20 Dec 2018 09:22:27 -0800 (PST) X-Google-Smtp-Source: AFSGD/X+QD81Zl1IFxElkaueGmfWSiFpfdGrmB0BAaXpp9di32rCpxP5WW18nNG+9dYKgVgb6U4t X-Received: by 2002:a63:6207:: with SMTP id w7mr23144604pgb.90.1545326547184; Thu, 20 Dec 2018 09:22:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1545326547; cv=none; d=google.com; s=arc-20160816; b=lo999E9+CSaZSDlzqG0ur67Gwkdfsg1V+ZDI4QMfku9uOMl+4miHNEG6lTwvPSLW4p epOobGiwyGF7U1f9s0g2RM41LOWlMikHjDr+lB1ZpEMuXqegvTRMzZ0FYrbGAUi58bYW cGsA7RsLXMUJC6568wH8mi5vZA0uhtrrA62SJMP0PThgY2wJSy6CQh+pevof8+VRHSGb U6Zb0VowO+s+2U/DQm+itRnX7w45qI8bb4PyYeLwpYuSJccL53OveU2mSnDZ6XaFe1qe RnVVrTB7XzNqKWYGeX0ASNPdu6QCBQ9r2ivBRPJkfGXuOPBTA4hWuK4gd4aH/sSnBXA1 lY2w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=rmG4ZlcyvhD9VSp8lwn4+bj/T7QhPo8ECih/FzexAec=; b=tIRpe6NlrB1wrwz/mbYgjIgdLMCWMRN/EzRr/sYa44vxLMoFNPaAZYBzOlhQqeAk9z dYN27/1NhtyAQ1a7uVF+xOdwio2M1oS4flzEsLZjFtoE/ki2GTyzi/eqh+Ifqja9KnQV 2w9AqZXYTok5tO8IEBJZHb/zs+o8iCw4HXUITjog0+U1myarTD63cQPN4mb7IHnbUP6O RZt9W60T9R1KSLuOHC2MH24K70bP99mZfE7s+qBQ0qpWuQL5nWxdy5Nv5g7WATHLk1rq jiE7ld93k4n6XsBp4Rd6pU8DrTzma5p3BUZeqzrXJ87n4OKOAlMoQtrmldRZEF35NWdn 3XIg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b=SAoSAuNr; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id r2si2290271pgo.483.2018.12.20.09.22.11; Thu, 20 Dec 2018 09:22:27 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b=SAoSAuNr; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387679AbeLTRJo (ORCPT + 99 others); Thu, 20 Dec 2018 12:09:44 -0500 Received: from mail-it1-f193.google.com ([209.85.166.193]:56225 "EHLO mail-it1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730526AbeLTRJo (ORCPT ); Thu, 20 Dec 2018 12:09:44 -0500 Received: by mail-it1-f193.google.com with SMTP id m62so3030946ith.5 for ; Thu, 20 Dec 2018 09:09:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=rmG4ZlcyvhD9VSp8lwn4+bj/T7QhPo8ECih/FzexAec=; b=SAoSAuNrIZHxCGFckGPc6/d//2vpMxD1nqQoAVSSE+bXT+/TY00tTIaIaIsNQXCEBw kEFya+BWaWnp8HYGaOHQOOrmnvkqBF3iWMYCeQbA9YagfFlIPl+mfdGwI1+ybtaMEh0z ReEs+HJKG37GL91qTJ88A/IRBFzsCG4Sk+T/U= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=rmG4ZlcyvhD9VSp8lwn4+bj/T7QhPo8ECih/FzexAec=; b=QIugMGUT+d6eishNKk7+Ga4Tebugv6WrfQFDSSTvR04IWrw9PpbjGJCR5c46eyn6HE BVQmdQL47dw1RDpcnfyzMW/S40s/n/vrnoG1jMICpaAkExrdgp2cVYEol6NQ7+F5R2Xx IxGLstLKBPZ75UvLPVRlxvLMsgTHg8SyaJqniRYZ/botHYYPCZw5h3zSmZDvNcvQiwyc /A9jQKSXEPP2Ei2OEkXT8qrI7KLcIpgEW2sgZ3rt/AvjE0IG9qnFNag1IqVEE/RBRe7+ POV//t7wUm4htzFmVKvv9ZyGnjC1tlSMPafCcNUknPacixCLvlIyqIR1/LMMhGetLYnK nUjQ== X-Gm-Message-State: AA+aEWYHVwuKb+kaSvnSbMnGyb55rxL0VdzJhpVQ0AFFT7ILuAJH+Bvj oZkHAKXw71gVVHtQeib5UjbQFzRd0VUwt8U2pfdliw== X-Received: by 2002:a24:94cb:: with SMTP id j194mr10143370ite.117.1545325782840; Thu, 20 Dec 2018 09:09:42 -0800 (PST) MIME-Version: 1.0 References: <20181123215326.14274-1-helen.koike@collabora.com> <20181127133418.GT9144@intel.com> <6aa39654-6949-88b3-b949-b338d915ffd2@collabora.com> <0a0a35bf-69e4-8dcd-46fc-7053081480d5@collabora.com> In-Reply-To: From: Daniel Vetter Date: Thu, 20 Dec 2018 18:09:31 +0100 Message-ID: Subject: Re: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE To: Alex Deucher Cc: "Wentland, Harry" , "Kazlauskas, Nicholas" , Tomasz Figa , dnicoara@chromium.org, =?UTF-8?Q?St=C3=A9phane_Marchesin?= , Sean Paul , Alexandros Frantzis , David Airlie , Linux Kernel Mailing List , dri-devel , Gustavo Padovan , Helen Koike , kernel@collabora.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher wrote: > > + Harry > > On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter wrote: > > > > On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher wr= ote: > > > > > > + Nicholas > > > > > > On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter wrote= : > > > > > > > > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa w= rote: > > > > > > > > > > Hi Helen, > > > > > > > > > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike wrote: > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > On 12/13/18 2:02 AM, Tomasz Figa wrote: > > > > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike wrote: > > > > > > >> > > > > > > >> Hi Ville > > > > > > >> > > > > > > >> On 11/27/18 11:34 AM, Ville Syrj=C3=A4l=C3=A4 wrote: > > > > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote= : > > > > > > >>>> Allow userspace to identify if the driver supports async u= pdate. > > > > > > >>> > > > > > > >>> And what exactly is an "async update"? > > > > > > >> > > > > > > >> I agree we are lacking docs on this, I'll send in the next v= ersion 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 w= e should > > > > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mai= lbox) to > > > > > > >>> avoid introducing yet another set of names for the same thi= ng. We'd > > > > > > >>> still want to document things properly though. > > > > > > >> > > > > > > >> Another name it was suggested was "atomic amend", this featu= re 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 "ato= mic amend"? > > > > > > > > > > > > > > Note that it doesn't seem to be what the code currently is do= ing. 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 ba= ck to a > > > > > > > normal sync commit, if there is an already pending commit tou= ching 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 i= f 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 ena= bled > > > > > > anyway. So the following code is wrong: > > > > > > > > > > > > - if (state->legacy_cursor_update) > > > > > > + if (state->async_update || state->legacy_cursor_update) > > > > > > state->async_update =3D !drm_atomic_helper_asyn= c_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 su= re > > > > if we want to support having a normal atomic commit and a cursor > > > > update in the same atomic ioctl, coming up with reasonable semantic= s > > > > 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 t= o > > > 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=3D106175 > > > 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=3D108917 > > > > Can we get these patches cc'ed to dri-devel so that there's some > > common solution? Everyone hacking legacy_cursor_update hacks on their > > own doesn't really work well. Or would at least give some visibility > > into what's all going on. > > Agreed. Bit more detail: The legacy_cursor_update hacks all over is probably the worst part of atomic, and everyone seems unhappy with it. Except all efforts to address it fall short by a lot. I think Gustavo from Collabora once had a patch series, but it only ever got merged partially, and now we're back with a slightly different pick of color it seems. Hence why I'm somewhat grumpy on this here. > > Not sure about the gamma thing since we had opposite bugs on i915 > > about gamma not being vsynced and tearing terribly. Cursor is special > > since it tends to be too small to notice tearing. > > Our cursor hw (and possibly gamma as well Nicholas? Harry?) is double > buffered, so we can update it any time for the most part and the > changes won't take affect until the next vupdate period. Hm, I guess we could make the gamma update optionally async, and let drivers deal. The issue is that the current async helper stuff won't cope with gamma updates (it's aimed at plane updates only, not at crtc property updates). Or we get userspace to do proper atomic updates. Or we do some faking in the kernel, e.g. waiting with the gamma update until the next atomic update happens. But that kinda breaks ->atomic_check. -Daniel > > Alex > > > > > Thanks, Daniel > > > > > Alex > > > > > > > > > > -Daniel > > > > > > > > > Best regards, > > > > > Tomasz > > > > > > > > > > > > > > > > > Helen > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > Tomasz > > > > > > > > > > > > > >> Or do you suggest another name? I am not familiar with vulka= n terminology. > > > > > > >> > > > > > > >> > > > > > > >> Thanks > > > > > > >> Helen > > > > > > >> > > > > > > >>> > > > > > > >>>> > > > > > > >>>> Signed-off-by: Enric Balletbo i Serra > > > > > > >>>> [prepared for upstream] > > > > > > >>>> Signed-off-by: Helen Koike > > > > > > >>>> > > > > > > >>>> --- > > > > > > >>>> Hi, > > > > > > >>>> > > > > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originat= ed from the > > > > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allo= w user to > > > > > > >>>> figure that async_update exists. > > > > > > >>>> > > > > > > >>>> This was tested using a small program that exercises the u= API 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 m= ainline 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/comm= its/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 > > > > > > >>>> #include > > > > > > >>>> #include > > > > > > >>>> +#include > > > > > > >>>> #include "drm_legacy.h" > > > > > > >>>> #include "drm_internal.h" > > > > > > >>>> #include "drm_crtc_internal.h" > > > > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_devic= e *dev, void *data, struct drm_file *file_ > > > > > > >>>> { > > > > > > >>>> struct drm_get_cap *req =3D data; > > > > > > >>>> struct drm_crtc *crtc; > > > > > > >>>> + struct drm_plane *plane; > > > > > > >>>> > > > > > > >>>> req->value =3D 0; > > > > > > >>>> > > > > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_devi= ce *dev, void *data, struct drm_file *file_ > > > > > > >>>> case DRM_CAP_CRTC_IN_VBLANK_EVENT: > > > > > > >>>> req->value =3D 1; > > > > > > >>>> break; > > > > > > >>>> + case DRM_CAP_ASYNC_UPDATE: > > > > > > >>>> + req->value =3D 1; > > > > > > >>>> + list_for_each_entry(plane, &dev->mode_config.= plane_list, head) { > > > > > > >>>> + if (!plane->helper_private->atomic_as= ync_update) { > > > > > > >>>> + req->value =3D 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@lists.freedesktop.org > > > > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > >>> > > > > > _______________________________________________ > > > > > dri-devel mailing list > > > > > dri-devel@lists.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 > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.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 > > > > On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher wr= ote: > > > > > > + Nicholas > > > > > > On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter wrote= : > > > > > > > > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa w= rote: > > > > > > > > > > Hi Helen, > > > > > > > > > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike wrote: > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > On 12/13/18 2:02 AM, Tomasz Figa wrote: > > > > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike wrote: > > > > > > >> > > > > > > >> Hi Ville > > > > > > >> > > > > > > >> On 11/27/18 11:34 AM, Ville Syrj=C3=A4l=C3=A4 wrote: > > > > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote= : > > > > > > >>>> Allow userspace to identify if the driver supports async u= pdate. > > > > > > >>> > > > > > > >>> And what exactly is an "async update"? > > > > > > >> > > > > > > >> I agree we are lacking docs on this, I'll send in the next v= ersion 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 w= e should > > > > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mai= lbox) to > > > > > > >>> avoid introducing yet another set of names for the same thi= ng. We'd > > > > > > >>> still want to document things properly though. > > > > > > >> > > > > > > >> Another name it was suggested was "atomic amend", this featu= re 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 "ato= mic amend"? > > > > > > > > > > > > > > Note that it doesn't seem to be what the code currently is do= ing. 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 ba= ck to a > > > > > > > normal sync commit, if there is an already pending commit tou= ching 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 i= f 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 ena= bled > > > > > > anyway. So the following code is wrong: > > > > > > > > > > > > - if (state->legacy_cursor_update) > > > > > > + if (state->async_update || state->legacy_cursor_update) > > > > > > state->async_update =3D !drm_atomic_helper_asyn= c_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 su= re > > > > if we want to support having a normal atomic commit and a cursor > > > > update in the same atomic ioctl, coming up with reasonable semantic= s > > > > 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 t= o > > > 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=3D106175 > > > 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=3D108917 > > > > > > Alex > > > > > > > > > > -Daniel > > > > > > > > > Best regards, > > > > > Tomasz > > > > > > > > > > > > > > > > > Helen > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > Tomasz > > > > > > > > > > > > > >> Or do you suggest another name? I am not familiar with vulka= n terminology. > > > > > > >> > > > > > > >> > > > > > > >> Thanks > > > > > > >> Helen > > > > > > >> > > > > > > >>> > > > > > > >>>> > > > > > > >>>> Signed-off-by: Enric Balletbo i Serra > > > > > > >>>> [prepared for upstream] > > > > > > >>>> Signed-off-by: Helen Koike > > > > > > >>>> > > > > > > >>>> --- > > > > > > >>>> Hi, > > > > > > >>>> > > > > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originat= ed from the > > > > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allo= w user to > > > > > > >>>> figure that async_update exists. > > > > > > >>>> > > > > > > >>>> This was tested using a small program that exercises the u= API 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 m= ainline 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/comm= its/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 > > > > > > >>>> #include > > > > > > >>>> #include > > > > > > >>>> +#include > > > > > > >>>> #include "drm_legacy.h" > > > > > > >>>> #include "drm_internal.h" > > > > > > >>>> #include "drm_crtc_internal.h" > > > > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_devic= e *dev, void *data, struct drm_file *file_ > > > > > > >>>> { > > > > > > >>>> struct drm_get_cap *req =3D data; > > > > > > >>>> struct drm_crtc *crtc; > > > > > > >>>> + struct drm_plane *plane; > > > > > > >>>> > > > > > > >>>> req->value =3D 0; > > > > > > >>>> > > > > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_devi= ce *dev, void *data, struct drm_file *file_ > > > > > > >>>> case DRM_CAP_CRTC_IN_VBLANK_EVENT: > > > > > > >>>> req->value =3D 1; > > > > > > >>>> break; > > > > > > >>>> + case DRM_CAP_ASYNC_UPDATE: > > > > > > >>>> + req->value =3D 1; > > > > > > >>>> + list_for_each_entry(plane, &dev->mode_config.= plane_list, head) { > > > > > > >>>> + if (!plane->helper_private->atomic_as= ync_update) { > > > > > > >>>> + req->value =3D 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@lists.freedesktop.org > > > > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > >>> > > > > > _______________________________________________ > > > > > dri-devel mailing list > > > > > dri-devel@lists.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 > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.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 --=20 Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch