Hi Laurent, On Mon, Nov 18, 2024 at 04:40:02AM +0200, Laurent Pinchart wrote: > 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. I'll use this text, with some modifications, in the documentation of sub-device configuration models. > > > >> 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. Currently the only way to determine CCS driver is being used is that some CCS specific controls are supported by the device. I'm beginning to lean towards having a CCS model, too. I'll add that in the next version. > > > >> 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. I think it was only about the IOCTLs supported, that's it. > > > 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. I'm open to making this an enum if you prefer that. My concern, and the reason why I used a bitmask, is that a sub-device could implement several models at a time. They could also be used to declare semantics of a specific part of the device interface, not the entire interface. For instance, analogue gain model could be an example of that albeit this likely could be derived from controls present. > > > 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. That name is very long. :-( Anyway, I don't see this being an issue in practice so I'll use that in the next version. > > > 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? > -- Kind regards, Sakari Ailus
