[PATCH v4 01/14] drm: Centralize format information
Tomi Valkeinen
tomi.valkeinen at ti.com
Thu Sep 15 06:22:54 UTC 2016
On 15/09/16 01:22, Laurent Pinchart wrote:
> 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.
Ok. Then the ARGB8888 & co. formats have depth wrong. I presumed those
were right as they're the "normal" ones =).
I'm not sure if it's worth the hassle, but if the depth is only for
fbdev compat code, maybe a separate format->depth table in fbdev code
(for the formats fbdev supports) would make this cleaner and avoid any
future mistakes with new drm drivers.
>>> 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" ?
Yes, that makes it clear.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160915/aab33957/attachment.sig>
More information about the dri-devel
mailing list