[libcamera-devel] [RFC 1/1] libcamera: V4L2VideoDevice::to[V4L2]PixelFormat(): add Bayer formats

[Laurent Pinchart]

Hi Andrey,

Revisiting a pretty old topic :-)

On Wed, Apr 08, 2020 at 08:58:11PM +0300, Andrey Konovalov wrote:
> On 07.04.2020 15:40, Niklas Söderlund wrote:
> > On 2020-04-05 00:57:30 +0300, Andrey Konovalov wrote:
> >> Add support for 8-bit, 10-bit, and 10-bit packed raw Bayer formats in
> >> toPixelFormat() and toV4L2PixelFormat() methods of V4L2VideoDevice class.
> >>
> >> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >> ---
> >>   src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++++++++++
> >>   1 file changed, 56 insertions(+)
> >>
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index eb33a68..2e3aafc 100644
> >> --- a/src/libcamera/v4l2_videodevice.cpp
> >> +++ b/src/libcamera/v4l2_videodevice.cpp
> >> @@ -1687,6 +1687,36 @@ PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc)
> >>   	case V4L2_PIX_FMT_MJPEG:
> >>   		return PixelFormat(DRM_FORMAT_MJPEG);
> >>   
> >> +	/* 8-bit Bayer formats. */
> >> +	case V4L2_PIX_FMT_SRGGB8:
> >> +		return PixelFormat(DRM_FORMAT_SRGGB8);
> >> +	case V4L2_PIX_FMT_SGRBG8:
> >> +		return PixelFormat(DRM_FORMAT_SGRBG8);
> >> +	case V4L2_PIX_FMT_SGBRG8:
> >> +		return PixelFormat(DRM_FORMAT_SGBRG8);
> >> +	case V4L2_PIX_FMT_SBGGR8:
> >> +		return PixelFormat(DRM_FORMAT_SBGGR8);
> >> +
> >> +	/* 10-bit Bayer formats. */
> >> +	case V4L2_PIX_FMT_SRGGB10:
> >> +		return PixelFormat(DRM_FORMAT_SRGGB10);
> >> +	case V4L2_PIX_FMT_SGRBG10:
> >> +		return PixelFormat(DRM_FORMAT_SGRBG10);
> >> +	case V4L2_PIX_FMT_SGBRG10:
> >> +		return PixelFormat(DRM_FORMAT_SGBRG10);
> >> +	case V4L2_PIX_FMT_SBGGR10:
> >> +		return PixelFormat(DRM_FORMAT_SBGGR10);
> >> +
> >> +	/* 10-bit Bayer packed formats. */
> >> +	case V4L2_PIX_FMT_SRGGB10P:
> >> +		return PixelFormat(DRM_FORMAT_SRGGB10, MIPI_FORMAT_MOD_CSI2_PACKED);
> >> +	case V4L2_PIX_FMT_SGRBG10P:
> >> +		return PixelFormat(DRM_FORMAT_SGRBG10, MIPI_FORMAT_MOD_CSI2_PACKED);
> >> +	case V4L2_PIX_FMT_SGBRG10P:
> >> +		return PixelFormat(DRM_FORMAT_SGBRG10, MIPI_FORMAT_MOD_CSI2_PACKED);
> >> +	case V4L2_PIX_FMT_SBGGR10P:
> >> +		return PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED);
> > 
> > This fits the idea we had when adding the MIPI_FORMAT_MOD_CSI2_PACKED
> > modifier. But it's I misunderstood how modifies are used in DRM when
> > doing so. The patch for DRM is not accepted in upstream Linux yet and
> > this is a local change we took into libcamera where we bet on it was the
> > right approach.
> 
> Ah.. OK. I am not familiar with modifiers usage in DRM. Will take a look.
> Thanks for the explanation.
> 
> > Now we know we need to think one more time about if this is right
> > solution for this problem so I'm  bit reluctant building more things on
> > top of.
> 
> OK. This makes sense.
> 
> > If other thinks it's fine to do so I will not block it however.
> > 
> >> +
> >>   	/* V4L2 formats not yet supported by DRM. */
> >>   	case V4L2_PIX_FMT_GREY:
> >>   	default:
> >> @@ -1734,6 +1764,8 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma
> >>   V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat,
> >>   						   bool multiplanar)
> >>   {
> >> +	bool csi2Packed = pixelFormat.modifier() == MIPI_FORMAT_MOD_CSI2_PACKED;
> >> +
> >>   	switch (pixelFormat) {
> >>   	/* RGB formats. */
> >>   	case DRM_FORMAT_BGR888:
> >> @@ -1777,6 +1809,30 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelForma
> >>   	/* Compressed formats. */
> >>   	case DRM_FORMAT_MJPEG:
> >>   		return V4L2PixelFormat(V4L2_PIX_FMT_MJPEG);
> >> +
> >> +	/* 8-bit Bayer formats. */
> >> +	case DRM_FORMAT_SRGGB8:
> >> +		return V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8);
> >> +	case DRM_FORMAT_SGRBG8:
> >> +		return V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8);
> >> +	case DRM_FORMAT_SGBRG8:
> >> +		return V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8);
> >> +	case DRM_FORMAT_SBGGR8:
> >> +		return V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);
> >> +
> >> +	/* 10-bit Bayer formats, the packed ones included. */
> >> +	case DRM_FORMAT_SRGGB10:
> >> +		return (csi2Packed) ? V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P)
> >> +			: V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10);
> >> +	case DRM_FORMAT_SGRBG10:
> >> +		return (csi2Packed) ? V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P)
> >> +			: V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10);
> >> +	case DRM_FORMAT_SGBRG10:
> >> +		return (csi2Packed) ? V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P)
> >> +			: V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10);
> >> +	case DRM_FORMAT_SBGGR10:
> >> +		return (csi2Packed) ? V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P)
> >> +			: V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10);
> > 
> > If the issue above is resolved in such a way that this patch is accepted
> > this needs to be extended to also support IPU3_FORMAT_MOD_PACKED.
> 
> Right. I'll add the support for IPU3_FORMAT_MOD_PACKED in the next version.

I haven't seen a new version of this patch, but I believe the feature
has been added in the meantime. If that's not the case, could you let me
know ?

-- 
Regards,

Laurent Pinchart