Re: [PATCH 11/21] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt

[<Eugen.Hristev@microchip.com>]

On 11/8/21 1:20 PM, Jacopo Mondi wrote:
> Hi Eugen
> 
> On Fri, Nov 05, 2021 at 11:03:25AM +0000, Eugen.Hristev@microchip.com wrote:
>> 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@microchip.com>
>>>>
>>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.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.
> 
> Ok, I think here it's were I disconnect.
> 
> I don't think you should care about the -current- format on the
> subdev, as once you move to MC the ISC formats should be enumerated
> regardless of the how the remote subdev is configured. Rather, you
> should consider if IS_RAW(f->mbus_code) and from there enumerate the
> output formats you can generate from raw bayer (in addition to the
> ones that can be produced by dumping the sensor produced formats)

Actually , in some words, this is what I am doing.
I am following Laurent's advice:
enumerate the formats supported for a given mbus code.

In consequence, if the mbus code given is a raw mbus code , I can 
advertise all my ISC formats, and if the mbus code is not raw, I don't .

So I am doing what you are saying, namely three cases:

If there is no mbus code given as a parameter to this function, I 
advertise all my formats, raw, non-raw, etc.

If there is raw mbus code given, I advertise this specific raw mbus code 
if the sensor supports it, and the ISC supports it, and in addition, all 
the formats ISC can convert from raw to.

If the mbus code given is not raw, then, I can only advertise this 
specific non-raw format, since there is nothing more I can do with it.
It wouldn't make much sense to still advertise my rgb,yuv formats, since 
they cannot be used at all, and my later try_validate_formats will bail out


> 
>> 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
>>
> 
> I think our disconnection comes from the way the supported formats are
> defined in ISC and I think their definition could be reworkd to
> simplify the format selection logic.
> 
> The driver defines three lists of formats:
> 
> - isc->controller_formats
> 
>         static const struct isc_format sama7g5_controller_formats[] = {
>          {
>                  .fourcc         = V4L2_PIX_FMT_ARGB444,
>          },
>          {
>                  .fourcc         = V4L2_PIX_FMT_ARGB555,
>          },
>          ...
> 
>          };
> 
>   These are listed with the output fourcc only, and if I get
>   try_configure_pipeline() right, they can be generated from any Bayer
>   RAW format. Is this right ?
> 
> - isc->formats_list
> 
>          static struct isc_format sama7g5_formats_list[] = {
>                  {
>                          .fourcc         = V4L2_PIX_FMT_SBGGR8,
>                          .mbus_code      = MEDIA_BUS_FMT_SBGGR8_1X8,
>                          .pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,
>                          .cfa_baycfg     = ISC_BAY_CFG_BGBG,
>                  },
>                  {
>                          .fourcc         = V4L2_PIX_FMT_SGBRG8,
>                          .mbus_code      = MEDIA_BUS_FMT_SGBRG8_1X8,
>                          .pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,
>                          .cfa_baycfg     = ISC_BAY_CFG_GBGB,
>                  },
> 
>           ...
> 
>           };
> 
>    These are formats the ISC can output by dumping the sensor output,
>    hence they require a specific format to be output from the sensor
> 
> - isc->user_formats
> 
>          static int isc_formats_init() {
> 
>                  ...
> 
>                  fmt = &isc->formats_list[0];
>                  for (i = 0, j = 0; i < list_size; i++) {
>                          if (fmt->sd_support)
>                                  isc->user_formats[j++] = fmt;
>                          fmt++;
>                  }
> 
>        }
> 
>    this list is obtained at runtime by restricting the formats_list to
>    what the sensor can actually output. I think these, if you move to
>    MC should be removed but I'm not 100% sure it is possible with the
>    current implementation of set_fmt().
> 
> Do you think controller_formats and formats_list should be unified ?
> 
> If my understanding that all controller_formats can be generated from
> RAW Bayer formats you could model struct isc_format as
> 
>          struct isc_format {
>                  u32     fourcc;
>                  bool    processed;
>                  u32     mbus_codes
>                  u32     cfa_baycfg;
>                  u32     pfe_cfg0_bps;
>          };
> 
> and have in example:
> 
>          {
>                  .fourcc         = V4L2_PIX_FMT_ARGB444,
>                  .processed      = true,
>                  .mbus_codes     = nullptr,
>                  ....
>          },
>          {
>                  .fourcc         = V4L2_PIX_FMT_SBGGR8,
>                  .processed      = false,
>                  .mbus_codes     = { MEDIA_BUS_FMT_SBGGR8_1X8 }
>                  .pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,
>                  .cfa_baycfg     = ISC_BAY_CFG_BGBG,
>          },
> 
> and when enumerating and configuring formats check that
> 
>          if (isc_format[i].processed &&
>              (f->mbus_code && IS_RAW(f->mbus)code))
> 
> or
> 
>          if (!isc_format[i].processed &&
>              (f->mbus_code == isc_format[i].mbus_code
> 
> if you have other restrictions as in example V4L2_PIX_FMT_YUV420
> requiring a packed YUV format, you can implement more complex
> validations to match processed formats, like you do in try_validate_formats()
> 
> Also, and a bit unrelated to enum_fmt, if I got this right
> at format configuration time you match the ISC format with
> the sensor format. I think this should be moved to .link_validate() op time.
> 
> The MC core calls .link_validate when streaming is started on each
> entity part of a pipeline, and there you could make sure that the ISC output format can be produced using
> the sensor format (and sizes). This will greatly simplify set_fmt() as
> there you will have just to make sure the supplied format is in the
> list of the ISC supported ones and that's it. If userspace fails to
> configure the pipeline correctly (in example by setting a non RAW
> Bayer sensor on the format and requesting a RAW Bayer format from ISC,
> you will fail at s_stream() time by returning an EPIPE.
> 
> Hope all of this makes a bit of sense :)

About this last part you are telling me: I have to tell you what am I 
doing right now: with this patch series, including this patch, I am 
adding support for mc handling in this driver, but! the driver is still 
completely compatible with 'the old way' like setting fmt-video for 
/dev/video0 and everything is propagated down the pipeline.

I am doing the conversion to mc-only type of driver in multiple steps.
This series adds the 'new way' while having the 'old way' still there.
At the moment I am working on another patch on top of this series that 
will basically remove most of my format propagation logic from 
/dev/video0 to the subdevice, and after this patch that is on the way, 
the 'old way' is gone. The sensor will *have to* be configured through 
userspace because ISC will never call set_fmt on it at all.

So the purpose of the patch you are reviewing now is to have the mbus 
code parameter in the enum_fmt_vid_cap in the way the current driver 
handles things.

So if you agree, I will let the other patch speak for itself and rework 
the logic completely .
I am currently trying to do it at streamon() time like Laurent told me, 
but I can try to have it at validate link as well, to see how it works.

I will add that patch to the series when it's ready and I have a v2 of 
this series as well. So in baby steps, I am working towards the goal. I 
am not sure if this means that you could agree to this patch as-is, or I 
have to integrate it completely into a bigger patch that will also fix 
everything up including the format logic.

Thanks again for your review and ideas

Eugen
> 
>>>>> +   /*
>>>>> +    * 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
>>>>>
>>