Re: [RFC v5 06/15] media: uapi: Add V4L2_CID_CONFIG_MODEL control

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Mon, Feb 10, 2025 at 03:07:34PM +0100, Hans Verkuil wrote:
> On 10/02/2025 14:25, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 10, 2025 at 10:09:33AM +0100, Hans Verkuil wrote:
> >> Hi Sakari,
> >>
> >> On 03/02/2025 09:58, Sakari Ailus wrote:
> >>> Add the V4L2_CID_CONFIG_MODEL control for the configuration model.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>  .../userspace-api/media/v4l/ext-ctrls-image-process.rst      | 4 ++++
> >>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                    | 5 +++++
> >>>  include/uapi/linux/v4l2-controls.h                           | 3 +++
> >>>  3 files changed, 12 insertions(+)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst
> >>> index 27803dca8d3e..2ae17ed99729 100644
> >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst
> >>> @@ -55,3 +55,7 @@ Image Process Control IDs
> >>>      control value divided by e.g. 0x100, meaning that to get no
> >>>      digital gain the control value needs to be 0x100. The no-gain
> >>>      configuration is also typically the default.
> >>> +
> >>> +``V4L2_CID_CONFIG_MODEL (bitmask)``
> >>> +    Which configuration models the sub-device supports. Please see
> >>> +    :ref:`media_subdev_config_model`. This is a read-only control.
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> index 1ea52011247a..24c9c25e20d1 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> @@ -1164,6 +1164,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>  	case V4L2_CID_TEST_PATTERN:		return "Test Pattern";
> >>>  	case V4L2_CID_DEINTERLACING_MODE:	return "Deinterlacing Mode";
> >>>  	case V4L2_CID_DIGITAL_GAIN:		return "Digital Gain";
> >>> +	case V4L2_CID_CONFIG_MODEL:		return "Sub-device configuration model";
> >>>  
> >>>  	/* DV controls */
> >>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> >>> @@ -1481,6 +1482,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>  	case V4L2_CID_DV_RX_POWER_PRESENT:
> >>>  		*type = V4L2_CTRL_TYPE_BITMASK;
> >>>  		break;
> >>> +	case V4L2_CID_CONFIG_MODEL:
> >>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >>> +		*type = V4L2_CTRL_TYPE_BITMASK;
> >>> +		break;
> >>>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
> >>>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
> >>>  		*type = V4L2_CTRL_TYPE_INTEGER;
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index 974fd254e573..731add75d9ee 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -1225,6 +1225,9 @@ enum v4l2_jpeg_chroma_subsampling {
> >>>  #define V4L2_CID_TEST_PATTERN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
> >>>  #define V4L2_CID_DEINTERLACING_MODE		(V4L2_CID_IMAGE_PROC_CLASS_BASE + 4)
> >>>  #define V4L2_CID_DIGITAL_GAIN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 5)
> >>> +#define V4L2_CID_CONFIG_MODEL			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 6)
> >>> +
> >>> +#define V4L2_CID_CONFIG_MODEL_COMMON_RAW	(1U << 0)
> >>
> >> I think I mentioned this before, but what's the point of this?
> > 
> > I recall Laurent was last to reply to the thread, with a good explanation
> > of the purpose. The message id is
> > <20241118024002.GJ31681@pendragon.ideasonboard.com> .
> 
> Now document that in this patch series. I double checked patch 04/15 again,
> and there is no mention of that explanation from Laurent. It should be incorporated
> in that patch.
> 
> Open questions:
> 
> 1) If this control is not available, but it is still a camera sensor, what is
>    userspace supposed to to? I guess all it can do is assume that the driver
>    follows the standard rules, since there is no way to figure out if there are
>    differences. So again, how does this help? In any case, this should be
>    documented as well.

The user space cannot expect the sub-device UAPI conforms to the common raw
sensor model without this control. The purpose of the model is indeed to
allow the user space to expect certain things to be supported in the API
and if they are supported, they work in a certain way. The alternative is
to do this driver by driver and there are quite a few sensor drivers out
there, with more to come.

> 
> 2) Are there compliance tests for this? Because there is no point adding this
>    without having tests for it as well. Otherwise I can 100% guarantee that
>    drivers will set this even if it deviates from what it should do in some
>    way. Relying on code review alone is going to be a very tough job.

Compliance tests need to be added, I agree.

> 
> > 
> >>
> >> You are adding a control describing a configuration model, but it has
> >> just a single possible configuration model. I see no description anywhere
> >> about when a new model would need to be added, or what userspace is
> >> supposed to do with this.
> > 
> > At this point I'm not sure how many other configuration models might be
> > needed or when they would be needed.
> > 
> >>
> >> And as long as there is only one model anyway, I don't see the point of
> >> this control.
> > 
> > I could create a control just for the common raw sensor model but 
> > 
> >>
> >> Is the intention that all sensor drivers will set this control? The RFC
> >> series isn't clear about this.
> > 
> > I'd expect almost all new raw sensor drivers to expose this control with
> > the common raw bit set.
> > 
> >>
> >> The problem I see with this series is that it seems to mix seemingly
> >> unrelated changes: adding COLOUR_PATTERN/BINNING controls doesn't seem to
> >> depend on configuration models. Or if they do, I clearly didn't get that.
> > 
> > These are all related to sensor API improvements. There is no direct
> > dependency, no, but I expect drivers implementing the common raw sensor
> > model would also support these controls. I can document this.
> 
> Just split it up in two separate series. I have no objections to the sensor API
> improvements, so it is much easier to get that in.
> 
> But I think that the 'config_model' part is poorly documented and I am quite
> skeptical about the whole thing. So that shouldn't block the other changes
> from this RFC series.

One option could be indeed to postpone this, and continue working on the
user space one driver at a time. My initial thought really was we could
require the common raw sensor model to be used with internal pads,
otherwise it'll be harder to ensure sensor drivers using them will be using
them consistently: compliance tests would require quite a few exceptions
for instance (doing the tests only for sensors that are non-CCS for
instance).

I wonder what Laurent thinks.

-- 
Regards,

Sakari Ailus