On Wed, Aug 9, 2023 at 7:59 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > On 8/9/23 12:15, Laurent Pinchart wrote: > > Hello, > > > > On Wed, Aug 09, 2023 at 10:36:16AM +0200, Hans Verkuil wrote: > >> On 8/9/23 09:34, Yunke Cao wrote: > >>> On Wed, Aug 9, 2023 at 4:05 PM Hans Verkuil wrote: > >>>> On 26/04/2023 10:29, Yunke Cao wrote: > >>>>> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > >>>>> > >>>>> Add the capability of retrieving the min and max values of a > >>>>> compound control. > >>>>> > >>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > >>>>> Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx> > >>>>> --- > >>>>> Changelog since v10: > >>>>> - No change. > >>>>> Changelog since v9: > >>>>> - No change. > >>>>> Changelog since v8: > >>>>> - Return ENODATA when min/max is not implemented. Document this behavior. > >>>>> - Created a shared helper function __v4l2_ctrl_type_op_init that takes "which" > >>>>> as a parameter. Call it in def, min and max operations. > >>>>> Changelog since v7: > >>>>> - Document that the definition of the min/max are provided by compound controls > >>>>> are defined in control documentation. > >>>>> - Return error, instead of zeroed memory for v4l2_ctrl_ptr_create(NULL). > >>>>> > >>>>> git am from https://lore.kernel.org/all/20191119113457.57833-3-hverkuil-cisco@xxxxxxxxx/ > >>>>> - Fixed some merge conflits. > >>>>> - Fixed the build error in drivers/media/platform/qcom/venus. > >>>>> > >>>>> .../media/v4l/vidioc-g-ext-ctrls.rst | 11 +- > >>>>> .../media/videodev2.h.rst.exceptions | 2 + > >>>>> drivers/media/i2c/imx214.c | 5 +- > >>>>> .../media/platform/qcom/venus/venc_ctrls.c | 9 +- > >>>>> drivers/media/v4l2-core/v4l2-ctrls-api.c | 57 +++++-- > >>>>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 156 +++++++++++++++--- > >>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > >>>>> include/media/v4l2-ctrls.h | 34 +++- > >>>>> include/uapi/linux/videodev2.h | 2 + > >>>>> 9 files changed, 236 insertions(+), 44 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > >>>>> index 927ef397f1ce..1cc21ee229aa 100644 > >>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > >>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > >>>>> @@ -304,14 +304,21 @@ still cause this situation. > >>>>> - Which value of the control to get/set/try. > >>>>> * - :cspan:`2` ``V4L2_CTRL_WHICH_CUR_VAL`` will return the current value of > >>>>> the control, ``V4L2_CTRL_WHICH_DEF_VAL`` will return the default > >>>>> + value of the control, ``V4L2_CTRL_WHICH_MIN_VAL`` will return the minimum > >>>>> + value of the control, ``V4L2_CTRL_WHICH_MAX_VAL`` will return the maximum > >>>>> value of the control and ``V4L2_CTRL_WHICH_REQUEST_VAL`` indicates that > >>>>> these controls have to be retrieved from a request or tried/set for > >>>>> a request. In the latter case the ``request_fd`` field contains the > >>>>> file descriptor of the request that should be used. If the device > >>>>> does not support requests, then ``EACCES`` will be returned. > >>>>> > >>>>> - When using ``V4L2_CTRL_WHICH_DEF_VAL`` be aware that you can only > >>>>> - get the default value of the control, you cannot set or try it. > >>>>> + When using ``V4L2_CTRL_WHICH_DEF_VAL``, ``V4L2_CTRL_WHICH_MIN_VAL`` > >>>>> + or ``V4L2_CTRL_WHICH_MAX_VAL`` be aware that you can only get the > >>>>> + default/minimum/maximum value of the control, you cannot set or try it. > >>>>> + The definition of minimum/maximum values for compound types are provided by > >>>>> + the control documentation. If the control documentation does not > >>>>> + document the meaning of minimum/maximum value, then it is not supported. > >>>>> + Querying its minmimum/maximum value will result in -ENODATA. > >>>> > >>>> typo: minmimum -> minimum > >>>> > >>>> That last line is a bit ambiguous, I suggest this: > >>>> > >>>> If ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` are not supported, > >>>> then querying the minimum or maximum value will result in -ENODATA. > >>> > >>> This sounds clearer indeed! I will change it in the next version. > >> > >> Thinking some more about this, I believe it would be better to add a flag > >> indicating WHICH_MIN/MAX support. I never like relying on an error to > >> discover a feature. You still need this error, but in addition we need a new > >> flag: > >> > >> #define V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX 0x1000 > >> > >> that is set for any control that supports this. > > > > I think the intent here was to indicate that drivers must return > > -ENODATA for V4L2_CTRL_WHICH_MIN_VAL and V4L2_CTRL_WHICH_MAX_VAL if the > > control's documentation doesn't specify the meaning of minimum and > > maximum for a control. A flag to indicate support for this new API is > > likely a good idea, but the documentation here should still clearly > > indicate that only controls that have defined minimum and maximum > > concepts in the API documentation can implement this API. > > This flag is specific to the control ID: so if set, then you can get > the min/max value using V4L2_CTRL_WHICH_MIN/MAX_VAL for that control ID. > > This flag must be set for any control that uses the s64 minimum/maximum > fields in struct v4l2_ext_query_ctrl, and for any compound control that > has explicit support for MIN/MAX_VAL (the UVC rectangle control in this > case). > any control that uses the s64 minimum/maximum fields Noob question: does this include all the non-compound controls? Are drivers responsible for setting this flag for these controls? Best, Yunke > > Regards, > > Hans > >
