Hi,
On 10/10/23 04:54, Bingbu Cao wrote:
> Hans,
>
> On 10/9/23 8:53 PM, Hans de Goede wrote:
>> Hi Bingbu,
>>
>> On 10/9/23 14:25, Bingbu Cao wrote:
>>>
>>> Hans and Laurent,
>>>
>>> On 10/3/23 1:41 AM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 10/2/23 19:38, Laurent Pinchart wrote:
>>>>> On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 9/4/23 05:13, Cao, Bingbu wrote:
>>>>>>> Hans,
>>>>>>>
>>>>>>> Thanks for your test and report.
>>>>>>>
>>>>>>> I have made some changes locally which will support the latest
>>>>>>> v4l2-async APIs, I will also add the fix for this issue and merge
>>>>>>> them in v3.
>>>>>>
>>>>>> I have been trying to make rawbayer capture with the mainline isys code
>>>>>> work in libcamera and I have hit several short comings in the ipu6-isys
>>>>>> code. I have attached 3 patches to fix various issues please integrate
>>>>>> these into the next version of this series.
>>>>>
>>>>> They look good to me.
>>>>
>>>> Actually I just realized I forgot to commit + squash in a bug fix:
>>>>
>>>>>> Talking about the next version of this series, I think it would be
>>>>>> good to post a new version soon ?
>>>>>>
>>>>>
>>>>>> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
>>>>>> From: Hans de Goede <hdegoede@xxxxxxxxxx>
>>>>>> Date: Mon, 2 Oct 2023 16:37:11 +0200
>>>>>> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>>>>>> /dev/video enum_fmt
>>>>>>
>>>>>> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>>>>> ---
>>>>>> .../media/pci/intel/ipu6/ipu6-isys-video.c | 29 +++++++++++++++----
>>>>>> 1 file changed, 23 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>> index dc1605491352..20fd03f6f204 100644
>>>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>>>>>> int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>>>>>> struct v4l2_fmtdesc *f)
>>>>>> {
>>>>>> - if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>>>> - return -EINVAL;
>>>>>> + unsigned int i, found = 0;
>>>>>>
>>>>>> - f->flags = 0;
>>>>>> - f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>>>> - f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>>>> + if (!f->mbus_code) {
>>>>>> + if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>>>> + return -EINVAL;
>>>>>>
>>>>>> - return 0;
>>>>>> + f->flags = 0;
>>>>>> + f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>>>> + f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>>
>>>> There is a:
>>>> return 0;
>>>>
>>>> missing here.
>>>>
>>>>>> + }
>>>>>> +
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>>> + for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
>>>>>> + if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>>>>> + continue;
>>>>>> +
>>>>>> + if (f->index == found) {
>>>>>> + f->flags = 0;
>>>>>> + f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
>>>>>> + return 0;
>>>>>> + }
>>>>>> +
>>>>>> + found++;
>>>>>> + }
>>>
>>> A little confused here -
>>>
>>> If the `mbus_code` argument here is not zero, does the user expect that
>>> the first try (f->index == 0) should be found and return? `found` is
>>> always 0 here?
>>
>> Notice how formats where:
>>
>> if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>
>> Is true are skipped:
>>
>> if (f->mbus_code != ipu6_isys_pfmts[i].code)
>> continue;
>>
>> The idea is that for (f->index == 0) the first format
>> matching the passed in mbus_code is returned and then
>> for (f->index == 1) the second format matching the passed
>> in mbus_code is returned, etc.
>
> Ack. So for 1:1 match case, is (f->index == 0) expected for all formats?
If there is only 1 format which matches the mbus-code then yes
that fmt will only be returned for (f->index == 0).
But as mentioned already for raw bayer there are typically
2 formats matching the same mbus-code:
>> In practice this means that e.g. for a mbus_code for
>> a 10bit raw bayer format both the padded (one 10 bits
>> pixel in each 16bit integer) and packed formats are
>> returned.
>>
>>
>>> My understanding is - user will try to enum again if return -EINVAL.
>>
>> No, -EINVAL means that the end of the list of
>> formats has been reached, so the user keeps
>> calling this function with higher
>> f->index values until -EINVAL is returned.
>
> So for libcamera, it's trying to enumerate all the pixel formats,
> Is it trying to enumerate from index 0 for each `mbus_code` or continue next
> format enumeration from higher index?
libcamera sets up the media-controller graph, so it already
knowns the mbus_code between the v4l2subdevs. AFAIK after setting
up the graph it uses mbus_code filtering to only get output formats
for /dev/video# which actually match with the configured mbus-code.
Regards,
Hans
