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.
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.
Regards,
Hans
>
>>>> +
>>>> + return -EINVAL;
>>>> }
>>>>
>
>
> <snip>
