On 8/9/23 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. >>>>> >>>>> 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? Yes. Almost all of those have valid and always present min/max ranges. > Are drivers responsible for setting this flag for these controls? No, the v4l2 control framework provides that automatically. It just needs to set the flag. Note that there are some standard types that do not support this (BUTTON, CTRL_CLASS are the only ones I think), so those should not set this flag. I think so, at least. Might have to sleep on it for a bit. Regards, Hans > > Best, > Yunke > >> >> Regards, >> >> Hans >> >>
