Re: [PATCH 03/59] drm/prime: Update docs

From: Daniel Vetter <daniel@ffwll.ch>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 03/59] drm/prime: Update docs
Date: Mon, 17 Jun 2019 18:38:11 +0200	[thread overview]
Message-ID: <20190617163811.GK12905@phenom.ffwll.local> (raw)
In-Reply-To: <20190615114153.GA20240@ravnborg.org>

On Sat, Jun 15, 2019 at 01:41:53PM +0200, Sam Ravnborg wrote:
> Hi Daniel
> 
> Better and more consistent docs - good!
> With relevant comments addressed:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> > +/**
> > + * DOC: overview and lifetime rules
> > + *
> > + * Similar to GEM global names, PRIME file descriptors are also used to share
> > + * buffer objects across processes. They offer additional security: as file
> > + * descriptors must be explicitly sent over UNIX domain sockets to be shared
> > + * between applications, they can't be guessed like the globally unique GEM
> > + * names.
> For a newbie like me the above does not really help to understand what
> PRIME is.
> Yes, it is file descriptors used to share buffer objects across
> processes.
> But the text say "also used ..", so the main usage of PRIME is something
> else.

No, what's mean here is that prime file descriptors are used like gem
global names, for buffer sharing. That's what the "also" means here.
There's no other use for prime fd than sharing buffers.

Not really clear why this is unclear ...

> But as said, newbie so it may be fine for most readers.
> 
> > + *
> > + * Drivers that support the PRIME API must set the DRIVER_PRIME bit in the
> > + * &drm_driver.driver_features field, and implement the
> > + * &drm_driver.prime_handle_to_fd and &drm_driver.prime_fd_to_handle operations.
> 
> > + * GEM based drivers must use drm_gem_prime_handle_to_fd() an
> > + * drm_gem_prime_fd_to_handle() to implement these.
> an => and in the above.
> 
> > + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> > + * @dev: dev to export the buffer from
> > + * @file_priv: drm file-private structure
> > + * @prime_fd: fd id of the dma-buf which should be imported
> > + * @handle: pointer to storage for the handle of the imported buffer object
> >   *
> > + * This is the PRIME import function which must be used mandatorily by GEM
> > + * drivers to ensure correct lifetime management of the underlying GEM object.
> > + * The actual importing of GEM object from the dma-buf is done through the
> > + * &drm_driver.gem_import_export driver callback.
> Here maybe add a description of the return result.
> If I read the code correct:
> Returns 0 on success or negative error code on failure.
> 
> >   */
> > -void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > +int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> > +			       struct drm_file *file_priv, int prime_fd,
> > +			       uint32_t *handle)
> >  {
> > -	struct drm_gem_object *obj = dma_buf->priv;
> > -	void *vaddr;
> > +	struct dma_buf *dma_buf;
> > +	struct drm_gem_object *obj;
> > +	int ret;
> >  
> > -	vaddr = drm_gem_vmap(obj);
> > -	if (IS_ERR(vaddr))
> > -		vaddr = NULL;
> > +	dma_buf = dma_buf_get(prime_fd);
> > +	if (IS_ERR(dma_buf))
> > +		return PTR_ERR(dma_buf);
> >  
> > +	mutex_lock(&file_priv->prime.lock);
> >  
> > +	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> > +			dma_buf, handle);
> > +	if (ret == 0)
> > +		goto out_put;
> >  
> > +	/* never seen this one, need to import */
> > +	mutex_lock(&dev->object_name_lock);
> > +	if (dev->driver->gem_prime_import)
> > +		obj = dev->driver->gem_prime_import(dev, dma_buf);
> > +	else
> > +		obj = drm_gem_prime_import(dev, dma_buf);
> > +	if (IS_ERR(obj)) {
> > +		ret = PTR_ERR(obj);
> > +		goto out_unlock;
> > +	}
> > +	if (obj->dma_buf) {
> > +		WARN_ON(obj->dma_buf != dma_buf);
> > +	} else {
> > +		obj->dma_buf = dma_buf;
> > +		get_dma_buf(dma_buf);
> > +	}
> >  
> > +	/* _handle_create_tail unconditionally unlocks dev->object_name_lock. */
> > +	ret = drm_gem_handle_create_tail(file_priv, obj, handle);
> > +	drm_gem_object_put_unlocked(obj);
> > +	if (ret)
> > +		goto out_put;
> >  
> > +	ret = drm_prime_add_buf_handle(&file_priv->prime,
> > +			dma_buf, *handle);
> > +	mutex_unlock(&file_priv->prime.lock);
> > +	if (ret)
> > +		goto fail;
> > +	dma_buf_put(dma_buf);
> >  
> > +	return 0;
> >  
> > +fail:
> > +	/* hmm, if driver attached, we are relying on the free-object path
> > +	 * to detach.. which seems ok..
> > +	 */
> > +	drm_gem_handle_delete(file_priv, *handle);
> > +	dma_buf_put(dma_buf);
> > +	return ret;
> > +
> > +out_unlock:
> > +	mutex_unlock(&dev->object_name_lock);
> > +out_put:
> > +	mutex_unlock(&file_priv->prime.lock);
> > +	dma_buf_put(dma_buf);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
> > +
> > +
> > +/**
> > + * DOC: PRIME Helpers
> > + *
> > + * Drivers can implement &drm_gem_object_funcs.export and
> > + * &drm_driver.gem_prime_import in terms of simpler APIs by using the helper
> > + * functions drm_gem_prime_export() and drm_gem_prime_import().  These functions
> > + * implement dma-buf support in terms of some lower-level helpers, which are
> > + * again exported for drivers to use individually:
> > + *
> > + * Exporting buffers
> > + * ~~~~~~~~~~~~~~~~~
> > + *
> > + * Optional pinning of buffers is handled at dma-buf attach and detach time in
> > + * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
> > + * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> > + * &drm_gem_object_funcs.get_sg_table.
> > + *
> > + * For kernel-internal access there's drm_gem_dmabuf_vmap() and
> > + * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> > + * drm_gem_dmabuf_mmap().
> > + *
> > + * Note that these export helpers can only be used if the underlying backing
> > + * storage is fully coherent and either permanently pinned, or it is safe to pin
> > + * it indefinitely.
> > + *
> > + * FIXME: The underlying helper functions are named rather inconsistently.
> > + *
> > + * Exporting buffers
> > + * ~~~~~~~~~~~~~~~~~
> > + *
> > + * Importing dma-bufs using drm_gem_prime_import() relies on
> > + * &drm_driver.gem_prime_import_sg_table.
> > + *
> > + * Note that similarly to the export helpers this permanently pins the
> > + * underlying backing storage. Which is ok for scanout, but is not the best
> > + * option for sharing lots of buffers for rendering.
> > + */
> > +
> > +/**
> > + * drm_gem_map_attach - dma_buf attach implementation for GEM
> > + * @dma_buf: buffer to attach device to
> > + * @attach: buffer attachment data
> > + *
> > + * Calls &drm_gem_object_funcs.pin for device specific handling. This can be
> > + * used as the &dma_buf_ops.attach callback. Must be used together with
> > + * drm_gem_map_detach().
> > + *
> > + * Returns 0 on success, negative error code on failure.
> > + */
> > +int drm_gem_map_attach(struct dma_buf *dma_buf,
> > +		       struct dma_buf_attachment *attach)
> > +{
> > +	struct drm_gem_object *obj = dma_buf->priv;
> > +
> > +	return drm_gem_pin(obj);
> > +}
> > +EXPORT_SYMBOL(drm_gem_map_attach);
> > +
> > +/**
> > + * drm_gem_map_detach - dma_buf detach implementation for GEM
> > + * @dma_buf: buffer to detach from
> > + * @attach: attachment to be detached
> > + *
> > + * Calls &drm_gem_object_funcs.pin for device specific handling.  Cleans up
> > + * &dma_buf_attachment from drm_gem_map_attach(). This can be used as the
> > + * &dma_buf_ops.detach callback.
> > + */
> > +void drm_gem_map_detach(struct dma_buf *dma_buf,
> > +			struct dma_buf_attachment *attach)
> > +{
> > +	struct drm_gem_object *obj = dma_buf->priv;
> > +
> > +	drm_gem_unpin(obj);
> > +}
> > +EXPORT_SYMBOL(drm_gem_map_detach);
> > +
> > +/**
> > + * drm_gem_map_dma_buf - map_dma_buf implementation for GEM
> > + * @attach: attachment whose scatterlist is to be returned
> > + * @dir: direction of DMA transfer
> > + *
> > + * Calls &drm_gem_object_funcs.get_sg_table and then maps the scatterlist. This
> > + * can be used as the &dma_buf_ops.map_dma_buf callback. Should be used together
> > + * with drm_gem_unmap_dma_buf().
> > + *
> > + * Returns:sg_table containing the scatterlist to be returned; returns ERR_PTR
> > + * on error. May return -EINTR if it is interrupted by a signal.
> > + */
> Nit - add space after ':'
> 
> > +
> > +/**
> > + * drm_gem_dmabuf_vmap - dma_buf vmap implementation for GEM
> > + * @dma_buf: buffer to be mapped
> > + *
> > + * Sets up a kernel virtual mapping. This can be used as the &dma_buf_ops.vmap
> > + * callback. Calls into &drm_gem_object_funcs.vmap for device specific handling.
> > + *
> > + * Returns the kernel virtual address.
> or NULL if failed to setup virtual mapping.
> 
> > + */
> > +void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > +{
> > +	struct drm_gem_object *obj = dma_buf->priv;
> > +	void *vaddr;
> > +
> > +	vaddr = drm_gem_vmap(obj);
> > +	if (IS_ERR(vaddr))
> > +		vaddr = NULL;
> > +
> > +	return vaddr;
> > +}
> > +EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
> 
> 
> >  /**
> >   * drm_gem_prime_import_dev - core implementation of the import callback
> >   * @dev: drm_device to import into
> >   * @dma_buf: dma-buf object to import
> >   * @attach_dev: struct device to dma_buf attach
> >   *
> > - * This is the core of drm_gem_prime_import. It's designed to be called by
> > - * drivers who want to use a different device structure than dev->dev for
> > - * attaching via dma_buf.
> > + * This is the core of drm_gem_prime_import(). It's designed to be called by
> > + * drivers who want to use a different device structure than &drm_device.dev for
> > + * attaching via dma_buf. This function calls
> > + * &drm_driver.gem_prime_import_sg_table internally.
> > + *
> > + * Drivers must arrange to call drm_prime_gem_destroy() from their
> > + * &drm_gem_object_funcs.free hook when using this function.
> >   */
> >  struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
> >  					    struct dma_buf *dma_buf,
> > @@ -728,7 +923,11 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev);
> >   * @dma_buf: dma-buf object to import
> >   *
> >   * This is the implementation of the gem_prime_import functions for GEM drivers
> > - * using the PRIME helpers.
> > + * using the PRIME helpers. Drivers can use this as their
> > + * &drm_driver.gem_prime_import implementation.
> > + *
> > + * Drivers must arrange to call drm_prime_gem_destroy() from their
> > + * &drm_gem_object_funcs.free hook when using this function.
> 
> Could we here document what function we provide as the default
> &drm_gem_object_funcs.free hook, which do the drm_prime_gem_destroy()
> call.
> I read the above that drivers have to provide their own implementation
> of a .free hook, and I do not think this is right.

There's no default implementation of the free hook at all, so not sure
what you mean. You do kinda have to provide a free hook, otherwise your
driver will leak badly.

> > +++ b/include/drm/drm_drv.h
> > @@ -505,21 +505,25 @@ struct drm_driver {
> >  	 * @gem_free_object: deconstructor for drm_gem_objects
> >  	 *
> >  	 * This is deprecated and should not be used by new drivers. Use
> > -	 * @gem_free_object_unlocked instead.
> > +	 * &drm_gem_object_funcs.free instead.
> >  	 */
> >  	void (*gem_free_object) (struct drm_gem_object *obj);
> >  
> >  	/**
> >  	 * @gem_free_object_unlocked: deconstructor for drm_gem_objects
> >  	 *
> > -	 * This is for drivers which are not encumbered with &drm_device.struct_mutex
> > -	 * legacy locking schemes. Use this hook instead of @gem_free_object.
> > +	 * This is deprecated and should not be used by new drivers. Use
> > +	 * &drm_gem_object_funcs.free instead.
> > +	 * Compared to @gem_free_object this is not encumbered with
> > +	 * &drm_device.struct_mutex legacy locking schemes.
> >  	 */
> It is confusing why the above comment refer to another callback.
> Is this the right wording?
> 	* Compared to @gem_free_object_unlocked this is not encumbered with
> 	* &drm_device.struct_mutex legacy locking schemes.
> 
> Do gem_free_object() warrant a similar comment?

I'm confused ... @gem_free_object is exactly the callback I want to refer
to. Because that one _is_ encumbered with the legacy struct_mutex locking
scheme. Unlike the .free callback, or the gem_free_object_unlocked
callback.

> > @@ -548,56 +557,120 @@ struct drm_driver {
> >  	/**
> >  	 * @gem_create_object: constructor for gem objects
> >  	 *
> > -	 * Hook for allocating the GEM object struct, for use by core
> > -	 * helpers.
> > +	 * Hook for allocating the GEM object struct, for use by the CMA and
> > +	 * SHMEM GEM helpers.
> >  	 */
> >  	struct drm_gem_object *(*gem_create_object)(struct drm_device *dev,
> >  						    size_t size);
> Nit: In some places we do:
> 	int (foo) (int bar);
> other:
> 	int (foo)(int bar);
> 
> No preference here, but it hurts my OCD a little that it is
> inconsistent.

I guess we could try to ocd fix that, but separate patch.

> 
> > -
> > -	/* prime: */
> >  	/**
> >  	 * @prime_handle_to_fd:
> >  	 *
> > -	 * export handle -> fd (see drm_gem_prime_handle_to_fd() helper)
> > +	 * Main PRIME export function. Should be implented with
> s/implented/implemented/
> 
> > +	 * drm_gem_prime_handle_to_fd() for GEM based drivers.
> > +	 *
> > +	 * For an in-depth discussion see :ref:`PRIME buffer sharing
> > +	 * documentation <prime_buffer_sharing>`.
> >  	 */
> >  	int (*prime_handle_to_fd)(struct drm_device *dev, struct drm_file *file_priv,
> >  				uint32_t handle, uint32_t flags, int *prime_fd);
> >  	/**
> >  	 * @prime_fd_to_handle:
> >  	 *
> > -	 * import fd -> handle (see drm_gem_prime_fd_to_handle() helper)
> > +	 * Main PRIME import function. Should be implented with
> s/implented/implemented/
> 
> > +	 * drm_gem_prime_fd_to_handle() for GEM based drivers.
> > +	 *
> > +	 * For an in-depth discussion see :ref:`PRIME buffer sharing
> > +	 * documentation <prime_buffer_sharing>`.
> >  	 */
> >  	int (*prime_fd_to_handle)(struct drm_device *dev, struct drm_file *file_priv,
> >  				int prime_fd, uint32_t *handle);
> >  	/**
> >  	 * @gem_prime_export:
> >  	 *
> > -	 * export GEM -> dmabuf
> > -	 *
> > -	 * This defaults to drm_gem_prime_export() if not set.
> > +	 * Export hook for GEM drivers. Deprecated in favour of
> > +	 * &drm_gem_object_funcs.export.
> >  	 */
> >  	struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
> >  				struct drm_gem_object *obj, int flags);
> >  	/**
> >  	 * @gem_prime_import:
> >  	 *
> > -	 * import dmabuf -> GEM
> > +	 * Import hook for GEM drivers.
> >  	 *
> >  	 * This defaults to drm_gem_prime_import() if not set.
> >  	 */
> >  	struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> >  				struct dma_buf *dma_buf);
> > +
> > +	/**
> > +	 * @gem_prime_pin:
> > +	 *
> > +	 * Deprecated hook in favour of &drm_gem_object_funcs.pin.
> > +	 */
> >  	int (*gem_prime_pin)(struct drm_gem_object *obj);
> > +
> > +	/**
> > +	 * @gem_prime_unpin:
> > +	 *
> > +	 * Deprecated hook in favour of &drm_gem_object_funcs.unpin.
> > +	 */
> >  	void (*gem_prime_unpin)(struct drm_gem_object *obj);
> > +
> > +
> > +	/**
> > +	 * @gem_prime_get_sg_table:
> > +	 *
> > +	 * Deprecated hook in favour of &drm_gem_object_funcs.get_sg_table.
> > +	 */
> > +	struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
> > +
> > +	/**
> > +	 * @gem_prime_res_obj:
> > +	 *
> > +	 * Optional hook to look up the &reservation_object for an buffer when
> > +	 * exporting it.
> > +	 *
> > +	 * FIXME: This hook is deprecated. User of this hook should be replaced
> User => Users?
> 
> > +	 * by setting &drm_gem_object.resv instead.
> > +	 */
> >  	struct reservation_object * (*gem_prime_res_obj)(
> >  				struct drm_gem_object *obj);
> > -	struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
> > +
> > +	/**
> > +	 * @gem_prime_import_sg_table:
> > +	 *
> > +	 * Optional hook used by the PRIME helper functions
> > +	 * drm_gem_prime_import() respectively drm_gem_prime_import_dev().
> > +	 */
> >  	struct drm_gem_object *(*gem_prime_import_sg_table)(
> >  				struct drm_device *dev,
> >  				struct dma_buf_attachment *attach,
> >  				struct sg_table *sgt);
> > +	/**
> > +	 * @gem_prime_vmap:
> > +	 *
> > +	 * Deprecated vmap hook for GEM drivers. Please use
> > +	 * &drm_gem_object_funcs.vmap instead.
> Or?
> 	 * Deprecated hook in favour of &drm_gem_object_funcs.vmap.
> 
> (Same wording as a few functions up)

Hm we're not super consistent with these deprecation notices even in
existing kerneldoc. Maybe something for a separate patch.

> 
> > +	 */
> >  	void *(*gem_prime_vmap)(struct drm_gem_object *obj);
> > +
> > +	/**
> > +	 * @gem_prime_vunmap:
> > +	 *
> > +	 * Deprecated vunmap hook for GEM drivers. Please use
> > +	 * &drm_gem_object_funcs.vunmap instead.
> Or?
> Y	 * Deprecated hook in favour of &drm_gem_object_funcs.vunmap.
> 
> > +	 */
> >  	void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
> > +
> > +	/**
> > +	 * @gem_prime_mmap:
> > +	 *
> > +	 * mmap hook for GEM drivers, used to implement dma-buf mmap in the
> > +	 * PRIME helpers.
> > +	 *
> > +	 * FIXME: There's way too much duplication going on here, and also moved
> > +	 * to &drm_gem_object_funcs.
> > +	 */
> >  	int (*gem_prime_mmap)(struct drm_gem_object *obj,
> >  				struct vm_area_struct *vma);
> >  
> > @@ -665,6 +738,9 @@ struct drm_driver {
> >  
> >  	/**
> >  	 * @gem_vm_ops: Driver private ops for this object
> > +	 *
> > +	 * For GEM driver this is deprecated in favour of
> > +	 * &drm_gem_object_funcs.vm_ops.
> Or?
> 	 * Deprecated hook in favour of &drm_gem_object_funcs.vm_ops.
> >  	 */
> >  	const struct vm_operations_struct *gem_vm_ops;
> >  

I've taken in all your other suggestions, thanks for taking a look.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx