Re: [PATCH 06/20] drm/i915: Introduce GEM object functions

Re: [PATCH 06/20] drm/i915: Introduce GEM object functions

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4339 bytes --]

Hi

Am 13.08.20 um 11:08 schrieb Jani Nikula:
> On Thu, 13 Aug 2020, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in i915.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/i915/gem/i915_gem_object.c       |  9 ++++++++-
>>  drivers/gpu/drm/i915/i915_drv.c                  | 10 ++++++----
>>  drivers/gpu/drm/i915/i915_drv.h                  |  1 +
>>  drivers/gpu/drm/i915/selftests/mock_gem_device.c |  3 ---
>>  4 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index c8421fd9d2dc..bc15ee4f2bd5 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -41,7 +41,14 @@ static struct i915_global_object {
>>  
>>  struct drm_i915_gem_object *i915_gem_object_alloc(void)
>>  {
>> -	return kmem_cache_zalloc(global.slab_objects, GFP_KERNEL);
>> +	struct drm_i915_gem_object *obj;
>> +
>> +	obj = kmem_cache_zalloc(global.slab_objects, GFP_KERNEL);
>> +	if (!obj)
>> +		return NULL;
>> +	obj->base.funcs = &i915_gem_object_funcs;
>> +
>> +	return obj;
>>  }
>>  
>>  void i915_gem_object_free(struct drm_i915_gem_object *obj)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 068447f565a9..b09eee11c540 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1840,6 +1840,12 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>>  	DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl, DRM_RENDER_ALLOW),
>>  };
>>  
>> +const struct drm_gem_object_funcs i915_gem_object_funcs = {
>> +	.free = i915_gem_free_object,
>> +	.close = i915_gem_close_object,
>> +	.export = i915_gem_prime_export,
>> +};
>> +
> 
> Any reason not to make this static in i915_gem_object.c next to its only
> user?

That is just an oversight. Will be fixed in the next iteration. Thanks.

Best regards
Thomas

> 
> BR,
> Jani.
> 
> 
>>  static struct drm_driver driver = {
>>  	/* Don't use MTRRs here; the Xserver or userspace app should
>>  	 * deal with them for Intel hardware.
>> @@ -1853,12 +1859,8 @@ static struct drm_driver driver = {
>>  	.lastclose = i915_driver_lastclose,
>>  	.postclose = i915_driver_postclose,
>>  
>> -	.gem_close_object = i915_gem_close_object,
>> -	.gem_free_object_unlocked = i915_gem_free_object,
>> -
>>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> -	.gem_prime_export = i915_gem_prime_export,
>>  	.gem_prime_import = i915_gem_prime_import,
>>  
>>  	.dumb_create = i915_gem_dumb_create,
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index bacb4c762f5b..666db65fe69e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1736,6 +1736,7 @@ intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *dev_priv)
>>  
>>  /* i915_drv.c */
>>  extern const struct dev_pm_ops i915_pm_ops;
>> +extern const struct drm_gem_object_funcs i915_gem_object_funcs;
>>  
>>  int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>>  void i915_driver_remove(struct drm_i915_private *i915);
>> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>> index ce4d4303229c..4725dad63e0a 100644
>> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>> @@ -86,9 +86,6 @@ static struct drm_driver mock_driver = {
>>  	.name = "mock",
>>  	.driver_features = DRIVER_GEM,
>>  	.release = mock_device_release,
>> -
>> -	.gem_close_object = i915_gem_close_object,
>> -	.gem_free_object_unlocked = i915_gem_free_object,
>>  };
>>  
>>  static void release_dev(struct device *dev)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 20/20] drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver

[Sam Ravnborg]

Hi Thomas.

On Thu, Aug 13, 2020 at 10:36:44AM +0200, Thomas Zimmermann wrote:
> Several GEM and PRIME callbacks have been deprecated in favor of
> per-instance GEM object functions. Remove the callbacks as they are
> now unused. The only exception is .gem_prime_mmap, which is still
> in use by several drivers.
> 
> What is also gone is gem_vm_ops in struct drm_driver. All drivers now
> use struct drm_gem_object_funcs.vm_ops instead.
> 
> While at it, the patch also improves error handling around calls
> to .free and .get_sg_table callbacks.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

After this following entry in todo.rst is done?

"
struct drm_gem_object_funcs
---------------------------

GEM objects can now have a function table instead of having the callbacks on the
DRM driver struct. This is now the preferred way and drivers can be moved over.

We also need a 2nd version of the CMA define that doesn't require the
vmapping to be present (different hook for prime importing). Plus this needs to
be rolled out to all drivers using their own implementations, too.
"

If yes, then delete it too.

	Sam

> ---
>  drivers/gpu/drm/drm_gem.c            | 35 +++---------
>  drivers/gpu/drm/drm_gem_cma_helper.c |  6 +-
>  drivers/gpu/drm/drm_prime.c          | 17 +++---
>  include/drm/drm_drv.h                | 85 ++--------------------------
>  4 files changed, 23 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 19d73868490e..96945bed8291 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -247,12 +247,9 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>  {
>  	struct drm_file *file_priv = data;
>  	struct drm_gem_object *obj = ptr;
> -	struct drm_device *dev = obj->dev;
>  
>  	if (obj->funcs && obj->funcs->close)
>  		obj->funcs->close(obj, file_priv);
> -	else if (dev->driver->gem_close_object)
> -		dev->driver->gem_close_object(obj, file_priv);
>  
>  	drm_gem_remove_prime_handles(obj, file_priv);
>  	drm_vma_node_revoke(&obj->vma_node, file_priv);
> @@ -407,10 +404,6 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  		ret = obj->funcs->open(obj, file_priv);
>  		if (ret)
>  			goto err_revoke;
> -	} else if (dev->driver->gem_open_object) {
> -		ret = dev->driver->gem_open_object(obj, file_priv);
> -		if (ret)
> -			goto err_revoke;
>  	}
>  
>  	*handlep = handle;
> @@ -982,12 +975,11 @@ drm_gem_object_free(struct kref *kref)
>  {
>  	struct drm_gem_object *obj =
>  		container_of(kref, struct drm_gem_object, refcount);
> -	struct drm_device *dev = obj->dev;
>  
> -	if (obj->funcs)
> -		obj->funcs->free(obj);
> -	else if (dev->driver->gem_free_object_unlocked)
> -		dev->driver->gem_free_object_unlocked(obj);
> +	if (drm_WARN_ON_ONCE(obj->dev, !obj->funcs || !obj->funcs->free))
> +		return;
> +
> +	obj->funcs->free(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_object_free);
>  
> @@ -1049,9 +1041,9 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * @obj_size: the object size to be mapped, in bytes
>   * @vma: VMA for the area to be mapped
>   *
> - * Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops
> - * provided by the driver. Depending on their requirements, drivers can either
> - * provide a fault handler in their gem_vm_ops (in which case any accesses to
> + * Set up the VMA to prepare mapping of the GEM object using the GEM object's
> + * vm_ops. Depending on their requirements, GEM objects can either
> + * provide a fault handler in their vm_ops (in which case any accesses to
>   * the object will be trapped, to perform migration, GTT binding, surface
>   * register allocation, or performance monitoring), or mmap the buffer memory
>   * synchronously after calling drm_gem_mmap_obj.
> @@ -1065,12 +1057,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * callers must verify access restrictions before calling this helper.
>   *
>   * Return 0 or success or -EINVAL if the object size is smaller than the VMA
> - * size, or if no gem_vm_ops are provided.
> + * size, or if no vm_ops are provided.
>   */
>  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  		     struct vm_area_struct *vma)
>  {
> -	struct drm_device *dev = obj->dev;
>  	int ret;
>  
>  	/* Check for valid size. */
> @@ -1095,8 +1086,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	} else {
>  		if (obj->funcs && obj->funcs->vm_ops)
>  			vma->vm_ops = obj->funcs->vm_ops;
> -		else if (dev->driver->gem_vm_ops)
> -			vma->vm_ops = dev->driver->gem_vm_ops;
>  		else {
>  			drm_gem_object_put(obj);
>  			return -EINVAL;
> @@ -1206,8 +1195,6 @@ int drm_gem_pin(struct drm_gem_object *obj)
>  {
>  	if (obj->funcs && obj->funcs->pin)
>  		return obj->funcs->pin(obj);
> -	else if (obj->dev->driver->gem_prime_pin)
> -		return obj->dev->driver->gem_prime_pin(obj);
>  	else
>  		return 0;
>  }
> @@ -1216,8 +1203,6 @@ void drm_gem_unpin(struct drm_gem_object *obj)
>  {
>  	if (obj->funcs && obj->funcs->unpin)
>  		obj->funcs->unpin(obj);
> -	else if (obj->dev->driver->gem_prime_unpin)
> -		obj->dev->driver->gem_prime_unpin(obj);
>  }
>  
>  void *drm_gem_vmap(struct drm_gem_object *obj)
> @@ -1226,8 +1211,6 @@ void *drm_gem_vmap(struct drm_gem_object *obj)
>  
>  	if (obj->funcs && obj->funcs->vmap)
>  		vaddr = obj->funcs->vmap(obj);
> -	else if (obj->dev->driver->gem_prime_vmap)
> -		vaddr = obj->dev->driver->gem_prime_vmap(obj);
>  	else
>  		vaddr = ERR_PTR(-EOPNOTSUPP);
>  
> @@ -1244,8 +1227,6 @@ void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
>  
>  	if (obj->funcs && obj->funcs->vunmap)
>  		obj->funcs->vunmap(obj, vaddr);
> -	else if (obj->dev->driver->gem_prime_vunmap)
> -		obj->dev->driver->gem_prime_vunmap(obj, vaddr);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 822edeadbab3..209202257cc2 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -419,7 +419,7 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
>   *
>   * This function exports a scatter/gather table suitable for PRIME usage by
>   * calling the standard DMA mapping API. Drivers using the CMA helpers should
> - * set this as their &drm_driver.gem_prime_get_sg_table callback.
> + * set this as their &drm_gem_object_funcs.get_sg_table callback.
>   *
>   * Returns:
>   * A pointer to the scatter/gather table of pinned pages or NULL on failure.
> @@ -542,7 +542,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
>   * virtual address space. Since the CMA buffers are already mapped into the
>   * kernel virtual address space this simply returns the cached virtual
>   * address. Drivers using the CMA helpers should set this as their DRM
> - * driver's &drm_driver.gem_prime_vmap callback.
> + * driver's &drm_gem_object_funcs.vmap callback.
>   *
>   * Returns:
>   * The kernel virtual address of the CMA GEM object's backing store.
> @@ -564,7 +564,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
>   * This function removes a buffer exported via DRM PRIME from the kernel's
>   * virtual address space. This is a no-op because CMA buffers cannot be
>   * unmapped from kernel space. Drivers using the CMA helpers should set this
> - * as their &drm_driver.gem_prime_vunmap callback.
> + * as their &drm_gem_object_funcs.vunmap callback.
>   */
>  void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
>  {
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 1693aa7c14b5..9a87639e7008 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -386,10 +386,6 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>  
>  	if (obj->funcs && obj->funcs->export)
>  		dmabuf = obj->funcs->export(obj, flags);
> -	else if (dev->driver->gem_prime_export)
> -		dmabuf = dev->driver->gem_prime_export(obj, flags);
> -	else
> -		dmabuf = drm_gem_prime_export(obj, flags);
>  	if (IS_ERR(dmabuf)) {
>  		/* normally the created dma-buf takes ownership of the ref,
>  		 * but if that fails then drop the ref
> @@ -419,7 +415,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>   * This is the PRIME export function which must be used mandatorily by GEM
>   * drivers to ensure correct lifetime management of the underlying GEM object.
>   * The actual exporting from GEM object to a dma-buf is done through the
> - * &drm_driver.gem_prime_export driver callback.
> + * &drm_gem_object_funcs.export callback.
>   */
>  int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  			       struct drm_file *file_priv, uint32_t handle,
> @@ -616,15 +612,18 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>  				     enum dma_data_direction dir)
>  {
>  	struct drm_gem_object *obj = attach->dmabuf->priv;
> -	struct sg_table *sgt;
> +	struct sg_table *sgt = NULL;
>  
>  	if (WARN_ON(dir == DMA_NONE))
>  		return ERR_PTR(-EINVAL);
>  
> -	if (obj->funcs)
> +	if (obj->funcs && obj->funcs->get_sg_table)
>  		sgt = obj->funcs->get_sg_table(obj);
> -	else
> -		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> +
> +	if (!sgt)
> +		return ERR_PTR(-EINVAL);
> +	else if (IS_ERR(sgt))
> +		return sgt;
>  
>  	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
>  			      DMA_ATTR_SKIP_CPU_SYNC)) {
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 7116abc1a04e..8b325bf6cef4 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -36,10 +36,12 @@ struct drm_file;
>  struct drm_gem_object;
>  struct drm_master;
>  struct drm_minor;
> +struct dma_buf;
>  struct dma_buf_attachment;
>  struct drm_display_mode;
>  struct drm_mode_create_dumb;
>  struct drm_printer;
> +struct sg_table;
>  
>  /**
>   * enum drm_driver_feature - feature flags
> @@ -327,32 +329,6 @@ struct drm_driver {
>  	 */
>  	void (*debugfs_init)(struct drm_minor *minor);
>  
> -	/**
> -	 * @gem_free_object_unlocked: deconstructor for drm_gem_objects
> -	 *
> -	 * This is deprecated and should not be used by new drivers. Use
> -	 * &drm_gem_object_funcs.free instead.
> -	 */
> -	void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
> -
> -	/**
> -	 * @gem_open_object:
> -	 *
> -	 * This callback is deprecated in favour of &drm_gem_object_funcs.open.
> -	 *
> -	 * Driver hook called upon gem handle creation
> -	 */
> -	int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
> -
> -	/**
> -	 * @gem_close_object:
> -	 *
> -	 * This callback is deprecated in favour of &drm_gem_object_funcs.close.
> -	 *
> -	 * Driver hook called upon gem handle release
> -	 */
> -	void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
> -
>  	/**
>  	 * @gem_create_object: constructor for gem objects
>  	 *
> @@ -361,6 +337,7 @@ struct drm_driver {
>  	 */
>  	struct drm_gem_object *(*gem_create_object)(struct drm_device *dev,
>  						    size_t size);
> +
>  	/**
>  	 * @prime_handle_to_fd:
>  	 *
> @@ -383,14 +360,7 @@ struct drm_driver {
>  	 */
>  	int (*prime_fd_to_handle)(struct drm_device *dev, struct drm_file *file_priv,
>  				int prime_fd, uint32_t *handle);
> -	/**
> -	 * @gem_prime_export:
> -	 *
> -	 * Export hook for GEM drivers. Deprecated in favour of
> -	 * &drm_gem_object_funcs.export.
> -	 */
> -	struct dma_buf * (*gem_prime_export)(struct drm_gem_object *obj,
> -					     int flags);
> +
>  	/**
>  	 * @gem_prime_import:
>  	 *
> @@ -400,29 +370,6 @@ struct drm_driver {
>  	 */
>  	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_import_sg_table:
>  	 *
> @@ -433,22 +380,6 @@ struct drm_driver {
>  				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.
> -	 */
> -	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.
> -	 */
> -	void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
> -
>  	/**
>  	 * @gem_prime_mmap:
>  	 *
> @@ -523,14 +454,6 @@ struct drm_driver {
>  			    struct drm_device *dev,
>  			    uint32_t handle);
>  
> -	/**
> -	 * @gem_vm_ops: Driver private ops for this object
> -	 *
> -	 * For GEM drivers this is deprecated in favour of
> -	 * &drm_gem_object_funcs.vm_ops.
> -	 */
> -	const struct vm_operations_struct *gem_vm_ops;
> -
>  	/** @major: driver major number */
>  	int major;
>  	/** @minor: driver minor number */
> -- 
> 2.28.0

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 20/20] drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 14886 bytes --]

Hi

Am 13.08.20 um 12:16 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Thu, Aug 13, 2020 at 10:36:44AM +0200, Thomas Zimmermann wrote:
>> Several GEM and PRIME callbacks have been deprecated in favor of
>> per-instance GEM object functions. Remove the callbacks as they are
>> now unused. The only exception is .gem_prime_mmap, which is still
>> in use by several drivers.
>>
>> What is also gone is gem_vm_ops in struct drm_driver. All drivers now
>> use struct drm_gem_object_funcs.vm_ops instead.
>>
>> While at it, the patch also improves error handling around calls
>> to .free and .get_sg_table callbacks.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> After this following entry in todo.rst is done?
> 
> "
> struct drm_gem_object_funcs
> ---------------------------
> 
> GEM objects can now have a function table instead of having the callbacks on the
> DRM driver struct. This is now the preferred way and drivers can be moved over.

This should only say that .gem_prime_mmap() is left in struct
drm_drivers for conversion.

> 
> We also need a 2nd version of the CMA define that doesn't require the
> vmapping to be present (different hook for prime importing). Plus this needs to
> be rolled out to all drivers using their own implementations, too.
> "

This sounds like it got fixed in the recent CMA clean-up series. There
are CMA initializer macros for drivers with and without vmap. I have to
take a closer look, but I think it can be removed.

Thanks for bringing this up.

Best regards
Thomas

> 
> If yes, then delete it too.
> 
> 	Sam
> 
>> ---
>>  drivers/gpu/drm/drm_gem.c            | 35 +++---------
>>  drivers/gpu/drm/drm_gem_cma_helper.c |  6 +-
>>  drivers/gpu/drm/drm_prime.c          | 17 +++---
>>  include/drm/drm_drv.h                | 85 ++--------------------------
>>  4 files changed, 23 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 19d73868490e..96945bed8291 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -247,12 +247,9 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>>  {
>>  	struct drm_file *file_priv = data;
>>  	struct drm_gem_object *obj = ptr;
>> -	struct drm_device *dev = obj->dev;
>>  
>>  	if (obj->funcs && obj->funcs->close)
>>  		obj->funcs->close(obj, file_priv);
>> -	else if (dev->driver->gem_close_object)
>> -		dev->driver->gem_close_object(obj, file_priv);
>>  
>>  	drm_gem_remove_prime_handles(obj, file_priv);
>>  	drm_vma_node_revoke(&obj->vma_node, file_priv);
>> @@ -407,10 +404,6 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>>  		ret = obj->funcs->open(obj, file_priv);
>>  		if (ret)
>>  			goto err_revoke;
>> -	} else if (dev->driver->gem_open_object) {
>> -		ret = dev->driver->gem_open_object(obj, file_priv);
>> -		if (ret)
>> -			goto err_revoke;
>>  	}
>>  
>>  	*handlep = handle;
>> @@ -982,12 +975,11 @@ drm_gem_object_free(struct kref *kref)
>>  {
>>  	struct drm_gem_object *obj =
>>  		container_of(kref, struct drm_gem_object, refcount);
>> -	struct drm_device *dev = obj->dev;
>>  
>> -	if (obj->funcs)
>> -		obj->funcs->free(obj);
>> -	else if (dev->driver->gem_free_object_unlocked)
>> -		dev->driver->gem_free_object_unlocked(obj);
>> +	if (drm_WARN_ON_ONCE(obj->dev, !obj->funcs || !obj->funcs->free))
>> +		return;
>> +
>> +	obj->funcs->free(obj);
>>  }
>>  EXPORT_SYMBOL(drm_gem_object_free);
>>  
>> @@ -1049,9 +1041,9 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>>   * @obj_size: the object size to be mapped, in bytes
>>   * @vma: VMA for the area to be mapped
>>   *
>> - * Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops
>> - * provided by the driver. Depending on their requirements, drivers can either
>> - * provide a fault handler in their gem_vm_ops (in which case any accesses to
>> + * Set up the VMA to prepare mapping of the GEM object using the GEM object's
>> + * vm_ops. Depending on their requirements, GEM objects can either
>> + * provide a fault handler in their vm_ops (in which case any accesses to
>>   * the object will be trapped, to perform migration, GTT binding, surface
>>   * register allocation, or performance monitoring), or mmap the buffer memory
>>   * synchronously after calling drm_gem_mmap_obj.
>> @@ -1065,12 +1057,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>>   * callers must verify access restrictions before calling this helper.
>>   *
>>   * Return 0 or success or -EINVAL if the object size is smaller than the VMA
>> - * size, or if no gem_vm_ops are provided.
>> + * size, or if no vm_ops are provided.
>>   */
>>  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>>  		     struct vm_area_struct *vma)
>>  {
>> -	struct drm_device *dev = obj->dev;
>>  	int ret;
>>  
>>  	/* Check for valid size. */
>> @@ -1095,8 +1086,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>>  	} else {
>>  		if (obj->funcs && obj->funcs->vm_ops)
>>  			vma->vm_ops = obj->funcs->vm_ops;
>> -		else if (dev->driver->gem_vm_ops)
>> -			vma->vm_ops = dev->driver->gem_vm_ops;
>>  		else {
>>  			drm_gem_object_put(obj);
>>  			return -EINVAL;
>> @@ -1206,8 +1195,6 @@ int drm_gem_pin(struct drm_gem_object *obj)
>>  {
>>  	if (obj->funcs && obj->funcs->pin)
>>  		return obj->funcs->pin(obj);
>> -	else if (obj->dev->driver->gem_prime_pin)
>> -		return obj->dev->driver->gem_prime_pin(obj);
>>  	else
>>  		return 0;
>>  }
>> @@ -1216,8 +1203,6 @@ void drm_gem_unpin(struct drm_gem_object *obj)
>>  {
>>  	if (obj->funcs && obj->funcs->unpin)
>>  		obj->funcs->unpin(obj);
>> -	else if (obj->dev->driver->gem_prime_unpin)
>> -		obj->dev->driver->gem_prime_unpin(obj);
>>  }
>>  
>>  void *drm_gem_vmap(struct drm_gem_object *obj)
>> @@ -1226,8 +1211,6 @@ void *drm_gem_vmap(struct drm_gem_object *obj)
>>  
>>  	if (obj->funcs && obj->funcs->vmap)
>>  		vaddr = obj->funcs->vmap(obj);
>> -	else if (obj->dev->driver->gem_prime_vmap)
>> -		vaddr = obj->dev->driver->gem_prime_vmap(obj);
>>  	else
>>  		vaddr = ERR_PTR(-EOPNOTSUPP);
>>  
>> @@ -1244,8 +1227,6 @@ void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
>>  
>>  	if (obj->funcs && obj->funcs->vunmap)
>>  		obj->funcs->vunmap(obj, vaddr);
>> -	else if (obj->dev->driver->gem_prime_vunmap)
>> -		obj->dev->driver->gem_prime_vunmap(obj, vaddr);
>>  }
>>  
>>  /**
>> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
>> index 822edeadbab3..209202257cc2 100644
>> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> @@ -419,7 +419,7 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
>>   *
>>   * This function exports a scatter/gather table suitable for PRIME usage by
>>   * calling the standard DMA mapping API. Drivers using the CMA helpers should
>> - * set this as their &drm_driver.gem_prime_get_sg_table callback.
>> + * set this as their &drm_gem_object_funcs.get_sg_table callback.
>>   *
>>   * Returns:
>>   * A pointer to the scatter/gather table of pinned pages or NULL on failure.
>> @@ -542,7 +542,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
>>   * virtual address space. Since the CMA buffers are already mapped into the
>>   * kernel virtual address space this simply returns the cached virtual
>>   * address. Drivers using the CMA helpers should set this as their DRM
>> - * driver's &drm_driver.gem_prime_vmap callback.
>> + * driver's &drm_gem_object_funcs.vmap callback.
>>   *
>>   * Returns:
>>   * The kernel virtual address of the CMA GEM object's backing store.
>> @@ -564,7 +564,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
>>   * This function removes a buffer exported via DRM PRIME from the kernel's
>>   * virtual address space. This is a no-op because CMA buffers cannot be
>>   * unmapped from kernel space. Drivers using the CMA helpers should set this
>> - * as their &drm_driver.gem_prime_vunmap callback.
>> + * as their &drm_gem_object_funcs.vunmap callback.
>>   */
>>  void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
>>  {
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 1693aa7c14b5..9a87639e7008 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -386,10 +386,6 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>>  
>>  	if (obj->funcs && obj->funcs->export)
>>  		dmabuf = obj->funcs->export(obj, flags);
>> -	else if (dev->driver->gem_prime_export)
>> -		dmabuf = dev->driver->gem_prime_export(obj, flags);
>> -	else
>> -		dmabuf = drm_gem_prime_export(obj, flags);
>>  	if (IS_ERR(dmabuf)) {
>>  		/* normally the created dma-buf takes ownership of the ref,
>>  		 * but if that fails then drop the ref
>> @@ -419,7 +415,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>>   * This is the PRIME export function which must be used mandatorily by GEM
>>   * drivers to ensure correct lifetime management of the underlying GEM object.
>>   * The actual exporting from GEM object to a dma-buf is done through the
>> - * &drm_driver.gem_prime_export driver callback.
>> + * &drm_gem_object_funcs.export callback.
>>   */
>>  int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>>  			       struct drm_file *file_priv, uint32_t handle,
>> @@ -616,15 +612,18 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>>  				     enum dma_data_direction dir)
>>  {
>>  	struct drm_gem_object *obj = attach->dmabuf->priv;
>> -	struct sg_table *sgt;
>> +	struct sg_table *sgt = NULL;
>>  
>>  	if (WARN_ON(dir == DMA_NONE))
>>  		return ERR_PTR(-EINVAL);
>>  
>> -	if (obj->funcs)
>> +	if (obj->funcs && obj->funcs->get_sg_table)
>>  		sgt = obj->funcs->get_sg_table(obj);
>> -	else
>> -		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>> +
>> +	if (!sgt)
>> +		return ERR_PTR(-EINVAL);
>> +	else if (IS_ERR(sgt))
>> +		return sgt;
>>  
>>  	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
>>  			      DMA_ATTR_SKIP_CPU_SYNC)) {
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 7116abc1a04e..8b325bf6cef4 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -36,10 +36,12 @@ struct drm_file;
>>  struct drm_gem_object;
>>  struct drm_master;
>>  struct drm_minor;
>> +struct dma_buf;
>>  struct dma_buf_attachment;
>>  struct drm_display_mode;
>>  struct drm_mode_create_dumb;
>>  struct drm_printer;
>> +struct sg_table;
>>  
>>  /**
>>   * enum drm_driver_feature - feature flags
>> @@ -327,32 +329,6 @@ struct drm_driver {
>>  	 */
>>  	void (*debugfs_init)(struct drm_minor *minor);
>>  
>> -	/**
>> -	 * @gem_free_object_unlocked: deconstructor for drm_gem_objects
>> -	 *
>> -	 * This is deprecated and should not be used by new drivers. Use
>> -	 * &drm_gem_object_funcs.free instead.
>> -	 */
>> -	void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
>> -
>> -	/**
>> -	 * @gem_open_object:
>> -	 *
>> -	 * This callback is deprecated in favour of &drm_gem_object_funcs.open.
>> -	 *
>> -	 * Driver hook called upon gem handle creation
>> -	 */
>> -	int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
>> -
>> -	/**
>> -	 * @gem_close_object:
>> -	 *
>> -	 * This callback is deprecated in favour of &drm_gem_object_funcs.close.
>> -	 *
>> -	 * Driver hook called upon gem handle release
>> -	 */
>> -	void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
>> -
>>  	/**
>>  	 * @gem_create_object: constructor for gem objects
>>  	 *
>> @@ -361,6 +337,7 @@ struct drm_driver {
>>  	 */
>>  	struct drm_gem_object *(*gem_create_object)(struct drm_device *dev,
>>  						    size_t size);
>> +
>>  	/**
>>  	 * @prime_handle_to_fd:
>>  	 *
>> @@ -383,14 +360,7 @@ struct drm_driver {
>>  	 */
>>  	int (*prime_fd_to_handle)(struct drm_device *dev, struct drm_file *file_priv,
>>  				int prime_fd, uint32_t *handle);
>> -	/**
>> -	 * @gem_prime_export:
>> -	 *
>> -	 * Export hook for GEM drivers. Deprecated in favour of
>> -	 * &drm_gem_object_funcs.export.
>> -	 */
>> -	struct dma_buf * (*gem_prime_export)(struct drm_gem_object *obj,
>> -					     int flags);
>> +
>>  	/**
>>  	 * @gem_prime_import:
>>  	 *
>> @@ -400,29 +370,6 @@ struct drm_driver {
>>  	 */
>>  	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_import_sg_table:
>>  	 *
>> @@ -433,22 +380,6 @@ struct drm_driver {
>>  				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.
>> -	 */
>> -	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.
>> -	 */
>> -	void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
>> -
>>  	/**
>>  	 * @gem_prime_mmap:
>>  	 *
>> @@ -523,14 +454,6 @@ struct drm_driver {
>>  			    struct drm_device *dev,
>>  			    uint32_t handle);
>>  
>> -	/**
>> -	 * @gem_vm_ops: Driver private ops for this object
>> -	 *
>> -	 * For GEM drivers this is deprecated in favour of
>> -	 * &drm_gem_object_funcs.vm_ops.
>> -	 */
>> -	const struct vm_operations_struct *gem_vm_ops;
>> -
>>  	/** @major: driver major number */
>>  	int major;
>>  	/** @minor: driver minor number */
>> -- 
>> 2.28.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 01/20] drm/amdgpu: Introduce GEM object functions

[Christian König]

Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 ------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12 ++++++++++++
>   2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 81a79760ca61..51525b8774c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1468,19 +1468,13 @@ static struct drm_driver kms_driver = {
>   	.lastclose = amdgpu_driver_lastclose_kms,
>   	.irq_handler = amdgpu_irq_handler,
>   	.ioctls = amdgpu_ioctls_kms,
> -	.gem_free_object_unlocked = amdgpu_gem_object_free,
> -	.gem_open_object = amdgpu_gem_object_open,
> -	.gem_close_object = amdgpu_gem_object_close,
>   	.dumb_create = amdgpu_mode_dumb_create,
>   	.dumb_map_offset = amdgpu_mode_dumb_mmap,
>   	.fops = &amdgpu_driver_kms_fops,
>   
>   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> -	.gem_prime_export = amdgpu_gem_prime_export,
>   	.gem_prime_import = amdgpu_gem_prime_import,
> -	.gem_prime_vmap = amdgpu_gem_prime_vmap,
> -	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>   	.gem_prime_mmap = amdgpu_gem_prime_mmap,
>   
>   	.name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 43f4966331dd..ca2b79f94e99 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -36,6 +36,7 @@
>   #include <drm/amdgpu_drm.h>
>   #include <drm/drm_cache.h>
>   #include "amdgpu.h"
> +#include "amdgpu_dma_buf.h"
>   #include "amdgpu_trace.h"
>   #include "amdgpu_amdkfd.h"
>   
> @@ -510,6 +511,15 @@ bool amdgpu_bo_support_uswc(u64 bo_flags)
>   #endif
>   }
>   
> +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
> +	.free = amdgpu_gem_object_free,
> +	.open = amdgpu_gem_object_open,
> +	.close = amdgpu_gem_object_close,
> +	.export = amdgpu_gem_prime_export,
> +	.vmap = amdgpu_gem_prime_vmap,
> +	.vunmap = amdgpu_gem_prime_vunmap,
> +};
> +

Wrong file, this belongs into amdgpu_gem.c

>   static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   			       struct amdgpu_bo_param *bp,
>   			       struct amdgpu_bo **bo_ptr)
> @@ -552,6 +562,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   	bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
>   	if (bo == NULL)
>   		return -ENOMEM;
> +
> +	bo->tbo.base.funcs = &amdgpu_gem_object_funcs;

And this should probably go into amdgpu_gem_object_create().

Apart from that looks like a good idea to me.

Christian.

>   	drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
>   	INIT_LIST_HEAD(&bo->shadow_list);
>   	bo->vm_bo = NULL;


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 01/20] drm/amdgpu: Introduce GEM object functions

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4089 bytes --]

Hi

Am 13.08.20 um 12:22 schrieb Christian König:
> Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
>> which is non-trivial to convert.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 ------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12 ++++++++++++
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 81a79760ca61..51525b8774c9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -1468,19 +1468,13 @@ static struct drm_driver kms_driver = {
>>       .lastclose = amdgpu_driver_lastclose_kms,
>>       .irq_handler = amdgpu_irq_handler,
>>       .ioctls = amdgpu_ioctls_kms,
>> -    .gem_free_object_unlocked = amdgpu_gem_object_free,
>> -    .gem_open_object = amdgpu_gem_object_open,
>> -    .gem_close_object = amdgpu_gem_object_close,
>>       .dumb_create = amdgpu_mode_dumb_create,
>>       .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>       .fops = &amdgpu_driver_kms_fops,
>>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> -    .gem_prime_export = amdgpu_gem_prime_export,
>>       .gem_prime_import = amdgpu_gem_prime_import,
>> -    .gem_prime_vmap = amdgpu_gem_prime_vmap,
>> -    .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>>       .gem_prime_mmap = amdgpu_gem_prime_mmap,
>>         .name = DRIVER_NAME,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 43f4966331dd..ca2b79f94e99 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -36,6 +36,7 @@
>>   #include <drm/amdgpu_drm.h>
>>   #include <drm/drm_cache.h>
>>   #include "amdgpu.h"
>> +#include "amdgpu_dma_buf.h"
>>   #include "amdgpu_trace.h"
>>   #include "amdgpu_amdkfd.h"
>>   @@ -510,6 +511,15 @@ bool amdgpu_bo_support_uswc(u64 bo_flags)
>>   #endif
>>   }
>>   +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
>> +    .free = amdgpu_gem_object_free,
>> +    .open = amdgpu_gem_object_open,
>> +    .close = amdgpu_gem_object_close,
>> +    .export = amdgpu_gem_prime_export,
>> +    .vmap = amdgpu_gem_prime_vmap,
>> +    .vunmap = amdgpu_gem_prime_vunmap,
>> +};
>> +
> 
> Wrong file, this belongs into amdgpu_gem.c
> 
>>   static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>>                      struct amdgpu_bo_param *bp,
>>                      struct amdgpu_bo **bo_ptr)
>> @@ -552,6 +562,8 @@ static int amdgpu_bo_do_create(struct
>> amdgpu_device *adev,
>>       bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
>>       if (bo == NULL)
>>           return -ENOMEM;
>> +
>> +    bo->tbo.base.funcs = &amdgpu_gem_object_funcs;
> 
> And this should probably go into amdgpu_gem_object_create().

I'm trying to understand what amdgpu does.  What about all the places
where amdgpu calls amdgpu_bo_create() internally? Wouldn't these miss
the free callback for the GEM object?

Best regards
Thomas

> 
> Apart from that looks like a good idea to me.
> 
> Christian.
> 
>>       drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
>>       INIT_LIST_HEAD(&bo->shadow_list);
>>       bo->vm_bo = NULL;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 12/20] drm/radeon: Introduce GEM object functions

[Christian König]

Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in radeon.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/radeon/radeon_drv.c    | 23 +----------------------
>   drivers/gpu/drm/radeon/radeon_object.c | 26 ++++++++++++++++++++++++++
>   2 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 4cd30613fa1d..65061c949aee 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -124,13 +124,6 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
>   int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
>   void radeon_driver_irq_uninstall_kms(struct drm_device *dev);
>   irqreturn_t radeon_driver_irq_handler_kms(int irq, void *arg);
> -void radeon_gem_object_free(struct drm_gem_object *obj);
> -int radeon_gem_object_open(struct drm_gem_object *obj,
> -				struct drm_file *file_priv);
> -void radeon_gem_object_close(struct drm_gem_object *obj,
> -				struct drm_file *file_priv);
> -struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
> -					int flags);
>   extern int radeon_get_crtc_scanoutpos(struct drm_device *dev, unsigned int crtc,
>   				      unsigned int flags, int *vpos, int *hpos,
>   				      ktime_t *stime, ktime_t *etime,
> @@ -145,14 +138,9 @@ int radeon_mode_dumb_mmap(struct drm_file *filp,
>   int radeon_mode_dumb_create(struct drm_file *file_priv,
>   			    struct drm_device *dev,
>   			    struct drm_mode_create_dumb *args);
> -struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj);
>   struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev,
>   							struct dma_buf_attachment *,
>   							struct sg_table *sg);
> -int radeon_gem_prime_pin(struct drm_gem_object *obj);
> -void radeon_gem_prime_unpin(struct drm_gem_object *obj);
> -void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
> -void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
>   
>   /* atpx handler */
>   #if defined(CONFIG_VGA_SWITCHEROO)
> @@ -550,7 +538,7 @@ long radeon_drm_ioctl(struct file *filp,
>   	}
>   
>   	ret = drm_ioctl(filp, cmd, arg);
> -	
> +
>   	pm_runtime_mark_last_busy(dev->dev);
>   	pm_runtime_put_autosuspend(dev->dev);
>   	return ret;
> @@ -609,22 +597,13 @@ static struct drm_driver kms_driver = {
>   	.irq_uninstall = radeon_driver_irq_uninstall_kms,
>   	.irq_handler = radeon_driver_irq_handler_kms,
>   	.ioctls = radeon_ioctls_kms,
> -	.gem_free_object_unlocked = radeon_gem_object_free,
> -	.gem_open_object = radeon_gem_object_open,
> -	.gem_close_object = radeon_gem_object_close,
>   	.dumb_create = radeon_mode_dumb_create,
>   	.dumb_map_offset = radeon_mode_dumb_mmap,
>   	.fops = &radeon_driver_kms_fops,
>   
>   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> -	.gem_prime_export = radeon_gem_prime_export,
> -	.gem_prime_pin = radeon_gem_prime_pin,
> -	.gem_prime_unpin = radeon_gem_prime_unpin,
> -	.gem_prime_get_sg_table = radeon_gem_prime_get_sg_table,
>   	.gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
> -	.gem_prime_vmap = radeon_gem_prime_vmap,
> -	.gem_prime_vunmap = radeon_gem_prime_vunmap,
>   
>   	.name = DRIVER_NAME,
>   	.desc = DRIVER_DESC,
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index bb7582afd803..882390e15dfe 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -45,6 +45,19 @@ int radeon_ttm_init(struct radeon_device *rdev);
>   void radeon_ttm_fini(struct radeon_device *rdev);
>   static void radeon_bo_clear_surface_reg(struct radeon_bo *bo);
>   
> +void radeon_gem_object_free(struct drm_gem_object *obj);
> +int radeon_gem_object_open(struct drm_gem_object *obj,
> +				struct drm_file *file_priv);
> +void radeon_gem_object_close(struct drm_gem_object *obj,
> +				struct drm_file *file_priv);
> +struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
> +					int flags);
> +struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj);
> +int radeon_gem_prime_pin(struct drm_gem_object *obj);
> +void radeon_gem_prime_unpin(struct drm_gem_object *obj);
> +void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
> +void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
> +
>   /*
>    * To exclude mutual BO access we rely on bo_reserve exclusion, as all
>    * function are calling it.
> @@ -180,6 +193,18 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
>   	}
>   }
>   
> +static const struct drm_gem_object_funcs radeon_gem_object_funcs = {
> +	.free = radeon_gem_object_free,
> +	.open = radeon_gem_object_open,
> +	.close = radeon_gem_object_close,
> +	.export = radeon_gem_prime_export,
> +	.pin = radeon_gem_prime_pin,
> +	.unpin = radeon_gem_prime_unpin,
> +	.get_sg_table = radeon_gem_prime_get_sg_table,
> +	.vmap = radeon_gem_prime_vmap,
> +	.vunmap = radeon_gem_prime_vunmap,
> +};
> +

Same comment as for amdgpu, please put that into radeon_gem.c instead.

Christian.

>   int radeon_bo_create(struct radeon_device *rdev,
>   		     unsigned long size, int byte_align, bool kernel,
>   		     u32 domain, u32 flags, struct sg_table *sg,
> @@ -209,6 +234,7 @@ int radeon_bo_create(struct radeon_device *rdev,
>   	bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL);
>   	if (bo == NULL)
>   		return -ENOMEM;
> +	bo->tbo.base.funcs = &radeon_gem_object_funcs;
>   	drm_gem_private_object_init(rdev->ddev, &bo->tbo.base, size);
>   	bo->rdev = rdev;
>   	bo->surface_reg = -1;


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 12/20] drm/radeon: Introduce GEM object functions

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 7363 bytes --]

Hi

Am 13.08.20 um 12:24 schrieb Christian König:
> Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in radeon.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/radeon/radeon_drv.c    | 23 +----------------------
>>   drivers/gpu/drm/radeon/radeon_object.c | 26 ++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
>> b/drivers/gpu/drm/radeon/radeon_drv.c
>> index 4cd30613fa1d..65061c949aee 100644
>> --- a/drivers/gpu/drm/radeon/radeon_drv.c
>> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
>> @@ -124,13 +124,6 @@ void radeon_driver_irq_preinstall_kms(struct
>> drm_device *dev);
>>   int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
>>   void radeon_driver_irq_uninstall_kms(struct drm_device *dev);
>>   irqreturn_t radeon_driver_irq_handler_kms(int irq, void *arg);
>> -void radeon_gem_object_free(struct drm_gem_object *obj);
>> -int radeon_gem_object_open(struct drm_gem_object *obj,
>> -                struct drm_file *file_priv);
>> -void radeon_gem_object_close(struct drm_gem_object *obj,
>> -                struct drm_file *file_priv);
>> -struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
>> -                    int flags);
>>   extern int radeon_get_crtc_scanoutpos(struct drm_device *dev,
>> unsigned int crtc,
>>                         unsigned int flags, int *vpos, int *hpos,
>>                         ktime_t *stime, ktime_t *etime,
>> @@ -145,14 +138,9 @@ int radeon_mode_dumb_mmap(struct drm_file *filp,
>>   int radeon_mode_dumb_create(struct drm_file *file_priv,
>>                   struct drm_device *dev,
>>                   struct drm_mode_create_dumb *args);
>> -struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object
>> *obj);
>>   struct drm_gem_object *radeon_gem_prime_import_sg_table(struct
>> drm_device *dev,
>>                               struct dma_buf_attachment *,
>>                               struct sg_table *sg);
>> -int radeon_gem_prime_pin(struct drm_gem_object *obj);
>> -void radeon_gem_prime_unpin(struct drm_gem_object *obj);
>> -void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
>> -void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
>>     /* atpx handler */
>>   #if defined(CONFIG_VGA_SWITCHEROO)
>> @@ -550,7 +538,7 @@ long radeon_drm_ioctl(struct file *filp,
>>       }
>>         ret = drm_ioctl(filp, cmd, arg);
>> -   
>> +
>>       pm_runtime_mark_last_busy(dev->dev);
>>       pm_runtime_put_autosuspend(dev->dev);
>>       return ret;
>> @@ -609,22 +597,13 @@ static struct drm_driver kms_driver = {
>>       .irq_uninstall = radeon_driver_irq_uninstall_kms,
>>       .irq_handler = radeon_driver_irq_handler_kms,
>>       .ioctls = radeon_ioctls_kms,
>> -    .gem_free_object_unlocked = radeon_gem_object_free,
>> -    .gem_open_object = radeon_gem_object_open,
>> -    .gem_close_object = radeon_gem_object_close,
>>       .dumb_create = radeon_mode_dumb_create,
>>       .dumb_map_offset = radeon_mode_dumb_mmap,
>>       .fops = &radeon_driver_kms_fops,
>>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> -    .gem_prime_export = radeon_gem_prime_export,
>> -    .gem_prime_pin = radeon_gem_prime_pin,
>> -    .gem_prime_unpin = radeon_gem_prime_unpin,
>> -    .gem_prime_get_sg_table = radeon_gem_prime_get_sg_table,
>>       .gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
>> -    .gem_prime_vmap = radeon_gem_prime_vmap,
>> -    .gem_prime_vunmap = radeon_gem_prime_vunmap,
>>         .name = DRIVER_NAME,
>>       .desc = DRIVER_DESC,
>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c
>> b/drivers/gpu/drm/radeon/radeon_object.c
>> index bb7582afd803..882390e15dfe 100644
>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>> @@ -45,6 +45,19 @@ int radeon_ttm_init(struct radeon_device *rdev);
>>   void radeon_ttm_fini(struct radeon_device *rdev);
>>   static void radeon_bo_clear_surface_reg(struct radeon_bo *bo);
>>   +void radeon_gem_object_free(struct drm_gem_object *obj);
>> +int radeon_gem_object_open(struct drm_gem_object *obj,
>> +                struct drm_file *file_priv);
>> +void radeon_gem_object_close(struct drm_gem_object *obj,
>> +                struct drm_file *file_priv);
>> +struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
>> +                    int flags);
>> +struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object
>> *obj);
>> +int radeon_gem_prime_pin(struct drm_gem_object *obj);
>> +void radeon_gem_prime_unpin(struct drm_gem_object *obj);
>> +void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
>> +void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
>> +
>>   /*
>>    * To exclude mutual BO access we rely on bo_reserve exclusion, as all
>>    * function are calling it.
>> @@ -180,6 +193,18 @@ void radeon_ttm_placement_from_domain(struct
>> radeon_bo *rbo, u32 domain)
>>       }
>>   }
>>   +static const struct drm_gem_object_funcs radeon_gem_object_funcs = {
>> +    .free = radeon_gem_object_free,
>> +    .open = radeon_gem_object_open,
>> +    .close = radeon_gem_object_close,
>> +    .export = radeon_gem_prime_export,
>> +    .pin = radeon_gem_prime_pin,
>> +    .unpin = radeon_gem_prime_unpin,
>> +    .get_sg_table = radeon_gem_prime_get_sg_table,
>> +    .vmap = radeon_gem_prime_vmap,
>> +    .vunmap = radeon_gem_prime_vunmap,
>> +};
>> +
> 
> Same comment as for amdgpu, please put that into radeon_gem.c instead.

There's no good header file to put the declarations, right? I'm asking
because checkpatch warns about declarations in the source files.

Best regards
Thomas

> 
> Christian.
> 
>>   int radeon_bo_create(struct radeon_device *rdev,
>>                unsigned long size, int byte_align, bool kernel,
>>                u32 domain, u32 flags, struct sg_table *sg,
>> @@ -209,6 +234,7 @@ int radeon_bo_create(struct radeon_device *rdev,
>>       bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL);
>>       if (bo == NULL)
>>           return -ENOMEM;
>> +    bo->tbo.base.funcs = &radeon_gem_object_funcs;
>>       drm_gem_private_object_init(rdev->ddev, &bo->tbo.base, size);
>>       bo->rdev = rdev;
>>       bo->surface_reg = -1;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 12/20] drm/radeon: Introduce GEM object functions

[Christian König]

Am 13.08.20 um 12:41 schrieb Thomas Zimmermann:
> Hi
>
> Am 13.08.20 um 12:24 schrieb Christian König:
>> Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
>>> GEM object functions deprecate several similar callback interfaces in
>>> struct drm_driver. This patch replaces the per-driver callbacks with
>>> per-instance callbacks in radeon.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>    drivers/gpu/drm/radeon/radeon_drv.c    | 23 +----------------------
>>>    drivers/gpu/drm/radeon/radeon_object.c | 26 ++++++++++++++++++++++++++
>>>    2 files changed, 27 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
>>> b/drivers/gpu/drm/radeon/radeon_drv.c
>>> index 4cd30613fa1d..65061c949aee 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_drv.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
>>> @@ -124,13 +124,6 @@ void radeon_driver_irq_preinstall_kms(struct
>>> drm_device *dev);
>>>    int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
>>>    void radeon_driver_irq_uninstall_kms(struct drm_device *dev);
>>>    irqreturn_t radeon_driver_irq_handler_kms(int irq, void *arg);
>>> -void radeon_gem_object_free(struct drm_gem_object *obj);
>>> -int radeon_gem_object_open(struct drm_gem_object *obj,
>>> -                struct drm_file *file_priv);
>>> -void radeon_gem_object_close(struct drm_gem_object *obj,
>>> -                struct drm_file *file_priv);
>>> -struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
>>> -                    int flags);
>>>    extern int radeon_get_crtc_scanoutpos(struct drm_device *dev,
>>> unsigned int crtc,
>>>                          unsigned int flags, int *vpos, int *hpos,
>>>                          ktime_t *stime, ktime_t *etime,
>>> @@ -145,14 +138,9 @@ int radeon_mode_dumb_mmap(struct drm_file *filp,
>>>    int radeon_mode_dumb_create(struct drm_file *file_priv,
>>>                    struct drm_device *dev,
>>>                    struct drm_mode_create_dumb *args);
>>> -struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object
>>> *obj);
>>>    struct drm_gem_object *radeon_gem_prime_import_sg_table(struct
>>> drm_device *dev,
>>>                                struct dma_buf_attachment *,
>>>                                struct sg_table *sg);
>>> -int radeon_gem_prime_pin(struct drm_gem_object *obj);
>>> -void radeon_gem_prime_unpin(struct drm_gem_object *obj);
>>> -void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
>>> -void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
>>>      /* atpx handler */
>>>    #if defined(CONFIG_VGA_SWITCHEROO)
>>> @@ -550,7 +538,7 @@ long radeon_drm_ioctl(struct file *filp,
>>>        }
>>>          ret = drm_ioctl(filp, cmd, arg);
>>> -
>>> +
>>>        pm_runtime_mark_last_busy(dev->dev);
>>>        pm_runtime_put_autosuspend(dev->dev);
>>>        return ret;
>>> @@ -609,22 +597,13 @@ static struct drm_driver kms_driver = {
>>>        .irq_uninstall = radeon_driver_irq_uninstall_kms,
>>>        .irq_handler = radeon_driver_irq_handler_kms,
>>>        .ioctls = radeon_ioctls_kms,
>>> -    .gem_free_object_unlocked = radeon_gem_object_free,
>>> -    .gem_open_object = radeon_gem_object_open,
>>> -    .gem_close_object = radeon_gem_object_close,
>>>        .dumb_create = radeon_mode_dumb_create,
>>>        .dumb_map_offset = radeon_mode_dumb_mmap,
>>>        .fops = &radeon_driver_kms_fops,
>>>          .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>>        .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>> -    .gem_prime_export = radeon_gem_prime_export,
>>> -    .gem_prime_pin = radeon_gem_prime_pin,
>>> -    .gem_prime_unpin = radeon_gem_prime_unpin,
>>> -    .gem_prime_get_sg_table = radeon_gem_prime_get_sg_table,
>>>        .gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
>>> -    .gem_prime_vmap = radeon_gem_prime_vmap,
>>> -    .gem_prime_vunmap = radeon_gem_prime_vunmap,
>>>          .name = DRIVER_NAME,
>>>        .desc = DRIVER_DESC,
>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c
>>> b/drivers/gpu/drm/radeon/radeon_object.c
>>> index bb7582afd803..882390e15dfe 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>> @@ -45,6 +45,19 @@ int radeon_ttm_init(struct radeon_device *rdev);
>>>    void radeon_ttm_fini(struct radeon_device *rdev);
>>>    static void radeon_bo_clear_surface_reg(struct radeon_bo *bo);
>>>    +void radeon_gem_object_free(struct drm_gem_object *obj);
>>> +int radeon_gem_object_open(struct drm_gem_object *obj,
>>> +                struct drm_file *file_priv);
>>> +void radeon_gem_object_close(struct drm_gem_object *obj,
>>> +                struct drm_file *file_priv);
>>> +struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
>>> +                    int flags);
>>> +struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object
>>> *obj);
>>> +int radeon_gem_prime_pin(struct drm_gem_object *obj);
>>> +void radeon_gem_prime_unpin(struct drm_gem_object *obj);
>>> +void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
>>> +void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
>>> +
>>>    /*
>>>     * To exclude mutual BO access we rely on bo_reserve exclusion, as all
>>>     * function are calling it.
>>> @@ -180,6 +193,18 @@ void radeon_ttm_placement_from_domain(struct
>>> radeon_bo *rbo, u32 domain)
>>>        }
>>>    }
>>>    +static const struct drm_gem_object_funcs radeon_gem_object_funcs = {
>>> +    .free = radeon_gem_object_free,
>>> +    .open = radeon_gem_object_open,
>>> +    .close = radeon_gem_object_close,
>>> +    .export = radeon_gem_prime_export,
>>> +    .pin = radeon_gem_prime_pin,
>>> +    .unpin = radeon_gem_prime_unpin,
>>> +    .get_sg_table = radeon_gem_prime_get_sg_table,
>>> +    .vmap = radeon_gem_prime_vmap,
>>> +    .vunmap = radeon_gem_prime_vunmap,
>>> +};
>>> +
>> Same comment as for amdgpu, please put that into radeon_gem.c instead.
> There's no good header file to put the declarations, right? I'm asking
> because checkpatch warns about declarations in the source files.

Yes, that is kind of a mess because radeon used to support both KMS and UMS.

Thanks for cleaning this up,
Christian.

>
> Best regards
> Thomas
>
>> Christian.
>>
>>>    int radeon_bo_create(struct radeon_device *rdev,
>>>                 unsigned long size, int byte_align, bool kernel,
>>>                 u32 domain, u32 flags, struct sg_table *sg,
>>> @@ -209,6 +234,7 @@ int radeon_bo_create(struct radeon_device *rdev,
>>>        bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL);
>>>        if (bo == NULL)
>>>            return -ENOMEM;
>>> +    bo->tbo.base.funcs = &radeon_gem_object_funcs;
>>>        drm_gem_private_object_init(rdev->ddev, &bo->tbo.base, size);
>>>        bo->rdev = rdev;
>>>        bo->surface_reg = -1;


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 18/20] drm/xen: Introduce GEM object functions

[Oleksandr Andrushchenko]

Hi,

On 8/13/20 11:36 AM, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in xen. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/xen/xen_drm_front.c     | 12 +-----------
>   drivers/gpu/drm/xen/xen_drm_front.h     |  2 ++
>   drivers/gpu/drm/xen/xen_drm_front_gem.c | 15 +++++++++++++++
>   3 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
> index 3e660fb111b3..bd9af1875af1 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
> @@ -433,7 +433,7 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
>   	return ret;
>   }
>   
> -static void xen_drm_drv_free_object_unlocked(struct drm_gem_object *obj)
> +void xen_drm_drv_free_object_unlocked(struct drm_gem_object *obj)

Can we please have naming consistent and name it as

xen_drm_front_drv_free_object_unlocked or any other name if this seems to be too long,

but starting with xen_drm_front_ as the rest of exported functions?

With this,

Acked-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Thank you,

Oleksandr

>   {
>   	struct xen_drm_front_drm_info *drm_info = obj->dev->dev_private;
>   	int idx;
> @@ -481,22 +481,12 @@ static const struct file_operations xen_drm_dev_fops = {
>   	.mmap           = xen_drm_front_gem_mmap,
>   };
>   
> -static const struct vm_operations_struct xen_drm_drv_vm_ops = {
> -	.open           = drm_gem_vm_open,
> -	.close          = drm_gem_vm_close,
> -};
> -
>   static struct drm_driver xen_drm_driver = {
>   	.driver_features           = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>   	.release                   = xen_drm_drv_release,
> -	.gem_vm_ops                = &xen_drm_drv_vm_ops,
> -	.gem_free_object_unlocked  = xen_drm_drv_free_object_unlocked,
>   	.prime_handle_to_fd        = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle        = drm_gem_prime_fd_to_handle,
>   	.gem_prime_import_sg_table = xen_drm_front_gem_import_sg_table,
> -	.gem_prime_get_sg_table    = xen_drm_front_gem_get_sg_table,
> -	.gem_prime_vmap            = xen_drm_front_gem_prime_vmap,
> -	.gem_prime_vunmap          = xen_drm_front_gem_prime_vunmap,
>   	.gem_prime_mmap            = xen_drm_front_gem_prime_mmap,
>   	.dumb_create               = xen_drm_drv_dumb_create,
>   	.fops                      = &xen_drm_dev_fops,
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.h b/drivers/gpu/drm/xen/xen_drm_front.h
> index f92c258350ca..93e60c1db550 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.h
> +++ b/drivers/gpu/drm/xen/xen_drm_front.h
> @@ -160,4 +160,6 @@ int xen_drm_front_page_flip(struct xen_drm_front_info *front_info,
>   void xen_drm_front_on_frame_done(struct xen_drm_front_info *front_info,
>   				 int conn_idx, u64 fb_cookie);
>   
> +void xen_drm_drv_free_object_unlocked(struct drm_gem_object *obj);
> +
>   #endif /* __XEN_DRM_FRONT_H_ */
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
> index f0b85e094111..7b315c08bcfc 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
> @@ -56,6 +56,19 @@ static void gem_free_pages_array(struct xen_gem_object *xen_obj)
>   	xen_obj->pages = NULL;
>   }
>   
> +static const struct vm_operations_struct xen_drm_drv_vm_ops = {
> +	.open           = drm_gem_vm_open,
> +	.close          = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs xen_drm_front_gem_object_funcs = {
> +	.free = xen_drm_drv_free_object_unlocked,
> +	.get_sg_table = xen_drm_front_gem_get_sg_table,
> +	.vmap = xen_drm_front_gem_prime_vmap,
> +	.vunmap = xen_drm_front_gem_prime_vunmap,
> +	.vm_ops = &xen_drm_drv_vm_ops,
> +};
> +
>   static struct xen_gem_object *gem_create_obj(struct drm_device *dev,
>   					     size_t size)
>   {
> @@ -66,6 +79,8 @@ static struct xen_gem_object *gem_create_obj(struct drm_device *dev,
>   	if (!xen_obj)
>   		return ERR_PTR(-ENOMEM);
>   
> +	xen_obj->base.funcs = &xen_drm_front_gem_object_funcs;
> +
>   	ret = drm_gem_object_init(dev, &xen_obj->base, size);
>   	if (ret < 0) {
>   		kfree(xen_obj);
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 18/20] drm/xen: Introduce GEM object functions

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5109 bytes --]

Hi

Am 13.08.20 um 13:19 schrieb Oleksandr Andrushchenko:
> Hi,
> 
> On 8/13/20 11:36 AM, Thomas Zimmermann wrote:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in xen. The only exception is gem_prime_mmap,
>> which is non-trivial to convert.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/xen/xen_drm_front.c     | 12 +-----------
>>   drivers/gpu/drm/xen/xen_drm_front.h     |  2 ++
>>   drivers/gpu/drm/xen/xen_drm_front_gem.c | 15 +++++++++++++++
>>   3 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
>> index 3e660fb111b3..bd9af1875af1 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
>> @@ -433,7 +433,7 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
>>   	return ret;
>>   }
>>   
>> -static void xen_drm_drv_free_object_unlocked(struct drm_gem_object *obj)
>> +void xen_drm_drv_free_object_unlocked(struct drm_gem_object *obj)
> 
> Can we please have naming consistent and name it as
> 
> xen_drm_front_drv_free_object_unlocked or any other name if this seems to be too long,
> 
> but starting with xen_drm_front_ as the rest of exported functions?

There already is a function with that name in drm_xen_front_gem.c. I'll
move the callback function next to the object-function structure and
rename it slightly.

Best regards
Thomas

> 
> With this,
> 
> Acked-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Thank you,
> 
> Oleksandr
> 
>>   {
>>   	struct xen_drm_front_drm_info *drm_info = obj->dev->dev_private;
>>   	int idx;
>> @@ -481,22 +481,12 @@ static const struct file_operations xen_drm_dev_fops = {
>>   	.mmap           = xen_drm_front_gem_mmap,
>>   };
>>   
>> -static const struct vm_operations_struct xen_drm_drv_vm_ops = {
>> -	.open           = drm_gem_vm_open,
>> -	.close          = drm_gem_vm_close,
>> -};
>> -
>>   static struct drm_driver xen_drm_driver = {
>>   	.driver_features           = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>>   	.release                   = xen_drm_drv_release,
>> -	.gem_vm_ops                = &xen_drm_drv_vm_ops,
>> -	.gem_free_object_unlocked  = xen_drm_drv_free_object_unlocked,
>>   	.prime_handle_to_fd        = drm_gem_prime_handle_to_fd,
>>   	.prime_fd_to_handle        = drm_gem_prime_fd_to_handle,
>>   	.gem_prime_import_sg_table = xen_drm_front_gem_import_sg_table,
>> -	.gem_prime_get_sg_table    = xen_drm_front_gem_get_sg_table,
>> -	.gem_prime_vmap            = xen_drm_front_gem_prime_vmap,
>> -	.gem_prime_vunmap          = xen_drm_front_gem_prime_vunmap,
>>   	.gem_prime_mmap            = xen_drm_front_gem_prime_mmap,
>>   	.dumb_create               = xen_drm_drv_dumb_create,
>>   	.fops                      = &xen_drm_dev_fops,
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front.h b/drivers/gpu/drm/xen/xen_drm_front.h
>> index f92c258350ca..93e60c1db550 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front.h
>> +++ b/drivers/gpu/drm/xen/xen_drm_front.h
>> @@ -160,4 +160,6 @@ int xen_drm_front_page_flip(struct xen_drm_front_info *front_info,
>>   void xen_drm_front_on_frame_done(struct xen_drm_front_info *front_info,
>>   				 int conn_idx, u64 fb_cookie);
>>   
>> +void xen_drm_drv_free_object_unlocked(struct drm_gem_object *obj);
>> +
>>   #endif /* __XEN_DRM_FRONT_H_ */
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> index f0b85e094111..7b315c08bcfc 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> @@ -56,6 +56,19 @@ static void gem_free_pages_array(struct xen_gem_object *xen_obj)
>>   	xen_obj->pages = NULL;
>>   }
>>   
>> +static const struct vm_operations_struct xen_drm_drv_vm_ops = {
>> +	.open           = drm_gem_vm_open,
>> +	.close          = drm_gem_vm_close,
>> +};
>> +
>> +static const struct drm_gem_object_funcs xen_drm_front_gem_object_funcs = {
>> +	.free = xen_drm_drv_free_object_unlocked,
>> +	.get_sg_table = xen_drm_front_gem_get_sg_table,
>> +	.vmap = xen_drm_front_gem_prime_vmap,
>> +	.vunmap = xen_drm_front_gem_prime_vunmap,
>> +	.vm_ops = &xen_drm_drv_vm_ops,
>> +};
>> +
>>   static struct xen_gem_object *gem_create_obj(struct drm_device *dev,
>>   					     size_t size)
>>   {
>> @@ -66,6 +79,8 @@ static struct xen_gem_object *gem_create_obj(struct drm_device *dev,
>>   	if (!xen_obj)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> +	xen_obj->base.funcs = &xen_drm_front_gem_object_funcs;
>> +
>>   	ret = drm_gem_object_init(dev, &xen_obj->base, size);
>>   	if (ret < 0) {
>>   		kfree(xen_obj);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 19/20] drm/xlnx: Initialize DRM driver instance with CMA helper macro

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Thu, Aug 13, 2020 at 10:36:43AM +0200, Thomas Zimmermann wrote:
> The xlnx driver uses CMA helpers with default callback functions.
> Initialize the driver structure with the rsp CMA helper macro. The
> driver is being converted to use GEM object functions as part of
> this change.
> 
> Two callbacks, .dumb_destroy and .gem_prime_import, were initialized
> to their default implementations, so they are just kept empty now.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 26328c76305b..058044dcc062 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -80,19 +80,7 @@ static struct drm_driver zynqmp_dpsub_drm_driver = {
>  	.driver_features		= DRIVER_MODESET | DRIVER_GEM |
>  					  DRIVER_ATOMIC,
>  
> -	.prime_handle_to_fd		= drm_gem_prime_handle_to_fd,
> -	.prime_fd_to_handle		= drm_gem_prime_fd_to_handle,
> -	.gem_prime_export		= drm_gem_prime_export,
> -	.gem_prime_import		= drm_gem_prime_import,
> -	.gem_prime_get_sg_table		= drm_gem_cma_prime_get_sg_table,
> -	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table,
> -	.gem_prime_vmap			= drm_gem_cma_prime_vmap,
> -	.gem_prime_vunmap		= drm_gem_cma_prime_vunmap,
> -	.gem_prime_mmap			= drm_gem_cma_prime_mmap,
> -	.gem_free_object_unlocked	= drm_gem_cma_free_object,
> -	.gem_vm_ops			= &drm_gem_cma_vm_ops,
> -	.dumb_create			= zynqmp_dpsub_dumb_create,
> -	.dumb_destroy			= drm_gem_dumb_destroy,
> +	DRM_GEM_CMA_DRIVER_OPS_VMAP_WITH_DUMB_CREATE(zynqmp_dpsub_dumb_create),

The only effective change here is

-	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table,
-	.gem_prime_mmap			= drm_gem_cma_prime_mmap,
+	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table_vmap,
+	.gem_prime_mmap			= drm_gem_prime_mmap,

The change is significant, and I have a hard time following the code to
verify that it's correct, or if it's an undesired side effect. If it's
correct, could the change be mentioned in the commit message, with at
least a brief explanation of why this is correct, and what the
consequences here ?

>  
>  	.fops				= &zynqmp_dpsub_drm_fops,
>  

-- 
Regards,

Laurent Pinchart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 19/20] drm/xlnx: Initialize DRM driver instance with CMA helper macro

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3004 bytes --]

Hi

Am 13.08.20 um 15:36 schrieb Laurent Pinchart:
> Hi Thomas,
> 
> Thank you for the patch.
> 
> On Thu, Aug 13, 2020 at 10:36:43AM +0200, Thomas Zimmermann wrote:
>> The xlnx driver uses CMA helpers with default callback functions.
>> Initialize the driver structure with the rsp CMA helper macro. The
>> driver is being converted to use GEM object functions as part of
>> this change.
>>
>> Two callbacks, .dumb_destroy and .gem_prime_import, were initialized
>> to their default implementations, so they are just kept empty now.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +-------------
>>  1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> index 26328c76305b..058044dcc062 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> @@ -80,19 +80,7 @@ static struct drm_driver zynqmp_dpsub_drm_driver = {
>>  	.driver_features		= DRIVER_MODESET | DRIVER_GEM |
>>  					  DRIVER_ATOMIC,
>>  
>> -	.prime_handle_to_fd		= drm_gem_prime_handle_to_fd,
>> -	.prime_fd_to_handle		= drm_gem_prime_fd_to_handle,
>> -	.gem_prime_export		= drm_gem_prime_export,
>> -	.gem_prime_import		= drm_gem_prime_import,
>> -	.gem_prime_get_sg_table		= drm_gem_cma_prime_get_sg_table,
>> -	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table,
>> -	.gem_prime_vmap			= drm_gem_cma_prime_vmap,
>> -	.gem_prime_vunmap		= drm_gem_cma_prime_vunmap,
>> -	.gem_prime_mmap			= drm_gem_cma_prime_mmap,
>> -	.gem_free_object_unlocked	= drm_gem_cma_free_object,
>> -	.gem_vm_ops			= &drm_gem_cma_vm_ops,
>> -	.dumb_create			= zynqmp_dpsub_dumb_create,
>> -	.dumb_destroy			= drm_gem_dumb_destroy,
>> +	DRM_GEM_CMA_DRIVER_OPS_VMAP_WITH_DUMB_CREATE(zynqmp_dpsub_dumb_create),
> 
> The only effective change here is
> 
> -	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table,
> -	.gem_prime_mmap			= drm_gem_cma_prime_mmap,
> +	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table_vmap,
> +	.gem_prime_mmap			= drm_gem_prime_mmap,
> 
> The change is significant, and I have a hard time following the code to
> verify that it's correct, or if it's an undesired side effect. If it's
> correct, could the change be mentioned in the commit message, with at
> least a brief explanation of why this is correct, and what the
> consequences here ?

I think this is a mistake that I didn't notice. Thanks for spotting it.
Initializing the driver structure
DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE should fix it. I'll change this
for the patch's next revision.

Best regards
Thomas

> 
>>  
>>  	.fops				= &zynqmp_dpsub_drm_fops,
>>  
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 10/20] drm/omapdrm: Introduce GEM object functions

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Thu, Aug 13, 2020 at 10:36:34AM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in omapdrm.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c |  9 ---------
>  drivers/gpu/drm/omapdrm/omap_gem.c | 16 +++++++++++++++-
>  drivers/gpu/drm/omapdrm/omap_gem.h |  1 -
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 53d5e184ee77..2e598b8b72af 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -521,12 +521,6 @@ static int dev_open(struct drm_device *dev, struct drm_file *file)
>  	return 0;
>  }
>  
> -static const struct vm_operations_struct omap_gem_vm_ops = {
> -	.fault = omap_gem_fault,
> -	.open = drm_gem_vm_open,
> -	.close = drm_gem_vm_close,
> -};
> -
>  static const struct file_operations omapdriver_fops = {
>  	.owner = THIS_MODULE,
>  	.open = drm_open,
> @@ -549,10 +543,7 @@ static struct drm_driver omap_drm_driver = {
>  #endif
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> -	.gem_prime_export = omap_gem_prime_export,
>  	.gem_prime_import = omap_gem_prime_import,
> -	.gem_free_object_unlocked = omap_gem_free_object,
> -	.gem_vm_ops = &omap_gem_vm_ops,
>  	.dumb_create = omap_gem_dumb_create,
>  	.dumb_map_offset = omap_gem_dumb_map_offset,
>  	.ioctls = ioctls,
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d0d12d5dd76c..d68dc63dea0a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -487,7 +487,7 @@ static vm_fault_t omap_gem_fault_2d(struct drm_gem_object *obj,
>   * vma->vm_private_data points to the GEM object that is backing this
>   * mapping.
>   */
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t omap_gem_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct drm_gem_object *obj = vma->vm_private_data;
> @@ -1169,6 +1169,18 @@ static bool omap_gem_validate_flags(struct drm_device *dev, u32 flags)
>  	return true;
>  }
>  
> +static const struct vm_operations_struct omap_gem_vm_ops = {
> +	.fault = omap_gem_fault,
> +	.open = drm_gem_vm_open,
> +	.close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs omap_gem_object_funcs = {
> +	.free = omap_gem_free_object,
> +	.export = omap_gem_prime_export,
> +	.vm_ops = &omap_gem_vm_ops,
> +};
> +
>  /* GEM buffer object constructor */
>  struct drm_gem_object *omap_gem_new(struct drm_device *dev,
>  		union omap_gem_size gsize, u32 flags)
> @@ -1236,6 +1248,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
>  		size = PAGE_ALIGN(gsize.bytes);
>  	}
>  
> +	obj->funcs = &omap_gem_object_funcs;
> +
>  	/* Initialize the GEM object. */
>  	if (!(flags & OMAP_BO_MEM_SHMEM)) {
>  		drm_gem_private_object_init(dev, obj, size);
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.h b/drivers/gpu/drm/omapdrm/omap_gem.h
> index 729b7812a815..9e6b5c8195d9 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.h
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.h
> @@ -69,7 +69,6 @@ struct dma_buf *omap_gem_prime_export(struct drm_gem_object *obj, int flags);
>  struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>  		struct dma_buf *buffer);
>  
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf);

I like how this function now becomes internal to omap_gem.c.

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

>  int omap_gem_roll(struct drm_gem_object *obj, u32 roll);
>  void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff);
>  void omap_gem_dma_sync_buffer(struct drm_gem_object *obj,

-- 
Regards,

Laurent Pinchart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 10/20] drm/omapdrm: Introduce GEM object functions

[Tomi Valkeinen]

Hi,

On 13/08/2020 11:36, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in omapdrm.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c |  9 ---------
>  drivers/gpu/drm/omapdrm/omap_gem.c | 16 +++++++++++++++-
>  drivers/gpu/drm/omapdrm/omap_gem.h |  1 -
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 53d5e184ee77..2e598b8b72af 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -521,12 +521,6 @@ static int dev_open(struct drm_device *dev, struct drm_file *file)
>  	return 0;
>  }
>  
> -static const struct vm_operations_struct omap_gem_vm_ops = {
> -	.fault = omap_gem_fault,
> -	.open = drm_gem_vm_open,
> -	.close = drm_gem_vm_close,
> -};
> -
>  static const struct file_operations omapdriver_fops = {
>  	.owner = THIS_MODULE,
>  	.open = drm_open,
> @@ -549,10 +543,7 @@ static struct drm_driver omap_drm_driver = {
>  #endif
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> -	.gem_prime_export = omap_gem_prime_export,
>  	.gem_prime_import = omap_gem_prime_import,
> -	.gem_free_object_unlocked = omap_gem_free_object,
> -	.gem_vm_ops = &omap_gem_vm_ops,
>  	.dumb_create = omap_gem_dumb_create,
>  	.dumb_map_offset = omap_gem_dumb_map_offset,
>  	.ioctls = ioctls,
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d0d12d5dd76c..d68dc63dea0a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -487,7 +487,7 @@ static vm_fault_t omap_gem_fault_2d(struct drm_gem_object *obj,
>   * vma->vm_private_data points to the GEM object that is backing this
>   * mapping.
>   */
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t omap_gem_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct drm_gem_object *obj = vma->vm_private_data;
> @@ -1169,6 +1169,18 @@ static bool omap_gem_validate_flags(struct drm_device *dev, u32 flags)
>  	return true;
>  }
>  
> +static const struct vm_operations_struct omap_gem_vm_ops = {
> +	.fault = omap_gem_fault,
> +	.open = drm_gem_vm_open,
> +	.close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs omap_gem_object_funcs = {
> +	.free = omap_gem_free_object,
> +	.export = omap_gem_prime_export,
> +	.vm_ops = &omap_gem_vm_ops,
> +};
> +
>  /* GEM buffer object constructor */
>  struct drm_gem_object *omap_gem_new(struct drm_device *dev,
>  		union omap_gem_size gsize, u32 flags)
> @@ -1236,6 +1248,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
>  		size = PAGE_ALIGN(gsize.bytes);
>  	}
>  
> +	obj->funcs = &omap_gem_object_funcs;
> +
>  	/* Initialize the GEM object. */
>  	if (!(flags & OMAP_BO_MEM_SHMEM)) {
>  		drm_gem_private_object_init(dev, obj, size);
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.h b/drivers/gpu/drm/omapdrm/omap_gem.h
> index 729b7812a815..9e6b5c8195d9 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.h
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.h
> @@ -69,7 +69,6 @@ struct dma_buf *omap_gem_prime_export(struct drm_gem_object *obj, int flags);
>  struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>  		struct dma_buf *buffer);
>  
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf);
>  int omap_gem_roll(struct drm_gem_object *obj, u32 roll);
>  void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff);
>  void omap_gem_dma_sync_buffer(struct drm_gem_object *obj,

omap_gem_free_object() can also be made static, and removed from omap_gem.h.

Tested on AM5 EVM.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip