Message-ID: <60f0d378-d998-464f-2d95-09db4e889b96@microchip.com>
Date:   Fri, 5 Nov 2021 11:03:25 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <jacopo@...ndi.org>
CC:     <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <laurent.pinchart@...asonboard.com>, <sakari.ailus@....fi>,
        <robh+dt@...nel.org>, <Nicolas.Ferre@...rochip.com>
Subject: Re: [PATCH 11/21] media: atmel: atmel-isc-base: implement mbus_code
 support in enumfmt

On 11/5/21 11:51 AM, Jacopo Mondi wrote:
> Hi Eugen
> 
> On Fri, Nov 05, 2021 at 10:25:59AM +0100, Jacopo Mondi wrote:
>> Hi Eugen,
>>
>> On Fri, Oct 22, 2021 at 10:52:37AM +0300, Eugen Hristev wrote:
>>> If enumfmt is called with an mbus_code, the enumfmt handler should only
>>> return the formats that are supported for this mbus_code.
>>> To make it more easy to understand the formats, changed the report order
>>> to report first the native formats, and after that the formats that the ISC
>>> can convert to.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
>>
>> Reviewed-by: Jacopo Mondi <jacopo@...ndi.org>
>>
> 
> Too soon! Sorry...
> 
>> Thanks
>>     j
>>
>>> ---
>>>   drivers/media/platform/atmel/atmel-isc-base.c | 51 ++++++++++++++++---
>>>   1 file changed, 43 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>> index 2dd2511c7be1..1f7fbe5e4d79 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>> @@ -499,21 +499,56 @@ static int isc_enum_fmt_vid_cap(struct file *file, void *priv,
>>>      u32 index = f->index;
>>>      u32 i, supported_index;
>>>
>>> -   if (index < isc->controller_formats_size) {
>>> -           f->pixelformat = isc->controller_formats[index].fourcc;
>>> -           return 0;
>>> +   supported_index = 0;
>>> +

Hi Jacopo,

This for loop below first iterates through the formats that were 
identified as directly supported by the subdevice.
As we are able in the ISC to just dump what the subdevice can stream, we 
advertise all these formats here.
(if the userspace requires one specific mbus code, we only advertise the 
corresponding format)

>>> +   for (i = 0; i < isc->formats_list_size; i++) {
>>> +           if (!isc->formats_list[i].sd_support)
> 
> 
>>> +                   continue;
>>> +           /*
>>> +            * If specific mbus_code is requested, provide only
>>> +            * supported formats with this mbus code
>>> +            */
>>> +           if (f->mbus_code && f->mbus_code !=
>>> +               isc->formats_list[i].mbus_code)
>>> +                   continue;
>>> +           if (supported_index == index) {
>>> +                   f->pixelformat = isc->formats_list[i].fourcc;
>>> +                   return 0;
>>> +           }
>>> +           supported_index++;
>>>      }
>>>
>>> -   index -= isc->controller_formats_size;
>>> +   /*
>>> +    * If the sensor does not support this mbus_code whatsoever,
>>> +    * there is no reason to advertise any of our output formats
>>> +    */
>>> +   if (supported_index == 0)
>>> +           return -EINVAL;
> 
> Shouldn't you also return -EINVAL if index > supported_index ?

No. If we could not find any format that the sensor can use 
(supported_index == 0), and that our ISC can also use, then we don't 
support anything, thus, return -EINVAL regardless of the index.

> 
> In that case, I'm not gettng what the rest of the function is for ?
> 

This is the case when we identified one supported format for both the 
sensor and the ISC (case where supported_index found earlier is >= 1)

>>> +
>>> +   /*
>>> +    * If the sensor uses a format that is not raw, then we cannot
>>> +    * convert it to any of the formats that we usually can with a
>>> +    * RAW sensor. Thus, do not advertise them.
>>> +    */
>>> +   if (!isc->config.sd_format ||
>>> +       !ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
>>> +           return -EINVAL;
>>>

Next, if the current format from the subdev is not raw, we are done.
But, if it's raw, we can use it to convert to a plethora of other 
formats. So we have to enumerate them below :

With the previous checks, I am making sure that the ISC can really 
convert to these formats, otherwise they are badly reported

Hope this makes things more clear, but please ask if something looks strange

>>> +   /*
>>> +    * Iterate again through the formats that we can convert to.
>>> +    * However, to avoid duplicates, skip the formats that
>>> +    * the sensor already supports directly
>>> +    */
>>> +   index -= supported_index;
>>>      supported_index = 0;
>>>
>>> -   for (i = 0; i < isc->formats_list_size; i++) {
>>> -           if (!ISC_IS_FORMAT_RAW(isc->formats_list[i].mbus_code) ||
>>> -               !isc->formats_list[i].sd_support)
>>> +   for (i = 0; i < isc->controller_formats_size; i++) {
>>> +           /* if this format is already supported by sensor, skip it */
>>> +           if (find_format_by_fourcc(isc, isc->controller_formats[i].fourcc))
>>>                      continue;
>>>              if (supported_index == index) {
>>> -                   f->pixelformat = isc->formats_list[i].fourcc;
>>> +                   f->pixelformat =
>>> +                           isc->controller_formats[i].fourcc;
>>>                      return 0;
>>>              }
>>>              supported_index++;
>>> --
>>> 2.25.1
>>>

Powered by blists - more mailing lists