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;
MIME-Version: 1.0
References: <20181123215354.14540-1-helen.koike@collabora.com>
In-Reply-To: <20181123215354.14540-1-helen.koike@collabora.com>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Thu, 13 Dec 2018 16:43:57 +0900
Message-ID: <CAAFQd5BHPjode=KqEHs=eNqqCe-xvsqx1ko9hQn29djFxuSWUA@mail.gmail.com>
Subject: Re: [PATCH v2] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.
To:     helen.koike@collabora.com
Cc:     David Airlie <airlied@linux.ie>, dnicoara@chromium.org,
        Alexandros Frantzis <alexandros.frantzis@collabora.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Gustavo Padovan <gustavo.padovan@collabora.com>,
        Sean Paul <seanpaul@google.com>, kernel@collabora.com,
        =?UTF-8?Q?St=C3=A9phane_Marchesin?= <marcheu@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Helen,

On Sat, Nov 24, 2018 at 6:54 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> This flag tells core to jump ahead the queued update if the conditions
> in drm_atomic_async_check() are met. That means we are only able to do an
> async update if no modeset is pending and update for the same plane is
> not queued.

First of all, thanks for the patch. Please see my comments below.

If the description above applies (and AFAICT that's what the existing
code does indeed), then this doesn't sound like "amend" to me. It
sounds exactly as the kernel code calls it - "async update" or perhaps
"instantaneous commit" could better describe it?

>
> It uses the already in place infrastructure for async updates.
>
> It is useful for cursor updates and async PageFlips over the atomic
> ioctl, otherwise in some cases updates may be delayed to the point the
> user will notice it. Note that for now it's only enabled for cursor
> planes.
>
> DRM_MODE_ATOMIC_AMEND should be passed to the Atomic IOCTL to use this
> feature.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> [updated for upstream]
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> Hi,
>
> This is the second attempt to introduce the new ATOMIC_AMEND flag for atomic
> operations, see the commit message for a more detailed description.
>
> 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].
>
> 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
>
> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> the patch to update cursors asynchronously through atomic for the
> drm/rockchip driver plus the DRM_CAP_ASYNC_UPDATE patch.
>
> Alexandros also did a proof-of-concept to use this flag and draw cursors
> using atomic if possible on ozone [1].
>
> Thanks
> Helen
>
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>
>
> Changes in v2:
> - rebase tree
> - do not fall back to a non-async update if if there isn't any
> pending commit to amend
>
> Changes in v1:
> - https://patchwork.freedesktop.org/patch/243088/
> - Only enable it if userspace requests it.
> - Only allow async update for cursor type planes.
> - Rename ASYNC_UPDATE for ATOMIC_AMEND.
>
>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++++-
>  drivers/gpu/drm/drm_atomic_uapi.c   | 6 ++++++
>  include/uapi/drm/drm_mode.h         | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 269f1a74de38..333190c6a0a4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -934,7 +934,7 @@ int drm_atomic_helper_check(struct drm_device *dev,
>         if (ret)
>                 return ret;
>
> -       if (state->legacy_cursor_update)
> +       if (state->async_update || state->legacy_cursor_update)
>                 state->async_update = !drm_atomic_helper_async_check(dev, state);
>
>         return ret;
> @@ -1602,6 +1602,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>         if (new_plane_state->fence)
>                 return -EINVAL;
>
> +       /* Only allow async update for cursor type planes. */
> +       if (plane->type != DRM_PLANE_TYPE_CURSOR)
> +               return -EINVAL;
> +

So the existing upstream code already allowed this for any planes and
we're restricting this to cursor planes only. Is this expected? No
potential users that already started using this for other plane types?

Best regards,
Tomasz