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 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> .

> 
> 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.

-- 
Kind regards,

Sakari Ailus