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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4628 bytes --]

Hi

Am 28.04.22 um 09:32 schrieb Christian König:
> 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 

But didn't checkpatch stop warning about the 80-char limit?

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

I've gradually changed the code towards the use of num_planes and 
stricter error reporting.  IMHO at some point, we should warn about 
empty plane BOs directly within drm_gem_fb_get_obj().

Best regards
Thomas

> 
> Thanks for taking a look,
> Christian.

-- 
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 --]