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@xxxxxxxxxxxxxxx> > >>> --- > >>> .../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? -- Regards, Laurent Pinchart
