Re: [RFC/PATCH v1 1/3] v4l: Add new framesamples field to struct v4l2_mbus_framefmt

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 11/22/2011 11:48 AM, Hans Verkuil wrote:
> On Tuesday, November 22, 2011 10:55:38 Sylwester Nawrocki wrote:
>> The purpose of the new field is to allow the video pipeline elements to
>> negotiate memory buffer size for compressed data frames, where the buffer
>> size cannot be derived from pixel width and height and the pixel code.
>>
>> For VIDIOC_SUBDEV_S_FMT and VIDIOC_SUBDEV_G_FMT ioctls, the framesamples
>> parameter should be calculated by the driver from pixel width, height,
>> color format and other parameters if required and returned to the caller.
>> This applies to compressed data formats only.
>>
>> The application should propagate the framesamples value, whatever returned
>> at the first sub-device within a data pipeline, i.e. at the pipeline's data
>> source.
>>
>> For compressed data formats the host drivers should internally validate
>> the framesamples parameter values before streaming is enabled, to make sure
>> the memory buffer size requirements are satisfied along the pipeline.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  Documentation/DocBook/media/v4l/subdev-formats.xml |    7 ++++++-
>>  include/linux/v4l2-mediabus.h                      |    4 +++-
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/subdev-formats.xml b/Documentation/DocBook/media/v4l/subdev-formats.xml
>> index 49c532e..d0827b4 100644
>> --- a/Documentation/DocBook/media/v4l/subdev-formats.xml
>> +++ b/Documentation/DocBook/media/v4l/subdev-formats.xml
>> @@ -35,7 +35,12 @@
>>  	</row>
>>  	<row>
>>  	  <entry>__u32</entry>
>> -	  <entry><structfield>reserved</structfield>[7]</entry>
>> +	  <entry><structfield>framesamples</structfield></entry>
>> +	  <entry>Number of data samples on media bus per frame.</entry>
> 
> Is this the *maximum* number of data samples, or is this the *required* number
> of data samples?
> 
> I think you mean 'maximum', but the documentation does not actually state that.

I forgot to mention I intended to update the DocBook part after there is a
common agreement on the new field. 

And yes, it's meant to be the maximum number of data samples.

> 
> It should also clearly state that this field is used only for compressed
> formats (right?). Should drivers be required to set this to 0 for uncompressed
> formats?

Yes, it's only for compressed formats. I'll update the DocBook for the next
iteration so it contains the information from current changelogs.

I think it's reasonable to require the drivers to clear the field for raw formats.
I'm going to add this requirement in the next version of this patch.

> 
> Regards,
> 
> 	Hans
> 
>> +	</row>
>> +	<row>
>> +	  <entry>__u32</entry>
>> +	  <entry><structfield>reserved</structfield>[6]</entry>
>>  	  <entry>Reserved for future extensions. Applications and drivers must
>>  	  set the array to zero.</entry>
>>  	</row>
>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>> index 5ea7f75..ce776e8 100644
>> --- a/include/linux/v4l2-mediabus.h
>> +++ b/include/linux/v4l2-mediabus.h
>> @@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
>>   * @code:	data format code (from enum v4l2_mbus_pixelcode)
>>   * @field:	used interlacing type (from enum v4l2_field)
>>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
>> + * @framesamples: number of data samples per frame
>>   */
>>  struct v4l2_mbus_framefmt {
>>  	__u32			width;
>> @@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
>>  	__u32			code;
>>  	__u32			field;
>>  	__u32			colorspace;
>> -	__u32			reserved[7];
>> +	__u32			framesamples;
>> +	__u32			reserved[6];
>>  };
>>  
>>  #endif
>>

--
Regards,
Sylwester