[RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[Sakari Ailus]

Hi all,

I recently came across a few issues in the definitions of v4l2_subdev_format
and v4l2_mbus_framefmt when I was working on sensor control that I wanted to
bring up here. The appropriate structure right now look like this:

include/linux/v4l2-subdev.h:
---8<---
/**
 * struct v4l2_subdev_format - Pad-level media bus format
 * @which: format type (from enum v4l2_subdev_format_whence)
 * @pad: pad number, as reported by the media API
 * @format: media bus format (format code and frame size)
 */
struct v4l2_subdev_format {
        __u32 which;
        __u32 pad;
        struct v4l2_mbus_framefmt format;
        __u32 reserved[8];
};
---8<---

include/linux/v4l2-mediabus.h:
---8<---
/**
 * struct v4l2_mbus_framefmt - frame format on the media bus
 * @width:      frame width
 * @height:     frame height
 * @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)
 */
struct v4l2_mbus_framefmt {
        __u32                   width;
        __u32                   height;
        __u32                   code;
        __u32                   field;
        __u32                   colorspace;
        __u32                   reserved[7];
};
---8<---

Offering a lower level interface for sensors which allows better control of
them from the user space involves providing the link frequency to the user
space. While the link frequency will be a control, together with the bus
type and number of lanes (on serial links), this will define the pixel
clock.

<URL:http://www.spinics.net/lists/linux-media/msg36492.html>

After adding pixel clock to v4l2_mbus_framefmt there will be six reserved
fields left, one of which will be further possibly consumed by maximum image
size:

<URL:http://www.spinics.net/lists/linux-media/msg35949.html>

Frame blanking (horizontal and vertical) and number of lanes might be needed
in the struct as well in the future, bringing the reserved count down to
two. I find this alarmingly low for a relatively new structure definition
which will potentially have a few different uses in the future.

The another issue is that the size of the v4l2_subdev_format struct is not
aligned to a power of two. Instead of the intended 32 u32's, the size is
actually 22 u32's.

The interface is present in the 3.0 and marked experimental. My proposal is
to add reserved fields to v4l2_mbus_framefmt to extend its size up to 32
u32's. I understand there are already few which use the interface right now
and thus this change must be done now or left as-is forever.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[Sylwester Nawrocki]

[-- Attachment #1: Type: text/plain, Size: 3840 bytes --]

Hi Sakari,

On 09/05/2011 05:55 PM, Sakari Ailus wrote:
> Hi all,
> 
> I recently came across a few issues in the definitions of v4l2_subdev_format
> and v4l2_mbus_framefmt when I was working on sensor control that I wanted to
> bring up here. The appropriate structure right now look like this:
> 
> include/linux/v4l2-subdev.h:
> ---8<---
> /**
>   * struct v4l2_subdev_format - Pad-level media bus format
>   * @which: format type (from enum v4l2_subdev_format_whence)
>   * @pad: pad number, as reported by the media API
>   * @format: media bus format (format code and frame size)
>   */
> struct v4l2_subdev_format {
>          __u32 which;
>          __u32 pad;
>          struct v4l2_mbus_framefmt format;
>          __u32 reserved[8];
> };
> ---8<---
> 
> include/linux/v4l2-mediabus.h:
> ---8<---
> /**
>   * struct v4l2_mbus_framefmt - frame format on the media bus
>   * @width:      frame width
>   * @height:     frame height
>   * @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)
>   */
> struct v4l2_mbus_framefmt {
>          __u32                   width;
>          __u32                   height;
>          __u32                   code;
>          __u32                   field;
>          __u32                   colorspace;
>          __u32                   reserved[7];
> };
> ---8<---
> 
> Offering a lower level interface for sensors which allows better control of
> them from the user space involves providing the link frequency to the user
> space. While the link frequency will be a control, together with the bus
> type and number of lanes (on serial links), this will define the pixel
> clock.
> 
> <URL:http://www.spinics.net/lists/linux-media/msg36492.html>
> 
> After adding pixel clock to v4l2_mbus_framefmt there will be six reserved
> fields left, one of which will be further possibly consumed by maximum image
> size:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg35949.html>

Yes, thanks for remembering about it. I have done some experiments with a sensor
producing JPEG data and I'd like to add '__u32 framesamples' field to struct
v4l2_mbus_framefmt, even though it solves only part of the problem.
I'm not sure when I'll be able to get this finished though. I've just attached
the initial patch now.

> 
> Frame blanking (horizontal and vertical) and number of lanes might be needed
> in the struct as well in the future, bringing the reserved count down to
> two. I find this alarmingly low for a relatively new structure definition
> which will potentially have a few different uses in the future.

Sorry, could you explain why we need to put the blanking information in struct
v4l2_mbus_framefmt ? I thought it had been initially agreed that the control
framework will be used for this.

> 
> The another issue is that the size of the v4l2_subdev_format struct is not
> aligned to a power of two. Instead of the intended 32 u32's, the size is
> actually 22 u32's.

hmm, is this really an issue ? What is advantage of having the structure size
being the power of 2 ? Isn't multiple of 4 just enough ?

> 
> The interface is present in the 3.0 and marked experimental. My proposal is
> to add reserved fields to v4l2_mbus_framefmt to extend its size up to 32
> u32's. I understand there are already few which use the interface right now
> and thus this change must be done now or left as-is forever.

hmm, I feel a bit uncomfortable with increasing size of data structure which
is quite widely used, not only in sensors, also in TV capture cards, tuners, etc.
So far struct v4l2_mbus_framefmt was quite generic. IMHO it might be good to try
to avoid extending it widely with properties specific to single subsystem.

--
Regards,
Sylwester

[-- Attachment #2: 0001-v4l-Add-framesamples-field-in-struct-v4l2_mbus_frame.patch --]
[-- Type: text/x-patch, Size: 4510 bytes --]

>From 18600fc32e65a6653491981b602af102f1dd52cb Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
Date: Sun, 21 Aug 2011 00:51:59 +0200
Subject: [PATCH/RFC] v4l: Add framesamples field in struct v4l2_mbus_framefmt

The purpose of the new 'framesamples' field is to allow
the video pipeline elements to negotiate memory buffer size
for a transmitted frame.
This is mostly useful for compressed data formats where the
buffer size cannot be derived from pixel width and height
and the pixel code.
In case of the user space subdev API the applications must
assure the 'framesamples' value is consistent across the
pipeline. The drivers should validate those values before
the streaming is enabled.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 Documentation/DocBook/media/v4l/dev-subdev.xml     |   15 +++++++++++----
 Documentation/DocBook/media/v4l/subdev-formats.xml |    7 ++++++-
 include/linux/v4l2-mediabus.h                      |    4 +++-
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/dev-subdev.xml b/Documentation/DocBook/media/v4l/dev-subdev.xml
index 05c8fef..48a1cdc 100644
--- a/Documentation/DocBook/media/v4l/dev-subdev.xml
+++ b/Documentation/DocBook/media/v4l/dev-subdev.xml
@@ -113,7 +113,7 @@
     <para>Drivers that implement the <link linkend="media-controller-intro">media
     API</link> can expose pad-level image format configuration to applications.
     When they do, applications can use the &VIDIOC-SUBDEV-G-FMT; and
-    &VIDIOC-SUBDEV-S-FMT; ioctls. to negotiate formats on a per-pad basis.</para>
+    &VIDIOC-SUBDEV-S-FMT; ioctls to negotiate formats on a per-pad basis.</para>
 
     <para>Applications are responsible for configuring coherent parameters on
     the whole pipeline and making sure that connected pads have compatible
@@ -158,9 +158,16 @@
 
       <para>Formats returned by the driver during a negotiation iteration are
       guaranteed to be supported by the device. In particular, drivers guarantee
-      that a returned format will not be further changed if passed to an
-      &VIDIOC-SUBDEV-S-FMT; call as-is (as long as external parameters, such as
-      formats on other pads or links' configuration are not changed).</para>
+      that returned format, except the <structfield>framesamples</structfield>
+      field, will not be further changed if passed to an &VIDIOC-SUBDEV-S-FMT;
+      call as-is (as long as external parameters, such as formats on other pads
+      or links' configuration are not changed).</para>
+
+      <para> For compressed data formats the number of samples per frame may be
+      influenced by controls related to the compression process. Thus the
+      applications should use &VIDIOC-SUBDEV-G-FMT; ioctl to query an exact format
+      whenever such controls have been applied after the negotiation iteration.
+      </para>
 
       <para>Drivers automatically propagate formats inside sub-devices. When a
       try or active format is set on a pad, corresponding formats on other pads
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>
+	</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
-- 
1.7.4.1

Re: [RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[Sakari Ailus]

On Tue, Sep 06, 2011 at 09:10:17PM +0200, Sylwester Nawrocki wrote:
> Hi Sakari,

Hi Sylwester,

> On 09/05/2011 05:55 PM, Sakari Ailus wrote:
> > Hi all,
> > 
> > I recently came across a few issues in the definitions of v4l2_subdev_format
> > and v4l2_mbus_framefmt when I was working on sensor control that I wanted to
> > bring up here. The appropriate structure right now look like this:
> > 
> > include/linux/v4l2-subdev.h:
> > ---8<---
> > /**
> >   * struct v4l2_subdev_format - Pad-level media bus format
> >   * @which: format type (from enum v4l2_subdev_format_whence)
> >   * @pad: pad number, as reported by the media API
> >   * @format: media bus format (format code and frame size)
> >   */
> > struct v4l2_subdev_format {
> >          __u32 which;
> >          __u32 pad;
> >          struct v4l2_mbus_framefmt format;
> >          __u32 reserved[8];
> > };
> > ---8<---
> > 
> > include/linux/v4l2-mediabus.h:
> > ---8<---
> > /**
> >   * struct v4l2_mbus_framefmt - frame format on the media bus
> >   * @width:      frame width
> >   * @height:     frame height
> >   * @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)
> >   */
> > struct v4l2_mbus_framefmt {
> >          __u32                   width;
> >          __u32                   height;
> >          __u32                   code;
> >          __u32                   field;
> >          __u32                   colorspace;
> >          __u32                   reserved[7];
> > };
> > ---8<---
> > 
> > Offering a lower level interface for sensors which allows better control of
> > them from the user space involves providing the link frequency to the user
> > space. While the link frequency will be a control, together with the bus
> > type and number of lanes (on serial links), this will define the pixel
> > clock.
> > 
> > <URL:http://www.spinics.net/lists/linux-media/msg36492.html>
> > 
> > After adding pixel clock to v4l2_mbus_framefmt there will be six reserved
> > fields left, one of which will be further possibly consumed by maximum image
> > size:
> > 
> > <URL:http://www.spinics.net/lists/linux-media/msg35949.html>
> 
> Yes, thanks for remembering about it. I have done some experiments with a sensor
> producing JPEG data and I'd like to add '__u32 framesamples' field to struct
> v4l2_mbus_framefmt, even though it solves only part of the problem.
> I'm not sure when I'll be able to get this finished though. I've just attached
> the initial patch now.
> 
> > 
> > Frame blanking (horizontal and vertical) and number of lanes might be needed
> > in the struct as well in the future, bringing the reserved count down to
> > two. I find this alarmingly low for a relatively new structure definition
> > which will potentially have a few different uses in the future.
> 
> Sorry, could you explain why we need to put the blanking information in struct
> v4l2_mbus_framefmt ? I thought it had been initially agreed that the control
> framework will be used for this.

Configuration of blanking will be implemented as controls, yes.

Bandwidth calculation in the ISP driver may well need to know more detailed
information than just the maximum pixel rate. Averge rate over certain
period may also be important.

For example, take a sensor which is able to produce pixel rate of 200 Mp/s.
In the OMAP 3 ISP only the CSI2 block will be able to process pixels at such
rate. The ISP driver needs this information to be able to decide whether
it's safe to start streaming or not.

Higher momentary pixel rates are still possible as there are buffers between
some of the blocks. When using downscaling on sensors this gets more tricky.
There may be bursts of data which may overflow these buffers since the
sensors do not output data at amortised rate. Information on the sensor
(bursts) and size of the buffers is at least required to assess this
question.

I have a vague feeling we may need some of this data as part of the
v4l2_mbus_framefmt before we have a solution.

> > The another issue is that the size of the v4l2_subdev_format struct is not
> > aligned to a power of two. Instead of the intended 32 u32's, the size is
> > actually 22 u32's.
> 
> hmm, is this really an issue ? What is advantage of having the structure size
> being the power of 2 ? Isn't multiple of 4 just enough ?

A power of two has been considered a good practice. It's also how kmalloc
will allocate memory for the duration of the ioctl call. Typical sizes can
be found in /proc/slabinfo. For small sizes also some non-power of two sizes
appear available, at least on my machines.

I'm not sure about the allocation in user space.

> > The interface is present in the 3.0 and marked experimental. My proposal is
> > to add reserved fields to v4l2_mbus_framefmt to extend its size up to 32
> > u32's. I understand there are already few which use the interface right now
> > and thus this change must be done now or left as-is forever.
> 
> hmm, I feel a bit uncomfortable with increasing size of data structure which
> is quite widely used, not only in sensors, also in TV capture cards, tuners, etc.
> So far struct v4l2_mbus_framefmt was quite generic. IMHO it might be good to try
> to avoid extending it widely with properties specific to single subsystem.

This is why I wanted to bring this up now rather than later. Of course I
should have realised these issues before we had .39. ;)

I agree the pixel rate management should be implemented in a way which is
not specific to sensors.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[Laurent Pinchart]

Hi Sylwester and Sakari,

On Tuesday 06 September 2011 23:07:43 Sakari Ailus wrote:
> On Tue, Sep 06, 2011 at 09:10:17PM +0200, Sylwester Nawrocki wrote:
> > On 09/05/2011 05:55 PM, Sakari Ailus wrote:
> > > Hi all,
> > > 
> > > I recently came across a few issues in the definitions of
> > > v4l2_subdev_format and v4l2_mbus_framefmt when I was working on sensor
> > > control that I wanted to bring up here. The appropriate structure
> > > right now look like this:
> > > 
> > > include/linux/v4l2-subdev.h:
> > > ---8<---
> > > /**
> > > 
> > >   * struct v4l2_subdev_format - Pad-level media bus format
> > >   * @which: format type (from enum v4l2_subdev_format_whence)
> > >   * @pad: pad number, as reported by the media API
> > >   * @format: media bus format (format code and frame size)
> > >   */
> > > 
> > > struct v4l2_subdev_format {
> > > 
> > >          __u32 which;
> > >          __u32 pad;
> > >          struct v4l2_mbus_framefmt format;
> > >          __u32 reserved[8];
> > > 
> > > };
> > > ---8<---
> > > 
> > > include/linux/v4l2-mediabus.h:
> > > ---8<---
> > > /**
> > > 
> > >   * struct v4l2_mbus_framefmt - frame format on the media bus
> > >   * @width:      frame width
> > >   * @height:     frame height
> > >   * @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)
> > >   */
> > > 
> > > struct v4l2_mbus_framefmt {
> > > 
> > >          __u32                   width;
> > >          __u32                   height;
> > >          __u32                   code;
> > >          __u32                   field;
> > >          __u32                   colorspace;
> > >          __u32                   reserved[7];
> > > 
> > > };
> > > ---8<---
> > > 
> > > Offering a lower level interface for sensors which allows better
> > > control of them from the user space involves providing the link
> > > frequency to the user space. While the link frequency will be a
> > > control, together with the bus type and number of lanes (on serial
> > > links), this will define the pixel clock.
> > > 
> > > <URL:http://www.spinics.net/lists/linux-media/msg36492.html>
> > > 
> > > After adding pixel clock to v4l2_mbus_framefmt there will be six
> > > reserved fields left, one of which will be further possibly consumed
> > > by maximum image size:
> > > 
> > > <URL:http://www.spinics.net/lists/linux-media/msg35949.html>
> > 
> > Yes, thanks for remembering about it. I have done some experiments with a
> > sensor producing JPEG data and I'd like to add '__u32 framesamples'
> > field to struct v4l2_mbus_framefmt, even though it solves only part of
> > the problem. I'm not sure when I'll be able to get this finished though.
> > I've just attached the initial patch now.
> > 
> > > Frame blanking (horizontal and vertical) and number of lanes might be
> > > needed in the struct as well in the future, bringing the reserved
> > > count down to two. I find this alarmingly low for a relatively new
> > > structure definition which will potentially have a few different uses
> > > in the future.
> > 
> > Sorry, could you explain why we need to put the blanking information in
> > struct v4l2_mbus_framefmt ? I thought it had been initially agreed that
> > the control framework will be used for this.
> 
> Configuration of blanking will be implemented as controls, yes.
> 
> Bandwidth calculation in the ISP driver may well need to know more detailed
> information than just the maximum pixel rate. Averge rate over certain
> period may also be important.
> 
> For example, take a sensor which is able to produce pixel rate of 200 Mp/s.
> In the OMAP 3 ISP only the CSI2 block will be able to process pixels at
> such rate. The ISP driver needs this information to be able to decide
> whether it's safe to start streaming or not.
> 
> Higher momentary pixel rates are still possible as there are buffers
> between some of the blocks. When using downscaling on sensors this gets
> more tricky. There may be bursts of data which may overflow these buffers
> since the sensors do not output data at amortised rate. Information on the
> sensor (bursts) and size of the buffers is at least required to assess
> this question.
> 
> I have a vague feeling we may need some of this data as part of the
> v4l2_mbus_framefmt before we have a solution.

Do we really need to make all this (including the proposed framesamples field) 
part of v4l2_mbus_framefmt ? My understanding is that the information needs to 
be propagated down the pipeline to verify pipeline validity at streamon time 
and to configure blocks down in the chain. That's an in-kernel requirement, 
wouldn't it be better to use an in-kernel API for that instead of requiring 
userspace to do the job ?

> > > The another issue is that the size of the v4l2_subdev_format struct is
> > > not aligned to a power of two. Instead of the intended 32 u32's, the
> > > size is actually 22 u32's.
> > 
> > hmm, is this really an issue ? What is advantage of having the structure
> > size being the power of 2 ? Isn't multiple of 4 just enough ?
> 
> A power of two has been considered a good practice. It's also how kmalloc
> will allocate memory for the duration of the ioctl call. Typical sizes can
> be found in /proc/slabinfo. For small sizes also some non-power of two
> sizes appear available, at least on my machines.
> 
> I'm not sure about the allocation in user space.
> 
> > > The interface is present in the 3.0 and marked experimental. My
> > > proposal is to add reserved fields to v4l2_mbus_framefmt to extend its
> > > size up to 32 u32's. I understand there are already few which use the
> > > interface right now and thus this change must be done now or left
> > > as-is forever.
> > 
> > hmm, I feel a bit uncomfortable with increasing size of data structure
> > which is quite widely used, not only in sensors, also in TV capture
> > cards, tuners, etc. So far struct v4l2_mbus_framefmt was quite generic.
> > IMHO it might be good to try to avoid extending it widely with
> > properties specific to single subsystem.
> 
> This is why I wanted to bring this up now rather than later. Of course I
> should have realised these issues before we had .39. ;)
> 
> I agree the pixel rate management should be implemented in a way which is
> not specific to sensors.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[Sylwester Nawrocki]

Hi Laurent,

On 09/08/2011 12:21 PM, Laurent Pinchart wrote:
> On Tuesday 06 September 2011 23:07:43 Sakari Ailus wrote:
>> On Tue, Sep 06, 2011 at 09:10:17PM +0200, Sylwester Nawrocki wrote:
>>> On 09/05/2011 05:55 PM, Sakari Ailus wrote:
>>>> Hi all,
>>>>
>>>> I recently came across a few issues in the definitions of
>>>> v4l2_subdev_format and v4l2_mbus_framefmt when I was working on sensor
>>>> control that I wanted to bring up here. The appropriate structure
>>>> right now look like this:
>>>>
>>>> include/linux/v4l2-subdev.h:
>>>> ---8<---
>>>> /**
>>>>
>>>>    * struct v4l2_subdev_format - Pad-level media bus format
>>>>    * @which: format type (from enum v4l2_subdev_format_whence)
>>>>    * @pad: pad number, as reported by the media API
>>>>    * @format: media bus format (format code and frame size)
>>>>    */
>>>>
>>>> struct v4l2_subdev_format {
>>>>
>>>>           __u32 which;
>>>>           __u32 pad;
>>>>           struct v4l2_mbus_framefmt format;
>>>>           __u32 reserved[8];
>>>>
>>>> };
>>>> ---8<---
>>>>
>>>> include/linux/v4l2-mediabus.h:
>>>> ---8<---
>>>> /**
>>>>
>>>>    * struct v4l2_mbus_framefmt - frame format on the media bus
>>>>    * @width:      frame width
>>>>    * @height:     frame height
>>>>    * @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)
>>>>    */
>>>>
>>>> struct v4l2_mbus_framefmt {
>>>>
>>>>           __u32                   width;
>>>>           __u32                   height;
>>>>           __u32                   code;
>>>>           __u32                   field;
>>>>           __u32                   colorspace;
>>>>           __u32                   reserved[7];
>>>>
>>>> };
>>>> ---8<---
>>>>
>>>> Offering a lower level interface for sensors which allows better
>>>> control of them from the user space involves providing the link
>>>> frequency to the user space. While the link frequency will be a
>>>> control, together with the bus type and number of lanes (on serial
>>>> links), this will define the pixel clock.
>>>>
>>>> <URL:http://www.spinics.net/lists/linux-media/msg36492.html>
>>>>
>>>> After adding pixel clock to v4l2_mbus_framefmt there will be six
>>>> reserved fields left, one of which will be further possibly consumed
>>>> by maximum image size:
>>>>
>>>> <URL:http://www.spinics.net/lists/linux-media/msg35949.html>
>>>
>>> Yes, thanks for remembering about it. I have done some experiments with a
>>> sensor producing JPEG data and I'd like to add '__u32 framesamples'
>>> field to struct v4l2_mbus_framefmt, even though it solves only part of
>>> the problem. I'm not sure when I'll be able to get this finished though.
>>> I've just attached the initial patch now.
>>>
>>>> Frame blanking (horizontal and vertical) and number of lanes might be
>>>> needed in the struct as well in the future, bringing the reserved
>>>> count down to two. I find this alarmingly low for a relatively new
>>>> structure definition which will potentially have a few different uses
>>>> in the future.
>>>
>>> Sorry, could you explain why we need to put the blanking information in
>>> struct v4l2_mbus_framefmt ? I thought it had been initially agreed that
>>> the control framework will be used for this.
>>
>> Configuration of blanking will be implemented as controls, yes.
>>
>> Bandwidth calculation in the ISP driver may well need to know more detailed
>> information than just the maximum pixel rate. Averge rate over certain
>> period may also be important.
>>
>> For example, take a sensor which is able to produce pixel rate of 200 Mp/s.
>> In the OMAP 3 ISP only the CSI2 block will be able to process pixels at
>> such rate. The ISP driver needs this information to be able to decide
>> whether it's safe to start streaming or not.
>>
>> Higher momentary pixel rates are still possible as there are buffers
>> between some of the blocks. When using downscaling on sensors this gets
>> more tricky. There may be bursts of data which may overflow these buffers
>> since the sensors do not output data at amortised rate. Information on the
>> sensor (bursts) and size of the buffers is at least required to assess
>> this question.
>>
>> I have a vague feeling we may need some of this data as part of the
>> v4l2_mbus_framefmt before we have a solution.
> 
> Do we really need to make all this (including the proposed framesamples field)
> part of v4l2_mbus_framefmt ? My understanding is that the information needs to
> be propagated down the pipeline to verify pipeline validity at streamon time
> and to configure blocks down in the chain. That's an in-kernel requirement,
> wouldn't it be better to use an in-kernel API for that instead of requiring
> userspace to do the job ?

I'll hold on for a moment on commenting the handling of blanking information
and the pixel clock in user space, but as far as memory buffer size negotiation
between drivers is concerned it always felt more appropriate to me to handle
such things in the kernel and isolate that from user space.

Actually we need to retrieve the size of the buffer during allocation time
in the host driver. So even if we have added maximum buffer size information
to struct v4l2_mbus_framefmt the format would have to be queried internally 
from a sensor subdev.

I can't really think of any usage of the v4l2_mbus_framefmt::framesamples
field in user space ATM. The MIPI CSI receiver drivers which transfer data 
directly to memory will, AFAIU, always expose a video node, so those subdevs
could possibly negotiate the buffer size in kernel with sensor subdev directly.

--
Regards,
Sylwester

Re: [RFC] Reserved fields in v4l2_mbus_framefmt, v4l2_subdev_format alignment

[Laurent Pinchart]

Hi Sylwester,

On Friday 09 September 2011 19:37:55 Sylwester Nawrocki wrote:
> On 09/08/2011 12:21 PM, Laurent Pinchart wrote:
> > On Tuesday 06 September 2011 23:07:43 Sakari Ailus wrote:
> >> On Tue, Sep 06, 2011 at 09:10:17PM +0200, Sylwester Nawrocki wrote:
> >>> On 09/05/2011 05:55 PM, Sakari Ailus wrote:
> >>>> Hi all,
> >>>> 
> >>>> I recently came across a few issues in the definitions of
> >>>> v4l2_subdev_format and v4l2_mbus_framefmt when I was working on sensor
> >>>> control that I wanted to bring up here. The appropriate structure
> >>>> right now look like this:
> >>>> 
> >>>> include/linux/v4l2-subdev.h:
> >>>> ---8<---
> >>>> /**
> >>>> 
> >>>>    * struct v4l2_subdev_format - Pad-level media bus format
> >>>>    * @which: format type (from enum v4l2_subdev_format_whence)
> >>>>    * @pad: pad number, as reported by the media API
> >>>>    * @format: media bus format (format code and frame size)
> >>>>    */
> >>>> 
> >>>> struct v4l2_subdev_format {
> >>>> 
> >>>>           __u32 which;
> >>>>           __u32 pad;
> >>>>           struct v4l2_mbus_framefmt format;
> >>>>           __u32 reserved[8];
> >>>> 
> >>>> };
> >>>> ---8<---
> >>>> 
> >>>> include/linux/v4l2-mediabus.h:
> >>>> ---8<---
> >>>> /**
> >>>> 
> >>>>    * struct v4l2_mbus_framefmt - frame format on the media bus
> >>>>    * @width:      frame width
> >>>>    * @height:     frame height
> >>>>    * @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)
> >>>>    */
> >>>> 
> >>>> struct v4l2_mbus_framefmt {
> >>>> 
> >>>>           __u32                   width;
> >>>>           __u32                   height;
> >>>>           __u32                   code;
> >>>>           __u32                   field;
> >>>>           __u32                   colorspace;
> >>>>           __u32                   reserved[7];
> >>>> 
> >>>> };
> >>>> ---8<---
> >>>> 
> >>>> Offering a lower level interface for sensors which allows better
> >>>> control of them from the user space involves providing the link
> >>>> frequency to the user space. While the link frequency will be a
> >>>> control, together with the bus type and number of lanes (on serial
> >>>> links), this will define the pixel clock.
> >>>> 
> >>>> <URL:http://www.spinics.net/lists/linux-media/msg36492.html>
> >>>> 
> >>>> After adding pixel clock to v4l2_mbus_framefmt there will be six
> >>>> reserved fields left, one of which will be further possibly consumed
> >>>> by maximum image size:
> >>>> 
> >>>> <URL:http://www.spinics.net/lists/linux-media/msg35949.html>
> >>> 
> >>> Yes, thanks for remembering about it. I have done some experiments with
> >>> a sensor producing JPEG data and I'd like to add '__u32 framesamples'
> >>> field to struct v4l2_mbus_framefmt, even though it solves only part of
> >>> the problem. I'm not sure when I'll be able to get this finished
> >>> though. I've just attached the initial patch now.
> >>> 
> >>>> Frame blanking (horizontal and vertical) and number of lanes might be
> >>>> needed in the struct as well in the future, bringing the reserved
> >>>> count down to two. I find this alarmingly low for a relatively new
> >>>> structure definition which will potentially have a few different uses
> >>>> in the future.
> >>> 
> >>> Sorry, could you explain why we need to put the blanking information in
> >>> struct v4l2_mbus_framefmt ? I thought it had been initially agreed that
> >>> the control framework will be used for this.
> >> 
> >> Configuration of blanking will be implemented as controls, yes.
> >> 
> >> Bandwidth calculation in the ISP driver may well need to know more
> >> detailed information than just the maximum pixel rate. Averge rate over
> >> certain period may also be important.
> >> 
> >> For example, take a sensor which is able to produce pixel rate of 200
> >> Mp/s. In the OMAP 3 ISP only the CSI2 block will be able to process
> >> pixels at such rate. The ISP driver needs this information to be able
> >> to decide whether it's safe to start streaming or not.
> >> 
> >> Higher momentary pixel rates are still possible as there are buffers
> >> between some of the blocks. When using downscaling on sensors this gets
> >> more tricky. There may be bursts of data which may overflow these
> >> buffers since the sensors do not output data at amortised rate.
> >> Information on the sensor (bursts) and size of the buffers is at least
> >> required to assess this question.
> >> 
> >> I have a vague feeling we may need some of this data as part of the
> >> v4l2_mbus_framefmt before we have a solution.
> > 
> > Do we really need to make all this (including the proposed framesamples
> > field) part of v4l2_mbus_framefmt ? My understanding is that the
> > information needs to be propagated down the pipeline to verify pipeline
> > validity at streamon time and to configure blocks down in the chain.
> > That's an in-kernel requirement, wouldn't it be better to use an
> > in-kernel API for that instead of requiring userspace to do the job ?
> 
> I'll hold on for a moment on commenting the handling of blanking
> information and the pixel clock in user space, but as far as memory buffer
> size negotiation between drivers is concerned it always felt more
> appropriate to me to handle such things in the kernel and isolate that
> from user space.
> 
> Actually we need to retrieve the size of the buffer during allocation time
> in the host driver. So even if we have added maximum buffer size
> information to struct v4l2_mbus_framefmt the format would have to be
> queried internally from a sensor subdev.
> 
> I can't really think of any usage of the v4l2_mbus_framefmt::framesamples
> field in user space ATM. The MIPI CSI receiver drivers which transfer data
> directly to memory will, AFAIU, always expose a video node, so those
> subdevs could possibly negotiate the buffer size in kernel with sensor
> subdev directly.

This has my preference. We can add a field to an in-kernel structure for this, 
or add a new subdev operation. As this will be a pure in-kernel API it will be 
easier to change it later if needed.

-- 
Regards,

Laurent Pinchart