[PATCH 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

Liviu Dudau liviu.dudau at arm.com
Thu Aug 16 10:58:26 UTC 2018 

On Thu, Aug 16, 2018 at 12:42:59PM +0200, Daniel Vetter wrote:
> On Thu, Aug 16, 2018 at 12:25 PM, Liviu Dudau <liviu.dudau at arm.com> wrote:
> > On Wed, Aug 15, 2018 at 03:40:17PM +0200, Daniel Vetter wrote:
> >> On Wed, Aug 15, 2018 at 2:27 PM, Alexandru-Cosmin Gheorghe
> >> <Alexandru-Cosmin.Gheorghe at arm.com> wrote:
> >> > On Wed, Aug 15, 2018 at 01:54:12PM +0200, Daniel Vetter wrote:
> >> >> On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
> >> >> > On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
> >> >> > > Some drivers can't use drm_gem_fb_create, so instead of copying the
> >> >> > > logic that does the framebuffer allocation allow them to use core
> >> >> > > drm_gem_fb_alloc.
> >> >> > >
> >> >> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe at arm.com>
> >> >> >
> >> >> > To me it looks like an useful thing to have exported, so for what is worth:
> >> >> >
> >> >> > Acked-by: Liviu Dudau <liviu.dudau at arm.com>
> >> >> >
> >> >> > Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?
> >> >>
> >> >> In general please add meaningful kernel-doc for exported functions,
> >> >> explaing what's different and how it works together.
> >> >>
> >> >> Wrt the specific issue, I think we should teach drm core and helpers how
> >> >> to allocate formats with tiled blocks. Means we need to extend
> >> >> drm_format_info a bit. Your YUV formats won't be the only ones where the
> >> >> format itself arranges pixels in blocks, and hence has format-based
> >> >> alignment criteria for pitch and size (due to line rounding).
> >> >
> >> > Something like a tile_size or do you have other ideas ?
> >> > Any idea if there are any tile formats out there where the tile it's
> >> > not a square ?
> >>
> >> I think both tile_w/_h in pixels and tile_size in bytes is what you
> >> want for full description of all options. And yes I think some of the
> >> compressed GL formats aren't square. Not that I expect we'll ever have
> >> to scan those out, but who knows. This would also allow drivers to let
> >> the core check basic tiled layouts by supplying that information for a
> >> (fourcc, modifier) pair.
> >>
> >> The tricky part is then how you expect stride to be set, since that's
> >> in bytes per line, which might not align. But since all the tiles I've
> >> seen are power-of-two in height, shouldn't be an issue in practice. A
> >> check + WARN_ON would be good though.
> >
> > Another option would be to add to the drm_framebuffer_funcs structure a
> > hook that drivers could implement that does the validation of the fb and

I meant to say "does the validation of the fb creation".

> > could be used to bypass the more strict check in drm_gem_fb_create_fb_with_funcs()
> 
> Can't call an fb operation if you don't yet have an fb :-)

The funcs get passed as an argument to the _with_funcs() drm_gem_fb_create variant,
so the hook should be available. And it is a validation operation, I would say it
should be allowed even if the fb is not yet present.

Best regards,
Liviu

> -Daniel
> 
> >
> > Best regards,
> > Liviu
> >
> >> -Daniel
> >>
> >> >> I think this would also fit better into the overall design where drivers
> >> >> can overwrite the drm_format_info for specific (fourcc, modifier)
> >> >> combinations.
> >> >> -Daniel
> >> >>
> >> >>
> >> >> >
> >> >> > Best regards,
> >> >> > Liviu
> >> >> >
> >> >> > > ---
> >> >> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
> >> >> > >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
> >> >> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> >> >> > >
> >> >> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> >> > > index 2810d4131411..64eddf5a1bd9 100644
> >> >> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> >> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> >> > > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> >> > >  }
> >> >> > >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
> >> >> > >
> >> >> > > -static struct drm_framebuffer *
> >> >> > > +struct drm_framebuffer *
> >> >> > >  drm_gem_fb_alloc(struct drm_device *dev,
> >> >> > >            const struct drm_mode_fb_cmd2 *mode_cmd,
> >> >> > >            struct drm_gem_object **obj, unsigned int num_planes,
> >> >> > > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
> >> >> > >
> >> >> > >   return fb;
> >> >> > >  }
> >> >> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
> >> >> > >
> >> >> > >  /**
> >> >> > >   * drm_gem_fb_destroy - Free GEM backed framebuffer
> >> >> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> >> >> > > index a38de7eb55b4..d20c1356000a 100644
> >> >> > > --- a/include/drm/drm_gem_framebuffer_helper.h
> >> >> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
> >> >> > > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
> >> >> > >
> >> >> > >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> >> > >                                     unsigned int plane);
> >> >> > > +struct drm_framebuffer *
> >> >> > > +drm_gem_fb_alloc(struct drm_device *dev,
> >> >> > > +          const struct drm_mode_fb_cmd2 *mode_cmd,
> >> >> > > +          struct drm_gem_object **obj, unsigned int num_planes,
> >> >> > > +          const struct drm_framebuffer_funcs *funcs);
> >> >> > >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
> >> >> > >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> >> >> > >                        unsigned int *handle);
> >> >> > > --
> >> >> > > 2.18.0
> >> >> > >
> >> >> >
> >> >> > --
> >> >> > ====================
> >> >> > | I would like to |
> >> >> > | fix the world,  |
> >> >> > | but they're not |
> >> >> > | giving me the   |
> >> >> >  \ source code!  /
> >> >> >   ---------------
> >> >> >     ¯\_(ツ)_/¯
> >> >>
> >> >> --
> >> >> Daniel Vetter
> >> >> Software Engineer, Intel Corporation
> >> >> http://blog.ffwll.ch
> >> >
> >> > --
> >> > Cheers,
> >> > Alex G
> >>
> >>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

More information about the dri-devel mailing list