RE: [PATCH libdrm] libdrm: update drm/drm_fourcc.h from kernel to add multi plane formats

[Inki Dae <inki.dae@samsung.com>]

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, April 06, 2012 3:14 AM
> To: airlied@redhat.com
> Cc: inki.dae@samsung.com; kyungmin.park@samsung.com; dri-
> devel@lists.freedesktop.org; Seung-Woo Kim
> Subject: Re: [PATCH libdrm] libdrm: update drm/drm_fourcc.h from kernel to
> add multi plane formats
> 
> On Fri, Mar 30, 2012 at 01:12:58PM +0300, Ville Syrjälä wrote:
> > On Fri, Mar 30, 2012 at 11:54:50AM +0900, Seung-Woo Kim wrote:
> > > Multi buffer plane pixel formats are added as like kernel header.
> > >
> > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> > > ---
> > >  include/drm/drm_fourcc.h |    7 +++++++
> > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > > index 85facb0..7cfd95a 100644
> > > --- a/include/drm/drm_fourcc.h
> > > +++ b/include/drm/drm_fourcc.h
> > > @@ -107,6 +107,10 @@
> > >  #define DRM_FORMAT_NV16		fourcc_code('N', 'V', '1', '6') /*
> 2x1 subsampled Cr:Cb plane */
> > >  #define DRM_FORMAT_NV61		fourcc_code('N', 'V', '6', '1') /*
> 2x1 subsampled Cb:Cr plane */
> > >
> > > +/* 2 non contiguous plane YCbCr */
> > > +#define DRM_FORMAT_NV12M	fourcc_code('N', 'M', '1', '2') /* 2x2
> subsampled Cr:Cb plane */
> >
> > NAK. DRM_FORMAT_NV12 handles this just fine.
> 
> And I just realized that I was already too late with my NAK since this a
> libdrm patch. Apparently the kernel drm_fourcc.h changes were snuck in
> via some backdoor without review. Sigh.
> 

We had already requested review for it. for this you can refer to link
below:
	
http://lists.freedesktop.org/archives/dri-devel/2011-December/017654.html

> So they're now in Linus's tree. But looks like format_check() was never
> updated to accept them, so there's no way anyone could actually be using
> them. So Dave, can we still remove them from the kernel header?
> 

Yes, right. these formats aren't used for any SoCs except Exynos series yet
but just we are first. I think they should be added because anyone may use
them someday at least possible. but they couldn’t be standard format so we
can send the patch that these formats are removed from drm_fourcc.h anytime
if you are still opposite to my opinion. but please know that at least in
case of v4l2, they have already been used as standard format. For this you
can refer to include/linux/videodev2.h file in mainline and Dave, please
give me your opinion.


Thanks,
Inki Dae

>
> Just to clarify once mode. The original planar formats I defined in
> drm_fourcc.h handle non-contiguous planes. The drivers are supposed to
> use handles[],offsets[],pitches[] to handle this. The 'index' comments
> in the drm_fourcc.h tells you which index of those arrays matches which
> plane. This means that DRM_FORMAT_NV12M is functionally _identical_ to
> DRM_FORMAT_NV12, and the same holds for the three plane format that got
> submarined in.
> 
> The driver is not even supposed to accept DRM_FORMAT_NV12 unless both
> index 0 and 1 are filled in properly by userspace. If the planes are
> contiguous then userspace _must_ pass the same handle for index 0 and
> 1, and use offsets[] to tell the driver where each plane is. If the
> hardware can't handle non-contiguous planes (never seen such hardware
> myself) then the driver must refuse the operation if userspace passes
> such a buffer to it.
> 
> I already posted a patch with a drm_framebuffer_check() function that
> does basic sanity checking on this stuff. I'll pull some checksa from
> that function and add them directly to drm_mode_addfb2(). Some of the
> checks require the driver to pass the BO sizes, so those I can't add.
> Also the plane overlap checks I had shouldn't be in generic code
> because some hardware can handle line-by-line interleaved planes, and
> my code would refuse those. Ideally the code should be improved to
> allow such interleaved planes, and then the check could be added to the
> generic code.
> 
> --
> Ville Syrjälä
> Intel OTC