[PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb v2
Thomas Zimmermann
tzimmermann at suse.de
Thu Apr 28 10:11:12 UTC 2022
Hi
Am 28.04.22 um 11:40 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.
>
> v2: improve kerneldoc, use local variable and num_planes, WARN_ON_ONCE
> on missing planes.
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch> (v1)
Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 47 ++--------------
> drivers/gpu/drm/drm_gem_atomic_helper.c | 73 +++++++++++++++++++------
> include/drm/drm_atomic_uapi.h | 2 -
> include/drm/drm_plane.h | 4 +-
> 4 files changed, 62 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c6394ba13b24..434f3d4cb8a2 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
> @@ -1077,10 +1040,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> *
> * As a contrast, with implicit fencing the kernel keeps track of any
> * ongoing rendering, and automatically ensures that the atomic update waits
> - * for any pending rendering to complete. For shared buffers represented with
> - * a &struct dma_buf this is tracked in &struct dma_resv.
> - * Implicit syncing is how Linux traditionally worked (e.g. DRI2/3 on X.org),
> - * whereas explicit fencing is what Android wants.
> + * for any pending rendering to complete. This is usually tracked in &struct
> + * dma_resv which can also contain mandatory kernel fences. Implicit syncing
> + * is how Linux traditionally worked (e.g. DRI2/3 on X.org), whereas explicit
> + * fencing is what Android wants.
> *
> * "IN_FENCE_FD”:
> * Use this property to pass a fence that DRM should wait on before
> @@ -1095,7 +1058,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> *
> * On the driver side the fence is stored on the @fence parameter of
> * &struct drm_plane_state. Drivers which also support implicit fencing
> - * should set the implicit fence using drm_atomic_set_fence_for_plane(),
> + * should extract the implicit fence using drm_gem_plane_helper_prepare_fb(),
> * to make sure there's consistent behaviour between drivers in precedence
> * of implicit vs. explicit fencing.
> *
> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> index a6d89aed0bda..a5026f617739 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>
> @@ -137,29 +138,67 @@
> *
> * This function is the default implementation for GEM drivers of
> * &drm_plane_helper_funcs.prepare_fb if no callback is provided.
> - *
> - * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
> - * 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)
> {
> - struct drm_gem_object *obj;
> - struct dma_fence *fence;
> + struct dma_fence *fence = dma_fence_get(state->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 < state->fb->format->num_planes; ++i) {
> + struct drm_gem_object *obj = drm_gem_fb_get_obj(state->fb, i);
> + struct dma_fence *new;
> +
> + if (WARN_ON_ONCE(!obj))
> + continue;
> +
> + 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);
>
> @@ -168,13 +207,13 @@ EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
> * @pipe: Simple display pipe
> * @plane_state: Plane state
> *
> - * This function uses drm_gem_plane_helper_prepare_fb() to extract the exclusive fence
> - * from &drm_gem_object.resv and attaches it to plane state for the atomic
> + * This function uses drm_gem_plane_helper_prepare_fb() to extract the fences
> + * from &drm_gem_object.resv and attaches them to the plane state for the atomic
> * helper to wait on. This is necessary to correctly implement implicit
> * synchronization for any buffers shared as a struct &dma_buf. Drivers can use
> * this as their &drm_simple_display_pipe_funcs.prepare_fb callback.
> *
> - * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
> + * See drm_gem_plane_helper_prepare_fb() for a discussion of implicit and
> * explicit fencing in atomic modeset updates.
> */
> int drm_gem_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220428/59dcb2f2/attachment.sig>
More information about the dri-devel
mailing list