From: Thomas Zimmermann <tzimmermann@suse.de>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>,
daniel@ffwll.ch, dri-devel@lists.freedesktop.org
Cc: "Christian König" <christian.koenig@amd.com>
Subject: Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb
Date: Thu, 28 Apr 2022 09:23:23 +0200 [thread overview]
Message-ID: <877dc0d9-c6c6-022c-20d8-14b33e863934@suse.de> (raw)
In-Reply-To: <20220421191002.2251-1-christian.koenig@amd.com>
[-- Attachment #1.1: Type: text/plain, Size: 8572 bytes --]
Hi
Am 21.04.22 um 21:10 schrieb Christian König:
> drm_gem_plane_helper_prepare_fb() was using
> drm_atomic_set_fence_for_plane() which ignores all implicit fences when an
> explicit fence is already set. That's rather unfortunate when the fb still
> has a kernel fence we need to wait for to avoid presenting garbage on the
> screen.
>
> So instead update the fence in the plane state directly. While at it also
> take care of all potential GEM objects and not just the first one.
>
> Also remove the now unused drm_atomic_set_fence_for_plane() function, new
> drivers should probably use the atomic helpers directly.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 37 ---------------
> drivers/gpu/drm/drm_gem_atomic_helper.c | 63 +++++++++++++++++++++----
> include/drm/drm_atomic_uapi.h | 2 -
> include/drm/drm_plane.h | 4 +-
> 4 files changed, 54 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c6394ba13b24..0f461261b3f3 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> }
> EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>
> -/**
> - * drm_atomic_set_fence_for_plane - set fence for plane
> - * @plane_state: atomic state object for the plane
> - * @fence: dma_fence to use for the plane
> - *
> - * Helper to setup the plane_state fence in case it is not set yet.
> - * By using this drivers doesn't need to worry if the user choose
> - * implicit or explicit fencing.
> - *
> - * This function will not set the fence to the state if it was set
> - * via explicit fencing interfaces on the atomic ioctl. In that case it will
> - * drop the reference to the fence as we are not storing it anywhere.
> - * Otherwise, if &drm_plane_state.fence is not set this function we just set it
> - * with the received implicit fence. In both cases this function consumes a
> - * reference for @fence.
> - *
> - * This way explicit fencing can be used to overrule implicit fencing, which is
> - * important to make explicit fencing use-cases work: One example is using one
> - * buffer for 2 screens with different refresh rates. Implicit fencing will
> - * clamp rendering to the refresh rate of the slower screen, whereas explicit
> - * fence allows 2 independent render and display loops on a single buffer. If a
> - * driver allows obeys both implicit and explicit fences for plane updates, then
> - * it will break all the benefits of explicit fencing.
> - */
> -void
> -drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> - struct dma_fence *fence)
> -{
> - if (plane_state->fence) {
> - dma_fence_put(fence);
> - return;
> - }
> -
> - plane_state->fence = fence;
> -}
> -EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
> -
> /**
> * drm_atomic_set_crtc_for_connector - set CRTC for connector
> * @conn_state: atomic state object for the connector
> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> index a6d89aed0bda..8fc0b42acdff 100644
> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-or-later
>
> #include <linux/dma-resv.h>
> +#include <linux/dma-fence-chain.h>
>
> #include <drm/drm_atomic_state_helper.h>
> #include <drm/drm_atomic_uapi.h>
> @@ -141,25 +142,67 @@
> * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
This comment still refers to the function you just deleted. Maybe the
deleted docs could be integrated here somehow, if still relevant?
> * explicit fencing in atomic modeset updates.
> */
> -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
> +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
> + struct drm_plane_state *state)
We have a 100-character limit. Please leave this on the same line.
> {
> + struct dma_fence *fence = dma_fence_get(state->fence);
> struct drm_gem_object *obj;
I'd declare this variable within the for loop.
> - struct dma_fence *fence;
> + enum dma_resv_usage usage;
> + size_t i;
> int ret;
>
> if (!state->fb)
> return 0;
>
> - obj = drm_gem_fb_get_obj(state->fb, 0);
> - ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
> - if (ret)
> - return ret;
> -
> - /* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
> - * to handle more fences in general for multiple BOs per fb.
> + /*
> + * Only add the kernel fences here if there is already a fence set via
> + * explicit fencing interfaces on the atomic ioctl.
> + *
> + * This way explicit fencing can be used to overrule implicit fencing,
> + * which is important to make explicit fencing use-cases work: One
> + * example is using one buffer for 2 screens with different refresh
> + * rates. Implicit fencing will clamp rendering to the refresh rate of
> + * the slower screen, whereas explicit fence allows 2 independent
> + * render and display loops on a single buffer. If a driver allows
> + * obeys both implicit and explicit fences for plane updates, then it
> + * will break all the benefits of explicit fencing.
> */
> - drm_atomic_set_fence_for_plane(state, fence);
> + usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
> +
> + for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
Instead of ARRAY_SIZE, rather use state->fb->format->num_planes. It's
the number of planes (i.e., GEM objects) in the framebuffer.
> + struct dma_fence *new;
> +
> + obj = drm_gem_fb_get_obj(state->fb, i);
> + if (!obj)
With the use of num_planes in the for loop, there should probably be a
drm_WARN_ON_ONCE() around this test.
> + continue;
goto error handling.
Or is there a use case for framebuffers with empty planes? At least it's
not possible to instantiate one via drm_gem_fb_init_with_funcs().
> +
> + ret = dma_resv_get_singleton(obj->resv, usage, &new);
> + if (ret)
> + goto error;
> +
> + if (new && fence) {
> + struct dma_fence_chain *chain = dma_fence_chain_alloc();
> +
> + if (!chain) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + dma_fence_chain_init(chain, fence, new, 1);
> + fence = &chain->base;
> +
> + } else if (new) {
> + fence = new;
> + }
> + }
> +
> + dma_fence_put(state->fence);
> + state->fence = fence;
> return 0;
> +
> +error:
> + dma_fence_put(fence)
> + return ret;
> }
> EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
>
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 8cec52ad1277..4c6d39d7bdb2 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> struct drm_crtc *crtc);
> void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> struct drm_framebuffer *fb);
> -void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> - struct dma_fence *fence);
> int __must_check
> drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> struct drm_crtc *crtc);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 2628c7cde2da..89ea54652e87 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -74,9 +74,7 @@ struct drm_plane_state {
> *
> * Optional fence to wait for before scanning out @fb. The core atomic
> * code will set this when userspace is using explicit fencing. Do not
> - * write this field directly for a driver's implicit fence, use
> - * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is
> - * preserved.
> + * write this field directly for a driver's implicit fence.
> *
> * Drivers should store any implicit fence in this from their
> * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
next prev parent reply other threads:[~2022-04-28 7:23 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-21 19:10 [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb Christian König
2022-04-27 16:03 ` Daniel Vetter
2022-04-28 6:41 ` Christian König
2022-04-28 12:27 ` Daniel Vetter
2022-04-28 7:23 ` Thomas Zimmermann [this message]
2022-04-28 7:32 ` Christian König
2022-04-28 7:50 ` Thomas Zimmermann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877dc0d9-c6c6-022c-20d8-14b33e863934@suse.de \
--to=tzimmermann@suse.de \
--cc=christian.koenig@amd.com \
--cc=ckoenig.leichtzumerken@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).