| Message ID | 20200604135317.9235-2-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | media: meson: vdec: Add support for compressed framebuffer | expand |
Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : > From: Maxime Jourdan <mjourdan@baylibre.com> > > Add two generic Compressed Framebuffer pixel formats to be used > with a modifier when imported back in another subsystem like DRM/KMS. > > These pixel formats represents generic 8bits and 10bits compressed buffers > with a vendor specific layout. > > These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT > used to describe the underlying compressed buffers used for ARM Framebuffer > Compression. In the Amlogic case, the compression is different but the > underlying buffer components is the same. > > Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ > include/uapi/linux/videodev2.h | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 2322f08a98be..8f14adfd5bc5 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; > case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; > case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; > + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; > + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; When I read the DRM documentation [0], I'm reading that YUV420_8BIT definition matches V4L2_PIX_FMT_YVU420 and V4L2_PIX_FMT_YVU420M fully. In fact, on DRM side, to represent that format you want to expose here, they will strictly combine this generic format (documented un- compressed) with a modifier generated with the macro DRM_FORMAT_MOD_ARM_AFBC(*). And only the combination represent a unique and share-able format. In absence of modifier in V4L2 API, this compressed format should be named accordingly to the compressed algorithm used (something like FMT_YUV420_8BIT_AML_FBC). So I believe these format name cannot be copied as-is like this, as they can only introduce more ambiguity in the already quite hard to follow naming of pixel formats. In fact, it is very common to see multiple different vendor compressions on the same SoC, so I don't really believe a "generic" compressed format make sense. To site one, the IMX8M, which got Verrisillicon/Vivante DEC400 on the GPU, and the Hantro G2 compression format. Both will apply to NV12 class of format so in DRM they would be NV12 + modifier, and the combination forms the unique format. Now, in term of sharing, they must be differiented by userspace, as support for compression/decompression is heterogeneous (in that case the GPU does not support Hantro G2 decompression or compression, and the VPU does not support DEC400). I'll remind that the modifier implementation has great value and is much more scalable then the current V4L2 approach. There has been some early proposal for this, maybe it's time to prioritize because this list will starts growing with hundred or even thousands or format, which is clearly indicated by the increase of modifier generator macro on the DRM side. > default: > if (fmt->description[0]) > return; > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index c3a1cf1c507f..90b9949acb8a 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -705,6 +705,15 @@ struct v4l2_pix_format { > #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ > #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ > > +/* > + * Compressed Luminance+Chrominance meta-formats > + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ > +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ > + > /* Vendor-specific formats */ > #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ > #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */
Le vendredi 05 juin 2020 à 11:35 -0400, Nicolas Dufresne a écrit : > Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : > > From: Maxime Jourdan <mjourdan@baylibre.com> > > > > Add two generic Compressed Framebuffer pixel formats to be used > > with a modifier when imported back in another subsystem like DRM/KMS. > > > > These pixel formats represents generic 8bits and 10bits compressed buffers > > with a vendor specific layout. > > > > These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT > > used to describe the underlying compressed buffers used for ARM Framebuffer > > Compression. In the Amlogic case, the compression is different but the > > underlying buffer components is the same. > > > > Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ > > include/uapi/linux/videodev2.h | 9 +++++++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 2322f08a98be..8f14adfd5bc5 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > > case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; > > case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; > > case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; > > + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; > > + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; > > When I read the DRM documentation [0], I'm reading that YUV420_8BIT > definition matches V4L2_PIX_FMT_YVU420 and V4L2_PIX_FMT_YVU420M fully. > In fact, on DRM side, to represent that format you want to expose here, > they will strictly combine this generic format (documented un- > compressed) with a modifier generated with the macro > DRM_FORMAT_MOD_ARM_AFBC(*). And only the combination represent a unique > and share-able format. [0] https://www.kernel.org/doc/html/v5.5-rc1/gpu/afbc.html > > In absence of modifier in V4L2 API, this compressed format should be > named accordingly to the compressed algorithm used (something like > FMT_YUV420_8BIT_AML_FBC). So I believe these format name cannot be > copied as-is like this, as they can only introduce more ambiguity in > the already quite hard to follow naming of pixel formats. In fact, it > is very common to see multiple different vendor compressions on the > same SoC, so I don't really believe a "generic" compressed format make > sense. To site one, the IMX8M, which got Verrisillicon/Vivante DEC400 > on the GPU, and the Hantro G2 compression format. Both will apply to > NV12 class of format so in DRM they would be NV12 + modifier, and the > combination forms the unique format. Now, in term of sharing, they must > be differiented by userspace, as support for compression/decompression > is heterogeneous (in that case the GPU does not support Hantro G2 > decompression or compression, and the VPU does not support DEC400). > > I'll remind that the modifier implementation has great value and is > much more scalable then the current V4L2 approach. There has been some > early proposal for this, maybe it's time to prioritize because this > list will starts growing with hundred or even thousands or format, > which is clearly indicated by the increase of modifier generator macro > on the DRM side. > > > default: > > if (fmt->description[0]) > > return; > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > index c3a1cf1c507f..90b9949acb8a 100644 > > --- a/include/uapi/linux/videodev2.h > > +++ b/include/uapi/linux/videodev2.h > > @@ -705,6 +705,15 @@ struct v4l2_pix_format { > > #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ > > #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ > > > > +/* > > + * Compressed Luminance+Chrominance meta-formats > > + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ > > +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ > > + > > /* Vendor-specific formats */ > > #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ > > #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */
Hi Nicolas, On 05/06/2020 17:35, Nicolas Dufresne wrote: > Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : >> From: Maxime Jourdan <mjourdan@baylibre.com> >> >> Add two generic Compressed Framebuffer pixel formats to be used >> with a modifier when imported back in another subsystem like DRM/KMS. >> >> These pixel formats represents generic 8bits and 10bits compressed buffers >> with a vendor specific layout. >> >> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT >> used to describe the underlying compressed buffers used for ARM Framebuffer >> Compression. In the Amlogic case, the compression is different but the >> underlying buffer components is the same. >> >> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ >> include/uapi/linux/videodev2.h | 9 +++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 2322f08a98be..8f14adfd5bc5 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; >> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; >> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; >> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; >> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; Seems we are totally on-par with the following :-) > > When I read the DRM documentation [0], I'm reading that YUV420_8BIT > definition matches V4L2_PIX_FMT_YVU420 and V4L2_PIX_FMT_YVU420M fully. > In fact, on DRM side, to represent that format you want to expose here, > they will strictly combine this generic format (documented un- > compressed) with a modifier generated with the macro > DRM_FORMAT_MOD_ARM_AFBC(*). And only the combination represent a unique > and share-able format. Yes, and this is exactly my goal here, and matches the Amlogic Framebuffer as described in patch 4. The modifier patchset is at [1]. > > In absence of modifier in V4L2 API, this compressed format should be > named accordingly to the compressed algorithm used (something like > FMT_YUV420_8BIT_AML_FBC). It's even more complex, the modifier depends on the SoC revision, so we can have up to6 different unique pixel format instead of 2 with a variable modifier. > So I believe these format name cannot be > copied as-is like this, as they can only introduce more ambiguity in > the already quite hard to follow naming of pixel formats. In fact, it > is very common to see multiple different vendor compressions on the > same SoC, so I don't really believe a "generic" compressed format make > sense. To site one, the IMX8M, which got Verrisillicon/Vivante DEC400 > on the GPU, and the Hantro G2 compression format. Both will apply to > NV12 class of format so in DRM they would be NV12 + modifier, and the > combination forms the unique format. Now, in term of sharing, they must > be differiented by userspace, as support for compression/decompression > is heterogeneous (in that case the GPU does not support Hantro G2 > decompression or compression, and the VPU does not support DEC400). > > I'll remind that the modifier implementation has great value and is > much more scalable then the current V4L2 approach. There has been some > early proposal for this, maybe it's time to prioritize because this > list will starts growing with hundred or even thousands or format, > which is clearly indicated by the increase of modifier generator macro > on the DRM side. Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one is decided, I'm stuck and this is the only intermediate solution I found. We have a working solution with Boris's patchset with ext_fmt passing the modifier to user-space. but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT will still be needed if we pass the modifier with an extended format struct. > >> default: >> if (fmt->description[0]) >> return; >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index c3a1cf1c507f..90b9949acb8a 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -705,6 +705,15 @@ struct v4l2_pix_format { >> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ >> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ >> >> +/* >> + * Compressed Luminance+Chrominance meta-formats >> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ >> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ >> + >> /* Vendor-specific formats */ >> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ >> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ > [1] https://patchwork.freedesktop.org/series/73722/#rev7
On 08/06/2020 10:16, Neil Armstrong wrote: > Hi Nicolas, > > On 05/06/2020 17:35, Nicolas Dufresne wrote: >> Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : >>> From: Maxime Jourdan <mjourdan@baylibre.com> >>> >>> Add two generic Compressed Framebuffer pixel formats to be used >>> with a modifier when imported back in another subsystem like DRM/KMS. >>> >>> These pixel formats represents generic 8bits and 10bits compressed buffers >>> with a vendor specific layout. >>> >>> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT >>> used to describe the underlying compressed buffers used for ARM Framebuffer >>> Compression. In the Amlogic case, the compression is different but the >>> underlying buffer components is the same. >>> >>> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ >>> include/uapi/linux/videodev2.h | 9 +++++++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>> index 2322f08a98be..8f14adfd5bc5 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; >>> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; >>> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; >>> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; >>> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; > > Seems we are totally on-par with the following :-) > >> >> When I read the DRM documentation [0], I'm reading that YUV420_8BIT >> definition matches V4L2_PIX_FMT_YVU420 and V4L2_PIX_FMT_YVU420M fully. >> In fact, on DRM side, to represent that format you want to expose here, >> they will strictly combine this generic format (documented un- >> compressed) with a modifier generated with the macro >> DRM_FORMAT_MOD_ARM_AFBC(*). And only the combination represent a unique >> and share-able format. > > Yes, and this is exactly my goal here, and matches the Amlogic Framebuffer as > described in patch 4. The modifier patchset is at [1]. > >> >> In absence of modifier in V4L2 API, this compressed format should be >> named accordingly to the compressed algorithm used (something like >> FMT_YUV420_8BIT_AML_FBC). > > It's even more complex, the modifier depends on the SoC revision, so we can > have up to6 different unique pixel format instead of 2 with a variable > modifier. > >> So I believe these format name cannot be >> copied as-is like this, as they can only introduce more ambiguity in >> the already quite hard to follow naming of pixel formats. In fact, it >> is very common to see multiple different vendor compressions on the >> same SoC, so I don't really believe a "generic" compressed format make >> sense. To site one, the IMX8M, which got Verrisillicon/Vivante DEC400 >> on the GPU, and the Hantro G2 compression format. Both will apply to >> NV12 class of format so in DRM they would be NV12 + modifier, and the >> combination forms the unique format. Now, in term of sharing, they must >> be differiented by userspace, as support for compression/decompression >> is heterogeneous (in that case the GPU does not support Hantro G2 >> decompression or compression, and the VPU does not support DEC400). >> >> I'll remind that the modifier implementation has great value and is >> much more scalable then the current V4L2 approach. There has been some >> early proposal for this, maybe it's time to prioritize because this >> list will starts growing with hundred or even thousands or format, >> which is clearly indicated by the increase of modifier generator macro >> on the DRM side. > > Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one > is decided, I'm stuck and this is the only intermediate solution I found. We can safely assume that drm fourcc and v4l2 fourcc won't be merged. There is too much divergence and not enough interest in creating common fourccs. But we *do* want to share the modifiers. > > We have a working solution with Boris's patchset with ext_fmt passing the > modifier to user-space. > > but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT > will still be needed if we pass the modifier with an extended format struct. We tried merging fourccs but that ran into resistance. Frankly, I wouldn't bother with this, it is much easier to just create a conversion table in the kernel docs. So don't block on this, I would really prefer if the ext_fmt series is picked up again and rebased and reposted and then worked on. The stateless codec support is taking less time (it's shaping up well) so there is more time to work on this. I believe we really need this since v4l2_buffer and v4l2_format are a real mess. Regards, Hans > >> >>> default: >>> if (fmt->description[0]) >>> return; >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>> index c3a1cf1c507f..90b9949acb8a 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -705,6 +705,15 @@ struct v4l2_pix_format { >>> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ >>> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ >>> >>> +/* >>> + * Compressed Luminance+Chrominance meta-formats >>> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ >>> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ >>> + >>> /* Vendor-specific formats */ >>> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ >>> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ >> > > [1] https://patchwork.freedesktop.org/series/73722/#rev7 >
On 08/06/2020 11:26, Hans Verkuil wrote: > On 08/06/2020 10:16, Neil Armstrong wrote: >> Hi Nicolas, >> >> On 05/06/2020 17:35, Nicolas Dufresne wrote: >>> Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : >>>> From: Maxime Jourdan <mjourdan@baylibre.com> >>>> >>>> Add two generic Compressed Framebuffer pixel formats to be used >>>> with a modifier when imported back in another subsystem like DRM/KMS. >>>> >>>> These pixel formats represents generic 8bits and 10bits compressed buffers >>>> with a vendor specific layout. >>>> >>>> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT >>>> used to describe the underlying compressed buffers used for ARM Framebuffer >>>> Compression. In the Amlogic case, the compression is different but the >>>> underlying buffer components is the same. >>>> >>>> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ >>>> include/uapi/linux/videodev2.h | 9 +++++++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>> index 2322f08a98be..8f14adfd5bc5 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>>> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; >>>> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; >>>> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; >>>> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; >>>> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; [..] >>> >>> I'll remind that the modifier implementation has great value and is >>> much more scalable then the current V4L2 approach. There has been some >>> early proposal for this, maybe it's time to prioritize because this >>> list will starts growing with hundred or even thousands or format, >>> which is clearly indicated by the increase of modifier generator macro >>> on the DRM side. >> >> Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one >> is decided, I'm stuck and this is the only intermediate solution I found. > > We can safely assume that drm fourcc and v4l2 fourcc won't be merged. > > There is too much divergence and not enough interest in creating common > fourccs. > > But we *do* want to share the modifiers. > >> >> We have a working solution with Boris's patchset with ext_fmt passing the >> modifier to user-space. >> >> but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT >> will still be needed if we pass the modifier with an extended format struct. > > We tried merging fourccs but that ran into resistance. Frankly, I wouldn't > bother with this, it is much easier to just create a conversion table in the > kernel docs. > > So don't block on this, I would really prefer if the ext_fmt series is picked > up again and rebased and reposted and then worked on. The stateless codec support > is taking less time (it's shaping up well) so there is more time to work on this. Ok, I already starting discussing with Helen Koike about the ext_fnt re-spin. Should I re-introduce different v4l2 pixfmt for these DRM YUV420_*BIT or I can keep this patch along the new ext_fmt and shared modifiers ? Neil > > I believe we really need this since v4l2_buffer and v4l2_format are a real mess. > > Regards, > > Hans > >> >>> >>>> default: >>>> if (fmt->description[0]) >>>> return; >>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>>> index c3a1cf1c507f..90b9949acb8a 100644 >>>> --- a/include/uapi/linux/videodev2.h >>>> +++ b/include/uapi/linux/videodev2.h >>>> @@ -705,6 +705,15 @@ struct v4l2_pix_format { >>>> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ >>>> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ >>>> >>>> +/* >>>> + * Compressed Luminance+Chrominance meta-formats >>>> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ >>>> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ >>>> + >>>> /* Vendor-specific formats */ >>>> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ >>>> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ >>> >> >> [1] https://patchwork.freedesktop.org/series/73722/#rev7 >> >
On 08/06/2020 16:14, Neil Armstrong wrote: > On 08/06/2020 11:26, Hans Verkuil wrote: >> On 08/06/2020 10:16, Neil Armstrong wrote: >>> Hi Nicolas, >>> >>> On 05/06/2020 17:35, Nicolas Dufresne wrote: >>>> Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : >>>>> From: Maxime Jourdan <mjourdan@baylibre.com> >>>>> >>>>> Add two generic Compressed Framebuffer pixel formats to be used >>>>> with a modifier when imported back in another subsystem like DRM/KMS. >>>>> >>>>> These pixel formats represents generic 8bits and 10bits compressed buffers >>>>> with a vendor specific layout. >>>>> >>>>> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT >>>>> used to describe the underlying compressed buffers used for ARM Framebuffer >>>>> Compression. In the Amlogic case, the compression is different but the >>>>> underlying buffer components is the same. >>>>> >>>>> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> >>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ >>>>> include/uapi/linux/videodev2.h | 9 +++++++++ >>>>> 2 files changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> index 2322f08a98be..8f14adfd5bc5 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>>>> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; >>>>> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; >>>>> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; >>>>> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; >>>>> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; > > [..] > >>>> >>>> I'll remind that the modifier implementation has great value and is >>>> much more scalable then the current V4L2 approach. There has been some >>>> early proposal for this, maybe it's time to prioritize because this >>>> list will starts growing with hundred or even thousands or format, >>>> which is clearly indicated by the increase of modifier generator macro >>>> on the DRM side. >>> >>> Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one >>> is decided, I'm stuck and this is the only intermediate solution I found. >> >> We can safely assume that drm fourcc and v4l2 fourcc won't be merged. >> >> There is too much divergence and not enough interest in creating common >> fourccs. >> >> But we *do* want to share the modifiers. >> >>> >>> We have a working solution with Boris's patchset with ext_fmt passing the >>> modifier to user-space. >>> >>> but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT >>> will still be needed if we pass the modifier with an extended format struct. >> >> We tried merging fourccs but that ran into resistance. Frankly, I wouldn't >> bother with this, it is much easier to just create a conversion table in the >> kernel docs. >> >> So don't block on this, I would really prefer if the ext_fmt series is picked >> up again and rebased and reposted and then worked on. The stateless codec support >> is taking less time (it's shaping up well) so there is more time to work on this. > > Ok, I already starting discussing with Helen Koike about the ext_fnt re-spin. > > Should I re-introduce different v4l2 pixfmt for these DRM YUV420_*BIT or I can keep this > patch along the new ext_fmt and shared modifiers ? So to be clear the DRM_FORMAT_YUV420_8BIT/10BIT fourccs define that this is a buffer containing compressed YUV420 in 8 or 10 bit and the modifier tells userspace which compression is used, right? And we would add V4L2_PIX_FMT_YUV420_8BIT/_10BIT that, I assume, use the same fourcc values as the DRM variants? Since these fourccs are basically useless without V4L2 modifier support it would only make sense in combination with the ext_fmt series. Regards, Hans > > Neil > >> >> I believe we really need this since v4l2_buffer and v4l2_format are a real mess. >> >> Regards, >> >> Hans >> >>> >>>> >>>>> default: >>>>> if (fmt->description[0]) >>>>> return; >>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>>>> index c3a1cf1c507f..90b9949acb8a 100644 >>>>> --- a/include/uapi/linux/videodev2.h >>>>> +++ b/include/uapi/linux/videodev2.h >>>>> @@ -705,6 +705,15 @@ struct v4l2_pix_format { >>>>> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ >>>>> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ >>>>> >>>>> +/* >>>>> + * Compressed Luminance+Chrominance meta-formats >>>>> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ >>>>> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ >>>>> + >>>>> /* Vendor-specific formats */ >>>>> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ >>>>> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ >>>> >>> >>> [1] https://patchwork.freedesktop.org/series/73722/#rev7 >>> >> >
Le lundi 08 juin 2020 à 16:43 +0200, Hans Verkuil a écrit : > On 08/06/2020 16:14, Neil Armstrong wrote: > > On 08/06/2020 11:26, Hans Verkuil wrote: > > > On 08/06/2020 10:16, Neil Armstrong wrote: > > > > Hi Nicolas, > > > > > > > > On 05/06/2020 17:35, Nicolas Dufresne wrote: > > > > > Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : > > > > > > From: Maxime Jourdan <mjourdan@baylibre.com> > > > > > > > > > > > > Add two generic Compressed Framebuffer pixel formats to be used > > > > > > with a modifier when imported back in another subsystem like DRM/KMS. > > > > > > > > > > > > These pixel formats represents generic 8bits and 10bits compressed buffers > > > > > > with a vendor specific layout. > > > > > > > > > > > > These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT > > > > > > used to describe the underlying compressed buffers used for ARM Framebuffer > > > > > > Compression. In the Amlogic case, the compression is different but the > > > > > > underlying buffer components is the same. > > > > > > > > > > > > Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> > > > > > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > > > > > --- > > > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ > > > > > > include/uapi/linux/videodev2.h | 9 +++++++++ > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > > > index 2322f08a98be..8f14adfd5bc5 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > > > @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > > > > > > case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; > > > > > > case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; > > > > > > case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; > > > > > > + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; > > > > > > + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; > > > > [..] > > > > > > > I'll remind that the modifier implementation has great value and is > > > > > much more scalable then the current V4L2 approach. There has been some > > > > > early proposal for this, maybe it's time to prioritize because this > > > > > list will starts growing with hundred or even thousands or format, > > > > > which is clearly indicated by the increase of modifier generator macro > > > > > on the DRM side. > > > > > > > > Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one > > > > is decided, I'm stuck and this is the only intermediate solution I found. > > > > > > We can safely assume that drm fourcc and v4l2 fourcc won't be merged. > > > > > > There is too much divergence and not enough interest in creating common > > > fourccs. > > > > > > But we *do* want to share the modifiers. > > > > > > > We have a working solution with Boris's patchset with ext_fmt passing the > > > > modifier to user-space. > > > > > > > > but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT > > > > will still be needed if we pass the modifier with an extended format struct. > > > > > > We tried merging fourccs but that ran into resistance. Frankly, I wouldn't > > > bother with this, it is much easier to just create a conversion table in the > > > kernel docs. > > > > > > So don't block on this, I would really prefer if the ext_fmt series is picked > > > up again and rebased and reposted and then worked on. The stateless codec support > > > is taking less time (it's shaping up well) so there is more time to work on this. > > > > Ok, I already starting discussing with Helen Koike about the ext_fnt re-spin. > > > > Should I re-introduce different v4l2 pixfmt for these DRM YUV420_*BIT or I can keep this > > patch along the new ext_fmt and shared modifiers ? > > So to be clear the DRM_FORMAT_YUV420_8BIT/10BIT fourccs define that this is a > buffer containing compressed YUV420 in 8 or 10 bit and the modifier tells userspace > which compression is used, right? > > And we would add V4L2_PIX_FMT_YUV420_8BIT/_10BIT that, I assume, use the same > fourcc values as the DRM variants? > > Since these fourccs are basically useless without V4L2 modifier support it would > only make sense in combination with the ext_fmt series. I personally still think that adding these fourcc will just create a source of confusion and that fourcc should not be tried to be matched at the cost of tripling the already duplicated pixel formats. Userspace already need to implement translation anyway. On DRM side, new fourcc was not create for NV12+modifier, I don't see why planar YUV420 has to be different, with or without ext_fmt. Nicolas > > Regards, > > Hans > > > Neil > > > > > I believe we really need this since v4l2_buffer and v4l2_format are a real mess. > > > > > > Regards, > > > > > > Hans > > > > > > > > > default: > > > > > > if (fmt->description[0]) > > > > > > return; > > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > > > > > index c3a1cf1c507f..90b9949acb8a 100644 > > > > > > --- a/include/uapi/linux/videodev2.h > > > > > > +++ b/include/uapi/linux/videodev2.h > > > > > > @@ -705,6 +705,15 @@ struct v4l2_pix_format { > > > > > > #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ > > > > > > #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ > > > > > > > > > > > > +/* > > > > > > + * Compressed Luminance+Chrominance meta-formats > > > > > > + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ > > > > > > +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ > > > > > > + > > > > > > /* Vendor-specific formats */ > > > > > > #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ > > > > > > #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ > > > > > > > > [1] https://patchwork.freedesktop.org/series/73722/#rev7 > > > >
Hi Nicolas, On 08/06/2020 20:59, Nicolas Dufresne wrote: > Le lundi 08 juin 2020 à 16:43 +0200, Hans Verkuil a écrit : >> On 08/06/2020 16:14, Neil Armstrong wrote: >>> On 08/06/2020 11:26, Hans Verkuil wrote: >>>> On 08/06/2020 10:16, Neil Armstrong wrote: >>>>> Hi Nicolas, >>>>> >>>>> On 05/06/2020 17:35, Nicolas Dufresne wrote: >>>>>> Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : >>>>>>> From: Maxime Jourdan <mjourdan@baylibre.com> >>>>>>> >>>>>>> Add two generic Compressed Framebuffer pixel formats to be used >>>>>>> with a modifier when imported back in another subsystem like DRM/KMS. >>>>>>> >>>>>>> These pixel formats represents generic 8bits and 10bits compressed buffers >>>>>>> with a vendor specific layout. >>>>>>> >>>>>>> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT >>>>>>> used to describe the underlying compressed buffers used for ARM Framebuffer >>>>>>> Compression. In the Amlogic case, the compression is different but the >>>>>>> underlying buffer components is the same. >>>>>>> >>>>>>> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> >>>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>>>>> --- >>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ >>>>>>> include/uapi/linux/videodev2.h | 9 +++++++++ >>>>>>> 2 files changed, 11 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>>>> index 2322f08a98be..8f14adfd5bc5 100644 >>>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>>>> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>>>>>> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; >>>>>>> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; >>>>>>> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; >>>>>>> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; >>>>>>> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; >>> >>> [..] >>> >>>>>> I'll remind that the modifier implementation has great value and is >>>>>> much more scalable then the current V4L2 approach. There has been some >>>>>> early proposal for this, maybe it's time to prioritize because this >>>>>> list will starts growing with hundred or even thousands or format, >>>>>> which is clearly indicated by the increase of modifier generator macro >>>>>> on the DRM side. >>>>> >>>>> Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one >>>>> is decided, I'm stuck and this is the only intermediate solution I found. >>>> >>>> We can safely assume that drm fourcc and v4l2 fourcc won't be merged. >>>> >>>> There is too much divergence and not enough interest in creating common >>>> fourccs. >>>> >>>> But we *do* want to share the modifiers. >>>> >>>>> We have a working solution with Boris's patchset with ext_fmt passing the >>>>> modifier to user-space. >>>>> >>>>> but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT >>>>> will still be needed if we pass the modifier with an extended format struct. >>>> >>>> We tried merging fourccs but that ran into resistance. Frankly, I wouldn't >>>> bother with this, it is much easier to just create a conversion table in the >>>> kernel docs. >>>> >>>> So don't block on this, I would really prefer if the ext_fmt series is picked >>>> up again and rebased and reposted and then worked on. The stateless codec support >>>> is taking less time (it's shaping up well) so there is more time to work on this. >>> >>> Ok, I already starting discussing with Helen Koike about the ext_fnt re-spin. >>> >>> Should I re-introduce different v4l2 pixfmt for these DRM YUV420_*BIT or I can keep this >>> patch along the new ext_fmt and shared modifiers ? >> >> So to be clear the DRM_FORMAT_YUV420_8BIT/10BIT fourccs define that this is a >> buffer containing compressed YUV420 in 8 or 10 bit and the modifier tells userspace >> which compression is used, right? >> >> And we would add V4L2_PIX_FMT_YUV420_8BIT/_10BIT that, I assume, use the same >> fourcc values as the DRM variants? >> >> Since these fourccs are basically useless without V4L2 modifier support it would >> only make sense in combination with the ext_fmt series. > > I personally still think that adding these fourcc will just create a > source of confusion and that fourcc should not be tried to be matched > at the cost of tripling the already duplicated pixel formats. Userspace > already need to implement translation anyway. By using the same fourcc + modifiers, the translation table would only be needed for v4l2-specific fourcc, by reusing the same it's not necessary anymore. We have a really simple ffmpeg implementation using ext_fmt, and it makes it generic. > > On DRM side, new fourcc was not create for NV12+modifier, I don't see > why planar YUV420 has to be different, with or without ext_fmt. These V4L2_PIX_FMT_YUV420_8BIT/_10BIT were added because of the compressed nature of buffers. It's not because of the modifiers, modifiers can be used we any fourcc to define vendor specific layout requirements or changes, but for compressed the underlying YUV buffer cannot be physically described by any YUV420 fourcc, so ARM introduced these fourcc to describe a virtual YUV420 8 or 10bit buffer which physical layout is defined by the modifier. They could have re-used DRM_FORMAT_YUV420, but it's a 2 plane fourcc, and the other describe a true single or multiple plane layout which are simply not true with ARM AFBC or Amlogic FBC. Neil > > Nicolas > >> >> Regards, >> >> Hans >> >>> Neil >>> >>>> I believe we really need this since v4l2_buffer and v4l2_format are a real mess. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>>>>> default: >>>>>>> if (fmt->description[0]) >>>>>>> return; >>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>>>>>> index c3a1cf1c507f..90b9949acb8a 100644 >>>>>>> --- a/include/uapi/linux/videodev2.h >>>>>>> +++ b/include/uapi/linux/videodev2.h >>>>>>> @@ -705,6 +705,15 @@ struct v4l2_pix_format { >>>>>>> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ >>>>>>> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ >>>>>>> >>>>>>> +/* >>>>>>> + * Compressed Luminance+Chrominance meta-formats >>>>>>> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ >>>>>>> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ >>>>>>> + >>>>>>> /* Vendor-specific formats */ >>>>>>> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ >>>>>>> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ >>>>> >>>>> [1] https://patchwork.freedesktop.org/series/73722/#rev7 >>>>> >
Adding Helen to the discussion. On Tue, 9 Jun 2020 at 04:43, Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi Nicolas, > > On 08/06/2020 20:59, Nicolas Dufresne wrote: > > Le lundi 08 juin 2020 à 16:43 +0200, Hans Verkuil a écrit : > >> On 08/06/2020 16:14, Neil Armstrong wrote: > >>> On 08/06/2020 11:26, Hans Verkuil wrote: > >>>> On 08/06/2020 10:16, Neil Armstrong wrote: > >>>>> Hi Nicolas, > >>>>> > >>>>> On 05/06/2020 17:35, Nicolas Dufresne wrote: > >>>>>> Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : > >>>>>>> From: Maxime Jourdan <mjourdan@baylibre.com> > >>>>>>> > >>>>>>> Add two generic Compressed Framebuffer pixel formats to be used > >>>>>>> with a modifier when imported back in another subsystem like DRM/KMS. > >>>>>>> > >>>>>>> These pixel formats represents generic 8bits and 10bits compressed buffers > >>>>>>> with a vendor specific layout. > >>>>>>> > >>>>>>> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT > >>>>>>> used to describe the underlying compressed buffers used for ARM Framebuffer > >>>>>>> Compression. In the Amlogic case, the compression is different but the > >>>>>>> underlying buffer components is the same. > >>>>>>> > >>>>>>> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> > >>>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >>>>>>> --- > >>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ > >>>>>>> include/uapi/linux/videodev2.h | 9 +++++++++ > >>>>>>> 2 files changed, 11 insertions(+) > >>>>>>> > >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > >>>>>>> index 2322f08a98be..8f14adfd5bc5 100644 > >>>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c > >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > >>>>>>> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > >>>>>>> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; > >>>>>>> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; > >>>>>>> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; > >>>>>>> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; > >>>>>>> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; > >>> > >>> [..] > >>> > >>>>>> I'll remind that the modifier implementation has great value and is > >>>>>> much more scalable then the current V4L2 approach. There has been some > >>>>>> early proposal for this, maybe it's time to prioritize because this > >>>>>> list will starts growing with hundred or even thousands or format, > >>>>>> which is clearly indicated by the increase of modifier generator macro > >>>>>> on the DRM side. > >>>>> > >>>>> Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one > >>>>> is decided, I'm stuck and this is the only intermediate solution I found. > >>>> > >>>> We can safely assume that drm fourcc and v4l2 fourcc won't be merged. > >>>> > >>>> There is too much divergence and not enough interest in creating common > >>>> fourccs. > >>>> > >>>> But we *do* want to share the modifiers. > >>>> > >>>>> We have a working solution with Boris's patchset with ext_fmt passing the > >>>>> modifier to user-space. > >>>>> > >>>>> but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT > >>>>> will still be needed if we pass the modifier with an extended format struct. > >>>> > >>>> We tried merging fourccs but that ran into resistance. Frankly, I wouldn't > >>>> bother with this, it is much easier to just create a conversion table in the > >>>> kernel docs. > >>>> > >>>> So don't block on this, I would really prefer if the ext_fmt series is picked > >>>> up again and rebased and reposted and then worked on. The stateless codec support > >>>> is taking less time (it's shaping up well) so there is more time to work on this. > >>> > >>> Ok, I already starting discussing with Helen Koike about the ext_fnt re-spin. > >>> > >>> Should I re-introduce different v4l2 pixfmt for these DRM YUV420_*BIT or I can keep this > >>> patch along the new ext_fmt and shared modifiers ? > >> > >> So to be clear the DRM_FORMAT_YUV420_8BIT/10BIT fourccs define that this is a > >> buffer containing compressed YUV420 in 8 or 10 bit and the modifier tells userspace > >> which compression is used, right? > >> > >> And we would add V4L2_PIX_FMT_YUV420_8BIT/_10BIT that, I assume, use the same > >> fourcc values as the DRM variants? > >> > >> Since these fourccs are basically useless without V4L2 modifier support it would > >> only make sense in combination with the ext_fmt series. > > > > I personally still think that adding these fourcc will just create a > > source of confusion and that fourcc should not be tried to be matched > > at the cost of tripling the already duplicated pixel formats. Userspace > > already need to implement translation anyway. > > By using the same fourcc + modifiers, the translation table would only be needed > for v4l2-specific fourcc, by reusing the same it's not necessary anymore. > We have a really simple ffmpeg implementation using ext_fmt, and it makes it > generic. > > > > > On DRM side, new fourcc was not create for NV12+modifier, I don't see > > why planar YUV420 has to be different, with or without ext_fmt. > > These V4L2_PIX_FMT_YUV420_8BIT/_10BIT were added because of the compressed nature > of buffers. It's not because of the modifiers, modifiers can be used we any fourcc > to define vendor specific layout requirements or changes, but for compressed the > underlying YUV buffer cannot be physically described by any YUV420 fourcc, so > ARM introduced these fourcc to describe a virtual YUV420 8 or 10bit buffer which > physical layout is defined by the modifier. > They could have re-used DRM_FORMAT_YUV420, but it's a 2 plane fourcc, and the other > describe a true single or multiple plane layout which are simply not true with > ARM AFBC or Amlogic FBC. > > Neil > > > > > Nicolas > > > >> > >> Regards, > >> > >> Hans > >> > >>> Neil > >>> > >>>> I believe we really need this since v4l2_buffer and v4l2_format are a real mess. > >>>> > >>>> Regards, > >>>> > >>>> Hans > >>>> > >>>>>>> default: > >>>>>>> if (fmt->description[0]) > >>>>>>> return; > >>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > >>>>>>> index c3a1cf1c507f..90b9949acb8a 100644 > >>>>>>> --- a/include/uapi/linux/videodev2.h > >>>>>>> +++ b/include/uapi/linux/videodev2.h > >>>>>>> @@ -705,6 +705,15 @@ struct v4l2_pix_format { > >>>>>>> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ > >>>>>>> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ > >>>>>>> > >>>>>>> +/* > >>>>>>> + * Compressed Luminance+Chrominance meta-formats > >>>>>>> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ > >>>>>>> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ > >>>>>>> + > >>>>>>> /* Vendor-specific formats */ > >>>>>>> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ > >>>>>>> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ > >>>>> > >>>>> [1] https://patchwork.freedesktop.org/series/73722/#rev7 > >>>>> > > >
Le mardi 09 juin 2020 à 07:28 -0300, Ezequiel Garcia a écrit : > Adding Helen to the discussion. > > On Tue, 9 Jun 2020 at 04:43, Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi Nicolas, > > > > On 08/06/2020 20:59, Nicolas Dufresne wrote: > > > Le lundi 08 juin 2020 à 16:43 +0200, Hans Verkuil a écrit : > > > > On 08/06/2020 16:14, Neil Armstrong wrote: > > > > > On 08/06/2020 11:26, Hans Verkuil wrote: > > > > > > On 08/06/2020 10:16, Neil Armstrong wrote: > > > > > > > Hi Nicolas, > > > > > > > > > > > > > > On 05/06/2020 17:35, Nicolas Dufresne wrote: > > > > > > > > Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : > > > > > > > > > From: Maxime Jourdan <mjourdan@baylibre.com> > > > > > > > > > > > > > > > > > > Add two generic Compressed Framebuffer pixel formats to be used > > > > > > > > > with a modifier when imported back in another subsystem like DRM/KMS. > > > > > > > > > > > > > > > > > > These pixel formats represents generic 8bits and 10bits compressed buffers > > > > > > > > > with a vendor specific layout. > > > > > > > > > > > > > > > > > > These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT > > > > > > > > > used to describe the underlying compressed buffers used for ARM Framebuffer > > > > > > > > > Compression. In the Amlogic case, the compression is different but the > > > > > > > > > underlying buffer components is the same. > > > > > > > > > > > > > > > > > > Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> > > > > > > > > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > > > > > > > > --- > > > > > > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ > > > > > > > > > include/uapi/linux/videodev2.h | 9 +++++++++ > > > > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > > > > > > index 2322f08a98be..8f14adfd5bc5 100644 > > > > > > > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > > > > > > @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > > > > > > > > > case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; > > > > > > > > > case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; > > > > > > > > > case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; > > > > > > > > > + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; > > > > > > > > > + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; > > > > > > > > > > [..] > > > > > > > > > > > > > I'll remind that the modifier implementation has great value and is > > > > > > > > much more scalable then the current V4L2 approach. There has been some > > > > > > > > early proposal for this, maybe it's time to prioritize because this > > > > > > > > list will starts growing with hundred or even thousands or format, > > > > > > > > which is clearly indicated by the increase of modifier generator macro > > > > > > > > on the DRM side. > > > > > > > > > > > > > > Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one > > > > > > > is decided, I'm stuck and this is the only intermediate solution I found. > > > > > > > > > > > > We can safely assume that drm fourcc and v4l2 fourcc won't be merged. > > > > > > > > > > > > There is too much divergence and not enough interest in creating common > > > > > > fourccs. > > > > > > > > > > > > But we *do* want to share the modifiers. > > > > > > > > > > > > > We have a working solution with Boris's patchset with ext_fmt passing the > > > > > > > modifier to user-space. > > > > > > > > > > > > > > but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT > > > > > > > will still be needed if we pass the modifier with an extended format struct. > > > > > > > > > > > > We tried merging fourccs but that ran into resistance. Frankly, I wouldn't > > > > > > bother with this, it is much easier to just create a conversion table in the > > > > > > kernel docs. > > > > > > > > > > > > So don't block on this, I would really prefer if the ext_fmt series is picked > > > > > > up again and rebased and reposted and then worked on. The stateless codec support > > > > > > is taking less time (it's shaping up well) so there is more time to work on this. > > > > > > > > > > Ok, I already starting discussing with Helen Koike about the ext_fnt re-spin. > > > > > > > > > > Should I re-introduce different v4l2 pixfmt for these DRM YUV420_*BIT or I can keep this > > > > > patch along the new ext_fmt and shared modifiers ? > > > > > > > > So to be clear the DRM_FORMAT_YUV420_8BIT/10BIT fourccs define that this is a > > > > buffer containing compressed YUV420 in 8 or 10 bit and the modifier tells userspace > > > > which compression is used, right? > > > > > > > > And we would add V4L2_PIX_FMT_YUV420_8BIT/_10BIT that, I assume, use the same > > > > fourcc values as the DRM variants? > > > > > > > > Since these fourccs are basically useless without V4L2 modifier support it would > > > > only make sense in combination with the ext_fmt series. > > > > > > I personally still think that adding these fourcc will just create a > > > source of confusion and that fourcc should not be tried to be matched > > > at the cost of tripling the already duplicated pixel formats. Userspace > > > already need to implement translation anyway. > > > > By using the same fourcc + modifiers, the translation table would only be needed > > for v4l2-specific fourcc, by reusing the same it's not necessary anymore. > > We have a really simple ffmpeg implementation using ext_fmt, and it makes it > > generic. But it's not overall generic because "generic" userspace still need to handle legacy, like the Samsung NV12 tiled format. There is no way to avoid translation while supporting multiple HW (even with modifiers). And adding a subset of generic fourcc that duplicates some existing pixel format seems like the wrong way. Quickly this will grow fourcc collision between DRM and V4L2. > > > > > On DRM side, new fourcc was not create for NV12+modifier, I don't see > > > why planar YUV420 has to be different, with or without ext_fmt. > > > > These V4L2_PIX_FMT_YUV420_8BIT/_10BIT were added because of the compressed nature > > of buffers. It's not because of the modifiers, modifiers can be used we any fourcc > > to define vendor specific layout requirements or changes, but for compressed the > > underlying YUV buffer cannot be physically described by any YUV420 fourcc, so > > ARM introduced these fourcc to describe a virtual YUV420 8 or 10bit buffer which > > physical layout is defined by the modifier. > > They could have re-used DRM_FORMAT_YUV420, but it's a 2 plane fourcc, and the other > > describe a true single or multiple plane layout which are simply not true with > > ARM AFBC or Amlogic FBC. As far as I'm concern, this argument does not hold, as NV12 is still used with compressed modifiers on other plaforms (even if NV12 is not by "nature" compressed) and only 1 out of the two NV12 formats we got in V4L2 matches the DRM fourcc. So it might be "generic" to your specific SoC, but that's not real. In any case, I still believe the naming is completely misleading if this is to indicate a buffer format which is compressed, and not supposed to be used for linear YUV420p (which already have two fourcc in V4L2 for). > > > > Neil > > > > > Nicolas > > > > > > > Regards, > > > > > > > > Hans > > > > > > > > > Neil > > > > > > > > > > > I believe we really need this since v4l2_buffer and v4l2_format are a real mess. > > > > > > > > > > > > Regards, > > > > > > > > > > > > Hans > > > > > > > > > > > > > > > default: > > > > > > > > > if (fmt->description[0]) > > > > > > > > > return; > > > > > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > > > > > > > > index c3a1cf1c507f..90b9949acb8a 100644 > > > > > > > > > --- a/include/uapi/linux/videodev2.h > > > > > > > > > +++ b/include/uapi/linux/videodev2.h > > > > > > > > > @@ -705,6 +705,15 @@ struct v4l2_pix_format { > > > > > > > > > #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ > > > > > > > > > #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ > > > > > > > > > > > > > > > > > > +/* > > > > > > > > > + * Compressed Luminance+Chrominance meta-formats > > > > > > > > > + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ > > > > > > > > > +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ > > > > > > > > > + > > > > > > > > > /* Vendor-specific formats */ > > > > > > > > > #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ > > > > > > > > > #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ > > > > > > > > > > > > > > [1] https://patchwork.freedesktop.org/series/73722/#rev7 > > > > > > >
On 6/8/20 6:26 AM, Hans Verkuil wrote: > On 08/06/2020 10:16, Neil Armstrong wrote: >> Hi Nicolas, >> >> On 05/06/2020 17:35, Nicolas Dufresne wrote: >>> Le jeudi 04 juin 2020 à 15:53 +0200, Neil Armstrong a écrit : >>>> From: Maxime Jourdan <mjourdan@baylibre.com> >>>> >>>> Add two generic Compressed Framebuffer pixel formats to be used >>>> with a modifier when imported back in another subsystem like DRM/KMS. >>>> >>>> These pixel formats represents generic 8bits and 10bits compressed buffers >>>> with a vendor specific layout. >>>> >>>> These are aligned with the DRM_FORMAT_YUV420_8BIT and DRM_FORMAT_YUV420_10BIT >>>> used to describe the underlying compressed buffers used for ARM Framebuffer >>>> Compression. In the Amlogic case, the compression is different but the >>>> underlying buffer components is the same. >>>> >>>> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ >>>> include/uapi/linux/videodev2.h | 9 +++++++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>> index 2322f08a98be..8f14adfd5bc5 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>> @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>>> case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; >>>> case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; >>>> case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; >>>> + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; >>>> + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; >> >> Seems we are totally on-par with the following :-) >> >>> >>> When I read the DRM documentation [0], I'm reading that YUV420_8BIT >>> definition matches V4L2_PIX_FMT_YVU420 and V4L2_PIX_FMT_YVU420M fully. >>> In fact, on DRM side, to represent that format you want to expose here, >>> they will strictly combine this generic format (documented un- >>> compressed) with a modifier generated with the macro >>> DRM_FORMAT_MOD_ARM_AFBC(*). And only the combination represent a unique >>> and share-able format. >> >> Yes, and this is exactly my goal here, and matches the Amlogic Framebuffer as >> described in patch 4. The modifier patchset is at [1]. >> >>> >>> In absence of modifier in V4L2 API, this compressed format should be >>> named accordingly to the compressed algorithm used (something like >>> FMT_YUV420_8BIT_AML_FBC). >> >> It's even more complex, the modifier depends on the SoC revision, so we can >> have up to6 different unique pixel format instead of 2 with a variable >> modifier. >> >>> So I believe these format name cannot be >>> copied as-is like this, as they can only introduce more ambiguity in >>> the already quite hard to follow naming of pixel formats. In fact, it >>> is very common to see multiple different vendor compressions on the >>> same SoC, so I don't really believe a "generic" compressed format make >>> sense. To site one, the IMX8M, which got Verrisillicon/Vivante DEC400 >>> on the GPU, and the Hantro G2 compression format. Both will apply to >>> NV12 class of format so in DRM they would be NV12 + modifier, and the >>> combination forms the unique format. Now, in term of sharing, they must >>> be differiented by userspace, as support for compression/decompression >>> is heterogeneous (in that case the GPU does not support Hantro G2 >>> decompression or compression, and the VPU does not support DEC400). >>> >>> I'll remind that the modifier implementation has great value and is >>> much more scalable then the current V4L2 approach. There has been some >>> early proposal for this, maybe it's time to prioritize because this >>> list will starts growing with hundred or even thousands or format, >>> which is clearly indicated by the increase of modifier generator macro >>> on the DRM side. >> >> Yes, but until the migration of drm_fourcc and v4l2 fourcc into a common one >> is decided, I'm stuck and this is the only intermediate solution I found. > > We can safely assume that drm fourcc and v4l2 fourcc won't be merged. > > There is too much divergence and not enough interest in creating common > fourccs. > > But we *do* want to share the modifiers. > >> >> We have a working solution with Boris's patchset with ext_fmt passing the >> modifier to user-space. >> >> but anyway, since the goal is to merge the fourcc between DRM & V4L2, these YUV420_*BIT >> will still be needed if we pass the modifier with an extended format struct. > > We tried merging fourccs but that ran into resistance. Frankly, I wouldn't > bother with this, it is much easier to just create a conversion table in the > kernel docs. > > So don't block on this, I would really prefer if the ext_fmt series is picked > up again and rebased and reposted and then worked on. jfyi, I picked it up, and should post a new version of the RFC soonish. Regards, Helen > The stateless codec support > is taking less time (it's shaping up well) so there is more time to work on this. > > I believe we really need this since v4l2_buffer and v4l2_format are a real mess. > > Regards, > > Hans > >> >>> >>>> default: >>>> if (fmt->description[0]) >>>> return; >>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>>> index c3a1cf1c507f..90b9949acb8a 100644 >>>> --- a/include/uapi/linux/videodev2.h >>>> +++ b/include/uapi/linux/videodev2.h >>>> @@ -705,6 +705,15 @@ struct v4l2_pix_format { >>>> #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ >>>> #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ >>>> >>>> +/* >>>> + * Compressed Luminance+Chrominance meta-formats >>>> + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ >>>> +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ >>>> + >>>> /* Vendor-specific formats */ >>>> #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ >>>> #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */ >>> >> >> [1] https://patchwork.freedesktop.org/series/73722/#rev7 >> >
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 2322f08a98be..8f14adfd5bc5 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -1447,6 +1447,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) case V4L2_PIX_FMT_S5C_UYVY_JPG: descr = "S5C73MX interleaved UYVY/JPEG"; break; case V4L2_PIX_FMT_MT21C: descr = "Mediatek Compressed Format"; break; case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; + case V4L2_PIX_FMT_YUV420_8BIT: descr = "Compressed YUV 4:2:0 8-bit Format"; break; + case V4L2_PIX_FMT_YUV420_10BIT: descr = "Compressed YUV 4:2:0 10-bit Format"; break; default: if (fmt->description[0]) return; diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index c3a1cf1c507f..90b9949acb8a 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -705,6 +705,15 @@ struct v4l2_pix_format { #define V4L2_PIX_FMT_FWHT v4l2_fourcc('F', 'W', 'H', 'T') /* Fast Walsh Hadamard Transform (vicodec) */ #define V4L2_PIX_FMT_FWHT_STATELESS v4l2_fourcc('S', 'F', 'W', 'H') /* Stateless FWHT (vicodec) */ +/* + * Compressed Luminance+Chrominance meta-formats + * 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 V4L2_PIX_FMT_YUV420_8BIT v4l2_fourcc('Y', 'U', '0', '8') /* 1-plane YUV 4:2:0 8-bit */ +#define V4L2_PIX_FMT_YUV420_10BIT v4l2_fourcc('Y', 'U', '1', '0') /* 1-plane YUV 4:2:0 10-bit */ + /* Vendor-specific formats */ #define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */ #define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */