Re: [PATCH v2 1/3] drm: Add some new format DRM_FORMAT_NVXX_10

From: Daniel Vetter <daniel@ffwll.ch>
To: "Heiko Stübner" <heiko@sntech.de>
Cc: Daniel Vetter <daniel@ffwll.ch>, Sandy Huang <hjc@rock-chips.com>,
	dri-devel@lists.freedesktop.org,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] drm: Add some new format DRM_FORMAT_NVXX_10
Date: Fri, 8 Nov 2019 17:37:27 +0100	[thread overview]
Message-ID: <20191108163727.GZ23790@phenom.ffwll.local> (raw)
In-Reply-To: <17879396.dXe580Ps6o@diego>

On Fri, Nov 08, 2019 at 04:08:50PM +0100, Heiko Stübner wrote:
> Hi Daniel, Sandy,
> 
> Am Mittwoch, 9. Oktober 2019, 16:50:08 CET schrieb Daniel Vetter:
> > On Thu, Sep 26, 2019 at 04:24:47PM +0800, Sandy Huang wrote:
> > > These new format is supported by some rockchip socs:
> > > 
> > > DRM_FORMAT_NV12_10/DRM_FORMAT_NV21_10
> > > DRM_FORMAT_NV16_10/DRM_FORMAT_NV61_10
> > > DRM_FORMAT_NV24_10/DRM_FORMAT_NV42_10
> > > 
> > > Signed-off-by: Sandy Huang <hjc@rock-chips.com>
> > > ---
> > >  drivers/gpu/drm/drm_fourcc.c  | 18 ++++++++++++++++++
> > >  include/uapi/drm/drm_fourcc.h | 14 ++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > index c630064..ccd78a3 100644
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -261,6 +261,24 @@ const struct drm_format_info *__drm_format_info(u32 format)
> > >  		{ .format = DRM_FORMAT_P016,		.depth = 0,  .num_planes = 2,
> > >  		  .char_per_block = { 2, 4, 0 }, .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 },
> > >  		  .hsub = 2, .vsub = 2, .is_yuv = true},
> > > +		{ .format = DRM_FORMAT_NV12_10,		.depth = 0,  .num_planes = 2,
> > > +		  .char_per_block = { 5, 10, 0 }, .block_w = { 4, 4, 0 }, .block_h = { 4, 4, 0 },
> > > +		  .hsub = 2, .vsub = 2, .is_yuv = true},
> > > +		{ .format = DRM_FORMAT_NV21_10,		.depth = 0,  .num_planes = 2,
> > > +		  .char_per_block = { 5, 10, 0 }, .block_w = { 4, 4, 0 }, .block_h = { 4, 4, 0 },
> > > +		  .hsub = 2, .vsub = 2, .is_yuv = true},
> > > +		{ .format = DRM_FORMAT_NV16_10,		.depth = 0,  .num_planes = 2,
> > > +		  .char_per_block = { 5, 10, 0 }, .block_w = { 4, 4, 0 }, .block_h = { 4, 4, 0 },
> > > +		  .hsub = 2, .vsub = 1, .is_yuv = true},
> > > +		{ .format = DRM_FORMAT_NV61_10,		.depth = 0,  .num_planes = 2,
> > > +		  .char_per_block = { 5, 10, 0 }, .block_w = { 4, 4, 0 }, .block_h = { 4, 4, 0 },
> > > +		  .hsub = 2, .vsub = 1, .is_yuv = true},
> > > +		{ .format = DRM_FORMAT_NV24_10,		.depth = 0,  .num_planes = 2,
> > > +		  .char_per_block = { 5, 10, 0 }, .block_w = { 4, 4, 0 }, .block_h = { 4, 4, 0 },
> > > +		  .hsub = 1, .vsub = 1, .is_yuv = true},
> > > +		{ .format = DRM_FORMAT_NV42_10,		.depth = 0,  .num_planes = 2,
> > > +		  .char_per_block = { 5, 10, 0 }, .block_w = { 4, 4, 0 }, .block_h = { 4, 4, 0 },
> > > +		  .hsub = 1, .vsub = 1, .is_yuv = true},
> > >  		{ .format = DRM_FORMAT_P210,		.depth = 0,
> > >  		  .num_planes = 2, .char_per_block = { 2, 4, 0 },
> > >  		  .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 }, .hsub = 2,
> > 
> > Yup this is what I had in mind with using the block stuff to describe your
> > new 10bit yuv formats. Thanks for respining.
> > 
> > Once we've nailed the exact bit description of the format precisely this
> > can be merged imo.
> 
> I just saw this series still sitting in my inbox, so was wondering about the
> "once we've nailed the exact bit description..." and what is missing for that.

Since my name is on this mail ... what I meant here is that in principle
I'm ok, but someone else needs to check that we've documented all the
details properly. And once that review is done they or you can also merge
it. From my side not going to dig more into this, we have tons of
reviewers/committers.
-Daniel


> 
> Thanks
> Heiko
> 
> 
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index 3feeaa3..08e2221 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -238,6 +238,20 @@ extern "C" {
> > >  #define DRM_FORMAT_NV42		fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */
> > >  
> > >  /*
> > > + * 2 plane YCbCr
> > > + * index 0 = Y plane, Y3:Y2:Y1:Y0 10:10:10:10
> > > + * index 1 = Cb:Cr plane, Cb3:Cr3:Cb2:Cr2:Cb1:Cr1:Cb0:Cr0 10:10:10:10:10:10:10:10
> > > + * or
> > > + * index 1 = Cr:Cb plane, Cr3:Cb3:Cr2:Cb2:Cr1:Cb1:Cr0:Cb0 10:10:10:10:10:10:10:10
> > > + */
> > > +#define DRM_FORMAT_NV12_10	fourcc_code('N', 'A', '1', '2') /* 2x2 subsampled Cr:Cb plane */
> > > +#define DRM_FORMAT_NV21_10	fourcc_code('N', 'A', '2', '1') /* 2x2 subsampled Cb:Cr plane */
> > > +#define DRM_FORMAT_NV16_10	fourcc_code('N', 'A', '1', '6') /* 2x1 subsampled Cr:Cb plane */
> > > +#define DRM_FORMAT_NV61_10	fourcc_code('N', 'A', '6', '1') /* 2x1 subsampled Cb:Cr plane */
> > > +#define DRM_FORMAT_NV24_10	fourcc_code('N', 'A', '2', '4') /* non-subsampled Cr:Cb plane */
> > > +#define DRM_FORMAT_NV42_10	fourcc_code('N', 'A', '4', '2') /* non-subsampled Cb:Cr plane */
> > > +
> > > +/*
> > >   * 2 plane YCbCr MSB aligned
> > >   * 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
> > 
> > 
> 
> 
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch