| Message ID | 20180726141005.8321-3-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Mali DP pixel formats | expand |
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@arm.com> To me it looks like an useful thing to have exported, so for what is worth: Acked-by: Liviu Dudau <liviu.dudau@arm.com> Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections? 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 >
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@arm.com> > > To me it looks like an useful thing to have exported, so for what is worth: > > Acked-by: Liviu Dudau <liviu.dudau@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). 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! / > --------------- > ¯\_(ツ)_/¯
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@arm.com> > > > > To me it looks like an useful thing to have exported, so for what is worth: > > > > Acked-by: Liviu Dudau <liviu.dudau@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 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
On Wed, Aug 15, 2018 at 2:27 PM, Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@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@arm.com> >> > >> > To me it looks like an useful thing to have exported, so for what is worth: >> > >> > Acked-by: Liviu Dudau <liviu.dudau@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
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@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@arm.com> > >> > > >> > To me it looks like an useful thing to have exported, so for what is worth: > >> > > >> > Acked-by: Liviu Dudau <liviu.dudau@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 could be used to bypass the more strict check in drm_gem_fb_create_fb_with_funcs() 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
On Thu, Aug 16, 2018 at 12:25 PM, Liviu Dudau <liviu.dudau@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@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@arm.com> >> >> > >> >> > To me it looks like an useful thing to have exported, so for what is worth: >> >> > >> >> > Acked-by: Liviu Dudau <liviu.dudau@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 > 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 :-) -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! / > --------------- > ¯\_(ツ)_/¯
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@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@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@arm.com> > >> >> > > >> >> > To me it looks like an useful thing to have exported, so for what is worth: > >> >> > > >> >> > Acked-by: Liviu Dudau <liviu.dudau@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
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);
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@arm.com> --- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++- include/drm/drm_gem_framebuffer_helper.h | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-)