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

[Daniel Vetter]

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