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

[Tomi Valkeinen]

Hi,

On 08/11/2024 11:30, Jacopo Mondi wrote:
> 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!

Right. The internal pad can (or "should", in most cases) use an mbus 
format that describes the format's content and form, whereas the 
external pad only describes the form.

The point here is that while it's fine that 100 sensor drivers each have 
their own specific embedded mbus format, it's not fine that each and 
every bridge driver should also support all those 100 formats, even if 
the bridges just pass the data forward.

  Tomi