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). Regards, Hans
