Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

["Christian König" <christian.koenig@amd.com>]

Am 28.04.22 um 09:23 schrieb Thomas Zimmermann:
> [SNIP]
>> 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?

Yeah, Daniel point that out as well.

>
>>    * 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.

Despite some efforts to change this that is still documented as 
80-character limit: 
https://www.kernel.org/doc/html/v5.18-rc4/process/coding-style.html#breaking-long-lines-and-strings

>
>>   {
>> +    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().
>

I was asking myself the same thing, but found this handling be used at 
other places as well.

Thanks for taking a look,
Christian.