| Message ID | 1518226556-7181-5-git-send-email-hyun.kwon@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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 >
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. 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
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
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, }, { .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