On 22/12/2021 02:00, Laurent Pinchart wrote:
+: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.
Whenever you're dealing with both controls and something in the state. Also
you've got a problem if the sensor driver does I²C writes to more than
8-bit registers in 8-bit chunks and relies on hardware caching some values
before the entire register is updated.
For instance, in the CCS driver, computing the PLL tree configuration
requires state (subdev format and selection rectangles) as well as control
values as input from multiple sub-devices. I suppose this is the case with
many sensor drivers --- I just know CCS best.
The current implementation uses a single mutex for all controls and
subdevs.
For a single subdev, that could be done by setting
v4l2_ctrl_handler.lock to &v4l2_subdev.active_state->lock.
That would only cover the active state, not the try state.
I made a test change by changing the subdev state to have 'struct mutex
_lock' and 'struct mutex *lock', similar to the struct
v4l2_ctrl_handler. The driver could then, in its init_cfg(), set the
state->lock to either v4l2_ctrl_handler.lock or a lock in the driver's
private data.
This kind of lock sharing makes me a bit uncomfortable (although the
controls are already allowing this, and a driver private mutex can be
set as the ctrl lock): if I call v4l2_subdev_lock_active_state(), I
don't expect the controls to be locked too.
Then again, if I think about this as the subdev state really containing
three parts: 1) the subdev active & try states, 2) controls, 3) driver
private data, then it kind of makes sense. In the long run we could move
towards combining these pieces together, and thus cleaning up the state
management and locking.
Tomi
