[PATCH/RFC 22/32] libcamera: Add CameraSensor implementation for raw V4L2 sensors

[Jacopo Mondi]

Hi Naush, Laurent, Tomi

On Wed, Mar 13, 2024 at 09:01:46PM +0200, Laurent Pinchart wrote:
> On Wed, Mar 13, 2024 at 06:16:15PM +0200, Tomi Valkeinen wrote:
> > On 13/03/2024 14:46, Tomi Valkeinen wrote:
> > > On 13/03/2024 14:20, Naushir Patuck wrote:
> > >
> > >> ?> +               case MediaBusFormatInfo::Type::Metadata:
> > >>> +                       /*
> > >>> +                        * Skip metadata streams that are not sensor
> > >>> embedded
> > >>> +                        * data. The source stream reports a generic
> > >>> metadata
> > >>> +                        * format, check the sink stream for the
> > >>> exact format.
> > >>> +                        */
> > >>> +                       formats = subdev_->formats(route.sink);
> > >>> +                       if (formats.size() != 1)
> > >>> +                               continue;
> > >>
> > >> Should this test be if (!formats.size()) insead?  It might be possible
> > >> to have multiple metadata types.
> > >
> > > The driver in my branch is old and hacky. I should see what Laurent has
> > > done with the imx219 in his branch, and possibly just take that one.
> > >
> > > I think advertising only a single format makes sense here, as the
> > > embedded format is defined by the video format.
> > >
> > >>> +
> > >>> +                       if
> > >>> (MediaBusFormatInfo::info(formats.cbegin()->first).type !=
> > >>> +                           MediaBusFormatInfo::Type::EmbeddedData)
> > >>> +                               continue;
> > >>
> > >> The IMX219 driver (from Tomi's kernel tree) advertises
> > >> MEDIA_BUS_FMT_META_8 / MEDIA_BUS_FMT_META_10 formats for the embedded
> > >> data stream, which translates to a type of
> > >> MediaBusFormatInfo::Type::Metadata.  Does the driver need updating, or
> > >> should this check include MediaBusFormatInfo::Type::Metadata?
> > >
> > > Laurent's version should also report those same mbus formats. Hmm, oh,
> > > but it uses MEDIA_BUS_FMT_CCS_EMBEDDED for the internal pad...
> >
> > This looks a bit odd. The driver gives MEDIA_BUS_FMT_CCS_EMBEDDED when
> > enumerating the mbus codes, but then always sets the format to META_8 or
> > META_10.
>
> Sounds like a bug.
>

With this fixed on the imx219 driver side, I confirm this patch works
as expected.

In my understanding the expected driver behaviour is to:

- MEDIA_BUS_FMT_CCS_EMBEDDED on the edata internal sink pad
- MEDIA_BUS_FMT_META_8 / MEDIA_BUS_FMT_META_10 on the edata stream in the
  source pad

I'll re-send this patch broken out from this larger series.

> > I'm guessing the driver is supposed to keep the internal pad's
> > format as MEDIA_BUS_FMT_CCS_EMBEDDED, and only the external pad would
> > use META_8/10...

That's my understanding too!

>
> --
> Regards,
>
> Laurent Pinchart