Re: [PATCH v4 3/8] media: v4l2-subdev: Add which field to struct v4l2_subdev_frame_interval

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Dec 11, 2023 at 05:42:37PM +0200, Laurent Pinchart wrote:
> 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@ideasonboard.com> 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. */

Actually I'll expand that to

 	/*
	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
	 * subdev active state API.
	 */

> 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@kernel.org>
> > >>>>   
> > >>>>>> 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