| Message ID | 1552414556-5756-1-git-send-email-ayan.halder@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/10] drm/fourcc: Add AFBC yuv fourccs for Mali | expand |
Op 12-03-2019 om 19:16 schreef Ayan Halder: > From: Brian Starkey <brian.starkey@arm.com> > > As we look to enable AFBC using DRM format modifiers, we run into > problems which we've historically handled via vendor-private details > (i.e. gralloc, on Android). > > AFBC (as an encoding) is fully flexible, and for example YUV data can > be encoded into 1, 2 or 3 encoded "planes", much like the linear > equivalents. Component order is also meaningful, as AFBC doesn't > necessarily care about what each "channel" of the data it encodes > contains. Therefore ABGR8888 and RGBA8888 can be encoded in AFBC with > different representations. Similarly, 'X' components may be encoded > into AFBC streams in cases where a decoder expects to decode a 4th > component. > > In addition, AFBC is a licensable IP, meaning that to support the > ecosystem we need to ensure that _all_ AFBC users are able to describe > the encodings that they need. This is much better achieved by > preserving meaning in the fourcc codes when they are combined with an > AFBC modifier. > > In essence, we want to use the modifier to describe the parameters of > the AFBC encode/decode, and use the fourcc code to describe the data > being encoded/decoded. > > To do anything different would be to introduce redundancy - we would > need to duplicate in the modifier information which is _already_ > conveyed clearly and non-ambigiously by a fourcc code. > > I hope that for RGB this is non-controversial. > (BGRA8888 + MODIFIER_AFBC) is a different format from > (RGBA8888 + MODIFIER_AFBC). > > Possibly more controversial is that (XBGR8888 + MODIFIER_AFBC) > is different from (BGR888 + MODIFIER_AFBC). I understand that in some > schemes it is not the case - but in AFBC it is so. > > Where we run into problems is where there are not already fourcc codes > which represent the data which the AFBC encoder/decoder is processing. > To that end, we want to introduce new fourcc codes to describe the > data being encoded/decoded, in the places where none of the existing > fourcc codes are applicable. > > Where we don't support an equivalent non-compressed layout, or where > no "obvious" linear layout exists, we are proposing adding fourcc > codes which have no associated linear layout - because any layout we > proposed would be completely arbitrary. > > Some formats are following the naming conventions from [2]. > > The summary of the new formats is: > DRM_FORMAT_VUY888 - Packed 8-bit YUV 444. Y followed by U then V. > DRM_FORMAT_VUY101010 - Packed 10-bit YUV 444. Y followed by U then > V. No defined linear encoding. > DRM_FORMAT_Y210 - Packed 10-bit YUV 422. Y followed by U (then Y) > then V. 10-bit samples in 16-bit words. > DRM_FORMAT_Y410 - Packed 10-bit YUV 444, with 2-bit alpha. > DRM_FORMAT_P210 - Semi-planar 10-bit YUV 422. Y plane, followed by > interleaved U-then-V plane. 10-bit samples in > 16-bit words. > DRM_FORMAT_YUV420_8BIT - Packed 8-bit YUV 420. Y followed by U then > V. No defined linear encoding > DRM_FORMAT_YUV420_10BIT - Packed 10-bit YUV 420. Y followed by U > then V. No defined linear encoding > > Please also note that in the absence of AFBC, we would still need to > add Y410, Y210 and P210. > > Full rationale follows: > > YUV 444 8-bit, 1-plane > ---------------------- > The currently defined AYUV format encodes a 4th alpha component, > which makes it unsuitable for representing a 3-component YUV 444 > AFBC stream. > > The proposed[1] XYUV format which is supported by Mali-DP in linear > layout is also unsuitable, because the component order is the > opposite of the AFBC version, and it encodes a 4th 'X' component. > > DRM_FORMAT_VUY888 is the "obvious" format for a 3-component, packed, > YUV 444 8-bit format, with the component order which our HW expects to > encode/decode. It conforms to the same naming convention as the > existing packed YUV 444 format. > The naming here is meant to be consistent with DRM_FORMAT_AYUV and > DRM_FORMAT_XYUV[1] > > YUV 444 10-bit, 1-plane > ----------------------- > There is no currently-defined YUV 444 10-bit format in > drm_fourcc.h, irrespective of number of planes. > > The proposed[1] XVYU2101010 format which is supported by Mali-DP in > linear layout uses the wrong component order, and also encodes a 4th > 'X' component, which doesn't match the AFBC version of YUV 444 > 10-bit which we support. > > DRM_FORMAT_Y410 is the same layout as XVYU2101010, but with 2 bits of > alpha. This format is supported with linear layout by Mali GPUs. The > naming follows[2]. > > There is no "obvious" linear encoding for a 3-component 10:10:10 > packed format, and so DRM_FORMAT_VUY101010 defines a component > order, but not a bit encoding. Again, the naming is meant to be > consistent with DRM_FORMAT_AYUV. > > YUV 422 8-bit, 1-plane > ---------------------- > The existing DRM_FORMAT_YUYV (and the other component orders) are > single-planar YUV 422 8-bit formats. Following the convention of > the component orders of the RGB formats, YUYV has the correct > component order for our AFBC encoding (Y followed by U followed by > V). We can use YUYV for AFBC YUV 422 8-bit. > > YUV 422 10-bit, 1-plane > ----------------------- > There is no currently-defined YUV 422 10-bit format in drm_fourcc.h > > DRM_FORMAT_Y210 is analogous to YUYV, but with 10-bits per sample > packed into the upper 10-bits of 16-bit samples. This format is > supported in both linear and AFBC by Mali GPUs. > > YUV 422 10-bit, 2-plane > ----------------------- > The recently defined DRM_FORMAT_P010 format is a 10-bit semi-planar > YUV 420 format, which has the correct component ordering for an AFBC > 2-plane YUV 420 buffer. The linear layout contains meaningless padding > bits, which will not be encoded in an AFBC stream. > > YUV 420 8-bit, 1-plane > ---------------------- > There is no currently defined single-planar YUV 420, 8-bit format > in drm_fourcc.h. There's differing opinions on whether using the > existing fourcc-implied n_planes where possible is a good idea or > not when using modifiers. > > For me, it's much more "obvious" to use NV12 for 2-plane AFBC and > YUV420 for 3-plane AFBC. This keeps the aforementioned separation > between the AFBC codec settings (in the modifier) and the pixel data > format (in the fourcc). With different vendors using AFBC, this helps > to ensure that there is no confusion in interoperation. It also > ensures that the AFBC modifiers describe AFBC itself (which is a > licensable component), and not implementation details which are not > defined by AFBC. > > The proposed[1] X0L0 format which Mali-DP supports with Linear layout > is unsuitable, as it contains a 4th 'X' component, and our AFBC > decoder expects only 3 components. > > To that end, we propose a new YUV 420 8-bit format. There is no > "obvious" linear encoding for a 3-component 8:8:8, 420, packed format, > and so DRM_FORMAT_YUV420_8BIT defines a component order, but not a > bit encoding. I'm happy to hear different naming suggestions. > > YUV 420 8-bit, 2-, 3-plane > -------------------------- > These already exist, we can use NV12 and YUV420. > > YUV 420 10-bit, 1-plane > ----------------------- > As above, no current definition exists, and X0L2 encodes a 4th 'X' > channel. > > Analogous to DRM_FORMAT_YUV420_8BIT, we define DRM_FORMAT_YUV420_10BIT. > > [1] https://lists.freedesktop.org/archives/dri-devel/2018-July/184598.html > [2] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats > > Changes since RFC v1: > - Fix confusing subsampling vs bit-depth X:X:X notation in > descriptions (danvet) > - Rename DRM_FORMAT_AVYU1101010 to DRM_FORMAT_Y410 (Lisa Wu) > - Add drm_format_info structures for the new formats, using the > new 'bpp' field for those with non-integer bytes-per-pixel > - Rebase, including Juha-Pekka Heikkila's format definitions > > Changes since RFC v2: > - Rebase on top of latest changes in drm-misc-next > - Change the description of DRM_FORMAT_P210 in __drm_format_info and > drm_fourcc.h so as to make it consistent with other DRM_FORMAT_PXXX > formats. > > Changes since v3: > - Added the ack > - Rebased on the latest drm-misc-next > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > Acked-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> > --- > drivers/gpu/drm/drm_fourcc.c | 16 ++++++++++++++++ > include/uapi/drm/drm_fourcc.h | 20 ++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 45c9882..a9df743 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -225,6 +225,9 @@ const struct drm_format_info *__drm_format_info(u32 format) > { .format = DRM_FORMAT_UYVY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, > { .format = DRM_FORMAT_VYUY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, > { .format = DRM_FORMAT_XYUV8888, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true }, > + { .format = DRM_FORMAT_Y210, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, > + { .format = DRM_FORMAT_VUY888, .depth = 0, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true }, > + { .format = DRM_FORMAT_Y410, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true }, > { .format = DRM_FORMAT_AYUV, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true }, > { .format = DRM_FORMAT_Y210, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, > { .format = DRM_FORMAT_Y212, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, > @@ -253,6 +256,19 @@ 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_P210, .depth = 0, > + .num_planes = 2, .char_per_block = { 2, 4, 0 }, > + .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 }, .hsub = 2, > + .vsub = 1, .is_yuv = true }, > + { .format = DRM_FORMAT_VUY101010, .depth = 0, > + .num_planes = 1, .cpp = { 0, 0, 0 }, .hsub = 1, .vsub = 1, > + .is_yuv = true }, > + { .format = DRM_FORMAT_YUV420_8BIT, .depth = 0, > + .num_planes = 1, .cpp = { 0, 0, 0 }, .hsub = 2, .vsub = 2, > + .is_yuv = true }, > + { .format = DRM_FORMAT_YUV420_10BIT, .depth = 0, > + .num_planes = 1, .cpp = { 0, 0, 0 }, .hsub = 2, .vsub = 2, > + .is_yuv = true }, > }; > > unsigned int i; > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 9fa7cf7..b992a38 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -149,9 +149,13 @@ extern "C" { > #define DRM_FORMAT_YVYU fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */ > #define DRM_FORMAT_UYVY fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */ > #define DRM_FORMAT_VYUY fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */ > +#define DRM_FORMAT_Y210 fourcc_code('Y', '2', '1', '0') /* [63:0] Cr0:0:Y1:0:Cb0:0:Y0:0 10:6:10:6:10:6:10:6 little endian */ > > #define DRM_FORMAT_AYUV fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ > #define DRM_FORMAT_XYUV8888 fourcc_code('X', 'Y', 'U', 'V') /* [31:0] X:Y:Cb:Cr 8:8:8:8 little endian */ > +#define DRM_FORMAT_VUY888 fourcc_code('V', 'U', '2', '4') /* [23:0] Cr:Cb:Y 8:8:8 little endian */ > +#define DRM_FORMAT_Y410 fourcc_code('Y', '4', '1', '0') /* [31:0] A:Cr:Y:Cb 2:10:10:10 little endian */ > +#define DRM_FORMAT_VUY101010 fourcc_code('V', 'U', '3', '0') /* Y followed by U then V, 10:10:10. Non-linear modifier only */ > > /* > * packed Y2xx indicate for each component, xx valid data occupy msb > @@ -184,6 +188,15 @@ extern "C" { > #define DRM_FORMAT_X0L2 fourcc_code('X', '0', 'L', '2') > > /* > + * 1-plane YUV 4:2:0 > + * In these formats, the component ordering is specified (Y, followed by U > + * then V), but the exact Linear layout is undefined. > + * These formats can only be used with a non-Linear modifier. > + */ > +#define DRM_FORMAT_YUV420_8BIT fourcc_code('Y', 'U', '0', '8') > +#define DRM_FORMAT_YUV420_10BIT fourcc_code('Y', 'U', '1', '0') > + > +/* > * 2 plane RGB + A > * index 0 = RGB plane, same format as the corresponding non _A8 format has > * index 1 = A plane, [7:0] A > @@ -216,6 +229,13 @@ extern "C" { > * 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 > */ > +#define DRM_FORMAT_P210 fourcc_code('P', '2', '1', '0') /* 2x1 subsampled Cr:Cb plane, 10 bit per channel */ > + > +/* > + * 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 > + */ > #define DRM_FORMAT_P010 fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane 10 bits per channel */ > > /* Hey.. There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. Worse still is that one has set has_alpha to true for Y41x and other to false. ~Maarten
Hi, On Mon, Mar 18, 2019 at 11:17:55AM +0100, Maarten Lankhorst wrote: <snip> > Hey.. > > There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. > > Worse still is that one has set has_alpha to true for Y41x and other to false. > > ~Maarten > Oh that's sad :-( I think this fell through the cracks on our side when someone left our team. Also turns out I'm not subscribed to igt-dev. I see you commented the same on one of the previous patches, and that there was some discussion of this on the test patches too. I have been referring to Microsoft's page[1] as "the" source for these formats, which does indeed call out Y410 as having 2 bits of alpha. Our GPU expects alpha. Was there a specific reason for opting to change the test instead of the definition? Any way to get this changed now? It doesn't seem that sensible for the kernel to call something Y410 which doesn't match an "existing" definition by the same name. If alpha needs to be ignored on scanout, the alpha blend mode property can be used (more archaeology - I see that was still giving CRC failures, but that might be a "known issue" for all YUV on your HW?) -Brian [1] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats#444-formats
Op 18-03-2019 om 16:40 schreef Brian Starkey: > Hi, > > On Mon, Mar 18, 2019 at 11:17:55AM +0100, Maarten Lankhorst wrote: > > <snip> > >> Hey.. >> >> There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. >> >> Worse still is that one has set has_alpha to true for Y41x and other to false. >> >> ~Maarten >> > Oh that's sad :-( I think this fell through the cracks on our side > when someone left our team. Also turns out I'm not subscribed to > igt-dev. > > I see you commented the same on one of the previous patches, and that > there was some discussion of this on the test patches too. > > I have been referring to Microsoft's page[1] as "the" source for these > formats, which does indeed call out Y410 as having 2 bits of alpha. > Our GPU expects alpha. Ah. Yeah there has been discussion on whether there was supposed to be alpha or not, but the original discussion on HDR formats has been completely ignored by arm. The patch had originally a few arm devs on cc and was sent to dri-devel with linux-media cc'd. Was sad to see it completely ignored so after having been sent twice I pushed it. > Was there a specific reason for opting to change the test instead of > the definition? Any way to get this changed now? > > It doesn't seem that sensible for the kernel to call something Y410 > which doesn't match an "existing" definition by the same name. If > alpha needs to be ignored on scanout, the alpha blend mode property > can be used (more archaeology - I see that was still giving CRC > failures, but that might be a "known issue" for all YUV on your HW?) Were a few bugs, but should be fixed now. :) Well only that we didn't have hw supporting alpha, and didn't hear back from others so we went without alpha. > -Brian > > [1] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats#444-formats
On Mon, Mar 18, 2019 at 07:12:24PM +0100, Maarten Lankhorst wrote: > Op 18-03-2019 om 16:40 schreef Brian Starkey: > > Hi, > > > > On Mon, Mar 18, 2019 at 11:17:55AM +0100, Maarten Lankhorst wrote: > > > > <snip> > > > >> Hey.. > >> > >> There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. > >> > >> Worse still is that one has set has_alpha to true for Y41x and other to false. > >> > >> ~Maarten > >> > > Oh that's sad :-( I think this fell through the cracks on our side > > when someone left our team. Also turns out I'm not subscribed to > > igt-dev. > > > > I see you commented the same on one of the previous patches, and that > > there was some discussion of this on the test patches too. > > > > I have been referring to Microsoft's page[1] as "the" source for these > > formats, which does indeed call out Y410 as having 2 bits of alpha. > > Our GPU expects alpha. > > Ah. Yeah there has been discussion on whether there was supposed to be alpha or not, but the original discussion on HDR formats has been completely ignored by arm. > > The patch had originally a few arm devs on cc and was sent to dri-devel with linux-media cc'd. Was sad to see it completely ignored so after having been sent twice I pushed it. That's the kernel patch(es)? It looks like I did receive them via dri-devel, and Ayan was Cc'd on two of the series', but it's disingenuous to say they had "a few Arm devs". I first submitted this patch with Y410 to dri-devel back in August, and since then it's been sent 8 times by my count (with you on Cc on all of them!), and all have been similarly completely ignored; so I'm sorry but I don't think you can put the blame entirely with Arm here. > > > Was there a specific reason for opting to change the test instead of > > the definition? Any way to get this changed now? > > > > It doesn't seem that sensible for the kernel to call something Y410 > > which doesn't match an "existing" definition by the same name. If > > alpha needs to be ignored on scanout, the alpha blend mode property > > can be used (more archaeology - I see that was still giving CRC > > failures, but that might be a "known issue" for all YUV on your HW?) > > Were a few bugs, but should be fixed now. :) > > Well only that we didn't have hw supporting alpha, and didn't hear back from others so we went without alpha. So what do you suggest? Can we change it, or we need to forever live with divergent definitions in DRM vs elsewhere? Gstreamer appears to have a Y410 definition including alpha[1], and there's the aforementioned Microsoft page too. To me it looks like we should change DRM, and if your HW supports something that's "almost but not quite" Y410 then you need a different name or only allow alpha-blend-mode "none". Thanks, -Brian [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/commit/0ac7d1187b234e86157ad74937c249a3c016807c > > > -Brian > > > > [1] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats#444-formats > >
On Mon, Mar 18, 2019 at 07:00:57PM +0000, Brian Starkey wrote: > On Mon, Mar 18, 2019 at 07:12:24PM +0100, Maarten Lankhorst wrote: > > Op 18-03-2019 om 16:40 schreef Brian Starkey: > > > Hi, > > > > > > On Mon, Mar 18, 2019 at 11:17:55AM +0100, Maarten Lankhorst wrote: > > > > > > <snip> > > > > > >> Hey.. > > >> > > >> There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. > > >> > > >> Worse still is that one has set has_alpha to true for Y41x and other to false. > > >> > > >> ~Maarten > > >> > > > Oh that's sad :-( I think this fell through the cracks on our side > > > when someone left our team. Also turns out I'm not subscribed to > > > igt-dev. > > > > > > I see you commented the same on one of the previous patches, and that > > > there was some discussion of this on the test patches too. > > > > > > I have been referring to Microsoft's page[1] as "the" source for these > > > formats, which does indeed call out Y410 as having 2 bits of alpha. > > > Our GPU expects alpha. > > > > Ah. Yeah there has been discussion on whether there was supposed to be alpha or not, but the original discussion on HDR formats has been completely ignored by arm. > > > > The patch had originally a few arm devs on cc and was sent to dri-devel with linux-media cc'd. Was sad to see it completely ignored so after having been sent twice I pushed it. > > That's the kernel patch(es)? It looks like I did receive them via > dri-devel, and Ayan was Cc'd on two of the series', but it's > disingenuous to say they had "a few Arm devs". > > I first submitted this patch with Y410 to dri-devel back in August, > and since then it's been sent 8 times by my count (with you on Cc on > all of them!), and all have been similarly completely ignored; so I'm > sorry but I don't think you can put the blame entirely with Arm here. > > > > > > Was there a specific reason for opting to change the test instead of > > > the definition? Any way to get this changed now? > > > > > > It doesn't seem that sensible for the kernel to call something Y410 > > > which doesn't match an "existing" definition by the same name. If > > > alpha needs to be ignored on scanout, the alpha blend mode property > > > can be used (more archaeology - I see that was still giving CRC > > > failures, but that might be a "known issue" for all YUV on your HW?) > > > > Were a few bugs, but should be fixed now. :) > > > > Well only that we didn't have hw supporting alpha, and didn't hear back from others so we went without alpha. > > So what do you suggest? Can we change it, or we need to forever live > with divergent definitions in DRM vs elsewhere? > > Gstreamer appears to have a Y410 definition including alpha[1], and > there's the aforementioned Microsoft page too. > > To me it looks like we should change DRM, and if your HW supports > something that's "almost but not quite" Y410 then you need a different > name or only allow alpha-blend-mode "none". > > Thanks, > -Brian > > [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/commit/0ac7d1187b234e86157ad74937c249a3c016807c Eh. I'm not familiar enough with the gitlab UI. Looks like this didn't merge in GStreamer so far. > > > > > -Brian > > > > > > [1] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats#444-formats > > > >
On Mon, Mar 18, 2019 at 07:06:39PM +0000, Brian Starkey wrote: > > [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/commit/0ac7d1187b234e86157ad74937c249a3c016807c > > Eh. I'm not familiar enough with the gitlab UI. Looks like this didn't > merge in GStreamer so far. Gah! Still wrong. It's in 1.16, with Alpha: https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-GstVideo.html#GstVideoFormat > > > > > > > > -Brian > > > > > > > > [1] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats#444-formats > > > > > >
On Mon, Mar 18, 2019 at 07:12:24PM +0100, Maarten Lankhorst wrote: > Op 18-03-2019 om 16:40 schreef Brian Starkey: > > Hi, > > > > On Mon, Mar 18, 2019 at 11:17:55AM +0100, Maarten Lankhorst wrote: > > > > <snip> > > > >> Hey.. > >> > >> There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. > >> > >> Worse still is that one has set has_alpha to true for Y41x and other to false. > >> > >> ~Maarten > >> > > Oh that's sad :-( I think this fell through the cracks on our side > > when someone left our team. Also turns out I'm not subscribed to > > igt-dev. > > > > I see you commented the same on one of the previous patches, and that > > there was some discussion of this on the test patches too. > > > > I have been referring to Microsoft's page[1] as "the" source for these > > formats, which does indeed call out Y410 as having 2 bits of alpha. > > Our GPU expects alpha. > > Ah. Yeah there has been discussion on whether there was supposed to be alpha or not, but the original discussion on HDR formats has been completely ignored by arm. > > The patch had originally a few arm devs on cc and was sent to dri-devel with linux-media cc'd. Was sad to see it completely ignored so after having been sent twice I pushed it. Apologies, I see that I was cc-ed in the mail 'drm: Add Y2xx and Y4xx (xx:10/12/16) format definitions and fourcc' sent by swati2.sharma@intel.com. It got lost in my pile of unread mails. :( About this patch, I had tagged you in irc channel (https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2019-03-11&show_html=true) for reviewing this seies. Did not hear back from you then ? > > > Was there a specific reason for opting to change the test instead of > > the definition? Any way to get this changed now? > > > > It doesn't seem that sensible for the kernel to call something Y410 > > which doesn't match an "existing" definition by the same name. If > > alpha needs to be ignored on scanout, the alpha blend mode property > > can be used (more archaeology - I see that was still giving CRC > > failures, but that might be a "known issue" for all YUV on your HW?) > > Were a few bugs, but should be fixed now. :) > > Well only that we didn't have hw supporting alpha, and didn't hear back from others so we went without alpha. In light of the suggestions made by brian.starkey@arm.com, I think changing the format from Y410 to X410 (in your case) might make sense as the alpha bits are absent. If this suggestion looks reasonable to you, I can volunteer myself to make this change in topic/hdr-formats. > > > -Brian > > > > [1] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats#444-formats >
Hey, Op 19-03-2019 om 00:27 schreef Ayan Halder: > On Mon, Mar 18, 2019 at 07:12:24PM +0100, Maarten Lankhorst wrote: >> Op 18-03-2019 om 16:40 schreef Brian Starkey: >>> Hi, >>> >>> On Mon, Mar 18, 2019 at 11:17:55AM +0100, Maarten Lankhorst wrote: >>> >>> <snip> >>> >>>> Hey.. >>>> >>>> There's a conflict with this patch and the merge of topic/hdr-formats, resulting in double definitions for Y210, Y410 and P010. >>>> >>>> Worse still is that one has set has_alpha to true for Y41x and other to false. >>>> >>>> ~Maarten >>>> >>> Oh that's sad :-( I think this fell through the cracks on our side >>> when someone left our team. Also turns out I'm not subscribed to >>> igt-dev. >>> >>> I see you commented the same on one of the previous patches, and that >>> there was some discussion of this on the test patches too. >>> >>> I have been referring to Microsoft's page[1] as "the" source for these >>> formats, which does indeed call out Y410 as having 2 bits of alpha. >>> Our GPU expects alpha. >> Ah. Yeah there has been discussion on whether there was supposed to be alpha or not, but the original discussion on HDR formats has been completely ignored by arm. >> >> The patch had originally a few arm devs on cc and was sent to dri-devel with linux-media cc'd. Was sad to see it completely ignored so after having been sent twice I pushed it. > Apologies, I see that I was cc-ed in the mail 'drm: Add Y2xx and Y4xx > (xx:10/12/16) format definitions and fourcc' sent by > swati2.sharma@intel.com. It got lost in my pile of unread mails. :( > > About this patch, I had tagged you in irc channel > (https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2019-03-11&show_html=true) > for reviewing this seies. Did not hear back from you then ? Looks like we both unfortunately not looked at each others series then, or at least I missed the HDR formats part. >>> Was there a specific reason for opting to change the test instead of >>> the definition? Any way to get this changed now? >>> >>> It doesn't seem that sensible for the kernel to call something Y410 >>> which doesn't match an "existing" definition by the same name. If >>> alpha needs to be ignored on scanout, the alpha blend mode property >>> can be used (more archaeology - I see that was still giving CRC >>> failures, but that might be a "known issue" for all YUV on your HW?) >> Were a few bugs, but should be fixed now. :) >> >> Well only that we didn't have hw supporting alpha, and didn't hear back from others so we went without alpha. > In light of the suggestions made by brian.starkey@arm.com, I think > changing the format from Y410 to X410 (in your case) might make sense > as the alpha bits are absent. > If this suggestion looks reasonable to you, I can volunteer myself to make > this change in topic/hdr-formats. I sent a patch to fix the conflicts. There's already XVYU2101010 in that series, which is Y410 without alpha. I've extended it and used it to convert i915. It's unfortunate those conflicts happen, but fortunately it didn't spread to drm-next or linux-next. Link: https://patchwork.freedesktop.org/patch/292977/?series=58182&rev=1 Tested it, basic tests seems to work for i915. ~Maarten
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 45c9882..a9df743 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -225,6 +225,9 @@ const struct drm_format_info *__drm_format_info(u32 format) { .format = DRM_FORMAT_UYVY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_VYUY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_XYUV8888, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true }, + { .format = DRM_FORMAT_Y210, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, + { .format = DRM_FORMAT_VUY888, .depth = 0, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true }, + { .format = DRM_FORMAT_Y410, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true }, { .format = DRM_FORMAT_AYUV, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true }, { .format = DRM_FORMAT_Y210, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, { .format = DRM_FORMAT_Y212, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true }, @@ -253,6 +256,19 @@ 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_P210, .depth = 0, + .num_planes = 2, .char_per_block = { 2, 4, 0 }, + .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 }, .hsub = 2, + .vsub = 1, .is_yuv = true }, + { .format = DRM_FORMAT_VUY101010, .depth = 0, + .num_planes = 1, .cpp = { 0, 0, 0 }, .hsub = 1, .vsub = 1, + .is_yuv = true }, + { .format = DRM_FORMAT_YUV420_8BIT, .depth = 0, + .num_planes = 1, .cpp = { 0, 0, 0 }, .hsub = 2, .vsub = 2, + .is_yuv = true }, + { .format = DRM_FORMAT_YUV420_10BIT, .depth = 0, + .num_planes = 1, .cpp = { 0, 0, 0 }, .hsub = 2, .vsub = 2, + .is_yuv = true }, }; unsigned int i; diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 9fa7cf7..b992a38 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -149,9 +149,13 @@ extern "C" { #define DRM_FORMAT_YVYU fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */ #define DRM_FORMAT_UYVY fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */ #define DRM_FORMAT_VYUY fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */ +#define DRM_FORMAT_Y210 fourcc_code('Y', '2', '1', '0') /* [63:0] Cr0:0:Y1:0:Cb0:0:Y0:0 10:6:10:6:10:6:10:6 little endian */ #define DRM_FORMAT_AYUV fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ #define DRM_FORMAT_XYUV8888 fourcc_code('X', 'Y', 'U', 'V') /* [31:0] X:Y:Cb:Cr 8:8:8:8 little endian */ +#define DRM_FORMAT_VUY888 fourcc_code('V', 'U', '2', '4') /* [23:0] Cr:Cb:Y 8:8:8 little endian */ +#define DRM_FORMAT_Y410 fourcc_code('Y', '4', '1', '0') /* [31:0] A:Cr:Y:Cb 2:10:10:10 little endian */ +#define DRM_FORMAT_VUY101010 fourcc_code('V', 'U', '3', '0') /* Y followed by U then V, 10:10:10. Non-linear modifier only */ /* * packed Y2xx indicate for each component, xx valid data occupy msb @@ -184,6 +188,15 @@ extern "C" { #define DRM_FORMAT_X0L2 fourcc_code('X', '0', 'L', '2') /* + * 1-plane YUV 4:2:0 + * In these formats, the component ordering is specified (Y, followed by U + * then V), but the exact Linear layout is undefined. + * These formats can only be used with a non-Linear modifier. + */ +#define DRM_FORMAT_YUV420_8BIT fourcc_code('Y', 'U', '0', '8') +#define DRM_FORMAT_YUV420_10BIT fourcc_code('Y', 'U', '1', '0') + +/* * 2 plane RGB + A * index 0 = RGB plane, same format as the corresponding non _A8 format has * index 1 = A plane, [7:0] A @@ -216,6 +229,13 @@ extern "C" { * 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 */ +#define DRM_FORMAT_P210 fourcc_code('P', '2', '1', '0') /* 2x1 subsampled Cr:Cb plane, 10 bit per channel */ + +/* + * 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 + */ #define DRM_FORMAT_P010 fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane 10 bits per channel */ /*