[RFC] Add query/get/set matrix ioctls

[RFC] Add query/get/set matrix ioctls

[Hans Verkuil]

Hi all,

When working on the Motion Detection API, I realized that my proposal for
two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
case.

The Motion Detection RFC can be found here:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html

What I was really attempting to do was to create an API to pass a matrix
to the hardware.

This is also related to a long-standing request to add support for
arrays to the control API. Adding such support to the control API is in my
opinion a very bad idea since the control API is meant to provide all
meta data necessary in order to create e.g. control panels. Adding array
support to the control API would make that very difficult, particularly
with respect to GUI design.

So instead this proposal creates a new API to query, get and set matrices:


/* Define to which motion detection region each element belongs.
 * Each element is a __u8. */
#define V4L2_MATRIX_TYPE_MD_REGION     (1)
/* Define the motion detection threshold for each element.
 * Each element is a __u16. */
#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)

/**
 * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
 * @type:       matrix type
 * @index:      matrix index of the given type
 * @columns:    number of columns in the matrix
 * @rows:       number of rows in the matrix
 * @elem_min:   minimum matrix element value
 * @elem_max:   maximum matrix element value
 * @elem_size:  size in bytes each matrix element
 * @reserved:   future extensions, applications and drivers must zero this.
 */
struct v4l2_query_matrix {
        __u32 type;
        __u32 index;
        __u32 columns;
        __u32 rows;
        __s64 elem_min;
        __s64 elem_max;
        __u32 elem_size;
        __u32 reserved[23];
} __attribute__ ((packed));

/**
 * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
 * @type:       matrix type
 * @index:      matrix index of the given type
 * @rect:       which part of the matrix to get/set
 * @matrix:     pointer to the matrix of size (in bytes):
 *              elem_size * rect.width * rect.height
 * @reserved:   future extensions, applications and drivers must zero this.
 */
struct v4l2_matrix {
        __u32 type;
        __u32 index;
        struct v4l2_rect rect;
        void __user *matrix;
        __u32 reserved[12];
} __attribute__ ((packed));


/* Experimental, these three ioctls may change over the next couple of kernel
   versions. */
#define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
#define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
#define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)


Each matrix has a type (which describes the meaning of the matrix) and an
index (allowing for multiple matrices of the same type).

QUERY_MATRIX will return the number of columns and rows in the full matrix,
the size (in bytes) of each element and the minimum and maximum value of
each element. Some matrix types may have non-integer elements, in which case
the minimum and maximum values are ignored.

With S_MATRIX and G_MATRIX you can get/set a (part of a) matrix. The rect
struct will give the part of the matrix that you want to set or retrieve, and
the matrix pointer points to the matrix data.

Currently only two matrix types are defined, see the motion detection RFC for
details.

This approach is basically the same as proposed in the motion detection RFC,
but it is much more general.

Discussion points:

1) I'm using elem_size to allow for any element size. An alternative would be to
define specific element types (e.g. U8, S8, U16, S16, etc.), but I feel that
that is overkill. It is easier to associate each matrix type with a specific
element type in the documentation for each type. For allocation purposes it
is more useful to know the element size than the element type. But perhaps
elem_size can be dropped altogether, or, alternatively, both an elem_size and
elem_type should be defined.

2) Per-driver matrix types are probably necessary as well: for example while
colorspace conversion matrices are in principle generic, in practice the
precise format of the elements is hardware specific. This isn't a problem
as long as it is a well-defined private matrix type.

Comments? Questions?

	Hans

Re: [RFC] Add query/get/set matrix ioctls

[Sylwester Nawrocki]

Hi Hans,

On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> Hi all,
>
> When working on the Motion Detection API, I realized that my proposal for
> two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> case.
>
> The Motion Detection RFC can be found here:
>
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
>
> What I was really attempting to do was to create an API to pass a matrix
> to the hardware.
>
> This is also related to a long-standing request to add support for
> arrays to the control API. Adding such support to the control API is in my
> opinion a very bad idea since the control API is meant to provide all
> meta data necessary in order to create e.g. control panels. Adding array
> support to the control API would make that very difficult, particularly
> with respect to GUI design.
>
> So instead this proposal creates a new API to query, get and set matrices:

This looks very interesting, one use case that comes immediately to mind is
configuring the quantization/Huffman tables in a hardware JPEG codec. 
The only
thing left would have probably been setting up the comment segments, 
consisting
of arbitrary byte strings.

This is even more nice than your previous proposal ;) Quite generic - but
I was wondering, what if we went one step further and defined QUERY/GET/
SET_PROPERTY ioctls, where the type (matrix or anything else) would be
also configurable ? :-) Just brainstorming, if memory serves me well few
people suggested something like this in the past.
So for the starters we could have matrix type and carefully be adding in
the future anything what's needed ?

> /* Define to which motion detection region each element belongs.
>   * Each element is a __u8. */
> #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> /* Define the motion detection threshold for each element.
>   * Each element is a __u16. */
> #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
>
> /**
>   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
>   * @type:       matrix type
>   * @index:      matrix index of the given type
>   * @columns:    number of columns in the matrix
>   * @rows:       number of rows in the matrix
>   * @elem_min:   minimum matrix element value
>   * @elem_max:   maximum matrix element value
>   * @elem_size:  size in bytes each matrix element
>   * @reserved:   future extensions, applications and drivers must zero this.
>   */
> struct v4l2_query_matrix {
>          __u32 type;
>          __u32 index;
>          __u32 columns;
>          __u32 rows;
>          __s64 elem_min;
>          __s64 elem_max;
>          __u32 elem_size;
>          __u32 reserved[23];
> } __attribute__ ((packed));
>
> /**
>   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
>   * @type:       matrix type
>   * @index:      matrix index of the given type
>   * @rect:       which part of the matrix to get/set
>   * @matrix:     pointer to the matrix of size (in bytes):
>   *              elem_size * rect.width * rect.height
>   * @reserved:   future extensions, applications and drivers must zero this.
>   */
> struct v4l2_matrix {
>          __u32 type;
>          __u32 index;
>          struct v4l2_rect rect;
>          void __user *matrix;
>          __u32 reserved[12];
> } __attribute__ ((packed));
>
>
> /* Experimental, these three ioctls may change over the next couple of kernel
>     versions. */
> #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
>
>
> Each matrix has a type (which describes the meaning of the matrix) and an
> index (allowing for multiple matrices of the same type).

I'm just wondering how this could be used to specify coefficients 
associated with
selection rectangles for auto focus ? This API would be used only for 
passing a
set of coefficients, assuming the individual rectangles are passed 
somehow else ?

> QUERY_MATRIX will return the number of columns and rows in the full matrix,
> the size (in bytes) of each element and the minimum and maximum value of
> each element. Some matrix types may have non-integer elements, in which case
> the minimum and maximum values are ignored.

I guess min/max could be made unions with a possibility to define other 
types ?
Why should we limit ourselves to integers only ? However anything 
(sensible?)
that comes to my mind ATM are rational numbers only.

> With S_MATRIX and G_MATRIX you can get/set a (part of a) matrix. The rect
> struct will give the part of the matrix that you want to set or retrieve, and
> the matrix pointer points to the matrix data.
>
> Currently only two matrix types are defined, see the motion detection RFC for
> details.
>
> This approach is basically the same as proposed in the motion detection RFC,
> but it is much more general.
>
> Discussion points:
>
> 1) I'm using elem_size to allow for any element size. An alternative would be to
> define specific element types (e.g. U8, S8, U16, S16, etc.), but I feel that
> that is overkill. It is easier to associate each matrix type with a specific
> element type in the documentation for each type. For allocation purposes it
> is more useful to know the element size than the element type. But perhaps
> elem_size can be dropped altogether, or, alternatively, both an elem_size and
> elem_type should be defined.

IMHO it makes sense to keep elem_size, for allocation purposes etc. And the
element type could be probably derived from the matrix type ?

> 2) Per-driver matrix types are probably necessary as well: for example while
> colorspace conversion matrices are in principle generic, in practice the
> precise format of the elements is hardware specific. This isn't a problem
> as long as it is a well-defined private matrix type.

Sounds reasonable.


Regards,
Sylwester

Re: [RFC] Add query/get/set matrix ioctls

[Hans Verkuil]

On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> Hi Hans,
> 
> On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > Hi all,
> >
> > When working on the Motion Detection API, I realized that my proposal for
> > two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> > case.
> >
> > The Motion Detection RFC can be found here:
> >
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> >
> > What I was really attempting to do was to create an API to pass a matrix
> > to the hardware.
> >
> > This is also related to a long-standing request to add support for
> > arrays to the control API. Adding such support to the control API is in my
> > opinion a very bad idea since the control API is meant to provide all
> > meta data necessary in order to create e.g. control panels. Adding array
> > support to the control API would make that very difficult, particularly
> > with respect to GUI design.
> >
> > So instead this proposal creates a new API to query, get and set matrices:
> 
> This looks very interesting, one use case that comes immediately to mind is
> configuring the quantization/Huffman tables in a hardware JPEG codec. 
> The only
> thing left would have probably been setting up the comment segments, 
> consisting
> of arbitrary byte strings.

Actually, I realized that that can be handled as well since those segments
are 1-dimensional matrices of unsigned chars.

> 
> This is even more nice than your previous proposal ;) Quite generic - but
> I was wondering, what if we went one step further and defined QUERY/GET/
> SET_PROPERTY ioctls, where the type (matrix or anything else) would be
> also configurable ? :-) Just brainstorming, if memory serves me well few
> people suggested something like this in the past.

The problem with that is that you basically create a meta-ioctl. Why not
just create an ioctl for whatever you want to do? After all, an ioctl is
basically a type (the command number) and a pointer. And with a property
ioctl you really just wrap that into another ioctl.

> So for the starters we could have matrix type and carefully be adding in
> the future anything what's needed ?
> 
> > /* Define to which motion detection region each element belongs.
> >   * Each element is a __u8. */
> > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > /* Define the motion detection threshold for each element.
> >   * Each element is a __u16. */
> > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> >
> > /**
> >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> >   * @type:       matrix type
> >   * @index:      matrix index of the given type
> >   * @columns:    number of columns in the matrix
> >   * @rows:       number of rows in the matrix
> >   * @elem_min:   minimum matrix element value
> >   * @elem_max:   maximum matrix element value
> >   * @elem_size:  size in bytes each matrix element
> >   * @reserved:   future extensions, applications and drivers must zero this.
> >   */
> > struct v4l2_query_matrix {
> >          __u32 type;
> >          __u32 index;
> >          __u32 columns;
> >          __u32 rows;
> >          __s64 elem_min;
> >          __s64 elem_max;
> >          __u32 elem_size;
> >          __u32 reserved[23];
> > } __attribute__ ((packed));
> >
> > /**
> >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> >   * @type:       matrix type
> >   * @index:      matrix index of the given type
> >   * @rect:       which part of the matrix to get/set
> >   * @matrix:     pointer to the matrix of size (in bytes):
> >   *              elem_size * rect.width * rect.height
> >   * @reserved:   future extensions, applications and drivers must zero this.
> >   */
> > struct v4l2_matrix {
> >          __u32 type;
> >          __u32 index;
> >          struct v4l2_rect rect;
> >          void __user *matrix;
> >          __u32 reserved[12];
> > } __attribute__ ((packed));
> >
> >
> > /* Experimental, these three ioctls may change over the next couple of kernel
> >     versions. */
> > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> >
> >
> > Each matrix has a type (which describes the meaning of the matrix) and an
> > index (allowing for multiple matrices of the same type).
> 
> I'm just wondering how this could be used to specify coefficients 
> associated with
> selection rectangles for auto focus ?

I've been thinking about this. The problem is that sometimes you want to
associate a matrix with some other object (a selection rectangle, a video
input, perhaps a video buffer, etc.). A simple index may not be enough. So
how about replacing the index field with a union:

	union {
		__u32 reserved[4];
	} ref;

The precise contents of the union will be defined by the matrix type. Apps
should probably zero ref to allow adding additional 'reference' fields in
the future. So to refer to a selection rectangle you would add:

	struct {
		__u32 type;
		__u32 target;
	};

to the union.

> This API would be used only for 
> passing a
> set of coefficients, assuming the individual rectangles are passed 
> somehow else ?

Yes.

> 
> > QUERY_MATRIX will return the number of columns and rows in the full matrix,
> > the size (in bytes) of each element and the minimum and maximum value of
> > each element. Some matrix types may have non-integer elements, in which case
> > the minimum and maximum values are ignored.
> 
> I guess min/max could be made unions with a possibility to define other 
> types ?

How about this?

	union {
		__s64 val;
		__u64 uval;
		__u32 reserved[4];
	} min;

Ditto for max.

> Why should we limit ourselves to integers only ? However anything 
> (sensible?)
> that comes to my mind ATM are rational numbers only.

Fixed point is another.

> 
> > With S_MATRIX and G_MATRIX you can get/set a (part of a) matrix. The rect
> > struct will give the part of the matrix that you want to set or retrieve, and
> > the matrix pointer points to the matrix data.
> >
> > Currently only two matrix types are defined, see the motion detection RFC for
> > details.
> >
> > This approach is basically the same as proposed in the motion detection RFC,
> > but it is much more general.
> >
> > Discussion points:
> >
> > 1) I'm using elem_size to allow for any element size. An alternative would be to
> > define specific element types (e.g. U8, S8, U16, S16, etc.), but I feel that
> > that is overkill. It is easier to associate each matrix type with a specific
> > element type in the documentation for each type. For allocation purposes it
> > is more useful to know the element size than the element type. But perhaps
> > elem_size can be dropped altogether, or, alternatively, both an elem_size and
> > elem_type should be defined.
> 
> IMHO it makes sense to keep elem_size, for allocation purposes etc. And the
> element type could be probably derived from the matrix type ?

Yes, each matrix type will document what element type it uses.

> 
> > 2) Per-driver matrix types are probably necessary as well: for example while
> > colorspace conversion matrices are in principle generic, in practice the
> > precise format of the elements is hardware specific. This isn't a problem
> > as long as it is a well-defined private matrix type.
> 
> Sounds reasonable.

Regards,

	Hans

Re: [RFC] Add query/get/set matrix ioctls

[Sakari Ailus]

Hi Hans,

Thanks for the RFC!

On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> > Hi Hans,
> > 
> > On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > > Hi all,
> > >
> > > When working on the Motion Detection API, I realized that my proposal for
> > > two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> > > case.
> > >
> > > The Motion Detection RFC can be found here:
> > >
> > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > >
> > > What I was really attempting to do was to create an API to pass a matrix
> > > to the hardware.
> > >
> > > This is also related to a long-standing request to add support for
> > > arrays to the control API. Adding such support to the control API is in my
> > > opinion a very bad idea since the control API is meant to provide all
> > > meta data necessary in order to create e.g. control panels. Adding array
> > > support to the control API would make that very difficult, particularly
> > > with respect to GUI design.
> > >
> > > So instead this proposal creates a new API to query, get and set matrices:
> > 
> > This looks very interesting, one use case that comes immediately to mind is
> > configuring the quantization/Huffman tables in a hardware JPEG codec. 
> > The only
> > thing left would have probably been setting up the comment segments, 
> > consisting
> > of arbitrary byte strings.
> 
> Actually, I realized that that can be handled as well since those segments
> are 1-dimensional matrices of unsigned chars.
> 
> > 
> > This is even more nice than your previous proposal ;) Quite generic - but
> > I was wondering, what if we went one step further and defined QUERY/GET/
> > SET_PROPERTY ioctls, where the type (matrix or anything else) would be
> > also configurable ? :-) Just brainstorming, if memory serves me well few
> > people suggested something like this in the past.

Interesting idea. This approach could be used on the Media controller
interface as well.

> The problem with that is that you basically create a meta-ioctl. Why not
> just create an ioctl for whatever you want to do? After all, an ioctl is
> basically a type (the command number) and a pointer. And with a property
> ioctl you really just wrap that into another ioctl.

Is this a problem?

One of the benefits is that you could send multiple IOCTL commands to the
device at one go (if the interface is designed as such, and I think it
should be).

It would be easier to model extensions to the V4L2 API using that kind of
model; currently we have a bunch of IOCTLs that certainly do show the age of
the API. With the property API, everything will be... properties you can
access using set and get operations, and probably enum as well.

I think this is a logical extension of the V4L2 control API.

I have to admit designing that kind of an API isn't trivial either: more
focus will be needed on what the properties are attached to: is that a
device or a subdev pad, for instance. This way it might be possible to
address such things at generic level rather than at the level of a single
IOCTL as we're doing (or rather avoid doing) right now.

> > So for the starters we could have matrix type and carefully be adding in
> > the future anything what's needed ?
> > 
> > > /* Define to which motion detection region each element belongs.
> > >   * Each element is a __u8. */
> > > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > > /* Define the motion detection threshold for each element.
> > >   * Each element is a __u16. */
> > > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > >
> > > /**
> > >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > >   * @type:       matrix type
> > >   * @index:      matrix index of the given type
> > >   * @columns:    number of columns in the matrix
> > >   * @rows:       number of rows in the matrix
> > >   * @elem_min:   minimum matrix element value
> > >   * @elem_max:   maximum matrix element value
> > >   * @elem_size:  size in bytes each matrix element
> > >   * @reserved:   future extensions, applications and drivers must zero this.
> > >   */
> > > struct v4l2_query_matrix {
> > >          __u32 type;
> > >          __u32 index;
> > >          __u32 columns;
> > >          __u32 rows;
> > >          __s64 elem_min;
> > >          __s64 elem_max;
> > >          __u32 elem_size;
> > >          __u32 reserved[23];
> > > } __attribute__ ((packed));
> > >
> > > /**
> > >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> > >   * @type:       matrix type
> > >   * @index:      matrix index of the given type
> > >   * @rect:       which part of the matrix to get/set
> > >   * @matrix:     pointer to the matrix of size (in bytes):
> > >   *              elem_size * rect.width * rect.height
> > >   * @reserved:   future extensions, applications and drivers must zero this.
> > >   */
> > > struct v4l2_matrix {
> > >          __u32 type;
> > >          __u32 index;
> > >          struct v4l2_rect rect;
> > >          void __user *matrix;
> > >          __u32 reserved[12];
> > > } __attribute__ ((packed));
> > >
> > >
> > > /* Experimental, these three ioctls may change over the next couple of kernel
> > >     versions. */
> > > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> > > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> > >
> > >
> > > Each matrix has a type (which describes the meaning of the matrix) and an
> > > index (allowing for multiple matrices of the same type).
> > 
> > I'm just wondering how this could be used to specify coefficients 
> > associated with
> > selection rectangles for auto focus ?
> 
> I've been thinking about this. The problem is that sometimes you want to
> associate a matrix with some other object (a selection rectangle, a video
> input, perhaps a video buffer, etc.). A simple index may not be enough. So
> how about replacing the index field with a union:
> 
> 	union {
> 		__u32 reserved[4];
> 	} ref;

...which is a proof of what I wrote above. :-)

-- 
Kind regards,

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

Re: [RFC] Add query/get/set matrix ioctls

[Hans Verkuil]

On Wed 12 June 2013 14:26:27 Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the RFC!
> 
> On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> > > Hi Hans,
> > > 
> > > On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > > > Hi all,
> > > >
> > > > When working on the Motion Detection API, I realized that my proposal for
> > > > two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> > > > case.
> > > >
> > > > The Motion Detection RFC can be found here:
> > > >
> > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > > >
> > > > What I was really attempting to do was to create an API to pass a matrix
> > > > to the hardware.
> > > >
> > > > This is also related to a long-standing request to add support for
> > > > arrays to the control API. Adding such support to the control API is in my
> > > > opinion a very bad idea since the control API is meant to provide all
> > > > meta data necessary in order to create e.g. control panels. Adding array
> > > > support to the control API would make that very difficult, particularly
> > > > with respect to GUI design.
> > > >
> > > > So instead this proposal creates a new API to query, get and set matrices:
> > > 
> > > This looks very interesting, one use case that comes immediately to mind is
> > > configuring the quantization/Huffman tables in a hardware JPEG codec. 
> > > The only
> > > thing left would have probably been setting up the comment segments, 
> > > consisting
> > > of arbitrary byte strings.
> > 
> > Actually, I realized that that can be handled as well since those segments
> > are 1-dimensional matrices of unsigned chars.
> > 
> > > 
> > > This is even more nice than your previous proposal ;) Quite generic - but
> > > I was wondering, what if we went one step further and defined QUERY/GET/
> > > SET_PROPERTY ioctls, where the type (matrix or anything else) would be
> > > also configurable ? :-) Just brainstorming, if memory serves me well few
> > > people suggested something like this in the past.
> 
> Interesting idea. This approach could be used on the Media controller
> interface as well.

What is needed for the MC (if memory serves) is something simple to list what
effectively are capabilities of entities. Basically just a list of integers.
That's quite different from this highly generic proposal.
 
> > The problem with that is that you basically create a meta-ioctl. Why not
> > just create an ioctl for whatever you want to do? After all, an ioctl is
> > basically a type (the command number) and a pointer. And with a property
> > ioctl you really just wrap that into another ioctl.
> 
> Is this a problem?

I think so, yes. It seems to me that this just adds an unnecessary indirection
that everyone (userspace and kernelspace) has to cope with.

I don't see any advantage of going in this direction.

> One of the benefits is that you could send multiple IOCTL commands to the
> device at one go (if the interface is designed as such, and I think it
> should be).

There are other ways this can be done (we discussed that in the past) without
introducing complex new ioctls. And the reality is that this doesn't really
help you much: the real complexity will be in handling such ioctl sets in a
driver.

> It would be easier to model extensions to the V4L2 API using that kind of
> model; currently we have a bunch of IOCTLs that certainly do show the age of
> the API. With the property API, everything will be... properties you can
> access using set and get operations, and probably enum as well.

It's easy to model extension today as well: just add a new ioctl. How is that
any different than adding a new property type, except instead of just filling
in one struct to pass with the ioctl, I now have to fill in two: one for the
property encapsulation struct, one for the actual payload struct. Yes, it
looks like you have just a few property ioctls, but the reality is that the
complexity has just been moved to the property side of things.

> 
> I think this is a logical extension of the V4L2 control API.
> 
> I have to admit designing that kind of an API isn't trivial either: more
> focus will be needed on what the properties are attached to: is that a
> device or a subdev pad, for instance. This way it might be possible to
> address such things at generic level rather than at the level of a single
> IOCTL as we're doing (or rather avoid doing) right now.

That's a problem that is unrelated to 'property' usage. Actually, that's
easier for 'non-property' ioctls since there you know how it is going to be
used, so there is no need to be generic.

BTW, I've been making the assumption that a property can hold any type of
data, not just 'simple' types. So it can hold a v4l2_format struct for example
(or something of similar complexity).

> 
> > > So for the starters we could have matrix type and carefully be adding in
> > > the future anything what's needed ?
> > > 
> > > > /* Define to which motion detection region each element belongs.
> > > >   * Each element is a __u8. */
> > > > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > > > /* Define the motion detection threshold for each element.
> > > >   * Each element is a __u16. */
> > > > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > > >
> > > > /**
> > > >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > > >   * @type:       matrix type
> > > >   * @index:      matrix index of the given type
> > > >   * @columns:    number of columns in the matrix
> > > >   * @rows:       number of rows in the matrix
> > > >   * @elem_min:   minimum matrix element value
> > > >   * @elem_max:   maximum matrix element value
> > > >   * @elem_size:  size in bytes each matrix element
> > > >   * @reserved:   future extensions, applications and drivers must zero this.
> > > >   */
> > > > struct v4l2_query_matrix {
> > > >          __u32 type;
> > > >          __u32 index;
> > > >          __u32 columns;
> > > >          __u32 rows;
> > > >          __s64 elem_min;
> > > >          __s64 elem_max;
> > > >          __u32 elem_size;
> > > >          __u32 reserved[23];
> > > > } __attribute__ ((packed));
> > > >
> > > > /**
> > > >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> > > >   * @type:       matrix type
> > > >   * @index:      matrix index of the given type
> > > >   * @rect:       which part of the matrix to get/set
> > > >   * @matrix:     pointer to the matrix of size (in bytes):
> > > >   *              elem_size * rect.width * rect.height
> > > >   * @reserved:   future extensions, applications and drivers must zero this.
> > > >   */
> > > > struct v4l2_matrix {
> > > >          __u32 type;
> > > >          __u32 index;
> > > >          struct v4l2_rect rect;
> > > >          void __user *matrix;
> > > >          __u32 reserved[12];
> > > > } __attribute__ ((packed));
> > > >
> > > >
> > > > /* Experimental, these three ioctls may change over the next couple of kernel
> > > >     versions. */
> > > > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> > > > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > > > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> > > >
> > > >
> > > > Each matrix has a type (which describes the meaning of the matrix) and an
> > > > index (allowing for multiple matrices of the same type).
> > > 
> > > I'm just wondering how this could be used to specify coefficients 
> > > associated with
> > > selection rectangles for auto focus ?
> > 
> > I've been thinking about this. The problem is that sometimes you want to
> > associate a matrix with some other object (a selection rectangle, a video
> > input, perhaps a video buffer, etc.). A simple index may not be enough. So
> > how about replacing the index field with a union:
> > 
> > 	union {
> > 		__u32 reserved[4];
> > 	} ref;
> 
> ...which is a proof of what I wrote above. :-)

Is it?

Regards,

	Hans

Re: [RFC] Add query/get/set matrix ioctls

[Sakari Ailus]

Hi Hans,

On Wed, Jun 12, 2013 at 02:57:21PM +0200, Hans Verkuil wrote:
> On Wed 12 June 2013 14:26:27 Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks for the RFC!
> > 
> > On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> > > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> > > > Hi Hans,
> > > > 
> > > > On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > > > > Hi all,
> > > > >
> > > > > When working on the Motion Detection API, I realized that my proposal for
> > > > > two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> > > > > case.
> > > > >
> > > > > The Motion Detection RFC can be found here:
> > > > >
> > > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > > > >
> > > > > What I was really attempting to do was to create an API to pass a matrix
> > > > > to the hardware.
> > > > >
> > > > > This is also related to a long-standing request to add support for
> > > > > arrays to the control API. Adding such support to the control API is in my
> > > > > opinion a very bad idea since the control API is meant to provide all
> > > > > meta data necessary in order to create e.g. control panels. Adding array
> > > > > support to the control API would make that very difficult, particularly
> > > > > with respect to GUI design.
> > > > >
> > > > > So instead this proposal creates a new API to query, get and set matrices:
> > > > 
> > > > This looks very interesting, one use case that comes immediately to mind is
> > > > configuring the quantization/Huffman tables in a hardware JPEG codec. 
> > > > The only
> > > > thing left would have probably been setting up the comment segments, 
> > > > consisting
> > > > of arbitrary byte strings.
> > > 
> > > Actually, I realized that that can be handled as well since those segments
> > > are 1-dimensional matrices of unsigned chars.
> > > 
> > > > 
> > > > This is even more nice than your previous proposal ;) Quite generic - but
> > > > I was wondering, what if we went one step further and defined QUERY/GET/
> > > > SET_PROPERTY ioctls, where the type (matrix or anything else) would be
> > > > also configurable ? :-) Just brainstorming, if memory serves me well few
> > > > people suggested something like this in the past.
> > 
> > Interesting idea. This approach could be used on the Media controller
> > interface as well.
> 
> What is needed for the MC (if memory serves) is something simple to list what
> effectively are capabilities of entities. Basically just a list of integers.
> That's quite different from this highly generic proposal.

I think I should have said "property API" instead. That's what I meant. This
could be prototyped and discussed. Which device nodes that API could
eventually use is another matter.

>From API design point of view it's somewhat odd that a device (especially
when it comes to embedded systems) is visible in the system as a large
number of device nodes. I know there are a huge number of reasons why it's
so currently but if I were to create a new API, that's one thing I would
correct.

But in the context of matrix IOCTLs, this begins to be off-topic.

> > > The problem with that is that you basically create a meta-ioctl. Why not
> > > just create an ioctl for whatever you want to do? After all, an ioctl is
> > > basically a type (the command number) and a pointer. And with a property
> > > ioctl you really just wrap that into another ioctl.
> > 
> > Is this a problem?
> 
> I think so, yes. It seems to me that this just adds an unnecessary indirection
> that everyone (userspace and kernelspace) has to cope with.
> 
> I don't see any advantage of going in this direction.

To some extent one could claim the controls API does exactly that: it
provides a generic way to access properties of certain kind. I like controls
and extended controls even more: the standard API makes many other things
easier in user space, including enumeration.

> > One of the benefits is that you could send multiple IOCTL commands to the
> > device at one go (if the interface is designed as such, and I think it
> > should be).
> 
> There are other ways this can be done (we discussed that in the past) without
> introducing complex new ioctls. And the reality is that this doesn't really

Could you refresh my memory a bit? I remember synchronisation of applying
configurations being discussed, and, well, most of the time that certainly
isn't an issue, but if you wish to ensure two configuration parameters on
different subdevs take effect at the same time, there's no fully generic way
to do that in the API: you have to rely on timing to some extent.

> help you much: the real complexity will be in handling such ioctl sets in a
> driver.

Very, very true. I still maintain there are cases where this a) could be
done and b) would be nice and useful. The configuration of different
omap3isp blocks, for instance, now passed to the driver using private ioctls.

> > It would be easier to model extensions to the V4L2 API using that kind of
> > model; currently we have a bunch of IOCTLs that certainly do show the age of
> > the API. With the property API, everything will be... properties you can
> > access using set and get operations, and probably enum as well.
> 
> It's easy to model extension today as well: just add a new ioctl. How is that
> any different than adding a new property type, except instead of just filling
> in one struct to pass with the ioctl, I now have to fill in two: one for the
> property encapsulation struct, one for the actual payload struct. Yes, it
> looks like you have just a few property ioctls, but the reality is that the
> complexity has just been moved to the property side of things.

I'm not claiming it'd magically fixed everything, but it'd be worth
prototyping. I wish I had time for that (and well, to do a bunch of other
pending things as well). :-P

> > 
> > I think this is a logical extension of the V4L2 control API.
> > 
> > I have to admit designing that kind of an API isn't trivial either: more
> > focus will be needed on what the properties are attached to: is that a
> > device or a subdev pad, for instance. This way it might be possible to
> > address such things at generic level rather than at the level of a single
> > IOCTL as we're doing (or rather avoid doing) right now.
> 
> That's a problem that is unrelated to 'property' usage. Actually, that's
> easier for 'non-property' ioctls since there you know how it is going to be
> used, so there is no need to be generic.
> 
> BTW, I've been making the assumption that a property can hold any type of
> data, not just 'simple' types. So it can hold a v4l2_format struct for example
> (or something of similar complexity).

I don't think properties should --- instead they should contain information
related to the fields of the struct. The structs combine together
information which is valid for a certain use case, and are unchangeable from
then on. There are many upsides from this but also downsides. The full
picture is not visible without at least prototyping a property-based API.

> > 
> > > > So for the starters we could have matrix type and carefully be adding in
> > > > the future anything what's needed ?
> > > > 
> > > > > /* Define to which motion detection region each element belongs.
> > > > >   * Each element is a __u8. */
> > > > > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > > > > /* Define the motion detection threshold for each element.
> > > > >   * Each element is a __u16. */
> > > > > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > > > >
> > > > > /**
> > > > >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > > > >   * @type:       matrix type
> > > > >   * @index:      matrix index of the given type
> > > > >   * @columns:    number of columns in the matrix
> > > > >   * @rows:       number of rows in the matrix
> > > > >   * @elem_min:   minimum matrix element value
> > > > >   * @elem_max:   maximum matrix element value
> > > > >   * @elem_size:  size in bytes each matrix element
> > > > >   * @reserved:   future extensions, applications and drivers must zero this.
> > > > >   */
> > > > > struct v4l2_query_matrix {
> > > > >          __u32 type;
> > > > >          __u32 index;
> > > > >          __u32 columns;
> > > > >          __u32 rows;
> > > > >          __s64 elem_min;
> > > > >          __s64 elem_max;
> > > > >          __u32 elem_size;
> > > > >          __u32 reserved[23];
> > > > > } __attribute__ ((packed));
> > > > >
> > > > > /**
> > > > >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> > > > >   * @type:       matrix type
> > > > >   * @index:      matrix index of the given type
> > > > >   * @rect:       which part of the matrix to get/set
> > > > >   * @matrix:     pointer to the matrix of size (in bytes):
> > > > >   *              elem_size * rect.width * rect.height
> > > > >   * @reserved:   future extensions, applications and drivers must zero this.
> > > > >   */
> > > > > struct v4l2_matrix {
> > > > >          __u32 type;
> > > > >          __u32 index;
> > > > >          struct v4l2_rect rect;
> > > > >          void __user *matrix;
> > > > >          __u32 reserved[12];
> > > > > } __attribute__ ((packed));
> > > > >
> > > > >
> > > > > /* Experimental, these three ioctls may change over the next couple of kernel
> > > > >     versions. */
> > > > > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> > > > > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > > > > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> > > > >
> > > > >
> > > > > Each matrix has a type (which describes the meaning of the matrix) and an
> > > > > index (allowing for multiple matrices of the same type).
> > > > 
> > > > I'm just wondering how this could be used to specify coefficients 
> > > > associated with
> > > > selection rectangles for auto focus ?
> > > 
> > > I've been thinking about this. The problem is that sometimes you want to
> > > associate a matrix with some other object (a selection rectangle, a video
> > > input, perhaps a video buffer, etc.). A simple index may not be enough. So
> > > how about replacing the index field with a union:
> > > 
> > > 	union {
> > > 		__u32 reserved[4];
> > > 	} ref;
> > 
> > ...which is a proof of what I wrote above. :-)
> 
> Is it?

Indeed to be more precise, to some of that: here, we do have the same
problem locally that the property API would need to resolve globally. But
the problem itself is the same.

That said, a property API would and could not fix any issues today, and not
even next year. To be more practical, the question in the meantime is:
should V4L2 be extended to provide functionality that would match better
with the idea of the property-based API or not? The difficulty in that
approach would more likely be in defining an object model that would make
sense in the general case.

I'm not against adding a partial implementation of a property-based API to
V4L2 (i.e. what you're proposing in thie RFC) but I think we need to go
through the content thoroughly (I'll reply again to the original RFC).

I'd like to get Laurent's opinion, too.

-- 
Kind regards,

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

Re: [RFC] Add query/get/set matrix ioctls

[Hans Verkuil]

On Mon 17 June 2013 00:09:59 Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Jun 12, 2013 at 02:57:21PM +0200, Hans Verkuil wrote:
> > On Wed 12 June 2013 14:26:27 Sakari Ailus wrote:
> > > Hi Hans,
> > > 
> > > Thanks for the RFC!
> > > 
> > > On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> > > > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> > > > > Hi Hans,
> > > > > 
> > > > > On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > When working on the Motion Detection API, I realized that my proposal for
> > > > > > two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> > > > > > case.
> > > > > >
> > > > > > The Motion Detection RFC can be found here:
> > > > > >
> > > > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > > > > >
> > > > > > What I was really attempting to do was to create an API to pass a matrix
> > > > > > to the hardware.
> > > > > >
> > > > > > This is also related to a long-standing request to add support for
> > > > > > arrays to the control API. Adding such support to the control API is in my
> > > > > > opinion a very bad idea since the control API is meant to provide all
> > > > > > meta data necessary in order to create e.g. control panels. Adding array
> > > > > > support to the control API would make that very difficult, particularly
> > > > > > with respect to GUI design.
> > > > > >
> > > > > > So instead this proposal creates a new API to query, get and set matrices:
> > > > > 
> > > > > This looks very interesting, one use case that comes immediately to mind is
> > > > > configuring the quantization/Huffman tables in a hardware JPEG codec. 
> > > > > The only
> > > > > thing left would have probably been setting up the comment segments, 
> > > > > consisting
> > > > > of arbitrary byte strings.
> > > > 
> > > > Actually, I realized that that can be handled as well since those segments
> > > > are 1-dimensional matrices of unsigned chars.
> > > > 
> > > > > 
> > > > > This is even more nice than your previous proposal ;) Quite generic - but
> > > > > I was wondering, what if we went one step further and defined QUERY/GET/
> > > > > SET_PROPERTY ioctls, where the type (matrix or anything else) would be
> > > > > also configurable ? :-) Just brainstorming, if memory serves me well few
> > > > > people suggested something like this in the past.
> > > 
> > > Interesting idea. This approach could be used on the Media controller
> > > interface as well.
> > 
> > What is needed for the MC (if memory serves) is something simple to list what
> > effectively are capabilities of entities. Basically just a list of integers.
> > That's quite different from this highly generic proposal.
> 
> I think I should have said "property API" instead. That's what I meant. This
> could be prototyped and discussed. Which device nodes that API could
> eventually use is another matter.
> 
> From API design point of view it's somewhat odd that a device (especially
> when it comes to embedded systems) is visible in the system as a large
> number of device nodes. I know there are a huge number of reasons why it's
> so currently but if I were to create a new API, that's one thing I would
> correct.
> 
> But in the context of matrix IOCTLs, this begins to be off-topic.
> 
> > > > The problem with that is that you basically create a meta-ioctl. Why not
> > > > just create an ioctl for whatever you want to do? After all, an ioctl is
> > > > basically a type (the command number) and a pointer. And with a property
> > > > ioctl you really just wrap that into another ioctl.
> > > 
> > > Is this a problem?
> > 
> > I think so, yes. It seems to me that this just adds an unnecessary indirection
> > that everyone (userspace and kernelspace) has to cope with.
> > 
> > I don't see any advantage of going in this direction.
> 
> To some extent one could claim the controls API does exactly that: it
> provides a generic way to access properties of certain kind. I like controls
> and extended controls even more: the standard API makes many other things
> easier in user space, including enumeration.
> 
> > > One of the benefits is that you could send multiple IOCTL commands to the
> > > device at one go (if the interface is designed as such, and I think it
> > > should be).
> > 
> > There are other ways this can be done (we discussed that in the past) without
> > introducing complex new ioctls. And the reality is that this doesn't really
> 
> Could you refresh my memory a bit?

I can't remember whether we discussed that during a meeting or via an RFC.
Anyway, the idea was to have something transaction based: e.g. queue up a
number of ioctls, then 'commit' or 'execute' them. I seem to remember I wrote
an RFC on that topic, but it was probably 1-2 years ago.

> I remember synchronisation of applying
> configurations being discussed, and, well, most of the time that certainly
> isn't an issue, but if you wish to ensure two configuration parameters on
> different subdevs take effect at the same time, there's no fully generic way
> to do that in the API: you have to rely on timing to some extent.

You cannot sync two different pieces of hardware at the same time anyway. Which
is why the extended controls API makes a 'best-effort' only in setting controls.

If you need to configure a device at a specific time, then that requires some
sort of hardware support to trigger a new configuration or you need to use a
real-time OS/firmware to do that.

> 
> > help you much: the real complexity will be in handling such ioctl sets in a
> > driver.
> 
> Very, very true. I still maintain there are cases where this a) could be
> done and b) would be nice and useful. The configuration of different
> omap3isp blocks, for instance, now passed to the driver using private ioctls.
> 
> > > It would be easier to model extensions to the V4L2 API using that kind of
> > > model; currently we have a bunch of IOCTLs that certainly do show the age of
> > > the API. With the property API, everything will be... properties you can
> > > access using set and get operations, and probably enum as well.
> > 
> > It's easy to model extension today as well: just add a new ioctl. How is that
> > any different than adding a new property type, except instead of just filling
> > in one struct to pass with the ioctl, I now have to fill in two: one for the
> > property encapsulation struct, one for the actual payload struct. Yes, it
> > looks like you have just a few property ioctls, but the reality is that the
> > complexity has just been moved to the property side of things.
> 
> I'm not claiming it'd magically fixed everything, but it'd be worth
> prototyping. I wish I had time for that (and well, to do a bunch of other
> pending things as well). :-P
> 
> > > 
> > > I think this is a logical extension of the V4L2 control API.
> > > 
> > > I have to admit designing that kind of an API isn't trivial either: more
> > > focus will be needed on what the properties are attached to: is that a
> > > device or a subdev pad, for instance. This way it might be possible to
> > > address such things at generic level rather than at the level of a single
> > > IOCTL as we're doing (or rather avoid doing) right now.
> > 
> > That's a problem that is unrelated to 'property' usage. Actually, that's
> > easier for 'non-property' ioctls since there you know how it is going to be
> > used, so there is no need to be generic.
> > 
> > BTW, I've been making the assumption that a property can hold any type of
> > data, not just 'simple' types. So it can hold a v4l2_format struct for example
> > (or something of similar complexity).
> 
> I don't think properties should --- instead they should contain information
> related to the fields of the struct. The structs combine together
> information which is valid for a certain use case, and are unchangeable from
> then on. There are many upsides from this but also downsides. The full
> picture is not visible without at least prototyping a property-based API.

I agree, this would require prototyping. As you can tell, I'm skeptical about
this :-), so I don't think I will work on that myself.

> > > 
> > > > > So for the starters we could have matrix type and carefully be adding in
> > > > > the future anything what's needed ?
> > > > > 
> > > > > > /* Define to which motion detection region each element belongs.
> > > > > >   * Each element is a __u8. */
> > > > > > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > > > > > /* Define the motion detection threshold for each element.
> > > > > >   * Each element is a __u16. */
> > > > > > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > > > > >
> > > > > > /**
> > > > > >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > > > > >   * @type:       matrix type
> > > > > >   * @index:      matrix index of the given type
> > > > > >   * @columns:    number of columns in the matrix
> > > > > >   * @rows:       number of rows in the matrix
> > > > > >   * @elem_min:   minimum matrix element value
> > > > > >   * @elem_max:   maximum matrix element value
> > > > > >   * @elem_size:  size in bytes each matrix element
> > > > > >   * @reserved:   future extensions, applications and drivers must zero this.
> > > > > >   */
> > > > > > struct v4l2_query_matrix {
> > > > > >          __u32 type;
> > > > > >          __u32 index;
> > > > > >          __u32 columns;
> > > > > >          __u32 rows;
> > > > > >          __s64 elem_min;
> > > > > >          __s64 elem_max;
> > > > > >          __u32 elem_size;
> > > > > >          __u32 reserved[23];
> > > > > > } __attribute__ ((packed));
> > > > > >
> > > > > > /**
> > > > > >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> > > > > >   * @type:       matrix type
> > > > > >   * @index:      matrix index of the given type
> > > > > >   * @rect:       which part of the matrix to get/set
> > > > > >   * @matrix:     pointer to the matrix of size (in bytes):
> > > > > >   *              elem_size * rect.width * rect.height
> > > > > >   * @reserved:   future extensions, applications and drivers must zero this.
> > > > > >   */
> > > > > > struct v4l2_matrix {
> > > > > >          __u32 type;
> > > > > >          __u32 index;
> > > > > >          struct v4l2_rect rect;
> > > > > >          void __user *matrix;
> > > > > >          __u32 reserved[12];
> > > > > > } __attribute__ ((packed));
> > > > > >
> > > > > >
> > > > > > /* Experimental, these three ioctls may change over the next couple of kernel
> > > > > >     versions. */
> > > > > > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> > > > > > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > > > > > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> > > > > >
> > > > > >
> > > > > > Each matrix has a type (which describes the meaning of the matrix) and an
> > > > > > index (allowing for multiple matrices of the same type).
> > > > > 
> > > > > I'm just wondering how this could be used to specify coefficients 
> > > > > associated with
> > > > > selection rectangles for auto focus ?
> > > > 
> > > > I've been thinking about this. The problem is that sometimes you want to
> > > > associate a matrix with some other object (a selection rectangle, a video
> > > > input, perhaps a video buffer, etc.). A simple index may not be enough. So
> > > > how about replacing the index field with a union:
> > > > 
> > > > 	union {
> > > > 		__u32 reserved[4];
> > > > 	} ref;
> > > 
> > > ...which is a proof of what I wrote above. :-)
> > 
> > Is it?
> 
> Indeed to be more precise, to some of that: here, we do have the same
> problem locally that the property API would need to resolve globally. But
> the problem itself is the same.
> 
> That said, a property API would and could not fix any issues today, and not
> even next year. To be more practical, the question in the meantime is:
> should V4L2 be extended to provide functionality that would match better
> with the idea of the property-based API or not? The difficulty in that
> approach would more likely be in defining an object model that would make
> sense in the general case.

I think I would need to see a more concrete proposal for a property-based API.
It's a bit too abstract for me right now. 

> I'm not against adding a partial implementation of a property-based API to
> V4L2 (i.e. what you're proposing in thie RFC) but I think we need to go
> through the content thoroughly (I'll reply again to the original RFC).

Yes, please. This discussion drifted fairly far from my original RFC :-)
Right now I am just trying to solve the specific problem of how to set a
vector/matrix of integers.

> I'd like to get Laurent's opinion, too.

Indeed.

	Hans

Re: [RFC] Add query/get/set matrix ioctls

[Laurent Pinchart]

Hi Hans,

On Monday 17 June 2013 13:57:04 Hans Verkuil wrote:
> On Mon 17 June 2013 00:09:59 Sakari Ailus wrote:
> > On Wed, Jun 12, 2013 at 02:57:21PM +0200, Hans Verkuil wrote:
> > > On Wed 12 June 2013 14:26:27 Sakari Ailus wrote:
> > > > On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> > > > > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:

[snip]

> > > > > > This is even more nice than your previous proposal ;) Quite
> > > > > > generic - but I was wondering, what if we went one step further
> > > > > > and defined QUERY/GET/SET_PROPERTY ioctls, where the type (matrix
> > > > > > or anything else) would be also configurable ? :-) Just
> > > > > > brainstorming, if memory serves me well few people suggested
> > > > > > something like this in the past.
> > > > 
> > > > Interesting idea. This approach could be used on the Media controller
> > > > interface as well.
> > > 
> > > What is needed for the MC (if memory serves) is something simple to list
> > > what effectively are capabilities of entities. Basically just a list of
> > > integers. That's quite different from this highly generic proposal.
> > 
> > I think I should have said "property API" instead. That's what I meant.
> > This could be prototyped and discussed. Which device nodes that API could
> > eventually use is another matter.
> > 
> > From API design point of view it's somewhat odd that a device (especially
> > when it comes to embedded systems) is visible in the system as a large
> > number of device nodes. I know there are a huge number of reasons why it's
> > so currently but if I were to create a new API, that's one thing I would
> > correct.
> > 
> > But in the context of matrix IOCTLs, this begins to be off-topic.
> > 
> > > > > The problem with that is that you basically create a meta-ioctl. Why
> > > > > not just create an ioctl for whatever you want to do? After all, an
> > > > > ioctl is basically a type (the command number) and a pointer. And
> > > > > with a property ioctl you really just wrap that into another ioctl.
> > > > 
> > > > Is this a problem?
> > > 
> > > I think so, yes. It seems to me that this just adds an unnecessary
> > > indirection that everyone (userspace and kernelspace) has to cope with.
> > > 
> > > I don't see any advantage of going in this direction.
> > 
> > To some extent one could claim the controls API does exactly that: it
> > provides a generic way to access properties of certain kind. I like
> > controls and extended controls even more: the standard API makes many
> > other things easier in user space, including enumeration.
> > 
> > > > One of the benefits is that you could send multiple IOCTL commands to
> > > > the device at one go (if the interface is designed as such, and I
> > > > think it should be).
> > > 
> > > There are other ways this can be done (we discussed that in the past)
> > > without introducing complex new ioctls. And the reality is that this
> > > doesn't really
> >
> > Could you refresh my memory a bit?
> 
> I can't remember whether we discussed that during a meeting or via an RFC.
> Anyway, the idea was to have something transaction based: e.g. queue up a
> number of ioctls, then 'commit' or 'execute' them. I seem to remember I
> wrote an RFC on that topic, but it was probably 1-2 years ago.

I remember discussing that with you, but the whereabouts escape me as well. I 
don't think this will fly though, as it wouldn't offer a way to commit across 
multiple entities

> > I remember synchronisation of applying configurations being discussed,
> > and, well, most of the time that certainly isn't an issue, but if you wish
> > to ensure two configuration parameters on different subdevs take effect at
> > the same time, there's no fully generic way to do that in the API: you
> > have to rely on timing to some extent.
> 
> You cannot sync two different pieces of hardware at the same time anyway.
> Which is why the extended controls API makes a 'best-effort' only in
> setting controls.

You can if the hardware allows for that, otherwise you will need a "best-
effort" implementation. That's how atomic mode setting has been designed in 
KMS, basically if the commit operation comes too close to vsync it will be 
delayed until the next frame.

> If you need to configure a device at a specific time, then that requires
> some sort of hardware support to trigger a new configuration or you need to
> use a real-time OS/firmware to do that.
> 
> > > help you much: the real complexity will be in handling such ioctl sets
> > > in a driver.
> > 
> > Very, very true. I still maintain there are cases where this a) could be
> > done and b) would be nice and useful. The configuration of different
> > omap3isp blocks, for instance, now passed to the driver using private
> > ioctls.
> >
> > > > It would be easier to model extensions to the V4L2 API using that kind
> > > > of model; currently we have a bunch of IOCTLs that certainly do show
> > > > the age of the API. With the property API, everything will be...
> > > > properties you can access using set and get operations, and probably
> > > > enum as well.
> > > 
> > > It's easy to model extension today as well: just add a new ioctl. How is
> > > that any different than adding a new property type, except instead of
> > > just filling in one struct to pass with the ioctl, I now have to fill
> > > in two: one for the property encapsulation struct, one for the actual
> > > payload struct. Yes, it looks like you have just a few property ioctls,
> > > but the reality is that the complexity has just been moved to the
> > > property side of things.
> > 
> > I'm not claiming it'd magically fixed everything, but it'd be worth
> > prototyping. I wish I had time for that (and well, to do a bunch of other
> > pending things as well). :-P
> > 
> > > > I think this is a logical extension of the V4L2 control API.
> > > > 
> > > > I have to admit designing that kind of an API isn't trivial either:
> > > > more focus will be needed on what the properties are attached to: is
> > > > that a device or a subdev pad, for instance. This way it might be
> > > > possible to address such things at generic level rather than at the
> > > > level of a single IOCTL as we're doing (or rather avoid doing) right
> > > > now.
> > > 
> > > That's a problem that is unrelated to 'property' usage. Actually, that's
> > > easier for 'non-property' ioctls since there you know how it is going to
> > > be used, so there is no need to be generic.
> > > 
> > > BTW, I've been making the assumption that a property can hold any type
> > > of data, not just 'simple' types. So it can hold a v4l2_format struct
> > > for example (or something of similar complexity).
> > 
> > I don't think properties should --- instead they should contain
> > information related to the fields of the struct. The structs combine
> > together information which is valid for a certain use case, and are
> > unchangeable from then on. There are many upsides from this but also
> > downsides. The full picture is not visible without at least prototyping a
> > property-based API.
>
> I agree, this would require prototyping. As you can tell, I'm skeptical
> about this :-), so I don't think I will work on that myself.

[snip]

> > > > > > > Each matrix has a type (which describes the meaning of the
> > > > > > > matrix) and an index (allowing for multiple matrices of the same
> > > > > > > type).
> > > > > > 
> > > > > > I'm just wondering how this could be used to specify coefficients
> > > > > > associated with selection rectangles for auto focus ?
> > > > > 
> > > > > I've been thinking about this. The problem is that sometimes you
> > > > > want to associate a matrix with some other object (a selection
> > > > > rectangle, a video input, perhaps a video buffer, etc.). A simple
> > > > > index may not be> enough. So how about replacing the index field
> > > > > with a union:
> > > > >
> > > > > 	union {
> > > > > 		__u32 reserved[4];
> > > > > 	} ref;
> > > > 
> > > > ...which is a proof of what I wrote above. :-)
> > > 
> > > Is it?
> > 
> > Indeed to be more precise, to some of that: here, we do have the same
> > problem locally that the property API would need to resolve globally. But
> > the problem itself is the same.
> > 
> > That said, a property API would and could not fix any issues today, and
> > not even next year. To be more practical, the question in the meantime is:
> > should V4L2 be extended to provide functionality that would match better
> > with the idea of the property-based API or not? The difficulty in that
> > approach would more likely be in defining an object model that would make
> > sense in the general case.
> 
> I think I would need to see a more concrete proposal for a property-based
> API. It's a bit too abstract for me right now.
> 
> > I'm not against adding a partial implementation of a property-based API to
> > V4L2 (i.e. what you're proposing in thie RFC) but I think we need to go
> > through the content thoroughly (I'll reply again to the original RFC).
> 
> Yes, please. This discussion drifted fairly far from my original RFC :-)
> Right now I am just trying to solve the specific problem of how to set a
> vector/matrix of integers.
> 
> > I'd like to get Laurent's opinion, too.
> 
> Indeed.

Done :-) Sorry once again for the delay.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Add query/get/set matrix ioctls

[Laurent Pinchart]

Hi Hans,

On Wednesday 12 June 2013 14:57:21 Hans Verkuil wrote:
> On Wed 12 June 2013 14:26:27 Sakari Ailus wrote:
> > On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> > > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> > > > On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > > > > Hi all,
> > > > > 
> > > > > When working on the Motion Detection API, I realized that my
> > > > > proposal for two new G/S_MD_BLOCKS ioctls was too specific for the
> > > > > motion detection use case.
> > > > > 
> > > > > The Motion Detection RFC can be found here:
> > > > > 
> > > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.htm
> > > > > l
> > > > > 
> > > > > What I was really attempting to do was to create an API to pass a
> > > > > matrix to the hardware.
> > > > > 
> > > > > This is also related to a long-standing request to add support for
> > > > > arrays to the control API. Adding such support to the control API is
> > > > > in my opinion a very bad idea since the control API is meant to
> > > > > provide all meta data necessary in order to create e.g. control
> > > > > panels. Adding array support to the control API would make that very
> > > > > difficult, particularly with respect to GUI design.
> > > > >
> > > > > So instead this proposal creates a new API to query, get and set
> > > > > matrices:
> > > >
> > > > This looks very interesting, one use case that comes immediately to
> > > > mind is configuring the quantization/Huffman tables in a hardware JPEG
> > > > codec. The only thing left would have probably been setting up the
> > > > comment segments, consisting of arbitrary byte strings.
> > > 
> > > Actually, I realized that that can be handled as well since those
> > > segments are 1-dimensional matrices of unsigned chars.
> > > 
> > > > This is even more nice than your previous proposal ;) Quite generic -
> > > > but I was wondering, what if we went one step further and defined
> > > > QUERY/GET/SET_PROPERTY ioctls, where the type (matrix or anything
> > > > else) would be also configurable ? :-) Just brainstorming, if memory
> > > > serves me well few people suggested something like this in the past.
> > 
> > Interesting idea. This approach could be used on the Media controller
> > interface as well.
> 
> What is needed for the MC (if memory serves) is something simple to list
> what effectively are capabilities of entities. Basically just a list of
> integers. That's quite different from this highly generic proposal.

Not only that, but we will need a way to configure pipelines atomically, as 
explained in my previous reply to this mail thread.

> > > The problem with that is that you basically create a meta-ioctl. Why not
> > > just create an ioctl for whatever you want to do? After all, an ioctl is
> > > basically a type (the command number) and a pointer. And with a property
> > > ioctl you really just wrap that into another ioctl.
> > 
> > Is this a problem?
> 
> I think so, yes. It seems to me that this just adds an unnecessary
> indirection that everyone (userspace and kernelspace) has to cope with.
> 
> I don't see any advantage of going in this direction.
> 
> > One of the benefits is that you could send multiple IOCTL commands to the
> > device at one go (if the interface is designed as such, and I think it
> > should be).
> 
> There are other ways this can be done (we discussed that in the past)
> without introducing complex new ioctls. And the reality is that this
> doesn't really help you much: the real complexity will be in handling such
> ioctl sets in a driver.
> 
> > It would be easier to model extensions to the V4L2 API using that kind of
> > model; currently we have a bunch of IOCTLs that certainly do show the age
> > of the API. With the property API, everything will be... properties you
> > can access using set and get operations, and probably enum as well.
> 
> It's easy to model extension today as well: just add a new ioctl. How is
> that any different than adding a new property type, except instead of just
> filling in one struct to pass with the ioctl, I now have to fill in two:
> one for the property encapsulation struct, one for the actual payload
> struct. Yes, it looks like you have just a few property ioctls, but the
> reality is that the complexity has just been moved to the property side of
> things.

The complexity won't go magically away, whatever API we decide to create. The 
point of a property API isn't so much about hiding/moving complexity or adding 
indirection, it's about being able to set properties that span currently 
unrelated V4L2 information (such as controls and formats for instance) on 
several entities at the same time.

> > I think this is a logical extension of the V4L2 control API.
> > 
> > I have to admit designing that kind of an API isn't trivial either: more
> > focus will be needed on what the properties are attached to: is that a
> > device or a subdev pad, for instance. This way it might be possible to
> > address such things at generic level rather than at the level of a single
> > IOCTL as we're doing (or rather avoid doing) right now.
> 
> That's a problem that is unrelated to 'property' usage. Actually, that's
> easier for 'non-property' ioctls since there you know how it is going to be
> used, so there is no need to be generic.
> 
> BTW, I've been making the assumption that a property can hold any type of
> data, not just 'simple' types. So it can hold a v4l2_format struct for
> example (or something of similar complexity).

Correct.

> > > > So for the starters we could have matrix type and carefully be adding
> > > > in the future anything what's needed ?
> > > > 
> > > > > /* Define to which motion detection region each element belongs.
> > > > >   * Each element is a __u8. */
> > > > > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > > > > /* Define the motion detection threshold for each element.
> > > > >   * Each element is a __u16. */
> > > > > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > > > > 
> > > > > /**
> > > > >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > > > >   * @type:       matrix type
> > > > >   * @index:      matrix index of the given type
> > > > >   * @columns:    number of columns in the matrix
> > > > >   * @rows:       number of rows in the matrix
> > > > >   * @elem_min:   minimum matrix element value
> > > > >   * @elem_max:   maximum matrix element value
> > > > >   * @elem_size:  size in bytes each matrix element
> > > > >   * @reserved:   future extensions, applications and drivers must
> > > > >   zero this.
> > > > >   */
> > > > > struct v4l2_query_matrix {
> > > > >          __u32 type;
> > > > >          __u32 index;
> > > > >          __u32 columns;
> > > > >          __u32 rows;
> > > > >          __s64 elem_min;
> > > > >          __s64 elem_max;
> > > > >          __u32 elem_size;
> > > > >          __u32 reserved[23];
> > > > > } __attribute__ ((packed));
> > > > > 
> > > > > /**
> > > > >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> > > > >   * @type:       matrix type
> > > > >   * @index:      matrix index of the given type
> > > > >   * @rect:       which part of the matrix to get/set
> > > > >   * @matrix:     pointer to the matrix of size (in bytes):
> > > > >   *              elem_size * rect.width * rect.height
> > > > >   * @reserved:   future extensions, applications and drivers must
> > > > >   zero this.
> > > > >   */
> > > > > struct v4l2_matrix {
> > > > >          __u32 type;
> > > > >          __u32 index;
> > > > >          struct v4l2_rect rect;
> > > > >          void __user *matrix;
> > > > >          __u32 reserved[12];
> > > > > } __attribute__ ((packed));
> > > > > 
> > > > > 
> > > > > /* Experimental, these three ioctls may change over the next couple
> > > > > of kernel versions. */
> > > > > 
> > > > > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct
> > > > > v4l2_query_matrix)
> > > > > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > > > > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> > > > > 
> > > > > 
> > > > > Each matrix has a type (which describes the meaning of the matrix)
> > > > > and an index (allowing for multiple matrices of the same type).
> > > > 
> > > > I'm just wondering how this could be used to specify coefficients
> > > > associated with selection rectangles for auto focus ?
> > > 
> > > I've been thinking about this. The problem is that sometimes you want to
> > > associate a matrix with some other object (a selection rectangle, a
> > > video input, perhaps a video buffer, etc.). A simple index may not be
> > > enough.
> > >
> > > So how about replacing the index field with a union:
> > > 	union {
> > > 		__u32 reserved[4];
> > > 	} ref;
> > 
> > ...which is a proof of what I wrote above. :-)
> 
> Is it?
-- 
Regards,

Laurent Pinchart

Re: [RFC] Add query/get/set matrix ioctls

[Laurent Pinchart]

Hello,

I'll jump in, sorry for the late review.

On Wednesday 12 June 2013 15:26:27 Sakari Ailus wrote:
> On Wed, Jun 12, 2013 at 10:35:07AM +0200, Hans Verkuil wrote:
> > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> > > On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> > > > Hi all,
> > > > 
> > > > When working on the Motion Detection API, I realized that my proposal
> > > > for two new G/S_MD_BLOCKS ioctls was too specific for the motion
> > > > detection use case.
> > > > 
> > > > The Motion Detection RFC can be found here:
> > > > 
> > > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > > > 
> > > > What I was really attempting to do was to create an API to pass a
> > > > matrix to the hardware.
> > > > 
> > > > This is also related to a long-standing request to add support for
> > > > arrays to the control API. Adding such support to the control API is
> > > > in my opinion a very bad idea since the control API is meant to
> > > > provide all meta data necessary in order to create e.g. control
> > > > panels. Adding array support to the control API would make that very
> > > > difficult, particularly with respect to GUI design.
> > > > 
> > > > So instead this proposal creates a new API to query, get and set
> > > > matrices:
> > >
> > > This looks very interesting, one use case that comes immediately to mind
> > > is configuring the quantization/Huffman tables in a hardware JPEG codec.
> > > The only thing left would have probably been setting up the comment
> > > segments, consisting of arbitrary byte strings.
> > 
> > Actually, I realized that that can be handled as well since those segments
> > are 1-dimensional matrices of unsigned chars.
> > 
> > > This is even more nice than your previous proposal ;) Quite generic -
> > > but I was wondering, what if we went one step further and defined
> > > QUERY/GET/SET_PROPERTY ioctls, where the type (matrix or anything else)
> > > would be also configurable ? :-) Just brainstorming, if memory serves me
> > > well few people suggested something like this in the past.
> 
> Interesting idea. This approach could be used on the Media controller
> interface as well.
> 
> > The problem with that is that you basically create a meta-ioctl. Why not
> > just create an ioctl for whatever you want to do? After all, an ioctl is
> > basically a type (the command number) and a pointer. And with a property
> > ioctl you really just wrap that into another ioctl.
> 
> Is this a problem?
> 
> One of the benefits is that you could send multiple IOCTL commands to the
> device at one go (if the interface is designed as such, and I think it
> should be).

That's where the real value of a property API would be. Setting properties one 
at a time would be pretty much equivalent to what we do today, it would just 
be a different way to express ioctls. Setting several properties in one go, 
however, would allow us not only to modify several controls in one go, but to 
completely reconfigure a pipeline, including formats, selection rectangles and 
links, in an atomic way.

Let's not deny it, achieving this won't be easy, but I think we won't be able 
to delay such an architecture evolution for many years. The KMS is already 
moving to an atomic mode setting API based on properties, and the line between 
capture devices, codecs, processing engines and display devices is getting 
thinner every day. Video pipelines are composed of several devices in most of 
today's SoCs, and we will need a way to handle dynamic runtime configuration.

The matrix ioctls proposal looks pretty good to me by itself, but I do believe 
we will need a property-like API, and that we should stop adding too many new 
ioctls before we really think this through.

> It would be easier to model extensions to the V4L2 API using that kind of
> model; currently we have a bunch of IOCTLs that certainly do show the age of
> the API. With the property API, everything will be... properties you can
> access using set and get operations, and probably enum as well.
> 
> I think this is a logical extension of the V4L2 control API.
> 
> I have to admit designing that kind of an API isn't trivial either: more
> focus will be needed on what the properties are attached to: is that a
> device or a subdev pad, for instance. This way it might be possible to
> address such things at generic level rather than at the level of a single
> IOCTL as we're doing (or rather avoid doing) right now.
> 
> > > So for the starters we could have matrix type and carefully be adding in
> > > the future anything what's needed ?
> > > 
> > > > /* Define to which motion detection region each element belongs.
> > > >   * Each element is a __u8. */
> > > > #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > > > /* Define the motion detection threshold for each element.
> > > >   * Each element is a __u16. */
> > > > #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > > > 
> > > > /**
> > > >   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > > >   * @type:       matrix type
> > > >   * @index:      matrix index of the given type
> > > >   * @columns:    number of columns in the matrix
> > > >   * @rows:       number of rows in the matrix
> > > >   * @elem_min:   minimum matrix element value
> > > >   * @elem_max:   maximum matrix element value
> > > >   * @elem_size:  size in bytes each matrix element
> > > >   * @reserved:   future extensions, applications and drivers must zero
> > > >   this.
> > > >   */
> > > > struct v4l2_query_matrix {
> > > >          __u32 type;
> > > >          __u32 index;
> > > >          __u32 columns;
> > > >          __u32 rows;
> > > >          __s64 elem_min;
> > > >          __s64 elem_max;
> > > >          __u32 elem_size;
> > > >          __u32 reserved[23];
> > > > } __attribute__ ((packed));
> > > > 
> > > > /**
> > > >   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> > > >   * @type:       matrix type
> > > >   * @index:      matrix index of the given type
> > > >   * @rect:       which part of the matrix to get/set
> > > >   * @matrix:     pointer to the matrix of size (in bytes):
> > > >   *              elem_size * rect.width * rect.height
> > > >   * @reserved:   future extensions, applications and drivers must zero
> > > >   this.
> > > >   */
> > > > struct v4l2_matrix {
> > > >          __u32 type;
> > > >          __u32 index;
> > > >          struct v4l2_rect rect;
> > > >          void __user *matrix;
> > > >          __u32 reserved[12];
> > > > } __attribute__ ((packed));
> > > > 
> > > > 
> > > > /* Experimental, these three ioctls may change over the next couple of
> > > > kernel versions. */
> > > > 
> > > > #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct
> > > > v4l2_query_matrix)
> > > > #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> > > > #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> > > > 
> > > > 
> > > > Each matrix has a type (which describes the meaning of the matrix) and
> > > > an index (allowing for multiple matrices of the same type).
> > > 
> > > I'm just wondering how this could be used to specify coefficients
> > > associated with selection rectangles for auto focus ?
> > 
> > I've been thinking about this. The problem is that sometimes you want to
> > associate a matrix with some other object (a selection rectangle, a video
> > input, perhaps a video buffer, etc.). A simple index may not be enough. So
> > 
> > how about replacing the index field with a union:
> > 	union {
> > 		__u32 reserved[4];
> > 	} ref;
> 
> ...which is a proof of what I wrote above. :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC] Add query/get/set matrix ioctls

[Andrzej Hajda]

Hi Hans and Sylwester,

I would like to add my two cents.

On 06/12/2013 10:35 AM, Hans Verkuil wrote:
> On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
>> Hi Hans,
>>
>> On 06/03/2013 02:14 PM, Hans Verkuil wrote:
>>> Hi all,
>>>
>>> When working on the Motion Detection API, I realized that my proposal for
>>> two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
>>> case.
>>>
>>> The Motion Detection RFC can be found here:
>>>
>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
>>>
>>> What I was really attempting to do was to create an API to pass a matrix
>>> to the hardware.
>>>
>>> This is also related to a long-standing request to add support for
>>> arrays to the control API. Adding such support to the control API is in my
>>> opinion a very bad idea since the control API is meant to provide all
>>> meta data necessary in order to create e.g. control panels. Adding array
>>> support to the control API would make that very difficult, particularly
>>> with respect to GUI design.
>>>
>>> So instead this proposal creates a new API to query, get and set matrices:
>>
>> This looks very interesting, one use case that comes immediately to mind is
>> configuring the quantization/Huffman tables in a hardware JPEG codec. 
>> The only
>> thing left would have probably been setting up the comment segments, 
>> consisting
>> of arbitrary byte strings.
> 
> Actually, I realized that that can be handled as well since those segments
> are 1-dimensional matrices of unsigned chars.

Treating array of chars as matrix seems for me to be an abuse. Why not
treat any blob of data as a matrix of chars with one row?
Additionally passing string from/to driver via VIDIOC_G/S_MATRIX seems
awkward.

> 
>>
>> This is even more nice than your previous proposal ;) Quite generic - but
>> I was wondering, what if we went one step further and defined QUERY/GET/
>> SET_PROPERTY ioctls, where the type (matrix or anything else) would be
>> also configurable ? :-) Just brainstorming, if memory serves me well few
>> people suggested something like this in the past.
> 
> The problem with that is that you basically create a meta-ioctl. Why not
> just create an ioctl for whatever you want to do? After all, an ioctl is
> basically a type (the command number) and a pointer. And with a property
> ioctl you really just wrap that into another ioctl.

Those ioctls would share similar scheme (QUERY/SET/GET) and similar
purpose - passing some properties between user space and kernel, I think
this could be a good reason to use three ioctls instead of 3xN.

> 
>> So for the starters we could have matrix type and carefully be adding in
>> the future anything what's needed ?
>>
>>> /* Define to which motion detection region each element belongs.
>>>   * Each element is a __u8. */
>>> #define V4L2_MATRIX_TYPE_MD_REGION     (1)
>>> /* Define the motion detection threshold for each element.
>>>   * Each element is a __u16. */
>>> #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
>>>
>>> /**
>>>   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
>>>   * @type:       matrix type
>>>   * @index:      matrix index of the given type
>>>   * @columns:    number of columns in the matrix
>>>   * @rows:       number of rows in the matrix
>>>   * @elem_min:   minimum matrix element value
>>>   * @elem_max:   maximum matrix element value
>>>   * @elem_size:  size in bytes each matrix element
>>>   * @reserved:   future extensions, applications and drivers must zero this.
>>>   */
>>> struct v4l2_query_matrix {
>>>          __u32 type;
>>>          __u32 index;
>>>          __u32 columns;
>>>          __u32 rows;
>>>          __s64 elem_min;
>>>          __s64 elem_max;
>>>          __u32 elem_size;
>>>          __u32 reserved[23];
>>> } __attribute__ ((packed));
>>>
>>> /**
>>>   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
>>>   * @type:       matrix type
>>>   * @index:      matrix index of the given type
>>>   * @rect:       which part of the matrix to get/set
>>>   * @matrix:     pointer to the matrix of size (in bytes):
>>>   *              elem_size * rect.width * rect.height
>>>   * @reserved:   future extensions, applications and drivers must zero this.
>>>   */
>>> struct v4l2_matrix {
>>>          __u32 type;
>>>          __u32 index;
>>>          struct v4l2_rect rect;
>>>          void __user *matrix;
>>>          __u32 reserved[12];
>>> } __attribute__ ((packed));
>>>
>>>
>>> /* Experimental, these three ioctls may change over the next couple of kernel
>>>     versions. */
>>> #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
>>> #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
>>> #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
>>>
>>>
>>> Each matrix has a type (which describes the meaning of the matrix) and an
>>> index (allowing for multiple matrices of the same type).
>>
>> I'm just wondering how this could be used to specify coefficients 
>> associated with
>> selection rectangles for auto focus ?
> 
> I've been thinking about this. The problem is that sometimes you want to
> associate a matrix with some other object (a selection rectangle, a video
> input, perhaps a video buffer, etc.). A simple index may not be enough. So
> how about replacing the index field with a union:
> 
> 	union {
> 		__u32 reserved[4];
> 	} ref;
> 
> The precise contents of the union will be defined by the matrix type. Apps
> should probably zero ref to allow adding additional 'reference' fields in
> the future. So to refer to a selection rectangle you would add:
> 
> 	struct {
> 		__u32 type;
> 		__u32 target;
> 	};
> 
> to the union.
> 
>> This API would be used only for 
>> passing a
>> set of coefficients, assuming the individual rectangles are passed 
>> somehow else ?
> 
> Yes.
> 

It will make AF setting dispersed across three APIs:

1. Set V4L2_CID_AUTO_FOCUS_AREA to rectangle.
2. Set rectangles via selection API.
3. Set coefficients via matrix API.
4. Trigger V4L2_CID_AUTO_FOCUS_START.

Passing coefficients via selection API (I hope u32 would be enough for
now) or passing rectangles together with coefficients via matrix API
could make it little shorter and we could avoid to create object references.

>>
>>> QUERY_MATRIX will return the number of columns and rows in the full matrix,
>>> the size (in bytes) of each element and the minimum and maximum value of
>>> each element. Some matrix types may have non-integer elements, in which case
>>> the minimum and maximum values are ignored.
>>
>> I guess min/max could be made unions with a possibility to define other 
>> types ?
> 
> How about this?
> 
> 	union {
> 		__s64 val;
> 		__u64 uval;
> 		__u32 reserved[4];
> 	} min;
> 
> Ditto for max.
> 
>> Why should we limit ourselves to integers only ? However anything 
>> (sensible?)
>> that comes to my mind ATM are rational numbers only.
> 
> Fixed point is another.
> 
>>
>>> With S_MATRIX and G_MATRIX you can get/set a (part of a) matrix. The rect
>>> struct will give the part of the matrix that you want to set or retrieve, and
>>> the matrix pointer points to the matrix data.
>>>
>>> Currently only two matrix types are defined, see the motion detection RFC for
>>> details.
>>>
>>> This approach is basically the same as proposed in the motion detection RFC,
>>> but it is much more general.
>>>
>>> Discussion points:
>>>
>>> 1) I'm using elem_size to allow for any element size. An alternative would be to
>>> define specific element types (e.g. U8, S8, U16, S16, etc.), but I feel that
>>> that is overkill. It is easier to associate each matrix type with a specific
>>> element type in the documentation for each type. For allocation purposes it
>>> is more useful to know the element size than the element type. But perhaps
>>> elem_size can be dropped altogether, or, alternatively, both an elem_size and
>>> elem_type should be defined.
>>
>> IMHO it makes sense to keep elem_size, for allocation purposes etc. And the
>> element type could be probably derived from the matrix type ?
> 
> Yes, each matrix type will document what element type it uses.
> 
>>
>>> 2) Per-driver matrix types are probably necessary as well: for example while
>>> colorspace conversion matrices are in principle generic, in practice the
>>> precise format of the elements is hardware specific. This isn't a problem
>>> as long as it is a well-defined private matrix type.
>>
>> Sounds reasonable.
> 
> Regards,
> 
> 	Hans
> 

Re: [RFC] Add query/get/set matrix ioctls

[Hans Verkuil]

On Wed June 12 2013 16:07:48 Andrzej Hajda wrote:
> Hi Hans and Sylwester,
> 
> I would like to add my two cents.
> 
> On 06/12/2013 10:35 AM, Hans Verkuil wrote:
> > On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
> >> Hi Hans,
> >>
> >> On 06/03/2013 02:14 PM, Hans Verkuil wrote:
> >>> Hi all,
> >>>
> >>> When working on the Motion Detection API, I realized that my proposal for
> >>> two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
> >>> case.
> >>>
> >>> The Motion Detection RFC can be found here:
> >>>
> >>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> >>>
> >>> What I was really attempting to do was to create an API to pass a matrix
> >>> to the hardware.
> >>>
> >>> This is also related to a long-standing request to add support for
> >>> arrays to the control API. Adding such support to the control API is in my
> >>> opinion a very bad idea since the control API is meant to provide all
> >>> meta data necessary in order to create e.g. control panels. Adding array
> >>> support to the control API would make that very difficult, particularly
> >>> with respect to GUI design.
> >>>
> >>> So instead this proposal creates a new API to query, get and set matrices:
> >>
> >> This looks very interesting, one use case that comes immediately to mind is
> >> configuring the quantization/Huffman tables in a hardware JPEG codec. 
> >> The only
> >> thing left would have probably been setting up the comment segments, 
> >> consisting
> >> of arbitrary byte strings.
> > 
> > Actually, I realized that that can be handled as well since those segments
> > are 1-dimensional matrices of unsigned chars.
> 
> Treating array of chars as matrix seems for me to be an abuse. Why not
> treat any blob of data as a matrix of chars with one row?
> Additionally passing string from/to driver via VIDIOC_G/S_MATRIX seems
> awkward.

It's not a string. If it was a string, then we could use a control. It is
literally a blob of data that can be inserted into a comment segment.

The alternative is to create a new ioctl to do just this, but that's a lot
of work and it can be just as well by this ioctl. Since we assign this a
specific type ('JPEG_COMMENT' or something) we can prevent it from being
abused.

> 
> > 
> >>
> >> This is even more nice than your previous proposal ;) Quite generic - but
> >> I was wondering, what if we went one step further and defined QUERY/GET/
> >> SET_PROPERTY ioctls, where the type (matrix or anything else) would be
> >> also configurable ? :-) Just brainstorming, if memory serves me well few
> >> people suggested something like this in the past.
> > 
> > The problem with that is that you basically create a meta-ioctl. Why not
> > just create an ioctl for whatever you want to do? After all, an ioctl is
> > basically a type (the command number) and a pointer. And with a property
> > ioctl you really just wrap that into another ioctl.
> 
> Those ioctls would share similar scheme (QUERY/SET/GET) and similar
> purpose - passing some properties between user space and kernel, I think
> this could be a good reason to use three ioctls instead of 3xN.

On the outside it looks like you have just three ioctls, but in the kernel
you promptly have a switch on the type that effectively acts as an ioctl
command. So you gain nothing, it's just an obfuscation layer.

Say you have two ioctls:

S_FOO(struct v4l2_foo *p)
S_BAR(struct v4l2_bar *p)

With properties you would implement it like this:

struct v4l2_prop {
	__u32 type;
	union {
		struct v4l2_foo foo;
		struct v4l2_bar bar;
	};
};

S_PROP(struct v4l2_prop)

What does that gain you? It looks shorter, but in the kernel you have to
do a switch (type) everywhere you deal with properties. That's no different
than using switch (cmd) when dealing with an ioctl.


> 
> > 
> >> So for the starters we could have matrix type and carefully be adding in
> >> the future anything what's needed ?
> >>
> >>> /* Define to which motion detection region each element belongs.
> >>>   * Each element is a __u8. */
> >>> #define V4L2_MATRIX_TYPE_MD_REGION     (1)
> >>> /* Define the motion detection threshold for each element.
> >>>   * Each element is a __u16. */
> >>> #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> >>>
> >>> /**
> >>>   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> >>>   * @type:       matrix type
> >>>   * @index:      matrix index of the given type
> >>>   * @columns:    number of columns in the matrix
> >>>   * @rows:       number of rows in the matrix
> >>>   * @elem_min:   minimum matrix element value
> >>>   * @elem_max:   maximum matrix element value
> >>>   * @elem_size:  size in bytes each matrix element
> >>>   * @reserved:   future extensions, applications and drivers must zero this.
> >>>   */
> >>> struct v4l2_query_matrix {
> >>>          __u32 type;
> >>>          __u32 index;
> >>>          __u32 columns;
> >>>          __u32 rows;
> >>>          __s64 elem_min;
> >>>          __s64 elem_max;
> >>>          __u32 elem_size;
> >>>          __u32 reserved[23];
> >>> } __attribute__ ((packed));
> >>>
> >>> /**
> >>>   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> >>>   * @type:       matrix type
> >>>   * @index:      matrix index of the given type
> >>>   * @rect:       which part of the matrix to get/set
> >>>   * @matrix:     pointer to the matrix of size (in bytes):
> >>>   *              elem_size * rect.width * rect.height
> >>>   * @reserved:   future extensions, applications and drivers must zero this.
> >>>   */
> >>> struct v4l2_matrix {
> >>>          __u32 type;
> >>>          __u32 index;
> >>>          struct v4l2_rect rect;
> >>>          void __user *matrix;
> >>>          __u32 reserved[12];
> >>> } __attribute__ ((packed));
> >>>
> >>>
> >>> /* Experimental, these three ioctls may change over the next couple of kernel
> >>>     versions. */
> >>> #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
> >>> #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
> >>> #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
> >>>
> >>>
> >>> Each matrix has a type (which describes the meaning of the matrix) and an
> >>> index (allowing for multiple matrices of the same type).
> >>
> >> I'm just wondering how this could be used to specify coefficients 
> >> associated with
> >> selection rectangles for auto focus ?
> > 
> > I've been thinking about this. The problem is that sometimes you want to
> > associate a matrix with some other object (a selection rectangle, a video
> > input, perhaps a video buffer, etc.). A simple index may not be enough. So
> > how about replacing the index field with a union:
> > 
> > 	union {
> > 		__u32 reserved[4];
> > 	} ref;
> > 
> > The precise contents of the union will be defined by the matrix type. Apps
> > should probably zero ref to allow adding additional 'reference' fields in
> > the future. So to refer to a selection rectangle you would add:
> > 
> > 	struct {
> > 		__u32 type;
> > 		__u32 target;
> > 	};
> > 
> > to the union.
> > 
> >> This API would be used only for 
> >> passing a
> >> set of coefficients, assuming the individual rectangles are passed 
> >> somehow else ?
> > 
> > Yes.
> > 
> 
> It will make AF setting dispersed across three APIs:
> 
> 1. Set V4L2_CID_AUTO_FOCUS_AREA to rectangle.
> 2. Set rectangles via selection API.
> 3. Set coefficients via matrix API.
> 4. Trigger V4L2_CID_AUTO_FOCUS_START.
> 
> Passing coefficients via selection API (I hope u32 would be enough for
> now) or passing rectangles together with coefficients via matrix API
> could make it little shorter and we could avoid to create object references.

The problem is always how to find a balance between generic API components
and specific API components. Adding coefficients to the selection API makes
a generic API suddenly specific to one particular type of driver, something
that most other drivers implementing the selection API do not need.

Ditto for passing rectangles via the matrix API.

So you have two alternatives: either create a custom ioctl for you driver
that sets both the rectangle and coefficients in one go, or have two smaller
building blocks that you combine to achieve the result you want. The latter
is much more the Unix philisophy.

BTW, can you give (or point to) some background information regarding these
autofocus coefficients? What are they and how are they used?

Regards,

	Hans

Re: [RFC] Add query/get/set matrix ioctls

[Andrzej Hajda]

On 06/12/2013 04:47 PM, Hans Verkuil wrote:
> On Wed June 12 2013 16:07:48 Andrzej Hajda wrote:
>> Hi Hans and Sylwester,
>>
>> I would like to add my two cents.
>>
>> On 06/12/2013 10:35 AM, Hans Verkuil wrote:
>>> On Tue 11 June 2013 23:33:42 Sylwester Nawrocki wrote:
>>>> Hi Hans,
>>>>
>>>> On 06/03/2013 02:14 PM, Hans Verkuil wrote:
>>>>> Hi all,
>>>>>
>>>>> When working on the Motion Detection API, I realized that my proposal for
>>>>> two new G/S_MD_BLOCKS ioctls was too specific for the motion detection use
>>>>> case.
>>>>>
>>>>> The Motion Detection RFC can be found here:
>>>>>
>>>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
>>>>>
>>>>> What I was really attempting to do was to create an API to pass a matrix
>>>>> to the hardware.
>>>>>
>>>>> This is also related to a long-standing request to add support for
>>>>> arrays to the control API. Adding such support to the control API is in my
>>>>> opinion a very bad idea since the control API is meant to provide all
>>>>> meta data necessary in order to create e.g. control panels. Adding array
>>>>> support to the control API would make that very difficult, particularly
>>>>> with respect to GUI design.
>>>>>
>>>>> So instead this proposal creates a new API to query, get and set matrices:
>>>> This looks very interesting, one use case that comes immediately to mind is
>>>> configuring the quantization/Huffman tables in a hardware JPEG codec. 
>>>> The only
>>>> thing left would have probably been setting up the comment segments, 
>>>> consisting
>>>> of arbitrary byte strings.
>>> Actually, I realized that that can be handled as well since those segments
>>> are 1-dimensional matrices of unsigned chars.
>> Treating array of chars as matrix seems for me to be an abuse. Why not
>> treat any blob of data as a matrix of chars with one row?
>> Additionally passing string from/to driver via VIDIOC_G/S_MATRIX seems
>> awkward.
> It's not a string. If it was a string, then we could use a control. It is
> literally a blob of data that can be inserted into a comment segment.
>
> The alternative is to create a new ioctl to do just this, but that's a lot
> of work and it can be just as well by this ioctl. Since we assign this a
> specific type ('JPEG_COMMENT' or something) we can prevent it from being
> abused.
But claiming that blob of data/jpeg comment is a matrix seems
to be an abuse itself.

I try to show here that instead of encapsulating different entities into
a matrix maybe better is to create little more general ioctls.

>
>>>> This is even more nice than your previous proposal ;) Quite generic - but
>>>> I was wondering, what if we went one step further and defined QUERY/GET/
>>>> SET_PROPERTY ioctls, where the type (matrix or anything else) would be
>>>> also configurable ? :-) Just brainstorming, if memory serves me well few
>>>> people suggested something like this in the past.
>>> The problem with that is that you basically create a meta-ioctl. Why not
>>> just create an ioctl for whatever you want to do? After all, an ioctl is
>>> basically a type (the command number) and a pointer. And with a property
>>> ioctl you really just wrap that into another ioctl.
>> Those ioctls would share similar scheme (QUERY/SET/GET) and similar
>> purpose - passing some properties between user space and kernel, I think
>> this could be a good reason to use three ioctls instead of 3xN.
> On the outside it looks like you have just three ioctls, but in the kernel
> you promptly have a switch on the type that effectively acts as an ioctl
> command. So you gain nothing, it's just an obfuscation layer.
>
> Say you have two ioctls:
>
> S_FOO(struct v4l2_foo *p)
> S_BAR(struct v4l2_bar *p)
>
> With properties you would implement it like this:
>
> struct v4l2_prop {
> 	__u32 type;
> 	union {
> 		struct v4l2_foo foo;
> 		struct v4l2_bar bar;
> 	};
> };
>
> S_PROP(struct v4l2_prop)
>
> What does that gain you? It looks shorter, but in the kernel you have to
> do a switch (type) everywhere you deal with properties. That's no different
> than using switch (cmd) when dealing with an ioctl.
The same is with v4l2_query_matrix - you should also use switch(type)
or similar constructs on the kernel side.
I see there would be more common code in matrices of different types
but I guess properly designed properties could also allow to optimize
code for similar types.
>>>> So for the starters we could have matrix type and carefully be adding in
>>>> the future anything what's needed ?
>>>>
>>>>> /* Define to which motion detection region each element belongs.
>>>>>   * Each element is a __u8. */
>>>>> #define V4L2_MATRIX_TYPE_MD_REGION     (1)
>>>>> /* Define the motion detection threshold for each element.
>>>>>   * Each element is a __u16. */
>>>>> #define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
>>>>>
>>>>> /**
>>>>>   * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
>>>>>   * @type:       matrix type
>>>>>   * @index:      matrix index of the given type
>>>>>   * @columns:    number of columns in the matrix
>>>>>   * @rows:       number of rows in the matrix
>>>>>   * @elem_min:   minimum matrix element value
>>>>>   * @elem_max:   maximum matrix element value
>>>>>   * @elem_size:  size in bytes each matrix element
>>>>>   * @reserved:   future extensions, applications and drivers must zero this.
>>>>>   */
>>>>> struct v4l2_query_matrix {
>>>>>          __u32 type;
>>>>>          __u32 index;
>>>>>          __u32 columns;
>>>>>          __u32 rows;
>>>>>          __s64 elem_min;
>>>>>          __s64 elem_max;
>>>>>          __u32 elem_size;
>>>>>          __u32 reserved[23];
>>>>> } __attribute__ ((packed));
>>>>>
>>>>> /**
>>>>>   * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
>>>>>   * @type:       matrix type
>>>>>   * @index:      matrix index of the given type
>>>>>   * @rect:       which part of the matrix to get/set
>>>>>   * @matrix:     pointer to the matrix of size (in bytes):
>>>>>   *              elem_size * rect.width * rect.height
>>>>>   * @reserved:   future extensions, applications and drivers must zero this.
>>>>>   */
>>>>> struct v4l2_matrix {
>>>>>          __u32 type;
>>>>>          __u32 index;
>>>>>          struct v4l2_rect rect;
>>>>>          void __user *matrix;
>>>>>          __u32 reserved[12];
>>>>> } __attribute__ ((packed));
>>>>>
>>>>>
>>>>> /* Experimental, these three ioctls may change over the next couple of kernel
>>>>>     versions. */
>>>>> #define VIDIOC_QUERY_MATRIX     _IORW('V', 103, struct v4l2_query_matrix)
>>>>> #define VIDIOC_G_MATRIX         _IORW('V', 104, struct v4l2_matrix)
>>>>> #define VIDIOC_S_MATRIX         _IORW('V', 105, struct v4l2_matrix)
>>>>>
>>>>>
>>>>> Each matrix has a type (which describes the meaning of the matrix) and an
>>>>> index (allowing for multiple matrices of the same type).
>>>> I'm just wondering how this could be used to specify coefficients 
>>>> associated with
>>>> selection rectangles for auto focus ?
>>> I've been thinking about this. The problem is that sometimes you want to
>>> associate a matrix with some other object (a selection rectangle, a video
>>> input, perhaps a video buffer, etc.). A simple index may not be enough. So
>>> how about replacing the index field with a union:
>>>
>>> 	union {
>>> 		__u32 reserved[4];
>>> 	} ref;
>>>
>>> The precise contents of the union will be defined by the matrix type. Apps
>>> should probably zero ref to allow adding additional 'reference' fields in
>>> the future. So to refer to a selection rectangle you would add:
>>>
>>> 	struct {
>>> 		__u32 type;
>>> 		__u32 target;
>>> 	};
>>>
>>> to the union.
>>>
>>>> This API would be used only for 
>>>> passing a
>>>> set of coefficients, assuming the individual rectangles are passed 
>>>> somehow else ?
>>> Yes.
>>>
>> It will make AF setting dispersed across three APIs:
>>
>> 1. Set V4L2_CID_AUTO_FOCUS_AREA to rectangle.
>> 2. Set rectangles via selection API.
>> 3. Set coefficients via matrix API.
>> 4. Trigger V4L2_CID_AUTO_FOCUS_START.
>>
>> Passing coefficients via selection API (I hope u32 would be enough for
>> now) or passing rectangles together with coefficients via matrix API
>> could make it little shorter and we could avoid to create object references.
> The problem is always how to find a balance between generic API components
> and specific API components. Adding coefficients to the selection API makes
> a generic API suddenly specific to one particular type of driver, something
> that most other drivers implementing the selection API do not need.
Android API has defined 'weight' coefficient for 'areas' passed to the
camera:
http://developer.android.com/reference/android/hardware/Camera.Area.html
Such rectangles are used for focus and metering.
It is a part of generic API for cameras.

>
> Ditto for passing rectangles via the matrix API.
>
> So you have two alternatives: either create a custom ioctl for you driver
> that sets both the rectangle and coefficients in one go, or have two smaller
> building blocks that you combine to achieve the result you want. The latter
> is much more the Unix philisophy.
>
> BTW, can you give (or point to) some background information regarding these
> autofocus coefficients? What are they and how are they used?
The main source of my informations is Android API mentioned above,
plus my findings from analysis of user manuals of different cameras.

Regards
Andrzej

>
> Regards,
>
> 	Hans
>