Re: [PATCH v2 6/6] media: Documentation: add documentation about subdev state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux