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 at 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 at amd.com>
> >> >>
> >> >>This implements the cgs interface for allocating
> >> >>GPU memory.
> >> >>
> >> >>Reviewed-by: Jammy Zhou <Jammy.Zhou at 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 at 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 at amd.com>
Reviewed-by: Daniel Vetter <daniel.vetter at 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
