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?
>
> 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?
>
> Regards,
>
> Hans
>
>
>
>
>
>>
>>>>> +
>>>>> + return -EINVAL;
>>>>> }
>>>>>
>>
>>
>> <snip>
>
--
Best regards,
Bingbu Cao
