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

[Dave Airlie]

On 30 September 2015 at 01:41, Alex Deucher <alexdeucher at gmail.com> 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.

Can we get these reviews done publically? it's kinda hard to know how
well someone
reviewed things if we have no external copy. Like did Jammy a) read
the patch, and
slap Reviewed-by on it, or did he b) comment on some whitespace issues
and slap R-b
on it, or c) did he suggest a bunch of changes and those changes were
made and a new
version was produced and he r-b'ed that etc.

>> 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!

You say for minimal gain, but this is pretty much going to keep happening
to you with the development model you have chosen, get used to rewriting
things you consider finished and reviewed. I've said it before so I'll use this
to reiterate, your patches are only starting the process when you post them,
all the internal stuff you do is nice and all but it could all be done
externally
if you guys weren't so stuck on internal IP review. Otherwise you
should be taking
into account that this overhead will continue to exist in all your development,
and adjust schedules to suit.

Dave.