Hi Tomi,
Thank you for the review.
On Wednesday 14 Sep 2016 14:49:26 Tomi Valkeinen wrote:
> On 08/09/16 17:44, 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>
> > ---
> >
> > Documentation/gpu/drm-kms.rst | 3 ++
> > drivers/gpu/drm/drm_fourcc.c | 84 ++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_fourcc.h | 19 ++++++++++
> > 3 files changed, 106 insertions(+)
[snip]
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 29c56b4331e0..6b91bd8a510d 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
> > EXPORT_SYMBOL(drm_get_format_name);
> >
> > /**
> > + * drm_format_info - query information for a given format
> > + * @format: pixel format (DRM_FORMAT_*)
> > + *
> > + * Returns:
> > + * The instance of struct drm_format_info that describes the pixel
> > format, or
> > + * NULL if the format is unsupported.
> > + */
> > +const struct drm_format_info *drm_format_info(u32 format)
> > +{
> > + static const struct drm_format_info formats[] = {
> > + { .format = DRM_FORMAT_C8, .depth = 8,
> > .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGB332, .depth = 8,
> > .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGR233, .depth = 8,
> > .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XRGB4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XBGR4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBX4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRX4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ARGB4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ABGR4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBA4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRA4444, .depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> Aren't these 16 bit deep modes? 4+4+4+4 = 16.
No, the depth value is the number of colour bits, excluding the alpha bits.
This is used to implement the fbdev compatibility code, as fbdev (unlike kms)
makes use of that information.
The total number of bits per pixel is not stored in the table as it can be
computed by cpp[0]*8 + (cpp[1]+cpp[2])*8/hsub/vsub. For RGB formats, which are
the only formats supported by the existing drm_fb_get_bpp_depth() function,
this simplifies to just cpp[0] * 8.
Same for the other formats below.
> > + { .format = DRM_FORMAT_XRGB1555, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XBGR1555, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBX5551, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRX5551, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ARGB1555, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ABGR1555, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBA5551, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRA5551, .depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> Aren't these 16 bit deep modes? 5+5+5+1 = 16.
>
> > + { .format = DRM_FORMAT_RGB565, .depth = 16,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGR565, .depth = 16,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGB888, .depth = 24,
> > .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGR888, .depth = 24,
> > .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XRGB8888, .depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XBGR8888, .depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBX8888, .depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRX8888, .depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XRGB2101010, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_XBGR2101010, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBX1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRX1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ARGB2101010, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ABGR2101010, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBA1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRA1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> Aren't these 32 bit deep modes? 10+10+10+2 = 32.
>
> > + { .format = DRM_FORMAT_ARGB8888, .depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_ABGR8888, .depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_RGBA8888, .depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_BGRA8888, .depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_YUV410, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> > + { .format = DRM_FORMAT_YVU410, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> > + { .format = DRM_FORMAT_YUV411, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> > + { .format = DRM_FORMAT_YVU411, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> > + { .format = DRM_FORMAT_YUV420, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> > + { .format = DRM_FORMAT_YVU420, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> > + { .format = DRM_FORMAT_YUV422, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_YVU422, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_YUV444, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_YVU444, .depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_NV12, .depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> > + { .format = DRM_FORMAT_NV21, .depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> > + { .format = DRM_FORMAT_NV16, .depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_NV61, .depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_NV24, .depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_NV42, .depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> > + { .format = DRM_FORMAT_YUYV, .depth = 0,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_YVYU, .depth = 0,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_UYVY, .depth = 0,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_VYUY, .depth = 0,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > + { .format = DRM_FORMAT_AYUV, .depth = 0,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > + };
> > +
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> > + if (formats[i].format == format)
> > + return &formats[i];
> > + }
> > +
> > + return NULL;
> > +}
> > +EXPORT_SYMBOL(drm_format_info);
> > +
> > +/**
> > * drm_fb_get_bpp_depth - get the bpp/depth values for format
> > * @format: pixel format (DRM_FORMAT_*)
> > * @depth: storage for the depth value
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index 30c30fa87ee8..4e79d6159856 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -25,6 +25,25 @@
> > #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)
>
> Depth is zero for yuv formats. Maybe that should be made clear here.
How about
"color depth (number of bits per pixel excluding padding and alpha bits),
valid for RGB formats only" ?
--
Regards,
Laurent Pinchart
