Re: [RFC 2/7] drm: Add GEM backed framebuffer library

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 21, 2017 at 08:39:17PM +0200, Noralf Trønnes wrote:
>
> Den 20.07.2017 10.10, skrev Daniel Vetter:
> > On Tue, Jul 18, 2017 at 05:42:28PM +0200, Noralf Trønnes wrote:
> > > Den 12.07.2017 15.46, skrev Noralf Trønnes:
> > > > Add a library for drivers that can use a simple representation
> > > > of a GEM backed framebuffer.
> > > >
> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > ---
> > > This patch adds a gem backed drm_framebuffer like this:
> > >
> > > struct drm_fb_gem {
> > >      /**
> > >       * @base: Base DRM framebuffer
> > >       */
> > >      struct drm_framebuffer base;
> > >      /**
> > >       * @obj: GEM object array backing the framebuffer. One object per
> > >       * plane.
> > >       */
> > >      struct drm_gem_object *obj[4];
> > > };
> > >
> > > Now I wonder if it would be better to extend drm_framebuffer instead:
> > >
> > >   struct drm_framebuffer {
> > > +    /**
> > > +     * @obj: GEM objects backing the framebuffer, one per plane (optional).
> > > +     */
> > > +    struct drm_gem_object *obj[4];
> > >   };
> > I think we can directly embedd the gem obj pointers into the
> > drm_framebuffer. Again the idea that there would be anything else than gem
> > kinda didn't pan out, except for vmwgfx.
> >
> > We could then have a gem version of drm_helper_mode_fill_fb_struct(),
> > which also does the gem bo lookups.
>
> How about this, I've copied from drm_fb_cma_create_with_funcs():

Hm, we might as well call it drm_fb_gem_create_with_funcs, to stay in line
with the naming scheme we have.

> /**
>  * drm_helper_mode_fill_fb_gem - fill out framebuffer metadata and lookup
> GEM
>  * @dev: DRM device
>  * @file_priv: DRM file to lookup buffers from
>  * @fb: drm_framebuffer object to fill out
>  * @mode_cmd: metadata from the userspace fb creation request
>  *
>  * This helper can be used in a drivers fb_create callback to pre-fill the
> fb's
>  * metadata fields and lookup the GEM buffer object(s).
>  *
>  * Returns:
>  * 0 on success or a negative error code on failure.
>  */
> int drm_helper_mode_fill_fb_gem(struct drm_device *dev,
>                 struct drm_file *file_priv,
>                 struct drm_framebuffer *fb,
>                 const struct drm_mode_fb_cmd2 *mode_cmd)
> {
>     int i, ret;
>
>     drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>     if (!fb->format)
>         return -EINVAL;
>
>     for (i = 0; i < fb->format->num_planes; i++) {
>         unsigned int width = mode_cmd->width /
>                      (i ? fb->format->hsub : 1);
>         unsigned int height = mode_cmd->height /
>                       (i ? fb->format->vsub : 1);
>         unsigned int min_size;
>
>         fb->objs[i] = drm_gem_object_lookup(file_priv,
>                             mode_cmd->handles[i]);
>         if (!fb->objs[i]) {
>             ret = -ENOENT;
>             goto err_gem_object_put;
>         }
>
>         min_size = (height - 1) * mode_cmd->pitches[i] +
>                width * fb->format->cpp[i] + mode_cmd->offsets[i];
>
>         if (fb->objs[i]->size < min_size) {
>             drm_gem_object_put_unlocked(fb->objs[i]);
>             fb->objs[i] = NULL;
>             ret = -EINVAL;
>             goto err_gem_object_put;
>         }
>     }
>
> err_gem_object_put:
>     for (i--; i >= 0; i--) {
>         drm_gem_object_put_unlocked(fb->objs[i]);
>         fb->objs[i] = NULL;
>     }
>
>     return ret;
> }
> EXPORT_SYMBOL(drm_helper_mode_fill_fb_gem);
>
> > > The reason for this, is that I have looked through all drivers that
> > > subclass drm_framebuffer and found this:
> > > - 11 drivers just adds drm_gem_object
> > Usually that's because they don't support yuv, so only need one plane.
> >
> > > - 2 drivers adds drm_gem_object and some more
> > > - 6 drivers adds drm_gem_object indirectly through their subclassed
> > >    drm_gem_object
> > > - 3 drivers doesn't add drm_gem_object
> > I think for easier conversion we can leave all the driver-specific stuff
> > intact, and just provide helpers that work on the core bits.
> >
> > I guess this would also help a lot with unifying the cma fb helpers by
> > essentially making them fully generic gem fb helpers?
>
> Yes both cma and shmem library can now use these helpers. But pushing
> the helpers all the way out to the cma drivers will be too much for me.

No worries, we can do this one by one across different people.

> Any chance the fbdev helper could get a struct drm_file?
> It would be nice to use drm_driver.dumb_create and
> drm_mode_config_funcs.fb_create to get a
> framebuffer for fbdev.

David Herrmann had a patch series to allow the kernel to create a drm_file
without a real struct file attached behind it. I think we landed the prep
work, but not any of the work to move the fbdev helper over to it. I'll
ping him whether he still has some of those patches somewhere.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel