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?
My understanding is - user will try to enum again if return -EINVAL.
>>> +
>>> + return -EINVAL;
>>> }
>>>
<snip>
--
Best regards,
Bingbu Cao
