Re: [PATCH 07/12] drm/amdgpu: implement cgs gpu memory callbacks

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 29, 2015 at 11:41:56AM -0400, Alex Deucher wrote:
> On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Sep 29, 2015 at 02:39:49PM +0200, Christian König wrote:
> >> On 29.09.2015 13:40, Daniel Vetter wrote:
> >> >On Thu, Jul 09, 2015 at 12:21:06PM -0400, Alex Deucher wrote:
> >> >>From: Chunming Zhou <david1.zhou@amd.com>
> >> >>
> >> >>This implements the cgs interface for allocating
> >> >>GPU memory.
> >> >>
> >> >>Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
> >> >I don't see that review anywhere on a m-l ... where is it?
> >>
> >> Jammy reviewed the stuff internally before we made it public, that's why you
> >> can't find it.
> >>
> >> >
> >> >I stumbled over this patch because it adds a new dev->struct_mutex user
> >> >(that should be a big warning sign) and then I somehow unrolled some giant
> >> >chain of wtfs. Either I'm completely missing what this is used for or it
> >> >probably should get reworked asap.
> >>
> >> The CGS functions aren't used at the moment because none of the components
> >> depending on them made it into the kernel, but we wanted to keep it so that
> >> we can get reviews on the general idea and if necessary rework it.
> >>
> >> In general it's an abstraction layer of device driver dependent functions
> >> which enables us to share more code internally.
> >>
> >> We worked quite hard on trying to avoid the OS abstraction trap with this,
> >> but if you think this still won't work feel free to object.
> >
> > The bit that made me look really is the import_gpu_mem thing, which seems
> > to partially reinvent drm_prime.c. Given how tricky importing and
> > import-caching is I'd really like to see that gone (and Alex said on irc
> > he'd be ok with that).
> >
> 
> See attached patch.  It was really only added for completeness.  We
> don't have any users of it at the moment.  If we end up needing the
> functionality in the future we can revisit it.
> 
> > The other stuff seems a lot more benign. For the irq abstraction
> > specifically it might be worth looking at the irq_chip stuff linux core
> > has, which is what's used to virtualize/abstract irq routing and handling.
> > But for that stuff it's a balance thing really how much you reinvent
> > wheels internally in the driver (e.g. i915 has it's own power_well stuff
> > which is pretty much just powerdomains reinvented, with less features).
> >
> 
> I think that's one of the hardest things in the kernel: finding out if
> a solution already exists or not.  We implemented our own version of
> mfd for our ACP audio block driver.  Upon upsteaming we were alerted
> to mfd's existence and we converted the driver to use mfd.  At the end
> of the day it was a lot of work for minimal gain, at least from a
> functionality perspective.  I wish we had known about it sooner.  I'll
> take a look at the irq_chip stuff.  Thanks for the heads up!
> 
> Alex
> 
> > But really I can't tell without the users whether I'd expect this to be
> > hurt longterm or not for you ;-) But the import stuff is hurt for me since
> > you noodle around in drm internals. And specifically I'd like to make
> > modern drivers completely struct_mutex free with the goal to untangle the
> > last hold-outs of that lock in the drm core.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

> From 013170490a934731bb5fbc4cb8ee46421d2f240e Mon Sep 17 00:00:00 2001
> From: Alex Deucher <alexander.deucher@amd.com>
> Date: Tue, 29 Sep 2015 10:35:45 -0400
> Subject: [PATCH] drm/amdgpu/cgs: remove import_gpu_mem
> 
> It was added for completeness, but we don't have any users
> for it yet.  Daniel Vetter noted that it may be racy. Remove
> it.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 39 ---------------------------------
>  drivers/gpu/drm/amd/include/cgs_linux.h | 17 --------------
>  2 files changed, 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> index 25be402..7949927 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> @@ -208,44 +208,6 @@ static int amdgpu_cgs_alloc_gpu_mem(void *cgs_device,
>  	return ret;
>  }
>  
> -static int amdgpu_cgs_import_gpu_mem(void *cgs_device, int dmabuf_fd,
> -				     cgs_handle_t *handle)
> -{
> -	CGS_FUNC_ADEV;
> -	int r;
> -	uint32_t dma_handle;
> -	struct drm_gem_object *obj;
> -	struct amdgpu_bo *bo;
> -	struct drm_device *dev = adev->ddev;
> -	struct drm_file *file_priv = NULL, *priv;
> -
> -	mutex_lock(&dev->struct_mutex);
> -	list_for_each_entry(priv, &dev->filelist, lhead) {
> -		rcu_read_lock();
> -		if (priv->pid == get_pid(task_pid(current)))
> -			file_priv = priv;
> -		rcu_read_unlock();
> -		if (file_priv)
> -			break;
> -	}
> -	mutex_unlock(&dev->struct_mutex);
> -	r = dev->driver->prime_fd_to_handle(dev,
> -					    file_priv, dmabuf_fd,
> -					    &dma_handle);
> -	spin_lock(&file_priv->table_lock);
> -
> -	/* Check if we currently have a reference on the object */
> -	obj = idr_find(&file_priv->object_idr, dma_handle);
> -	if (obj == NULL) {
> -		spin_unlock(&file_priv->table_lock);
> -		return -EINVAL;
> -	}
> -	spin_unlock(&file_priv->table_lock);
> -	bo = gem_to_amdgpu_bo(obj);
> -	*handle = (cgs_handle_t)bo;
> -	return 0;
> -}
> -
>  static int amdgpu_cgs_free_gpu_mem(void *cgs_device, cgs_handle_t handle)
>  {
>  	struct amdgpu_bo *obj = (struct amdgpu_bo *)handle;
> @@ -846,7 +808,6 @@ static const struct cgs_ops amdgpu_cgs_ops = {
>  };
>  
>  static const struct cgs_os_ops amdgpu_cgs_os_ops = {
> -	amdgpu_cgs_import_gpu_mem,
>  	amdgpu_cgs_add_irq_source,
>  	amdgpu_cgs_irq_get,
>  	amdgpu_cgs_irq_put
> diff --git a/drivers/gpu/drm/amd/include/cgs_linux.h b/drivers/gpu/drm/amd/include/cgs_linux.h
> index 488642f..3b47ae3 100644
> --- a/drivers/gpu/drm/amd/include/cgs_linux.h
> +++ b/drivers/gpu/drm/amd/include/cgs_linux.h
> @@ -27,19 +27,6 @@
>  #include "cgs_common.h"
>  
>  /**
> - * cgs_import_gpu_mem() - Import dmabuf handle
> - * @cgs_device:  opaque device handle
> - * @dmabuf_fd:   DMABuf file descriptor
> - * @handle:      memory handle (output)
> - *
> - * Must be called in the process context that dmabuf_fd belongs to.
> - *
> - * Return:  0 on success, -errno otherwise
> - */
> -typedef int (*cgs_import_gpu_mem_t)(void *cgs_device, int dmabuf_fd,
> -				    cgs_handle_t *handle);
> -
> -/**
>   * cgs_irq_source_set_func() - Callback for enabling/disabling interrupt sources
>   * @private_data:  private data provided to cgs_add_irq_source
>   * @src_id:        interrupt source ID
> @@ -114,16 +101,12 @@ typedef int (*cgs_irq_get_t)(void *cgs_device, unsigned src_id, unsigned type);
>  typedef int (*cgs_irq_put_t)(void *cgs_device, unsigned src_id, unsigned type);
>  
>  struct cgs_os_ops {
> -	cgs_import_gpu_mem_t import_gpu_mem;
> -
>  	/* IRQ handling */
>  	cgs_add_irq_source_t add_irq_source;
>  	cgs_irq_get_t irq_get;
>  	cgs_irq_put_t irq_put;
>  };
>  
> -#define cgs_import_gpu_mem(dev,dmabuf_fd,handle)		\
> -	CGS_OS_CALL(import_gpu_mem,dev,dmabuf_fd,handle)
>  #define cgs_add_irq_source(dev,src_id,num_types,set,handler,private_data) \
>  	CGS_OS_CALL(add_irq_source,dev,src_id,num_types,set,handler,	\
>  		    private_data)
> -- 
> 1.8.3.1
> 


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel