[libcamera-devel,v4,03/10] libcamera: formats: Add R10_CSI2P format

14379 diff mbox series

This new formats corresponds to the V4L2 V4L2_PIX_FMT_Y10P format, and is a
CSI2-packed version of the DRM_FORMAT_R10 format.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/formats.cpp  | 13 +++++++++++++
 src/libcamera/formats.yaml |  4 ++++
 2 files changed, 17 insertions(+)

Quoting Naushir Patuck (2021-10-28 09:46:39)
> This new formats corresponds to the V4L2 V4L2_PIX_FMT_Y10P format, and is a
> CSI2-packed version of the DRM_FORMAT_R10 format.
> 

Relating to your question in the cover letter, I think it's better to
keep this separate.

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/formats.cpp  | 13 +++++++++++++
>  src/libcamera/formats.yaml |  4 ++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index d4781dfb3874..9b6ccdca0663 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -510,6 +510,19 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>                 .pixelsPerGroup = 1,
>                 .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>         } },
> +       { formats::R10_CSI2P, {
> +               .name = "R10_CSI2P",
> +               .format = formats::R10,
> +               .v4l2Formats = {
> +                       .single = V4L2PixelFormat(V4L2_PIX_FMT_Y10P),
> +                       .multi = V4L2PixelFormat(),
> +               },
> +               .bitsPerPixel = 10,
> +               .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +               .packed = true,
> +               .pixelsPerGroup = 1,

Is this correct for a packed format?

Shouldn't this be pixelsPerGroup = 4; bytesPerGroup = 5; ?

https://01.org/linuxgraphics/gfx-docs/drm/media/uapi/v4l/pixfmt-y10p.html
states:

"""
This is a packed grey-scale image format with a depth of 10 bits per
pixel. Every four consecutive pixels are packed into 5 bytes.  Each of
the first 4 bytes contain the 8 high order bits of the pixels, and the
5th byte contains the 2 least significants bits of each pixel, in the
same order.
"""



> +               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},

And therefore I'd expect this to be { 5, 1 }, instead of { 2, 1 } ...

> +       } },
>  
>         /* Bayer formats. */
>         { formats::SBGGR8, {
> diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> index 22a8e473a183..1f3f043302c3 100644
> --- a/src/libcamera/formats.yaml
> +++ b/src/libcamera/formats.yaml
> @@ -109,6 +109,10 @@ formats:
>    - SBGGR16:
>        fourcc: DRM_FORMAT_SBGGR16
>  
> +  - R10_CSI2P:
> +      fourcc: DRM_FORMAT_R10
> +      mod: MIPI_FORMAT_MOD_CSI2_PACKED
> +
>    - SRGGB10_CSI2P:
>        fourcc: DRM_FORMAT_SRGGB10
>        mod: MIPI_FORMAT_MOD_CSI2_PACKED
> -- 
> 2.25.1
>

Hi Kieran,

On Thu, 28 Oct 2021 at 13:15, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Naushir Patuck (2021-10-28 09:46:39)
> > This new formats corresponds to the V4L2 V4L2_PIX_FMT_Y10P format, and
> is a
> > CSI2-packed version of the DRM_FORMAT_R10 format.
> >
>
> Relating to your question in the cover letter, I think it's better to
> keep this separate.
>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/formats.cpp  | 13 +++++++++++++
> >  src/libcamera/formats.yaml |  4 ++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index d4781dfb3874..9b6ccdca0663 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -510,6 +510,19 @@ const std::map<PixelFormat, PixelFormatInfo>
> pixelFormatInfo{
> >                 .pixelsPerGroup = 1,
> >                 .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> >         } },
> > +       { formats::R10_CSI2P, {
> > +               .name = "R10_CSI2P",
> > +               .format = formats::R10,
> > +               .v4l2Formats = {
> > +                       .single = V4L2PixelFormat(V4L2_PIX_FMT_Y10P),
> > +                       .multi = V4L2PixelFormat(),
> > +               },
> > +               .bitsPerPixel = 10,
> > +               .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +               .packed = true,
> > +               .pixelsPerGroup = 1,
>
> Is this correct for a packed format?
>
> Shouldn't this be pixelsPerGroup = 4; bytesPerGroup = 5; ?
>
> https://01.org/linuxgraphics/gfx-docs/drm/media/uapi/v4l/pixfmt-y10p.html
> states:
>
> """
> This is a packed grey-scale image format with a depth of 10 bits per
> pixel. Every four consecutive pixels are packed into 5 bytes.  Each of
> the first 4 bytes contain the 8 high order bits of the pixels, and the
> 5th byte contains the 2 least significants bits of each pixel, in the
> same order.
> """
>
>
>
> > +               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>
> And therefore I'd expect this to be { 5, 1 }, instead of { 2, 1 } ...
>

Ugh, good catch!  That was a copy-paste error on my part.  I meant to update
the planes but forgot :-(

Naush



>
> > +       } },
> >
> >         /* Bayer formats. */
> >         { formats::SBGGR8, {
> > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > index 22a8e473a183..1f3f043302c3 100644
> > --- a/src/libcamera/formats.yaml
> > +++ b/src/libcamera/formats.yaml
> > @@ -109,6 +109,10 @@ formats:
> >    - SBGGR16:
> >        fourcc: DRM_FORMAT_SBGGR16
> >
> > +  - R10_CSI2P:
> > +      fourcc: DRM_FORMAT_R10
> > +      mod: MIPI_FORMAT_MOD_CSI2_PACKED
> > +
> >    - SRGGB10_CSI2P:
> >        fourcc: DRM_FORMAT_SRGGB10
> >        mod: MIPI_FORMAT_MOD_CSI2_PACKED
> > --
> > 2.25.1
> >
>