| Message ID | 20241011075535.588140-5-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Sub-device configuration models | expand |
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
Hello, On Wed, Nov 13, 2024 at 01:26:26PM +0100, Hans Verkuil wrote: > On 11/13/24 09:35, Sakari Ailus wrote: > > 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 :-) I'll try to rephrase what Sakari wrote in the patches. The V4L2 specification defines a subdev API that exposes three type of configuration elements: formats, selection rectangles and controls. The specification contains generic information about how those configuration elements behave, but not precisly how they apply to particular hardware features. We leave some leeway to drivers to decide how to map selection rectangles to device features, as long as they comply with the V4L2 specification. This is needed, as hardware features differ between devices, so it's the driver's responsibility to handle this mapping. Unfortunately, this lack of clearly defined mapping in the specification has led to different drivers mapping the same hardware features to different API elements, or implementing the API elements with slightly different behaviours. Furthermore, many drivers have implemented selection rectangles in ways that do not comply with the V4L2 specification. All of this makes userspace development difficult. We can't define precisely how all configuration elements apply to hardware features in a way that applies to all devices, as devices differ widely. We can however develop such precise definitions for classes of similar devices. In order to develop generic userspace code, we then need a way for subdevs to indicate which class they belong to. This is what the configuration model control does. The configuration model tells userspace which section of the V4L2 specification defines the precise behaviour of the device. One example of how drivers implement features in different ways is skipping and binning. Some sensor drivers use selection rectangles, other just formats. > >> 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). The control can be used by any type of device, as long as someone documents a corresponding configuration model. > >> 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. CCS would be another model, although I'm not sure if any other driver would implement that model. Still, even if used by the CCS driver only, I think it would make sense to define a CCS model. > >> 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'. There are similarities but it's not the same concept. What Mauro named "profile" was more about which ioctls were implemented by the device, and less about their detailed behaviour. > 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. I would have used an enum as well. In theory we could define models that cover non-overlaping parts of the device features, and a device could then implement multiple models, but I'm not sure that would happen. > 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? Yes, mentioning "SENSOR" in the name makes sense. > Isn't it the case that pretty much all sensor drivers fall into this > category? "raw" is by opposition to YUV sensors. YUV sensors (a.k.a. "smart sensors") require very different configuration parameters compared to raw sensors, so the model we're standardizing for raw sensors isn't applicable. > The only reason I see for this is if there are actually other configuration > modes going to be added in the near future. Even before we add a second model, this is useful for userspace. We have many camera sensor drivers that implement the V4L2 API in different (and sometimes non-compliant) ways. Knowing that a sensor is compatible with the new model we're defining will be useful for libcamera. > 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?
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(+)