diff mbox

[RFC,1/4] V4L: Add V4L2_CID_FRAMESIZE image source class control

Message ID 1345715489-30158-2-git-send-email-s.nawrocki@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

The V4L2_CID_FRAMESIZE control determines maximum number
of media bus samples transmitted within a single data frame.
It is useful for determining size of data buffer at the
receiver side.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
 include/linux/videodev2.h                    |  1 +
 3 files changed, 15 insertions(+)

Comments

Sakari Ailus Aug. 23, 2012, 12:13 p.m. UTC | #1
Hi Sylwester,

Thanks for the patch.

On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
> The V4L2_CID_FRAMESIZE control determines maximum number
> of media bus samples transmitted within a single data frame.
> It is useful for determining size of data buffer at the
> receiver side.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
>  include/linux/videodev2.h                    |  1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index 93b9c68..ad5d4e5 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
>  	    conversion.
>  	    </entry>
>  	  </row>
> +	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
> +	    <entry>integer</entry>
> +	  </row>
> +	  <row>
> +	    <entry spanname="descr">Maximum size of a data frame in media bus
> +	      sample units. This control determines maximum number of samples
> +	      transmitted per single compressed data frame. For generic raw
> +	      pixel formats the value of this control is undefined. This is
> +	      a read-only control.
> +	    </entry>
> +	  </row>
>  	  <row><entry></entry></row>
>  	</tbody>
>        </tgroup>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b6a2ee7..0043fd2 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>  	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>  	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";

I would put this to the image processing class, as the control isn't related
to image capture. Jpeg encoding (or image compression in general) after all
is related to image processing rather than capturing it.

>  	/* Image processing controls */
>  	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
> @@ -933,6 +934,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_STROBE_STATUS:
>  	case V4L2_CID_AUTO_FOCUS_STATUS:
>  	case V4L2_CID_FLASH_READY:
> +	case V4L2_CID_FRAMESIZE:
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  		break;
>  	}
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 7a147c8..d3fd19f 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1985,6 +1985,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_VBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
>  #define V4L2_CID_HBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
>  #define V4L2_CID_ANALOGUE_GAIN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
> +#define V4L2_CID_FRAMESIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
>  
>  /* Image processing controls */
>  #define V4L2_CID_IMAGE_PROC_CLASS_BASE		(V4L2_CTRL_CLASS_IMAGE_PROC | 0x900)

Kind regards,
Hi Sakari,

On 08/23/2012 02:13 PM, Sakari Ailus wrote:
> Hi Sylwester,
> 
> Thanks for the patch.

Thanks for your review.

> On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
>> The V4L2_CID_FRAMESIZE control determines maximum number
>> of media bus samples transmitted within a single data frame.
>> It is useful for determining size of data buffer at the
>> receiver side.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
>>  include/linux/videodev2.h                    |  1 +
>>  3 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
>> index 93b9c68..ad5d4e5 100644
>> --- a/Documentation/DocBook/media/v4l/controls.xml
>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
>>  	    conversion.
>>  	    </entry>
>>  	  </row>
>> +	  <row>
>> +	    <entry spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
>> +	    <entry>integer</entry>
>> +	  </row>
>> +	  <row>
>> +	    <entry spanname="descr">Maximum size of a data frame in media bus
>> +	      sample units. This control determines maximum number of samples
>> +	      transmitted per single compressed data frame. For generic raw
>> +	      pixel formats the value of this control is undefined. This is
>> +	      a read-only control.
>> +	    </entry>
>> +	  </row>
>>  	  <row><entry></entry></row>
>>  	</tbody>
>>        </tgroup>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index b6a2ee7..0043fd2 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>>  	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>>  	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
> 
> I would put this to the image processing class, as the control isn't related
> to image capture. Jpeg encoding (or image compression in general) after all
> is related to image processing rather than capturing it.

All right, might make more sense that way. Let me move it to the image
processing class then. It probably also makes sense to name it
V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.

>>  	/* Image processing controls */
>>  	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
>> @@ -933,6 +934,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_FLASH_STROBE_STATUS:
>>  	case V4L2_CID_AUTO_FOCUS_STATUS:
>>  	case V4L2_CID_FLASH_READY:
>> +	case V4L2_CID_FRAMESIZE:
>>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>  		break;
>>  	}
>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> index 7a147c8..d3fd19f 100644
>> --- a/include/linux/videodev2.h
>> +++ b/include/linux/videodev2.h
>> @@ -1985,6 +1985,7 @@ enum v4l2_jpeg_chroma_subsampling {
>>  #define V4L2_CID_VBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
>>  #define V4L2_CID_HBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
>>  #define V4L2_CID_ANALOGUE_GAIN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
>> +#define V4L2_CID_FRAMESIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
>>  
>>  /* Image processing controls */
>>  #define V4L2_CID_IMAGE_PROC_CLASS_BASE		(V4L2_CTRL_CLASS_IMAGE_PROC | 0x900)

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sakari Ailus Aug. 23, 2012, 6:24 p.m. UTC | #3
Hi Sylwester,

Sylwester Nawrocki wrote:
> On 08/23/2012 02:13 PM, Sakari Ailus wrote:
>> Hi Sylwester,
>>
>> Thanks for the patch.
>
> Thanks for your review.
>
>> On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
>>> The V4L2_CID_FRAMESIZE control determines maximum number
>>> of media bus samples transmitted within a single data frame.
>>> It is useful for determining size of data buffer at the
>>> receiver side.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>   Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
>>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
>>>   include/linux/videodev2.h                    |  1 +
>>>   3 files changed, 15 insertions(+)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
>>> index 93b9c68..ad5d4e5 100644
>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
>>>   	    conversion.
>>>   	    </entry>
>>>   	  </row>
>>> +	  <row>
>>> +	    <entry spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
>>> +	    <entry>integer</entry>
>>> +	  </row>
>>> +	  <row>
>>> +	    <entry spanname="descr">Maximum size of a data frame in media bus
>>> +	      sample units. This control determines maximum number of samples
>>> +	      transmitted per single compressed data frame. For generic raw
>>> +	      pixel formats the value of this control is undefined. This is
>>> +	      a read-only control.
>>> +	    </entry>
>>> +	  </row>
>>>   	  <row><entry></entry></row>
>>>   	</tbody>
>>>         </tgroup>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index b6a2ee7..0043fd2 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>   	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>>>   	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>>>   	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
>>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
>>
>> I would put this to the image processing class, as the control isn't related
>> to image capture. Jpeg encoding (or image compression in general) after all
>> is related to image processing rather than capturing it.
>
> All right, might make more sense that way. Let me move it to the image
> processing class then. It probably also makes sense to name it
> V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.

Hmm. While we're at it, as the size is maximum --- it can be lower --- 
how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the 
unit is samples?

Does sample in this context mean pixels for uncompressed formats and 
bytes (octets) for compressed formats? It's important to define it as 
we're also using the term "sample" to refer to data units transferred 
over a parallel bus per a clock cycle.

On serial busses the former meaning is more obvious.

Kind regards,
Laurent Pinchart Aug. 23, 2012, 10:41 p.m. UTC | #4
Hi Sylwester,

On Thursday 23 August 2012 21:24:12 Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
> >> On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
> >>> The V4L2_CID_FRAMESIZE control determines maximum number
> >>> of media bus samples transmitted within a single data frame.
> >>> It is useful for determining size of data buffer at the
> >>> receiver side.
> >>> 
> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>> ---
> >>> 
> >>>   Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
> >>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
> >>>   include/linux/videodev2.h                    |  1 +
> >>>   3 files changed, 15 insertions(+)
> >>> 
> >>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> >>> b/Documentation/DocBook/media/v4l/controls.xml index 93b9c68..ad5d4e5
> >>> 100644
> >>> --- a/Documentation/DocBook/media/v4l/controls.xml
> >>> +++ b/Documentation/DocBook/media/v4l/controls.xml
> >>> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
> >>> 
> >>>   	    conversion.
> >>>   	    </entry>
> >>>   	  
> >>>   	  </row>
> >>> 
> >>> +	  <row>
> >>> +	    <entry
> >>> spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
> >>> +	    <entry>integer</entry>
> >>> +	  </row>
> >>> +	  <row>
> >>> +	    <entry spanname="descr">Maximum size of a data frame in media bus
> >>> +	      sample units. This control determines maximum number of samples
> >>> +	      transmitted per single compressed data frame. For generic raw
> >>> +	      pixel formats the value of this control is undefined. This is
> >>> +	      a read-only control.
> >>> +	    </entry>
> >>> +	  </row>
> >>> 
> >>>   	  <row><entry></entry></row>
> >>>   	
> >>>   	</tbody>
> >>>   	
> >>>         </tgroup>
> >>> 
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> b/drivers/media/v4l2-core/v4l2-ctrls.c index b6a2ee7..0043fd2 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>> 
> >>>   	case V4L2_CID_VBLANK:			return "Vertical Blanking";
> >>>   	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
> >>>   	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
> >>> 
> >>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
> >> 
> >> I would put this to the image processing class, as the control isn't
> >> related to image capture. Jpeg encoding (or image compression in
> >> general) after all is related to image processing rather than capturing
> >> it.
> > 
> > All right, might make more sense that way. Let me move it to the image
> > processing class then. It probably also makes sense to name it
> > V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.
> 
> Hmm. While we're at it, as the size is maximum --- it can be lower ---
> how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the
> unit is samples?
> 
> Does sample in this context mean pixels for uncompressed formats and
> bytes (octets) for compressed formats? It's important to define it as
> we're also using the term "sample" to refer to data units transferred
> over a parallel bus per a clock cycle.

I agree with Sakari here, I find the documentation quite vague, I wouldn't 
understand what the control is meant for from the documentation only.

> On serial busses the former meaning is more obvious.
Hi,

On 08/24/2012 12:41 AM, Laurent Pinchart wrote:
> On Thursday 23 August 2012 21:24:12 Sakari Ailus wrote:
>> Sylwester Nawrocki wrote:
>>>> On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
>>>>> The V4L2_CID_FRAMESIZE control determines maximum number
>>>>> of media bus samples transmitted within a single data frame.
>>>>> It is useful for determining size of data buffer at the
>>>>> receiver side.
>>>>>
>>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>> ---
>>>>>
>>>>>   Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
>>>>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
>>>>>   include/linux/videodev2.h                    |  1 +
>>>>>   3 files changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>>> b/Documentation/DocBook/media/v4l/controls.xml index 93b9c68..ad5d4e5
>>>>> 100644
>>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>>> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
>>>>>
>>>>>   	    conversion.
>>>>>   	    </entry>
>>>>>   	  
>>>>>   	  </row>
>>>>>
>>>>> +	  <row>
>>>>> +	    <entry
>>>>> spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
>>>>> +	    <entry>integer</entry>
>>>>> +	  </row>
>>>>> +	  <row>
>>>>> +	    <entry spanname="descr">Maximum size of a data frame in media bus
>>>>> +	      sample units. This control determines maximum number of samples
>>>>> +	      transmitted per single compressed data frame. For generic raw
>>>>> +	      pixel formats the value of this control is undefined. This is
>>>>> +	      a read-only control.
>>>>> +	    </entry>
>>>>> +	  </row>
>>>>>
>>>>>   	  <row><entry></entry></row>
>>>>>   	
>>>>>   	</tbody>
>>>>>   	
>>>>>         </tgroup>
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c index b6a2ee7..0043fd2 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>>
>>>>>   	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>>>>>   	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>>>>>   	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
>>>>>
>>>>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
>>>>
>>>> I would put this to the image processing class, as the control isn't
>>>> related to image capture. Jpeg encoding (or image compression in
>>>> general) after all is related to image processing rather than capturing
>>>> it.
>>>
>>> All right, might make more sense that way. Let me move it to the image
>>> processing class then. It probably also makes sense to name it
>>> V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.
>>
>> Hmm. While we're at it, as the size is maximum --- it can be lower ---
>> how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the
>> unit is samples?
>
>> Does sample in this context mean pixels for uncompressed formats and
>> bytes (octets) for compressed formats? It's important to define it as
>> we're also using the term "sample" to refer to data units transferred
>> over a parallel bus per a clock cycle.
> 
> I agree with Sakari here, I find the documentation quite vague, I wouldn't 
> understand what the control is meant for from the documentation only.

I thought it was clear enough:

Maximum size of a data frame in media bus sample units.
                             ^^^^^^^^^^^^^^^^^^^^^^^^^
So that means the unit is a number of bits clocked by a single clock
pulse on parallel video bus... :) But how is media bus sample defined
in case of CSI bus ? Looks like "media bus sample" is a useless term
for our purpose.

I thought it was better than just 8-bit byte, because the data receiver
(bridge) could layout data received from video bus in various ways in
memory, e.g. add some padding. OTOH, would any padding make sense
for compressed streams ? It would break the content, wouldn't it ?

So I would propose to use 8-bit byte as a unit for this control and
name it V4L2_CID_MAX_FRAME_SIZE. All in all it's not really tied
to the media bus.

>> On serial busses the former meaning is more obvious.

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sakari Ailus Aug. 24, 2012, 10:51 p.m. UTC | #6
Hi Sylwester,

On Fri, Aug 24, 2012 at 10:15:58AM +0200, Sylwester Nawrocki wrote:
> On 08/24/2012 12:41 AM, Laurent Pinchart wrote:
> > On Thursday 23 August 2012 21:24:12 Sakari Ailus wrote:
> >> Sylwester Nawrocki wrote:
> >>>> On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
> >>>>> The V4L2_CID_FRAMESIZE control determines maximum number
> >>>>> of media bus samples transmitted within a single data frame.
> >>>>> It is useful for determining size of data buffer at the
> >>>>> receiver side.
> >>>>>
> >>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>>>> ---
> >>>>>
> >>>>>   Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
> >>>>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
> >>>>>   include/linux/videodev2.h                    |  1 +
> >>>>>   3 files changed, 15 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> >>>>> b/Documentation/DocBook/media/v4l/controls.xml index 93b9c68..ad5d4e5
> >>>>> 100644
> >>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
> >>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
> >>>>> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
> >>>>>
> >>>>>   	    conversion.
> >>>>>   	    </entry>
> >>>>>   	  
> >>>>>   	  </row>
> >>>>>
> >>>>> +	  <row>
> >>>>> +	    <entry
> >>>>> spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
> >>>>> +	    <entry>integer</entry>
> >>>>> +	  </row>
> >>>>> +	  <row>
> >>>>> +	    <entry spanname="descr">Maximum size of a data frame in media bus
> >>>>> +	      sample units. This control determines maximum number of samples
> >>>>> +	      transmitted per single compressed data frame. For generic raw
> >>>>> +	      pixel formats the value of this control is undefined. This is
> >>>>> +	      a read-only control.
> >>>>> +	    </entry>
> >>>>> +	  </row>
> >>>>>
> >>>>>   	  <row><entry></entry></row>
> >>>>>   	
> >>>>>   	</tbody>
> >>>>>   	
> >>>>>         </tgroup>
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c index b6a2ee7..0043fd2 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>>>
> >>>>>   	case V4L2_CID_VBLANK:			return "Vertical Blanking";
> >>>>>   	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
> >>>>>   	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
> >>>>>
> >>>>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
> >>>>
> >>>> I would put this to the image processing class, as the control isn't
> >>>> related to image capture. Jpeg encoding (or image compression in
> >>>> general) after all is related to image processing rather than capturing
> >>>> it.
> >>>
> >>> All right, might make more sense that way. Let me move it to the image
> >>> processing class then. It probably also makes sense to name it
> >>> V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.
> >>
> >> Hmm. While we're at it, as the size is maximum --- it can be lower ---
> >> how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the
> >> unit is samples?
> >
> >> Does sample in this context mean pixels for uncompressed formats and
> >> bytes (octets) for compressed formats? It's important to define it as
> >> we're also using the term "sample" to refer to data units transferred
> >> over a parallel bus per a clock cycle.
> > 
> > I agree with Sakari here, I find the documentation quite vague, I wouldn't 
> > understand what the control is meant for from the documentation only.
> 
> I thought it was clear enough:
> 
> Maximum size of a data frame in media bus sample units.
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^

Oops. I somehow managed to miss that. My mistake.

> So that means the unit is a number of bits clocked by a single clock
> pulse on parallel video bus... :) But how is media bus sample defined
> in case of CSI bus ? Looks like "media bus sample" is a useless term
> for our purpose.

The CSI2 transmitters and receivers, as far as I understand, want to know a
lot more about the data that I think would be necessary. This doesn't only
involve the bits per sample (e.g. pixel for raw bayer formats) but some
information on the media bus code, too. I.e. if you're transferring 11 bit
pixels the bus has to know that.

I think it would have been better to use different media bus codes for
serial busses than on parallel busses that transfer the sample on a single
clock cycle. But that's out of the scope of this patch.

In respect to this the CCP2 AFAIR works mostly the same way.

> I thought it was better than just 8-bit byte, because the data receiver
> (bridge) could layout data received from video bus in various ways in
> memory, e.g. add some padding. OTOH, would any padding make sense
> for compressed streams ? It would break the content, wouldn't it ?

I guess it't break if the padding is applied anywhere else than the end,
where I hope it's ok. I'm not that familiar with compressed formats, though.
The hardware typically has limitations on the DMA transaction width and that
can easily be e.g. 32 or 64 bytes, so some padding may easily be introduced
at the end of the compressed image.

> So I would propose to use 8-bit byte as a unit for this control and
> name it V4L2_CID_MAX_FRAME_SIZE. All in all it's not really tied
> to the media bus.

It took me quite a while toe remember what the control really was for. ;)

How about using bytes on video nodes and bus and media bus code specific
extended samples (or how we should call pixels in uncompressed formats and
units of data in compressed formats?) on subdevs? The information how the
former is derived from the latter resides in the driver controlling the DMA
anyway.

As you proposed originally, this control is more relevant to subdevs so we
could also not define it for video nodes at all. Especially if the control
isn't needed: the information should be available from VIDIOC_TRY_FMT.

Kind regards,
Sylwester Nawrocki Aug. 26, 2012, 7:22 p.m. UTC | #7
Hi Sakari,

On 08/25/2012 12:51 AM, Sakari Ailus wrote:
>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>>>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>>>>
>>>>>>>    	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>>>>>>>    	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>>>>>>>    	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
>>>>>>>
>>>>>>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
>>>>>>
>>>>>> I would put this to the image processing class, as the control isn't
>>>>>> related to image capture. Jpeg encoding (or image compression in
>>>>>> general) after all is related to image processing rather than capturing
>>>>>> it.
>>>>>
>>>>> All right, might make more sense that way. Let me move it to the image
>>>>> processing class then. It probably also makes sense to name it
>>>>> V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.
>>>>
>>>> Hmm. While we're at it, as the size is maximum --- it can be lower ---
>>>> how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the
>>>> unit is samples?
>>>
>>>> Does sample in this context mean pixels for uncompressed formats and
>>>> bytes (octets) for compressed formats? It's important to define it as
>>>> we're also using the term "sample" to refer to data units transferred
>>>> over a parallel bus per a clock cycle.
>>>
>>> I agree with Sakari here, I find the documentation quite vague, I wouldn't
>>> understand what the control is meant for from the documentation only.
>>
>> I thought it was clear enough:
>>
>> Maximum size of a data frame in media bus sample units.
>>                               ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Oops. I somehow managed to miss that. My mistake.
> 
>> So that means the unit is a number of bits clocked by a single clock
>> pulse on parallel video bus... :) But how is media bus sample defined
>> in case of CSI bus ? Looks like "media bus sample" is a useless term
>> for our purpose.
> 
> The CSI2 transmitters and receivers, as far as I understand, want to know a
> lot more about the data that I think would be necessary. This doesn't only
> involve the bits per sample (e.g. pixel for raw bayer formats) but some
> information on the media bus code, too. I.e. if you're transferring 11 bit
> pixels the bus has to know that.
> 
> I think it would have been better to use different media bus codes for
> serial busses than on parallel busses that transfer the sample on a single
> clock cycle. But that's out of the scope of this patch.
> 
> In respect to this the CCP2 AFAIR works mostly the same way.
> 
>> I thought it was better than just 8-bit byte, because the data receiver
>> (bridge) could layout data received from video bus in various ways in
>> memory, e.g. add some padding. OTOH, would any padding make sense
>> for compressed streams ? It would break the content, wouldn't it ?
> 
> I guess it't break if the padding is applied anywhere else than the end,
> where I hope it's ok. I'm not that familiar with compressed formats, though.
> The hardware typically has limitations on the DMA transaction width and that
> can easily be e.g. 32 or 64 bytes, so some padding may easily be introduced
> at the end of the compressed image.

The padding at the frame end is not a problem, since it would be a property
of a bridge. So the reported sizeimage value with VIDIOC_G_FMT, for example, 
could be easily adjusted a a bridge driver to account any padding.

>> So I would propose to use 8-bit byte as a unit for this control and
>> name it V4L2_CID_MAX_FRAME_SIZE. All in all it's not really tied
>> to the media bus.
> 
> It took me quite a while toe remember what the control really was for. ;)

:-) Yeah, looks like I have been going in circles with this issue.. ;)

> How about using bytes on video nodes and bus and media bus code specific
> extended samples (or how we should call pixels in uncompressed formats and
> units of data in compressed formats?) on subdevs? The information how the
> former is derived from the latter resides in the driver controlling the DMA
> anyway.
> 
> As you proposed originally, this control is more relevant to subdevs so we
> could also not define it for video nodes at all. Especially if the control
> isn't needed: the information should be available from VIDIOC_TRY_FMT.

Yeah, I seem to have forgotten to add a note that this control would be
valid only on subdev nodes :/
OTOH, how do we handle cases where subdev's controls are inherited by 
a video node ? Referring to an media bus pixel code seems wrong in that 
cases.

Also for compressed formats, where this control is only needed, the bus
receivers/DMA just do transparent transfers, without actually modifying 
the data stream.

The problem is that ideally this V4L2_CID_FRAMESIZE control shouldn't be 
exposed to user space. It might be useful, for example for checking what 
would be resulting file size on a subdev modelling a JPEG encoder, etc.
I agree on video nodes ioctls like VIDIOC_[S/G/TRY]_FMT are just sufficient 
for retrieving that information.

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sakari Ailus Aug. 27, 2012, 7:28 p.m. UTC | #8
Hi Sylwester,

On Sun, Aug 26, 2012 at 09:22:12PM +0200, Sylwester Nawrocki wrote:
> On 08/25/2012 12:51 AM, Sakari Ailus wrote:
> >>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>>>>>
> >>>>>>>    	case V4L2_CID_VBLANK:			return "Vertical Blanking";
> >>>>>>>    	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
> >>>>>>>    	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
> >>>>>>>
> >>>>>>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
> >>>>>>
> >>>>>> I would put this to the image processing class, as the control isn't
> >>>>>> related to image capture. Jpeg encoding (or image compression in
> >>>>>> general) after all is related to image processing rather than capturing
> >>>>>> it.
> >>>>>
> >>>>> All right, might make more sense that way. Let me move it to the image
> >>>>> processing class then. It probably also makes sense to name it
> >>>>> V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.
> >>>>
> >>>> Hmm. While we're at it, as the size is maximum --- it can be lower ---
> >>>> how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the
> >>>> unit is samples?
> >>>
> >>>> Does sample in this context mean pixels for uncompressed formats and
> >>>> bytes (octets) for compressed formats? It's important to define it as
> >>>> we're also using the term "sample" to refer to data units transferred
> >>>> over a parallel bus per a clock cycle.
> >>>
> >>> I agree with Sakari here, I find the documentation quite vague, I wouldn't
> >>> understand what the control is meant for from the documentation only.
> >>
> >> I thought it was clear enough:
> >>
> >> Maximum size of a data frame in media bus sample units.
> >>                               ^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Oops. I somehow managed to miss that. My mistake.
> > 
> >> So that means the unit is a number of bits clocked by a single clock
> >> pulse on parallel video bus... :) But how is media bus sample defined
> >> in case of CSI bus ? Looks like "media bus sample" is a useless term
> >> for our purpose.
> > 
> > The CSI2 transmitters and receivers, as far as I understand, want to know a
> > lot more about the data that I think would be necessary. This doesn't only
> > involve the bits per sample (e.g. pixel for raw bayer formats) but some
> > information on the media bus code, too. I.e. if you're transferring 11 bit
> > pixels the bus has to know that.
> > 
> > I think it would have been better to use different media bus codes for
> > serial busses than on parallel busses that transfer the sample on a single
> > clock cycle. But that's out of the scope of this patch.
> > 
> > In respect to this the CCP2 AFAIR works mostly the same way.
> > 
> >> I thought it was better than just 8-bit byte, because the data receiver
> >> (bridge) could layout data received from video bus in various ways in
> >> memory, e.g. add some padding. OTOH, would any padding make sense
> >> for compressed streams ? It would break the content, wouldn't it ?
> > 
> > I guess it't break if the padding is applied anywhere else than the end,
> > where I hope it's ok. I'm not that familiar with compressed formats, though.
> > The hardware typically has limitations on the DMA transaction width and that
> > can easily be e.g. 32 or 64 bytes, so some padding may easily be introduced
> > at the end of the compressed image.
> 
> The padding at the frame end is not a problem, since it would be a property
> of a bridge. So the reported sizeimage value with VIDIOC_G_FMT, for example, 
> could be easily adjusted a a bridge driver to account any padding.
> 
> >> So I would propose to use 8-bit byte as a unit for this control and
> >> name it V4L2_CID_MAX_FRAME_SIZE. All in all it's not really tied
> >> to the media bus.
> > 
> > It took me quite a while toe remember what the control really was for. ;)
> 
> :-) Yeah, looks like I have been going in circles with this issue.. ;)
> 
> > How about using bytes on video nodes and bus and media bus code specific
> > extended samples (or how we should call pixels in uncompressed formats and
> > units of data in compressed formats?) on subdevs? The information how the
> > former is derived from the latter resides in the driver controlling the DMA
> > anyway.
> > 
> > As you proposed originally, this control is more relevant to subdevs so we
> > could also not define it for video nodes at all. Especially if the control
> > isn't needed: the information should be available from VIDIOC_TRY_FMT.
> 
> Yeah, I seem to have forgotten to add a note that this control would be
> valid only on subdev nodes :/
> OTOH, how do we handle cases where subdev's controls are inherited by 
> a video node ? Referring to an media bus pixel code seems wrong in that 
> cases.

I don't think this control ever should be visible on a video device if we
define it's only valid for subdevs. Even then, if it's implemented by a
subdev driver, its value refers to samples rather than bytes which would
make more sense on a video node (in case we'd define it there).

AFAIR Hans once suggested a flag to hide low-level controls from video nodes
and inherit the rest from subdevs; I think that would be a valid use case
for this flag. On the other hand, I see little or no meaningful use for
inheriting controls from subdevs on systems I'm the most familiar with, but
it may be more useful elsewhere. Most of the subdev controls are not
something a regular V4L2 application would like to touch as such; look at
the image processing controls, for example.

> Also for compressed formats, where this control is only needed, the bus
> receivers/DMA just do transparent transfers, without actually modifying 
> the data stream.

That makes sense. I don't have the documentation in front of my eyes right
now, but what would you think about adding a note to the documentation that
the control is only valid for compressed formats? That would limit the
defined use of it to cases we (or you) know well?

> The problem is that ideally this V4L2_CID_FRAMESIZE control shouldn't be 
> exposed to user space. It might be useful, for example for checking what 
> would be resulting file size on a subdev modelling a JPEG encoder, etc.
> I agree on video nodes ioctls like VIDIOC_[S/G/TRY]_FMT are just sufficient 
> for retrieving that information.

One case I could imagine where the user might want to touch the control is
to modify the maximum size of the compressed images, should the hardware
support that. But in that case, the user should be aware of how to convert
samples into bytes, and I don't see a regular V4L2 application should
necessarily be aware of something potentially as hardware-dependent as that.

Cc Hans.

Kind regards,
Sylwester Nawrocki Sept. 11, 2012, 7:21 p.m. UTC | #9
Hi Sakari,

On 08/27/2012 09:28 PM, Sakari Ailus wrote:
>>> How about using bytes on video nodes and bus and media bus code specific
>>> extended samples (or how we should call pixels in uncompressed formats and
>>> units of data in compressed formats?) on subdevs? The information how the
>>> former is derived from the latter resides in the driver controlling the DMA
>>> anyway.
>>>
>>> As you proposed originally, this control is more relevant to subdevs so we
>>> could also not define it for video nodes at all. Especially if the control
>>> isn't needed: the information should be available from VIDIOC_TRY_FMT.
>>
>> Yeah, I seem to have forgotten to add a note that this control would be
>> valid only on subdev nodes :/
>> OTOH, how do we handle cases where subdev's controls are inherited by
>> a video node ? Referring to an media bus pixel code seems wrong in that
>> cases.
> 
> I don't think this control ever should be visible on a video device if we
> define it's only valid for subdevs. Even then, if it's implemented by a
> subdev driver, its value refers to samples rather than bytes which would
> make more sense on a video node (in case we'd define it there).

I agree as for not exposing such a control on video nodes. We don't seem to
have support for that in v4l2-core though.

Hmm, not sure how would media bus data samples make sense on a video node,
where media bus is not quite defined at the user space API level.

> AFAIR Hans once suggested a flag to hide low-level controls from video nodes
> and inherit the rest from subdevs; I think that would be a valid use case
> for this flag. On the other hand, I see little or no meaningful use for

Yeah, sounds interesting.

> inheriting controls from subdevs on systems I'm the most familiar with, but
> it may be more useful elsewhere. Most of the subdev controls are not
> something a regular V4L2 application would like to touch as such; look at
> the image processing controls, for example.

Yeah, but in general controls can be inherited and there could be an 
ambiguity. Not all drivers involving subdevs use the subdev user space API.

>> Also for compressed formats, where this control is only needed, the bus
>> receivers/DMA just do transparent transfers, without actually modifying
>> the data stream.
> 
> That makes sense. I don't have the documentation in front of my eyes right
> now, but what would you think about adding a note to the documentation that
> the control is only valid for compressed formats? That would limit the
> defined use of it to cases we (or you) know well?

I think that could be done, all in all nobody ever needed such a control
for uncompressed/raw formats so far.

>> The problem is that ideally this V4L2_CID_FRAMESIZE control shouldn't be
>> exposed to user space. It might be useful, for example for checking what
>> would be resulting file size on a subdev modelling a JPEG encoder, etc.
>> I agree on video nodes ioctls like VIDIOC_[S/G/TRY]_FMT are just sufficient
>> for retrieving that information.
> 
> One case I could imagine where the user might want to touch the control is
> to modify the maximum size of the compressed images, should the hardware
> support that. But in that case, the user should be aware of how to convert
> samples into bytes, and I don't see a regular V4L2 application should
> necessarily be aware of something potentially as hardware-dependent as that.

Yes, V4L2_CID_FRAME_SIZE/SAMPLES looks like something only for a library
to play with. But for compressed formats conversion framesamples <-> data
octets in a memory buffer should be rather straightforward.
 
That said, V4L2_CID_FRAME_SAMPLES control would have been insufficient
where you want to query frame sizes for multiple logical streams within
a frame, e.g. multi-planar frame transmitted on MIPI-CSI2 bus with 
different data types and received on user space side as a multi-planar
v4l2 buffer. We would need to query a value for each plane and single 
control won't do the job in such cases. Hence I'm inclined to drop the 
idea of V4L2_CID_FRAME_SAMPLES control and carry on with the frame format 
descriptors approach. We probably can't/shouldn't have both solutions 
in use.

--

Regards,
Sylwester

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil Sept. 12, 2012, 6:48 a.m. UTC | #10
On Tue September 11 2012 21:21:03 Sylwester Nawrocki wrote:
> Hi Sakari,
> 
> On 08/27/2012 09:28 PM, Sakari Ailus wrote:
> >>> How about using bytes on video nodes and bus and media bus code specific
> >>> extended samples (or how we should call pixels in uncompressed formats and
> >>> units of data in compressed formats?) on subdevs? The information how the
> >>> former is derived from the latter resides in the driver controlling the DMA
> >>> anyway.
> >>>
> >>> As you proposed originally, this control is more relevant to subdevs so we
> >>> could also not define it for video nodes at all. Especially if the control
> >>> isn't needed: the information should be available from VIDIOC_TRY_FMT.
> >>
> >> Yeah, I seem to have forgotten to add a note that this control would be
> >> valid only on subdev nodes :/
> >> OTOH, how do we handle cases where subdev's controls are inherited by
> >> a video node ? Referring to an media bus pixel code seems wrong in that
> >> cases.
> > 
> > I don't think this control ever should be visible on a video device if we
> > define it's only valid for subdevs. Even then, if it's implemented by a
> > subdev driver, its value refers to samples rather than bytes which would
> > make more sense on a video node (in case we'd define it there).
> 
> I agree as for not exposing such a control on video nodes. We don't seem to
> have support for that in v4l2-core though.

Yes, there is: if you set the is_private flag of struct v4l2_ctrl then that
control will not be inherited by a video device and will only be visible on
the v4l2-subdevX node.

> Hmm, not sure how would media bus data samples make sense on a video node,
> where media bus is not quite defined at the user space API level.

It doesn't make sense.

> > AFAIR Hans once suggested a flag to hide low-level controls from video nodes
> > and inherit the rest from subdevs; I think that would be a valid use case
> > for this flag. On the other hand, I see little or no meaningful use for
> 
> Yeah, sounds interesting.
> 
> > inheriting controls from subdevs on systems I'm the most familiar with, but
> > it may be more useful elsewhere.

Inheriting controls is very common for video receivers: in many cases the video
receiver subdev is the one that implements common controls like brightness,
contrast, etc. You really want to inherit controls there.

Private controls are actually not used at the moment. They only make sense if
your driver has the Media Controller API as well so apps can discover the
subdev nodes.

The problem is that all too often I found that you want to control in the bridge
driver whether or not to honor the is_private flag. If you have just a simple
pipeline, then exposing those private controls on a video node actually makes
sense since you don't need to use the MC and open additional subdev nodes.

Currently no method to ignore the is_private flag exists (although it would
be easy to add).

My 'hide low-level controls' flag idea was something different: it would only
control whether such low-level controls would show up in a GUI.

It is basically just a hint for applications.

Regards,

	Hans

> > Most of the subdev controls are not
> > something a regular V4L2 application would like to touch as such; look at
> > the image processing controls, for example.
> 
> Yeah, but in general controls can be inherited and there could be an 
> ambiguity. Not all drivers involving subdevs use the subdev user space API.
> 
> >> Also for compressed formats, where this control is only needed, the bus
> >> receivers/DMA just do transparent transfers, without actually modifying
> >> the data stream.
> > 
> > That makes sense. I don't have the documentation in front of my eyes right
> > now, but what would you think about adding a note to the documentation that
> > the control is only valid for compressed formats? That would limit the
> > defined use of it to cases we (or you) know well?
> 
> I think that could be done, all in all nobody ever needed such a control
> for uncompressed/raw formats so far.
> 
> >> The problem is that ideally this V4L2_CID_FRAMESIZE control shouldn't be
> >> exposed to user space. It might be useful, for example for checking what
> >> would be resulting file size on a subdev modelling a JPEG encoder, etc.
> >> I agree on video nodes ioctls like VIDIOC_[S/G/TRY]_FMT are just sufficient
> >> for retrieving that information.
> > 
> > One case I could imagine where the user might want to touch the control is
> > to modify the maximum size of the compressed images, should the hardware
> > support that. But in that case, the user should be aware of how to convert
> > samples into bytes, and I don't see a regular V4L2 application should
> > necessarily be aware of something potentially as hardware-dependent as that.
> 
> Yes, V4L2_CID_FRAME_SIZE/SAMPLES looks like something only for a library
> to play with. But for compressed formats conversion framesamples <-> data
> octets in a memory buffer should be rather straightforward.
>  
> That said, V4L2_CID_FRAME_SAMPLES control would have been insufficient
> where you want to query frame sizes for multiple logical streams within
> a frame, e.g. multi-planar frame transmitted on MIPI-CSI2 bus with 
> different data types and received on user space side as a multi-planar
> v4l2 buffer. We would need to query a value for each plane and single 
> control won't do the job in such cases. Hence I'm inclined to drop the 
> idea of V4L2_CID_FRAME_SAMPLES control and carry on with the frame format 
> descriptors approach. We probably can't/shouldn't have both solutions 
> in use.
> 
> --
> 
> Regards,
> Sylwester
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 93b9c68..ad5d4e5 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -4184,6 +4184,18 @@  interface and may change in the future.</para>
 	    conversion.
 	    </entry>
 	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Maximum size of a data frame in media bus
+	      sample units. This control determines maximum number of samples
+	      transmitted per single compressed data frame. For generic raw
+	      pixel formats the value of this control is undefined. This is
+	      a read-only control.
+	    </entry>
+	  </row>
 	  <row><entry></entry></row>
 	</tbody>
       </tgroup>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b6a2ee7..0043fd2 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -727,6 +727,7 @@  const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_VBLANK:			return "Vertical Blanking";
 	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
 	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
+	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
 
 	/* Image processing controls */
 	case V4L2_CID_IMAGE_PROC_CLASS:		return "Image Processing Controls";
@@ -933,6 +934,7 @@  void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_FLASH_STROBE_STATUS:
 	case V4L2_CID_AUTO_FOCUS_STATUS:
 	case V4L2_CID_FLASH_READY:
+	case V4L2_CID_FRAMESIZE:
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
 	}
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 7a147c8..d3fd19f 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1985,6 +1985,7 @@  enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_VBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 1)
 #define V4L2_CID_HBLANK				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
 #define V4L2_CID_ANALOGUE_GAIN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
+#define V4L2_CID_FRAMESIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
 
 /* Image processing controls */
 #define V4L2_CID_IMAGE_PROC_CLASS_BASE		(V4L2_CTRL_CLASS_IMAGE_PROC | 0x900)