| Message ID | 20190211101357.48754-5-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vb2/v4l2-ctrls: check if requests are required | expand |
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 >
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 > >
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 >>>
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
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(-)