LinuxTV Patchwork [PATCHv2,4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS

login
register
mail settings
Submitter Hans Verkuil
Date Feb. 11, 2019, 10:13 a.m.
Message ID <20190211101357.48754-5-hverkuil-cisco@xs4all.nl>
Download mbox | patch
Permalink /patch/54440/
State New
Delegated to: Hans Verkuil
Headers show

Comments

Hans Verkuil - Feb. 11, 2019, 10:13 a.m.
Indicate if a control can only be set through a request, as opposed
to being set directly. This is necessary for stateless codecs where
it makes no sense to set the state controls directly.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
 .../media/videodev2.h.rst.exceptions          |  1 +
 include/uapi/linux/videodev2.h                | 23 ++++++++++---------
 3 files changed, 17 insertions(+), 11 deletions(-)
Hans Verkuil - Feb. 11, 2019, 1:04 p.m.
On 2/11/19 11:13 AM, Hans Verkuil wrote:
> Indicate if a control can only be set through a request, as opposed
> to being set directly. This is necessary for stateless codecs where
> it makes no sense to set the state controls directly.

Kwiboo on irc pointed out that this clashes with this line the in Initialization
section of the stateless decoder API:

"Call VIDIOC_S_EXT_CTRLS() to set all the controls (parsed headers, etc.) required
 by the OUTPUT format to enumerate the CAPTURE formats."

So for now ignore patches 4-6: I need to think about this some more.

My worry here is what happens when userspace is adding these controls to a
request and at the same time sets them directly. That may cause weird side-effects.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  include/uapi/linux/videodev2.h                | 23 ++++++++++---------
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index f824162d0ea9..b08c69cedb92 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -539,6 +539,10 @@ See also the examples in :ref:`control`.
>  	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
>  	streaming is in progress since most drivers do not support changing
>  	the format in that case.
> +    * - ``V4L2_CTRL_FLAG_REQUIRES_REQUESTS``
> +      - 0x0800
> +      - This control cannot be set directly, but only through a request
> +        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
>  
>  
>  Return Value
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index 64d348e67df9..0caa72014dba 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -351,6 +351,7 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
>  replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
>  replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
>  replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
> +replace define V4L2_CTRL_FLAG_REQUIRES_REQUESTS control-flags
>  
>  replace define V4L2_CTRL_FLAG_NEXT_CTRL control
>  replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 7f035d44666e..a78bfdc1df97 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
>  } __attribute__ ((packed));
>  
>  /*  Control flags  */
> -#define V4L2_CTRL_FLAG_DISABLED		0x0001
> -#define V4L2_CTRL_FLAG_GRABBED		0x0002
> -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
> -#define V4L2_CTRL_FLAG_UPDATE		0x0008
> -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
> -#define V4L2_CTRL_FLAG_SLIDER		0x0020
> -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
> -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
> -#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
> -#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
> -#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
> +#define V4L2_CTRL_FLAG_DISABLED			0x0001
> +#define V4L2_CTRL_FLAG_GRABBED			0x0002
> +#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
> +#define V4L2_CTRL_FLAG_UPDATE			0x0008
> +#define V4L2_CTRL_FLAG_INACTIVE			0x0010
> +#define V4L2_CTRL_FLAG_SLIDER			0x0020
> +#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
> +#define V4L2_CTRL_FLAG_VOLATILE			0x0080
> +#define V4L2_CTRL_FLAG_HAS_PAYLOAD		0x0100
> +#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE		0x0200
> +#define V4L2_CTRL_FLAG_MODIFY_LAYOUT		0x0400
> +#define V4L2_CTRL_FLAG_REQUIRES_REQUESTS	0x0800
>  
>  /*  Query flags, to be ORed with the control ID */
>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
>
Paul Kocialkowski - Feb. 13, 2019, 10:38 a.m.
Hi,

On Mon, 2019-02-11 at 14:04 +0100, Hans Verkuil wrote:
> On 2/11/19 11:13 AM, Hans Verkuil wrote:
> > Indicate if a control can only be set through a request, as opposed
> > to being set directly. This is necessary for stateless codecs where
> > it makes no sense to set the state controls directly.
> 
> Kwiboo on irc pointed out that this clashes with this line the in Initialization
> section of the stateless decoder API:
> 
> "Call VIDIOC_S_EXT_CTRLS() to set all the controls (parsed headers, etc.) required
>  by the OUTPUT format to enumerate the CAPTURE formats."
> 
> So for now ignore patches 4-6: I need to think about this some more.
> 
> My worry here is what happens when userspace is adding these controls to a
> request and at the same time sets them directly. That may cause weird side-effects.

This seems to be a very legitimate concern, as nothing guarantees that
the controls setup by v4l2_ctrl_request_setup won't be overridden
before the driver uses them.

One solution could be to mark the controls as "in use" when the request
has new data for them, clear that in v4l2_ctrl_request_complete and
return EBUSY when trying to set the control in between the two calls.

This way, we ensure that any control set via a request will retain the
value passed with the request, which is independent from the control
itself (so we don't need special handling for stateless codec
controls). It also allows setting the control outside of a request for
enumerating formats.

What do you think?

Cheers,

Paul

> Regards,
> 
> 	Hans
> 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> >  .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
> >  .../media/videodev2.h.rst.exceptions          |  1 +
> >  include/uapi/linux/videodev2.h                | 23 ++++++++++---------
> >  3 files changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > index f824162d0ea9..b08c69cedb92 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > @@ -539,6 +539,10 @@ See also the examples in :ref:`control`.
> >  	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
> >  	streaming is in progress since most drivers do not support changing
> >  	the format in that case.
> > +    * - ``V4L2_CTRL_FLAG_REQUIRES_REQUESTS``
> > +      - 0x0800
> > +      - This control cannot be set directly, but only through a request
> > +        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
> >  
> >  
> >  Return Value
> > diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> > index 64d348e67df9..0caa72014dba 100644
> > --- a/Documentation/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/media/videodev2.h.rst.exceptions
> > @@ -351,6 +351,7 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
> >  replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
> >  replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
> >  replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
> > +replace define V4L2_CTRL_FLAG_REQUIRES_REQUESTS control-flags
> >  
> >  replace define V4L2_CTRL_FLAG_NEXT_CTRL control
> >  replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 7f035d44666e..a78bfdc1df97 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
> >  } __attribute__ ((packed));
> >  
> >  /*  Control flags  */
> > -#define V4L2_CTRL_FLAG_DISABLED		0x0001
> > -#define V4L2_CTRL_FLAG_GRABBED		0x0002
> > -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
> > -#define V4L2_CTRL_FLAG_UPDATE		0x0008
> > -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
> > -#define V4L2_CTRL_FLAG_SLIDER		0x0020
> > -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
> > -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
> > -#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
> > -#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
> > -#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
> > +#define V4L2_CTRL_FLAG_DISABLED			0x0001
> > +#define V4L2_CTRL_FLAG_GRABBED			0x0002
> > +#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
> > +#define V4L2_CTRL_FLAG_UPDATE			0x0008
> > +#define V4L2_CTRL_FLAG_INACTIVE			0x0010
> > +#define V4L2_CTRL_FLAG_SLIDER			0x0020
> > +#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
> > +#define V4L2_CTRL_FLAG_VOLATILE			0x0080
> > +#define V4L2_CTRL_FLAG_HAS_PAYLOAD		0x0100
> > +#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE		0x0200
> > +#define V4L2_CTRL_FLAG_MODIFY_LAYOUT		0x0400
> > +#define V4L2_CTRL_FLAG_REQUIRES_REQUESTS	0x0800
> >  
> >  /*  Query flags, to be ORed with the control ID */
> >  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
> >
Hans Verkuil - Feb. 13, 2019, 11:59 a.m.
On 2/13/19 11:38 AM, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2019-02-11 at 14:04 +0100, Hans Verkuil wrote:
>> On 2/11/19 11:13 AM, Hans Verkuil wrote:
>>> Indicate if a control can only be set through a request, as opposed
>>> to being set directly. This is necessary for stateless codecs where
>>> it makes no sense to set the state controls directly.
>>
>> Kwiboo on irc pointed out that this clashes with this line the in Initialization
>> section of the stateless decoder API:
>>
>> "Call VIDIOC_S_EXT_CTRLS() to set all the controls (parsed headers, etc.) required
>>  by the OUTPUT format to enumerate the CAPTURE formats."
>>
>> So for now ignore patches 4-6: I need to think about this some more.
>>
>> My worry here is what happens when userspace is adding these controls to a
>> request and at the same time sets them directly. That may cause weird side-effects.
> 
> This seems to be a very legitimate concern, as nothing guarantees that
> the controls setup by v4l2_ctrl_request_setup won't be overridden
> before the driver uses them.
> 
> One solution could be to mark the controls as "in use" when the request
> has new data for them, clear that in v4l2_ctrl_request_complete and
> return EBUSY when trying to set the control in between the two calls.
> 
> This way, we ensure that any control set via a request will retain the
> value passed with the request, which is independent from the control
> itself (so we don't need special handling for stateless codec
> controls). It also allows setting the control outside of a request for
> enumerating formats.
> 
> What do you think?

That's a good idea. I'll see if I can make that work.

Regards,

	Hans

> 
> Cheers,
> 
> Paul
> 
>> Regards,
>>
>> 	Hans
>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>>  .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>  include/uapi/linux/videodev2.h                | 23 ++++++++++---------
>>>  3 files changed, 17 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> index f824162d0ea9..b08c69cedb92 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> @@ -539,6 +539,10 @@ See also the examples in :ref:`control`.
>>>  	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
>>>  	streaming is in progress since most drivers do not support changing
>>>  	the format in that case.
>>> +    * - ``V4L2_CTRL_FLAG_REQUIRES_REQUESTS``
>>> +      - 0x0800
>>> +      - This control cannot be set directly, but only through a request
>>> +        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
>>>  
>>>  
>>>  Return Value
>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>>> index 64d348e67df9..0caa72014dba 100644
>>> --- a/Documentation/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>>> @@ -351,6 +351,7 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
>>>  replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
>>>  replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
>>>  replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
>>> +replace define V4L2_CTRL_FLAG_REQUIRES_REQUESTS control-flags
>>>  
>>>  replace define V4L2_CTRL_FLAG_NEXT_CTRL control
>>>  replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 7f035d44666e..a78bfdc1df97 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
>>>  } __attribute__ ((packed));
>>>  
>>>  /*  Control flags  */
>>> -#define V4L2_CTRL_FLAG_DISABLED		0x0001
>>> -#define V4L2_CTRL_FLAG_GRABBED		0x0002
>>> -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
>>> -#define V4L2_CTRL_FLAG_UPDATE		0x0008
>>> -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
>>> -#define V4L2_CTRL_FLAG_SLIDER		0x0020
>>> -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
>>> -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
>>> -#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
>>> -#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
>>> -#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
>>> +#define V4L2_CTRL_FLAG_DISABLED			0x0001
>>> +#define V4L2_CTRL_FLAG_GRABBED			0x0002
>>> +#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
>>> +#define V4L2_CTRL_FLAG_UPDATE			0x0008
>>> +#define V4L2_CTRL_FLAG_INACTIVE			0x0010
>>> +#define V4L2_CTRL_FLAG_SLIDER			0x0020
>>> +#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
>>> +#define V4L2_CTRL_FLAG_VOLATILE			0x0080
>>> +#define V4L2_CTRL_FLAG_HAS_PAYLOAD		0x0100
>>> +#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE		0x0200
>>> +#define V4L2_CTRL_FLAG_MODIFY_LAYOUT		0x0400
>>> +#define V4L2_CTRL_FLAG_REQUIRES_REQUESTS	0x0800
>>>  
>>>  /*  Query flags, to be ORed with the control ID */
>>>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
>>>

Patch

diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
index f824162d0ea9..b08c69cedb92 100644
--- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
+++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
@@ -539,6 +539,10 @@  See also the examples in :ref:`control`.
 	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
 	streaming is in progress since most drivers do not support changing
 	the format in that case.
+    * - ``V4L2_CTRL_FLAG_REQUIRES_REQUESTS``
+      - 0x0800
+      - This control cannot be set directly, but only through a request
+        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
 
 
 Return Value
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index 64d348e67df9..0caa72014dba 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -351,6 +351,7 @@  replace define V4L2_CTRL_FLAG_VOLATILE control-flags
 replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
 replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
 replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
+replace define V4L2_CTRL_FLAG_REQUIRES_REQUESTS control-flags
 
 replace define V4L2_CTRL_FLAG_NEXT_CTRL control
 replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 7f035d44666e..a78bfdc1df97 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1736,17 +1736,18 @@  struct v4l2_querymenu {
 } __attribute__ ((packed));
 
 /*  Control flags  */
-#define V4L2_CTRL_FLAG_DISABLED		0x0001
-#define V4L2_CTRL_FLAG_GRABBED		0x0002
-#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
-#define V4L2_CTRL_FLAG_UPDATE		0x0008
-#define V4L2_CTRL_FLAG_INACTIVE		0x0010
-#define V4L2_CTRL_FLAG_SLIDER		0x0020
-#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
-#define V4L2_CTRL_FLAG_VOLATILE		0x0080
-#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
-#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
-#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
+#define V4L2_CTRL_FLAG_DISABLED			0x0001
+#define V4L2_CTRL_FLAG_GRABBED			0x0002
+#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
+#define V4L2_CTRL_FLAG_UPDATE			0x0008
+#define V4L2_CTRL_FLAG_INACTIVE			0x0010
+#define V4L2_CTRL_FLAG_SLIDER			0x0020
+#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
+#define V4L2_CTRL_FLAG_VOLATILE			0x0080
+#define V4L2_CTRL_FLAG_HAS_PAYLOAD		0x0100
+#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE		0x0200
+#define V4L2_CTRL_FLAG_MODIFY_LAYOUT		0x0400
+#define V4L2_CTRL_FLAG_REQUIRES_REQUESTS	0x0800
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000

Privacy Policy