Re: [PATCH v4 01/14] drm: Centralize format information

From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 01/14] drm: Centralize format information
Date: Thu, 15 Sep 2016 09:22:54 +0300	[thread overview]
Message-ID: <1ef50c0f-dc06-0a62-8728-82ee601eec34@ti.com> (raw)
In-Reply-To: <1622000.t01kXMM4V9@avalon>

[-- Attachment #1.1.1: Type: text/plain, Size: 4712 bytes --]

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@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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel