On 09/08/2023 14:32, Yunke Cao wrote: > 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. I realized that ENODATA is wrong: ENODATA implies that while there is no data now, there might be in the future. That's not the case here. I think the correct error code is EINVAL: the value of the 'which' field is invalid for this control. Regards, Hans >>>>> >>>>> 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 >> >>
