| Message ID | 20250203085853.1361401-7-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Sub-device configuration models | expand |
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.
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. 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. > >> >> 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. Regards, Hans
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) /* DV-class control IDs defined by V4L2 */ #define V4L2_CID_DV_CLASS_BASE (V4L2_CTRL_CLASS_DV | 0x900)