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@xxxxxxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxx>
>
> > + 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
