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

[Christian König]

On 30.09.2015 08:54, Daniel Vetter wrote:
> On Tue, Sep 29, 2015 at 04:28:13PM -0400, Alex Deucher wrote:
>> On Tue, Sep 29, 2015 at 4:10 PM, Dave Airlie  wrote:
>>> On 30 September 2015 at 01:41, Alex Deucher  
>>> wrote:
 On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 

 This implements the cgs interface for allocating
 GPU memory.

 Reviewed-by: Jammy Zhou 
>>> 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.
>>>
>> We are pushing for that and making steady progress, but things are
>> slow to move internally.
>>
> 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.
>> We do take that into account as evidenced by the multiple revisions of
>> the ACP patch set for example.  We know there may be a delta between
>> short term deliverables and what eventually goes upstream and we take
>> that into account.  That doesn't change the overall amount of work
>> involved.  The fact is we didn't know about mfd so we didn't use it.
>> I don't see how we could have avoided rewriting it if we didn't know
>> about it in the first place.  When we sent the patches out, we found
>> out about it and made the appropriate changes.  My point was just that
>> we aren't the only ones this happens to.
> Discussing early designs on irc helps a lot with that. But ime irc is one
> step further away from just dragging engineers onto the mailing list, and
> discussing new stuff on irc before patches get written instead of just
> review is one step more. That still leaves you with the problem with
> knowing whom to talk to, but for modularization we have at least Thierry
> Reding and Laurent Pinchart with a lot of soc experience outside of drm,
> they tend to know what's out there.

Yeah, completely agree.

Alex and I have spend a lot of time and effort with developers 
previously working on closed source code to enable them to contribute to 
the different open source projects.

Anybody who did something like that before knows that it certainly needs 
time for certain concepts to sink in. Especially thinks like stable 
interface and certain design criteria seem to be hard to get accepted.

So guys feel to criticize the code whenever you think it make sense and 
please do so as early as possible, e.g. on the first round of patches 
not after ten revisions.

We in turn try to get the code out as soon as possible, without keeping 
it stuck internally for too long.

Christian.

> -Daniel

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

[Daniel Vetter]

On Tue, Sep 29, 2015 at 11:41:56AM -0400, Alex Deucher wrote:
> On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 
> >> >>
> >> >>This implements the cgs interface for allocating
> >> >>GPU memory.
> >> >>
> >> >>Reviewed-by: Jammy Zhou 
> >> >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 
> 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 

Reviewed-by: Daniel Vetter 

> ---
>  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(>struct_mutex);
> - list_for_each_entry(priv, >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(>struct_mutex);
> - r = 

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

[Daniel Vetter]

On Tue, Sep 29, 2015 at 04:28:13PM -0400, Alex Deucher wrote:
> On Tue, Sep 29, 2015 at 4:10 PM, Dave Airlie  wrote:
> > On 30 September 2015 at 01:41, Alex Deucher  
> > wrote:
> >> On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 
>  >>
>  >>This implements the cgs interface for allocating
>  >>GPU memory.
>  >>
>  >>Reviewed-by: Jammy Zhou 
>  >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.
> >
> 
> We are pushing for that and making steady progress, but things are
> slow to move internally.
> 
> >>> 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.
> 
> We do take that into account as evidenced by the multiple revisions of
> the ACP patch set for example.  We know there may be a delta between
> short term deliverables and what eventually goes upstream and we take
> that into account.  That doesn't change the overall amount of work
> involved.  The fact is we didn't know about mfd so we didn't use it.
> I don't see how we could have avoided rewriting it if we didn't know
> about it in the first place.  When we sent the patches out, we found
> out about it and made the appropriate changes.  My point was just that
> we aren't the only ones this happens to.

Discussing early designs on irc helps a lot with that. But ime irc is one
step further away from just dragging engineers onto the mailing list, and
discussing new stuff on irc before patches get written instead of just
review is one step more. That still leaves you with the problem with
knowing whom to talk to, but for modularization we have at least Thierry
Reding and Laurent Pinchart with a lot of soc experience outside of drm,
they tend to know what's out there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

On Wed, Sep 30, 2015 at 06:10:52AM +1000, Dave Airlie wrote:
> On 30 September 2015 at 01:41, Alex Deucher  wrote:
> > On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 
> >>> >>
> >>> >>This implements the cgs interface for allocating
> >>> >>GPU memory.
> >>> >>
> >>> >>Reviewed-by: Jammy Zhou 
> >>> >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.

Yeah review in public is great. We can do that for almost everythingn
since nowadays we have blanket approval for everything, except a short
list of marketing relevant new features on new platforms. Occasionally the
public review with mails don't work out though and then we have a big
meeting, but even for those we try to summarize what we discussed and
decided on the m-l.

But without blanket approval this won't work I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Dave Airlie]

On 30 September 2015 at 01:41, Alex Deucher  wrote:
> On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 
>>> >>
>>> >>This implements the cgs interface for allocating
>>> >>GPU memory.
>>> >>
>>> >>Reviewed-by: Jammy Zhou 
>>> >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.

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

[Daniel Vetter]

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 
> >>
> >>This implements the cgs interface for allocating
> >>GPU memory.
> >>
> >>Reviewed-by: Jammy Zhou 
> >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

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

[Alex Deucher]

On Tue, Sep 29, 2015 at 4:10 PM, Dave Airlie  wrote:
> On 30 September 2015 at 01:41, Alex Deucher  wrote:
>> On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 
 >>
 >>This implements the cgs interface for allocating
 >>GPU memory.
 >>
 >>Reviewed-by: Jammy Zhou 
 >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.
>

We are pushing for that and making steady progress, but things are
slow to move internally.

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

We do take that into account as evidenced by the multiple revisions of
the ACP patch set for example.  We know there may be a delta between
short term deliverables and what eventually goes upstream and we take
that into account.  That doesn't change the overall amount of work
involved.  The fact is we didn't know about mfd so we didn't use it.
I don't see how we could have avoided rewriting it if we didn't know
about it in the first place.  When we sent the patches out, we found
out about it and made the appropriate changes.  My point was just that
we aren't the only ones this happens to.

Alex

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

[Christian König]

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 
>>
>> This implements the cgs interface for allocating
>> GPU memory.
>>
>> Reviewed-by: Jammy Zhou 
> 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.

Regards,
Christian.

> -Daniel
>
>> Signed-off-by: Chunming Zhou 
>> Signed-off-by: Alex Deucher 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 238 
>> ++--
>>   1 file changed, 226 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
>> index c1ee39e..ac0f124 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
>> @@ -21,7 +21,11 @@
>>*
>>*
>>*/
>> +#include 
>> +#include 
>>   #include 
>> +#include 
>> +#include 
>>   #include "amdgpu.h"
>>   #include "cgs_linux.h"
>>   #include "atom.h"
>> @@ -39,6 +43,30 @@ static int amdgpu_cgs_gpu_mem_info(void *cgs_device, enum 
>> cgs_gpu_mem_type type,
>> uint64_t *mc_start, uint64_t *mc_size,
>> uint64_t *mem_size)
>>   {
>> +CGS_FUNC_ADEV;
>> +switch(type) {
>> +case CGS_GPU_MEM_TYPE__VISIBLE_CONTIG_FB:
>> +case CGS_GPU_MEM_TYPE__VISIBLE_FB:
>> +*mc_start = 0;
>> +*mc_size = adev->mc.visible_vram_size;
>> +*mem_size = adev->mc.visible_vram_size - adev->vram_pin_size;
>> +break;
>> +case CGS_GPU_MEM_TYPE__INVISIBLE_CONTIG_FB:
>> +case CGS_GPU_MEM_TYPE__INVISIBLE_FB:
>> +*mc_start = adev->mc.visible_vram_size;
>> +*mc_size = adev->mc.real_vram_size - adev->mc.visible_vram_size;
>> +*mem_size = *mc_size;
>> +break;
>> +case CGS_GPU_MEM_TYPE__GART_CACHEABLE:
>> +case CGS_GPU_MEM_TYPE__GART_WRITECOMBINE:
>> +*mc_start = adev->mc.gtt_start;
>> +*mc_size = adev->mc.gtt_size;
>> +*mem_size = adev->mc.gtt_size - adev->gart_pin_size;
>> +break;
>> +default:
>> +return -EINVAL;
>> +}
>> +
>>  return 0;
>>   }
>>   
>> @@ -47,11 +75,43 @@ static int amdgpu_cgs_gmap_kmem(void *cgs_device, void 
>> *kmem,
>>  uint64_t min_offset, uint64_t max_offset,
>>  cgs_handle_t *kmem_handle, uint64_t *mcaddr)
>>   {
>> -return 0;
>> +CGS_FUNC_ADEV;
>> +int ret;
>> +struct amdgpu_bo *bo;
>> +struct page *kmem_page = vmalloc_to_page(kmem);
>> +int npages = ALIGN(size, PAGE_SIZE) >> PAGE_SHIFT;
>> +
>> +struct sg_table *sg = drm_prime_pages_to_sg(_page, npages);
>> +ret = amdgpu_bo_create(adev, size, PAGE_SIZE, false,
>> +   AMDGPU_GEM_DOMAIN_GTT, 0, sg, );
>> +if (ret)
>> +return ret;
>> +ret = amdgpu_bo_reserve(bo, false);
>> +if (unlikely(ret != 0))
>> +return ret;
>> +
>> +/* pin buffer into GTT */
>> +ret = amdgpu_bo_pin_restricted(bo, AMDGPU_GEM_DOMAIN_GTT,
>> +   min_offset, max_offset, mcaddr);
>> +amdgpu_bo_unreserve(bo);
>> +
>> +*kmem_handle = (cgs_handle_t)bo;
>> +return ret;
>>   }
>>   
>>   static int amdgpu_cgs_gunmap_kmem(void *cgs_device, cgs_handle_t 
>> kmem_handle)
>>   {
>> +struct amdgpu_bo *obj = (struct amdgpu_bo *)kmem_handle;
>> +
>> +if (obj) {
>> +int r = amdgpu_bo_reserve(obj, false);
>> +if (likely(r == 0)) {
>> +amdgpu_bo_unpin(obj);
>> +amdgpu_bo_unreserve(obj);
>> +}
>> +amdgpu_bo_unref();
>> +
>> +}
>>  return 0;
>>   }
>>   
>> @@ -61,46 +121,200 @@ static int amdgpu_cgs_alloc_gpu_mem(void *cgs_device,
>>  uint64_t min_offset, uint64_t max_offset,
>>  cgs_handle_t *handle)
>>   {
>> -return 0;
>> +CGS_FUNC_ADEV;
>> +uint16_t flags = 0;
>> +int ret = 0;
>> +uint32_t domain = 0;
>> +struct amdgpu_bo *obj;
>> +

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

[Daniel Vetter]

On Thu, Jul 09, 2015 at 12:21:06PM -0400, Alex Deucher wrote:
> From: Chunming Zhou 
> 
> This implements the cgs interface for allocating
> GPU memory.
> 
> Reviewed-by: Jammy Zhou 

I don't see that review anywhere on a m-l ... where is 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.
-Daniel

> Signed-off-by: Chunming Zhou 
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 238 
> ++--
>  1 file changed, 226 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> index c1ee39e..ac0f124 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> @@ -21,7 +21,11 @@
>   *
>   *
>   */
> +#include 
> +#include 
>  #include 
> +#include 
> +#include 
>  #include "amdgpu.h"
>  #include "cgs_linux.h"
>  #include "atom.h"
> @@ -39,6 +43,30 @@ static int amdgpu_cgs_gpu_mem_info(void *cgs_device, enum 
> cgs_gpu_mem_type type,
>  uint64_t *mc_start, uint64_t *mc_size,
>  uint64_t *mem_size)
>  {
> + CGS_FUNC_ADEV;
> + switch(type) {
> + case CGS_GPU_MEM_TYPE__VISIBLE_CONTIG_FB:
> + case CGS_GPU_MEM_TYPE__VISIBLE_FB:
> + *mc_start = 0;
> + *mc_size = adev->mc.visible_vram_size;
> + *mem_size = adev->mc.visible_vram_size - adev->vram_pin_size;
> + break;
> + case CGS_GPU_MEM_TYPE__INVISIBLE_CONTIG_FB:
> + case CGS_GPU_MEM_TYPE__INVISIBLE_FB:
> + *mc_start = adev->mc.visible_vram_size;
> + *mc_size = adev->mc.real_vram_size - adev->mc.visible_vram_size;
> + *mem_size = *mc_size;
> + break;
> + case CGS_GPU_MEM_TYPE__GART_CACHEABLE:
> + case CGS_GPU_MEM_TYPE__GART_WRITECOMBINE:
> + *mc_start = adev->mc.gtt_start;
> + *mc_size = adev->mc.gtt_size;
> + *mem_size = adev->mc.gtt_size - adev->gart_pin_size;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
>   return 0;
>  }
>  
> @@ -47,11 +75,43 @@ static int amdgpu_cgs_gmap_kmem(void *cgs_device, void 
> *kmem,
>   uint64_t min_offset, uint64_t max_offset,
>   cgs_handle_t *kmem_handle, uint64_t *mcaddr)
>  {
> - return 0;
> + CGS_FUNC_ADEV;
> + int ret;
> + struct amdgpu_bo *bo;
> + struct page *kmem_page = vmalloc_to_page(kmem);
> + int npages = ALIGN(size, PAGE_SIZE) >> PAGE_SHIFT;
> +
> + struct sg_table *sg = drm_prime_pages_to_sg(_page, npages);
> + ret = amdgpu_bo_create(adev, size, PAGE_SIZE, false,
> +AMDGPU_GEM_DOMAIN_GTT, 0, sg, );
> + if (ret)
> + return ret;
> + ret = amdgpu_bo_reserve(bo, false);
> + if (unlikely(ret != 0))
> + return ret;
> +
> + /* pin buffer into GTT */
> + ret = amdgpu_bo_pin_restricted(bo, AMDGPU_GEM_DOMAIN_GTT,
> +min_offset, max_offset, mcaddr);
> + amdgpu_bo_unreserve(bo);
> +
> + *kmem_handle = (cgs_handle_t)bo;
> + return ret;
>  }
>  
>  static int amdgpu_cgs_gunmap_kmem(void *cgs_device, cgs_handle_t kmem_handle)
>  {
> + struct amdgpu_bo *obj = (struct amdgpu_bo *)kmem_handle;
> +
> + if (obj) {
> + int r = amdgpu_bo_reserve(obj, false);
> + if (likely(r == 0)) {
> + amdgpu_bo_unpin(obj);
> + amdgpu_bo_unreserve(obj);
> + }
> + amdgpu_bo_unref();
> +
> + }
>   return 0;
>  }
>  
> @@ -61,46 +121,200 @@ static int amdgpu_cgs_alloc_gpu_mem(void *cgs_device,
>   uint64_t min_offset, uint64_t max_offset,
>   cgs_handle_t *handle)
>  {
> - return 0;
> + CGS_FUNC_ADEV;
> + uint16_t flags = 0;
> + int ret = 0;
> + uint32_t domain = 0;
> + struct amdgpu_bo *obj;
> + struct ttm_placement placement;
> + struct ttm_place place;
> +
> + if (min_offset > max_offset) {
> + BUG_ON(1);
> + return -EINVAL;
> + }
> +
> + /* fail if the alignment is not a power of 2 */
> + if (((align != 1) && (align & (align - 1)))
> + || size == 0 || align == 0)
> + return -EINVAL;
> +
> +
> + switch(type) {
> + case CGS_GPU_MEM_TYPE__VISIBLE_CONTIG_FB:
> + case CGS_GPU_MEM_TYPE__VISIBLE_FB:
> + flags = AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> + domain = AMDGPU_GEM_DOMAIN_VRAM;
> + if (max_offset > adev->mc.real_vram_size)
> + return -EINVAL;
> + place.fpfn = min_offset 

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

[Alex Deucher]

On Tue, Sep 29, 2015 at 11:20 AM, Daniel Vetter  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 
>> >>
>> >>This implements the cgs interface for allocating
>> >>GPU memory.
>> >>
>> >>Reviewed-by: Jammy Zhou 
>> >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: 

drm/amdgpu: implement cgs gpu memory callbacks

[Dan Carpenter]

On Tue, Aug 25, 2015 at 02:07:21AM +, Zhou, David(ChunMing) wrote:
> >and can the shift actually
> > wrap? 
> [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so 
> <

drm/amdgpu: implement cgs gpu memory callbacks

[Zhou, David(ChunMing)]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Tuesday, August 25, 2015 1:51 PM
> To: Zhou, David(ChunMing)
> Cc: dri-devel at lists.freedesktop.org
> Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
> 
> On Tue, Aug 25, 2015 at 02:07:21AM +, Zhou, David(ChunMing) wrote:
> > >and can the shift actually
> > > wrap?
> > [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so
> < 
> By "Shift wrap" I meant how you shift beyond the end of a 32bit number and it
> truncates, or if it's signed then it can wrap around (it's undefined 
> actually, I
> probably should use a different word?).
> 
> int main(void)
> {
>   unsigned long long a = 0xf000U << 12;
>   unsigned long long b = 0xf000ULL << 12;
> 
>   printf("%llx %llx\n", a, b);
> 
>   return 0;
> }
[DZ] oh, I understand your mean, with previous PAGE_SHIFT, I think it should be 
like 'b' in your example without truncates.

Thanks,
 David Zhou
> 
> regards,
> dan carpenter

drm/amdgpu: implement cgs gpu memory callbacks

[Zhou, David(ChunMing)]

Inline...[DZ]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Tuesday, August 25, 2015 3:51 AM
> To: Zhou, David(ChunMing)
> Cc: dri-devel at lists.freedesktop.org
> Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
> 
> On Mon, Aug 24, 2015 at 07:09:15AM +, Zhou, David(ChunMing) wrote:
> > Hi Dan,
> > Thanks for figuring out that.
> > >274  min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
> > >275  max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
> > Maybe should be:
> > min_offset = obj->placements[0].fpfn;
> > min_offset <<= PAGE_SHIFT;
> > max_offset = obj->placements[0].lpfn;
> > max_offset <<= PAGE_SHIFT;
> 
> 
> It's probably just simpler to be:
> 
>   min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT;
>   max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT;
> 
> But the larger questions aer why is min_offset a u64,
[DZ] max/min_offset is memory size.

>and can the shift actually
> wrap? 
[DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so 
< I'm just looking at static checker warnings, and I'm not very familiar
> with this code so I don't know the answers.
> 
> regards,
> dan carpenter

drm/amdgpu: implement cgs gpu memory callbacks

[Dan Carpenter]

On Mon, Aug 24, 2015 at 07:09:15AM +, Zhou, David(ChunMing) wrote:
> Hi Dan,
> Thanks for figuring out that. 
> >274  min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
> >275  max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
> Maybe should be:
>   min_offset = obj->placements[0].fpfn;
>   min_offset <<= PAGE_SHIFT;
>   max_offset = obj->placements[0].lpfn;
>   max_offset <<= PAGE_SHIFT;


It's probably just simpler to be:

min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT;
max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT;

But the larger questions aer why is min_offset a u64, and can the shift
actually wrap?  I'm just looking at static checker warnings, and I'm
not very familiar with this code so I don't know the answers.

regards,
dan carpenter

drm/amdgpu: implement cgs gpu memory callbacks

[Zhou, David(ChunMing)]

Hi Dan,
Thanks for figuring out that. 
>274  min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
>275  max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Maybe should be:
min_offset = obj->placements[0].fpfn;
min_offset <<= PAGE_SHIFT;
max_offset = obj->placements[0].lpfn;
max_offset <<= PAGE_SHIFT;

Regards,
  David Zhou

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Saturday, August 22, 2015 12:24 AM
> To: Zhou, David(ChunMing)
> Cc: dri-devel at lists.freedesktop.org
> Subject: re: drm/amdgpu: implement cgs gpu memory callbacks
> 
> Hello Chunming Zhou,
> 
> The patch 57ff96cf471a: "drm/amdgpu: implement cgs gpu memory callbacks"
> from Apr 24, 2015, leads to the following static checker
> warning:
> 
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:274
> amdgpu_cgs_gmap_gpu_mem()
>   warn: should 'obj->placements[0]->fpfn << 12' be a 64 bit type?
> 
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:275
> amdgpu_cgs_gmap_gpu_mem()
>   warn: should 'obj->placements[0]->lpfn << 12' be a 64 bit type?
> 
> 
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
>265  static int amdgpu_cgs_gmap_gpu_mem(void *cgs_device, cgs_handle_t
> handle,
>266 uint64_t *mcaddr)
>267  {
>268  int r;
>269  u64 min_offset, max_offset;
>270  struct amdgpu_bo *obj = (struct amdgpu_bo *)handle;
>271
>272  WARN_ON_ONCE(obj->placement.num_placement > 1);
>273
>274  min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
>275  max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
> 
> Both of these.
> 
>276
>277  r = amdgpu_bo_reserve(obj, false);
>278  if (unlikely(r != 0))
>279  return r;
>280  r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT,
>281   min_offset, max_offset, mcaddr);
>282  amdgpu_bo_unreserve(obj);
>283  return r;
>284  }
> 
> There are actually a few of these warnings which were less clear whether the
> warning was correct or not so I didn't send them.
> 
> drivers/gpu/drm/amd/amdgpu/cz_smc.c:463
> cz_smu_populate_single_firmware_entry() warn: should '((header->jt_offset))
> << 2' be a 64 bit type?
> drivers/gpu/drm/amd/amdgpu/fiji_smc.c:404
> fiji_smu_populate_single_firmware_entry() warn: should '((header->jt_offset))
> << 2' be a 64 bit type?
> drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:724
> amdgpu_cgs_get_firmware_info() warn: should '((header->jt_offset)) << 2' be a
> 64 bit type?
> drivers/gpu/drm/amd/amdgpu/tonga_smc.c:406
> tonga_smu_populate_single_firmware_entry() warn: should '((header-
> >jt_offset)) << 2' be a 64 bit type?
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:615 amdgpu_gem_op_ioctl()
> warn: should 'robj->tbo.mem.page_alignment << 12' be a 64 bit type?
> 
> regards,
> dan carpenter

drm/amdgpu: implement cgs gpu memory callbacks

[Dan Carpenter]

Hello Chunming Zhou,

The patch 57ff96cf471a: "drm/amdgpu: implement cgs gpu memory
callbacks" from Apr 24, 2015, leads to the following static checker
warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:274 amdgpu_cgs_gmap_gpu_mem()
warn: should 'obj->placements[0]->fpfn << 12' be a 64 bit type?

drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:275 amdgpu_cgs_gmap_gpu_mem()
warn: should 'obj->placements[0]->lpfn << 12' be a 64 bit type?


drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
   265  static int amdgpu_cgs_gmap_gpu_mem(void *cgs_device, cgs_handle_t 
handle,
   266 uint64_t *mcaddr)
   267  {
   268  int r;
   269  u64 min_offset, max_offset;
   270  struct amdgpu_bo *obj = (struct amdgpu_bo *)handle;
   271  
   272  WARN_ON_ONCE(obj->placement.num_placement > 1);
   273  
   274  min_offset = obj->placements[0].fpfn << PAGE_SHIFT;
   275  max_offset = obj->placements[0].lpfn << PAGE_SHIFT;

Both of these.

   276  
   277  r = amdgpu_bo_reserve(obj, false);
   278  if (unlikely(r != 0))
   279  return r;
   280  r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT,
   281   min_offset, max_offset, mcaddr);
   282  amdgpu_bo_unreserve(obj);
   283  return r;
   284  }

There are actually a few of these warnings which were less clear
whether the warning was correct or not so I didn't send them.

drivers/gpu/drm/amd/amdgpu/cz_smc.c:463 cz_smu_populate_single_firmware_entry() 
warn: should '((header->jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/fiji_smc.c:404 
fiji_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) 
<< 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:724 amdgpu_cgs_get_firmware_info() 
warn: should '((header->jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/tonga_smc.c:406 
tonga_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) 
<< 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:615 amdgpu_gem_op_ioctl() warn: should 
'robj->tbo.mem.page_alignment << 12' be a 64 bit type?

regards,
dan carpenter

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

[Alex Deucher]

From: Chunming Zhou 

This implements the cgs interface for allocating
GPU memory.

Reviewed-by: Jammy Zhou 
Signed-off-by: Chunming Zhou 
Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 238 ++--
 1 file changed, 226 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
index c1ee39e..ac0f124 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
@@ -21,7 +21,11 @@
  *
  *
  */
+#include 
+#include 
 #include 
+#include 
+#include 
 #include "amdgpu.h"
 #include "cgs_linux.h"
 #include "atom.h"
@@ -39,6 +43,30 @@ static int amdgpu_cgs_gpu_mem_info(void *cgs_device, enum 
cgs_gpu_mem_type type,
   uint64_t *mc_start, uint64_t *mc_size,
   uint64_t *mem_size)
 {
+   CGS_FUNC_ADEV;
+   switch(type) {
+   case CGS_GPU_MEM_TYPE__VISIBLE_CONTIG_FB:
+   case CGS_GPU_MEM_TYPE__VISIBLE_FB:
+   *mc_start = 0;
+   *mc_size = adev->mc.visible_vram_size;
+   *mem_size = adev->mc.visible_vram_size - adev->vram_pin_size;
+   break;
+   case CGS_GPU_MEM_TYPE__INVISIBLE_CONTIG_FB:
+   case CGS_GPU_MEM_TYPE__INVISIBLE_FB:
+   *mc_start = adev->mc.visible_vram_size;
+   *mc_size = adev->mc.real_vram_size - adev->mc.visible_vram_size;
+   *mem_size = *mc_size;
+   break;
+   case CGS_GPU_MEM_TYPE__GART_CACHEABLE:
+   case CGS_GPU_MEM_TYPE__GART_WRITECOMBINE:
+   *mc_start = adev->mc.gtt_start;
+   *mc_size = adev->mc.gtt_size;
+   *mem_size = adev->mc.gtt_size - adev->gart_pin_size;
+   break;
+   default:
+   return -EINVAL;
+   }
+
return 0;
 }

@@ -47,11 +75,43 @@ static int amdgpu_cgs_gmap_kmem(void *cgs_device, void 
*kmem,
uint64_t min_offset, uint64_t max_offset,
cgs_handle_t *kmem_handle, uint64_t *mcaddr)
 {
-   return 0;
+   CGS_FUNC_ADEV;
+   int ret;
+   struct amdgpu_bo *bo;
+   struct page *kmem_page = vmalloc_to_page(kmem);
+   int npages = ALIGN(size, PAGE_SIZE) >> PAGE_SHIFT;
+
+   struct sg_table *sg = drm_prime_pages_to_sg(_page, npages);
+   ret = amdgpu_bo_create(adev, size, PAGE_SIZE, false,
+  AMDGPU_GEM_DOMAIN_GTT, 0, sg, );
+   if (ret)
+   return ret;
+   ret = amdgpu_bo_reserve(bo, false);
+   if (unlikely(ret != 0))
+   return ret;
+
+   /* pin buffer into GTT */
+   ret = amdgpu_bo_pin_restricted(bo, AMDGPU_GEM_DOMAIN_GTT,
+  min_offset, max_offset, mcaddr);
+   amdgpu_bo_unreserve(bo);
+
+   *kmem_handle = (cgs_handle_t)bo;
+   return ret;
 }

 static int amdgpu_cgs_gunmap_kmem(void *cgs_device, cgs_handle_t kmem_handle)
 {
+   struct amdgpu_bo *obj = (struct amdgpu_bo *)kmem_handle;
+
+   if (obj) {
+   int r = amdgpu_bo_reserve(obj, false);
+   if (likely(r == 0)) {
+   amdgpu_bo_unpin(obj);
+   amdgpu_bo_unreserve(obj);
+   }
+   amdgpu_bo_unref();
+
+   }
return 0;
 }

@@ -61,46 +121,200 @@ static int amdgpu_cgs_alloc_gpu_mem(void *cgs_device,
uint64_t min_offset, uint64_t max_offset,
cgs_handle_t *handle)
 {
-   return 0;
+   CGS_FUNC_ADEV;
+   uint16_t flags = 0;
+   int ret = 0;
+   uint32_t domain = 0;
+   struct amdgpu_bo *obj;
+   struct ttm_placement placement;
+   struct ttm_place place;
+
+   if (min_offset > max_offset) {
+   BUG_ON(1);
+   return -EINVAL;
+   }
+
+   /* fail if the alignment is not a power of 2 */
+   if (((align != 1) && (align & (align - 1)))
+   || size == 0 || align == 0)
+   return -EINVAL;
+
+
+   switch(type) {
+   case CGS_GPU_MEM_TYPE__VISIBLE_CONTIG_FB:
+   case CGS_GPU_MEM_TYPE__VISIBLE_FB:
+   flags = AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+   domain = AMDGPU_GEM_DOMAIN_VRAM;
+   if (max_offset > adev->mc.real_vram_size)
+   return -EINVAL;
+   place.fpfn = min_offset >> PAGE_SHIFT;
+   place.lpfn = max_offset >> PAGE_SHIFT;
+   place.flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
+   TTM_PL_FLAG_VRAM;
+   break;
+   case CGS_GPU_MEM_TYPE__INVISIBLE_CONTIG_FB:
+   case CGS_GPU_MEM_TYPE__INVISIBLE_FB:
+   flags = AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
+   domain = AMDGPU_GEM_DOMAIN_VRAM;
+   if (adev->mc.visible_vram_size < adev->mc.real_vram_size)