Re: [RFC v3 6/9] media: uapi: Add V4L2_CID_CONFIG_MODEL control

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

Hi Jacopo,

On Thu, Dec 05, 2024 at 09:43:35AM +0100, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Fri, Nov 29, 2024 at 11:51:39AM +0200, 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..928e8e3eed7f 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst
> 
> I was initially not sure this belongs to "Image process" controls, but
> having had a look at the existing ones I'm not sure there are better
> places.

Good question. Let's keep it here for the time being. I'm not sure any
other extended control class is a better target. Maybe old user controls? I
wonder what Hans would think.

> 
> I would have considered "Camera Controls" but I'm not sure it's -that-
> better
> 
> > @@ -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`.
> 
> Other RO controls in the file (such as PIXEL_RATE) specify "this
> control is read-only"

I'll add that.

> 
> > 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..0152240229ab 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	(1ULL << 0)
> 
> I wonder if "COMMON_" couldn't be removed.

We might have different models for raw sensors. These are just for the
current ones but new ones will have new functionality and we may need to
make changes that won't be compliant. Maybe in that case we'd have v2 or
something though.

> 
> Anyway, all nits or mostly questions, whatever is fine really
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thank you!

-- 
Kind regards,

Sakari Ailus