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

[Alex Deucher]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-drm-amdgpu-cgs-remove-import_gpu_mem.patch
Type: text/x-patch
Size: 3760 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150929/1e0f0d74/attachment-0001.bin>