[PATCH v2 2/5] drm: Centralize format information

[Laurent Pinchart]

Hi Eric,

On Wednesday 08 Jun 2016 11:27:32 Eric Engestrom wrote:
> On Wed, Jun 08, 2016 at 02:35:35AM +0300, Laurent Pinchart wrote:
> > Various pieces of information about DRM formats (number of planes, color
> > depth, chroma subsampling, ...) are scattered across different helper
> > functions in the DRM core. Callers of those functions often need to
> > access more than a single parameter of the format, leading to
> > inefficiencies due to multiple lookups.
> > 
> > Centralize all format information in a data structure and create a
> > function to look up information based on the format 4CC.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_fourcc.c | 84 +++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_fourcc.h     | 21 +++++++++++
> >  2 files changed, 105 insertions(+)
> 
> [snip]
> 
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index 7f90a396cf2b..d33411bd19b1 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -25,6 +25,27 @@
> > 
> >  #include <linux/types.h>
> >  #include <uapi/drm/drm_fourcc.h>
> > 
> > +/**
> > + * struct drm_format_info - information about a DRM format
> > + * @format: 4CC format identifier (DRM_FORMAT_*)
> > + * @depth: color depth (number of bits per pixel excluding padding bits)
> > + * @bpp: number of bits per pixel including padding
> > + * @num_planes: number of color planes (1 to 3)
> > + * @cpp: number of bytes per pixel (per plane)
> > + * @hsub: horizontal chroma subsampling factor
> > + * @vsub: vertical chroma subsampling factor
> > + */
> > +struct drm_format_info {
> > +	u32 format;
> > +	u8 depth;
> > +	u8 bpp;
> 
> I think `bpp` is redundant here: it's always `cpp[0] << 3`, except when
> ignored (yuv formats), which `depth` already indicates by being 0.
> 
> Unless I missed something, the only change required to drop this field
> would be in patch #3, in drm_fb_get_bpp_depth():
> 
> 	*bpp = info->bpp;
> 
> becomes:
> 
> 	*bpp = info->depth ? info->cpp[0] << 3 : 0;

I agree with you, it's redundant. I'll remove it, and will also patch callers 
to user drm_format_cpp() instead of drm_fb_get_bpp_depth() where applicable.

> I'm not really opposed to it being here if someone else finds it useful
> though, so either way, the series is:
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
> 
> > +	u8 num_planes;
> > +	u8 cpp[3];
> > +	u8 hsub;
> > +	u8 vsub;
> > +};
> > +
> > +const struct drm_format_info *drm_format_info(u32 format);
> > 
> >  void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int
> >  *bpp);
> >  int drm_format_num_planes(uint32_t format);
> >  int drm_format_plane_cpp(uint32_t format, int plane);

-- 
Regards,

Laurent Pinchart