diff mbox

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

Message ID 1488216069-11544-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Clint Taylor Feb. 27, 2017, 5:21 p.m. UTC 
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(+)

Comments

Ville Syrjala Feb. 27, 2017, 5:41 p.m. UTC | #1 
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?

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

> +#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
Clint Taylor Feb. 27, 2017, 6:28 p.m. UTC | #2 
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
>
Ville Syrjala Feb. 27, 2017, 6:47 p.m. UTC | #3 
On Mon, Feb 27, 2017 at 10:28:21AM -0800, Clint Taylor wrote:
> 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?

Sorry. Just misread it hastily and thought it was something else. This
lgtm.

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

I was mainly refrering to the way you specified the bits for each
component.

So to be consistent with most other things I think it
should look something like this:

"index 0 = Y plane, [15:0] Y:X 10:6 little endian"
"index 1 = Cr:Cb plane, [31:0] Cr:X:Cb:X 10:6:10:6 little endian"

That might not be the world's best notation, but consistency trumps
everything else here IMO. If someone has a good idea for a better
notation I'd like to see it. And if we change the notation for one
thing then we should change it for everything. Otherwise people
will get even more confused when reading this stuff.

> 
> -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
> >
diff mbox

Patch

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 },
 	};
 
 	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
+ */
+#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