Re: [PATCH v4 02/11] drm/fourcc: Add DRM_FORMAT_XV15/XV20

[Laurent Pinchart]

On Fri, Mar 28, 2025 at 12:05:40PM +0200, Tomi Valkeinen wrote:
> On 28/03/2025 00:34, Laurent Pinchart wrote:
> > On Wed, Mar 26, 2025 at 03:22:45PM +0200, Tomi Valkeinen wrote:
> >> Add two new pixel formats:
> >>
> >> DRM_FORMAT_XV15 ("XV15")
> >> DRM_FORMAT_XV20 ("XV20")
> >>
> >> The formats are 2 plane 10 bit per component YCbCr, with the XV15 2x2
> >> subsampled whereas XV20 is 2x1 subsampled.
> >>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/gpu/drm/drm_fourcc.c  | 6 ++++++
> >>   include/uapi/drm/drm_fourcc.h | 8 ++++++++
> >>   2 files changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> >> index 2f5781f5dcda..e101d1b99aeb 100644
> >> --- a/drivers/gpu/drm/drm_fourcc.c
> >> +++ b/drivers/gpu/drm/drm_fourcc.c
> >> @@ -346,6 +346,12 @@ const struct drm_format_info *__drm_format_info(u32 format)
> >>   		{ .format = DRM_FORMAT_P030,            .depth = 0,  .num_planes = 2,
> >>   		  .char_per_block = { 4, 8, 0 }, .block_w = { 3, 3, 0 }, .block_h = { 1, 1, 0 },
> >>   		  .hsub = 2, .vsub = 2, .is_yuv = true},
> >> +		{ .format = DRM_FORMAT_XV15,		.depth = 0,  .num_planes = 2,
> >> +		  .char_per_block = { 4, 8, 0 }, .block_w = { 3, 3, 0 }, .block_h = { 1, 1, 0 },
> >> +		  .hsub = 2, .vsub = 2, .is_yuv = true },
> >> +		{ .format = DRM_FORMAT_XV20,		.depth = 0,  .num_planes = 2,
> >> +		  .char_per_block = { 4, 8, 0 }, .block_w = { 3, 3, 0 }, .block_h = { 1, 1, 0 },
> >> +		  .hsub = 2, .vsub = 1, .is_yuv = true },
> > 
> > It appears we can never have too much (or enough) documentation, as
> > reading the format info documentation leaves me with unanswered
> > questions.
> > 
> > Looking at drm_format_info_min_pitch():
> > 
> > uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
> > 				   int plane, unsigned int buffer_width)
> > {
> > 	if (!info || plane < 0 || plane >= info->num_planes)
> > 		return 0;
> > 
> > 	return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane],
> > 			    drm_format_info_block_width(info, plane) *
> > 			    drm_format_info_block_height(info, plane));
> > }
> > 
> > For the first plane, the function will return `buffer_width * 4 / 3`
> > (rouding up), which I think is right. For the second plane, it will
> > return `buffer_width * 8 / 3`, which I believe is wrong as the format is
> > subsampled by a factor 2 horizontally. It seems that either
> > char_per_block and block_w need to take horizontal subsampling into
> > account (and therefore be 8 and 6 for the second plane), or
> > drm_format_info_min_pitch() should consider .hsub. Or there's something
> > else I'm missing :-)
> 
> The buffer_width is already divided by the hsub, in 
> drm_format_info_plane_width().

The function documentation doesn't clearly indicate if buffer_width is
the width of the buffer (as in the width of the first plane), or the
width of the requested plane. The variable name implies (for me) that
it's the width of the buffer, not the plane.

I've checked users of the function, and it seems to be a mess. The
following users pass the plane width to the function:

drivers/gpu/drm/drm_framebuffer.c
drivers/gpu/drm/drm_gem_framebuffer_helper.c
drivers/gpu/drm/tests/drm_format_test.c

However, the following users seem to pass the full buffer width:

drivers/gpu/drm/rockchip/rockchip_drm_vop.c
drivers/gpu/drm/tests/drm_format_helper_test.c

> >>   	};
> >>   
> >>   	unsigned int i;
> >> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> >> index 81202a50dc9e..1247b814bd66 100644
> >> --- a/include/uapi/drm/drm_fourcc.h
> >> +++ b/include/uapi/drm/drm_fourcc.h
> >> @@ -304,6 +304,14 @@ extern "C" {
> >>   #define DRM_FORMAT_RGB565_A8	fourcc_code('R', '5', 'A', '8')
> >>   #define DRM_FORMAT_BGR565_A8	fourcc_code('B', '5', 'A', '8')
> >>   
> >> +/*
> >> + * 2 plane 10 bit per component YCrCb
> >> + * index 0 = Y plane, [31:0] x:Y2:Y1:Y0 2:10:10:10 little endian
> >> + * index 1 = Cb:Cr plane, [63:0] x:Cr2:Cb2:Cr1:x:Cb1:Cr0:Cb0 2:10:10:10:2:10:10:10 little endian
> > 
> > I believe this is right, but I have a hard time validating it, as I
> > think the corresponding figures in UG1085 are incorrect (they show a
> > 8bpp format as far as I can tell). Do I assume correctly that you've
> > tested the formats ?
> 
> Yes. kms++'s master branch has support for all the formats in this series.
> 
>   Tomi

-- 
Regards,

Laurent Pinchart