On 11/12/2023 16:07, Laurent Pinchart wrote: > Hello, > > On Mon, Dec 11, 2023 at 02:21:19PM +0100, Mauro Carvalho Chehab wrote: >> Em Mon, 11 Dec 2023 13:25:23 +0100 Hans Verkuil escreveu: >>> On 11/12/2023 12:53, Mauro Carvalho Chehab wrote: >>>> Em Mon, 11 Dec 2023 09:59:25 +0100 Hans Verkuil escreveu: >>>>> On 09/12/2023 12:11, Laurent Pinchart wrote: >>>>>> On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote: >>>>>>> Em Fri, 8 Dec 2023 20:16:43 +0200 >>>>>>> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> escreveu: >>>>>>> >>>>>>>> Due to a historical mishap, the v4l2_subdev_frame_interval structure >>>>>>>> is the only part of the V4L2 subdev userspace API that doesn't contain a >>>>>>>> 'which' field. This prevents trying frame intervals using the subdev >>>>>>>> 'TRY' state mechanism. >>>>>>>> >>>>>>>> Adding a 'which' field is simple as the structure has 8 reserved fields. >>>>>>>> This would however break userspace as the field is currently set to 0, >>>>>>>> corresponding to V4L2_SUBDEV_FORMAT_TRY, while the corresponding ioctls >>>>>>>> currently operate on the 'ACTIVE' state. We thus need to add a new >>>>>>>> subdev client cap, V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL, to indicate >>>>>>>> that userspace is aware of this new field. >>>>>>>> >>>>>>>> All drivers that implement the subdev .get_frame_interval() and >>>>>>>> .set_frame_interval() operations are updated to return -EINVAL when >>>>>>>> operating on the TRY state, preserving the current behaviour. >>>>>>>> >>>>>>>> While at it, fix a bad copy&paste in the documentation of the struct >>>>>>>> v4l2_subdev_frame_interval_enum 'which' field. >>>>>>> >>>>>>> The uAPI change looks ok to me. However, having to add an special >>>>>>> logic on every client using (get/set)_time_interval seems risky, >>>>>>> as it will require an extra check before accepting new subdev >>>>>>> drivers. >>>>>>> >>>>>>> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c: >>>>>>> >>>>>>> if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE) >>>>>>> return -EINVAL; >>>>>> >>>>>> But then no new driver will be able to use this API. Look at patch 8/8 >>>>>> in this series, the whole point of this exercise is to support >>>>>> V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers >>>>>> is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward, >>>>>> the drivers that are still maintained should be converted eventually. >>>> >>>> Then please add a FIXME note there warning that the logic there >>>> exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY. >>> >>> I think there are two types of cases here: the first case is when the subdev >>> does not support TRY at all for any of the pad ops, i.e. it can't be used by >>> MC-centric drivers. >>> >>> In that case a top-level comment to that effect might be useful (that can be >>> done in a separate patch). >> >> Agreed. > > Recording the need to convert those drivers in a TODO comment seems fine > to me. Whether or not someone will address those comments is another > question... I'd like that to be the case, but most (if not all) of those > drivers are for old devices, so the incentive will be very low, not even > mentioning the lack of hardware availability for testing. > > We can add TODO comments in a separate patch on top. Or would it be > better to add a single TODO file in drivers/media/i2c/, like done in > staging ? Or a todo.rst in Documentation, like done in > Documentation/gpu/todo.rst ? Definitely add it to the source code itself since nobody reads those TODO or todo.rst files :-) > > Update: I checked the drivers touched by this patch, and they all > implement TRY support for the pad ops, so there's no TODO to record :-) OK, good. I think it would be useful to record in a comment which drivers need work in order to be MC-centric capable. But it is a nice-to-have patch and can be done separately from this series. For the g/s_frame_interval drivers a FIXME is needed for those that do not support TRY (all except thp7312). > >>> The second case is where it does support TRY for the other pad ops, just not >>> for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but >>> it is not clear whether that would be considered a bug or not. That is something >>> that should be clarified. >>> >>> Frankly, it is not clear to me if, for an MC-centric capable subdev, support >>> for FORMAT_TRY is required to be present for all pad ops, or not. And if not, >>> for which pad ops is it optional and what does that mean for applications. >> >> Agreed. >> >> Ideally, the best would be to add support for this feature for all subdevs >> at the same patch series, but I understand that this will require tests >> from the developers proposing the uAPI change and/or tested-by. >> >> So, I can accept merging this series and enforcing for new drivers without >> requiring a conversion of all existing ones at the same patch series, but >> someone (the ones behind this change) should work to have existing drivers >> with g/s_frame_interval subdev uAPI also supporting TRY_. > > Many of the drivers are unmaintained :-( Nobody has the hardware > anymore, or the time to test it. Some drivers can be converted, but not > all of them unfortunately. > > This series addresses the thp7312 already. I can address the mt9m114 > too. I think we can try to also address the other three drivers for > sensors supported by libcamera, namely the ov5640, ov5693 and ov8865. > The rest will need other volunteers. So the main question is: what is userspace (esp. libcamera) supposed to do if a subdev supports TRY for all ops except g/s_frame_interval? Does that mean it is not suitable for MC-centric use anymore? Or is userspace required to work around that? At the very least that should be documented. > >>>> With that, feel free to add: >>>> >>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> >>>> >>>>>> All new drivers should use the V4L2 subdev active state API and support >>>>>> V4L2_SUBDEV_FORMAT_TRY right away. >>>> >>>> OK. >>>> >>>>> I agree with Laurent here. Note that the logic isn't 'special', it is standard >>>>> handling of the 'which' field. It was never there for g/s_frame_interval >>>>> because that was the only one that didn't have a 'which' field, but now that >>>>> it does, the drivers need to be adapted to handle this new field. >>>>> >>>>> It shouldn't be hidden in some core code, that would only confuse matters. >>>> >>>> If the idea is to move forward without implementing support for such >>>> features at the existing drivers, we should at least have something >>>> annotated at the drivers that this is something that will require >>>> further changes in the future to support the current behavior. >>>> >>>> I also expect that the API compliance tool to produce warnings >>>> on drivers using the old behavior. >>> >>> Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means >>> it is very likely created through an MC-centric driver and TRY support should >>> be there. >> >> Yes. > > Hans, would you be able to handle v4l2-compliance ? > Sure. Although that likely will be in January. Regards, Hans
