Hans,
On 10/10/23 4:10 PM, Hans de Goede wrote:
> 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.
Ack, thanks.
>
> Regards,
>
> Hans
>
>
>
--
Best regards,
Bingbu Cao
