| Message ID | 20241011075535.588140-5-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Sub-device configuration models | expand |
Hi Sakari, Thank you for the patch. On Fri, Oct 11, 2024 at 10:55:35AM +0300, Sakari Ailus wrote: > Add the V4L2_CID_CONFIG_MODEL control for the configuration model. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 4 ++++ > .../userspace-api/media/v4l/subdev-config-model.rst | 2 ++ > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++ > include/uapi/linux/v4l2-controls.h | 3 +++ > 4 files changed, 14 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 > @@ -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`. > diff --git a/Documentation/userspace-api/media/v4l/subdev-config-model.rst b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > index 8ec801998f5f..d4ae921b69c8 100644 > --- a/Documentation/userspace-api/media/v4l/subdev-config-model.rst > +++ b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > @@ -1,5 +1,7 @@ > .. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-no-invariants-or-later > > +.. _media_subdev_config_model: > + This could be moved to 3/4. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sub-device configuration models > =============================== > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > index 6b9188a4a220..378657a52cd5 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > @@ -1167,6 +1167,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! */ > @@ -1489,6 +1490,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) > > /* DV-class control IDs defined by V4L2 */ > #define V4L2_CID_DV_CLASS_BASE (V4L2_CTRL_CLASS_DV | 0x900)
Hi Laurent, On Tue, Oct 22, 2024 at 10:42:53PM +0300, Laurent Pinchart wrote: > > index 8ec801998f5f..d4ae921b69c8 100644 > > --- a/Documentation/userspace-api/media/v4l/subdev-config-model.rst > > +++ b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > > @@ -1,5 +1,7 @@ > > .. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-no-invariants-or-later > > > > +.. _media_subdev_config_model: > > + > > This could be moved to 3/4. Yes, that's where it belongs indeed. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks!
On 11/10/2024 09:55, Sakari Ailus wrote: > Add the V4L2_CID_CONFIG_MODEL control for the configuration model. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 4 ++++ > .../userspace-api/media/v4l/subdev-config-model.rst | 2 ++ > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++ > include/uapi/linux/v4l2-controls.h | 3 +++ > 4 files changed, 14 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 > @@ -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`. First of all the naming is confusing: since this is specific to sub-devices, it should at least have 'SUBDEV' in the name. I first thought this reported the model name or something like that, I'm not sure "configuration model" is a very good name. Secondly, is this supposed to be valid for all subdevices? Only for sensors? Would an HDMI-to-CSI bridge qualify? Thirdly, only V4L2_CID_CONFIG_MODEL_COMMON_RAW is defined right now. What other models do you have in mind? What models can co-exist (since this is a bitmask)? Finally, why choose a control for this? Should this perhaps be better done as a field in media_entity_desc/media_v2_entity? Regards, Hans > diff --git a/Documentation/userspace-api/media/v4l/subdev-config-model.rst b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > index 8ec801998f5f..d4ae921b69c8 100644 > --- a/Documentation/userspace-api/media/v4l/subdev-config-model.rst > +++ b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > @@ -1,5 +1,7 @@ > .. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-no-invariants-or-later > > +.. _media_subdev_config_model: > + > Sub-device configuration models > =============================== > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > index 6b9188a4a220..378657a52cd5 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > @@ -1167,6 +1167,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"; Start each word capitalized, just like all the other strings. > > /* DV controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > @@ -1489,6 +1490,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) > > /* DV-class control IDs defined by V4L2 */ > #define V4L2_CID_DV_CLASS_BASE (V4L2_CTRL_CLASS_DV | 0x900)
Hi Hans, Thank you for the review. On Wed, Nov 13, 2024 at 09:03:57AM +0100, Hans Verkuil wrote: > On 11/10/2024 09:55, Sakari Ailus wrote: > > Add the V4L2_CID_CONFIG_MODEL control for the configuration model. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 4 ++++ > > .../userspace-api/media/v4l/subdev-config-model.rst | 2 ++ > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++ > > include/uapi/linux/v4l2-controls.h | 3 +++ > > 4 files changed, 14 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 > > @@ -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`. > > First of all the naming is confusing: since this is specific to sub-devices, it > should at least have 'SUBDEV' in the name. I first thought this reported the I don't object in principle, but the reason why I didn't add that in v1 was the names would get quite long. Maybe V4L2_CID_SUBDEV_CFG_MODEL? > model name or something like that, I'm not sure "configuration model" is a very > good name. Feel free to propose a different one. :-) > > Secondly, is this supposed to be valid for all subdevices? Only for sensors? > Would an HDMI-to-CSI bridge qualify? I think it could but we should have a use case for it. In other words, something we can't reasonably express using existing means. In this case it's a number of interfaces and device type specific behaviour (see the 3rd patch). > > Thirdly, only V4L2_CID_CONFIG_MODEL_COMMON_RAW is defined right now. What other > models do you have in mind? What models can co-exist (since this is a bitmask)? We could have different raw camera models if needed. I don't have any planned right now, though. > > Finally, why choose a control for this? Should this perhaps be better done as > a field in media_entity_desc/media_v2_entity? I don't think it's a great fit. This is largely about V4L2 (to some but lesser extent about MC) and we traditionally have avoided MC -> V4L2 dependencies.
On 11/13/24 09:35, Sakari Ailus wrote: > Hi Hans, > > Thank you for the review. > > On Wed, Nov 13, 2024 at 09:03:57AM +0100, Hans Verkuil wrote: >> On 11/10/2024 09:55, Sakari Ailus wrote: >>> Add the V4L2_CID_CONFIG_MODEL control for the configuration model. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 4 ++++ >>> .../userspace-api/media/v4l/subdev-config-model.rst | 2 ++ >>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++ >>> include/uapi/linux/v4l2-controls.h | 3 +++ >>> 4 files changed, 14 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 >>> @@ -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`. >> >> First of all the naming is confusing: since this is specific to sub-devices, it >> should at least have 'SUBDEV' in the name. I first thought this reported the > > I don't object in principle, but the reason why I didn't add that in v1 was > the names would get quite long. Maybe V4L2_CID_SUBDEV_CFG_MODEL? > >> model name or something like that, I'm not sure "configuration model" is a very >> good name. > > Feel free to propose a different one. :-) I would, if I understood what you intend to achieve :-) > >> >> Secondly, is this supposed to be valid for all subdevices? Only for sensors? >> Would an HDMI-to-CSI bridge qualify? > > I think it could but we should have a use case for it. In other words, > something we can't reasonably express using existing means. In this case > it's a number of interfaces and device type specific behaviour (see the 3rd > patch). > >> >> Thirdly, only V4L2_CID_CONFIG_MODEL_COMMON_RAW is defined right now. What other >> models do you have in mind? What models can co-exist (since this is a bitmask)? > > We could have different raw camera models if needed. I don't have any > planned right now, though. > >> >> Finally, why choose a control for this? Should this perhaps be better done as >> a field in media_entity_desc/media_v2_entity? > > I don't think it's a great fit. This is largely about V4L2 (to some but > lesser extent about MC) and we traditionally have avoided MC -> V4L2 > dependencies. > It sounds a bit like you want to report what Mauro calls a 'Profile'. But I would expect the control to be an enum and not a bitmask, since I would expect a device to fit just a single configuration mode, and not multiple modes. Also, V4L2_CID_CONFIG_MODEL_COMMON_RAW applies only to sensors, right? So this should be V4L2_CID_CONFIG_MODEL_SENSOR_COMMON_RAW. But what is common about it and what is raw about it? Isn't it the case that pretty much all sensor drivers fall into this category? The only reason I see for this is if there are actually other configuration modes going to be added in the near future. What I am missing in this RFC is a high-level view of why it is needed and how it is going to be used. Perhaps I missed a discussion on linux-media? 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..928e8e3eed7f 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`. diff --git a/Documentation/userspace-api/media/v4l/subdev-config-model.rst b/Documentation/userspace-api/media/v4l/subdev-config-model.rst index 8ec801998f5f..d4ae921b69c8 100644 --- a/Documentation/userspace-api/media/v4l/subdev-config-model.rst +++ b/Documentation/userspace-api/media/v4l/subdev-config-model.rst @@ -1,5 +1,7 @@ .. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-no-invariants-or-later +.. _media_subdev_config_model: + Sub-device configuration models =============================== diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c index 6b9188a4a220..378657a52cd5 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c @@ -1167,6 +1167,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! */ @@ -1489,6 +1490,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) /* DV-class control IDs defined by V4L2 */ #define V4L2_CID_DV_CLASS_BASE (V4L2_CTRL_CLASS_DV | 0x900)
Add the V4L2_CID_CONFIG_MODEL control for the configuration model. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 4 ++++ .../userspace-api/media/v4l/subdev-config-model.rst | 2 ++ drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++ include/uapi/linux/v4l2-controls.h | 3 +++ 4 files changed, 14 insertions(+)