[PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb
Christian König
christian.koenig at amd.com
Thu Apr 28 06:41:28 UTC 2022
Am 27.04.22 um 18:03 schrieb Daniel Vetter:
> On Thu, Apr 21, 2022 at 09:10:02PM +0200, Christian König wrote:
>> 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 at amd.com>
> Is this enough to have amdgpu (and well others using ttm) fully rely on
> this for atomic kms updates? Anything to clean up there? Would be neat to
> include that in this series too if there's anything.
At least I strongly think so. I just haven't removed the
dma_resv_wait_timeout() from amdgpu_dm_commit_planes() because that is
simply not code I'm very familiar with.
>> ---
>> 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)
> I was a bit on the fence with ditching this, but the only offender not
> using the prepare_fb helpers is i915, and so just more reasons that i915
> needs to get off its hand-rolled atomic code with its own funky dependency
> handling and everything.
Yeah, agree totally. amdgpu should now also work out of the box, I just
didn't dared to actually enable it in the same patch.
>
>> -{
>> - 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
> You forgot to update the kerneldoc here, and also the reference to the
> same function in the IN_FENCE_FD.
>
> I think it'd be best to put a reference to that DOC: section here, and
> adjust the uapi property doc to just state that the explicit fence will
> overrule implicit sync. And then maybe also mention here that USAGE_KERNEL
> fences are still obeyed.
>
> With these changes (which should make sure that all references to
> drm_atomic_set_fence_for_plane() are truly gone) this is
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Ok, going to update the doc and push this if nobody objects.
Thanks,
Christian.
>
>
>
>> * 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 dma_fence *fence = dma_fence_get(state->fence);
>> struct drm_gem_object *obj;
>> - 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) {
>> + struct dma_fence *new;
>> +
>> + obj = drm_gem_fb_get_obj(state->fb, i);
>> + if (!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);
>>
>> 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()
>> --
>> 2.25.1
>>
More information about the dri-devel
mailing list