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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Christian König" <deathsimple@vodafone.de>
Cc: Alex Deucher <alexander.deucher@amd.com>,
	Maruthi.Bayyavarapu@amd.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 07/12] drm/amdgpu: implement cgs gpu memory callbacks
Date: Tue, 29 Sep 2015 17:20:09 +0200	[thread overview]
Message-ID: <20150929152009.GO3383@phenom.ffwll.local> (raw)
In-Reply-To: <560A8695.3070506@vodafone.de>

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

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

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel