[RFC PATCH 00/11] Add configuration store support

[RFC PATCH 00/11] Add configuration store support

[Hans Verkuil]

This patch series adds support for configuration stores to the control framework.
This allows you to store control values for a particular configuration (up to
VIDEO_MAX_FRAME configuration stores are currently supported). When you queue
a new buffer you can supply the store ID and the driver will apply all controls
for that configuration store.

When you set a new value for a configuration store then you can choose whether
this is 'fire and forget', i.e. after the driver applies the control value for that
store it won't be applied again until a new value is set. Or you can set the
value every time that configuration store is applied.

The first 7 patches add support for this in the API and in v4l2-ctrls.c. Patch
8 adds configure store support for the contrast control in the vivid driver.

Patches 9-11 add support for crop/compose controls to v4l2-ctrls and vivid
as a proof-of-concept. This allows you to play around with things like
digital zoom by manipulating crop and compose rectangles for specific buffers.
It's basically a hack just to allow me to test this so don't bother reviewing
these last three patches.

This patch series is available here:

http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=confstore

A patched version of qv4l2 and v4l2-ctl that add config store support
is available here:

http://git.linuxtv.org/cgit.cgi/hverkuil/v4l-utils.git/log/?h=confstore

The easiest way to test this is with vivid. Load vivid, then run the patched
qv4l2. This will associate every queued buffer with a corresponding config
store. There are 4 buffers, so stores 1-4 are available for use.

You can change the contrast value for a buffer as follows:

v4l2-ctl --store=1 -c contrast=90

For a fire-and-forget you add the --ignore-after-use option:

v4l2-ctl --store=1 -c contrast=90 --ignore-after-use

So you can cycle between different contrast values as follows:

v4l2-ctl --store=1 -c contrast=90
v4l2-ctl --store=2 -c contrast=100
v4l2-ctl --store=3 -c contrast=110
v4l2-ctl --store=4 -c contrast=120

This patch series and the API enhancements will be discussed during the
upcoming media workshop.

Regards,

	Hans

[RFC PATCH 01/11] videodev2.h: add V4L2_CTRL_FLAG_CAN_STORE

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Controls that have a configuration store will set this flag.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/videodev2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 0b1ba5c..199834b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1375,6 +1375,7 @@ struct v4l2_querymenu {
 #define V4L2_CTRL_FLAG_WRITE_ONLY 	0x0040
 #define V4L2_CTRL_FLAG_VOLATILE		0x0080
 #define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
+#define V4L2_CTRL_FLAG_CAN_STORE	0x0200
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
-- 
2.1.0

[RFC PATCH 02/11] videodev2.h: add config_store to v4l2_ext_controls

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The ctrl_class is fairly pointless when used with drivers that use the control
framework: you can just fill in 0 and it will just work fine. There are still
some old unconverted drivers that do not support 0 and instead want the control
class there. The idea being that all controls in the list all belong to that
class. This was done to simplify drivers in the absence of the control framework.

When using the control framework the framework itself is smart enough to allow
controls of any class to be included in the control list.

Since configuration store IDs are in the range 1..255 (or so, in any case a relatively
small non-zero positive integer) it makes sense to effectively rename ctrl_class
to config_store. Set it to 0 and you get the normal behavior (you change the current
control value), set it to a configuration store ID and you get/set the control for
that store.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/videodev2.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 199834b..83ef28a 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1294,7 +1294,10 @@ struct v4l2_ext_control {
 } __attribute__ ((packed));
 
 struct v4l2_ext_controls {
-	__u32 ctrl_class;
+	union {
+		__u32 ctrl_class;
+		__u32 config_store;
+	};
 	__u32 count;
 	__u32 error_idx;
 	__u32 reserved[2];
-- 
2.1.0

[RFC PATCH 03/11] videodev2.h: rename reserved2 to config_store in v4l2_buffer.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When queuing buffers allow for passing the configuration store ID that
should be associated with this buffer. Use the 'reserved2' field for this.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/cpia2/cpia2_v4l.c           | 2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
 drivers/media/v4l2-core/videobuf2-core.c      | 4 +++-
 include/uapi/linux/videodev2.h                | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 9caea83..0f28d2b 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->sequence = cam->buffers[buf->index].seq;
 	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
 	buf->length = cam->frame_size;
-	buf->reserved2 = 0;
+	buf->config_store = 0;
 	buf->reserved = 0;
 	memset(&buf->timecode, 0, sizeof(buf->timecode));
 
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index e502a5f..5afef3a 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -324,7 +324,7 @@ struct v4l2_buffer32 {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__u32			config_store;
 	__u32			reserved;
 };
 
@@ -489,7 +489,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
 		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
 		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
 		put_user(kp->sequence, &up->sequence) ||
-		put_user(kp->reserved2, &up->reserved2) ||
+		put_user(kp->config_store, &up->config_store) ||
 		put_user(kp->reserved, &up->reserved))
 			return -EFAULT;
 
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7e6aff6..e3b6c50 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -655,7 +655,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 
 	/* Copy back data such as timestamp, flags, etc. */
 	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
-	b->reserved2 = vb->v4l2_buf.reserved2;
+	b->config_store = vb->v4l2_buf.config_store;
 	b->reserved = vb->v4l2_buf.reserved;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
@@ -680,6 +680,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 		else if (q->memory == V4L2_MEMORY_DMABUF)
 			b->m.fd = vb->v4l2_planes[0].m.fd;
 	}
+	b->config_store = vb->v4l2_buf.config_store;
 
 	/*
 	 * Clear any buffer state related flags.
@@ -1324,6 +1325,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 		 */
 		vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 	}
+	vb->v4l2_buf.config_store = b->config_store;
 
 	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
 		/*
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 83ef28a..2ca44ed 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -672,6 +672,7 @@ struct v4l2_plane {
  * @length:	size in bytes of the buffer (NOT its payload) for single-plane
  *		buffers (when type != *_MPLANE); number of elements in the
  *		planes array for multi-plane buffers
+ * @config_store: this buffer should use this configuration store
  *
  * Contains data exchanged by application and driver using one of the Streaming
  * I/O methods.
@@ -695,7 +696,7 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__u32			config_store;
 	__u32			reserved;
 };
 
-- 
2.1.0

Re: [RFC PATCH 03/11] videodev2.h: rename reserved2 to config_store in v4l2_buffer.

[Sakari Ailus]

Hi Hans,

On Sun, Sep 21, 2014 at 04:48:21PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> When queuing buffers allow for passing the configuration store ID that
> should be associated with this buffer. Use the 'reserved2' field for this.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/cpia2/cpia2_v4l.c           | 2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
>  drivers/media/v4l2-core/videobuf2-core.c      | 4 +++-
>  include/uapi/linux/videodev2.h                | 3 ++-
>  4 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> index 9caea83..0f28d2b 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	buf->sequence = cam->buffers[buf->index].seq;
>  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>  	buf->length = cam->frame_size;
> -	buf->reserved2 = 0;
> +	buf->config_store = 0;
>  	buf->reserved = 0;
>  	memset(&buf->timecode, 0, sizeof(buf->timecode));
>  
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index e502a5f..5afef3a 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -324,7 +324,7 @@ struct v4l2_buffer32 {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__u32			config_store;
>  	__u32			reserved;
>  };
>  
> @@ -489,7 +489,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>  		put_user(kp->sequence, &up->sequence) ||
> -		put_user(kp->reserved2, &up->reserved2) ||
> +		put_user(kp->config_store, &up->config_store) ||
>  		put_user(kp->reserved, &up->reserved))
>  			return -EFAULT;
>  
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 7e6aff6..e3b6c50 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -655,7 +655,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  
>  	/* Copy back data such as timestamp, flags, etc. */
>  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> -	b->reserved2 = vb->v4l2_buf.reserved2;
> +	b->config_store = vb->v4l2_buf.config_store;
>  	b->reserved = vb->v4l2_buf.reserved;
>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> @@ -680,6 +680,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		else if (q->memory == V4L2_MEMORY_DMABUF)
>  			b->m.fd = vb->v4l2_planes[0].m.fd;
>  	}
> +	b->config_store = vb->v4l2_buf.config_store;

Either this chunk or the one above it is redundant. I'd keep the upper one.

>  
>  	/*
>  	 * Clear any buffer state related flags.
> @@ -1324,6 +1325,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  		 */
>  		vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>  	}
> +	vb->v4l2_buf.config_store = b->config_store;
>  
>  	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>  		/*
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 83ef28a..2ca44ed 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -672,6 +672,7 @@ struct v4l2_plane {
>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>   *		buffers (when type != *_MPLANE); number of elements in the
>   *		planes array for multi-plane buffers
> + * @config_store: this buffer should use this configuration store
>   *
>   * Contains data exchanged by application and driver using one of the Streaming
>   * I/O methods.
> @@ -695,7 +696,7 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__u32			config_store;
>  	__u32			reserved;
>  };
>  

-- 
Regards,

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

Re: [RFC PATCH 03/11] videodev2.h: rename reserved2 to config_store in v4l2_buffer.

[Hans Verkuil]

On 11/14/2014 03:42 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Sun, Sep 21, 2014 at 04:48:21PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> When queuing buffers allow for passing the configuration store ID that
>> should be associated with this buffer. Use the 'reserved2' field for this.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/usb/cpia2/cpia2_v4l.c           | 2 +-
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
>>  drivers/media/v4l2-core/videobuf2-core.c      | 4 +++-
>>  include/uapi/linux/videodev2.h                | 3 ++-
>>  4 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
>> index 9caea83..0f28d2b 100644
>> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
>> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
>> @@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>>  	buf->sequence = cam->buffers[buf->index].seq;
>>  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>>  	buf->length = cam->frame_size;
>> -	buf->reserved2 = 0;
>> +	buf->config_store = 0;
>>  	buf->reserved = 0;
>>  	memset(&buf->timecode, 0, sizeof(buf->timecode));
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index e502a5f..5afef3a 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -324,7 +324,7 @@ struct v4l2_buffer32 {
>>  		__s32		fd;
>>  	} m;
>>  	__u32			length;
>> -	__u32			reserved2;
>> +	__u32			config_store;
>>  	__u32			reserved;
>>  };
>>  
>> @@ -489,7 +489,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>>  		put_user(kp->sequence, &up->sequence) ||
>> -		put_user(kp->reserved2, &up->reserved2) ||
>> +		put_user(kp->config_store, &up->config_store) ||
>>  		put_user(kp->reserved, &up->reserved))
>>  			return -EFAULT;
>>  
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 7e6aff6..e3b6c50 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -655,7 +655,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>>  
>>  	/* Copy back data such as timestamp, flags, etc. */
>>  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
>> -	b->reserved2 = vb->v4l2_buf.reserved2;
>> +	b->config_store = vb->v4l2_buf.config_store;
>>  	b->reserved = vb->v4l2_buf.reserved;
>>  
>>  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
>> @@ -680,6 +680,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>>  		else if (q->memory == V4L2_MEMORY_DMABUF)
>>  			b->m.fd = vb->v4l2_planes[0].m.fd;
>>  	}
>> +	b->config_store = vb->v4l2_buf.config_store;
> 
> Either this chunk or the one above it is redundant. I'd keep the upper one.

Well spotted. I agree, I'll keep the upper one.

Regards,

	Hans

Re: [RFC PATCH 03/11] videodev2.h: rename reserved2 to config_store in v4l2_buffer.

[Sakari Ailus]

Hi Hans,

One more comment...

On Sun, Sep 21, 2014 at 04:48:21PM +0200, Hans Verkuil wrote:
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 83ef28a..2ca44ed 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -672,6 +672,7 @@ struct v4l2_plane {
>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>   *		buffers (when type != *_MPLANE); number of elements in the
>   *		planes array for multi-plane buffers
> + * @config_store: this buffer should use this configuration store
>   *
>   * Contains data exchanged by application and driver using one of the Streaming
>   * I/O methods.
> @@ -695,7 +696,7 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__u32			config_store;
>  	__u32			reserved;
>  };
>  

I would use __u16 instead since the value is 16-bit on the control
interface.

-- 
Regards,

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

Re: [RFC PATCH 03/11] videodev2.h: rename reserved2 to config_store in v4l2_buffer.

[Hans Verkuil]

On 11/14/2014 04:35 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> One more comment...
> 
> On Sun, Sep 21, 2014 at 04:48:21PM +0200, Hans Verkuil wrote:
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 83ef28a..2ca44ed 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -672,6 +672,7 @@ struct v4l2_plane {
>>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>>   *		buffers (when type != *_MPLANE); number of elements in the
>>   *		planes array for multi-plane buffers
>> + * @config_store: this buffer should use this configuration store
>>   *
>>   * Contains data exchanged by application and driver using one of the Streaming
>>   * I/O methods.
>> @@ -695,7 +696,7 @@ struct v4l2_buffer {
>>  		__s32		fd;
>>  	} m;
>>  	__u32			length;
>> -	__u32			reserved2;
>> +	__u32			config_store;
>>  	__u32			reserved;
>>  };
>>  
> 
> I would use __u16 instead since the value is 16-bit on the control
> interface.
> 

Good point. Will do.

	Hans

[RFC PATCH 04/11] v4l2-ctrls: add config store support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 150 +++++++++++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-ioctl.c |   4 +-
 include/media/v4l2-ctrls.h           |  14 ++++
 3 files changed, 141 insertions(+), 27 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 35d1f3d..df0f3ee 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1478,6 +1478,15 @@ static int cur_to_user(struct v4l2_ext_control *c,
 	return ptr_to_user(c, ctrl, ctrl->p_cur);
 }
 
+/* Helper function: copy the store's control value back to the caller */
+static int store_to_user(struct v4l2_ext_control *c,
+		       struct v4l2_ctrl *ctrl, unsigned store)
+{
+	if (store == 0)
+		return ptr_to_user(c, ctrl, ctrl->p_new);
+	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
+}
+
 /* Helper function: copy the new control value back to the caller */
 static int new_to_user(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl)
@@ -1585,6 +1594,14 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
 	}
 }
 
+/* Helper function: copy the new control value to the store */
+static void new_to_store(struct v4l2_ctrl *ctrl)
+{
+	/* has_changed is set by cluster_changed */
+	if (ctrl && ctrl->has_changed)
+		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
+}
+
 /* Copy the current value to the new value */
 static void cur_to_new(struct v4l2_ctrl *ctrl)
 {
@@ -1603,13 +1620,18 @@ static int cluster_changed(struct v4l2_ctrl *master)
 
 	for (i = 0; i < master->ncontrols; i++) {
 		struct v4l2_ctrl *ctrl = master->cluster[i];
+		union v4l2_ctrl_ptr ptr;
 		bool ctrl_changed = false;
 
 		if (ctrl == NULL)
 			continue;
+		if (ctrl->store)
+			ptr = ctrl->p_stores[ctrl->store - 1];
+		else
+			ptr = ctrl->p_cur;
 		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
 			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
-				ctrl->p_cur, ctrl->p_new);
+				ptr, ctrl->p_new);
 		ctrl->has_changed = ctrl_changed;
 		changed |= ctrl->has_changed;
 	}
@@ -1740,6 +1762,13 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 		list_del(&ctrl->node);
 		list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
 			list_del(&sev->node);
+		if (ctrl->p_stores) {
+			unsigned s;
+
+			for (s = 0; s < ctrl->nr_of_stores; s++)
+				kfree(ctrl->p_stores[s].p);
+		}
+		kfree(ctrl->p_stores);
 		kfree(ctrl);
 	}
 	kfree(hdl->buckets);
@@ -1970,7 +1999,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		handler_set_err(hdl, -ERANGE);
 		return NULL;
 	}
-	if (is_array &&
+	if ((is_array || (flags & V4L2_CTRL_FLAG_CAN_STORE)) &&
 	    (type == V4L2_CTRL_TYPE_BUTTON ||
 	     type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
 		handler_set_err(hdl, -EINVAL);
@@ -2084,8 +2113,10 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
 			is_menu ? cfg->menu_skip_mask : step, def,
 			cfg->dims, cfg->elem_size,
 			flags, qmenu, qmenu_int, priv);
-	if (ctrl)
+	if (ctrl) {
 		ctrl->is_private = cfg->is_private;
+		ctrl->can_store = cfg->can_store;
+	}
 	return ctrl;
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_custom);
@@ -2452,6 +2483,7 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
 				cur_to_new(master->cluster[i]);
 				master->cluster[i]->is_new = 1;
 				master->cluster[i]->done = true;
+				master->cluster[i]->store = 0;
 			}
 		}
 		ret = call_op(master, s_ctrl);
@@ -2539,6 +2571,8 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
 		qc->id = ctrl->id;
 	strlcpy(qc->name, ctrl->name, sizeof(qc->name));
 	qc->flags = ctrl->flags;
+	if (ctrl->can_store)
+		qc->flags |= V4L2_CTRL_FLAG_CAN_STORE;
 	qc->type = ctrl->type;
 	if (ctrl->is_ptr)
 		qc->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
@@ -2700,6 +2734,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			     struct v4l2_ctrl_helper *helpers,
 			     bool get)
 {
+	u32 ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+	unsigned store = cs->config_store & 0xffff;
 	struct v4l2_ctrl_helper *h;
 	bool have_clusters = false;
 	u32 i;
@@ -2712,7 +2748,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 
 		cs->error_idx = i;
 
-		if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
+		if (ctrl_class && V4L2_CTRL_ID2CLASS(id) != ctrl_class)
 			return -EINVAL;
 
 		/* Old-style private controls are not allowed for
@@ -2725,6 +2761,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		ctrl = ref->ctrl;
 		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
 			return -EINVAL;
+		if (store && !ctrl->can_store)
+			return -EINVAL;
 
 		if (ctrl->cluster[0]->ncontrols > 1)
 			have_clusters = true;
@@ -2796,6 +2834,32 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
 	return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
 }
 
+static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
+{
+	unsigned s, idx;
+	union v4l2_ctrl_ptr *p;
+
+	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
+	if (p == NULL)
+		return -ENOMEM;
+	for (s = ctrl->nr_of_stores; s < stores; s++) {
+		p[s].p = kcalloc(ctrl->elems, ctrl->elem_size, GFP_KERNEL);
+		if (p[s].p == NULL) {
+			while (s > ctrl->nr_of_stores)
+				kfree(p[--s].p);
+			kfree(p);
+			return -ENOMEM;
+		}
+		for (idx = 0; idx < ctrl->elems; idx++)
+			ctrl->type_ops->init(ctrl, idx, p[s]);
+	}
+	if (ctrl->p_stores)
+		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
+	kfree(ctrl->p_stores);
+	ctrl->p_stores = p;
+	ctrl->nr_of_stores = stores;
+	return 0;
+}
 
 
 /* Get extended controls. Allocates the helpers array if needed. */
@@ -2803,17 +2867,21 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 {
 	struct v4l2_ctrl_helper helper[4];
 	struct v4l2_ctrl_helper *helpers = helper;
+	unsigned store = 0;
 	int ret;
 	int i, j;
 
 	cs->error_idx = cs->count;
-	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+	if (V4L2_CTRL_ID2CLASS(cs->ctrl_class))
+		cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+	else
+		store = cs->config_store;
 
 	if (hdl == NULL)
 		return -EINVAL;
 
 	if (cs->count == 0)
-		return class_check(hdl, cs->ctrl_class);
+		return class_check(hdl, V4L2_CTRL_ID2CLASS(cs->ctrl_class));
 
 	if (cs->count > ARRAY_SIZE(helper)) {
 		helpers = kmalloc_array(cs->count, sizeof(helper[0]),
@@ -2843,13 +2911,19 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 		v4l2_ctrl_lock(master);
 
 		/* g_volatile_ctrl will update the new control values */
-		if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
-			(master->has_volatiles && !is_cur_manual(master))) {
+		if (store == 0 &&
+		    ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
+		     (master->has_volatiles && !is_cur_manual(master)))) {
 			for (j = 0; j < master->ncontrols; j++)
 				cur_to_new(master->cluster[j]);
 			ret = call_op(master, g_volatile_ctrl);
 			ctrl_to_user = new_to_user;
 		}
+		for (j = 0; j < master->ncontrols; j++)
+			if (!ret && master->cluster[j] &&
+			    store > master->cluster[j]->nr_of_stores)
+				ret = extend_store(master->cluster[j], store);
+
 		/* If OK, then copy the current (for non-volatile controls)
 		   or the new (for volatile controls) control values to the
 		   caller */
@@ -2857,7 +2931,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 			u32 idx = i;
 
 			do {
-				ret = ctrl_to_user(cs->controls + idx,
+				if (store)
+					ret = store_to_user(cs->controls + idx,
+						   helpers[idx].ctrl, store);
+				else
+					ret = ctrl_to_user(cs->controls + idx,
 						   helpers[idx].ctrl);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
@@ -2952,12 +3030,11 @@ s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
 }
 EXPORT_SYMBOL(v4l2_ctrl_g_ctrl_int64);
 
-
 /* Core function that calls try/s_ctrl and ensures that the new value is
    copied to the current value on a set.
    Must be called with ctrl->handler->lock held. */
 static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
-			      bool set, u32 ch_flags)
+			      u16 store, bool set, u32 ch_flags)
 {
 	bool update_flag;
 	int ret;
@@ -2973,6 +3050,14 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
 		if (ctrl == NULL)
 			continue;
 
+		if (store && !ctrl->can_store)
+			return -EINVAL;
+		if (store > ctrl->nr_of_stores) {
+			ret = extend_store(ctrl, store);
+			if (ret)
+				return ret;
+		}
+		ctrl->store = store;
 		if (!ctrl->is_new) {
 			cur_to_new(ctrl);
 			continue;
@@ -2994,9 +3079,13 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
 
 	/* If OK, then make the new values permanent. */
 	update_flag = is_cur_manual(master) != is_new_manual(master);
-	for (i = 0; i < master->ncontrols; i++)
-		new_to_cur(fh, master->cluster[i], ch_flags |
-			((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
+	for (i = 0; i < master->ncontrols; i++) {
+		if (store)
+			new_to_store(master->cluster[i]);
+		else
+			new_to_cur(fh, master->cluster[i], ch_flags |
+				((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
+	}
 	return 0;
 }
 
@@ -3036,8 +3125,12 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
 {
 	int i;
 
-	for (i = 0; i < master->ncontrols; i++)
+	for (i = 0; i < master->ncontrols; i++) {
+		if (master->cluster[i] == NULL)
+			continue;
 		cur_to_new(master->cluster[i]);
+		master->cluster[i]->store = 0;
+	}
 	if (!call_op(master, g_volatile_ctrl))
 		for (i = 1; i < master->ncontrols; i++)
 			if (master->cluster[i])
@@ -3051,17 +3144,21 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 {
 	struct v4l2_ctrl_helper helper[4];
 	struct v4l2_ctrl_helper *helpers = helper;
+	unsigned store = 0;
 	unsigned i, j;
 	int ret;
 
 	cs->error_idx = cs->count;
-	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+	if (V4L2_CTRL_ID2CLASS(cs->ctrl_class))
+		cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+	else
+		store = cs->config_store;
 
 	if (hdl == NULL)
 		return -EINVAL;
 
 	if (cs->count == 0)
-		return class_check(hdl, cs->ctrl_class);
+		return class_check(hdl, V4L2_CTRL_ID2CLASS(cs->ctrl_class));
 
 	if (cs->count > ARRAY_SIZE(helper)) {
 		helpers = kmalloc_array(cs->count, sizeof(helper[0]),
@@ -3096,7 +3193,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		   first since those will become the new manual values (which
 		   may be overwritten by explicit new values from this set
 		   of controls). */
-		if (master->is_auto && master->has_volatiles &&
+		if (!store && master->is_auto && master->has_volatiles &&
 						!is_cur_manual(master)) {
 			/* Pick an initial non-manual value */
 			s32 new_auto_val = master->manual_mode_value + 1;
@@ -3123,14 +3220,14 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		} while (!ret && idx);
 
 		if (!ret)
-			ret = try_or_set_cluster(fh, master, set, 0);
+			ret = try_or_set_cluster(fh, master, store, set, 0);
 
 		/* Copy the new values back to userspace. */
 		if (!ret) {
 			idx = i;
 			do {
-				ret = new_to_user(cs->controls + idx,
-						helpers[idx].ctrl);
+				ret = store_to_user(cs->controls + idx,
+						helpers[idx].ctrl, store);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
@@ -3175,9 +3272,12 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 	int i;
 
 	/* Reset the 'is_new' flags of the cluster */
-	for (i = 0; i < master->ncontrols; i++)
-		if (master->cluster[i])
-			master->cluster[i]->is_new = 0;
+	for (i = 0; i < master->ncontrols; i++) {
+		if (master->cluster[i] == NULL)
+			continue;
+		master->cluster[i]->is_new = 0;
+		master->cluster[i]->store = 0;
+	}
 
 	if (c)
 		user_to_new(c, ctrl);
@@ -3190,7 +3290,7 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 		update_from_auto_cluster(master);
 
 	ctrl->is_new = 1;
-	return try_or_set_cluster(fh, master, true, ch_flags);
+	return try_or_set_cluster(fh, master, 0, true, ch_flags);
 }
 
 /* Helper function for VIDIOC_S_CTRL compatibility */
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 46f4c04..628852c 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -533,8 +533,8 @@ static void v4l_print_query_ext_ctrl(const void *arg, bool write_only)
 	pr_cont("id=0x%x, type=%d, name=%.*s, min/max=%lld/%lld, "
 		"step=%lld, default=%lld, flags=0x%08x, elem_size=%u, elems=%u, "
 		"nr_of_dims=%u, dims=%u,%u,%u,%u\n",
-			p->id, p->type, (int)sizeof(p->name), p->name,
-			p->minimum, p->maximum,
+			p->id, p->type, (int)sizeof(p->name),
+			p->name, p->minimum, p->maximum,
 			p->step, p->default_value, p->flags,
 			p->elem_size, p->elems, p->nr_of_dims,
 			p->dims[0], p->dims[1], p->dims[2], p->dims[3]);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index b7cd7a6..4c31688 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -122,6 +122,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
   *		Drivers should never touch this flag.
   * @call_notify: If set, then call the handler's notify function whenever the
   *		control's value changes.
+  * @can_store: If set, then this control supports configuration stores.
   * @manual_mode_value: If the is_auto flag is set, then this is the value
   *		of the auto control that determines if that control is in
   *		manual mode. So if the value of the auto control equals this
@@ -140,6 +141,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
   * @elem_size:	The size in bytes of the control.
   * @dims:	The size of each dimension.
   * @nr_of_dims:The number of dimensions in @dims.
+  * @nr_of_stores: The number of allocated configuration stores of this control.
+  * @store:	The configuration store that the control op operates on.
   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
   *		easy to skip menu items that are not valid. If bit X is set,
   *		then menu item X is skipped. Of course, this only works for
@@ -179,6 +182,7 @@ struct v4l2_ctrl {
 	unsigned int is_array:1;
 	unsigned int has_volatiles:1;
 	unsigned int call_notify:1;
+	unsigned int can_store:1;
 	unsigned int manual_mode_value:8;
 
 	const struct v4l2_ctrl_ops *ops;
@@ -191,6 +195,8 @@ struct v4l2_ctrl {
 	u32 elem_size;
 	u32 dims[V4L2_CTRL_MAX_DIMS];
 	u32 nr_of_dims;
+	u16 nr_of_stores;
+	u16 store;
 	union {
 		u64 step;
 		u64 menu_skip_mask;
@@ -208,6 +214,7 @@ struct v4l2_ctrl {
 
 	union v4l2_ctrl_ptr p_new;
 	union v4l2_ctrl_ptr p_cur;
+	union v4l2_ctrl_ptr *p_stores;
 };
 
 /** struct v4l2_ctrl_ref - The control reference.
@@ -284,6 +291,7 @@ struct v4l2_ctrl_handler {
   *		must be NULL.
   * @is_private: If set, then this control is private to its handler and it
   *		will not be added to any other handlers.
+  * @can_store: If set, then this control supports configuration stores.
   */
 struct v4l2_ctrl_config {
 	const struct v4l2_ctrl_ops *ops;
@@ -302,6 +310,7 @@ struct v4l2_ctrl_config {
 	const char * const *qmenu;
 	const s64 *qmenu_int;
 	unsigned int is_private:1;
+	unsigned int can_store:1;
 };
 
 /** v4l2_ctrl_fill() - Fill in the control fields based on the control ID.
@@ -763,6 +772,11 @@ static inline int v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
 	return rval;
 }
 
+static inline void v4l2_ctrl_can_store(struct v4l2_ctrl *ctrl)
+{
+	ctrl->can_store = true;
+}
+
 /* Internal helper functions that deal with control events. */
 extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
 void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
-- 
2.1.0

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Sakari Ailus]

Hi Hans,

Some comments below.

On Sun, Sep 21, 2014 at 04:48:22PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 150 +++++++++++++++++++++++++++++------
>  drivers/media/v4l2-core/v4l2-ioctl.c |   4 +-
>  include/media/v4l2-ctrls.h           |  14 ++++
>  3 files changed, 141 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 35d1f3d..df0f3ee 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1478,6 +1478,15 @@ static int cur_to_user(struct v4l2_ext_control *c,
>  	return ptr_to_user(c, ctrl, ctrl->p_cur);
>  }
>  
> +/* Helper function: copy the store's control value back to the caller */
> +static int store_to_user(struct v4l2_ext_control *c,
> +		       struct v4l2_ctrl *ctrl, unsigned store)
> +{
> +	if (store == 0)
> +		return ptr_to_user(c, ctrl, ctrl->p_new);
> +	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
> +}
> +
>  /* Helper function: copy the new control value back to the caller */
>  static int new_to_user(struct v4l2_ext_control *c,
>  		       struct v4l2_ctrl *ctrl)
> @@ -1585,6 +1594,14 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>  	}
>  }
>  
> +/* Helper function: copy the new control value to the store */
> +static void new_to_store(struct v4l2_ctrl *ctrl)
> +{
> +	/* has_changed is set by cluster_changed */
> +	if (ctrl && ctrl->has_changed)
> +		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
> +}
> +
>  /* Copy the current value to the new value */
>  static void cur_to_new(struct v4l2_ctrl *ctrl)
>  {
> @@ -1603,13 +1620,18 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  
>  	for (i = 0; i < master->ncontrols; i++) {
>  		struct v4l2_ctrl *ctrl = master->cluster[i];
> +		union v4l2_ctrl_ptr ptr;
>  		bool ctrl_changed = false;
>  
>  		if (ctrl == NULL)
>  			continue;
> +		if (ctrl->store)
> +			ptr = ctrl->p_stores[ctrl->store - 1];
> +		else
> +			ptr = ctrl->p_cur;
>  		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
>  			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
> -				ctrl->p_cur, ctrl->p_new);
> +				ptr, ctrl->p_new);
>  		ctrl->has_changed = ctrl_changed;
>  		changed |= ctrl->has_changed;
>  	}
> @@ -1740,6 +1762,13 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  		list_del(&ctrl->node);
>  		list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
>  			list_del(&sev->node);
> +		if (ctrl->p_stores) {
> +			unsigned s;
> +
> +			for (s = 0; s < ctrl->nr_of_stores; s++)
> +				kfree(ctrl->p_stores[s].p);
> +		}
> +		kfree(ctrl->p_stores);
>  		kfree(ctrl);
>  	}
>  	kfree(hdl->buckets);
> @@ -1970,7 +1999,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  		handler_set_err(hdl, -ERANGE);
>  		return NULL;
>  	}
> -	if (is_array &&
> +	if ((is_array || (flags & V4L2_CTRL_FLAG_CAN_STORE)) &&
>  	    (type == V4L2_CTRL_TYPE_BUTTON ||
>  	     type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
>  		handler_set_err(hdl, -EINVAL);
> @@ -2084,8 +2113,10 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>  			is_menu ? cfg->menu_skip_mask : step, def,
>  			cfg->dims, cfg->elem_size,
>  			flags, qmenu, qmenu_int, priv);
> -	if (ctrl)
> +	if (ctrl) {

I think it'd be cleaner to return NULL here if ctrl == NULL. Up to you.

>  		ctrl->is_private = cfg->is_private;
> +		ctrl->can_store = cfg->can_store;
> +	}
>  	return ctrl;
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_custom);
> @@ -2452,6 +2483,7 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
>  				cur_to_new(master->cluster[i]);
>  				master->cluster[i]->is_new = 1;
>  				master->cluster[i]->done = true;
> +				master->cluster[i]->store = 0;
>  			}
>  		}
>  		ret = call_op(master, s_ctrl);
> @@ -2539,6 +2571,8 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>  		qc->id = ctrl->id;
>  	strlcpy(qc->name, ctrl->name, sizeof(qc->name));
>  	qc->flags = ctrl->flags;
> +	if (ctrl->can_store)
> +		qc->flags |= V4L2_CTRL_FLAG_CAN_STORE;
>  	qc->type = ctrl->type;
>  	if (ctrl->is_ptr)
>  		qc->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
> @@ -2700,6 +2734,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>  			     struct v4l2_ctrl_helper *helpers,
>  			     bool get)
>  {
> +	u32 ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
> +	unsigned store = cs->config_store & 0xffff;
>  	struct v4l2_ctrl_helper *h;
>  	bool have_clusters = false;
>  	u32 i;
> @@ -2712,7 +2748,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>  
>  		cs->error_idx = i;
>  
> -		if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
> +		if (ctrl_class && V4L2_CTRL_ID2CLASS(id) != ctrl_class)
>  			return -EINVAL;
>  
>  		/* Old-style private controls are not allowed for
> @@ -2725,6 +2761,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>  		ctrl = ref->ctrl;
>  		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
>  			return -EINVAL;
> +		if (store && !ctrl->can_store)
> +			return -EINVAL;
>  
>  		if (ctrl->cluster[0]->ncontrols > 1)
>  			have_clusters = true;
> @@ -2796,6 +2834,32 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
>  	return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
>  }
>  
> +static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)

What limits the number of stores? In my opinion 2^16 - 1 is just a tad too
many... I think it'll be always easier to extend this rather than shrink it.
Also the user shouldn't be allowed to allocate obscene amounts of memory for
something like this.

I might limit this to 255 for instance.

> +{
> +	unsigned s, idx;
> +	union v4l2_ctrl_ptr *p;
> +
> +	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
> +	if (p == NULL)
> +		return -ENOMEM;
> +	for (s = ctrl->nr_of_stores; s < stores; s++) {
> +		p[s].p = kcalloc(ctrl->elems, ctrl->elem_size, GFP_KERNEL);
> +		if (p[s].p == NULL) {
> +			while (s > ctrl->nr_of_stores)
> +				kfree(p[--s].p);
> +			kfree(p);
> +			return -ENOMEM;
> +		}
> +		for (idx = 0; idx < ctrl->elems; idx++)
> +			ctrl->type_ops->init(ctrl, idx, p[s]);
> +	}
> +	if (ctrl->p_stores)
> +		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));

Please consider wrapping the line. I'd might use sizeof(*p) instead.

> +	kfree(ctrl->p_stores);
> +	ctrl->p_stores = p;
> +	ctrl->nr_of_stores = stores;
> +	return 0;
> +}
>  
>  
>  /* Get extended controls. Allocates the helpers array if needed. */
> @@ -2803,17 +2867,21 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
>  {
>  	struct v4l2_ctrl_helper helper[4];
>  	struct v4l2_ctrl_helper *helpers = helper;
> +	unsigned store = 0;
>  	int ret;
>  	int i, j;
>  
>  	cs->error_idx = cs->count;
> -	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
> +	if (V4L2_CTRL_ID2CLASS(cs->ctrl_class))
> +		cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
> +	else
> +		store = cs->config_store;
>  
>  	if (hdl == NULL)
>  		return -EINVAL;
>  
>  	if (cs->count == 0)
> -		return class_check(hdl, cs->ctrl_class);
> +		return class_check(hdl, V4L2_CTRL_ID2CLASS(cs->ctrl_class));
>  
>  	if (cs->count > ARRAY_SIZE(helper)) {
>  		helpers = kmalloc_array(cs->count, sizeof(helper[0]),
> @@ -2843,13 +2911,19 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
>  		v4l2_ctrl_lock(master);
>  
>  		/* g_volatile_ctrl will update the new control values */
> -		if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
> -			(master->has_volatiles && !is_cur_manual(master))) {
> +		if (store == 0 &&
> +		    ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
> +		     (master->has_volatiles && !is_cur_manual(master)))) {
>  			for (j = 0; j < master->ncontrols; j++)
>  				cur_to_new(master->cluster[j]);
>  			ret = call_op(master, g_volatile_ctrl);
>  			ctrl_to_user = new_to_user;
>  		}
> +		for (j = 0; j < master->ncontrols; j++)
> +			if (!ret && master->cluster[j] &&
> +			    store > master->cluster[j]->nr_of_stores)
> +				ret = extend_store(master->cluster[j], store);
> +
>  		/* If OK, then copy the current (for non-volatile controls)
>  		   or the new (for volatile controls) control values to the
>  		   caller */
> @@ -2857,7 +2931,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
>  			u32 idx = i;
>  
>  			do {
> -				ret = ctrl_to_user(cs->controls + idx,
> +				if (store)
> +					ret = store_to_user(cs->controls + idx,
> +						   helpers[idx].ctrl, store);
> +				else
> +					ret = ctrl_to_user(cs->controls + idx,
>  						   helpers[idx].ctrl);
>  				idx = helpers[idx].next;
>  			} while (!ret && idx);
> @@ -2952,12 +3030,11 @@ s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_g_ctrl_int64);
>  
> -
>  /* Core function that calls try/s_ctrl and ensures that the new value is
>     copied to the current value on a set.
>     Must be called with ctrl->handler->lock held. */
>  static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
> -			      bool set, u32 ch_flags)
> +			      u16 store, bool set, u32 ch_flags)
>  {
>  	bool update_flag;
>  	int ret;
> @@ -2973,6 +3050,14 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
>  		if (ctrl == NULL)
>  			continue;
>  
> +		if (store && !ctrl->can_store)
> +			return -EINVAL;
> +		if (store > ctrl->nr_of_stores) {
> +			ret = extend_store(ctrl, store);
> +			if (ret)
> +				return ret;
> +		}
> +		ctrl->store = store;
>  		if (!ctrl->is_new) {
>  			cur_to_new(ctrl);
>  			continue;
> @@ -2994,9 +3079,13 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
>  
>  	/* If OK, then make the new values permanent. */
>  	update_flag = is_cur_manual(master) != is_new_manual(master);
> -	for (i = 0; i < master->ncontrols; i++)
> -		new_to_cur(fh, master->cluster[i], ch_flags |
> -			((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> +	for (i = 0; i < master->ncontrols; i++) {
> +		if (store)
> +			new_to_store(master->cluster[i]);
> +		else
> +			new_to_cur(fh, master->cluster[i], ch_flags |
> +				((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> +	}
>  	return 0;
>  }
>  
> @@ -3036,8 +3125,12 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
>  {
>  	int i;
>  
> -	for (i = 0; i < master->ncontrols; i++)
> +	for (i = 0; i < master->ncontrols; i++) {
> +		if (master->cluster[i] == NULL)
> +			continue;
>  		cur_to_new(master->cluster[i]);
> +		master->cluster[i]->store = 0;
> +	}
>  	if (!call_op(master, g_volatile_ctrl))
>  		for (i = 1; i < master->ncontrols; i++)
>  			if (master->cluster[i])
> @@ -3051,17 +3144,21 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  {
>  	struct v4l2_ctrl_helper helper[4];
>  	struct v4l2_ctrl_helper *helpers = helper;
> +	unsigned store = 0;
>  	unsigned i, j;
>  	int ret;
>  
>  	cs->error_idx = cs->count;
> -	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
> +	if (V4L2_CTRL_ID2CLASS(cs->ctrl_class))
> +		cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
> +	else
> +		store = cs->config_store;
>  
>  	if (hdl == NULL)
>  		return -EINVAL;
>  
>  	if (cs->count == 0)
> -		return class_check(hdl, cs->ctrl_class);
> +		return class_check(hdl, V4L2_CTRL_ID2CLASS(cs->ctrl_class));
>  
>  	if (cs->count > ARRAY_SIZE(helper)) {
>  		helpers = kmalloc_array(cs->count, sizeof(helper[0]),
> @@ -3096,7 +3193,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  		   first since those will become the new manual values (which
>  		   may be overwritten by explicit new values from this set
>  		   of controls). */
> -		if (master->is_auto && master->has_volatiles &&
> +		if (!store && master->is_auto && master->has_volatiles &&
>  						!is_cur_manual(master)) {
>  			/* Pick an initial non-manual value */
>  			s32 new_auto_val = master->manual_mode_value + 1;
> @@ -3123,14 +3220,14 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  		} while (!ret && idx);
>  
>  		if (!ret)
> -			ret = try_or_set_cluster(fh, master, set, 0);
> +			ret = try_or_set_cluster(fh, master, store, set, 0);
>  
>  		/* Copy the new values back to userspace. */
>  		if (!ret) {
>  			idx = i;
>  			do {
> -				ret = new_to_user(cs->controls + idx,
> -						helpers[idx].ctrl);
> +				ret = store_to_user(cs->controls + idx,
> +						helpers[idx].ctrl, store);
>  				idx = helpers[idx].next;
>  			} while (!ret && idx);
>  		}
> @@ -3175,9 +3272,12 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>  	int i;
>  
>  	/* Reset the 'is_new' flags of the cluster */
> -	for (i = 0; i < master->ncontrols; i++)
> -		if (master->cluster[i])
> -			master->cluster[i]->is_new = 0;
> +	for (i = 0; i < master->ncontrols; i++) {
> +		if (master->cluster[i] == NULL)
> +			continue;
> +		master->cluster[i]->is_new = 0;
> +		master->cluster[i]->store = 0;
> +	}
>  
>  	if (c)
>  		user_to_new(c, ctrl);
> @@ -3190,7 +3290,7 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>  		update_from_auto_cluster(master);
>  
>  	ctrl->is_new = 1;
> -	return try_or_set_cluster(fh, master, true, ch_flags);
> +	return try_or_set_cluster(fh, master, 0, true, ch_flags);
>  }
>  
>  /* Helper function for VIDIOC_S_CTRL compatibility */
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 46f4c04..628852c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -533,8 +533,8 @@ static void v4l_print_query_ext_ctrl(const void *arg, bool write_only)
>  	pr_cont("id=0x%x, type=%d, name=%.*s, min/max=%lld/%lld, "
>  		"step=%lld, default=%lld, flags=0x%08x, elem_size=%u, elems=%u, "
>  		"nr_of_dims=%u, dims=%u,%u,%u,%u\n",
> -			p->id, p->type, (int)sizeof(p->name), p->name,
> -			p->minimum, p->maximum,
> +			p->id, p->type, (int)sizeof(p->name),
> +			p->name, p->minimum, p->maximum,
>  			p->step, p->default_value, p->flags,
>  			p->elem_size, p->elems, p->nr_of_dims,
>  			p->dims[0], p->dims[1], p->dims[2], p->dims[3]);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index b7cd7a6..4c31688 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -122,6 +122,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>    *		Drivers should never touch this flag.
>    * @call_notify: If set, then call the handler's notify function whenever the
>    *		control's value changes.
> +  * @can_store: If set, then this control supports configuration stores.
>    * @manual_mode_value: If the is_auto flag is set, then this is the value
>    *		of the auto control that determines if that control is in
>    *		manual mode. So if the value of the auto control equals this
> @@ -140,6 +141,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>    * @elem_size:	The size in bytes of the control.
>    * @dims:	The size of each dimension.
>    * @nr_of_dims:The number of dimensions in @dims.
> +  * @nr_of_stores: The number of allocated configuration stores of this control.
> +  * @store:	The configuration store that the control op operates on.
>    * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>    *		easy to skip menu items that are not valid. If bit X is set,
>    *		then menu item X is skipped. Of course, this only works for
> @@ -179,6 +182,7 @@ struct v4l2_ctrl {
>  	unsigned int is_array:1;
>  	unsigned int has_volatiles:1;
>  	unsigned int call_notify:1;
> +	unsigned int can_store:1;
>  	unsigned int manual_mode_value:8;
>  
>  	const struct v4l2_ctrl_ops *ops;
> @@ -191,6 +195,8 @@ struct v4l2_ctrl {
>  	u32 elem_size;
>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>  	u32 nr_of_dims;
> +	u16 nr_of_stores;
> +	u16 store;
>  	union {
>  		u64 step;
>  		u64 menu_skip_mask;
> @@ -208,6 +214,7 @@ struct v4l2_ctrl {
>  
>  	union v4l2_ctrl_ptr p_new;
>  	union v4l2_ctrl_ptr p_cur;
> +	union v4l2_ctrl_ptr *p_stores;
>  };
>  
>  /** struct v4l2_ctrl_ref - The control reference.
> @@ -284,6 +291,7 @@ struct v4l2_ctrl_handler {
>    *		must be NULL.
>    * @is_private: If set, then this control is private to its handler and it
>    *		will not be added to any other handlers.
> +  * @can_store: If set, then this control supports configuration stores.
>    */
>  struct v4l2_ctrl_config {
>  	const struct v4l2_ctrl_ops *ops;
> @@ -302,6 +310,7 @@ struct v4l2_ctrl_config {
>  	const char * const *qmenu;
>  	const s64 *qmenu_int;
>  	unsigned int is_private:1;
> +	unsigned int can_store:1;
>  };
>  
>  /** v4l2_ctrl_fill() - Fill in the control fields based on the control ID.
> @@ -763,6 +772,11 @@ static inline int v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
>  	return rval;
>  }
>  
> +static inline void v4l2_ctrl_can_store(struct v4l2_ctrl *ctrl)
> +{
> +	ctrl->can_store = true;
> +}
> +
>  /* Internal helper functions that deal with control events. */
>  extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
>  void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);

-- 
Kind regards,

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

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Hans Verkuil]

On 11/14/2014 04:44 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Some comments below.
> 
> On Sun, Sep 21, 2014 at 04:48:22PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 150 +++++++++++++++++++++++++++++------
>>  drivers/media/v4l2-core/v4l2-ioctl.c |   4 +-
>>  include/media/v4l2-ctrls.h           |  14 ++++
>>  3 files changed, 141 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 35d1f3d..df0f3ee 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1478,6 +1478,15 @@ static int cur_to_user(struct v4l2_ext_control *c,
>>  	return ptr_to_user(c, ctrl, ctrl->p_cur);
>>  }
>>  
>> +/* Helper function: copy the store's control value back to the caller */
>> +static int store_to_user(struct v4l2_ext_control *c,
>> +		       struct v4l2_ctrl *ctrl, unsigned store)
>> +{
>> +	if (store == 0)
>> +		return ptr_to_user(c, ctrl, ctrl->p_new);
>> +	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
>> +}
>> +
>>  /* Helper function: copy the new control value back to the caller */
>>  static int new_to_user(struct v4l2_ext_control *c,
>>  		       struct v4l2_ctrl *ctrl)
>> @@ -1585,6 +1594,14 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>>  	}
>>  }
>>  
>> +/* Helper function: copy the new control value to the store */
>> +static void new_to_store(struct v4l2_ctrl *ctrl)
>> +{
>> +	/* has_changed is set by cluster_changed */
>> +	if (ctrl && ctrl->has_changed)
>> +		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
>> +}
>> +
>>  /* Copy the current value to the new value */
>>  static void cur_to_new(struct v4l2_ctrl *ctrl)
>>  {
>> @@ -1603,13 +1620,18 @@ static int cluster_changed(struct v4l2_ctrl *master)
>>  
>>  	for (i = 0; i < master->ncontrols; i++) {
>>  		struct v4l2_ctrl *ctrl = master->cluster[i];
>> +		union v4l2_ctrl_ptr ptr;
>>  		bool ctrl_changed = false;
>>  
>>  		if (ctrl == NULL)
>>  			continue;
>> +		if (ctrl->store)
>> +			ptr = ctrl->p_stores[ctrl->store - 1];
>> +		else
>> +			ptr = ctrl->p_cur;
>>  		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
>>  			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
>> -				ctrl->p_cur, ctrl->p_new);
>> +				ptr, ctrl->p_new);
>>  		ctrl->has_changed = ctrl_changed;
>>  		changed |= ctrl->has_changed;
>>  	}
>> @@ -1740,6 +1762,13 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>>  		list_del(&ctrl->node);
>>  		list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
>>  			list_del(&sev->node);
>> +		if (ctrl->p_stores) {
>> +			unsigned s;
>> +
>> +			for (s = 0; s < ctrl->nr_of_stores; s++)
>> +				kfree(ctrl->p_stores[s].p);
>> +		}
>> +		kfree(ctrl->p_stores);
>>  		kfree(ctrl);
>>  	}
>>  	kfree(hdl->buckets);
>> @@ -1970,7 +1999,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>  		handler_set_err(hdl, -ERANGE);
>>  		return NULL;
>>  	}
>> -	if (is_array &&
>> +	if ((is_array || (flags & V4L2_CTRL_FLAG_CAN_STORE)) &&
>>  	    (type == V4L2_CTRL_TYPE_BUTTON ||
>>  	     type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
>>  		handler_set_err(hdl, -EINVAL);
>> @@ -2084,8 +2113,10 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>  			is_menu ? cfg->menu_skip_mask : step, def,
>>  			cfg->dims, cfg->elem_size,
>>  			flags, qmenu, qmenu_int, priv);
>> -	if (ctrl)
>> +	if (ctrl) {
> 
> I think it'd be cleaner to return NULL here if ctrl == NULL. Up to you.

Agreed.

> 
>>  		ctrl->is_private = cfg->is_private;
>> +		ctrl->can_store = cfg->can_store;
>> +	}
>>  	return ctrl;
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_custom);
>> @@ -2452,6 +2483,7 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
>>  				cur_to_new(master->cluster[i]);
>>  				master->cluster[i]->is_new = 1;
>>  				master->cluster[i]->done = true;
>> +				master->cluster[i]->store = 0;
>>  			}
>>  		}
>>  		ret = call_op(master, s_ctrl);
>> @@ -2539,6 +2571,8 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>>  		qc->id = ctrl->id;
>>  	strlcpy(qc->name, ctrl->name, sizeof(qc->name));
>>  	qc->flags = ctrl->flags;
>> +	if (ctrl->can_store)
>> +		qc->flags |= V4L2_CTRL_FLAG_CAN_STORE;
>>  	qc->type = ctrl->type;
>>  	if (ctrl->is_ptr)
>>  		qc->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
>> @@ -2700,6 +2734,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>>  			     struct v4l2_ctrl_helper *helpers,
>>  			     bool get)
>>  {
>> +	u32 ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
>> +	unsigned store = cs->config_store & 0xffff;
>>  	struct v4l2_ctrl_helper *h;
>>  	bool have_clusters = false;
>>  	u32 i;
>> @@ -2712,7 +2748,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>>  
>>  		cs->error_idx = i;
>>  
>> -		if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
>> +		if (ctrl_class && V4L2_CTRL_ID2CLASS(id) != ctrl_class)
>>  			return -EINVAL;
>>  
>>  		/* Old-style private controls are not allowed for
>> @@ -2725,6 +2761,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>>  		ctrl = ref->ctrl;
>>  		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
>>  			return -EINVAL;
>> +		if (store && !ctrl->can_store)
>> +			return -EINVAL;
>>  
>>  		if (ctrl->cluster[0]->ncontrols > 1)
>>  			have_clusters = true;
>> @@ -2796,6 +2834,32 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
>>  	return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
>>  }
>>  
>> +static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
> 
> What limits the number of stores? In my opinion 2^16 - 1 is just a tad too
> many... I think it'll be always easier to extend this rather than shrink it.
> Also the user shouldn't be allowed to allocate obscene amounts of memory for
> something like this.
> 
> I might limit this to 255 for instance.

My plan (not part of this patch series) was to have a default limit (probably
VIDEO_MAX_FRAME) that drivers can override. I suspect that bridge drivers may
need to be able to set this limit for all subdev drivers as well, but we'll
see how that will work out. All the information necessary is available, so if
we need it, it wouldn't be too difficult.

But it certainly will have to be limited.

> 
>> +{
>> +	unsigned s, idx;
>> +	union v4l2_ctrl_ptr *p;
>> +
>> +	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
>> +	if (p == NULL)
>> +		return -ENOMEM;
>> +	for (s = ctrl->nr_of_stores; s < stores; s++) {
>> +		p[s].p = kcalloc(ctrl->elems, ctrl->elem_size, GFP_KERNEL);
>> +		if (p[s].p == NULL) {
>> +			while (s > ctrl->nr_of_stores)
>> +				kfree(p[--s].p);
>> +			kfree(p);
>> +			return -ENOMEM;
>> +		}
>> +		for (idx = 0; idx < ctrl->elems; idx++)
>> +			ctrl->type_ops->init(ctrl, idx, p[s]);
>> +	}
>> +	if (ctrl->p_stores)
>> +		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
> 
> Please consider wrapping the line. I'd might use sizeof(*p) instead.

Agreed.

> 
>> +	kfree(ctrl->p_stores);
>> +	ctrl->p_stores = p;
>> +	ctrl->nr_of_stores = stores;
>> +	return 0;
>> +}

Regards,

	Hans

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Enric Balletbo Serra]

Dear Hans,

2014-11-17 9:46 GMT+01:00 Hans Verkuil <hverkuil@xs4all.nl>:
> On 11/14/2014 04:44 PM, Sakari Ailus wrote:
>> Hi Hans,
>>
>> Some comments below.
>>
>> On Sun, Sep 21, 2014 at 04:48:22PM +0200, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 150 +++++++++++++++++++++++++++++------
>>>  drivers/media/v4l2-core/v4l2-ioctl.c |   4 +-
>>>  include/media/v4l2-ctrls.h           |  14 ++++
>>>  3 files changed, 141 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index 35d1f3d..df0f3ee 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -1478,6 +1478,15 @@ static int cur_to_user(struct v4l2_ext_control *c,
>>>      return ptr_to_user(c, ctrl, ctrl->p_cur);
>>>  }
>>>
>>> +/* Helper function: copy the store's control value back to the caller */
>>> +static int store_to_user(struct v4l2_ext_control *c,
>>> +                   struct v4l2_ctrl *ctrl, unsigned store)
>>> +{
>>> +    if (store == 0)
>>> +            return ptr_to_user(c, ctrl, ctrl->p_new);
>>> +    return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
>>> +}
>>> +
>>>  /* Helper function: copy the new control value back to the caller */
>>>  static int new_to_user(struct v4l2_ext_control *c,
>>>                     struct v4l2_ctrl *ctrl)
>>> @@ -1585,6 +1594,14 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>>>      }
>>>  }
>>>
>>> +/* Helper function: copy the new control value to the store */
>>> +static void new_to_store(struct v4l2_ctrl *ctrl)
>>> +{
>>> +    /* has_changed is set by cluster_changed */
>>> +    if (ctrl && ctrl->has_changed)
>>> +            ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
>>> +}
>>> +
>>>  /* Copy the current value to the new value */
>>>  static void cur_to_new(struct v4l2_ctrl *ctrl)
>>>  {
>>> @@ -1603,13 +1620,18 @@ static int cluster_changed(struct v4l2_ctrl *master)
>>>
>>>      for (i = 0; i < master->ncontrols; i++) {
>>>              struct v4l2_ctrl *ctrl = master->cluster[i];
>>> +            union v4l2_ctrl_ptr ptr;
>>>              bool ctrl_changed = false;
>>>
>>>              if (ctrl == NULL)
>>>                      continue;
>>> +            if (ctrl->store)
>>> +                    ptr = ctrl->p_stores[ctrl->store - 1];
>>> +            else
>>> +                    ptr = ctrl->p_cur;
>>>              for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
>>>                      ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
>>> -                            ctrl->p_cur, ctrl->p_new);
>>> +                            ptr, ctrl->p_new);
>>>              ctrl->has_changed = ctrl_changed;
>>>              changed |= ctrl->has_changed;
>>>      }
>>> @@ -1740,6 +1762,13 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>>>              list_del(&ctrl->node);
>>>              list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
>>>                      list_del(&sev->node);
>>> +            if (ctrl->p_stores) {
>>> +                    unsigned s;
>>> +
>>> +                    for (s = 0; s < ctrl->nr_of_stores; s++)
>>> +                            kfree(ctrl->p_stores[s].p);
>>> +            }
>>> +            kfree(ctrl->p_stores);
>>>              kfree(ctrl);
>>>      }
>>>      kfree(hdl->buckets);
>>> @@ -1970,7 +1999,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>              handler_set_err(hdl, -ERANGE);
>>>              return NULL;
>>>      }
>>> -    if (is_array &&
>>> +    if ((is_array || (flags & V4L2_CTRL_FLAG_CAN_STORE)) &&
>>>          (type == V4L2_CTRL_TYPE_BUTTON ||
>>>           type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
>>>              handler_set_err(hdl, -EINVAL);
>>> @@ -2084,8 +2113,10 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>>                      is_menu ? cfg->menu_skip_mask : step, def,
>>>                      cfg->dims, cfg->elem_size,
>>>                      flags, qmenu, qmenu_int, priv);
>>> -    if (ctrl)
>>> +    if (ctrl) {
>>
>> I think it'd be cleaner to return NULL here if ctrl == NULL. Up to you.
>
> Agreed.
>
>>
>>>              ctrl->is_private = cfg->is_private;
>>> +            ctrl->can_store = cfg->can_store;
>>> +    }
>>>      return ctrl;
>>>  }
>>>  EXPORT_SYMBOL(v4l2_ctrl_new_custom);
>>> @@ -2452,6 +2483,7 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
>>>                              cur_to_new(master->cluster[i]);
>>>                              master->cluster[i]->is_new = 1;
>>>                              master->cluster[i]->done = true;
>>> +                            master->cluster[i]->store = 0;
>>>                      }
>>>              }
>>>              ret = call_op(master, s_ctrl);
>>> @@ -2539,6 +2571,8 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>>>              qc->id = ctrl->id;
>>>      strlcpy(qc->name, ctrl->name, sizeof(qc->name));
>>>      qc->flags = ctrl->flags;
>>> +    if (ctrl->can_store)
>>> +            qc->flags |= V4L2_CTRL_FLAG_CAN_STORE;
>>>      qc->type = ctrl->type;
>>>      if (ctrl->is_ptr)
>>>              qc->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
>>> @@ -2700,6 +2734,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>>>                           struct v4l2_ctrl_helper *helpers,
>>>                           bool get)
>>>  {
>>> +    u32 ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
>>> +    unsigned store = cs->config_store & 0xffff;
>>>      struct v4l2_ctrl_helper *h;
>>>      bool have_clusters = false;
>>>      u32 i;
>>> @@ -2712,7 +2748,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>>>
>>>              cs->error_idx = i;
>>>
>>> -            if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
>>> +            if (ctrl_class && V4L2_CTRL_ID2CLASS(id) != ctrl_class)
>>>                      return -EINVAL;
>>>
>>>              /* Old-style private controls are not allowed for
>>> @@ -2725,6 +2761,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>>>              ctrl = ref->ctrl;
>>>              if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
>>>                      return -EINVAL;
>>> +            if (store && !ctrl->can_store)
>>> +                    return -EINVAL;
>>>
>>>              if (ctrl->cluster[0]->ncontrols > 1)
>>>                      have_clusters = true;
>>> @@ -2796,6 +2834,32 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
>>>      return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
>>>  }
>>>
>>> +static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
>>
>> What limits the number of stores? In my opinion 2^16 - 1 is just a tad too
>> many... I think it'll be always easier to extend this rather than shrink it.
>> Also the user shouldn't be allowed to allocate obscene amounts of memory for
>> something like this.
>>
>> I might limit this to 255 for instance.
>
> My plan (not part of this patch series) was to have a default limit (probably
> VIDEO_MAX_FRAME) that drivers can override. I suspect that bridge drivers may
> need to be able to set this limit for all subdev drivers as well, but we'll
> see how that will work out. All the information necessary is available, so if
> we need it, it wouldn't be too difficult.
>
> But it certainly will have to be limited.
>
>>
>>> +{
>>> +    unsigned s, idx;
>>> +    union v4l2_ctrl_ptr *p;
>>> +
>>> +    p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
>>> +    if (p == NULL)
>>> +            return -ENOMEM;
>>> +    for (s = ctrl->nr_of_stores; s < stores; s++) {
>>> +            p[s].p = kcalloc(ctrl->elems, ctrl->elem_size, GFP_KERNEL);
>>> +            if (p[s].p == NULL) {
>>> +                    while (s > ctrl->nr_of_stores)
>>> +                            kfree(p[--s].p);
>>> +                    kfree(p);
>>> +                    return -ENOMEM;
>>> +            }
>>> +            for (idx = 0; idx < ctrl->elems; idx++)
>>> +                    ctrl->type_ops->init(ctrl, idx, p[s]);
>>> +    }
>>> +    if (ctrl->p_stores)
>>> +            memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
>>
>> Please consider wrapping the line. I'd might use sizeof(*p) instead.
>
> Agreed.
>
>>
>>> +    kfree(ctrl->p_stores);
>>> +    ctrl->p_stores = p;
>>> +    ctrl->nr_of_stores = stores;
>>> +    return 0;
>>> +}
>
> Regards,
>
>         Hans
>

We've a driver that uses your confstore stuff and we'd like to push
upstream. I'm wondering if there is any plan to upstream the confstore
patches or if this was abandoned for some reason. Thanks

Regards,

  Enric


> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Hans Verkuil]

On 12/02/15 13:03, Enric Balletbo Serra wrote:
> Dear Hans,
> 
> We've a driver that uses your confstore stuff and we'd like to push
> upstream. I'm wondering if there is any plan to upstream the confstore
> patches or if this was abandoned for some reason. Thanks

Ouch, that's really old code you're using.

The latest version is here:

http://git.linuxtv.org/hverkuil/media_tree.git/log/?h=requests

But that too won't be the final version.

There is still work going on in this area (specifically by Laurent Pinchart)
since we really need this functionality. But we need to make sure that the
API is good enough to handle complex hardware before it can be upstreamed.

I suspect that once Laurent has it working for his non-trivial use-case we
can start looking at upstreaming it.

I recommend rebasing to at least the version in my git tree as that will
be much closer to the final version. I'll try to rebase that branch to
the latest kernel, but that's a bit difficult and takes more time than I
have at the moment.

Regards,

	Hans

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Enric Balletbo Serra]

2015-12-02 13:33 GMT+01:00 Hans Verkuil <hverkuil@xs4all.nl>:
> On 12/02/15 13:03, Enric Balletbo Serra wrote:
>> Dear Hans,
>>
>> We've a driver that uses your confstore stuff and we'd like to push
>> upstream. I'm wondering if there is any plan to upstream the confstore
>> patches or if this was abandoned for some reason. Thanks
>
> Ouch, that's really old code you're using.
>
> The latest version is here:
>
> http://git.linuxtv.org/hverkuil/media_tree.git/log/?h=requests
>
> But that too won't be the final version.
>
> There is still work going on in this area (specifically by Laurent Pinchart)
> since we really need this functionality. But we need to make sure that the
> API is good enough to handle complex hardware before it can be upstreamed.
>
> I suspect that once Laurent has it working for his non-trivial use-case we
> can start looking at upstreaming it.
>
> I recommend rebasing to at least the version in my git tree as that will
> be much closer to the final version. I'll try to rebase that branch to
> the latest kernel, but that's a bit difficult and takes more time than I
> have at the moment.
>

Thanks to point me in the right direction.

Regards,
    Enric

> Regards,
>
>         Hans

[RFC PATCH 05/11] v4l2-ctrls: add function to apply a configuration store.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Drivers need to be able to select a specific store. Add a new function that can
be used to apply a given store.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 61 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           |  2 ++
 2 files changed, 63 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index df0f3ee..e5dccf2 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1610,6 +1610,17 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
 	ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
 }
 
+static void store_to_new(struct v4l2_ctrl *ctrl, unsigned store)
+{
+	if (ctrl == NULL)
+		return;
+	if (store)
+		ptr_to_ptr(ctrl, ctrl->p_stores[store - 1], ctrl->p_new);
+	else
+		ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
+	ctrl->is_new = true;
+}
+
 /* Return non-zero if one or more of the controls in the cluster has a new
    value that differs from the current value. */
 static int cluster_changed(struct v4l2_ctrl *master)
@@ -3368,6 +3379,56 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
 }
 EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
 
+int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store)
+{
+	struct v4l2_ctrl_ref *ref;
+	bool found_store = false;
+	unsigned i;
+
+	if (hdl == NULL || store == 0)
+		return -EINVAL;
+
+	mutex_lock(hdl->lock);
+
+	list_for_each_entry(ref, &hdl->ctrl_refs, node) {
+		struct v4l2_ctrl *master;
+
+		if (store > ref->ctrl->nr_of_stores)
+			continue;
+		found_store = true;
+		master = ref->ctrl->cluster[0];
+		if (ref->ctrl != master)
+			continue;
+		if (master->handler != hdl)
+			v4l2_ctrl_lock(master);
+		for (i = 0; i < master->ncontrols; i++)
+			store_to_new(master->cluster[i], store);
+
+		/* For volatile autoclusters that are currently in auto mode
+		   we need to discover if it will be set to manual mode.
+		   If so, then we have to copy the current volatile values
+		   first since those will become the new manual values (which
+		   may be overwritten by explicit new values from this set
+		   of controls). */
+		if (master->is_auto && master->has_volatiles &&
+						!is_cur_manual(master)) {
+			s32 new_auto_val = *master->p_stores[store - 1].p_s32;
+
+			/* If the new value == the manual value, then copy
+			   the current volatile values. */
+			if (new_auto_val == master->manual_mode_value)
+				update_from_auto_cluster(master);
+		}
+
+		try_or_set_cluster(NULL, master, 0, true, 0);
+		if (master->handler != hdl)
+			v4l2_ctrl_unlock(master);
+	}
+	mutex_unlock(hdl->lock);
+	return found_store ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL(v4l2_ctrl_apply_store);
+
 void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv)
 {
 	if (ctrl == NULL)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 4c31688..713980a 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -777,6 +777,8 @@ static inline void v4l2_ctrl_can_store(struct v4l2_ctrl *ctrl)
 	ctrl->can_store = true;
 }
 
+int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store);
+
 /* Internal helper functions that deal with control events. */
 extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
 void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
-- 
2.1.0

[RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Replace reserved2 by a flags field. This is used to tell whether
setting a new store value is applied only once or every time that
v4l2_ctrl_apply_store() is called for that store.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/videodev2.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2ca44ed..fa84070 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1282,7 +1282,7 @@ struct v4l2_control {
 struct v4l2_ext_control {
 	__u32 id;
 	__u32 size;
-	__u32 reserved2[1];
+	__u32 flags;
 	union {
 		__s32 value;
 		__s64 value64;
@@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
 	};
 } __attribute__ ((packed));
 
+/* v4l2_ext_control flags */
+#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE	0x00000001
+#define V4L2_EXT_CTRL_FL_IGN_STORE		0x00000002
+
 struct v4l2_ext_controls {
 	union {
 		__u32 ctrl_class;
-- 
2.1.0

Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field

[Sakari Ailus]

Hi Hans,

On Sun, Sep 21, 2014 at 04:48:24PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Replace reserved2 by a flags field. This is used to tell whether
> setting a new store value is applied only once or every time that
> v4l2_ctrl_apply_store() is called for that store.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/uapi/linux/videodev2.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2ca44ed..fa84070 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1282,7 +1282,7 @@ struct v4l2_control {
>  struct v4l2_ext_control {
>  	__u32 id;
>  	__u32 size;
> -	__u32 reserved2[1];
> +	__u32 flags;

16 bits, please. The pad number (for sub-devices) would need to be added
here as well, and that's 16 bits. A flag might be needed to tell it's valid,
too.

>  	union {
>  		__s32 value;
>  		__s64 value64;
> @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
>  	};
>  } __attribute__ ((packed));
>  
> +/* v4l2_ext_control flags */
> +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE	0x00000001
> +#define V4L2_EXT_CTRL_FL_IGN_STORE		0x00000002

Do we need both? Aren't these mutually exclusive, and you must have either
to be meaningful in the context of a store?

> +
>  struct v4l2_ext_controls {
>  	union {
>  		__u32 ctrl_class;

-- 
Kind regards,

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

Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field

[Sakari Ailus]

Hi,

On Sat, Nov 15, 2014 at 04:18:59PM +0200, Sakari Ailus wrote:
...
> >  	union {
> >  		__s32 value;
> >  		__s64 value64;
> > @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
> >  	};
> >  } __attribute__ ((packed));
> >  
> > +/* v4l2_ext_control flags */
> > +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE	0x00000001
> > +#define V4L2_EXT_CTRL_FL_IGN_STORE		0x00000002
> 
> Do we need both? Aren't these mutually exclusive, and you must have either
> to be meaningful in the context of a store?

Ah. Now I think I understand what do these mean. Please ignore my previous
comment.

I might call them differently. What would you think of
V4L2_EXT_CTRL_FL_STORE_IGNORE and V4L2_EXT_CTRL_FL_STORE_ONCE?
V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE is quite long IMO. Up to you.

I wonder if we need EXT in V4L2_EXT_CTRL_FL. It's logical but also
redundant since the old control interface won't have flags either.

I'd assume that for cameras the vast majority of users will always want to
just apply the values once. How are the use cases in video decoding?

-- 
Regards,

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

Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field

[Hans Verkuil]

On 11/15/2014 06:44 PM, Sakari Ailus wrote:
> Hi,
> 
> On Sat, Nov 15, 2014 at 04:18:59PM +0200, Sakari Ailus wrote:
> ...
>>>  	union {
>>>  		__s32 value;
>>>  		__s64 value64;
>>> @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
>>>  	};
>>>  } __attribute__ ((packed));
>>>  
>>> +/* v4l2_ext_control flags */
>>> +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE	0x00000001
>>> +#define V4L2_EXT_CTRL_FL_IGN_STORE		0x00000002
>>
>> Do we need both? Aren't these mutually exclusive, and you must have either
>> to be meaningful in the context of a store?
> 
> Ah. Now I think I understand what do these mean. Please ignore my previous
> comment.
> 
> I might call them differently. What would you think of

I was never happy with the naming :-)

> V4L2_EXT_CTRL_FL_STORE_IGNORE and V4L2_EXT_CTRL_FL_STORE_ONCE?

I will give this some more thought.

> V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE is quite long IMO. Up to you.
> 
> I wonder if we need EXT in V4L2_EXT_CTRL_FL. It's logical but also
> redundant since the old control interface won't have flags either.

True.

> I'd assume that for cameras the vast majority of users will always want to
> just apply the values once. How are the use cases in video decoding?

I am wondering whether 'apply once' shouldn't be the default and whether I
really need to implement the 'apply always' (Hey, not bad names either!)
functionality for this initial version.

I only used the 'apply always' functionality for a somewhat contrived test
example where I changed the cropping rectangle (this is with the selection
controls from patch 10/11) for each buffer so that while streaming I would
get a continuous zoom-in/zoom-out effect. While nice for testing, it isn't
really practical in reality.

Regards,

	Hans

Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field

[Sakari Ailus]

Hi Hans,

On Mon, Nov 17, 2014 at 09:57:24AM +0100, Hans Verkuil wrote:
> On 11/15/2014 06:44 PM, Sakari Ailus wrote:
> > Hi,
> > 
> > On Sat, Nov 15, 2014 at 04:18:59PM +0200, Sakari Ailus wrote:
> > ...
> >>>  	union {
> >>>  		__s32 value;
> >>>  		__s64 value64;
> >>> @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
> >>>  	};
> >>>  } __attribute__ ((packed));
> >>>  
> >>> +/* v4l2_ext_control flags */
> >>> +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE	0x00000001
> >>> +#define V4L2_EXT_CTRL_FL_IGN_STORE		0x00000002
> >>
> >> Do we need both? Aren't these mutually exclusive, and you must have either
> >> to be meaningful in the context of a store?
> > 
> > Ah. Now I think I understand what do these mean. Please ignore my previous
> > comment.
> > 
> > I might call them differently. What would you think of
> 
> I was never happy with the naming :-)

:-)

> > V4L2_EXT_CTRL_FL_STORE_IGNORE and V4L2_EXT_CTRL_FL_STORE_ONCE?
> 
> I will give this some more thought.
> 
> > V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE is quite long IMO. Up to you.
> > 
> > I wonder if we need EXT in V4L2_EXT_CTRL_FL. It's logical but also
> > redundant since the old control interface won't have flags either.
> 
> True.

I think I'm inclined to keep EXT there. These values aren't used in that
many places in typical programs.

> > I'd assume that for cameras the vast majority of users will always want to
> > just apply the values once. How are the use cases in video decoding?
> 
> I am wondering whether 'apply once' shouldn't be the default and whether I
> really need to implement the 'apply always' (Hey, not bad names either!)
> functionality for this initial version.

After thinking more about it, I'm still leaning towards making the values
stick to a store by default. Forgetting the values after use is something on
top of the basic behaviour. Just my 5 euro cents. Pawel, others?

It could be nice to be able to forget an entire store. An application might
fill it, but only later figure out it will never be needed.

Do you think this could be a button control? :-) No need for this now,
though, we could see when someone needs that.

> I only used the 'apply always' functionality for a somewhat contrived test
> example where I changed the cropping rectangle (this is with the selection
> controls from patch 10/11) for each buffer so that while streaming I would
> get a continuous zoom-in/zoom-out effect. While nice for testing, it isn't
> really practical in reality.

-- 
Regards,

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

Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field

[Hans Verkuil]

On 11/15/2014 03:18 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Sun, Sep 21, 2014 at 04:48:24PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Replace reserved2 by a flags field. This is used to tell whether
>> setting a new store value is applied only once or every time that
>> v4l2_ctrl_apply_store() is called for that store.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  include/uapi/linux/videodev2.h | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 2ca44ed..fa84070 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1282,7 +1282,7 @@ struct v4l2_control {
>>  struct v4l2_ext_control {
>>  	__u32 id;
>>  	__u32 size;
>> -	__u32 reserved2[1];
>> +	__u32 flags;
> 
> 16 bits, please.

Good idea.

> The pad number (for sub-devices) would need to be added
> here as well,

Why? We never needed that for subdevs in the past. Not that I am against
reserving space for it, I'm just wondering if you have something specific
in mind.

> and that's 16 bits. A flag might be needed to tell it's valid,
> too.

Regards,

	Hans

[RFC PATCH 07/11] v4l2-ctrls: implement 'ignore after use' support.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Sometimes you want to apply a value every time v4l2_ctrl_apply_store
is called, and sometimes you want to apply that value only once.

This adds support for that feature.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 75 ++++++++++++++++++++++++++++--------
 drivers/media/v4l2-core/v4l2-ioctl.c | 14 +++----
 include/media/v4l2-ctrls.h           | 12 ++++++
 3 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index e5dccf2..21560b0 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1475,6 +1475,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
 static int cur_to_user(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl)
 {
+	c->flags = 0;
 	return ptr_to_user(c, ctrl, ctrl->p_cur);
 }
 
@@ -1482,8 +1483,13 @@ static int cur_to_user(struct v4l2_ext_control *c,
 static int store_to_user(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl, unsigned store)
 {
+	c->flags = 0;
 	if (store == 0)
 		return ptr_to_user(c, ctrl, ctrl->p_new);
+	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store_after_use))
+		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
+	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store))
+		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE;
 	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
 }
 
@@ -1491,6 +1497,7 @@ static int store_to_user(struct v4l2_ext_control *c,
 static int new_to_user(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl)
 {
+	c->flags = 0;
 	return ptr_to_user(c, ctrl, ctrl->p_new);
 }
 
@@ -1546,6 +1553,8 @@ static int user_to_ptr(struct v4l2_ext_control *c,
 static int user_to_new(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl)
 {
+	ctrl->cluster[0]->new_ignore_store_after_use =
+		c->flags & V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
 	return user_to_ptr(c, ctrl, ctrl->p_new);
 }
 
@@ -1597,8 +1606,11 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
 /* Helper function: copy the new control value to the store */
 static void new_to_store(struct v4l2_ctrl *ctrl)
 {
+	if (ctrl == NULL)
+		return;
+
 	/* has_changed is set by cluster_changed */
-	if (ctrl && ctrl->has_changed)
+	if (ctrl->has_changed)
 		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
 }
 
@@ -2328,6 +2340,12 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
 
 	for (i = 0; i < ncontrols; i++) {
 		if (controls[i]) {
+			/*
+			 * All controls in a cluster must have the same
+			 * V4L2_CTRL_FLAG_CAN_STORE flag value.
+			 */
+			WARN_ON((controls[0]->flags & controls[i]->flags) &
+					V4L2_CTRL_FLAG_CAN_STORE);
 			controls[i]->cluster = controls;
 			controls[i]->ncontrols = ncontrols;
 			if (controls[i]->flags & V4L2_CTRL_FLAG_VOLATILE)
@@ -2850,6 +2868,10 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
 	unsigned s, idx;
 	union v4l2_ctrl_ptr *p;
 
+	/* round up to the next multiple of 4 */
+	stores = (stores + 3) & ~3;
+	if (stores > V4L2_CTRLS_MAX_STORES)
+		return -EINVAL;
 	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
 	if (p == NULL)
 		return -ENOMEM;
@@ -2868,6 +2890,7 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
 		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
 	kfree(ctrl->p_stores);
 	ctrl->p_stores = p;
+	bitmap_set(ctrl->ignore_store, ctrl->nr_of_stores, stores - ctrl->nr_of_stores);
 	ctrl->nr_of_stores = stores;
 	return 0;
 }
@@ -3081,21 +3104,33 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
 
 	ret = call_op(master, try_ctrl);
 
-	/* Don't set if there is no change */
-	if (ret || !set || !cluster_changed(master))
-		return ret;
-	ret = call_op(master, s_ctrl);
-	if (ret)
+	if (ret || !set)
 		return ret;
 
-	/* If OK, then make the new values permanent. */
-	update_flag = is_cur_manual(master) != is_new_manual(master);
-	for (i = 0; i < master->ncontrols; i++) {
-		if (store)
-			new_to_store(master->cluster[i]);
+	/* Don't set if there is no change */
+	if (cluster_changed(master)) {
+		ret = call_op(master, s_ctrl);
+		if (ret)
+			return ret;
+
+		/* If OK, then make the new values permanent. */
+		update_flag = is_cur_manual(master) != is_new_manual(master);
+		for (i = 0; i < master->ncontrols; i++) {
+			if (store)
+				new_to_store(master->cluster[i]);
+			else
+				new_to_cur(fh, master->cluster[i], ch_flags |
+						((update_flag && i > 0) ?
+						 V4L2_EVENT_CTRL_CH_FLAGS : 0));
+		}
+	}
+
+	if (store) {
+		if (master->new_ignore_store_after_use)
+			set_bit(store - 1, master->ignore_store_after_use);
 		else
-			new_to_cur(fh, master->cluster[i], ch_flags |
-				((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
+			clear_bit(store - 1, master->ignore_store_after_use);
+		clear_bit(store - 1, master->ignore_store);
 	}
 	return 0;
 }
@@ -3401,8 +3436,18 @@ int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store)
 			continue;
 		if (master->handler != hdl)
 			v4l2_ctrl_lock(master);
-		for (i = 0; i < master->ncontrols; i++)
-			store_to_new(master->cluster[i], store);
+		for (i = 0; i < master->ncontrols; i++) {
+			struct v4l2_ctrl *ctrl = master->cluster[i];
+
+			if (!ctrl || (store && test_bit(store - 1, master->ignore_store)))
+				continue;
+			store_to_new(ctrl, store);
+		}
+
+		if (store && !test_bit(store - 1, master->ignore_store)) {
+			if (test_bit(store - 1, master->ignore_store_after_use))
+				set_bit(store - 1, master->ignore_store);
+		}
 
 		/* For volatile autoclusters that are currently in auto mode
 		   we need to discover if it will be set to manual mode.
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 628852c..9d3b4f2 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -562,12 +562,14 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
 	pr_cont("class=0x%x, count=%d, error_idx=%d",
 			p->ctrl_class, p->count, p->error_idx);
 	for (i = 0; i < p->count; i++) {
-		if (p->controls[i].size)
-			pr_cont(", id/val=0x%x/0x%x",
-				p->controls[i].id, p->controls[i].value);
+		if (!p->controls[i].size)
+			pr_cont(", id/flags/val=0x%x/0x%x/0x%x",
+				p->controls[i].id, p->controls[i].flags,
+				p->controls[i].value);
 		else
-			pr_cont(", id/size=0x%x/%u",
-				p->controls[i].id, p->controls[i].size);
+			pr_cont(", id/flags/size=0x%x/0x%x/%u",
+				p->controls[i].id, p->controls[i].flags,
+				p->controls[i].size);
 	}
 	pr_cont("\n");
 }
@@ -888,8 +890,6 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
 
 	/* zero the reserved fields */
 	c->reserved[0] = c->reserved[1] = 0;
-	for (i = 0; i < c->count; i++)
-		c->controls[i].reserved2[0] = 0;
 
 	/* V4L2_CID_PRIVATE_BASE cannot be used as control class
 	   when using extended controls.
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 713980a..3005d88 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -36,6 +36,9 @@ struct v4l2_subscribed_event;
 struct v4l2_fh;
 struct poll_table_struct;
 
+/* Must be a multiple of 4 */
+#define V4L2_CTRLS_MAX_STORES VIDEO_MAX_FRAME
+
 /** union v4l2_ctrl_ptr - A pointer to a control value.
  * @p_s32:	Pointer to a 32-bit signed value.
  * @p_s64:	Pointer to a 64-bit signed value.
@@ -123,6 +126,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
   * @call_notify: If set, then call the handler's notify function whenever the
   *		control's value changes.
   * @can_store: If set, then this control supports configuration stores.
+  * @new_ignore_store_after_use: If set, then the new control had the
+  *		V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE flag set.
   * @manual_mode_value: If the is_auto flag is set, then this is the value
   *		of the auto control that determines if that control is in
   *		manual mode. So if the value of the auto control equals this
@@ -143,6 +148,10 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
   * @nr_of_dims:The number of dimensions in @dims.
   * @nr_of_stores: The number of allocated configuration stores of this control.
   * @store:	The configuration store that the control op operates on.
+  * @ignore_store: If the bit for the corresponding store is 1, then don't apply that
+  *		store's value.
+  * @ignore_store_after_use: If the bit for the corresponding store is 1, then set the
+  *		bit in @ignore_store after the store's value has been applied.
   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
   *		easy to skip menu items that are not valid. If bit X is set,
   *		then menu item X is skipped. Of course, this only works for
@@ -183,6 +192,7 @@ struct v4l2_ctrl {
 	unsigned int has_volatiles:1;
 	unsigned int call_notify:1;
 	unsigned int can_store:1;
+	unsigned int new_ignore_store_after_use:1;
 	unsigned int manual_mode_value:8;
 
 	const struct v4l2_ctrl_ops *ops;
@@ -197,6 +207,8 @@ struct v4l2_ctrl {
 	u32 nr_of_dims;
 	u16 nr_of_stores;
 	u16 store;
+	DECLARE_BITMAP(ignore_store, V4L2_CTRLS_MAX_STORES);
+	DECLARE_BITMAP(ignore_store_after_use, V4L2_CTRLS_MAX_STORES);
 	union {
 		u64 step;
 		u64 menu_skip_mask;
-- 
2.1.0

Re: [RFC PATCH 07/11] v4l2-ctrls: implement 'ignore after use' support.

[Sakari Ailus]

Hi Hans,

A few comments below.

On Sun, Sep 21, 2014 at 04:48:25PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Sometimes you want to apply a value every time v4l2_ctrl_apply_store
> is called, and sometimes you want to apply that value only once.
> 
> This adds support for that feature.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 75 ++++++++++++++++++++++++++++--------
>  drivers/media/v4l2-core/v4l2-ioctl.c | 14 +++----
>  include/media/v4l2-ctrls.h           | 12 ++++++
>  3 files changed, 79 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index e5dccf2..21560b0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1475,6 +1475,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
>  static int cur_to_user(struct v4l2_ext_control *c,
>  		       struct v4l2_ctrl *ctrl)
>  {
> +	c->flags = 0;
>  	return ptr_to_user(c, ctrl, ctrl->p_cur);
>  }
>  
> @@ -1482,8 +1483,13 @@ static int cur_to_user(struct v4l2_ext_control *c,
>  static int store_to_user(struct v4l2_ext_control *c,
>  		       struct v4l2_ctrl *ctrl, unsigned store)
>  {
> +	c->flags = 0;
>  	if (store == 0)
>  		return ptr_to_user(c, ctrl, ctrl->p_new);
> +	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store_after_use))
> +		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
> +	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store))
> +		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE;
>  	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
>  }
>  
> @@ -1491,6 +1497,7 @@ static int store_to_user(struct v4l2_ext_control *c,
>  static int new_to_user(struct v4l2_ext_control *c,
>  		       struct v4l2_ctrl *ctrl)
>  {
> +	c->flags = 0;
>  	return ptr_to_user(c, ctrl, ctrl->p_new);
>  }
>  
> @@ -1546,6 +1553,8 @@ static int user_to_ptr(struct v4l2_ext_control *c,
>  static int user_to_new(struct v4l2_ext_control *c,
>  		       struct v4l2_ctrl *ctrl)
>  {
> +	ctrl->cluster[0]->new_ignore_store_after_use =
> +		c->flags & V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
>  	return user_to_ptr(c, ctrl, ctrl->p_new);
>  }
>  
> @@ -1597,8 +1606,11 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>  /* Helper function: copy the new control value to the store */
>  static void new_to_store(struct v4l2_ctrl *ctrl)
>  {
> +	if (ctrl == NULL)
> +		return;
> +
>  	/* has_changed is set by cluster_changed */
> -	if (ctrl && ctrl->has_changed)
> +	if (ctrl->has_changed)
>  		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
>  }
>  
> @@ -2328,6 +2340,12 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>  
>  	for (i = 0; i < ncontrols; i++) {
>  		if (controls[i]) {
> +			/*
> +			 * All controls in a cluster must have the same
> +			 * V4L2_CTRL_FLAG_CAN_STORE flag value.
> +			 */
> +			WARN_ON((controls[0]->flags & controls[i]->flags) &
> +					V4L2_CTRL_FLAG_CAN_STORE);
>  			controls[i]->cluster = controls;
>  			controls[i]->ncontrols = ncontrols;
>  			if (controls[i]->flags & V4L2_CTRL_FLAG_VOLATILE)
> @@ -2850,6 +2868,10 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
>  	unsigned s, idx;
>  	union v4l2_ctrl_ptr *p;
>  
> +	/* round up to the next multiple of 4 */
> +	stores = (stores + 3) & ~3;

You said it, round_up(). :-)

The comment becomes redundant as a result, too.

The above may also overflow. 

> +	if (stores > V4L2_CTRLS_MAX_STORES)
> +		return -EINVAL;
>  	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
>  	if (p == NULL)
>  		return -ENOMEM;
> @@ -2868,6 +2890,7 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
>  		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
>  	kfree(ctrl->p_stores);
>  	ctrl->p_stores = p;
> +	bitmap_set(ctrl->ignore_store, ctrl->nr_of_stores, stores - ctrl->nr_of_stores);
>  	ctrl->nr_of_stores = stores;
>  	return 0;
>  }
> @@ -3081,21 +3104,33 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
>  
>  	ret = call_op(master, try_ctrl);
>  
> -	/* Don't set if there is no change */
> -	if (ret || !set || !cluster_changed(master))
> -		return ret;
> -	ret = call_op(master, s_ctrl);
> -	if (ret)
> +	if (ret || !set)
>  		return ret;
>  
> -	/* If OK, then make the new values permanent. */
> -	update_flag = is_cur_manual(master) != is_new_manual(master);
> -	for (i = 0; i < master->ncontrols; i++) {
> -		if (store)
> -			new_to_store(master->cluster[i]);
> +	/* Don't set if there is no change */
> +	if (cluster_changed(master)) {
> +		ret = call_op(master, s_ctrl);
> +		if (ret)
> +			return ret;
> +
> +		/* If OK, then make the new values permanent. */
> +		update_flag = is_cur_manual(master) != is_new_manual(master);
> +		for (i = 0; i < master->ncontrols; i++) {
> +			if (store)
> +				new_to_store(master->cluster[i]);
> +			else
> +				new_to_cur(fh, master->cluster[i], ch_flags |
> +						((update_flag && i > 0) ?
> +						 V4L2_EVENT_CTRL_CH_FLAGS : 0));
> +		}
> +	}
> +
> +	if (store) {
> +		if (master->new_ignore_store_after_use)
> +			set_bit(store - 1, master->ignore_store_after_use);
>  		else
> -			new_to_cur(fh, master->cluster[i], ch_flags |
> -				((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> +			clear_bit(store - 1, master->ignore_store_after_use);
> +		clear_bit(store - 1, master->ignore_store);

How about allowing the user to forget a control in store as well?

>  	}
>  	return 0;
>  }
> @@ -3401,8 +3436,18 @@ int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store)
>  			continue;
>  		if (master->handler != hdl)
>  			v4l2_ctrl_lock(master);
> -		for (i = 0; i < master->ncontrols; i++)
> -			store_to_new(master->cluster[i], store);
> +		for (i = 0; i < master->ncontrols; i++) {
> +			struct v4l2_ctrl *ctrl = master->cluster[i];
> +
> +			if (!ctrl || (store && test_bit(store - 1, master->ignore_store)))
> +				continue;
> +			store_to_new(ctrl, store);
> +		}
> +
> +		if (store && !test_bit(store - 1, master->ignore_store)) {
> +			if (test_bit(store - 1, master->ignore_store_after_use))

How about:

if (store && test_bit() && test_bit())

> +				set_bit(store - 1, master->ignore_store);
> +		}
>  
>  		/* For volatile autoclusters that are currently in auto mode
>  		   we need to discover if it will be set to manual mode.
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 628852c..9d3b4f2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -562,12 +562,14 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
>  	pr_cont("class=0x%x, count=%d, error_idx=%d",
>  			p->ctrl_class, p->count, p->error_idx);
>  	for (i = 0; i < p->count; i++) {
> -		if (p->controls[i].size)
> -			pr_cont(", id/val=0x%x/0x%x",
> -				p->controls[i].id, p->controls[i].value);
> +		if (!p->controls[i].size)
> +			pr_cont(", id/flags/val=0x%x/0x%x/0x%x",
> +				p->controls[i].id, p->controls[i].flags,
> +				p->controls[i].value);
>  		else
> -			pr_cont(", id/size=0x%x/%u",
> -				p->controls[i].id, p->controls[i].size);
> +			pr_cont(", id/flags/size=0x%x/0x%x/%u",
> +				p->controls[i].id, p->controls[i].flags,
> +				p->controls[i].size);
>  	}
>  	pr_cont("\n");
>  }
> @@ -888,8 +890,6 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
>  
>  	/* zero the reserved fields */
>  	c->reserved[0] = c->reserved[1] = 0;
> -	for (i = 0; i < c->count; i++)
> -		c->controls[i].reserved2[0] = 0;
>  
>  	/* V4L2_CID_PRIVATE_BASE cannot be used as control class
>  	   when using extended controls.
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 713980a..3005d88 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -36,6 +36,9 @@ struct v4l2_subscribed_event;
>  struct v4l2_fh;
>  struct poll_table_struct;
>  
> +/* Must be a multiple of 4 */
> +#define V4L2_CTRLS_MAX_STORES VIDEO_MAX_FRAME
> +
>  /** union v4l2_ctrl_ptr - A pointer to a control value.
>   * @p_s32:	Pointer to a 32-bit signed value.
>   * @p_s64:	Pointer to a 64-bit signed value.
> @@ -123,6 +126,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>    * @call_notify: If set, then call the handler's notify function whenever the
>    *		control's value changes.
>    * @can_store: If set, then this control supports configuration stores.
> +  * @new_ignore_store_after_use: If set, then the new control had the
> +  *		V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE flag set.
>    * @manual_mode_value: If the is_auto flag is set, then this is the value
>    *		of the auto control that determines if that control is in
>    *		manual mode. So if the value of the auto control equals this
> @@ -143,6 +148,10 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>    * @nr_of_dims:The number of dimensions in @dims.
>    * @nr_of_stores: The number of allocated configuration stores of this control.
>    * @store:	The configuration store that the control op operates on.
> +  * @ignore_store: If the bit for the corresponding store is 1, then don't apply that
> +  *		store's value.
> +  * @ignore_store_after_use: If the bit for the corresponding store is 1, then set the
> +  *		bit in @ignore_store after the store's value has been applied.
>    * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>    *		easy to skip menu items that are not valid. If bit X is set,
>    *		then menu item X is skipped. Of course, this only works for
> @@ -183,6 +192,7 @@ struct v4l2_ctrl {
>  	unsigned int has_volatiles:1;
>  	unsigned int call_notify:1;
>  	unsigned int can_store:1;
> +	unsigned int new_ignore_store_after_use:1;
>  	unsigned int manual_mode_value:8;
>  
>  	const struct v4l2_ctrl_ops *ops;
> @@ -197,6 +207,8 @@ struct v4l2_ctrl {
>  	u32 nr_of_dims;
>  	u16 nr_of_stores;
>  	u16 store;
> +	DECLARE_BITMAP(ignore_store, V4L2_CTRLS_MAX_STORES);
> +	DECLARE_BITMAP(ignore_store_after_use, V4L2_CTRLS_MAX_STORES);

I'd store this information next to the value itself. The reason is that
stores are typically accessed one at a time, and thus keeping data related
to a single store in a single contiguous location reduces cache misses.

>  	union {
>  		u64 step;
>  		u64 menu_skip_mask;

-- 
Kind regards,

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

Re: [RFC PATCH 07/11] v4l2-ctrls: implement 'ignore after use' support.

[Hans Verkuil]

On 11/15/2014 10:10 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> A few comments below.
> 
> On Sun, Sep 21, 2014 at 04:48:25PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Sometimes you want to apply a value every time v4l2_ctrl_apply_store
>> is called, and sometimes you want to apply that value only once.
>>
>> This adds support for that feature.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 75 ++++++++++++++++++++++++++++--------
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 14 +++----
>>  include/media/v4l2-ctrls.h           | 12 ++++++
>>  3 files changed, 79 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index e5dccf2..21560b0 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1475,6 +1475,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
>>  static int cur_to_user(struct v4l2_ext_control *c,
>>  		       struct v4l2_ctrl *ctrl)
>>  {
>> +	c->flags = 0;
>>  	return ptr_to_user(c, ctrl, ctrl->p_cur);
>>  }
>>  
>> @@ -1482,8 +1483,13 @@ static int cur_to_user(struct v4l2_ext_control *c,
>>  static int store_to_user(struct v4l2_ext_control *c,
>>  		       struct v4l2_ctrl *ctrl, unsigned store)
>>  {
>> +	c->flags = 0;
>>  	if (store == 0)
>>  		return ptr_to_user(c, ctrl, ctrl->p_new);
>> +	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store_after_use))
>> +		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
>> +	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store))
>> +		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE;
>>  	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
>>  }
>>  
>> @@ -1491,6 +1497,7 @@ static int store_to_user(struct v4l2_ext_control *c,
>>  static int new_to_user(struct v4l2_ext_control *c,
>>  		       struct v4l2_ctrl *ctrl)
>>  {
>> +	c->flags = 0;
>>  	return ptr_to_user(c, ctrl, ctrl->p_new);
>>  }
>>  
>> @@ -1546,6 +1553,8 @@ static int user_to_ptr(struct v4l2_ext_control *c,
>>  static int user_to_new(struct v4l2_ext_control *c,
>>  		       struct v4l2_ctrl *ctrl)
>>  {
>> +	ctrl->cluster[0]->new_ignore_store_after_use =
>> +		c->flags & V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
>>  	return user_to_ptr(c, ctrl, ctrl->p_new);
>>  }
>>  
>> @@ -1597,8 +1606,11 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>>  /* Helper function: copy the new control value to the store */
>>  static void new_to_store(struct v4l2_ctrl *ctrl)
>>  {
>> +	if (ctrl == NULL)
>> +		return;
>> +
>>  	/* has_changed is set by cluster_changed */
>> -	if (ctrl && ctrl->has_changed)
>> +	if (ctrl->has_changed)
>>  		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
>>  }
>>  
>> @@ -2328,6 +2340,12 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>>  
>>  	for (i = 0; i < ncontrols; i++) {
>>  		if (controls[i]) {
>> +			/*
>> +			 * All controls in a cluster must have the same
>> +			 * V4L2_CTRL_FLAG_CAN_STORE flag value.
>> +			 */
>> +			WARN_ON((controls[0]->flags & controls[i]->flags) &
>> +					V4L2_CTRL_FLAG_CAN_STORE);
>>  			controls[i]->cluster = controls;
>>  			controls[i]->ncontrols = ncontrols;
>>  			if (controls[i]->flags & V4L2_CTRL_FLAG_VOLATILE)
>> @@ -2850,6 +2868,10 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
>>  	unsigned s, idx;
>>  	union v4l2_ctrl_ptr *p;
>>  
>> +	/* round up to the next multiple of 4 */
>> +	stores = (stores + 3) & ~3;
> 
> You said it, round_up(). :-)
> 
> The comment becomes redundant as a result, too.
> 
> The above may also overflow. 

Will fix.

> 
>> +	if (stores > V4L2_CTRLS_MAX_STORES)
>> +		return -EINVAL;
>>  	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
>>  	if (p == NULL)
>>  		return -ENOMEM;
>> @@ -2868,6 +2890,7 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
>>  		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
>>  	kfree(ctrl->p_stores);
>>  	ctrl->p_stores = p;
>> +	bitmap_set(ctrl->ignore_store, ctrl->nr_of_stores, stores - ctrl->nr_of_stores);
>>  	ctrl->nr_of_stores = stores;
>>  	return 0;
>>  }
>> @@ -3081,21 +3104,33 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
>>  
>>  	ret = call_op(master, try_ctrl);
>>  
>> -	/* Don't set if there is no change */
>> -	if (ret || !set || !cluster_changed(master))
>> -		return ret;
>> -	ret = call_op(master, s_ctrl);
>> -	if (ret)
>> +	if (ret || !set)
>>  		return ret;
>>  
>> -	/* If OK, then make the new values permanent. */
>> -	update_flag = is_cur_manual(master) != is_new_manual(master);
>> -	for (i = 0; i < master->ncontrols; i++) {
>> -		if (store)
>> -			new_to_store(master->cluster[i]);
>> +	/* Don't set if there is no change */
>> +	if (cluster_changed(master)) {
>> +		ret = call_op(master, s_ctrl);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* If OK, then make the new values permanent. */
>> +		update_flag = is_cur_manual(master) != is_new_manual(master);
>> +		for (i = 0; i < master->ncontrols; i++) {
>> +			if (store)
>> +				new_to_store(master->cluster[i]);
>> +			else
>> +				new_to_cur(fh, master->cluster[i], ch_flags |
>> +						((update_flag && i > 0) ?
>> +						 V4L2_EVENT_CTRL_CH_FLAGS : 0));
>> +		}
>> +	}
>> +
>> +	if (store) {
>> +		if (master->new_ignore_store_after_use)
>> +			set_bit(store - 1, master->ignore_store_after_use);
>>  		else
>> -			new_to_cur(fh, master->cluster[i], ch_flags |
>> -				((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
>> +			clear_bit(store - 1, master->ignore_store_after_use);
>> +		clear_bit(store - 1, master->ignore_store);
> 
> How about allowing the user to forget a control in store as well?

Yeah, that's one thing I need to add. I need to think about this some more how this
can be done cleanly.

> 
>>  	}
>>  	return 0;
>>  }
>> @@ -3401,8 +3436,18 @@ int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store)
>>  			continue;
>>  		if (master->handler != hdl)
>>  			v4l2_ctrl_lock(master);
>> -		for (i = 0; i < master->ncontrols; i++)
>> -			store_to_new(master->cluster[i], store);
>> +		for (i = 0; i < master->ncontrols; i++) {
>> +			struct v4l2_ctrl *ctrl = master->cluster[i];
>> +
>> +			if (!ctrl || (store && test_bit(store - 1, master->ignore_store)))
>> +				continue;
>> +			store_to_new(ctrl, store);
>> +		}
>> +
>> +		if (store && !test_bit(store - 1, master->ignore_store)) {
>> +			if (test_bit(store - 1, master->ignore_store_after_use))
> 
> How about:
> 
> if (store && test_bit() && test_bit())

OK.

> 
>> +				set_bit(store - 1, master->ignore_store);
>> +		}
>>  
>>  		/* For volatile autoclusters that are currently in auto mode
>>  		   we need to discover if it will be set to manual mode.
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 628852c..9d3b4f2 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -562,12 +562,14 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
>>  	pr_cont("class=0x%x, count=%d, error_idx=%d",
>>  			p->ctrl_class, p->count, p->error_idx);
>>  	for (i = 0; i < p->count; i++) {
>> -		if (p->controls[i].size)
>> -			pr_cont(", id/val=0x%x/0x%x",
>> -				p->controls[i].id, p->controls[i].value);
>> +		if (!p->controls[i].size)
>> +			pr_cont(", id/flags/val=0x%x/0x%x/0x%x",
>> +				p->controls[i].id, p->controls[i].flags,
>> +				p->controls[i].value);
>>  		else
>> -			pr_cont(", id/size=0x%x/%u",
>> -				p->controls[i].id, p->controls[i].size);
>> +			pr_cont(", id/flags/size=0x%x/0x%x/%u",
>> +				p->controls[i].id, p->controls[i].flags,
>> +				p->controls[i].size);
>>  	}
>>  	pr_cont("\n");
>>  }
>> @@ -888,8 +890,6 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
>>  
>>  	/* zero the reserved fields */
>>  	c->reserved[0] = c->reserved[1] = 0;
>> -	for (i = 0; i < c->count; i++)
>> -		c->controls[i].reserved2[0] = 0;
>>  
>>  	/* V4L2_CID_PRIVATE_BASE cannot be used as control class
>>  	   when using extended controls.
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index 713980a..3005d88 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -36,6 +36,9 @@ struct v4l2_subscribed_event;
>>  struct v4l2_fh;
>>  struct poll_table_struct;
>>  
>> +/* Must be a multiple of 4 */
>> +#define V4L2_CTRLS_MAX_STORES VIDEO_MAX_FRAME
>> +
>>  /** union v4l2_ctrl_ptr - A pointer to a control value.
>>   * @p_s32:	Pointer to a 32-bit signed value.
>>   * @p_s64:	Pointer to a 64-bit signed value.
>> @@ -123,6 +126,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>    * @call_notify: If set, then call the handler's notify function whenever the
>>    *		control's value changes.
>>    * @can_store: If set, then this control supports configuration stores.
>> +  * @new_ignore_store_after_use: If set, then the new control had the
>> +  *		V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE flag set.
>>    * @manual_mode_value: If the is_auto flag is set, then this is the value
>>    *		of the auto control that determines if that control is in
>>    *		manual mode. So if the value of the auto control equals this
>> @@ -143,6 +148,10 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>    * @nr_of_dims:The number of dimensions in @dims.
>>    * @nr_of_stores: The number of allocated configuration stores of this control.
>>    * @store:	The configuration store that the control op operates on.
>> +  * @ignore_store: If the bit for the corresponding store is 1, then don't apply that
>> +  *		store's value.
>> +  * @ignore_store_after_use: If the bit for the corresponding store is 1, then set the
>> +  *		bit in @ignore_store after the store's value has been applied.
>>    * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>>    *		easy to skip menu items that are not valid. If bit X is set,
>>    *		then menu item X is skipped. Of course, this only works for
>> @@ -183,6 +192,7 @@ struct v4l2_ctrl {
>>  	unsigned int has_volatiles:1;
>>  	unsigned int call_notify:1;
>>  	unsigned int can_store:1;
>> +	unsigned int new_ignore_store_after_use:1;
>>  	unsigned int manual_mode_value:8;
>>  
>>  	const struct v4l2_ctrl_ops *ops;
>> @@ -197,6 +207,8 @@ struct v4l2_ctrl {
>>  	u32 nr_of_dims;
>>  	u16 nr_of_stores;
>>  	u16 store;
>> +	DECLARE_BITMAP(ignore_store, V4L2_CTRLS_MAX_STORES);
>> +	DECLARE_BITMAP(ignore_store_after_use, V4L2_CTRLS_MAX_STORES);
> 
> I'd store this information next to the value itself. The reason is that
> stores are typically accessed one at a time, and thus keeping data related
> to a single store in a single contiguous location reduces cache misses.

Hmm, sounds like overengineering to me. If I can do that without sacrificing
readability, then I can more it around. It's likely that these datastructures
will change anyway.

Regards,

	Hans

Re: [RFC PATCH 07/11] v4l2-ctrls: implement 'ignore after use' support.

[Sakari Ailus]

Hi Hans,

On Mon, Nov 17, 2014 at 10:02:03AM +0100, Hans Verkuil wrote:
> On 11/15/2014 10:10 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > A few comments below.
> > 
> > On Sun, Sep 21, 2014 at 04:48:25PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> Sometimes you want to apply a value every time v4l2_ctrl_apply_store
> >> is called, and sometimes you want to apply that value only once.
> >>
> >> This adds support for that feature.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-ctrls.c | 75 ++++++++++++++++++++++++++++--------
> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 14 +++----
> >>  include/media/v4l2-ctrls.h           | 12 ++++++
> >>  3 files changed, 79 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> index e5dccf2..21560b0 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> @@ -1475,6 +1475,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
> >>  static int cur_to_user(struct v4l2_ext_control *c,
> >>  		       struct v4l2_ctrl *ctrl)
> >>  {
> >> +	c->flags = 0;
> >>  	return ptr_to_user(c, ctrl, ctrl->p_cur);
> >>  }
> >>  
> >> @@ -1482,8 +1483,13 @@ static int cur_to_user(struct v4l2_ext_control *c,
> >>  static int store_to_user(struct v4l2_ext_control *c,
> >>  		       struct v4l2_ctrl *ctrl, unsigned store)
> >>  {
> >> +	c->flags = 0;
> >>  	if (store == 0)
> >>  		return ptr_to_user(c, ctrl, ctrl->p_new);
> >> +	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store_after_use))
> >> +		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
> >> +	if (test_bit(store - 1, ctrl->cluster[0]->ignore_store))
> >> +		c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE;
> >>  	return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
> >>  }
> >>  
> >> @@ -1491,6 +1497,7 @@ static int store_to_user(struct v4l2_ext_control *c,
> >>  static int new_to_user(struct v4l2_ext_control *c,
> >>  		       struct v4l2_ctrl *ctrl)
> >>  {
> >> +	c->flags = 0;
> >>  	return ptr_to_user(c, ctrl, ctrl->p_new);
> >>  }
> >>  
> >> @@ -1546,6 +1553,8 @@ static int user_to_ptr(struct v4l2_ext_control *c,
> >>  static int user_to_new(struct v4l2_ext_control *c,
> >>  		       struct v4l2_ctrl *ctrl)
> >>  {
> >> +	ctrl->cluster[0]->new_ignore_store_after_use =
> >> +		c->flags & V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
> >>  	return user_to_ptr(c, ctrl, ctrl->p_new);
> >>  }
> >>  
> >> @@ -1597,8 +1606,11 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
> >>  /* Helper function: copy the new control value to the store */
> >>  static void new_to_store(struct v4l2_ctrl *ctrl)
> >>  {
> >> +	if (ctrl == NULL)
> >> +		return;
> >> +
> >>  	/* has_changed is set by cluster_changed */
> >> -	if (ctrl && ctrl->has_changed)
> >> +	if (ctrl->has_changed)
> >>  		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
> >>  }
> >>  
> >> @@ -2328,6 +2340,12 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> >>  
> >>  	for (i = 0; i < ncontrols; i++) {
> >>  		if (controls[i]) {
> >> +			/*
> >> +			 * All controls in a cluster must have the same
> >> +			 * V4L2_CTRL_FLAG_CAN_STORE flag value.
> >> +			 */
> >> +			WARN_ON((controls[0]->flags & controls[i]->flags) &
> >> +					V4L2_CTRL_FLAG_CAN_STORE);
> >>  			controls[i]->cluster = controls;
> >>  			controls[i]->ncontrols = ncontrols;
> >>  			if (controls[i]->flags & V4L2_CTRL_FLAG_VOLATILE)
> >> @@ -2850,6 +2868,10 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
> >>  	unsigned s, idx;
> >>  	union v4l2_ctrl_ptr *p;
> >>  
> >> +	/* round up to the next multiple of 4 */
> >> +	stores = (stores + 3) & ~3;
> > 
> > You said it, round_up(). :-)
> > 
> > The comment becomes redundant as a result, too.
> > 
> > The above may also overflow. 
> 
> Will fix.

I just realised round_up() will naturally also overflow, but it'll overflow
"correctly" to zero. So the upper limit check must be before this. The
change then effectually only makes the comment unnecessary.

> > 
> >> +	if (stores > V4L2_CTRLS_MAX_STORES)
> >> +		return -EINVAL;
> >>  	p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
> >>  	if (p == NULL)
> >>  		return -ENOMEM;
> >> @@ -2868,6 +2890,7 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
> >>  		memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
> >>  	kfree(ctrl->p_stores);
> >>  	ctrl->p_stores = p;
> >> +	bitmap_set(ctrl->ignore_store, ctrl->nr_of_stores, stores - ctrl->nr_of_stores);
> >>  	ctrl->nr_of_stores = stores;
> >>  	return 0;
> >>  }
> >> @@ -3081,21 +3104,33 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
> >>  
> >>  	ret = call_op(master, try_ctrl);
> >>  
> >> -	/* Don't set if there is no change */
> >> -	if (ret || !set || !cluster_changed(master))
> >> -		return ret;
> >> -	ret = call_op(master, s_ctrl);
> >> -	if (ret)
> >> +	if (ret || !set)
> >>  		return ret;
> >>  
> >> -	/* If OK, then make the new values permanent. */
> >> -	update_flag = is_cur_manual(master) != is_new_manual(master);
> >> -	for (i = 0; i < master->ncontrols; i++) {
> >> -		if (store)
> >> -			new_to_store(master->cluster[i]);
> >> +	/* Don't set if there is no change */
> >> +	if (cluster_changed(master)) {
> >> +		ret = call_op(master, s_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		/* If OK, then make the new values permanent. */
> >> +		update_flag = is_cur_manual(master) != is_new_manual(master);
> >> +		for (i = 0; i < master->ncontrols; i++) {
> >> +			if (store)
> >> +				new_to_store(master->cluster[i]);
> >> +			else
> >> +				new_to_cur(fh, master->cluster[i], ch_flags |
> >> +						((update_flag && i > 0) ?
> >> +						 V4L2_EVENT_CTRL_CH_FLAGS : 0));
> >> +		}
> >> +	}
> >> +
> >> +	if (store) {
> >> +		if (master->new_ignore_store_after_use)
> >> +			set_bit(store - 1, master->ignore_store_after_use);
> >>  		else
> >> -			new_to_cur(fh, master->cluster[i], ch_flags |
> >> -				((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> >> +			clear_bit(store - 1, master->ignore_store_after_use);
> >> +		clear_bit(store - 1, master->ignore_store);
> > 
> > How about allowing the user to forget a control in store as well?
> 
> Yeah, that's one thing I need to add. I need to think about this some more how this
> can be done cleanly.

Ack.

> > 
> >>  	}
> >>  	return 0;
> >>  }
> >> @@ -3401,8 +3436,18 @@ int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store)
> >>  			continue;
> >>  		if (master->handler != hdl)
> >>  			v4l2_ctrl_lock(master);
> >> -		for (i = 0; i < master->ncontrols; i++)
> >> -			store_to_new(master->cluster[i], store);
> >> +		for (i = 0; i < master->ncontrols; i++) {
> >> +			struct v4l2_ctrl *ctrl = master->cluster[i];
> >> +
> >> +			if (!ctrl || (store && test_bit(store - 1, master->ignore_store)))
> >> +				continue;
> >> +			store_to_new(ctrl, store);
> >> +		}
> >> +
> >> +		if (store && !test_bit(store - 1, master->ignore_store)) {
> >> +			if (test_bit(store - 1, master->ignore_store_after_use))
> > 
> > How about:
> > 
> > if (store && test_bit() && test_bit())
> 
> OK.
> 
> > 
> >> +				set_bit(store - 1, master->ignore_store);
> >> +		}
> >>  
> >>  		/* For volatile autoclusters that are currently in auto mode
> >>  		   we need to discover if it will be set to manual mode.
> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> index 628852c..9d3b4f2 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -562,12 +562,14 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
> >>  	pr_cont("class=0x%x, count=%d, error_idx=%d",
> >>  			p->ctrl_class, p->count, p->error_idx);
> >>  	for (i = 0; i < p->count; i++) {
> >> -		if (p->controls[i].size)
> >> -			pr_cont(", id/val=0x%x/0x%x",
> >> -				p->controls[i].id, p->controls[i].value);
> >> +		if (!p->controls[i].size)
> >> +			pr_cont(", id/flags/val=0x%x/0x%x/0x%x",
> >> +				p->controls[i].id, p->controls[i].flags,
> >> +				p->controls[i].value);
> >>  		else
> >> -			pr_cont(", id/size=0x%x/%u",
> >> -				p->controls[i].id, p->controls[i].size);
> >> +			pr_cont(", id/flags/size=0x%x/0x%x/%u",
> >> +				p->controls[i].id, p->controls[i].flags,
> >> +				p->controls[i].size);
> >>  	}
> >>  	pr_cont("\n");
> >>  }
> >> @@ -888,8 +890,6 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
> >>  
> >>  	/* zero the reserved fields */
> >>  	c->reserved[0] = c->reserved[1] = 0;
> >> -	for (i = 0; i < c->count; i++)
> >> -		c->controls[i].reserved2[0] = 0;
> >>  
> >>  	/* V4L2_CID_PRIVATE_BASE cannot be used as control class
> >>  	   when using extended controls.
> >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >> index 713980a..3005d88 100644
> >> --- a/include/media/v4l2-ctrls.h
> >> +++ b/include/media/v4l2-ctrls.h
> >> @@ -36,6 +36,9 @@ struct v4l2_subscribed_event;
> >>  struct v4l2_fh;
> >>  struct poll_table_struct;
> >>  
> >> +/* Must be a multiple of 4 */
> >> +#define V4L2_CTRLS_MAX_STORES VIDEO_MAX_FRAME
> >> +
> >>  /** union v4l2_ctrl_ptr - A pointer to a control value.
> >>   * @p_s32:	Pointer to a 32-bit signed value.
> >>   * @p_s64:	Pointer to a 64-bit signed value.
> >> @@ -123,6 +126,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> >>    * @call_notify: If set, then call the handler's notify function whenever the
> >>    *		control's value changes.
> >>    * @can_store: If set, then this control supports configuration stores.
> >> +  * @new_ignore_store_after_use: If set, then the new control had the
> >> +  *		V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE flag set.
> >>    * @manual_mode_value: If the is_auto flag is set, then this is the value
> >>    *		of the auto control that determines if that control is in
> >>    *		manual mode. So if the value of the auto control equals this
> >> @@ -143,6 +148,10 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> >>    * @nr_of_dims:The number of dimensions in @dims.
> >>    * @nr_of_stores: The number of allocated configuration stores of this control.
> >>    * @store:	The configuration store that the control op operates on.
> >> +  * @ignore_store: If the bit for the corresponding store is 1, then don't apply that
> >> +  *		store's value.
> >> +  * @ignore_store_after_use: If the bit for the corresponding store is 1, then set the
> >> +  *		bit in @ignore_store after the store's value has been applied.
> >>    * @menu_skip_mask: The control's skip mask for menu controls. This makes it
> >>    *		easy to skip menu items that are not valid. If bit X is set,
> >>    *		then menu item X is skipped. Of course, this only works for
> >> @@ -183,6 +192,7 @@ struct v4l2_ctrl {
> >>  	unsigned int has_volatiles:1;
> >>  	unsigned int call_notify:1;
> >>  	unsigned int can_store:1;
> >> +	unsigned int new_ignore_store_after_use:1;
> >>  	unsigned int manual_mode_value:8;
> >>  
> >>  	const struct v4l2_ctrl_ops *ops;
> >> @@ -197,6 +207,8 @@ struct v4l2_ctrl {
> >>  	u32 nr_of_dims;
> >>  	u16 nr_of_stores;
> >>  	u16 store;
> >> +	DECLARE_BITMAP(ignore_store, V4L2_CTRLS_MAX_STORES);
> >> +	DECLARE_BITMAP(ignore_store_after_use, V4L2_CTRLS_MAX_STORES);
> > 
> > I'd store this information next to the value itself. The reason is that
> > stores are typically accessed one at a time, and thus keeping data related
> > to a single store in a single contiguous location reduces cache misses.
> 
> Hmm, sounds like overengineering to me. If I can do that without sacrificing
> readability, then I can more it around. It's likely that these datastructures
> will change anyway.

The controls are accessed very often in practice so this kind of things
count. There's already a lot of code which gets executed in order to set a
single control that's relevant only in some cases, such as clusters.

I think it'd probably be more readable as well if information related to a
store was located in a single place. As a bonus you wouldn't need to set a
global maximum for the number of stores one may have.

-- 
Kind regards,

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

Re: [RFC PATCH 07/11] v4l2-ctrls: implement 'ignore after use' support.

[Hans Verkuil]

On 11/17/2014 10:31 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Nov 17, 2014 at 10:02:03AM +0100, Hans Verkuil wrote:
>> On 11/15/2014 10:10 PM, Sakari Ailus wrote:
>>>> @@ -197,6 +207,8 @@ struct v4l2_ctrl {
>>>>  	u32 nr_of_dims;
>>>>  	u16 nr_of_stores;
>>>>  	u16 store;
>>>> +	DECLARE_BITMAP(ignore_store, V4L2_CTRLS_MAX_STORES);
>>>> +	DECLARE_BITMAP(ignore_store_after_use, V4L2_CTRLS_MAX_STORES);
>>>
>>> I'd store this information next to the value itself. The reason is that
>>> stores are typically accessed one at a time, and thus keeping data related
>>> to a single store in a single contiguous location reduces cache misses.
>>
>> Hmm, sounds like overengineering to me. If I can do that without sacrificing
>> readability, then I can more it around. It's likely that these datastructures
>> will change anyway.
> 
> The controls are accessed very often in practice so this kind of things
> count. There's already a lot of code which gets executed in order to set a
> single control that's relevant only in some cases, such as clusters.

Complexity is the biggest problem in video drivers, not speed. Optimizations for
the sake of speeding up code at the expense of complexity should only be implemented
if you can *prove* that there is a measurable speedup.

Personally I would be very surprised if you can measure this in this specific
case.

Anyway, it doesn't matter in this case since I intend to rework those data
structures in any case.

Regards,

	Hans

> I think it'd probably be more readable as well if information related to a
> store was located in a single place. As a bonus you wouldn't need to set a
> global maximum for the number of stores one may have.

[RFC PATCH 08/11] vivid: add test config store for the contrast control

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vivid/vivid-ctrls.c   | 4 ++++
 drivers/media/platform/vivid/vivid-vid-cap.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
index d5cbf00..42376a1 100644
--- a/drivers/media/platform/vivid/vivid-ctrls.c
+++ b/drivers/media/platform/vivid/vivid-ctrls.c
@@ -255,6 +255,9 @@ static int vivid_user_vid_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct vivid_dev *dev = container_of(ctrl->handler, struct vivid_dev, ctrl_hdl_user_vid);
 
+	if (ctrl->store)
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_BRIGHTNESS:
 		dev->input_brightness[dev->input] = ctrl->val - dev->input * 128;
@@ -1199,6 +1202,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 			dev->input_brightness[i] = 128;
 		dev->contrast = v4l2_ctrl_new_std(hdl_user_vid, &vivid_user_vid_ctrl_ops,
 			V4L2_CID_CONTRAST, 0, 255, 1, 128);
+		v4l2_ctrl_can_store(dev->contrast);
 		dev->saturation = v4l2_ctrl_new_std(hdl_user_vid, &vivid_user_vid_ctrl_ops,
 			V4L2_CID_SATURATION, 0, 255, 1, 128);
 		dev->hue = v4l2_ctrl_new_std(hdl_user_vid, &vivid_user_vid_ctrl_ops,
diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
index b016aed..95eda68 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -197,6 +197,7 @@ static int vid_cap_buf_prepare(struct vb2_buffer *vb)
 		vb2_set_plane_payload(vb, p, size);
 		vb->v4l2_planes[p].data_offset = dev->fmt_cap->data_offset[p];
 	}
+	v4l2_ctrl_apply_store(dev->vid_cap_dev.ctrl_handler, vb->v4l2_buf.config_store);
 
 	return 0;
 }
-- 
2.1.0

[RFC PATCH 09/11] videodev2.h: add v4l2_ctrl_selection compound control type.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This will be used by a new selection control.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/media/v4l2-ctrls.h     | 2 ++
 include/uapi/linux/videodev2.h | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 3005d88..c2fd050 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -46,6 +46,7 @@ struct poll_table_struct;
  * @p_u16:	Pointer to a 16-bit unsigned value.
  * @p_u32:	Pointer to a 32-bit unsigned value.
  * @p_char:	Pointer to a string.
+ * @p_sel:	Pointer to a struct v4l2_ctrl_selection.
  * @p:		Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
@@ -55,6 +56,7 @@ union v4l2_ctrl_ptr {
 	u16 *p_u16;
 	u32 *p_u32;
 	char *p_char;
+	struct v4l2_ctrl_selection *p_sel;
 	void *p;
 };
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fa84070..e956472 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1271,6 +1271,12 @@ struct v4l2_output {
 #define V4L2_OUT_CAP_CUSTOM_TIMINGS	V4L2_OUT_CAP_DV_TIMINGS /* For compatibility */
 #define V4L2_OUT_CAP_STD		0x00000004 /* Supports S_STD */
 
+struct v4l2_ctrl_selection {
+	__u32 flags;
+	struct v4l2_rect r;
+	__u32 reserved[9];
+};
+
 /*
  *	C O N T R O L S
  */
@@ -1290,6 +1296,7 @@ struct v4l2_ext_control {
 		__u8 __user *p_u8;
 		__u16 __user *p_u16;
 		__u32 __user *p_u32;
+		struct v4l2_ctrl_selection __user *p_sel;
 		void __user *ptr;
 	};
 } __attribute__ ((packed));
@@ -1330,6 +1337,7 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	V4L2_CTRL_TYPE_SELECTION     = 0x0103,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
-- 
2.1.0

Re: [RFC PATCH 09/11] videodev2.h: add v4l2_ctrl_selection compound control type.

[Sakari Ailus]

Hi Hans,

(Cc Ricardo.)

Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This will be used by a new selection control.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/media/v4l2-ctrls.h     | 2 ++
>  include/uapi/linux/videodev2.h | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 3005d88..c2fd050 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -46,6 +46,7 @@ struct poll_table_struct;
>   * @p_u16:	Pointer to a 16-bit unsigned value.
>   * @p_u32:	Pointer to a 32-bit unsigned value.
>   * @p_char:	Pointer to a string.
> + * @p_sel:	Pointer to a struct v4l2_ctrl_selection.
>   * @p:		Pointer to a compound value.
>   */
>  union v4l2_ctrl_ptr {
> @@ -55,6 +56,7 @@ union v4l2_ctrl_ptr {
>  	u16 *p_u16;
>  	u32 *p_u32;
>  	char *p_char;
> +	struct v4l2_ctrl_selection *p_sel;
>  	void *p;
>  };

In order to be usable on sub-devices, pad information should be added.
That results in having a pad per rectangle, which probably doesn't make
sense. Also, other controls may benefit from being pad related.

What would you think of including the pad information in struct
v4l2_ext_control? That should be in a different patch. Would a flags
field be needed to tell whether the pad field is valid? 16 bits should
be good for both, but we anyway had just a single reserved field.

This would leave you with essentially a rectangle control, which you
still might want to call (or not) a selection control.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

[RFC PATCH 10/11] v4l2-ctrls: add multi-selection controls

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 24 ++++++++++++++++++++++++
 include/uapi/linux/v4l2-controls.h   |  7 ++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 21560b0..af76bda 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -643,6 +643,10 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Min Number of Output Buffers";
 	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha Component";
 	case V4L2_CID_COLORFX_CBCR:		return "Color Effects, CbCr";
+	case V4L2_CID_CAPTURE_CROP:		return "Capture Crop Selections";
+	case V4L2_CID_CAPTURE_COMPOSE:		return "Capture Compose Selections";
+	case V4L2_CID_OUTPUT_CROP:		return "Output Crop Selections";
+	case V4L2_CID_OUTPUT_COMPOSE:		return "Output Compose Selections";
 
 	/* Codec controls */
 	/* The MPEG controls are applicable to all codec controls
@@ -1122,6 +1126,13 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RDS_TX_ALT_FREQS:
 		*type = V4L2_CTRL_TYPE_U32;
 		break;
+	case V4L2_CID_CAPTURE_CROP:
+	case V4L2_CID_CAPTURE_COMPOSE:
+	case V4L2_CID_OUTPUT_CROP:
+	case V4L2_CID_OUTPUT_COMPOSE:
+		*type = V4L2_CTRL_TYPE_SELECTION;
+		*min = *max = *step = *def = 0;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
@@ -1336,6 +1347,12 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 	case V4L2_CTRL_TYPE_U32:
 		pr_cont("%u", (unsigned)*ptr.p_u32);
 		break;
+	case V4L2_CTRL_TYPE_SELECTION:
+		pr_cont("%ux%u@%dx%d (0x%x)",
+			ptr.p_sel->r.width, ptr.p_sel->r.height,
+			ptr.p_sel->r.left, ptr.p_sel->r.top,
+			ptr.p_sel->flags);
+		break;
 	default:
 		pr_cont("unknown type %d", ctrl->type);
 		break;
@@ -1429,6 +1446,10 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
 			return -ERANGE;
 		return 0;
 
+	case V4L2_CTRL_TYPE_SELECTION:
+		/* TODO: check for valid rectangle */
+		return 0;
+
 	default:
 		return -EINVAL;
 	}
@@ -1998,6 +2019,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_U32:
 		elem_size = sizeof(u32);
 		break;
+	case V4L2_CTRL_TYPE_SELECTION:
+		elem_size = sizeof(struct v4l2_ctrl_selection);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 8b93021..1c2fbf3 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -140,8 +140,13 @@ enum v4l2_colorfx {
 #define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
 #define V4L2_CID_COLORFX_CBCR			(V4L2_CID_BASE+42)
 
+#define V4L2_CID_CAPTURE_CROP			(V4L2_CID_BASE+43)
+#define V4L2_CID_CAPTURE_COMPOSE		(V4L2_CID_BASE+44)
+#define V4L2_CID_OUTPUT_CROP			(V4L2_CID_BASE+45)
+#define V4L2_CID_OUTPUT_COMPOSE			(V4L2_CID_BASE+46)
+
 /* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+43)
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+47)
 
 /* USER-class private control IDs */
 
-- 
2.1.0

[RFC PATCH 11/11] vivid: add crop/compose selection control support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vivid/vivid-core.h    |  5 ++++
 drivers/media/platform/vivid/vivid-ctrls.c   | 37 ++++++++++++++++++++++++++++
 drivers/media/platform/vivid/vivid-vid-cap.c | 10 ++++++--
 drivers/media/platform/vivid/vivid-vid-cap.h |  1 +
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 811c286..f9d6c45 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -214,6 +214,11 @@ struct vivid_dev {
 		struct v4l2_ctrl	*ctrl_dv_timings_signal_mode;
 		struct v4l2_ctrl	*ctrl_dv_timings;
 	};
+	struct {
+		/* capture selection crop/compose cluster */
+		struct v4l2_ctrl	*ctrl_cap_crop;
+		struct v4l2_ctrl	*ctrl_cap_compose;
+	};
 	struct v4l2_ctrl		*ctrl_has_crop_cap;
 	struct v4l2_ctrl		*ctrl_has_compose_cap;
 	struct v4l2_ctrl		*ctrl_has_scaler_cap;
diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
index 42376a1..04d18be 100644
--- a/drivers/media/platform/vivid/vivid-ctrls.c
+++ b/drivers/media/platform/vivid/vivid-ctrls.c
@@ -398,6 +398,25 @@ static int vivid_vid_cap_s_ctrl(struct v4l2_ctrl *ctrl)
 		if (dev->edid_blocks > dev->edid_max_blocks)
 			dev->edid_blocks = dev->edid_max_blocks;
 		break;
+	case V4L2_CID_CAPTURE_CROP: {
+		struct v4l2_selection s = { V4L2_BUF_TYPE_VIDEO_CAPTURE };
+		struct v4l2_ctrl_selection *crop = dev->ctrl_cap_crop->p_new.p_sel;
+		struct v4l2_ctrl_selection *compose = dev->ctrl_cap_compose->p_new.p_sel;
+		int ret;
+
+		if (ctrl->store)
+			break;
+		s.target = V4L2_SEL_TGT_CROP;
+		s.r = crop->r;
+		s.flags = crop->flags;
+		ret = vivid_vid_cap_s_selection_int(dev, &s);
+		if (ret)
+			return ret;
+		s.target = V4L2_SEL_TGT_COMPOSE;
+		s.r = compose->r;
+		s.flags = compose->flags;
+		return vivid_vid_cap_s_selection_int(dev, &s);
+	}
 	}
 	return 0;
 }
@@ -668,6 +687,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_limited_rgb_range = {
 	.step = 1,
 };
 
+static const struct v4l2_ctrl_config vivid_ctrl_capture_crop = {
+	.ops = &vivid_vid_cap_ctrl_ops,
+	.id = V4L2_CID_CAPTURE_CROP,
+	.dims = { 1 },
+	.can_store = true,
+};
+
+static const struct v4l2_ctrl_config vivid_ctrl_capture_compose = {
+	.ops = &vivid_vid_cap_ctrl_ops,
+	.id = V4L2_CID_CAPTURE_COMPOSE,
+	.dims = { 1 },
+	.can_store = true,
+};
 
 /* VBI Capture Control */
 
@@ -1263,6 +1295,10 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 		dev->colorspace = v4l2_ctrl_new_custom(hdl_vid_cap,
 			&vivid_ctrl_colorspace, NULL);
 		v4l2_ctrl_new_custom(hdl_vid_cap, &vivid_ctrl_alpha_mode, NULL);
+		dev->ctrl_cap_crop = v4l2_ctrl_new_custom(hdl_vid_cap,
+				&vivid_ctrl_capture_crop, NULL);
+		dev->ctrl_cap_compose = v4l2_ctrl_new_custom(hdl_vid_cap,
+				&vivid_ctrl_capture_compose, NULL);
 	}
 
 	if (dev->has_vid_out && show_ccs_out) {
@@ -1435,6 +1471,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_aud, NULL);
 		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_streaming, NULL);
 		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_sdtv_cap, NULL);
+		v4l2_ctrl_cluster(2, &dev->ctrl_cap_crop);
 		if (hdl_vid_cap->error)
 			return hdl_vid_cap->error;
 		dev->vid_cap_dev.ctrl_handler = hdl_vid_cap;
diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
index 95eda68..934ea3d 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -831,9 +831,8 @@ int vivid_vid_cap_g_selection(struct file *file, void *priv,
 	return 0;
 }
 
-int vivid_vid_cap_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
+int vivid_vid_cap_s_selection_int(struct vivid_dev *dev, struct v4l2_selection *s)
 {
-	struct vivid_dev *dev = video_drvdata(file);
 	struct v4l2_rect *crop = &dev->crop_cap;
 	struct v4l2_rect *compose = &dev->compose_cap;
 	unsigned factor = V4L2_FIELD_HAS_T_OR_B(dev->field_cap) ? 2 : 1;
@@ -967,6 +966,13 @@ int vivid_vid_cap_s_selection(struct file *file, void *fh, struct v4l2_selection
 	return 0;
 }
 
+int vivid_vid_cap_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
+{
+	struct vivid_dev *dev = video_drvdata(file);
+
+	return vivid_vid_cap_s_selection_int(dev, s);
+}
+
 int vivid_vid_cap_cropcap(struct file *file, void *priv,
 			      struct v4l2_cropcap *cap)
 {
diff --git a/drivers/media/platform/vivid/vivid-vid-cap.h b/drivers/media/platform/vivid/vivid-vid-cap.h
index 9407981..e7d36cb 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.h
+++ b/drivers/media/platform/vivid/vivid-vid-cap.h
@@ -39,6 +39,7 @@ int vidioc_g_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f);
 int vidioc_try_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f);
 int vidioc_s_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f);
 int vivid_vid_cap_g_selection(struct file *file, void *priv, struct v4l2_selection *sel);
+int vivid_vid_cap_s_selection_int(struct vivid_dev *dev, struct v4l2_selection *s);
 int vivid_vid_cap_s_selection(struct file *file, void *fh, struct v4l2_selection *s);
 int vivid_vid_cap_cropcap(struct file *file, void *priv, struct v4l2_cropcap *cap);
 int vidioc_enum_fmt_vid_overlay(struct file *file, void  *priv, struct v4l2_fmtdesc *f);
-- 
2.1.0

Re: [RFC PATCH 00/11] Add configuration store support

[Sakari Ailus]

Hi Hans,

Thank you for the set, and my apologies for taking a look only now.

On Sun, Sep 21, 2014 at 04:48:18PM +0200, Hans Verkuil wrote:
> This patch series adds support for configuration stores to the control
> framework. This allows you to store control values for a particular
> configuration (up to VIDEO_MAX_FRAME configuration stores are currently
> supported). When you queue a new buffer you can supply the store ID and
> the driver will apply all controls for that configuration store.
> 
> When you set a new value for a configuration store then you can choose
> whether this is 'fire and forget', i.e. after the driver applies the
> control value for that store it won't be applied again until a new value
> is set. Or you can set the value every time that configuration store is
> applied.

This does work for video device nodes but not for sub-device nodes which
have no buffer queues. Also if you think of using just a value from the
closest video buffer queue, that doesn't work either since there could be
more than one of those.

Most of the time the controls that need to be applied on per-frame basis are
present in embedded systems with complex media pipelines where most of the
controls are present on sub-device nodes.

In other words this approach alone is not sufficient to bind control related
configurations to individual frames. For preparing and applying
configurations it is applicable.

Thinking about the Android camera API v3, controls are a part of the picture
only: capture requests contain buffer sets as well. I think the concept
makes sense also outside Android. Let's discuss this further at the Media
summit.

-- 
Kind regards,

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

Re: [RFC PATCH 00/11] Add configuration store support

[Hans Verkuil]

On 10/09/14 13:55, Sakari Ailus wrote:
> Hi Hans,
> 
> Thank you for the set, and my apologies for taking a look only now.
> 
> On Sun, Sep 21, 2014 at 04:48:18PM +0200, Hans Verkuil wrote:
>> This patch series adds support for configuration stores to the control
>> framework. This allows you to store control values for a particular
>> configuration (up to VIDEO_MAX_FRAME configuration stores are currently
>> supported). When you queue a new buffer you can supply the store ID and
>> the driver will apply all controls for that configuration store.
>>
>> When you set a new value for a configuration store then you can choose
>> whether this is 'fire and forget', i.e. after the driver applies the
>> control value for that store it won't be applied again until a new value
>> is set. Or you can set the value every time that configuration store is
>> applied.
> 
> This does work for video device nodes but not for sub-device nodes which
> have no buffer queues.

The subdevices may not have buffer queues, but the high-level media driver
will know when a subdevice has to apply a specific configuration store and
can tell the subdev to do so. That's the way I expect it to work.

> Also if you think of using just a value from the
> closest video buffer queue, that doesn't work either since there could be
> more than one of those.

Good point. One solution might be to allow for a larger range of config
store IDs (i.e., more than just 1-32, where 32 is the current maximum number
of buffers). That way different buffer queues can use unique config store
IDs. This does make the internal data structures a bit more complex, but it's
not a big deal.

> 
> Most of the time the controls that need to be applied on per-frame basis are
> present in embedded systems with complex media pipelines where most of the
> controls are present on sub-device nodes.
> 
> In other words this approach alone is not sufficient to bind control related
> configurations to individual frames. For preparing and applying
> configurations it is applicable.
> 
> Thinking about the Android camera API v3, controls are a part of the picture
> only: capture requests contain buffer sets as well. I think the concept
> makes sense also outside Android. Let's discuss this further at the Media
> summit.

Let's do that.

Regards,

	Hans