Re: [PATCH 11/21] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt
From: Eugen.Hristev
Date: Thu Nov 11 2021 - 04:49:29 EST
On 11/8/21 4:08 PM, Eugen Hristev - M18282 wrote:
> On 11/8/21 1:20 PM, Jacopo Mondi wrote:
>> Hi Eugen
>>
>> On Fri, Nov 05, 2021 at 11:03:25AM +0000, Eugen.Hristev@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
>>>>>
>>>>> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>>>>
>>>>
>>>> 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.
Hi Jacopo,
I tried to look over the link_validate call.
It looks like this is only called by media_pipeline_start, which most
drivers use in start_streaming() .
However, I need the format validation to be done before that, at
streamon() time, because after streamon() , the vb2 queue will be filled
with buffers, and I need to know exactly which format I will be using.
So, do you think it's fine to keep the format validation at streamon()
time instead of calling media_pipeline_start in start_streaming ?
Currently I am not calling media_pipeline_start at all.
Do you think it's required?
Or maybe I am missing something and it should be done in a different way ?
Thanks !
>>
>> 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
>>>>>>
>>>
>