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

Add documentation about centrally managed subdev state.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

On 17/12/2021 14:50, Tomi Valkeinen wrote:
> Add documentation about centrally managed subdev state.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../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

e.g. as -> as e.g.

> +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> +and composition rectangles.
> +
> +In addition to the active configuration, each subdev file-handle has an array of
> +struct v4l2_subdev_pad_config, managed by V4L2 core, which contains the try

by V4L2 -> by the V4L2

> +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
> +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.
> +
> +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.
> +The core automatically initializes a state for each open file-handle where to
> +store the try configurations and releases them at file-handle closing time.
> +
> +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
> +'state' parameter. The sub-device driver can access and modify the
> +configuration stored in the provided state after having locked it by calling
> +: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.
> +
> +While the V4L2 core will pass the correct try- or active-state to the
> +subdevice operations, device drivers might call operations on other
> +subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
> +state. This is only a problem for subdev drivers which use the
> +centrally managed active-state and are used in media pipelines with older
> +subdev drivers. In these cases the called subdev ops must also handle the NULL
> +case. This can be easily managed by the use of
> +:c:func:`v4l2_subdev_lock_and_return_state()` helper.
> +
> +:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
> +an existing driver to the new state management when it cannot be guaranteed
> +that the current callers will pass the state properly. The function prints a
> +notice when the passed state is NULL to encourage the porting of the callers
> +to the new state management.
> +
>  V4L2 sub-device functions and data structures
>  ---------------------------------------------
>  
> 

With those tiny changes:

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

Moi,

Thanks for the update.

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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../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.
> +
> +In addition to the active configuration, each subdev file-handle has an array of
> +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
> +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.
> +
> +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.
> +The core automatically initializes a state for each open file-handle where to
> +store the try configurations and releases them at file-handle closing time.
> +
> +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
> +'state' parameter. The sub-device driver can access and modify the
> +configuration stored in the provided state after having locked it by calling
> +: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.

> +
> +While the V4L2 core will pass the correct try- or active-state to the
> +subdevice operations, device drivers might call operations on other
> +subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
> +state. This is only a problem for subdev drivers which use the
> +centrally managed active-state and are used in media pipelines with older
> +subdev drivers. In these cases the called subdev ops must also handle the NULL
> +case. This can be easily managed by the use of
> +:c:func:`v4l2_subdev_lock_and_return_state()` helper.
> +
> +:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
> +an existing driver to the new state management when it cannot be guaranteed
> +that the current callers will pass the state properly. The function prints a
> +notice when the passed state is NULL to encourage the porting of the callers
> +to the new state management.
> +
>  V4L2 sub-device functions and data structures
>  ---------------------------------------------
>

Moi Sakari,

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@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../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.

> > +
> > +While the V4L2 core will pass the correct try- or active-state to the

s/try-/try/
s/active-/active /

> > +subdevice operations, device drivers might call operations on other
> > +subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
> > +state. This is only a problem for subdev drivers which use the
> > +centrally managed active-state and are used in media pipelines with older
> > +subdev drivers. In these cases the called subdev ops must also handle the NULL
> > +case. This can be easily managed by the use of
> > +:c:func:`v4l2_subdev_lock_and_return_state()` helper.

This doesn't emphasize strongly enough that this pattern should be
phased out. You could write it as follows.

While the V4L2 core passes the correct try or active state to the
subdevice operations, some existing device drivers pass a NULL state
when calling operations with :c:func:`v4l2_subdev_call()`. This legacy
construct causes issues with subdevice drivers that let the V4L2 core
manage the active state, as they expect to receive the appropriate state
as a parameter. To help the conversion of subdevice drivers to a managed
active state without having to convert all callers at the same time, the
:c:func:`v4l2_subdev_lock_and_return_state()` helper function can be
used by subdevice drivers to retrieve the active state if a NULL state
is passed to the subdevice operation.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> > +:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
> > +an existing driver to the new state management when it cannot be guaranteed
> > +that the current callers will pass the state properly. The function prints a
> > +notice when the passed state is NULL to encourage the porting of the callers
> > +to the new state management.
> > +
> >  V4L2 sub-device functions and data structures
> >  ---------------------------------------------
> >

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@ideasonboard.com>
> > > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  .../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.

Hi Sakari,

On Tue, Dec 21, 2021 at 01:10:08PM +0200, Sakari Ailus wrote:
> On Tue, Dec 21, 2021 at 12:33:46PM +0200, Laurent Pinchart wrote:
> > 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@ideasonboard.com>
> > > > > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > ---
> > > > > >  .../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.
> 
> 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.

If we need to access the state of multiple subdevs I think we'll need to
switch to ww_mutex. That will require moving lock handling in the V4L2
core, I don't trust drivers to handle the ww_mutex lock failures and the
necessary rewind and retry correctly.

This will require quite a bit of work, which I think will need to be
done step by step.

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

Hi Tomi,

On Mon, Feb 07, 2022 at 11:19:23AM +0200, Tomi Valkeinen wrote:
> 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.

That's right, but semantics of the try operation for formats and
controls are inherently different, so it's impossible to get it
completely right in any case. To really solve this, we need to move
control storage to the v4l2_subdev_state structure, and extend the
subdev control ioctls with a which field. That's out of scope for this
series.

> 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.

I'm fine with this approach as a temporary hack.