Re: [PATCH v2 24/29] media: Documentation: Document how Media device resources are released

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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);
> >  
> 

-- 
Regards,

Sakari Ailus