On Mon, Dec 11, 2023 at 04:27:27PM +0100, Hans Verkuil wrote: > On 11/12/2023 16:07, Laurent Pinchart wrote: > > 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). OK, I can handle that easily. I will add /* FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY. */ above all the checks. > >>> 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. Existing userspace isn't supposed to do anything, it will continue working as it does today. libcamera doesn't use those ioctls yet, so there will be no change in behaviour there. Once we start using them, we will likely require support for TRY, and consider the driver not supported if it doesn't implement that feature. I would encourage other future userspace users of the ioctls to do the same, but I don't think we need to enforce any specific behaviour there. > >>>> 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. Thank you :-) -- Regards, Laurent Pinchart
