Re: [igt-dev] [PATCH i-g-t 3/8] lib/fb: Handle planar formats in igt_calc_fb_size and create_bo_for_fb

[Mika Kahola <mika.kahola@intel.com>]

On Fri, 2018-01-26 at 13:01 +0100, Maarten Lankhorst wrote:
> Op 26-01-18 om 11:24 schreef Mika Kahola:
> > 
> > On Fri, 2018-01-26 at 11:20 +0100, Maarten Lankhorst wrote:
> > > 
> > > Op 26-01-18 om 10:00 schreef Mika Kahola:
> > > > 
> > > > On Tue, 2018-01-23 at 13:56 +0100, Maarten Lankhorst wrote:
> > > > > 
> > > > > By adding support for planar formats to igt_calc_fb_size and
> > > > > create_bo_for_fb,
> > > > > we can calculate dimensions and create backing storage for
> > > > > planar
> > > > > framebuffers.
> > > > > 
> > > > > This is required for adding support to create planar
> > > > > framebuffers
> > > > > in
> > > > > the next patch.
> > > > > 
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.int
> > > > > el.c
> > > > > om>
> > > > > ---
> > > > >  lib/igt_fb.c | 168
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > > ----
> > > > > ------------
> > > > >  1 file changed, 123 insertions(+), 45 deletions(-)
> > > > > 
> > > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > > > index da07d1a9e21f..6a331f06724b 100644
> > > > > --- a/lib/igt_fb.c
> > > > > +++ b/lib/igt_fb.c
> > > > > @@ -54,14 +54,16 @@
> > > > >   */
> > > > >  
> > > > >  /* drm fourcc/cairo format maps */
> > > > > -#define DF(did, cid, _bpp, _depth)	\
> > > > > -	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did, _bpp,
> > > > > _depth
> > > > > }
> > > > > +#define DF(did, cid, ...)	\
> > > > > +	{ DRM_FORMAT_##did, CAIRO_FORMAT_##cid, # did,
> > > > > __VA_ARGS__ }
> > > > >  static struct format_desc_struct {
> > > > >  	uint32_t drm_id;
> > > > >  	cairo_format_t cairo_id;
> > > > >  	const char *name;
> > > > >  	int bpp;
> > > > >  	int depth;
> > > > > +	int planes;
> > > > > +	int plane_bpp[4];
> > > > should we define a max value for this instead of hardcoded one?
> > > 4 planes is the maximum ever. Alpha, R, G and B.
> > Yes, but even so I'm not really a fan of harcoded values.
> Only the array definitions use those values. There aren't even planar
> formats that support > 4 planes,
> and uapi/drm/drm_mode.h also uses 4 everywhere. It's fine unless we
> everextend the kernel api to support
> more than 4 planes, but this is very unlikely since the hw doesn't
> even exist. I think it's fine to hardcode this. All users use
> ARRAY_SIZE or sizeof as appropriate, so it won't matter much.
What I was after on my comment was that when you are browsing through
the patch or the file itself, you don't realize that easily what is
meant with these hardcoded values. You have to stop for a second to
think and then you figure it out. The comment was all about enhancing
readability. For loops, maybe we could use ARRAY_SIZE() here too?

> 
> ~Maarten
-- 
Mika Kahola - Intel OTC

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev