Re: [PATCH RFC v3 4/6] drm: drm_fourcc: Add new formats for Xilinx IPs

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 22, 2018 at 06:41:24PM -0800, Hyun Kwon wrote:
> Hi Danidel,
> 
> Thanks for the comment.
> 
> On Mon, 2018-02-19 at 06:22:56 -0800, Daniel Vetter wrote:
> > On Fri, Feb 09, 2018 at 05:35:54PM -0800, Hyun Kwon wrote:
> > > This patch adds new formats needed by Xilinx IP. Pixels are not
> > > byte-aligned in these formats, and the drm_format_info for these
> > > formats has macro-pixel information.
> > > 
> > > Signed-off-by: Jeffrey Mouroux <jmouroux@xilinx.com>
> > > Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > > ---
> > > v3
> > > - Update entries for changes
> > > - Squash fourcc patch into this
> > > v2
> > > - Add detailed descriptions
> > > - Remove formats with no user
> > > ---
> > > ---
> > >  drivers/gpu/drm/drm_fourcc.c  | 2 ++
> > >  include/uapi/drm/drm_fourcc.h | 8 ++++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > index ed95de2..36bff7a 100644
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -168,6 +168,8 @@ const struct drm_format_info *__drm_format_info(u32 format)
> > >  		{ .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> > >  		{ .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> > >  		{ .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> > > +		{ .format = DRM_FORMAT_XV15,		.depth = 0,  .num_planes = 2, .pixels_per_macropixel = { 3, 3, 0 }, .bytes_per_macropixel = { 4, 8, 0 }, .hsub = 2, .vsub = 2, },
> > > +		{ .format = DRM_FORMAT_XV20,		.depth = 0,  .num_planes = 2, .pixels_per_macropixel = { 3, 3, 0 }, .bytes_per_macropixel = { 4, 8, 0 }, .hsub = 2, .vsub = 1, },
> > 
> > There's no need to set fields explicitly to 0. I think we could even do a
> > separate patch to nuke all the .depth = 0, assignments.
> > 
> > One thing that I've realized now that your new pixel formats stick out:
> > How is macropixel supposed to interact with hsub/vsub? From you example it
> > looks like macropixels are applied after subsampling (i.e. a macropixel
> > block of 3 pixels, but hsub = 2 means the macroblock will actually span 6
> > pixels). I think the kerneldoc in the earlier patch should explain this is
> > allowed, and how it's supposed to work exactly.
> > 
> > Also, do we have open-source userspace somewhere for this new pixel format?
> > 
> 
> We have modified modetest to test these formats. The change, especially
> the pattern generation part, isn't clean enough to be shared at the moment. But
> I can do some clean-up and share if that helps.
> 
> Then this change (may not be the latest set) was used to prototype the support
> in the gstreamer kmssink plug-in, but it's implemented by Collabora. Not sure
> if the change is accessible, but I can check.

Yeah the gstremare kmssink changes would be the userspace enabling we're
looking for for merging to upstream. Please include a link to the pull
requests/mailing list thread where those patches get reviewed in the
commit message for this.
-Daniel

> 
> I'll address rest of your comments.
> 
> Thanks,
> -hyun
> 
> > Thanks, Daniel
> > 
> > 
> > >  		{ .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > >  		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > >  		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index e04613d..6ac5282 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -142,6 +142,14 @@ extern "C" {
> > >  #define DRM_FORMAT_NV42		fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */
> > >  
> > >  /*
> > > + * 2 plane 10 bit per component YCbCr
> > > + * index 0 = Y plane, [31:0] x:Y2:Y1:Y0 2:10:10:10 little endian
> > > + * index 1 = Cb:Cr plane, [63:0] x:Cb2:Cr2:Cb1:x:Cr1:Cb0:Cr0 2:10:10:10:2:10:10:10 little endian
> > > + */
> > > +#define DRM_FORMAT_XV15		fourcc_code('X', 'V', '1', '5') /* 2x2 subsampled Cb:Cr plane 2:10:10:10 */
> > > +#define DRM_FORMAT_XV20		fourcc_code('X', 'V', '2', '0') /* 2x1 subsampled Cb:Cr plane 2:10:10:10 */
> > > +
> > > +/*
> > >   * 3 plane YCbCr
> > >   * index 0: Y plane, [7:0] Y
> > >   * index 1: Cb plane, [7:0] Cb
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel