[v14,03/34] media: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8

  V4L2_FRAME_DESC_ENTRY_MAX is currently set to 4. In theory it's possible
to have an arbitrary amount of streams in a single pad, so preferably
there should be no hardcoded maximum number.

However, I believe a reasonable max is 8, which would cover a CSI-2 pad
with 4 streams of pixel data and 4 streams of metadata.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
  

On 8/31/22 10:13 PM, Tomi Valkeinen wrote:
> V4L2_FRAME_DESC_ENTRY_MAX is currently set to 4. In theory it's possible
> to have an arbitrary amount of streams in a single pad, so preferably
> there should be no hardcoded maximum number.
> 
> However, I believe a reasonable max is 8, which would cover a CSI-2 pad
> with 4 streams of pixel data and 4 streams of metadata.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 9689f38a0af1..3797b99bb408 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -358,7 +358,11 @@ struct v4l2_mbus_frame_desc_entry {
>  	} bus;
>  };
>  
> -#define V4L2_FRAME_DESC_ENTRY_MAX	4
> + /*
> +  * If this number is too small, it should be dropped altogether and the
> +  * API switched to a dynamic number of frame descriptor entries.
> +  */
> +#define V4L2_FRAME_DESC_ENTRY_MAX	8

The number 8 here is still not enough I think, CSI2 specification already
extended the VC identifier to be at most 5 bits, which support a max of
32 VCs.

Considering the metadata, the number should be larger, it looks like that
we have to switch using dynamic number?

BTW, does this change break the uAPI?

>  
>  /**
>   * enum v4l2_mbus_frame_desc_type - media bus frame description type
>
  

Hi,

On 29/09/2022 09:48, Bingbu Cao wrote:
> 
> 
> On 8/31/22 10:13 PM, Tomi Valkeinen wrote:
>> V4L2_FRAME_DESC_ENTRY_MAX is currently set to 4. In theory it's possible
>> to have an arbitrary amount of streams in a single pad, so preferably
>> there should be no hardcoded maximum number.
>>
>> However, I believe a reasonable max is 8, which would cover a CSI-2 pad
>> with 4 streams of pixel data and 4 streams of metadata.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>   include/media/v4l2-subdev.h | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 9689f38a0af1..3797b99bb408 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -358,7 +358,11 @@ struct v4l2_mbus_frame_desc_entry {
>>   	} bus;
>>   };
>>   
>> -#define V4L2_FRAME_DESC_ENTRY_MAX	4
>> + /*
>> +  * If this number is too small, it should be dropped altogether and the
>> +  * API switched to a dynamic number of frame descriptor entries.
>> +  */
>> +#define V4L2_FRAME_DESC_ENTRY_MAX	8
> 
> The number 8 here is still not enough I think, CSI2 specification already
> extended the VC identifier to be at most 5 bits, which support a max of
> 32 VCs.

Well, 8 frame desc entries is not enough for the "old" CSI2 either as 
there can be a lot of data-types, each a separate stream ("stream" as 
defined by this series) which needs a frame desc.

> Considering the metadata, the number should be larger, it looks like that
> we have to switch using dynamic number?

Do we have a current use case which needs more than 8 streams per pad? 
If not, I'd stay away from this for the time being. This can be changed 
later.

> BTW, does this change break the uAPI?

No, the frame desc is fully internal to the kernel.

  Tomi
  

Tomi, 

On 10/3/22 7:32 PM, Tomi Valkeinen wrote:
> Hi,
> 
> On 29/09/2022 09:48, Bingbu Cao wrote:
>>
>>
>> On 8/31/22 10:13 PM, Tomi Valkeinen wrote:
>>> V4L2_FRAME_DESC_ENTRY_MAX is currently set to 4. In theory it's possible
>>> to have an arbitrary amount of streams in a single pad, so preferably
>>> there should be no hardcoded maximum number.
>>>
>>> However, I believe a reasonable max is 8, which would cover a CSI-2 pad
>>> with 4 streams of pixel data and 4 streams of metadata.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>   include/media/v4l2-subdev.h | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index 9689f38a0af1..3797b99bb408 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -358,7 +358,11 @@ struct v4l2_mbus_frame_desc_entry {
>>>       } bus;
>>>   };
>>>   -#define V4L2_FRAME_DESC_ENTRY_MAX    4
>>> + /*
>>> +  * If this number is too small, it should be dropped altogether and the
>>> +  * API switched to a dynamic number of frame descriptor entries.
>>> +  */
>>> +#define V4L2_FRAME_DESC_ENTRY_MAX    8
>>
>> The number 8 here is still not enough I think, CSI2 specification already
>> extended the VC identifier to be at most 5 bits, which support a max of
>> 32 VCs.
> 
> Well, 8 frame desc entries is not enough for the "old" CSI2 either as there can be a lot of data-types, each a separate stream ("stream" as defined by this series) which needs a frame desc.
> 
>> Considering the metadata, the number should be larger, it looks like that
>> we have to switch using dynamic number?
> 
> Do we have a current use case which needs more than 8 streams per pad? If not, I'd stay away from this for the time being. This can be changed later.

Yes, we have a use case which run at maximum 6 streams + 4 meta-data.

> 
>> BTW, does this change break the uAPI?
> 
> No, the frame desc is fully internal to the kernel.

Got it, thanks.
> 
>  Tomi