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@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 :-) > >> >> 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
