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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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: OpenPGP_signature
Description: OpenPGP digital signature