Re: [PATCH v4 1/2] drm_fourcc: Add new P010, P016 video format

[Clint Taylor <clinton.a.taylor@intel.com>]

On 02/27/2017 09:41 AM, Ville Syrjälä wrote:
> On Mon, Feb 27, 2017 at 09:21:09AM -0800, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> P010 is a planar 4:2:0 YUV with interleaved UV plane, 10 bits per
>> channel video format. Rockchip's vop support this video format(little
>> endian only) as the input video format.
>>
>> P016 is a planar 4:2:0 YUV 12 bits per channel
>>
>> P016 is a planar 4:2:0 YUV with interleaved UV plane, 16 bits per
>> channel video format.
>>
>> V3: Added P012 and fixed cpp for P010
>> V4: format definition refined per review
>>
>> Cc: Daniel Stone <daniel@fooishbar.org>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Signed-off-by: Randy Li <ayaka@soulik.info>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> ---
>>  drivers/gpu/drm/drm_fourcc.c  |    4 ++++
>>  include/uapi/drm/drm_fourcc.h |   18 ++++++++++++++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> index 90d2cc8..5494764 100644
>> --- a/drivers/gpu/drm/drm_fourcc.c
>> +++ b/drivers/gpu/drm/drm_fourcc.c
>> @@ -165,6 +165,10 @@ const struct drm_format_info *__drm_format_info(u32 format)
>>  		{ .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 },
>> +		/* FIXME a pixel in Y for P010 is 10 bits */
>> +		{ .format = DRM_FORMAT_P010,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2 },
>> +		{ .format = DRM_FORMAT_P012,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2 },
>> +		{ .format = DRM_FORMAT_P016,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2 },
>>  	};
>
> What's this hunk doing here?

This hunk defines the memory layout definition to DRM, so framebuffers 
can be checked to make sure they are large enough for the format. Can 
you describe your concern here?

>
>>
>>  	unsigned int i;
>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>> index ef20abb..ad94464 100644
>> --- a/include/uapi/drm/drm_fourcc.h
>> +++ b/include/uapi/drm/drm_fourcc.h
>> @@ -128,6 +128,24 @@
>>  #define DRM_FORMAT_NV42		fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */
>>
>>  /*
>> + * 2 plane YCbCr MSB aligned P0?? formats
>> + * index 0 = Y plane, word array [15:6] P010
>> + * or
>> + * index 0 = Y plane, word array [15:4] P012
>> + * or
>> + * index 0 = Y plane, word array [15:0] P016
>> + *
>> + * index 1 = Cb:Cr plane, [31:22] Cb [15:6] Cr little endian P010
>> + * or
>> + * index 1 = Cb:Cr plane, [31:20] Cb [15:4] Cr little endian P012
>> + * or
>> + * index 1 = Cb:Cr plane, [31:16] Cb [15:0] Cr little endian P016
>> + */
>
> Still looks somewhat out of place when compared with the rest of the file.

The other YUV entries in the file all have 8 bit definitions and are 
easily grouped together. The P0?? formats change their number of bits. 
The only way I see is to make the comments look like the other YUV 
formats is to make each format have its own comment block describing the 
layout. I feel it's better to group them together since the difference 
between them (bpc) is minor.

perhaps moving the bit definitions to the defines, but then again it 
won't look like the other YUV format. It will look more like the RGB 
defines.

-Clint

>
>> +#define DRM_FORMAT_P010		fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cb:Cr plane 10 bits per channel */
>> +#define DRM_FORMAT_P012		fourcc_code('P', '0', '1', '2') /* 2x2 subsampled Cb:Cr plane 12 bits per channel */
>> +#define DRM_FORMAT_P016		fourcc_code('P', '0', '1', '6') /* 2x2 subsampled Cb:Cr plane 16 bits per channel */
>> +
>> +/*
>>   * 3 plane YCbCr
>>   * index 0: Y plane, [7:0] Y
>>   * index 1: Cb plane, [7:0] Cb
>> --
>> 1.7.9.5
>

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