Hi Sakari, On Tue, Dec 21, 2021 at 12:27:26PM +0200, Sakari Ailus wrote: > On Tue, Dec 21, 2021 at 10:36:43AM +0200, Laurent Pinchart wrote: > > On Mon, Dec 20, 2021 at 07:56:31PM +0200, Sakari Ailus wrote: > > > Moi, > > > > > > Thanks for the update. > > > > Kiitos arvostelusta. > > > > > On Fri, Dec 17, 2021 at 03:50:22PM +0200, Tomi Valkeinen wrote: > > > > Add documentation about centrally managed subdev state. > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> > > > > Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> > > > > --- > > > > .../driver-api/media/v4l2-subdev.rst | 57 +++++++++++++++++++ > > > > 1 file changed, 57 insertions(+) > > > > > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > > > index 08ea2673b19e..18b00bd3d6d4 100644 > > > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > > > @@ -518,6 +518,63 @@ The :c:func:`v4l2_i2c_new_subdev` function will call > > > > :c:type:`i2c_board_info` structure using the ``client_type`` and the > > > > ``addr`` to fill it. > > > > > > > > +Centrally managed subdev active state > > > > +------------------------------------- > > > > + > > > > +Traditionally V4L2 subdev drivers maintained internal state for the active > > > > +device configuration. This is often implemented e.g. as an array of > > > > +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping > > > > +and composition rectangles. > > > > I think we usually write > > > > s/cropping/crop/ > > s/composition/compose/ > > > > > > + > > > > +In addition to the active configuration, each subdev file-handle has an array of > > > > s/file-handle/file handle/ (through the whole patch) > > > > > > +struct v4l2_subdev_pad_config, managed by V4L2 core, which contains the try > > > > +configuration. > > > > + > > > > +To simplify the subdev drivers the V4L2 subdev API now optionally supports a > > > > +centrally managed active configuration represented by > > > > +:c:type:`v4l2_subdev_state`. One instance of state, which contains the active > > > > +device configuration, is associated with the sub-device itself as part of > > > > s/associated with/stored in/ > > > > > > +the :c:type:`v4l2_subdev` structure, while the core associates to each open > > > > +file-handle a try state, which contains the configuration valid in the > > > > +file-handle context only. > > > > I'd write > > > > [...] while the core associates a try state to each open file-handle, to > > store the try configuration related to that file-handle. > > > > > > + > > > > +Sub-device drivers can opt-in and use state to manage their active configuration > > > > +by initializing the subdevice state with a call to v4l2_subdev_init_finalize() > > > > +before registering the sub-device. They must also call v4l2_subdev_cleanup() > > > > +to release all the acquired resources before unregistering the sub-device. > > > > s/acquired/allocated/ > > > > > > +The core automatically initializes a state for each open file-handle where to > > > > s/initializes/allocates and initializes/ > > s/where to/to/ > > > > > > +store the try configurations and releases them at file-handle closing time. > > > > s/releases it at file-handle closing time/frees it when closing the file handle/ > > > > > > + > > > > +V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats > > > > +<v4l2-subdev-format-whence>` receive the correct state to operate on as a > > > > s/as a/through the/ > > > > > > +'state' parameter. The sub-device driver can access and modify the > > > > +configuration stored in the provided state after having locked it by calling > > > > s/locked it/locked the state/ > > > > > > +:c:func:`v4l2_subdev_lock_state()`. The driver must then call > > > > +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done. > > > > + > > > > +Operations that do not receive a state parameter implicitly operate on the > > > > +subdevice active state, which drivers can exclusively access by > > > > +calling :c:func:`v4l2_subdev_lock_active_state()`. The sub-device active state > > > > +must equally be released by calling :c:func:`v4l2_subdev_unlock_state()`. > > > > + > > > > +Drivers must never manually access the state stored in the :c:type:`v4l2_subdev` > > > > +or in the file-handle without going through the designated helpers. > > > > > > Have you thought how this will interact with controls? > > > > > > Generally active state information exists for a device in struct > > > v4l2_subdev_state as well as the device's control handler as control > > > values. Controls have dependencies to other state information (active and > > > try). > > > > > > Until now, drivers have been responsible for serialising access to this > > > state on their own, mostly using a single mutex. Controls require a mutex > > > as well, but it's the same mutex independently of whether a driver is > > > dealing with active or try subdev state. > > > > > > In other words, if the above is assumed, when you're dealing with try state > > > that has dependencies to controls, you have to hold both that try state's > > > mutex as well as the control handler's mutex. > > > > Going forward, I think we should store the controls in the subdev state. > > That will require a uAPI extension to pass a `which` parameter to the > > control ioctls, and deprecated the control TRY ioctl on subdevs. > > Interactions between controls and pad formats will be easier to test, as > > applications will be able to set controls in the TRY state, interacting > > with the TRY formats. We will also need to rework the control handler > > operations to split .s_ctrl() in two, with one function to adjust a > > control value and one function to apply it. > > > > In the meantime, I think we'll need to acquire both locks, or possibly > > use the active state lock as the control handler lock. > > Note that also trying controls requires locking the control handler, > meaning that the control handler's mutex may not be the same as the active > state mutex (unless access also to try state is serialised using the same > mutex). > > What I'm saying is that to make this better usable with controls, changes > will be needed somewhere as the locking scheme is a poor match with that of > controls currently. Just saying the mutexes are acquired in a certain > order and pushing the problem to drivers is not a great solution. Could you maybe provide an example of existing subdev driver code that showcases this issue ? I'm not sure we really understand each other here. -- Regards, Laurent Pinchart
