Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org,
	Xavier Roumegue <xavier.roumegue@oss.nxp.com>
Subject: Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change
Date: Fri, 8 Jul 2022 12:59:41 +0200	[thread overview]
Message-ID: <c02fe42a-5694-06b4-c140-1e42ca9fe8ac@xs4all.nl> (raw)
In-Reply-To: <YsgJ1OpWCERSVqrB@pendragon.ideasonboard.com>

Hi Laurent,

On 7/8/22 12:41, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tue, Jun 28, 2022 at 02:05:22PM +0200, Hans Verkuil wrote:
>> Add a new V4L2_EVENT_CTRL_CH_DIMENSIONS change flag that is issued
>> when the dimensions of an array change as a result of a
>> __v4l2_ctrl_modify_dimensions() call.
>>
>> This will inform userspace that there are new dimensions.
> 
> While this is easy to add, I'm not sure it will actually be useful in
> real use cases. Should we delay adding this new API until someone
> actually needs it ?

Well, there is a user: dw100. This driver can change dimensions, so any
userspace application that subscribes to such a control has to be able to
know that the dimensions of that control have changed. Otherwise it will
not be able to correctly obtain the control's value.

It's not really about this specific driver, it is about the new control
feature where dimensions of an array can change. It's also consistent
with the other control event change flags.

> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  Documentation/userspace-api/media/v4l/vidioc-dqevent.rst     | 5 +++++
>>  Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 +
>>  drivers/media/v4l2-core/v4l2-ctrls-api.c                     | 2 ++
>>  include/uapi/linux/videodev2.h                               | 1 +
>>  4 files changed, 9 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>> index 6eb40073c906..8db103760930 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>> @@ -332,6 +332,11 @@ call.
>>        - 0x0004
>>        - This control event was triggered because the minimum, maximum,
>>  	step or the default value of the control changed.
>> +    * - ``V4L2_EVENT_CTRL_CH_DIMENSIONS``
>> +      - 0x0008
>> +      - This control event was triggered because the dimensions of the
>> +	control changed. Note that the number of dimensions remains the
>> +	same.
>>  
>>  
>>  .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.5cm}|
>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> index 0b91200776f8..274474425b05 100644
>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> @@ -506,6 +506,7 @@ replace define V4L2_EVENT_PRIVATE_START event-type
>>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
>>  replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags
>>  replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags
>> +replace define V4L2_EVENT_CTRL_CH_DIMENSIONS ctrl-changes-flags
>>  
>>  replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> index 16be31966cb1..47f69de9a067 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> @@ -1019,6 +1019,8 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl,
>>  		ctrl->type_ops->init(ctrl, i, ctrl->p_cur);
>>  		ctrl->type_ops->init(ctrl, i, ctrl->p_new);
>>  	}
>> +	send_event(NULL, ctrl,
>> +		   V4L2_EVENT_CTRL_CH_VALUE | V4L2_EVENT_CTRL_CH_DIMENSIONS);
> 
> Sending V4L2_EVENT_CTRL_CH_VALUE propably belongs to the previous patch
> already.

True.

Regards,

	Hans

> 
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions);
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 9018aa984db3..3971af623c56 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2398,6 +2398,7 @@ struct v4l2_event_vsync {
>>  #define V4L2_EVENT_CTRL_CH_VALUE		(1 << 0)
>>  #define V4L2_EVENT_CTRL_CH_FLAGS		(1 << 1)
>>  #define V4L2_EVENT_CTRL_CH_RANGE		(1 << 2)
>> +#define V4L2_EVENT_CTRL_CH_DIMENSIONS		(1 << 3)
>>  
>>  struct v4l2_event_ctrl {
>>  	__u32 changes;
> 