| Message ID | 20231220103713.113386-25-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
On 20/12/2023 11:37, Sakari Ailus wrote: > Document that after unregistering, Media device memory resources are > released by the release() callback rather than by calling > media_device_cleanup(). > > Also add that driver memory resources should be bound to the Media device, > not V4L2 device. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > Documentation/driver-api/media/mc-core.rst | 18 ++++++++++++++++-- > include/media/media-device.h | 6 ++++-- > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 2456950ce8ff..346f67760671 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -46,13 +46,27 @@ Drivers initialise media device instances by calling > :c:func:`media_device_init()`. After initialising a media device instance, it is > registered by calling :c:func:`__media_device_register()` via the macro > ``media_device_register()`` and unregistered by calling > -:c:func:`media_device_unregister()`. An initialised media device must be > -eventually cleaned up by calling :c:func:`media_device_cleanup()`. > +:c:func:`media_device_unregister()`. The resources of an unregistered media > +device will be released by the ``release()`` callback of :c:type:`media_device` > +ops, which will be called when the last user of the media device has released it > +calling :c:func:`media_device_put()`. > + > +The ``release()`` callback is the way all the resources of the media device are > +released once :c:func:`media_device_init()` has been called. This is also > +relevant during device driver's probe function as the ``release()`` callback > +will also have to be able to safely release the resources related to a partially > +initialised media device. > > Note that it is not allowed to unregister a media device instance that was not > previously registered, or clean up a media device instance that was not > previously initialised. > > +Media device and driver's per-device context > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Drivers should use the struct media_device_ops ``release()`` callback to release > +their own resources and not e.g. that of the struct v4l2_device. > + > Entities > ^^^^^^^^ > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index c6816be0eee8..98e1892f1b51 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -250,8 +250,10 @@ void media_device_init(struct media_device *mdev); > * > * @mdev: pointer to struct &media_device > * > - * This function that will destroy the graph_mutex that is > - * initialized in media_device_init(). > + * This function that will destroy the graph_mutex that is initialized in > + * media_device_init(). Note that *only* drivers that do not manage releasing > + * the memory of th media device itself call this function. This function is > + * thus effectively DEPRECATED. > */ > void media_device_cleanup(struct media_device *mdev); >
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:37:08PM +0200, Sakari Ailus wrote: > Document that after unregistering, Media device memory resources are > released by the release() callback rather than by calling > media_device_cleanup(). > > Also add that driver memory resources should be bound to the Media device, > not V4L2 device. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Documentation/driver-api/media/mc-core.rst | 18 ++++++++++++++++-- > include/media/media-device.h | 6 ++++-- > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 2456950ce8ff..346f67760671 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -46,13 +46,27 @@ Drivers initialise media device instances by calling > :c:func:`media_device_init()`. After initialising a media device instance, it is > registered by calling :c:func:`__media_device_register()` via the macro > ``media_device_register()`` and unregistered by calling > -:c:func:`media_device_unregister()`. An initialised media device must be > -eventually cleaned up by calling :c:func:`media_device_cleanup()`. > +:c:func:`media_device_unregister()`. The resources of an unregistered media "of an unregistered media device" sounds weird here, I interpret it as applying only to media devices that have never been registered. > +device will be released by the ``release()`` callback of :c:type:`media_device` > +ops, which will be called when the last user of the media device has released it > +calling :c:func:`media_device_put()`. > + > +The ``release()`` callback is the way all the resources of the media device are > +released once :c:func:`media_device_init()` has been called. This is also > +relevant during device driver's probe function as the ``release()`` callback > +will also have to be able to safely release the resources related to a partially > +initialised media device. > > Note that it is not allowed to unregister a media device instance that was not > previously registered, or clean up a media device instance that was not > previously initialised. Does this need an update, as we don't cleanup explicitly instead ? > > +Media device and driver's per-device context > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Drivers should use the struct media_device_ops ``release()`` callback to release > +their own resources and not e.g. that of the struct v4l2_device. > + > Entities > ^^^^^^^^ > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index c6816be0eee8..98e1892f1b51 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -250,8 +250,10 @@ void media_device_init(struct media_device *mdev); > * > * @mdev: pointer to struct &media_device > * > - * This function that will destroy the graph_mutex that is > - * initialized in media_device_init(). > + * This function that will destroy the graph_mutex that is initialized in While at it, s/that will/will/ > + * media_device_init(). Note that *only* drivers that do not manage releasing > + * the memory of th media device itself call this function. This function is s/of th/of the/ "that do not manage releasing the memory of the media device itself" is hard to understand for someone who hasn't paid close attention to the development of this series. This text needs improvements. > + * thus effectively DEPRECATED. > */ > void media_device_cleanup(struct media_device *mdev); >
Hi Laurent, On Wed, Feb 07, 2024 at 04:43:04PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:37:08PM +0200, Sakari Ailus wrote: > > Document that after unregistering, Media device memory resources are > > released by the release() callback rather than by calling > > media_device_cleanup(). > > > > Also add that driver memory resources should be bound to the Media device, > > not V4L2 device. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Documentation/driver-api/media/mc-core.rst | 18 ++++++++++++++++-- > > include/media/media-device.h | 6 ++++-- > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > > index 2456950ce8ff..346f67760671 100644 > > --- a/Documentation/driver-api/media/mc-core.rst > > +++ b/Documentation/driver-api/media/mc-core.rst > > @@ -46,13 +46,27 @@ Drivers initialise media device instances by calling > > :c:func:`media_device_init()`. After initialising a media device instance, it is > > registered by calling :c:func:`__media_device_register()` via the macro > > ``media_device_register()`` and unregistered by calling > > -:c:func:`media_device_unregister()`. An initialised media device must be > > -eventually cleaned up by calling :c:func:`media_device_cleanup()`. > > +:c:func:`media_device_unregister()`. The resources of an unregistered media > > "of an unregistered media device" sounds weird here, I interpret it as > applying only to media devices that have never been registered. How about "newly unregistered"? > > > +device will be released by the ``release()`` callback of :c:type:`media_device` > > +ops, which will be called when the last user of the media device has released it > > +calling :c:func:`media_device_put()`. > > + > > +The ``release()`` callback is the way all the resources of the media device are > > +released once :c:func:`media_device_init()` has been called. This is also > > +relevant during device driver's probe function as the ``release()`` callback > > +will also have to be able to safely release the resources related to a partially > > +initialised media device. > > > > Note that it is not allowed to unregister a media device instance that was not > > previously registered, or clean up a media device instance that was not > > previously initialised. > > Does this need an update, as we don't cleanup explicitly instead ? I think this should remain unchanged, at least as long as there are drivers using the old API. > > > > > +Media device and driver's per-device context > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Drivers should use the struct media_device_ops ``release()`` callback to release > > +their own resources and not e.g. that of the struct v4l2_device. > > + > > Entities > > ^^^^^^^^ > > > > diff --git a/include/media/media-device.h b/include/media/media-device.h > > index c6816be0eee8..98e1892f1b51 100644 > > --- a/include/media/media-device.h > > +++ b/include/media/media-device.h > > @@ -250,8 +250,10 @@ void media_device_init(struct media_device *mdev); > > * > > * @mdev: pointer to struct &media_device > > * > > - * This function that will destroy the graph_mutex that is > > - * initialized in media_device_init(). > > + * This function that will destroy the graph_mutex that is initialized in > > While at it, s/that will/will/ Yes. > > > + * media_device_init(). Note that *only* drivers that do not manage releasing > > + * the memory of th media device itself call this function. This function is > > s/of th/of the/ > > "that do not manage releasing the memory of the media device itself" is > hard to understand for someone who hasn't paid close attention to the > development of this series. This text needs improvements. Hmm. How about: Note that only drivers that do not have a proper release callback of the struct media_device call this function. > > > + * thus effectively DEPRECATED. > > */ > > void media_device_cleanup(struct media_device *mdev); > > >
diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst index 2456950ce8ff..346f67760671 100644 --- a/Documentation/driver-api/media/mc-core.rst +++ b/Documentation/driver-api/media/mc-core.rst @@ -46,13 +46,27 @@ Drivers initialise media device instances by calling :c:func:`media_device_init()`. After initialising a media device instance, it is registered by calling :c:func:`__media_device_register()` via the macro ``media_device_register()`` and unregistered by calling -:c:func:`media_device_unregister()`. An initialised media device must be -eventually cleaned up by calling :c:func:`media_device_cleanup()`. +:c:func:`media_device_unregister()`. The resources of an unregistered media +device will be released by the ``release()`` callback of :c:type:`media_device` +ops, which will be called when the last user of the media device has released it +calling :c:func:`media_device_put()`. + +The ``release()`` callback is the way all the resources of the media device are +released once :c:func:`media_device_init()` has been called. This is also +relevant during device driver's probe function as the ``release()`` callback +will also have to be able to safely release the resources related to a partially +initialised media device. Note that it is not allowed to unregister a media device instance that was not previously registered, or clean up a media device instance that was not previously initialised. +Media device and driver's per-device context +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Drivers should use the struct media_device_ops ``release()`` callback to release +their own resources and not e.g. that of the struct v4l2_device. + Entities ^^^^^^^^ diff --git a/include/media/media-device.h b/include/media/media-device.h index c6816be0eee8..98e1892f1b51 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -250,8 +250,10 @@ void media_device_init(struct media_device *mdev); * * @mdev: pointer to struct &media_device * - * This function that will destroy the graph_mutex that is - * initialized in media_device_init(). + * This function that will destroy the graph_mutex that is initialized in + * media_device_init(). Note that *only* drivers that do not manage releasing + * the memory of th media device itself call this function. This function is + * thus effectively DEPRECATED. */ void media_device_cleanup(struct media_device *mdev);
Document that after unregistering, Media device memory resources are released by the release() callback rather than by calling media_device_cleanup(). Also add that driver memory resources should be bound to the Media device, not V4L2 device. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Documentation/driver-api/media/mc-core.rst | 18 ++++++++++++++++-- include/media/media-device.h | 6 ++++-- 2 files changed, 20 insertions(+), 4 deletions(-)