RE: [PATCH] drm/amdgpu: add kernel DOC for ioctls in amdgpu_cs.c file

[Li, Samuel]

I can see them now.

Sam

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Samuel Li
> Sent: Friday, June 01, 2018 5:10 PM
> To: Michel Dänzer ; Liu, Leo 
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: add kernel DOC for ioctls in amdgpu_cs.c
> file
> 
> 
> 
> On 2018-05-31 12:30 PM, Michel Dänzer wrote:
> > On 2018-05-30 08:42 PM, Leo Liu wrote:
> >> There are four ioctls in this files, and DOC gives details of data
> >> structures for each of ioctls, and their functionalities.
> >>
> >> Signed-off-by: Leo Liu 
> >
> > This isn't enough to actually make this part of the generated
> > documentation. It needs to be hooked up to a *.rst file for that.
> >
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

Ø  But we still have to support cases where we have 1GB stolen VRAM, and 
wasting those 1GB would suck a bit.
Not really, since we only move display buffer here.

Regards,
Samuel Li

From: Koenig, Christian
Sent: Wednesday, March 21, 2018 10:07 AM
To: Marek Olšák <mar...@gmail.com>
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Alex Deucher 
<alexdeuc...@gmail.com>; Michel Dänzer <mic...@daenzer.net>; amd-gfx list 
<amd-gfx@lists.freedesktop.org>; Li, Samuel <samuel...@amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

Am 21.03.2018 um 14:57 schrieb Marek Olšák:
On Wed, Mar 21, 2018 at 4:13 AM, Christian König 
<ckoenig.leichtzumer...@gmail.com<mailto:ckoenig.leichtzumer...@gmail.com>> 
wrote:
Am 21.03.2018 um 06:08 schrieb Marek Olšák:
On Tue, Mar 20, 2018 at 4:16 PM, Christian König 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>> wrote:
That's what I meant with use up the otherwise unused VRAM. I don't see any 
disadvantage of always setting GTT as second domain on APUs.

My assumption was that we dropped this in userspace for displayable surfaces, 
but Marek proved that wrong.

So what we should do is actually to add GTT as fallback to all BOs on APUs in 
Mesa and only make sure that the kernel is capable of handling GTT with optimal 
performance (e.g. have user huge pages etc..).

VRAM|GTT is practically as good as GTT. VRAM with BO priorities and eviction 
throttling is the true "VRAM|GTT".
I don't know how else to make use of VRAM intelligently.

Well why not set VRAM|GTT as default on APUs? That should still save quite a 
bunch of moves even with throttling.

I explained why: VRAM|GTT is practically as good as GTT.


I mean there really shouldn't be any advantage to use VRAM any more except that 
we want to use it up as long as it is available.

Why are you suggesting to use VRAM|GTT then? Let's just only use GTT on all 
APUs.

Then we don't use the memory stolen for VRAM.

See we want to get to a point where we have any ~16MB of stolen VRAM on APUs 
and everything else in GTT.

But we still have to support cases where we have 1GB stolen VRAM, and wasting 
those 1GB would suck a bit.

Christian.



Marek

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

Also, the change here is only for display buffer, so it is not really a "big" 
waste.

Regards,
Samuel Li

From: Li, Samuel
Sent: Tuesday, March 20, 2018 4:38 PM
To: Deucher, Alexander <alexander.deuc...@amd.com>
Cc: Alex Deucher <alexdeuc...@gmail.com>; Michel Dänzer <mic...@daenzer.net>; 
amd-gfx list <amd-gfx@lists.freedesktop.org>; Marek Olšák <mar...@gmail.com>; 
Koenig, Christian <christian.koe...@amd.com>
Subject: RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support


O  I think we can also have the case of systems with similar amounts of carve 
out and system ram.  E.g., on a system with 4 GB of system memory with 1 GB 
carved out for vram.  It would be a big waste not to use VRAM.  We'll probably 
need a heuristic at some point.
Agreed. But for CZ/ST, due to hardware limitation as discussed before, we 
either use VRAM or GTT, not both. That might be changed for later ASICs, but it 
is beyond the scope of this patch.


Regards,
Samuel Li

From: Koenig, Christian
Sent: Tuesday, March 20, 2018 4:17 PM
To: Deucher, Alexander 
<alexander.deuc...@amd.com<mailto:alexander.deuc...@amd.com>>; Marek Olšák 
<mar...@gmail.com<mailto:mar...@gmail.com>>
Cc: Alex Deucher <alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>; Michel 
Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; Li, Samuel 
<samuel...@amd.com<mailto:samuel...@amd.com>>; amd-gfx list 
<amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

That's what I meant with use up the otherwise unused VRAM. I don't see any 
disadvantage of always setting GTT as second domain on APUs.

My assumption was that we dropped this in userspace for displayable surfaces, 
but Marek proved that wrong.

So what we should do is actually to add GTT as fallback to all BOs on APUs in 
Mesa and only make sure that the kernel is capable of handling GTT with optimal 
performance (e.g. have user huge pages etc..).

Christian.

Am 20.03.2018 um 21:04 schrieb Deucher, Alexander:

I think we can also have the case of systems with similar amounts of carve out 
and system ram.  E.g., on a system with 4 GB of system memory with 1 GB carved 
out for vram.  It would be a big waste not to use VRAM.  We'll probably need a 
heuristic at some point.



Alex


From: Christian König 
<ckoenig.leichtzumer...@gmail.com><mailto:ckoenig.leichtzumer...@gmail.com>
Sent: Tuesday, March 20, 2018 2:32:49 PM
To: Marek Olšák; Koenig, Christian
Cc: Alex Deucher; Deucher, Alexander; Michel Dänzer; Li, Samuel; amd-gfx list
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

I don't think that is a good idea.

Ideally GTT should now have the same performance as VRAM on APUs and we should 
use VRAM only for things where we absolutely have to and to actually use up the 
otherwise unused VRAM.

Can you run some tests with all BOs forced to GTT and see if there is any 
performance regression?

Christian.

Am 20.03.2018 um 15:51 schrieb Marek Olšák:
On Tue, Mar 20, 2018 at 9:55 AM, Christian König 
<ckoenig.leichtzumer...@gmail.com<mailto:ckoenig.leichtzumer...@gmail.com>> 
wrote:
Yes, exactly. And if I remember correctly Mesa used to always set GTT as 
fallback on APUs, correct?

"used to" is the key part. Mesa doesn't force GTT on APUs anymore. It expects 
that the combination of BO priorities and BO move throttling will result in 
optimal BO placements over time.
Marek


The problem seems to be that this fallback isn't set for displayable BOs.

So what needs to be done is to just enable this fallback for displayable BOs as 
well if the kernel can handle it.

Christian.


Am 20.03.2018 um 00:01 schrieb Marek Olšák:
In theory, Mesa doesn't have to do anything. It can continue setting VRAM and 
if the kernel has to put a display buffer into GTT, it doesn't matter (for 
Mesa). Whether the VRAM placement is really used is largely determined by BO 
priorities.

The way I understand scather/gather is that it only allows the GTT placement. 
It doesn't force the GTT placement. Mesa also doesn't force the GTT placement.

Marek

On Mon, Mar 19, 2018 at 5:12 PM, Alex Deucher 
<alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>> wrote:
On Mon, Mar 19, 2018 at 4:29 PM, Li, Samuel 
<samuel...@amd.com<mailto:samuel...@amd.com>> wrote:
>>to my earlier point, there may be cases where it is advantageous to put
>> display buffers in vram even if s/g display is supported
>
> Agreed. That is also why the patch has the options to let user select where
> to put display buffers.
>
> As whether to put the option in Mesa or kernel, it seems the difference is
> not much. Also, since amdgpufb can request even without mesa, kernel might
> be a better choice. I

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

O  I think we can also have the case of systems with similar amounts of carve 
out and system ram.  E.g., on a system with 4 GB of system memory with 1 GB 
carved out for vram.  It would be a big waste not to use VRAM.  We'll probably 
need a heuristic at some point.
Agreed. But for CZ/ST, due to hardware limitation as discussed before, we 
either use VRAM or GTT, not both. That might be changed for later ASICs, but it 
is beyond the scope of this patch.


Regards,
Samuel Li

From: Koenig, Christian
Sent: Tuesday, March 20, 2018 4:17 PM
To: Deucher, Alexander <alexander.deuc...@amd.com>; Marek Olšák 
<mar...@gmail.com>
Cc: Alex Deucher <alexdeuc...@gmail.com>; Michel Dänzer <mic...@daenzer.net>; 
Li, Samuel <samuel...@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

That's what I meant with use up the otherwise unused VRAM. I don't see any 
disadvantage of always setting GTT as second domain on APUs.

My assumption was that we dropped this in userspace for displayable surfaces, 
but Marek proved that wrong.

So what we should do is actually to add GTT as fallback to all BOs on APUs in 
Mesa and only make sure that the kernel is capable of handling GTT with optimal 
performance (e.g. have user huge pages etc..).

Christian.

Am 20.03.2018 um 21:04 schrieb Deucher, Alexander:

I think we can also have the case of systems with similar amounts of carve out 
and system ram.  E.g., on a system with 4 GB of system memory with 1 GB carved 
out for vram.  It would be a big waste not to use VRAM.  We'll probably need a 
heuristic at some point.



Alex


From: Christian König 
<ckoenig.leichtzumer...@gmail.com><mailto:ckoenig.leichtzumer...@gmail.com>
Sent: Tuesday, March 20, 2018 2:32:49 PM
To: Marek Olšák; Koenig, Christian
Cc: Alex Deucher; Deucher, Alexander; Michel Dänzer; Li, Samuel; amd-gfx list
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

I don't think that is a good idea.

Ideally GTT should now have the same performance as VRAM on APUs and we should 
use VRAM only for things where we absolutely have to and to actually use up the 
otherwise unused VRAM.

Can you run some tests with all BOs forced to GTT and see if there is any 
performance regression?

Christian.

Am 20.03.2018 um 15:51 schrieb Marek Olšák:
On Tue, Mar 20, 2018 at 9:55 AM, Christian König 
<ckoenig.leichtzumer...@gmail.com<mailto:ckoenig.leichtzumer...@gmail.com>> 
wrote:
Yes, exactly. And if I remember correctly Mesa used to always set GTT as 
fallback on APUs, correct?

"used to" is the key part. Mesa doesn't force GTT on APUs anymore. It expects 
that the combination of BO priorities and BO move throttling will result in 
optimal BO placements over time.
Marek


The problem seems to be that this fallback isn't set for displayable BOs.

So what needs to be done is to just enable this fallback for displayable BOs as 
well if the kernel can handle it.

Christian.


Am 20.03.2018 um 00:01 schrieb Marek Olšák:
In theory, Mesa doesn't have to do anything. It can continue setting VRAM and 
if the kernel has to put a display buffer into GTT, it doesn't matter (for 
Mesa). Whether the VRAM placement is really used is largely determined by BO 
priorities.

The way I understand scather/gather is that it only allows the GTT placement. 
It doesn't force the GTT placement. Mesa also doesn't force the GTT placement.

Marek

On Mon, Mar 19, 2018 at 5:12 PM, Alex Deucher 
<alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>> wrote:
On Mon, Mar 19, 2018 at 4:29 PM, Li, Samuel 
<samuel...@amd.com<mailto:samuel...@amd.com>> wrote:
>>to my earlier point, there may be cases where it is advantageous to put
>> display buffers in vram even if s/g display is supported
>
> Agreed. That is also why the patch has the options to let user select where
> to put display buffers.
>
> As whether to put the option in Mesa or kernel, it seems the difference is
> not much. Also, since amdgpufb can request even without mesa, kernel might
> be a better choice. In addition, putting in the kernel can save client's
> duplicate work(mesa, ogl, vulkan, 2d, kernel...)

Why do we even expose different memory pools to the UMDs in the first
place ;)  Each pool has performance characteristics that may be
relevant for a particular work load.  Only the UMDs really know the
finer points of those workloads. In general, you don't want the kernel
dictating policy if you can avoid it.  The kernel exposes
functionality and userspace sets the policy.  With the location set in
userspace, each app/user can have whatever policy makes sense for
their use case all at the same time without needing to tweak their
kernel for every use case.

Alex



___

amd-gfx mailing

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

>to my earlier point, there may be cases where it is advantageous to put 
>display buffers in vram even if s/g display is supported
Agreed. That is also why the patch has the options to let user select where to 
put display buffers.
As whether to put the option in Mesa or kernel, it seems the difference is not 
much. Also, since amdgpufb can request even without mesa, kernel might be a 
better choice. In addition, putting in the kernel can save client’s duplicate 
work(mesa, ogl, vulkan, 2d, kernel…)

Regards,
Samuel Li

From: Marek Olšák [mailto:mar...@gmail.com]
Sent: Monday, March 19, 2018 4:27 PM
To: Deucher, Alexander <alexander.deuc...@amd.com>
Cc: Li, Samuel <samuel...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; Alex Deucher <alexdeuc...@gmail.com>; Michel Dänzer 
<mic...@daenzer.net>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

When Mesa wants a buffer in VRAM, it always sets VRAM. It relies on BO move 
throttling to prevent unnecessary BO moves.

My questions are:
- what should Mesa do differently for tiny VRAM?
- what is a tiny VRAM?
- if VRAM is tiny, which allocations should we put there?

Marek

On Mon, Mar 19, 2018 at 4:15 PM, Deucher, Alexander 
<alexander.deuc...@amd.com<mailto:alexander.deuc...@amd.com>> wrote:
s/not/now/.  I meant to say, “we have NOW excluded the possibility of ever 
setting displays anywhere else without a kernel update”.

Alex

From: amd-gfx 
[mailto:amd-gfx-boun...@lists.freedesktop.org<mailto:amd-gfx-boun...@lists.freedesktop.org>]
 On Behalf Of Deucher, Alexander
Sent: Monday, March 19, 2018 4:13 PM

To: Li, Samuel <samuel...@amd.com<mailto:samuel...@amd.com>>; Koenig, Christian 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>>; Marek Olšák 
<mar...@gmail.com<mailto:mar...@gmail.com>>
Cc: Alex Deucher <alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>; Michel 
Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; amd-gfx list 
<amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support


I'm not sure what you mean by the 3 scenarios.  Generally userspace selects 
what domains it wants a buffer to be in, vram, gtt, or both (don't care).  I'd 
rather not have the kernel second guess the UMDs if we can help it.  I'd rather 
leave the kernel for cases where we have to force things due to hw bugs, or hw 
restrictions, etc.  If we force all display buffers to be in gtt in the kernel, 
we have not excluded the possibility of ever setting displays anywhere else 
without a kernel update.  E.g., to my earlier point, there may be cases where 
it is advantageous to put display buffers in vram even if s/g display is 
supported.  That was the point I was trying to make about user mode selecting 
the domain (vram of gtt or vram|gtt).  Say you have a board with 2 GB of ram 
and 1 GB is carved out for "vram".  In that case, it would make sense to put 
the buffer in vram because otherwise you are wasting a comparatively scarce 
resource.



Alex


From: Li, Samuel
Sent: Monday, March 19, 2018 3:58:52 PM
To: Deucher, Alexander; Koenig, Christian; Marek Olšák
Cc: Alex Deucher; Michel Dänzer; amd-gfx list
Subject: RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support


Alex,



I assume you are talking the three scenarios here, 1)VRAM, 2)GTT, 3)VRAM/GTT.

But kernel will need the decision too(amdgpufb). I think it shall be better to 
do it in kernel, instead of different clients(mesa, ddx, kernel …)



Regards,

Samuel Li



From: Deucher, Alexander
Sent: Monday, March 19, 2018 3:54 PM
To: Li, Samuel <samuel...@amd.com<mailto:samuel...@amd.com>>; Koenig, Christian 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>>; Marek Olšák 
<mar...@gmail.com<mailto:mar...@gmail.com>>
Cc: Alex Deucher <alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>; Michel 
Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; amd-gfx list 
<amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support



My personal preference is still to plumb this through to mesa rather than 
forcing it in the kernel.



Alex



From: amd-gfx 
<amd-gfx-boun...@lists.freedesktop.org<mailto:amd-gfx-boun...@lists.freedesktop.org>>
 on behalf of Li, Samuel <samuel...@amd.com<mailto:samuel...@amd.com>>
Sent: Monday, March 19, 2018 3:50:34 PM
To: Koenig, Christian; Marek Olšák
Cc: Alex Deucher; Michel Dänzer; amd-gfx list
Subject: RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support



Christian,



You misunderstood Alex’s comm

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

Alex,

I assume you are talking the three scenarios here, 1)VRAM, 2)GTT, 3)VRAM/GTT.
But kernel will need the decision too(amdgpufb). I think it shall be better to 
do it in kernel, instead of different clients(mesa, ddx, kernel ...)

Regards,
Samuel Li

From: Deucher, Alexander
Sent: Monday, March 19, 2018 3:54 PM
To: Li, Samuel <samuel...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; Marek Olšák <mar...@gmail.com>
Cc: Alex Deucher <alexdeuc...@gmail.com>; Michel Dänzer <mic...@daenzer.net>; 
amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support


My personal preference is still to plumb this through to mesa rather than 
forcing it in the kernel.



Alex


From: amd-gfx 
<amd-gfx-boun...@lists.freedesktop.org<mailto:amd-gfx-boun...@lists.freedesktop.org>>
 on behalf of Li, Samuel <samuel...@amd.com<mailto:samuel...@amd.com>>
Sent: Monday, March 19, 2018 3:50:34 PM
To: Koenig, Christian; Marek Olšák
Cc: Alex Deucher; Michel Dänzer; amd-gfx list
Subject: RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support


Christian,



You misunderstood Alex's comments,



>Regardless of which scenarios we need to support, I think we also need

>to really plumb this through to mesa however since user space is who

>ultimately requests the location.  Overriding it in the kernel gets

>tricky and can lead to ping-ponging as others have noted.  Better to



Here Alex mentioned the scenarios is 1)VRAM, 2)GTT, 3)VRAM/GTT.

His concern is this might cause ping-pong, not about preferred domain. Since 
preferred domain can solve the ping-pong issue, it shall address his concern 
here.



Regards,

Samuel Li



From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
Sent: Monday, March 19, 2018 3:45 PM
To: Li, Samuel <samuel...@amd.com<mailto:samuel...@amd.com>>; Marek Olšák 
<mar...@gmail.com<mailto:mar...@gmail.com>>; Koenig, Christian 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>>
Cc: Alex Deucher <alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>; Michel 
Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; amd-gfx list 
<amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support



Quoting Alex:

Regardless of which scenarios we need to support, I think we also need

to really plumb this through to mesa however since user space is who

ultimately requests the location.  Overriding it in the kernel gets

tricky and can lead to ping-ponging as others have noted.  Better to

have user space know what chips support it or not and request display

buffers in GTT or VRAM from the start.

And I completely agree with Alex here. So overriding the domain in the kernel 
is a serious NAK from my side as well.

Please implement the necessary bits in Mesa, shouldn't be more than a few lines 
of code anyway.

Regards,
Christian.

Am 19.03.2018 um 20:42 schrieb Li, Samuel:

Agreed.



>I think that the consensus with Alex and me is that we should avoid exactly 
>that.
Christian, Alex's concern is about ping-pong, not about the preferred domain.


Regards,

Samuel Li



From: Marek Olšák [mailto:mar...@gmail.com]
Sent: Monday, March 19, 2018 3:39 PM
To: Koenig, Christian 
<christian.koe...@amd.com><mailto:christian.koe...@amd.com>
Cc: Li, Samuel <samuel...@amd.com><mailto:samuel...@amd.com>; Michel Dänzer 
<mic...@daenzer.net><mailto:mic...@daenzer.net>; Alex Deucher 
<alexdeuc...@gmail.com><mailto:alexdeuc...@gmail.com>; amd-gfx list 
<amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support



On Mon, Mar 19, 2018 at 3:27 PM, Christian König 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>> wrote:

I think that the consensus with Alex and me is that we should avoid exactly 
that.

Overriding the preferred domain in the kernel is a no-go for that patch set, so 
please implement the discussed changes in Mesa.



I don't see how Mesa can make a smarter decision than the kernel. If you 
overwrite the preferred domain of the buffer in the kernel, there will be no 
ping-ponging between domains. Mesa never changes the initial preferred domain.



Marek





Regards,
Christian.


Am 19.03.2018 um 20:22 schrieb Li, Samuel:

I agree with Marek/Michel: since kernel sets the domain before scanning out, it 
shall update the preferred domain here too.

Regards,
Samuel Li


-Original Message-
From: Koenig, Christian
Sent: Thursday, March 08, 2018 4:07 AM
To: Michel Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; Li, Samuel
<samuel...@amd.com<mailto:samuel...@amd.com>>;

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

Christian,

You misunderstood Alex’s comments,


>Regardless of which scenarios we need to support, I think we also need

>to really plumb this through to mesa however since user space is who

>ultimately requests the location.  Overriding it in the kernel gets

>tricky and can lead to ping-ponging as others have noted.  Better to

Here Alex mentioned the scenarios is 1)VRAM, 2)GTT, 3)VRAM/GTT.
His concern is this might cause ping-pong, not about preferred domain. Since 
preferred domain can solve the ping-pong issue, it shall address his concern 
here.

Regards,
Samuel Li

From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
Sent: Monday, March 19, 2018 3:45 PM
To: Li, Samuel <samuel...@amd.com>; Marek Olšák <mar...@gmail.com>; Koenig, 
Christian <christian.koe...@amd.com>
Cc: Alex Deucher <alexdeuc...@gmail.com>; Michel Dänzer <mic...@daenzer.net>; 
amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

Quoting Alex:


Regardless of which scenarios we need to support, I think we also need

to really plumb this through to mesa however since user space is who

ultimately requests the location.  Overriding it in the kernel gets

tricky and can lead to ping-ponging as others have noted.  Better to

have user space know what chips support it or not and request display

buffers in GTT or VRAM from the start.
And I completely agree with Alex here. So overriding the domain in the kernel 
is a serious NAK from my side as well.

Please implement the necessary bits in Mesa, shouldn't be more than a few lines 
of code anyway.

Regards,
Christian.

Am 19.03.2018 um 20:42 schrieb Li, Samuel:
Agreed.

>I think that the consensus with Alex and me is that we should avoid exactly 
>that.
Christian, Alex’s concern is about ping-pong, not about the preferred domain.


Regards,
Samuel Li

From: Marek Olšák [mailto:mar...@gmail.com]
Sent: Monday, March 19, 2018 3:39 PM
To: Koenig, Christian 
<christian.koe...@amd.com><mailto:christian.koe...@amd.com>
Cc: Li, Samuel <samuel...@amd.com><mailto:samuel...@amd.com>; Michel Dänzer 
<mic...@daenzer.net><mailto:mic...@daenzer.net>; Alex Deucher 
<alexdeuc...@gmail.com><mailto:alexdeuc...@gmail.com>; amd-gfx list 
<amd-gfx@lists.freedesktop.org><mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

On Mon, Mar 19, 2018 at 3:27 PM, Christian König 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>> wrote:
I think that the consensus with Alex and me is that we should avoid exactly 
that.

Overriding the preferred domain in the kernel is a no-go for that patch set, so 
please implement the discussed changes in Mesa.

I don't see how Mesa can make a smarter decision than the kernel. If you 
overwrite the preferred domain of the buffer in the kernel, there will be no 
ping-ponging between domains. Mesa never changes the initial preferred domain.

Marek



Regards,
Christian.


Am 19.03.2018 um 20:22 schrieb Li, Samuel:
I agree with Marek/Michel: since kernel sets the domain before scanning out, it 
shall update the preferred domain here too.

Regards,
Samuel Li


-Original Message-
From: Koenig, Christian
Sent: Thursday, March 08, 2018 4:07 AM
To: Michel Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; Li, Samuel
<samuel...@amd.com<mailto:samuel...@amd.com>>; Alex Deucher 
<alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>
Cc: amd-gfx list 
<amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

Am 08.03.2018 um 09:35 schrieb Michel Dänzer:
On 2018-03-07 10:47 AM, Christian König wrote:
Am 07.03.2018 um 09:42 schrieb Michel Dänzer:
On 2018-03-06 07:23 PM, Christian König wrote:
E.g. the last time I tested it placing things into GTT still
resulted in quite a performance penalty for rendering.
FWIW, I think the penalty is most likely IOMMU related. Last time I
tested, I couldn't measure a big difference with IOMMU disabled.
No, the penalty I'm talking about came from the ping/pong we did with
the scanout buffers.

See when I tested this the DDX and Mesa where unmodified, so both
still assumed VRAM as placement for scanout BOs, but the kernel
forced scanout BOs into GTT for testing.

So what happened was that on scanout we moved the VRAM BO to GTT
and
after unpinning it on the first command submission which used the BO
we moved it back to VRAM again.
In the meantime, I've had the same idea as Marek: Can't the kernel
driver simply change the BO's preferred domain to GTT when scanning
out from it? Then it won't move back to VRAM.
Yes, I've considered this as well.

But I think making the decision in Mesa is the cleaner approach.

E.g. so far we only override the placement

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

Agreed.

>I think that the consensus with Alex and me is that we should avoid exactly 
>that.
Christian, Alex’s concern is about ping-pong, not about the preferred domain.

Regards,
Samuel Li

From: Marek Olšák [mailto:mar...@gmail.com]
Sent: Monday, March 19, 2018 3:39 PM
To: Koenig, Christian <christian.koe...@amd.com>
Cc: Li, Samuel <samuel...@amd.com>; Michel Dänzer <mic...@daenzer.net>; Alex 
Deucher <alexdeuc...@gmail.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

On Mon, Mar 19, 2018 at 3:27 PM, Christian König 
<christian.koe...@amd.com<mailto:christian.koe...@amd.com>> wrote:
I think that the consensus with Alex and me is that we should avoid exactly 
that.

Overriding the preferred domain in the kernel is a no-go for that patch set, so 
please implement the discussed changes in Mesa.

I don't see how Mesa can make a smarter decision than the kernel. If you 
overwrite the preferred domain of the buffer in the kernel, there will be no 
ping-ponging between domains. Mesa never changes the initial preferred domain.

Marek



Regards,
Christian.


Am 19.03.2018 um 20:22 schrieb Li, Samuel:
I agree with Marek/Michel: since kernel sets the domain before scanning out, it 
shall update the preferred domain here too.

Regards,
Samuel Li

-Original Message-
From: Koenig, Christian
Sent: Thursday, March 08, 2018 4:07 AM
To: Michel Dänzer <mic...@daenzer.net<mailto:mic...@daenzer.net>>; Li, Samuel
<samuel...@amd.com<mailto:samuel...@amd.com>>; Alex Deucher 
<alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>
Cc: amd-gfx list 
<amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

Am 08.03.2018 um 09:35 schrieb Michel Dänzer:
On 2018-03-07 10:47 AM, Christian König wrote:
Am 07.03.2018 um 09:42 schrieb Michel Dänzer:
On 2018-03-06 07:23 PM, Christian König wrote:
E.g. the last time I tested it placing things into GTT still
resulted in quite a performance penalty for rendering.
FWIW, I think the penalty is most likely IOMMU related. Last time I
tested, I couldn't measure a big difference with IOMMU disabled.
No, the penalty I'm talking about came from the ping/pong we did with
the scanout buffers.

See when I tested this the DDX and Mesa where unmodified, so both
still assumed VRAM as placement for scanout BOs, but the kernel
forced scanout BOs into GTT for testing.

So what happened was that on scanout we moved the VRAM BO to GTT
and
after unpinning it on the first command submission which used the BO
we moved it back to VRAM again.
In the meantime, I've had the same idea as Marek: Can't the kernel
driver simply change the BO's preferred domain to GTT when scanning
out from it? Then it won't move back to VRAM.
Yes, I've considered this as well.

But I think making the decision in Mesa is the cleaner approach.

E.g. so far we only override the placement decision of userspace for two
reasons:
1. We where running out of memory in VRAM.
2. We have a hardware restriction which makes VRAM usage mandatory.

And even then we never adjust the placement permanently, we just
temporary moved the buffer where it was needed and moved it back after
the operation completed.

Additional to that Mesa might want to set even more flags and/or changes
it's behavior. E.g. use a tilling mode which both importer and export in an
A+A laptop understands etc...

Regards,
Christian.

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

I agree with Marek/Michel: since kernel sets the domain before scanning out, it 
shall update the preferred domain here too.

Regards,
Samuel Li


> -Original Message-
> From: Koenig, Christian
> Sent: Thursday, March 08, 2018 4:07 AM
> To: Michel Dänzer <mic...@daenzer.net>; Li, Samuel
> <samuel...@amd.com>; Alex Deucher <alexdeuc...@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support
> 
> Am 08.03.2018 um 09:35 schrieb Michel Dänzer:
> > On 2018-03-07 10:47 AM, Christian König wrote:
> >> Am 07.03.2018 um 09:42 schrieb Michel Dänzer:
> >>> On 2018-03-06 07:23 PM, Christian König wrote:
> >>>
> >>>> E.g. the last time I tested it placing things into GTT still
> >>>> resulted in quite a performance penalty for rendering.
> >>> FWIW, I think the penalty is most likely IOMMU related. Last time I
> >>> tested, I couldn't measure a big difference with IOMMU disabled.
> >> No, the penalty I'm talking about came from the ping/pong we did with
> >> the scanout buffers.
> >>
> >> See when I tested this the DDX and Mesa where unmodified, so both
> >> still assumed VRAM as placement for scanout BOs, but the kernel
> >> forced scanout BOs into GTT for testing.
> >>
> >> So what happened was that on scanout we moved the VRAM BO to GTT
> and
> >> after unpinning it on the first command submission which used the BO
> >> we moved it back to VRAM again.
> > In the meantime, I've had the same idea as Marek: Can't the kernel
> > driver simply change the BO's preferred domain to GTT when scanning
> > out from it? Then it won't move back to VRAM.
> 
> Yes, I've considered this as well.
> 
> But I think making the decision in Mesa is the cleaner approach.
> 
> E.g. so far we only override the placement decision of userspace for two
> reasons:
> 1. We where running out of memory in VRAM.
> 2. We have a hardware restriction which makes VRAM usage mandatory.
> 
> And even then we never adjust the placement permanently, we just
> temporary moved the buffer where it was needed and moved it back after
> the operation completed.
> 
> Additional to that Mesa might want to set even more flags and/or changes
> it's behavior. E.g. use a tilling mode which both importer and export in an
> A+A laptop understands etc...
> 
> Regards,
> Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

How about that, in my previous patches, it actually allows three scenarios,
1) VRAM as display buffer
2) GTT as display buffer
3) Mixed GTT/display buffer, as you have asked.

I think the first step is to make the 2nd scenario work at optimal level. After 
that, if anyone wants to work the 3rd scenario, I have no objection at all.

Regards,
Samuel Li


> -Original Message-
> From: Samuel Li [mailto:samuel...@amd.com]
> Sent: Wednesday, March 07, 2018 1:54 PM
> To: Alex Deucher <alexdeuc...@gmail.com>
> Cc: Michel Dänzer <mic...@daenzer.net>; Koenig, Christian
> <christian.koe...@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support
> 
> > You might also want to prefer VRAM, but also allow buffers to fall
> > back to GTT if necessary.
> For display buffer, this case seems not really attractive. When display buffer
> changes between GTT and VRAM dynamically, our driver needs to adpat too,
> which is hard to see the benefits and not really worth the effort.
> 
> Sam
> 
> 
> 
> On 2018-03-07 01:27 PM, Alex Deucher wrote:
> > On Wed, Mar 7, 2018 at 1:18 PM, Samuel Li <samuel...@amd.com> wrote:
> >> I think it's not useful though. Think about that, SG display feature is
> intended to use as less VRAM as possible. Will someone want a display
> buffer sometimes VRAM, sometimes GTT?
> >> Hardly a case to me, and I think it's a waste of effort. That also might
> explain no driver does that now.
> >
> > You might want different strategies depending on how much VRAM you
> > have.  If you have a bunch and it performs better, prefer it.  If you
> > have limited VRAM, you might want to prefer GTT.  You might also want
> > to prefer VRAM, but also allow buffers to fall back to GTT if
> > necessary.  This would make the logic dynamic and all in one place.
> > The kernel can advertise what asics support scanout from system ram
> > and then UMDs can just query that to choose placements rather than
> > adding hardcoded logic for specific asics.
> >
> > Alex
> >
> >>
> >> Sam
> >>
> >>
> >>
> >> On 2018-03-07 12:50 PM, Michel Dänzer wrote:
> >>> On 2018-03-07 06:38 PM, Alex Deucher wrote:
> >>>> On Wed, Mar 7, 2018 at 12:29 PM, Samuel Li <samuel...@amd.com>
> wrote:
> >>>>>
> >>>>> Why so complicated? If old user space compatibility is required, just
> use sg_display=0.
> >>>>
> >>>> It will always just work in that case and we can adjust for the
> >>>> optimal scenario by default in all cases without requiring the user
> >>>> to set misc parameters to tune their setup that may break in the
> >>>> future or hide bugs because a user sets it and forgets it.
> >>>
> >>> Right. I don't agree it's all that complicated anyway, we do this
> >>> sort of thing all the time, it also makes our lives easier down the road.
> >>>
> >>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

> Buffer placement is specified by the DDX/Mesa, when we override that in the 
> kernel we just force unnecessary buffer moves.
Since scan out buffer is moved to GTT now, DDX/Mesa might need to make some 
changes for this too.

Regards,
Samuel Li


> -Original Message-
> From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
> Sent: Tuesday, March 06, 2018 1:02 PM
> To: Michel Dänzer <mic...@daenzer.net>; Koenig, Christian
> <christian.koe...@amd.com>; Li, Samuel <samuel...@amd.com>; Alex
> Deucher <alexdeuc...@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support
> 
> Am 06.03.2018 um 18:51 schrieb Michel Dänzer:
> > On 2018-03-06 06:44 PM, Christian König wrote:
> >> Am 06.03.2018 um 18:22 schrieb Li, Samuel:
> >>>> addition to that I agree with Michel that the module parameter is
> >>>> overkill.
> >>> That I already explained. Currently SG display feature needs to
> >>> provide options for all kinds of use cases. All amd drivers so far
> >>> provides options, and the default configuration is actually to
> >>> allocate everything from GTT when possible.
> >> That isn't job of the kernel to have this parameter. Saying that we
> >> should probably add a DDX and/or environment option to control that.
> > Why would we need that? It's the kernel driver's job to handle this as
> > needed.
> 
> Buffer placement is specified by the DDX/Mesa, when we override that in
> the kernel we just force unnecessary buffer moves.
> 
> Regards,
> Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

> And exactly that's the problematical assumption.
Not assumption, I tested. You have any typical use case that it might become a 
problem?

> addition to that I agree with Michel that the module parameter is overkill.
That I already explained. Currently SG display feature needs to provide options 
for all kinds of use cases. All amd drivers so far provides options, and the 
default configuration is actually to allocate everything from GTT when 
possible. 

Regards,
Samuel Li


> -Original Message-
> From: Koenig, Christian
> Sent: Tuesday, March 06, 2018 12:12 PM
> To: Li, Samuel <samuel...@amd.com>; Alex Deucher
> <alexdeuc...@gmail.com>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support
> 
> And exactly that's the problematical assumption.
> 
> This doesn't print only when the module is loaded, but rather when a
> framebuffer object is created.
> 
> And depending on the use case that can even be many many times per
> second.
> 
> Please remove that printing, addition to that I agree with Michel that the
> module parameter is overkill.
> 
> Regards,
> Christian.
> 
> Am 06.03.2018 um 18:09 schrieb Li, Samuel:
> > This information is kind of important, and it only prints once typically 
> > when
> module is loaded.
> >
> > Regards,
> > Samuel Li
> >
> >
> >> -Original Message-
> >> From: Alex Deucher [mailto:alexdeuc...@gmail.com]
> >> Sent: Tuesday, March 06, 2018 12:04 PM
> >> To: Li, Samuel <samuel...@amd.com>
> >> Cc: Koenig, Christian <christian.koe...@amd.com>; amd-gfx list  >> g...@lists.freedesktop.org>
> >> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display
> >> support
> >>
> >> On Tue, Mar 6, 2018 at 11:49 AM, Samuel Li <samuel...@amd.com> wrote:
> >>>>>domain = amdgpu_display_framebuffer_domains(adev);
> >>>>> +if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> >>>>> +DRM_INFO("Scatter gather display: enabled\n");
> >>>>> +} else if (domain & AMDGPU_GEM_DOMAIN_GTT) {
> >>>>> +DRM_INFO("Scatter gather display: auto\n");
> >>>>> +}
> >>>> Dito and printing that here is not a good idea as far as I can see.
> >>>>
> >>>> Christian.
> >>>
> >>> The intention here is to print out when fb is created. You have
> >>> other
> >> suggestions?
> >>
> >> Make it DRM_DEBUG?  otherwise you'll spam the logs.
> >>
> >>> Sam
> >>>
> >>>
> >>> On 2018-03-03 08:41 AM, Christian König wrote:
> >>>> Am 03.03.2018 um 00:25 schrieb Samuel Li:
> >>>>> It's enabled by default. -1 is auto, to allow both vram and gtt
> >>>>> memory be available, for testing purpose only.
> >>>>> ---
> >>>>>drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> >>>>>drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++--
> >>>>>drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 
> >>>>>drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  | 5 +
> >>>>>4 files changed, 17 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> index 292c7e7..6b0ee34 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> @@ -129,6 +129,7 @@ extern int amdgpu_lbpw;
> >>>>>extern int amdgpu_compute_multipipe;
> >>>>>extern int amdgpu_gpu_recovery;
> >>>>>extern int amdgpu_emu_mode;
> >>>>> +extern int amdgpu_sg_display;
> >>>>>  #ifdef CONFIG_DRM_AMDGPU_SI
> >>>>>extern int amdgpu_si_support;
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>>>> index 5495b29..dfa11b1 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>>>> @@ -513,8 +513,13 @@ uint32_t
> >> amdgpu_display_framebuffer_domains(struct amdgpu_device *adev)
> >>>>>#if defined

RE: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

[Li, Samuel]

This information is kind of important, and it only prints once typically when 
module is loaded. 

Regards,
Samuel Li


> -Original Message-
> From: Alex Deucher [mailto:alexdeuc...@gmail.com]
> Sent: Tuesday, March 06, 2018 12:04 PM
> To: Li, Samuel <samuel...@amd.com>
> Cc: Koenig, Christian <christian.koe...@amd.com>; amd-gfx list  g...@lists.freedesktop.org>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support
> 
> On Tue, Mar 6, 2018 at 11:49 AM, Samuel Li <samuel...@amd.com> wrote:
> >>>   domain = amdgpu_display_framebuffer_domains(adev);
> >>> +if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> >>> +DRM_INFO("Scatter gather display: enabled\n");
> >>> +} else if (domain & AMDGPU_GEM_DOMAIN_GTT) {
> >>> +DRM_INFO("Scatter gather display: auto\n");
> >>> +}
> >>
> >> Dito and printing that here is not a good idea as far as I can see.
> >>
> >> Christian.
> >
> >
> > The intention here is to print out when fb is created. You have other
> suggestions?
> 
> Make it DRM_DEBUG?  otherwise you'll spam the logs.
> 
> >
> > Sam
> >
> >
> > On 2018-03-03 08:41 AM, Christian König wrote:
> >> Am 03.03.2018 um 00:25 schrieb Samuel Li:
> >>> It's enabled by default. -1 is auto, to allow both vram and gtt
> >>> memory be available, for testing purpose only.
> >>> ---
> >>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> >>>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++--
> >>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 
> >>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  | 5 +
> >>>   4 files changed, 17 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index 292c7e7..6b0ee34 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -129,6 +129,7 @@ extern int amdgpu_lbpw;
> >>>   extern int amdgpu_compute_multipipe;
> >>>   extern int amdgpu_gpu_recovery;
> >>>   extern int amdgpu_emu_mode;
> >>> +extern int amdgpu_sg_display;
> >>> #ifdef CONFIG_DRM_AMDGPU_SI
> >>>   extern int amdgpu_si_support;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> index 5495b29..dfa11b1 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> @@ -513,8 +513,13 @@ uint32_t
> amdgpu_display_framebuffer_domains(struct amdgpu_device *adev)
> >>>   #if defined(CONFIG_DRM_AMD_DC)
> >>>   if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <
> CHIP_RAVEN &&
> >>>   adev->flags & AMD_IS_APU &&
> >>> -amdgpu_device_asic_has_dc_support(adev->asic_type))
> >>> -domain |= AMDGPU_GEM_DOMAIN_GTT;
> >>> +amdgpu_device_asic_has_dc_support(adev->asic_type)) {
> >>> +if (amdgpu_sg_display == 1) {
> >>> +domain = AMDGPU_GEM_DOMAIN_GTT;
> >>> +} else if (amdgpu_sg_display == -1) {
> >>> +domain |= AMDGPU_GEM_DOMAIN_GTT;
> >>> +}
> >>
> >> Coding style, that if shouldn't use "{" and "}".
> >>
> >>> +}
> >>>   #endif
> >>> return domain;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> index e670936..f0ada24 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> @@ -132,6 +132,7 @@ int amdgpu_lbpw = -1;
> >>>   int amdgpu_compute_multipipe = -1;
> >>>   int amdgpu_gpu_recovery = -1; /* auto */
> >>>   int amdgpu_emu_mode = 0;
> >>> +int amdgpu_sg_display = 1;
> >>> MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in
> megabytes");
> >>>   module_param_named(vramlimit, amdgpu_vram_limit, int, 0600); @@
> >>> -290,6 +291,9 @@ module_param_named(gpu_recovery,
> amdgpu_gpu_recovery, int, 0444);
> >>>   MODULE_PARM_DESC(emu_mode, "Emulation mod

RE: [PATCH v2 1/1] drm: add kernel doc for exported gem dmabuf_ops

[Li, Samuel]

OK, I can do that.

Regards,
Samuel Li


> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, February 20, 2018 11:23 AM
> To: Li, Samuel <samuel...@amd.com>
> Cc: Daniel Vetter <dan...@ffwll.ch>; Koenig, Christian
> <christian.koe...@amd.com>; amd-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/1] drm: add kernel doc for exported gem
> dmabuf_ops
> 
> On Tue, Feb 20, 2018 at 03:46:51PM +, Li, Samuel wrote:
> > > ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> > > member 'attach' not described in 'drm_gem_unmap_dma_buf'
> > > ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> > > member 'sgt' not described in 'drm_gem_unmap_dma_buf'
> > > ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> > > member 'dir' not described in 'drm_gem_unmap_dma_buf'
> > ...
> >
> > Actually I tested. Those parameters are left out on purpose since they are
> empty functions so far.
> 
> Can you pls add dummy explanations? I know it's somewhat silly, but when
> there's lots of warnings it's much harder to spot new ones. Docs are
> sometimes just plain boring rote work :-/
> 
> Thanks, Daniel
> 
> >
> > Regards,
> > Samuel Li
> >
> > > -----Original Message-
> > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Tuesday, February 20, 2018 10:13 AM
> > > To: Li, Samuel <samuel...@amd.com>
> > > Cc: Daniel Vetter <dan...@ffwll.ch>; Koenig, Christian
> > > <christian.koe...@amd.com>; amd-gfx@lists.freedesktop.org; dri-
> > > de...@lists.freedesktop.org
> > > Subject: Re: [PATCH v2 1/1] drm: add kernel doc for exported gem
> > > dmabuf_ops
> > >
> > > On Tue, Jan 30, 2018 at 04:01:23PM +, Li, Samuel wrote:
> > > >
> > > > Alex helped push this into drm-tip,
> > > > https://cgit.freedesktop.org/drm/drm-
> > > tip/commit/drivers/gpu/drm?id=f7a
> > > > 71b0cf9e36c1b0edbfe89ce028a01164b864d
> > > >
> > > > Thanks,
> > > > Samuel Li
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
> > > > > Daniel Vetter
> > > > > Sent: Tuesday, January 30, 2018 4:33 AM
> > > > > To: Koenig, Christian <christian.koe...@amd.com>
> > > > > Cc: Li, Samuel <samuel...@amd.com>;
> > > > > amd-gfx@lists.freedesktop.org;
> > > > > dri- de...@lists.freedesktop.org
> > > > > Subject: Re: [PATCH v2 1/1] drm: add kernel doc for exported gem
> > > > > dmabuf_ops
> > > > >
> > > > > On Fri, Jan 19, 2018 at 09:51:20AM +0100, Christian König wrote:
> > > > > > Am 18.01.2018 um 22:44 schrieb Samuel Li:
> > > > > > > Signed-off-by: Samuel Li <samuel...@amd.com>
> > > > > > > Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > >
> > > > > Thanks for updating the docs.
> > > > > >
> > > > > > Reviewed-by: Christian König <christian.koe...@amd.com>
> > > > >
> > > > > I'm assuming you'll als push this one.
> > >
> > > I just noticed that fairly obviously this wasn't tested when writing:
> > >
> > > ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> > > member 'attach' not described in 'drm_gem_unmap_dma_buf'
> > > ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> > > member 'sgt' not described in 'drm_gem_unmap_dma_buf'
> > > ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> > > member 'dir' not described in 'drm_gem_unmap_dma_buf'
> > > ./drivers/gpu/drm/drm_prime.c:438: warning: Function parameter or
> > > member 'dma_buf' not described in 'drm_gem_dmabuf_kmap_atomic'
> > > ./drivers/gpu/drm/drm_prime.c:438: warning: Function parameter or
> > > member 'page_num' not described in 'drm_gem_dmabuf_kmap_atomic'
> > > ./drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or
> > > member 'dma_buf' not described in
> 'drm_gem_dmabuf_kunmap_atomic'
> > > ./drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or
> > > member 'page_num' not described in
> 'drm_gem_dmabuf

RE: [PATCH v2 1/1] drm: add kernel doc for exported gem dmabuf_ops

[Li, Samuel]

> ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> member 'attach' not described in 'drm_gem_unmap_dma_buf'
> ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> member 'sgt' not described in 'drm_gem_unmap_dma_buf'
> ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> member 'dir' not described in 'drm_gem_unmap_dma_buf'
...

Actually I tested. Those parameters are left out on purpose since they are 
empty functions so far.

Regards,
Samuel Li

> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, February 20, 2018 10:13 AM
> To: Li, Samuel <samuel...@amd.com>
> Cc: Daniel Vetter <dan...@ffwll.ch>; Koenig, Christian
> <christian.koe...@amd.com>; amd-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/1] drm: add kernel doc for exported gem
> dmabuf_ops
> 
> On Tue, Jan 30, 2018 at 04:01:23PM +, Li, Samuel wrote:
> >
> > Alex helped push this into drm-tip,
> > https://cgit.freedesktop.org/drm/drm-
> tip/commit/drivers/gpu/drm?id=f7a
> > 71b0cf9e36c1b0edbfe89ce028a01164b864d
> >
> > Thanks,
> > Samuel Li
> >
> >
> > > -Original Message-
> > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Tuesday, January 30, 2018 4:33 AM
> > > To: Koenig, Christian <christian.koe...@amd.com>
> > > Cc: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org;
> > > dri- de...@lists.freedesktop.org
> > > Subject: Re: [PATCH v2 1/1] drm: add kernel doc for exported gem
> > > dmabuf_ops
> > >
> > > On Fri, Jan 19, 2018 at 09:51:20AM +0100, Christian König wrote:
> > > > Am 18.01.2018 um 22:44 schrieb Samuel Li:
> > > > > Signed-off-by: Samuel Li <samuel...@amd.com>
> > > > > Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > >
> > > Thanks for updating the docs.
> > > >
> > > > Reviewed-by: Christian König <christian.koe...@amd.com>
> > >
> > > I'm assuming you'll als push this one.
> 
> I just noticed that fairly obviously this wasn't tested when writing:
> 
> ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> member 'attach' not described in 'drm_gem_unmap_dma_buf'
> ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> member 'sgt' not described in 'drm_gem_unmap_dma_buf'
> ./drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or
> member 'dir' not described in 'drm_gem_unmap_dma_buf'
> ./drivers/gpu/drm/drm_prime.c:438: warning: Function parameter or
> member 'dma_buf' not described in 'drm_gem_dmabuf_kmap_atomic'
> ./drivers/gpu/drm/drm_prime.c:438: warning: Function parameter or
> member 'page_num' not described in 'drm_gem_dmabuf_kmap_atomic'
> ./drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or
> member 'dma_buf' not described in 'drm_gem_dmabuf_kunmap_atomic'
> ./drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or
> member 'page_num' not described in 'drm_gem_dmabuf_kunmap_atomic'
> ./drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or
> member 'addr' not described in 'drm_gem_dmabuf_kunmap_atomic'
> ./drivers/gpu/drm/drm_prime.c:461: warning: Function parameter or
> member 'dma_buf' not described in 'drm_gem_dmabuf_kmap'
> ./drivers/gpu/drm/drm_prime.c:461: warning: Function parameter or
> member 'page_num' not described in 'drm_gem_dmabuf_kmap'
> ./drivers/gpu/drm/drm_prime.c:473: warning: Function parameter or
> member 'dma_buf' not described in 'drm_gem_dmabuf_kunmap'
> ./drivers/gpu/drm/drm_prime.c:473: warning: Function parameter or
> member 'page_num' not described in 'drm_gem_dmabuf_kunmap'
> ./drivers/gpu/drm/drm_prime.c:473: warning: Function parameter or
> member 'addr' not described in 'drm_gem_dmabuf_kunmap'
> 
> Samuel, can you pls build docs using
> 
> $ make htmldocs
> 
> and fix up the fallout and submit a patch?
> 
> Thanks, Daniel
> 
> > > -Daniel
> > >
> > > >
> > > > > ---
> > > > >   drivers/gpu/drm/drm_prime.c | 88
> > > +
> > > > >   1 file changed, 88 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_prime.c
> > > > > b/drivers/gpu/drm/drm_prime.c index ca09ce7..e82a976 100644
> > > > > --- a/drivers/gpu/drm/drm_prime.c
> > > > > +++ b/drivers/gpu/drm/drm_prime.c
> > > > > @@ -73,

RE: [PATCH v2 1/1] drm: add kernel doc for exported gem dmabuf_ops

[Li, Samuel]

Alex helped push this into drm-tip,
https://cgit.freedesktop.org/drm/drm-tip/commit/drivers/gpu/drm?id=f7a71b0cf9e36c1b0edbfe89ce028a01164b864d

Thanks,
Samuel Li


> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, January 30, 2018 4:33 AM
> To: Koenig, Christian <christian.koe...@amd.com>
> Cc: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/1] drm: add kernel doc for exported gem
> dmabuf_ops
> 
> On Fri, Jan 19, 2018 at 09:51:20AM +0100, Christian König wrote:
> > Am 18.01.2018 um 22:44 schrieb Samuel Li:
> > > Signed-off-by: Samuel Li <samuel...@amd.com>
> > > Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> 
> Thanks for updating the docs.
> >
> > Reviewed-by: Christian König <christian.koe...@amd.com>
> 
> I'm assuming you'll als push this one.
> -Daniel
> 
> >
> > > ---
> > >   drivers/gpu/drm/drm_prime.c | 88
> +
> > >   1 file changed, 88 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_prime.c
> > > b/drivers/gpu/drm/drm_prime.c index ca09ce7..e82a976 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -73,6 +73,9 @@
> > >* Drivers should detect this situation and return back the gem object
> > >* from the dma-buf private.  Prime will do this automatically for 
> > > drivers
> that
> > >* use the drm_gem_prime_{import,export} helpers.
> > > + *
> > > + * GEM struct _buf_ops symbols are now exported. They can be
> > > + resued by
> > > + * drivers which implement GEM interface.
> > >*/
> > >   struct drm_prime_member {
> > > @@ -180,6 +183,18 @@ static int drm_prime_lookup_buf_handle(struct
> drm_prime_file_private *prime_fpri
> > >   return -ENOENT;
> > >   }
> > > +/**
> > > + * drm_gem_map_attach - dma_buf attach implementation for GEM
> > > + * @dma_buf: buffer to attach device to
> > > + * @target_dev: not used
> > > + * @attach: buffer attachment data
> > > + *
> > > + * Allocates _prime_attachment and calls
> > > +_driver.gem_prime_pin for
> > > + * device specific attachment. This can be used as the
> > > +_buf_ops.attach
> > > + * callback.
> > > + *
> > > + * Returns 0 on success, negative error code on failure.
> > > + */
> > >   int drm_gem_map_attach(struct dma_buf *dma_buf, struct device
> *target_dev,
> > >  struct dma_buf_attachment *attach)
> > >   {
> > > @@ -201,6 +216,14 @@ int drm_gem_map_attach(struct dma_buf
> *dma_buf, struct device *target_dev,
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_map_attach);
> > > +/**
> > > + * drm_gem_map_detach - dma_buf detach implementation for GEM
> > > + * @dma_buf: buffer to detach from
> > > + * @attach: attachment to be detached
> > > + *
> > > + * Cleans up _buf_attachment. This can be used as the
> > > +_buf_ops.detach
> > > + * callback.
> > > + */
> > >   void drm_gem_map_detach(struct dma_buf *dma_buf,
> > >   struct dma_buf_attachment *attach)
> > >   {
> > > @@ -255,6 +278,18 @@ void
> drm_prime_remove_buf_handle_locked(struct drm_prime_file_private
> *prime_fpr
> > >   }
> > >   }
> > > +/**
> > > + * drm_gem_map_dma_buf - map_dma_buf implementation for GEM
> > > + * @attach: attachment whose scatterlist is to be returned
> > > + * @dir: direction of DMA transfer
> > > + *
> > > + * Calls _driver.gem_prime_get_sg_table and then maps the
> > > +scatterlist. This
> > > + * can be used as the _buf_ops.map_dma_buf callback.
> > > + *
> > > + * Returns sg_table containing the scatterlist to be returned;
> > > +returns ERR_PTR
> > > + * on error. May return -EINTR if it is interrupted by a signal.
> > > + */
> > > +
> > >   struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment
> *attach,
> > >enum dma_data_direction dir)
> > >   {
> > > @@ -294,6 +329,12 @@ struct sg_table *drm_gem_map_dma_buf(struct
> dma_buf_attachment *attach,
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_map_dma_buf);
> > > +/**
> > > 

RE: [PATCH] drm/amdgpu: only allow scatter/gather display with DC

[Li, Samuel]

Somehow my test case seems working although DC not enabled?

Regards,
Samuel Li


> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Friday, January 12, 2018 4:03 PM
> To: Alex Deucher ; amd-
> g...@lists.freedesktop.org
> Cc: Deucher, Alexander 
> Subject: Re: [PATCH] drm/amdgpu: only allow scatter/gather display with DC
> 
> Am 12.01.2018 um 20:58 schrieb Alex Deucher:
> > Check if DC is enabled before allowing scanout buffers to be pinned in
> > system memory.
> >
> > Signed-off-by: Alex Deucher 
> 
> Reviewed-by: Christian König 
> 
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 -
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index 8ede2645a06c..859942552e9f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -507,9 +507,12 @@ uint32_t
> amdgpu_display_framebuffer_domains(struct amdgpu_device *adev)
> >   {
> > uint32_t domain = AMDGPU_GEM_DOMAIN_VRAM;
> >
> > +#if defined(CONFIG_DRM_AMD_DC)
> > if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <
> CHIP_RAVEN &&
> > -   adev->flags & AMD_IS_APU)
> > +   adev->flags & AMD_IS_APU &&
> > +   amdgpu_device_asic_has_dc_support(adev->asic_type))
> > domain |= AMDGPU_GEM_DOMAIN_GTT;
> > +#endif
> >
> > return domain;
> >   }
> 
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/3] drm/amdgpu: Move to gtt before cpu accesses dma buf.

[Li, Samuel]

A good catch. I'll fix that.

Regards,
Samuel Li


> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Thursday, January 11, 2018 1:43 AM
> To: kbu...@01.org; Li, Samuel <samuel...@amd.com>
> Cc: kbuild-...@01.org; dri-de...@lists.freedesktop.org; amd-
> g...@lists.freedesktop.org; Li, Samuel <samuel...@amd.com>; Dan Carpenter
> <dan.carpen...@oracle.com>
> Subject: Re: [PATCH 3/3] drm/amdgpu: Move to gtt before cpu accesses dma
> buf.
> 
> 
> Hi Samuel,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on drm/drm-next] [also build test WARNING on
> v4.15-rc7 next-20180110] [if your patch is applied to the wrong git tree,
> please drop us a note to help improve the system]
> 
> url:https://github.com/0day-ci/linux/commits/Samuel-Li/drm-amdgpu-
> allow-framebuffer-in-GART-memory-as-well/20180106-050432
> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> 
> smatch warnings:
> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c:226
> amdgpu_gem_prime_export() error: 'buf' dereferencing possible ERR_PTR()
> 
> # https://github.com/0day-
> ci/linux/commit/6aa2afecb5a3efc463d200023839399571404843
> git remote add linux-review https://github.com/0day-ci/linux git remote
> update linux-review git checkout
> 6aa2afecb5a3efc463d200023839399571404843
> vim +/buf +226 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> 
> 6aa2afecb5 Samuel Li   2018-01-04  211
> d38ceaf99e Alex Deucher2015-04-20  212  struct dma_buf
> *amdgpu_gem_prime_export(struct drm_device *dev,
> d38ceaf99e Alex Deucher2015-04-20  213
>   struct drm_gem_object *gobj,
> d38ceaf99e Alex Deucher2015-04-20  214
>   int flags)
> d38ceaf99e Alex Deucher2015-04-20  215  {
> d38ceaf99e Alex Deucher2015-04-20  216struct amdgpu_bo *bo =
> gem_to_amdgpu_bo(gobj);
> 4b277247b1 Christian König 2017-11-13  217struct dma_buf *buf;
> d38ceaf99e Alex Deucher2015-04-20  218
> e1eb899b45 Christian König 2017-08-25  219if
> (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) ||
> e1eb899b45 Christian König 2017-08-25  220bo->flags &
> AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
> d38ceaf99e Alex Deucher2015-04-20  221return ERR_PTR(-
> EPERM);
> d38ceaf99e Alex Deucher2015-04-20  222
> 4b277247b1 Christian König 2017-11-13  223buf =
> drm_gem_prime_export(dev, gobj, flags);
> 4b277247b1 Christian König 2017-11-13  224if (!IS_ERR(buf))
> 4b277247b1 Christian König 2017-11-13  225buf->file->f_mapping
> = dev->anon_inode->i_mapping;
> 6aa2afecb5 Samuel Li   2018-01-04 @226buf->ops =
> _dmabuf_ops;
> 6aa2afecb5 Samuel Li   2018-01-04  227
> 4b277247b1 Christian König 2017-11-13  228return buf;
> d38ceaf99e Alex Deucher2015-04-20  229  }
> 6aa2afecb5 Samuel Li   2018-01-04  230
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/3] drm: export gem dmabuf_ops for drivers to reuse

[Li, Samuel]

OK. I'll do it.

Samuel Li


> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, January 10, 2018 3:17 AM
> To: Alex Deucher <alexdeuc...@gmail.com>
> Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Li, Samuel
> <samuel...@amd.com>; Daniel Vetter <dan...@ffwll.ch>; Koenig, Christian
> <christian.koe...@amd.com>; dri-de...@lists.freedesktop.org; amd-
> g...@lists.freedesktop.org
> Subject: Re: [PATCH 2/3] drm: export gem dmabuf_ops for drivers to reuse
> 
> On Tue, Jan 09, 2018 at 01:13:08PM -0500, Alex Deucher wrote:
> > On Tue, Jan 9, 2018 at 10:56 AM, Deucher, Alexander
> > <alexander.deuc...@amd.com> wrote:
> > > I'll can push this and a few other misc patches today.
> > >
> >
> > Pushed to drm-misc-next.
> 
> One thing I just noticed: Some kerneldoc for the newly exported functions
> and maybe a small update to the intro section to explain what to do with this
> would be neat.
> -Daniel
> 
> >
> > Thanks,
> >
> > Alex
> >
> >
> > >
> > > Alex
> > > 
> > > From: amd-gfx <amd-gfx-boun...@lists.freedesktop.org> on behalf of
> > > Li, Samuel <samuel...@amd.com>
> > > Sent: Tuesday, January 9, 2018 10:20 AM
> > > To: Daniel Vetter; Koenig, Christian
> > > Cc: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
> > > Subject: RE: [PATCH 2/3] drm: export gem dmabuf_ops for drivers to
> > > reuse
> > >
> > > Yes, please hush this for me.
> > >
> > > Regards,
> > > Samuel Li
> > >
> > >
> > >> -Original Message-
> > >> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
> > >> Daniel Vetter
> > >> Sent: Tuesday, January 09, 2018 4:22 AM
> > >> To: Koenig, Christian <christian.koe...@amd.com>
> > >> Cc: Li, Samuel <samuel...@amd.com>;
> > >> dri-de...@lists.freedesktop.org; amd- g...@lists.freedesktop.org
> > >> Subject: Re: [PATCH 2/3] drm: export gem dmabuf_ops for drivers to
> > >> reuse
> > >>
> > >> On Fri, Jan 05, 2018 at 10:16:04AM +0100, Christian König wrote:
> > >> > Am 04.01.2018 um 22:12 schrieb Samuel Li:
> > >> > > Change-Id: I03c22a890d2305f3243d88019d1a28bddd4ddda7
> > >> > > Signed-off-by: Samuel Li <samuel...@amd.com>
> > >> >
> > >> > Reviewed-by: Christian König <christian.koe...@amd.com>
> > >>
> > >> Want to push to drm-misc or some other plan with this?
> > >> -Daniel
> > >>
> > >> >
> > >> > > ---
> > >> > >   drivers/gpu/drm/drm_prime.c | 53
> > >> > > ++---
> > >> 
> > >> > >   include/drm/drm_prime.h | 22 +++
> > >> > >   2 files changed, 53 insertions(+), 22 deletions(-)
> > >> > >
> > >> > > diff --git a/drivers/gpu/drm/drm_prime.c
> > >> > > b/drivers/gpu/drm/drm_prime.c index 8de93a2..68a69e9 100644
> > >> > > --- a/drivers/gpu/drm/drm_prime.c
> > >> > > +++ b/drivers/gpu/drm/drm_prime.c
> > >> > > @@ -180,9 +180,8 @@ static int
> > >> > > drm_prime_lookup_buf_handle(struct
> > >> drm_prime_file_private *prime_fpri
> > >> > >return -ENOENT;
> > >> > >   }
> > >> > > -static int drm_gem_map_attach(struct dma_buf *dma_buf,
> > >> > > -   struct device *target_dev,
> > >> > > -   struct dma_buf_attachment *attach)
> > >> > > +int drm_gem_map_attach(struct dma_buf *dma_buf, struct
> device
> > >> *target_dev,
> > >> > > +struct dma_buf_attachment *attach)
> > >> > >   {
> > >> > >struct drm_prime_attachment *prime_attach;
> > >> > >struct drm_gem_object *obj = dma_buf->priv; @@ -200,9
> > >> > > +199,10
> > >> @@
> > >> > > static int drm_gem_map_attach(struct dma_buf *dma_buf,
> > >> > >return dev->driver->gem_prime_pin(obj);
> > >> > >   }
> > >> > > +EXPORT_SYMBOL(drm_gem_map_attach);
> > >> > > -static void drm_gem_map_det

RE: [PATCH 2/3] drm: export gem dmabuf_ops for drivers to reuse

[Li, Samuel]

Yes, please hush this for me.

Regards,
Samuel Li


> -Original Message-
> From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, January 09, 2018 4:22 AM
> To: Koenig, Christian <christian.koe...@amd.com>
> Cc: Li, Samuel <samuel...@amd.com>; dri-de...@lists.freedesktop.org; amd-
> g...@lists.freedesktop.org
> Subject: Re: [PATCH 2/3] drm: export gem dmabuf_ops for drivers to reuse
> 
> On Fri, Jan 05, 2018 at 10:16:04AM +0100, Christian König wrote:
> > Am 04.01.2018 um 22:12 schrieb Samuel Li:
> > > Change-Id: I03c22a890d2305f3243d88019d1a28bddd4ddda7
> > > Signed-off-by: Samuel Li <samuel...@amd.com>
> >
> > Reviewed-by: Christian König <christian.koe...@amd.com>
> 
> Want to push to drm-misc or some other plan with this?
> -Daniel
> 
> >
> > > ---
> > >   drivers/gpu/drm/drm_prime.c | 53 ++---
> 
> > >   include/drm/drm_prime.h | 22 +++
> > >   2 files changed, 53 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_prime.c
> > > b/drivers/gpu/drm/drm_prime.c index 8de93a2..68a69e9 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -180,9 +180,8 @@ static int drm_prime_lookup_buf_handle(struct
> drm_prime_file_private *prime_fpri
> > >   return -ENOENT;
> > >   }
> > > -static int drm_gem_map_attach(struct dma_buf *dma_buf,
> > > -   struct device *target_dev,
> > > -   struct dma_buf_attachment *attach)
> > > +int drm_gem_map_attach(struct dma_buf *dma_buf, struct device
> *target_dev,
> > > +struct dma_buf_attachment *attach)
> > >   {
> > >   struct drm_prime_attachment *prime_attach;
> > >   struct drm_gem_object *obj = dma_buf->priv; @@ -200,9 +199,10
> @@
> > > static int drm_gem_map_attach(struct dma_buf *dma_buf,
> > >   return dev->driver->gem_prime_pin(obj);
> > >   }
> > > +EXPORT_SYMBOL(drm_gem_map_attach);
> > > -static void drm_gem_map_detach(struct dma_buf *dma_buf,
> > > -struct dma_buf_attachment *attach)
> > > +void drm_gem_map_detach(struct dma_buf *dma_buf,
> > > + struct dma_buf_attachment *attach)
> > >   {
> > >   struct drm_prime_attachment *prime_attach = attach->priv;
> > >   struct drm_gem_object *obj = dma_buf->priv; @@ -227,6 +227,7
> @@
> > > static void drm_gem_map_detach(struct dma_buf *dma_buf,
> > >   kfree(prime_attach);
> > >   attach->priv = NULL;
> > >   }
> > > +EXPORT_SYMBOL(drm_gem_map_detach);
> > >   void drm_prime_remove_buf_handle_locked(struct
> drm_prime_file_private *prime_fpriv,
> > >   struct dma_buf *dma_buf)
> > > @@ -253,8 +254,8 @@ void
> drm_prime_remove_buf_handle_locked(struct drm_prime_file_private
> *prime_fpr
> > >   }
> > >   }
> > > -static struct sg_table *drm_gem_map_dma_buf(struct
> dma_buf_attachment *attach,
> > > - enum dma_data_direction dir)
> > > +struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment
> *attach,
> > > +  enum dma_data_direction dir)
> > >   {
> > >   struct drm_prime_attachment *prime_attach = attach->priv;
> > >   struct drm_gem_object *obj = attach->dmabuf->priv; @@ -289,13
> > > +290,15 @@ static struct sg_table *drm_gem_map_dma_buf(struct
> dma_buf_attachment *attach,
> > >   return sgt;
> > >   }
> > > +EXPORT_SYMBOL(drm_gem_map_dma_buf);
> > > -static void drm_gem_unmap_dma_buf(struct dma_buf_attachment
> *attach,
> > > -   struct sg_table *sgt,
> > > -   enum dma_data_direction dir)
> > > +void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> > > +struct sg_table *sgt,
> > > +enum dma_data_direction dir)
> > >   {
> > >   /* nothing to be done here */
> > >   }
> > > +EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
> > >   /**
> > >* drm_gem_dmabuf_export - dma_buf export implementation for
> GEM
> > > @@ -346,47 +349,52 @@ void drm_gem_dmabuf_release(struct
> dma

RE: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to drivers

[Li, Samuel]

Ping... can someone please review this patch?

Samuel Li



> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Samuel Li
> Sent: Friday, December 15, 2017 11:28 AM
> To: dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian 
> Subject: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to
> drivers
> 
> From: Christian König 
> 
> Allow drivers to implement their own begin_cpu_access callback.
> 
> Change-Id: I97709b42b9351a04ee7e01106107a87bc56ea258
> Signed-off-by: Christian König 
> ---
>  drivers/gpu/drm/drm_prime.c | 13 +
>  include/drm/drm_drv.h   |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 8de93a2..b4b0e64 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -346,6 +346,18 @@ void drm_gem_dmabuf_release(struct dma_buf
> *dma_buf)  }  EXPORT_SYMBOL(drm_gem_dmabuf_release);
> 
> +static int drm_gem_dmabuf_begin_cpu_access(struct dma_buf *dma_buf,
> + enum dma_data_direction
> direction) {
> + struct drm_gem_object *obj = dma_buf->priv;
> + struct drm_device *dev = obj->dev;
> +
> + if (!dev->driver->gem_prime_begin_cpu_access)
> + return 0;
> +
> + return dev->driver->gem_prime_begin_cpu_access(obj, direction); }
> +
>  static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)  {
>   struct drm_gem_object *obj = dma_buf->priv; @@ -403,6 +415,7
> @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>   .map_dma_buf = drm_gem_map_dma_buf,
>   .unmap_dma_buf = drm_gem_unmap_dma_buf,
>   .release = drm_gem_dmabuf_release,
> + .begin_cpu_access = drm_gem_dmabuf_begin_cpu_access,
>   .map = drm_gem_dmabuf_kmap,
>   .map_atomic = drm_gem_dmabuf_kmap_atomic,
>   .unmap = drm_gem_dmabuf_kunmap,
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index
> 412e83a..1fbf298 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -475,6 +475,8 @@ struct drm_driver {
>   struct drm_device *dev,
>   struct dma_buf_attachment *attach,
>   struct sg_table *sgt);
> + int (*gem_prime_begin_cpu_access)(struct drm_gem_object *obj,
> +enum dma_data_direction
> direction);
>   void *(*gem_prime_vmap)(struct drm_gem_object *obj);
>   void (*gem_prime_vunmap)(struct drm_gem_object *obj, void
> *vaddr);
>   int (*gem_prime_mmap)(struct drm_gem_object *obj,
> --
> 2.7.4
> 
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: FW: [PATCH v2 2/2] drm/amdgpu: Move to gtt before cpu accesses dma buf.

[Li, Samuel]

Will do after some basic testing.

Sam

From: Deucher, Alexander
Sent: Wednesday, December 13, 2017 2:49 PM
To: Li, Samuel <samuel...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: FW: [PATCH v2 2/2] drm/amdgpu: Move to gtt before cpu accesses dma 
buf.


Please send the drm prime patch to dri-devel if you didn't already.



Alex


From: amd-gfx 
<amd-gfx-boun...@lists.freedesktop.org<mailto:amd-gfx-boun...@lists.freedesktop.org>>
 on behalf of Samuel Li <samuel...@amd.com<mailto:samuel...@amd.com>>
Sent: Wednesday, December 13, 2017 2:17:49 PM
To: Koenig, Christian; 
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: FW: [PATCH v2 2/2] drm/amdgpu: Move to gtt before cpu accesses dma 
buf.

For the record.


On 2017-12-13 01:26 PM, Christian König wrote:
> Actually we try to avoid that drivers define their own dma_buf_ops in DRM.
>
> That's why you have all those callbacks in drm_driver which just mirror the 
> dma_buf interface but unpack the GEM object from the dma-buf object.
>
> There are quite a number of exceptions, but those drivers then implement 
> everything on their own because the DRM marshaling doesn't make sense for 
> them.
>
> Christian.
>
> Am 13.12.2017 um 19:01 schrieb Samuel Li:
>> That is an approach. The cost is to add a new call back, which is not 
>> necessary though, since driver can always actually define their own 
>> dma_buf_ops.
>> The intention here is to allow a driver reuse drm_gem_prime_dmabuf_ops{}. If 
>> you would like to go this far, maybe a more straight forward way is to 
>> export those ops, e.g. drm_gem_map_attach, so that a driver can use them in 
>> its own definitions.
>>
>> Sam
>>
>>
>>
>> On 2017-12-13 05:23 AM, Christian König wrote:
>>> Something like the attached patch. Not even compile tested.
>>>
>>> Christian.
>>>
>>> Am 12.12.2017 um 20:13 schrieb Samuel Li:
>>>> Not sure if I understand your comments correctly. Currently amdgpu prime 
>>>> reuses drm_gem_prime_dmabuf_ops{}, and it is defined as static which is 
>>>> reasonable. I do not see an easier way to introduce 
>>>> amdgpu_gem_begin_cpu_access().
>>>>
>>>> Sam
>>>>
>>>> On 2017-12-12 01:30 PM, Christian König wrote:
>>>>>> +while (amdgpu_dmabuf_ops.begin_cpu_access != 
>>>>>> amdgpu_gem_begin_cpu_access)
>>>>> I would rather just add the four liner code to drm to forward the 
>>>>> begin_cpu_access callback into a drm_driver callback instead of all this.
>>>>>
>>>>> But apart from that it looks good to me.
>>>>>
>>>>> Christian.
>>>>>
>>>>> Am 12.12.2017 um 19:14 schrieb Li, Samuel:
>>>>>> A gentle ping on this one, Christian, can you take a look at this?
>>>>>>
>>>>>> Sam
>>>>>>
>>>>>> -Original Message-
>>>>>> From: Li, Samuel
>>>>>> Sent: Friday, December 08, 2017 5:22 PM
>>>>>> To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>>>>>> Cc: Li, Samuel <samuel...@amd.com<mailto:samuel...@amd.com>>
>>>>>> Subject: [PATCH v2 2/2] drm/amdgpu: Move to gtt before cpu accesses dma 
>>>>>> buf.
>>>>>>
>>>>>> To improve cpu read performance. This is implemented for APUs currently.
>>>>>>
>>>>>> v2: Adapt to change 
>>>>>> https://lists.freedesktop.org/archives/amd-gfx/2017-October/015174.html
>>>>>>
>>>>>> Change-Id: I7a583e23a9ee706e0edd2a46f4e4186a609368e3
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  2 ++
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 58 
>>>>>> +++
>>>>>> 3 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> index f8657c3..193db70 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> @@ -417,6 +417,8 @@ amdgpu_g

RE: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses dma buf.

[Li, Samuel]

To explain the operations a little bit, the tests include repeated testing of 
the following sequences,
91 const int sequences[4][4] = {
92 { STEP_MMAP, STEP_FAULT, STEP_FLIP, STEP_DRAW },
93 { STEP_MMAP, STEP_FLIP, STEP_DRAW, STEP_SKIP },
94 { STEP_MMAP, STEP_DRAW, STEP_FLIP, STEP_SKIP },
95 { STEP_FLIP, STEP_MMAP, STEP_DRAW, STEP_SKIP },
96 };
Here STEP_MMAP includes prime_mmap() and begin_cpu_access(). STEP_DRAW includes 
repeated cpu reads/writes.
It looks to me the dma_buf has to be pinned to gtt here, to prevent it being 
pinned back by STEP_FLIP before drawing.

> I've send out a WIP branch a for this a few weeks ago, but haven't worked on
> this in a while. BTW it is only supported on Carizzo and Raven.
Can you show me the branch? Looks like we need to get if finished.

Sam




> -Original Message-
> From: Koenig, Christian
> Sent: Wednesday, November 29, 2017 10:01 AM
> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses dma
> buf.
> 
> > What is the concern for scanning out from GTT on APUs?
> It's simply not implemented yet. You need quite a bunch of different setup in
> DC for this to work.
> 
> I've send out a WIP branch a for this a few weeks ago, but haven't worked on
> this in a while. BTW it is only supported on Carizzo and Raven.
> 
> But even then pinning a BO to GTT for this would still be a no-go. We could
> just avoid the copy on scanout when the BO is already inside GTT because of
> the CPU access.
> 
> In general we should rather work on this as Michel described and avoid
> creating the BO in VRAM in the first place if possible.
> 
> Regards,
> Christian.
> 
> Am 29.11.2017 um 15:56 schrieb Li, Samuel:
> > One major purpose of the ChromeOS mmap_test is to avoid buffer copying.
> What is the concern for scanning out from GTT on APUs?
> >
> > Sam
> >
> >> -Original Message-
> >> From: Koenig, Christian
> >> Sent: Wednesday, November 29, 2017 9:54 AM
> >> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses
> >> dma buf.
> >>
> >> And exactly that's the reason why it is a no-go.
> >>
> >> Scanning out from GTT isn't supported at the moment.
> >>
> >> Christian.
> >>
> >> Am 29.11.2017 um 15:48 schrieb Li, Samuel:
> >>> Actually it needs to be pinned here, since otherwise page flip will
> >>> pin it into
> >> vram.
> >>> SAm
> >>>
> >>>
> >>>> -Original Message-
> >>>> From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
> >>>> Sent: Wednesday, November 29, 2017 4:39 AM
> >>>> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> >>>> Subject: Re: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses
> >>>> dma buf.
> >>>>
> >>>> Am 29.11.2017 um 01:20 schrieb Samuel Li:
> >>>>> To improve cpu read performance. This is implemented for APUs
> >> currently.
> >>>> And once more a NAK for this approach.
> >>>>
> >>>> What we could do is migrate the BO to GTT during mmap, but pinning
> >>>> it is out of question.
> >>>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>> Change-Id: I300a7daed8f2b0ba6be71a43196a6b8617ddc62e
> >>>>> ---
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   2 +
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  10 +-
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |   2 +-
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 108
> >>>> ++
> >>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   8 +-
> >>>>> 5 files changed, 126 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> index f8657c3..193db70 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> @@ -417,6 +417,8 @@ amdgpu_gem_prime_import_sg_table(struct
> >>>> drm_device *dev,
> >>>&g

RE: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses dma buf.

[Li, Samuel]

One major purpose of the ChromeOS mmap_test is to avoid buffer copying. What is 
the concern for scanning out from GTT on APUs?

Sam

> -Original Message-
> From: Koenig, Christian
> Sent: Wednesday, November 29, 2017 9:54 AM
> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses dma
> buf.
> 
> And exactly that's the reason why it is a no-go.
> 
> Scanning out from GTT isn't supported at the moment.
> 
> Christian.
> 
> Am 29.11.2017 um 15:48 schrieb Li, Samuel:
> > Actually it needs to be pinned here, since otherwise page flip will pin it 
> > into
> vram.
> >
> > SAm
> >
> >
> >> -Original Message-
> >> From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
> >> Sent: Wednesday, November 29, 2017 4:39 AM
> >> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH 1/1] drm/amdgpu: Pin to gtt before cpu accesses
> >> dma buf.
> >>
> >> Am 29.11.2017 um 01:20 schrieb Samuel Li:
> >>> To improve cpu read performance. This is implemented for APUs
> currently.
> >> And once more a NAK for this approach.
> >>
> >> What we could do is migrate the BO to GTT during mmap, but pinning it
> >> is out of question.
> >>
> >> Regards,
> >> Christian.
> >>
> >>> Change-Id: I300a7daed8f2b0ba6be71a43196a6b8617ddc62e
> >>> ---
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   2 +
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  10 +-
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |   2 +-
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 108
> >> ++
> >>>drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   8 +-
> >>>5 files changed, 126 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index f8657c3..193db70 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -417,6 +417,8 @@ amdgpu_gem_prime_import_sg_table(struct
> >> drm_device *dev,
> >>>struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
> >>>   struct drm_gem_object *gobj,
> >>>   int flags);
> >>> +struct drm_gem_object *amdgpu_gem_prime_import(struct
> drm_device
> >> *dev,
> >>> + struct dma_buf *dma_buf);
> >>>int amdgpu_gem_prime_pin(struct drm_gem_object *obj);
> >>>void amdgpu_gem_prime_unpin(struct drm_gem_object *obj);
> >>>struct reservation_object *amdgpu_gem_prime_res_obj(struct
> >>> drm_gem_object *); diff --git
> >>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> index d704a45..b5483e4 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >>> @@ -147,6 +147,7 @@ int amdgpu_crtc_page_flip_target(struct
> drm_crtc
> >> *crtc,
> >>>   struct drm_device *dev = crtc->dev;
> >>>   struct amdgpu_device *adev = dev->dev_private;
> >>>   struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> >>> + bool gtt_scannable = (adev->asic_type >= CHIP_CARRIZO && adev-
> >>> flags
> >>> +& AMD_IS_APU);
> >>>   struct amdgpu_framebuffer *old_amdgpu_fb;
> >>>   struct amdgpu_framebuffer *new_amdgpu_fb;
> >>>   struct drm_gem_object *obj;
> >>> @@ -190,8 +191,13 @@ int amdgpu_crtc_page_flip_target(struct
> >>> drm_crtc *crtc,
> >>>
> >>>   r = amdgpu_bo_pin(new_abo, AMDGPU_GEM_DOMAIN_VRAM,
> >> );
> >>>   if (unlikely(r != 0)) {
> >>> - DRM_ERROR("failed to pin new abo buffer before flip\n");
> >>> - goto unreserve;
> >>> + /* latest APUs support gtt scan out */
> >>> + if (gtt_scannable)
> >>> + r = amdgpu_bo_pin(new_abo,
> >> AMDGPU_GEM_DOMAIN_GTT, );
> >>> + if (unlikely(r != 0)) 

RE: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap support

[Li, Samuel]

> If that is what this callback should do then this implementation would be
> incorrect. Pinning doesn't wait for any GPU operation to finish.

During pining, it will all the fences to finish. That shall be OK.

Sam



> -Original Message-
> From: Koenig, Christian
> Sent: Wednesday, September 20, 2017 12:21 PM
> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap support
> 
> Am 20.09.2017 um 17:44 schrieb Li, Samuel:
> >> What happens when another thread is using amdgpu_dmabuf_ops to call
> >> begin_cpu_access/end_cpu_access when you are fixing it up again?
> > Right, that is an issue.
> 
> A simple "if (!amdgpu_dmabuf_ops.begin_cpu_access)" should be able to
> deal with that.
> 
> >
> >> I would just completely drop the two callbacks, pinning is not
> >> necessary for CPU access and thinking more about it it actually has
> >> some unwanted side effects.
> > CPU access needs synchronization anyway, so the two callbacks cannot be
> dropped (other drivers implemented too), so I would like to keep it there for
> now.
> 
> Wait a second what do you mean with "CPU access needs synchronization"?
> 
> At least i915 makes the memory GPU inaccessible when you start to use it
> with the CPU.
> 
> If that is what this callback should do then this implementation would be
> incorrect. Pinning doesn't wait for any GPU operation to finish.
> 
> Regards,
> Christian.
> 
> >
> > Sam
> >
> >
> >> -Original Message-
> >> From: Koenig, Christian
> >> Sent: Wednesday, September 20, 2017 2:58 AM
> >> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap
> support
> >>
> >>> What do you mean "This isn't race free"?
> >> Take a look at the code again:
> >>> +dma_buf = drm_gem_prime_export(dev, gobj, flags);
> >>> +amdgpu_dmabuf_ops = *(dma_buf->ops);
> >>> +amdgpu_dmabuf_ops.begin_cpu_access =
> >> amdgpu_gem_begin_cpu_access;
> >>> +amdgpu_dmabuf_ops.end_cpu_access =
> >> amdgpu_gem_end_cpu_access;
> >>> +dma_buf->ops = _dmabuf_ops;
> >> What happens when another thread is using amdgpu_dmabuf_ops to call
> >> begin_cpu_access/end_cpu_access when you are fixing it up again?
> >>
> >> I would just completely drop the two callbacks, pinning is not
> >> necessary for CPU access and thinking more about it it actually has
> >> some unwanted side effects.
> >>
> >> Regards,
> >> Christian.
> >>
> >> Am 19.09.2017 um 23:22 schrieb Samuel Li:
> >>>>> +vma->vm_pgoff = amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT;
> >>>> Maybe better use "vma->vm_pgoff +=
> amdgpu_bo_mmap_offset(bo) >>
> >> PAGE_SHIFT;", but I'm not sure.
> >>>> How other drivers handle this?
> >>> This is a good catch. Looks like pgoff is honored during prime mmap,
> >>> not a
> >> fake offset here.
> >>>>> +dma_buf->ops = _dmabuf_ops;
> >>>> This isn't race free and needs to be fixed.
> >>>> Better add callbacks to drm_prime.c similar to
> >> drm_gem_dmabuf_mmap().
> >>> What do you mean "This isn't race free"?
> >>>
> >>> Regards,
> >>> Sam
> >>>
> >>>
> >>>
> >>> On 2017-09-15 11:05 AM, Christian König wrote:
> >>>> Am 14.09.2017 um 00:39 schrieb Samuel Li:
> >>>>> v2: drop hdp invalidate/flush.
> >>>>>
> >>>>> Signed-off-by: Samuel Li <samuel...@amd.com>
> >>>>> ---
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  3 ++
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  3 +-
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 77
> >> ++-
> >>>>> 3 files changed, 81 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> index d2aaad7..188b705 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>>> @@ -395,11 +395,14 @@
> amdgpu_gem_prime_i

RE: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap support

[Li, Samuel]

> What happens when another thread is using amdgpu_dmabuf_ops to call
> begin_cpu_access/end_cpu_access when you are fixing it up again?
Right, that is an issue.

> I would just completely drop the two callbacks, pinning is not necessary for
> CPU access and thinking more about it it actually has some unwanted side
> effects.
CPU access needs synchronization anyway, so the two callbacks cannot be dropped 
(other drivers implemented too), so I would like to keep it there for now.

Sam


> -Original Message-
> From: Koenig, Christian
> Sent: Wednesday, September 20, 2017 2:58 AM
> To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap support
> 
> > What do you mean "This isn't race free"?
> 
> Take a look at the code again:
> > +dma_buf = drm_gem_prime_export(dev, gobj, flags);
> > +amdgpu_dmabuf_ops = *(dma_buf->ops);
> > +amdgpu_dmabuf_ops.begin_cpu_access =
> amdgpu_gem_begin_cpu_access;
> > +amdgpu_dmabuf_ops.end_cpu_access =
> amdgpu_gem_end_cpu_access;
> > +dma_buf->ops = _dmabuf_ops;
> 
> What happens when another thread is using amdgpu_dmabuf_ops to call
> begin_cpu_access/end_cpu_access when you are fixing it up again?
> 
> I would just completely drop the two callbacks, pinning is not necessary for
> CPU access and thinking more about it it actually has some unwanted side
> effects.
> 
> Regards,
> Christian.
> 
> Am 19.09.2017 um 23:22 schrieb Samuel Li:
> >>> +vma->vm_pgoff = amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT;
> >> Maybe better use "vma->vm_pgoff += amdgpu_bo_mmap_offset(bo) >>
> PAGE_SHIFT;", but I'm not sure.
> >> How other drivers handle this?
> > This is a good catch. Looks like pgoff is honored during prime mmap, not a
> fake offset here.
> >
> >>> +dma_buf->ops = _dmabuf_ops;
> >> This isn't race free and needs to be fixed.
> >> Better add callbacks to drm_prime.c similar to
> drm_gem_dmabuf_mmap().
> > What do you mean "This isn't race free"?
> >
> > Regards,
> > Sam
> >
> >
> >
> > On 2017-09-15 11:05 AM, Christian König wrote:
> >> Am 14.09.2017 um 00:39 schrieb Samuel Li:
> >>> v2: drop hdp invalidate/flush.
> >>>
> >>> Signed-off-by: Samuel Li <samuel...@amd.com>
> >>> ---
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  3 ++
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  3 +-
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 77
> ++-
> >>>3 files changed, 81 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index d2aaad7..188b705 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -395,11 +395,14 @@ amdgpu_gem_prime_import_sg_table(struct
> drm_device *dev,
> >>>struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
> >>>struct drm_gem_object *gobj,
> >>>int flags);
> >>> +struct drm_gem_object *amdgpu_gem_prime_import(struct
> drm_device *dev,
> >>> +struct dma_buf *dma_buf);
> >>>int amdgpu_gem_prime_pin(struct drm_gem_object *obj);
> >>>void amdgpu_gem_prime_unpin(struct drm_gem_object *obj);
> >>>struct reservation_object *amdgpu_gem_prime_res_obj(struct
> drm_gem_object *);
> >>>void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj);
> >>>void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, void
> >>> *vaddr);
> >>> +int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct
> >>> +vm_area_struct *vma);
> >>>int amdgpu_gem_debugfs_init(struct amdgpu_device *adev);
> >>>  /* sub-allocation manager, it has to be protected by another lock.
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> index 2cdf844..9b63ac5 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> @@ -835,7 +835,7 @@ static struct drm_driver kms_driver = {
> >>>.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> >>>.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> >

RE: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name

[Li, Samuel]

>  - above all, as-is make check will fail  
Right, I did not check that.

>  - keeping the radeon API symmetrical to the amdgpu one would a good idea
The issue is Radeon does not have a struct similar to amdgpu_device_handle. 
I think the current radeon API is simpler. Maybe a follow up change can change 
amdgpu's API similar to radeon.

>  - is adding yet another header really justified?
radeon_asic_id.h? That is going to be used by ddx/mesa.

Sam

> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Wednesday, July 05, 2017 7:03 AM
> To: Li, Samuel <samuel...@amd.com>
> Cc: amd-gfx mailing list <amd-gfx@lists.freedesktop.org>; ML dri-devel  de...@lists.freedesktop.org>
> Subject: Re: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name
> 
> Hi Samuel,
> 
> On 30 June 2017 at 20:25, Samuel Li <samuel...@amd.com> wrote:
> > Change-Id: I24b6624789d1a9dc0fd3a446b0e6f21ed5183ff2
> > Signed-off-by: Samuel Li <samuel...@amd.com>
> > ---
> >  radeon/Makefile.am  |   6 +++
> >  radeon/Makefile.sources |   6 ++-
> >  radeon/radeon_asic_id.c | 106
> > 
> >  radeon/radeon_asic_id.h |  37 +
> >  4 files changed, 153 insertions(+), 2 deletions(-)  create mode
> > 100644 radeon/radeon_asic_id.c  create mode 100644
> > radeon/radeon_asic_id.h
> >
> > diff --git a/radeon/Makefile.am b/radeon/Makefile.am index
> > e241531..69407bc 100644
> > --- a/radeon/Makefile.am
> > +++ b/radeon/Makefile.am
> > @@ -30,6 +30,12 @@ AM_CFLAGS = \
> > $(PTHREADSTUBS_CFLAGS) \
> > -I$(top_srcdir)/include/drm
> >
> > +libdrmdatadir = @libdrmdatadir@
> > +ASIC_ID_TABLE_NUM_ENTRIES := $(shell egrep -ci '^[0-9a-f]{4},.*[0-9a-
> f]+,' \
> > +   $(top_srcdir)/data/amdgpu.ids) AM_CPPFLAGS =
> > +-DRADEON_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\" \
> > +
> > +-
> DRADEON_ASIC_ID_TABLE_NUM_ENTRIES=$(ASIC_ID_TABLE_NUM_ENTRIE
> S)
> > +
> Reusing amdgpu.ids by libdrm_radeon is not so obvious. I'm wondering if
> adding a comment in the file [amdgpu.ids] may be a good idea.
> "File is used by $LIST" or "File has multiple users" or anything that
> hints/makes people look up the details.
> 
> Couple of other ideas/suggestions:
>  - above all, as-is make check will fail  
>  - keeping the radeon API symmetrical to the amdgpu one would a good idea
>  - is adding yet another header really justified?
> 
> -Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name

[Li, Samuel]

> do you also have a Mesa patch showing how the new APIs will be used?
Sent out to mesa dev just now.

> Remove these lines.
Right.

> Please consistently either use this, or don't add the util directory to the
> include path anywhere.
OK.

> Put an empty line between declarations and statements.
OK.

Sam


> -Original Message-
> From: Michel Dänzer [mailto:mic...@daenzer.net]
> Sent: Tuesday, July 04, 2017 5:43 AM
> To: Li, Samuel <samuel...@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
> Subject: Re: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name
> 
> 
> Hi Sam,
> 
> 
> do you also have a Mesa patch showing how the new APIs will be used?
> Without seeing that, some minor comments below.
> 
> 
> On 01/07/17 04:25 AM, Samuel Li wrote:
> >
> > +//#include 
> > +//#include 
> 
> Remove these lines.
> 
> 
> > +#include "util/util_asic_id.h"
> 
> Patch 1 adds the util directory to include paths, which would allow just
> 
> #include "util_asic_id.h"
> 
> Please consistently either use this, or don't add the util directory to the
> include path anywhere.
> 
> 
> > +int radeon_asic_id_initialize(void)
> > +{
> > +   int r = 0;
> > +   pthread_mutex_lock(_id_mutex);
> 
> Put an empty line between declarations and statements.
> 
> 
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/psp: upper_32_bits/lower_32_bits for address setup

[Li, Samuel]

Reviewed-by: Samuel Li <samuel...@amd.com>

Sam

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Alex Deucher
> Sent: Thursday, June 22, 2017 6:29 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <alexander.deuc...@amd.com>
> Subject: [PATCH] drm/amdgpu/psp: upper_32_bits/lower_32_bits for
> address setup
> 
> Rather than casting and shifting.  Fixes sparse case warnings.
> 
> Signed-off-by: Alex Deucher <alexander.deuc...@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c |  4 ++--
> drivers/gpu/drm/amd/amdgpu/psp_v10_0.c  | 12 ++--
>  drivers/gpu/drm/amd/amdgpu/psp_v3_1.c   | 12 ++--
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index c224c5c..0b5f533 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -152,8 +152,8 @@ static void psp_prep_tmr_cmd_buf(struct
> psp_gfx_cmd_resp *cmd,
>uint64_t tmr_mc, uint32_t size)
>  {
>   cmd->cmd_id = GFX_CMD_ID_SETUP_TMR;
> - cmd->cmd.cmd_setup_tmr.buf_phy_addr_lo = (uint32_t)tmr_mc;
> - cmd->cmd.cmd_setup_tmr.buf_phy_addr_hi = (uint32_t)(tmr_mc >>
> 32);
> + cmd->cmd.cmd_setup_tmr.buf_phy_addr_lo =
> lower_32_bits(tmr_mc);
> + cmd->cmd.cmd_setup_tmr.buf_phy_addr_hi =
> upper_32_bits(tmr_mc);
>   cmd->cmd.cmd_setup_tmr.buf_size = size;  }
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> index 20c1e53..2258323 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> @@ -96,8 +96,8 @@ int psp_v10_0_prep_cmd_buf(struct
> amdgpu_firmware_info *ucode, struct psp_gfx_cm
>   header = (struct common_firmware_header *)ucode->fw;
> 
>   cmd->cmd_id = GFX_CMD_ID_LOAD_IP_FW;
> - cmd->cmd.cmd_load_ip_fw.fw_phy_addr_lo =
> (uint32_t)fw_mem_mc_addr;
> - cmd->cmd.cmd_load_ip_fw.fw_phy_addr_hi =
> (uint32_t)((uint64_t)fw_mem_mc_addr >> 32);
> + cmd->cmd.cmd_load_ip_fw.fw_phy_addr_lo =
> lower_32_bits(fw_mem_mc_addr);
> + cmd->cmd.cmd_load_ip_fw.fw_phy_addr_hi =
> +upper_32_bits(fw_mem_mc_addr);
>   cmd->cmd.cmd_load_ip_fw.fw_size = le32_to_cpu(header-
> >ucode_size_bytes);
> 
>   ret = psp_v10_0_get_fw_type(ucode, 
> >cmd.cmd_load_ip_fw.fw_type);
> @@ -172,10 +172,10 @@ int psp_v10_0_cmd_submit(struct psp_context
> *psp,
>   write_frame = ring->ring_mem + (psp_write_ptr_reg /
> (sizeof(struct psp_gfx_rb_frame) / 4));
> 
>   /* Update KM RB frame */
> - write_frame->cmd_buf_addr_hi = (unsigned
> int)(cmd_buf_mc_addr >> 32);
> - write_frame->cmd_buf_addr_lo = (unsigned
> int)(cmd_buf_mc_addr);
> - write_frame->fence_addr_hi = (unsigned int)(fence_mc_addr >> 32);
> - write_frame->fence_addr_lo = (unsigned int)(fence_mc_addr);
> + write_frame->cmd_buf_addr_hi =
> upper_32_bits(cmd_buf_mc_addr);
> + write_frame->cmd_buf_addr_lo =
> lower_32_bits(cmd_buf_mc_addr);
> + write_frame->fence_addr_hi = upper_32_bits(fence_mc_addr);
> + write_frame->fence_addr_lo = lower_32_bits(fence_mc_addr);
>   write_frame->fence_value = index;
> 
>   /* Update the write Pointer in DWORDs */ diff --git
> a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> index 6e5c6ed..c98d77d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> @@ -254,8 +254,8 @@ int psp_v3_1_prep_cmd_buf(struct
> amdgpu_firmware_info *ucode, struct psp_gfx_cmd
>   memset(cmd, 0, sizeof(struct psp_gfx_cmd_resp));
> 
>   cmd->cmd_id = GFX_CMD_ID_LOAD_IP_FW;
> - cmd->cmd.cmd_load_ip_fw.fw_phy_addr_lo =
> (uint32_t)fw_mem_mc_addr;
> - cmd->cmd.cmd_load_ip_fw.fw_phy_addr_hi =
> (uint32_t)((uint64_t)fw_mem_mc_addr >> 32);
> + cmd->cmd.cmd_load_ip_fw.fw_phy_addr_lo =
> lower_32_bits(fw_mem_mc_addr);
> + cmd->cmd.cmd_load_ip_fw.fw_phy_addr_hi =
> +upper_32_bits(fw_mem_mc_addr);
>   cmd->cmd.cmd_load_ip_fw.fw_size = ucode->ucode_size;
> 
>   ret = psp_v3_1_get_fw_type(ucode, 
> >cmd.cmd_load_ip_fw.fw_type);
> @@ -375,10 +375,10 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
>   memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
> 
>   /* Update KM RB frame */
> - write_frame->cmd_buf_addr_hi = (unsigned
> int)(cmd_buf_mc_addr >> 3

RE: [PATCH libdrm v6 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

> what is the purpose of the file version
The initial purpose is to act like a version tag, not necessarily format 
related. 

Sam

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Sunday, June 04, 2017 10:10 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v6 1/1] amdgpu: move asic id table to a separate file

On 01/06/17 05:22 AM, Samuel Li wrote:
> From: Xiaojie Yuan <xiaojie.y...@amd.com>
> 
> v2: fix an off by one error and leading white spaces
> v3: use thread safe strtok_r(); initialize len before calling getline();
> change printf() to drmMsg(); add initial amdgpu.ids
> v4: integrate some recent internal changes, including format changes
> v5: fix line number for empty/commented lines; realloc to save memory; 
> indentation changes
> v6: remove a line error
> 
> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
> Reviewed-by: Junwei Zhang <jerry.zh...@amd.com>
> Signed-off-by: Samuel Li <samuel...@amd.com>

[...]

> + /* 1st valid line is file version */
> + while ((n = getline(, , fp)) != -1) {
> + /* trim trailing newline */
> + if (line[n - 1] == '\n')
> + line[n - 1] = '\0';
> +
> + /* ignore empty line and commented line */
> + if (strlen(line) == 0 || line[0] == '#') {
> + line_num++;
> + continue;
> + }
> +
> + drmMsg("%s version: %s\n", AMDGPU_ASIC_ID_TABLE, line);
> + break;
> + }

BTW, what is the purpose of the file version? If it's about the format of the 
file, we need to check here that the file has a format we can parse, something 
like

if ( != 1)
return -EINVAL;


Note that making backwards incompatible changes to the file format would pretty 
much kill the idea of updating the file with a script like update-pciids.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

> Another possibility is to simply put it in the toplevel directory.
I considered that too, but I prefer not to pollute the toplevel.

> It shouldn't take as much time as we've spent talking about it in this 
> thread. :}
Right, I meant you can go ahead if you would like to verify the names.

Sam

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Thursday, June 01, 2017 11:06 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

On 01/06/17 11:27 PM, Li, Samuel wrote:
>> If amdgpu.ids living in the amdgpu directory prevents it from being used by 
>> libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
>> "data".
>>> README is also located in this directory.
>> Not the same thing. It documents things about the header files, and doesn't 
>> get installed anywhere.
> I think that is a personal preference.

If you're referring to naming the new directory "data", that's just a 
suggestion. Another possibility is to simply put it in the toplevel directory.

What I wrote about include/drm/README is easily verifiable fact.


>>> My preference is to pass the names only, not to audit from a coder's 
>>> view ...
>> That's not how we do things.
> It is a data file, not really a part of code.

There is nothing magic about it. It's subject to the review process just like 
any other file in the repository.

BTW, if you put the file outside of the amdgpu directory, you should send the 
patch to the dri-devel mailing list for review as well.


> It shall be your preference to decide how much time you would like to 
> spend in auditing the names :)

It shouldn't take as much time as we've spent talking about it in this thread. 
:}


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

>If amdgpu.ids living in the amdgpu directory prevents it from being used by 
>libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
>"data".
>> README is also located in this directory.
> Not the same thing. It documents things about the header files, and doesn't 
> get installed anywhere.
I think that is a personal preference.

>> My preference is to pass the names only, not to audit from a coder's 
>> view ...
>That's not how we do things.
It is a data file, not really a part of code. It shall be your preference to 
decide how much time you would like to spend in auditing the names :)

Sam


-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Thursday, June 01, 2017 2:09 AM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

On 01/06/17 12:32 AM, Li, Samuel wrote:
>> From: Michel Dänzer [mailto:mic...@daenzer.net] On 31/05/17 07:31 AM, 
>> Li, Samuel wrote:
>>> From: Michel Dänzer [mailto:mic...@daenzer.net]
>>>> On 30/05/17 06:16 AM, Samuel Li wrote:
>>>>
>>>>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
>>>>> file mode 100644 index 000..a43ca33
>>>>> --- /dev/null
>>>>> +++ b/amdgpu/amdgpu_asic_id.c
>>>>
>>>> [...]
>>>>
>>>>> +#include "xf86drm.h"
>>>>> +#include "amdgpu_drm.h"
>>>>
>>>> Should be
>>>>
>>>> #include 
>>>> #include 
>>>>
>>>> since these header files are not located in the same directory as 
>>>> amdgpu_asic_id.c.
>>>
>>>  [Sam] Actually, "" is used to include programmer-defined header 
>>> files, and <>  is used for files pre-designated by the compiler/IDE.
>> 
>> The only difference between the two is that #include "" first looks for the 
>> header file in the same directory where the file containing the #include 
>> directive (not necessarily the same as the original *.c file passed to the 
>> compiler/preprocessor) is located, after that it looks in the same paths in 
>> the same order as <>. So "" only really makes sense when the header file is 
>> in the same directory as the file including it.
> 
> [Sam] Please see here
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

Thanks, I didn't know different search paths can apply with "" vs <>.
One learns something new every day. :) You can ignore the above then.


>>>>> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids new 
>>>>> file mode 100644 index 000..6d6b944
>>>>> --- /dev/null
>>>>> +++ b/include/drm/amdgpu.ids
>>>>
>>>> I think the path of this file in the repository should be 
>>>> amdgpu/amdgpu.ids rather than include/drm/amdgpu.ids.
>>>
>>> [Sam] The file is going to be shared with radeon.
>> 
>> We can cross that bridge when we get there. Meanwhile, it's not a header 
>> file and not installed under $prefix/include/, so it doesn't belong in 
>> include/.
> [Sam] I am planning to do it right after this.

If amdgpu.ids living in the amdgpu directory prevents it from being used by 
libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
"data".

> README is also located in this directory.

Not the same thing. It documents things about the header files, and doesn't get 
installed anywhere.


>>>>> @@ -0,0 +1,170 @@
>>>>> +# List of AMDGPU ID's
>>>>
>>>> This should say "IDs" instead of "ID's".
>>>>
>>>>
>>>>> +67FF,CF, 67FF:CF
>>>>> +67FF,EF, 67FF:EF
>>>>
>>>> There should be no such dummy entries in the file. If it's useful, 
>>>> amdgpu_get_marketing_name can return a dummy string based on the 
>>>> PCI ID and revision when there's no matching entry in the file.
>>>
>>> [Sam] I forwarded another thread to you.
>> 
>> Please make your argument explicitly, for the benefit of non-AMD readers of 
>> the amd-gfx list.
>> Anyway, I don't think that invalidates what I wrote, and Alex seems to 
>> agree. "67FF:CF" isn't a marketing name, so there should be no such entries 
>> in this file. It's not necessary anyway; assuming it's useful for 
>> amdgpu_get_marketing_name to return such "names", it

RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

Please see comments incline,

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Wednesday, May 31, 2017 1:15 AM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

On 31/05/17 07:31 AM, Li, Samuel wrote:
> From: Michel Dänzer [mailto:mic...@daenzer.net]
>> On 30/05/17 06:16 AM, Samuel Li wrote:
>> 
>>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
>>> file mode 100644 index 000..a43ca33
>>> --- /dev/null
>>> +++ b/amdgpu/amdgpu_asic_id.c
>> 
>> [...]
>> 
>>> +#include "xf86drm.h"
>>> +#include "amdgpu_drm.h"
>> 
>> Should be
>> 
>> #include 
>> #include 
>> 
>> since these header files are not located in the same directory as 
>> amdgpu_asic_id.c.
> 
>  [Sam] Actually, "" is used to include programmer-defined header 
> files, and <>  is used for files pre-designated by the compiler/IDE.

The only difference between the two is that #include "" first looks for the 
header file in the same directory where the file containing the #include 
directive (not necessarily the same as the original *.c file passed to the 
compiler/preprocessor) is located, after that it looks in the same paths in the 
same order as <>. So "" only really makes sense when the header file is in the 
same directory as the file including it.

[Sam] Please see here
https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html


>>> @@ -267,6 +274,11 @@ int amdgpu_device_initialize(int fd,
>>> amdgpu_vamgr_init(>vamgr_32, start, max,
>>>   dev->dev_info.virtual_address_alignment);
>>>  
>>> +   r = amdgpu_parse_asic_ids(>asic_ids);
>>> +   if (r)
>>> +   fprintf(stderr, "%s: Can not parse asic ids, 0x%x.",
>>> +   __func__, r);
>> 
>> "Cannot parse ASIC IDs"
>> 
>> Also, there should be curly braces around a multi-line statement.
> 
> [Sam] Can be done. However, it is still a single statement. Does it matter?

It might not be strictly required, but I think it does make the code clearer in 
this case.

[Sam] OK.

>>> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids new 
>>> file mode 100644 index 000..6d6b944
>>> --- /dev/null
>>> +++ b/include/drm/amdgpu.ids
>> 
>> I think the path of this file in the repository should be 
>> amdgpu/amdgpu.ids rather than include/drm/amdgpu.ids.
> 
> [Sam] The file is going to be shared with radeon.

We can cross that bridge when we get there. Meanwhile, it's not a header file 
and not installed under $prefix/include/, so it doesn't belong in include/.
[Sam] I am planning to do it right after this. README is also located in this 
directory.


>>> @@ -0,0 +1,170 @@
>>> +# List of AMDGPU ID's
>> 
>> This should say "IDs" instead of "ID's".
>> 
>> 
>>> +67FF,  CF, 67FF:CF
>>> +67FF,  EF, 67FF:EF
>> 
>> There should be no such dummy entries in the file. If it's useful, 
>> amdgpu_get_marketing_name can return a dummy string based on the PCI 
>> ID and revision when there's no matching entry in the file.
> 
> [Sam] I forwarded another thread to you.

Please make your argument explicitly, for the benefit of non-AMD readers of the 
amd-gfx list.
Anyway, I don't think that invalidates what I wrote, and Alex seems to agree. 
"67FF:CF" isn't a marketing name, so there should be no such entries in this 
file. It's not necessary anyway; assuming it's useful for 
amdgpu_get_marketing_name to return such "names", it can generate them on the 
fly when there is no matching entry in the file.
Ideally the issues above should be fixed in the original file we get from 
marketing (?), but meanwhile / failing that we should fix them up (and can 
easily with Git).

[Sam] Essentially marketing names are defined by Marketing. They are 
complicated as I can imagine. If you have questions regarding the names, the 
thread I forwarded has the contact you can use.
IIRC, the hex format in marketing names has been used for very long time. My 
preference is to pass the names only, not to audit from a coder's view ... that 
can make discussions much much more difficult.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

Please see comments inline.

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Monday, May 29, 2017 9:26 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

On 30/05/17 06:16 AM, Samuel Li wrote:
> From: Xiaojie Yuan <xiaojie.y...@amd.com>

I took a closer look and noticed some details (and some non-details about the 
amdgpu.ids file at the end).


> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
> file mode 100644 index 000..a43ca33
> --- /dev/null
> +++ b/amdgpu/amdgpu_asic_id.c

[...]

> +#include "xf86drm.h"
> +#include "amdgpu_drm.h"

Should be

#include 
#include 

since these header files are not located in the same directory as
amdgpu_asic_id.c.

 [Sam] Actually, "" is used to include programmer-defined header files, and <>  
is used for files pre-designated by the compiler/IDE.

> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
> +{
> + char *buf, *saveptr;
> + char *s_did;
> + char *s_rid;
> + char *s_name;
> + char *endptr;
> + int r = 0;

This function could be simplified slightly by initializing r = -EINVAL
here and only setting it to 0 just before the out label.

[Sam]This is likely a personal preference. I am fine with current 
implementation which is clear.

> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> +{

[...]

> + /* 1st valid line is file version */
> + while ((n = getline(, , fp)) != -1) {
> + /* trim trailing newline */
> + if (line[n - 1] == '\n')
> + line[n - 1] = '\0';

Should probably increment line_num here, otherwise the line number in
the error message below might be confusing.

[Sam]That is a good catch.

> + fprintf(stderr, "Invalid format: %s: line %d: %s\n",
> + AMDGPU_ASIC_ID_TABLE, line_num, line);

The second line should be indented to align with the opening parenthesis.

 [Sam] Can be done.

> + if (table_size >= table_max_size) {
> + /* double table size */
> + table_max_size *= 2;
> + asic_id_table = realloc(asic_id_table, table_max_size *
> + sizeof(struct amdgpu_asic_id));

Ditto.

[Sam] Can be done.

> + /* end of table */
> + id = asic_id_table + table_size;
> + memset(id, 0, sizeof(struct amdgpu_asic_id));

Might also want to realloc asic_id_table according to the final table
size, to avoid wasting memory.

[Sam] Good one.

> + if (r && asic_id_table) {
> + while (table_size--) {
> + id = asic_id_table + table_size;
> + if (id->marketing_name !=  NULL)
> + free(id->marketing_name);

free(NULL) works fine (and parse_one_line returns an error for
id->marketing_name == NULL anyway), so this can be simplified to

free(id->marketing_name);


[Sam] Can be done.

> @@ -140,6 +140,13 @@ static void 
> amdgpu_device_free_internal(amdgpu_device_handle dev)
>   close(dev->fd);
>   if ((dev->flink_fd >= 0) && (dev->fd != dev->flink_fd))
>   close(dev->flink_fd);
> + if (dev->asic_ids) {
> + for (id = dev->asic_ids; id->did; id++) {
> + if (id->marketing_name !=  NULL)
> + free(id->marketing_name);
> + }

Ditto, this can be simplified to

[Sam] Can be done.

for (id = dev->asic_ids; id->did; id++)
free(id->marketing_name);


> @@ -267,6 +274,11 @@ int amdgpu_device_initialize(int fd,
>   amdgpu_vamgr_init(>vamgr_32, start, max,
> dev->dev_info.virtual_address_alignment);
>  
> + r = amdgpu_parse_asic_ids(>asic_ids);
> + if (r)
> + fprintf(stderr, "%s: Can not parse asic ids, 0x%x.",
> + __func__, r);

"Cannot parse ASIC IDs"

Also, there should be curly braces around a multi-line statement.

[Sam] Can be done. However, it is still a single statement. Does it matter?

> @@ -297,13 +309,15 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
>  
>  const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
>  {
> - const struct amdgpu_asic_id_table_t *t = amdgpu_asic_id_table;
> + const struct amdgpu_asic_id *id;
> +
> + if (!dev->asic_ids)
> + return NULL;
>  
> - while (t->

RE: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

>  - Marketing can make mistakes or have IT glitches The inconsistent use of 
> "(TM)" and using a 67C2:00 is something one wants to double-check with them.
Marketing names are there for a lot of reasons. The code here is to pass the 
names only.
If you are interested in a vendor's marketing names, please reach out to the 
vendor, e.g. through your contacts in the vendor who also shares your interest.

>- Having a separate file so that clients can update/edit it does not help 
>much. 
Please say it to pci.ids/usb.ids :)

> Adding ~200 loc for ~170 devices entries sounds like a step in the wrong 
> direction.
Check the vendor's entries in pci.ids, and you might have some better idea.

Sam

-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com] 
Sent: Tuesday, May 30, 2017 7:01 AM
To: Li, Samuel <samuel...@amd.com>
Cc: Alex Deucher <alexdeuc...@gmail.com>; amd-gfx list 
<amd-gfx@lists.freedesktop.org>; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

Hi all,

Pardon for dropping in uninvited. Just some food for thought.

On 29 May 2017 at 22:01, Li, Samuel <samuel...@amd.com> wrote:
> Understood your point. However as discussed internally before, marketing 
> names are there for a lot of reasons; my understanding of the policy is we do 
> not need to touch them as long as there is no error in the names and they are 
> allowed to be public.
>
Samuel,

It seems that most comments put forward by people are going on deaf ears.

While there may be valid arguments behind doing so, do consider the following:
 - Review is always encouraged
Regardless if the information is within or outside of the source code.
 - Marketing can make mistakes or have IT glitches The inconsistent use of 
"(TM)" and using a 67C2:00 is something one wants to double-check with them.
 - Having a separate file so that clients can update/edit it does not help much.
You want to ship the whole driver, in order to have a predictable and 
consistent user experience.
 - Adding ~200 loc for ~170 devices entries sounds like a step in the wrong 
direction.

In either case, not my call. I might follow-up with some issues in the code 
itself ;-)

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

Recently there are some changes internally, I can integrate them into this one.
There are still discussions on going regarding the format.

Sam

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Sunday, May 28, 2017 10:11 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

On 27/05/17 04:23 AM, Samuel Li wrote:
> 
> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids new file 
> mode 100644 index 000..1b00b60
> --- /dev/null
> +++ b/include/drm/amdgpu.ids
> @@ -0,0 +1,154 @@
> +1.0.0
> +6600,0,AMD Radeon HD 8600/8700M

This doesn't look like the current format we've settled on internally.
There are supposed to be tabs after the commas to align the columns.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

Understood your point. However as discussed internally before, marketing names 
are there for a lot of reasons; my understanding of the policy is we do not 
need to touch them as long as there is no error in the names and they are 
allowed to be public.

Regards,
Sam

-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com] 
Sent: Friday, May 26, 2017 3:27 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Yuan, Xiaojie 
<xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

On Fri, May 26, 2017 at 3:23 PM, Samuel Li <samuel...@amd.com> wrote:
> From: Xiaojie Yuan <xiaojie.y...@amd.com>
>
> v2: fix an off by one error and leading white spaces
> v3: use thread safe strtok_r(); initialize len before calling getline();
> change printf() to drmMsg(); add initial amdgpu.ids
>
> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
> Reviewed-by: Junwei Zhang <jerry.zh...@amd.com>
> Signed-off-by: Samuel Li <samuel...@amd.com>
> ---
>  Makefile.am  |   3 +
>  amdgpu/Makefile.am   |   2 +
>  amdgpu/Makefile.sources  |   2 +-
>  amdgpu/amdgpu_asic_id.c  | 199 
> +++
>  amdgpu/amdgpu_asic_id.h  | 165 ---
>  amdgpu/amdgpu_device.c   |  28 +--
>  amdgpu/amdgpu_internal.h |  10 +++
>  include/drm/amdgpu.ids   | 154 
>  8 files changed, 390 insertions(+), 173 deletions(-)  create mode 
> 100644 amdgpu/amdgpu_asic_id.c  delete mode 100644 
> amdgpu/amdgpu_asic_id.h  create mode 100644 include/drm/amdgpu.ids
>
> diff --git a/Makefile.am b/Makefile.am index dfb8fcd..8de8f6c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -45,6 +45,9 @@ AM_DISTCHECK_CONFIGURE_FLAGS = \
>
>  pkgconfigdir = @pkgconfigdir@
>  pkgconfig_DATA = libdrm.pc
> +libdrmdatadir = $(datadir)/libdrm
> +dist_libdrmdata_DATA = include/drm/amdgpu.ids export libdrmdatadir
>
>  if HAVE_LIBKMS
>  LIBKMS_SUBDIR = libkms
> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am index 
> cf7bc1b..da71c1c 100644
> --- a/amdgpu/Makefile.am
> +++ b/amdgpu/Makefile.am
> @@ -30,6 +30,8 @@ AM_CFLAGS = \
> $(PTHREADSTUBS_CFLAGS) \
> -I$(top_srcdir)/include/drm
>
> +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\"
> +
>  libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la  libdrm_amdgpu_ladir 
> = $(libdir)  libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 
> -no-undefined diff --git a/amdgpu/Makefile.sources 
> b/amdgpu/Makefile.sources index 487b9e0..bc3abaa 100644
> --- a/amdgpu/Makefile.sources
> +++ b/amdgpu/Makefile.sources
> @@ -1,5 +1,5 @@
>  LIBDRM_AMDGPU_FILES := \
> -   amdgpu_asic_id.h \
> +   amdgpu_asic_id.c \
> amdgpu_bo.c \
> amdgpu_cs.c \
> amdgpu_device.c \
> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
> file mode 100644 index 000..5b415e3
> --- /dev/null
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright © 2017 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person 
> +obtaining a
> + * copy of this software and associated documentation files (the 
> +"Software"),
> + * to deal in the Software without restriction, including without 
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute, 
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom 
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be 
> +included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> +SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
> +DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
> +OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE 
> +OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "xf86drm.h"
> +#include "amdgpu_drm.h"
> +#inc

RE: [PATCH 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

>There's no need for a separate repo upstream.  It's purely to aid internal 
>packaging.
As a first step, we can put a snapshot in libdrm for now. 
External packaging  will face the same issue though ... the packagers need to 
put the ids in various versions of various distros. We also likely need to 
automate the upstream update to save sometime for ourselves.

Sam
-Original Message-
From: Deucher, Alexander 
Sent: Tuesday, May 23, 2017 8:12 AM
To: 'Michel Dänzer' <mic...@daenzer.net>; Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: RE: [PATCH 1/1] amdgpu: move asic id table to a separate file

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf 
> Of Michel Dänzer
> Sent: Thursday, May 11, 2017 8:50 PM
> To: Li, Samuel
> Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie
> Subject: Re: [PATCH 1/1] amdgpu: move asic id table to a separate file
> 
> On 12/05/17 06:13 AM, Li, Samuel wrote:
> > Submitted a request to create a new repo on freedesktop.
> 
> What's the point of having a separate repository upstream? Can't we 
> just keep it in the libdrm repository?

There's no need for a separate repo upstream.  It's purely to aid internal 
packaging.

Alex

> 
> 
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

>OK then, how will users that compile the OSS stack know where to get that file 
>(or that they need some new file at all) after your patch is applied?
There is a discussion regarding whether the ids file shall be separate 
upstream. This question can be answer after that ... previously we were 
thinking a fixed path /usr/share/libdrm.

> Libraries can't print to stdout as it will break all programs that parse the 
> output, apitrace is one such example.
That sounds a good practice.

Sam

-Original Message-
From: Grazvydas Ignotas [mailto:nota...@gmail.com] 
Sent: Sunday, May 14, 2017 1:46 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file

On Sat, May 13, 2017 at 12:50 AM, Li, Samuel <samuel...@amd.com> wrote:
>> If you add this here, you should add the ids file itself and make libdrm 
>> install it too...
> ? Here the ids file is separate from libdrm. It is passed during compilation 
> so that libdrm knows where to get it.

OK then, how will users that compile the OSS stack know where to get that file 
(or that they need some new file at all) after your patch is applied?

>>> +   printf("%s version: %s\n", AMDGPU_ASIC_ID_TABLE,  
>>> + line);
>>debug leftover?
> This is to print out the ids file version.

Libraries can't print to stdout as it will break all programs that parse the 
output, apitrace is one such example.

>
> Thanks,
> Sam
>
> -Original Message-----
> From: Grazvydas Ignotas [mailto:nota...@gmail.com]
> Sent: Friday, May 12, 2017 8:16 AM
> To: Li, Samuel <samuel...@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie 
> <xiaojie.y...@amd.com>
> Subject: Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a 
> separate file
>
> On Fri, May 12, 2017 at 12:19 AM, Samuel Li <samuel...@amd.com> wrote:
>> From: Xiaojie Yuan <xiaojie.y...@amd.com>
>>
>> v2: fix an off by one error and leading white spaces
>>
>> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
>> Reviewed-by: Junwei Zhang <jerry.zh...@amd.com>
>> Signed-off-by: Samuel Li <samuel...@amd.com>
>> ---
>>  amdgpu/Makefile.am   |   2 +
>>  amdgpu/Makefile.sources  |   2 +-
>>  amdgpu/amdgpu_asic_id.c  | 198
>> +++
>>  amdgpu/amdgpu_asic_id.h  | 165 ---
>>  amdgpu/amdgpu_device.c   |  28 +--
>>  amdgpu/amdgpu_internal.h |  10 +++
>>  6 files changed, 232 insertions(+), 173 deletions(-)  create mode
>> 100644 amdgpu/amdgpu_asic_id.c  delete mode 100644 
>> amdgpu/amdgpu_asic_id.h
>>
>> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am index
>> cf7bc1b..ecf9e82 100644
>> --- a/amdgpu/Makefile.am
>> +++ b/amdgpu/Makefile.am
>> @@ -30,6 +30,8 @@ AM_CFLAGS = \
>> $(PTHREADSTUBS_CFLAGS) \
>> -I$(top_srcdir)/include/drm
>>
>> +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${datadir}/libdrm/amdgpu.ids\"
>
> If you add this here, you should add the ids file itself and make libdrm 
> install it too...
>
>> +
>>  libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la  libdrm_amdgpu_ladir 
>> = $(libdir)  libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 
>> -no-undefined diff --git a/amdgpu/Makefile.sources 
>> b/amdgpu/Makefile.sources index 487b9e0..bc3abaa 100644
>> --- a/amdgpu/Makefile.sources
>> +++ b/amdgpu/Makefile.sources
>> @@ -1,5 +1,5 @@
>>  LIBDRM_AMDGPU_FILES := \
>> -   amdgpu_asic_id.h \
>> +   amdgpu_asic_id.c \
>> amdgpu_bo.c \
>> amdgpu_cs.c \
>> amdgpu_device.c \
>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
>> file mode 100644 index 000..067f38c
>> --- /dev/null
>> +++ b/amdgpu/amdgpu_asic_id.c
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright © 2017 Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> +obtaining a
>> + * copy of this software and associated documentation files (the 
>> +"Software"),
>> + * to deal in the Software without restriction, including without 
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyr

RE: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

> If you add this here, you should add the ids file itself and make libdrm 
> install it too...
? Here the ids file is separate from libdrm. It is passed during compilation so 
that libdrm knows where to get it. 

> You can't use strtok() in a library. Any other thread may call strtok() 
> anytime too and screw you up.
Right, better to call strtok_r() instead.

> The manpage says len must be 0 before the first getline() call.
Right, that shall be safer.

>> +   printf("%s version: %s\n", AMDGPU_ASIC_ID_TABLE,  line);
>debug leftover?
This is to print out the ids file version.

Thanks,
Sam

-Original Message-
From: Grazvydas Ignotas [mailto:nota...@gmail.com] 
Sent: Friday, May 12, 2017 8:16 AM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file

On Fri, May 12, 2017 at 12:19 AM, Samuel Li <samuel...@amd.com> wrote:
> From: Xiaojie Yuan <xiaojie.y...@amd.com>
>
> v2: fix an off by one error and leading white spaces
>
> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
> Reviewed-by: Junwei Zhang <jerry.zh...@amd.com>
> Signed-off-by: Samuel Li <samuel...@amd.com>
> ---
>  amdgpu/Makefile.am   |   2 +
>  amdgpu/Makefile.sources  |   2 +-
>  amdgpu/amdgpu_asic_id.c  | 198 
> +++
>  amdgpu/amdgpu_asic_id.h  | 165 ---
>  amdgpu/amdgpu_device.c   |  28 +--
>  amdgpu/amdgpu_internal.h |  10 +++
>  6 files changed, 232 insertions(+), 173 deletions(-)  create mode 
> 100644 amdgpu/amdgpu_asic_id.c  delete mode 100644 
> amdgpu/amdgpu_asic_id.h
>
> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am index 
> cf7bc1b..ecf9e82 100644
> --- a/amdgpu/Makefile.am
> +++ b/amdgpu/Makefile.am
> @@ -30,6 +30,8 @@ AM_CFLAGS = \
> $(PTHREADSTUBS_CFLAGS) \
> -I$(top_srcdir)/include/drm
>
> +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${datadir}/libdrm/amdgpu.ids\"

If you add this here, you should add the ids file itself and make libdrm 
install it too...

> +
>  libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la  libdrm_amdgpu_ladir 
> = $(libdir)  libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 
> -no-undefined diff --git a/amdgpu/Makefile.sources 
> b/amdgpu/Makefile.sources index 487b9e0..bc3abaa 100644
> --- a/amdgpu/Makefile.sources
> +++ b/amdgpu/Makefile.sources
> @@ -1,5 +1,5 @@
>  LIBDRM_AMDGPU_FILES := \
> -   amdgpu_asic_id.h \
> +   amdgpu_asic_id.c \
> amdgpu_bo.c \
> amdgpu_cs.c \
> amdgpu_device.c \
> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
> file mode 100644 index 000..067f38c
> --- /dev/null
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -0,0 +1,198 @@
> +/*
> + * Copyright © 2017 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person 
> +obtaining a
> + * copy of this software and associated documentation files (the 
> +"Software"),
> + * to deal in the Software without restriction, including without 
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute, 
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom 
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be 
> +included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> +SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
> +DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
> +OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE 
> +OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "amdgpu_drm.h"
> +#include "amdgpu_internal.h"
> +
> +static int parse_one_line(const char *line, struct amdgpu_asic_id 
> +*id) {
> +   char *buf;
> +   char *s_did;
> +   char *s_rid;
> +   char *s_name;
> +   char *endptr;
> +   int r = 0;
> +
> +   buf = strdup(line

RE: [PATCH 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

My understanding is this is actually a data file. Similar to amdgpu firmware, 
which is also separate from the kernel source code.

Sam


-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Thursday, May 11, 2017 8:50 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH 1/1] amdgpu: move asic id table to a separate file

On 12/05/17 06:13 AM, Li, Samuel wrote:
> Submitted a request to create a new repo on freedesktop.

What's the point of having a separate repository upstream? Can't we just keep 
it in the libdrm repository?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

The bugzilla id is actually here,
https://bugs.freedesktop.org/show_bug.cgi?id=101013


Sam

-Original Message-
From: Li, Samuel 
Sent: Thursday, May 11, 2017 5:13 PM
To: 'Michel Dänzer' <mic...@daenzer.net>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: RE: [PATCH 1/1] amdgpu: move asic id table to a separate file

Submitted a request to create a new repo on freedesktop. Michel, do you have 
the privilege to create it?
https://bugs.freedesktop.org/show_bug.cgi?id=99589

Sam

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net]
Sent: Wednesday, May 10, 2017 10:33 PM
To: Li, Samuel <samuel...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <xiaojie.y...@amd.com>
Subject: Re: [PATCH 1/1] amdgpu: move asic id table to a separate file

On 11/05/17 06:10 AM, Li, Samuel wrote:
> Also attach a sample ids file for reference. The names are from 
> marketing, not related to source code and no reviews necessary here:)

Just because it's not source code doesn't mean no review is necessary. :)


> It can be put in directory /usr/share/libdrm.

What is the canonical location where distros or users building upstream libdrm 
can get this file from? There needs to be a good solution for that before this 
can land.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

Good catch!

Sam

From: Yuan, Xiaojie
Sent: Thursday, May 11, 2017 6:51 AM
To: Li, Samuel <samuel...@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/1] amdgpu: move asic id table to a separate file


Hi Samuel,



Here's an off-by-one error:

+   id = asic_id_table + table_size -1;



should be:

+   id = asic_id_table + table_size;



Regards,

Xiaojie


From: Li, Samuel
Sent: Thursday, May 11, 2017 4:56:55 AM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Cc: Yuan, Xiaojie; Li, Samuel
Subject: [PATCH 1/1] amdgpu: move asic id table to a separate file

From: Xiaojie Yuan <xiaojie.y...@amd.com<mailto:xiaojie.y...@amd.com>>

Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
Signed-off-by: Samuel Li <samuel...@amd.com<mailto:samuel...@amd.com>>
---
 amdgpu/Makefile.am   |   2 +
 amdgpu/Makefile.sources  |   2 +-
 amdgpu/amdgpu_asic_id.c  | 198 +++
 amdgpu/amdgpu_asic_id.h  | 165 ---
 amdgpu/amdgpu_device.c   |  28 +--
 amdgpu/amdgpu_internal.h |  10 +++
 6 files changed, 232 insertions(+), 173 deletions(-)
 create mode 100644 amdgpu/amdgpu_asic_id.c
 delete mode 100644 amdgpu/amdgpu_asic_id.h

diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am
index cf7bc1b..ecf9e82 100644
--- a/amdgpu/Makefile.am
+++ b/amdgpu/Makefile.am
@@ -30,6 +30,8 @@ AM_CFLAGS = \
 $(PTHREADSTUBS_CFLAGS) \
 -I$(top_srcdir)/include/drm

+AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${datadir}/libdrm/amdgpu.ids\"
+
 libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la
 libdrm_amdgpu_ladir = $(libdir)
 libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 -no-undefined
diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
index 487b9e0..bc3abaa 100644
--- a/amdgpu/Makefile.sources
+++ b/amdgpu/Makefile.sources
@@ -1,5 +1,5 @@
 LIBDRM_AMDGPU_FILES := \
-   amdgpu_asic_id.h \
+   amdgpu_asic_id.c \
 amdgpu_bo.c \
 amdgpu_cs.c \
 amdgpu_device.c \
diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
new file mode 100644
index 000..d50e21a
--- /dev/null
+++ b/amdgpu/amdgpu_asic_id.c
@@ -0,0 +1,198 @@
+/*
+ * Copyright (c) 2017 Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "amdgpu_drm.h"
+#include "amdgpu_internal.h"
+
+static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
+{
+   char *buf;
+   char *s_did;
+   char *s_rid;
+   char *s_name;
+   char *endptr;
+   int r = 0;
+
+   buf = strdup(line);
+   if (!buf)
+   return -ENOMEM;
+
+   /* ignore empty line and commented line */
+   if (strlen(line) == 0 || line[0] == '#') {
+   r = -EAGAIN;
+   goto out;
+   }
+
+   /* device id */
+   s_did = strtok(buf, ",");
+   if (!s_did) {
+   r = -EINVAL;
+   goto out;
+   }
+
+   id->did = strtol(s_did, , 16);
+   if (*endptr) {
+   r = -EINVAL;
+   goto out;
+   }
+
+   /* revision id */
+   s_rid = strtok(NULL, ",");
+   if (!s_rid) {
+   r = -EINVAL;
+   goto out;
+   }
+
+   id->rid = strtol(s_rid, , 16);
+   if (*endptr) {
+   r = -EINVAL;
+   goto out;
+   }
+
+   /* marketing name */
+   s_name = strtok(NULL, ",");
+   if (!s_name) {
+   r = -EINVAL;
+   goto out;
+   }
+
+   id->marketing_name = strdup(s_name);
+   if (id->marketing

RE: [PATCH 2/3] drm/radeon: Make SI support in Radeon conditional

[Li, Samuel]

My understanding is that SI support in AMDGPU is limited. 
So maybe here it can be the other way around, that by default SI support in 
AMDGPU shall be disabled.

Sam


-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of Felix 
Kuehling
Sent: Friday, April 07, 2017 4:16 PM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix 
Subject: [PATCH 2/3] drm/radeon: Make SI support in Radeon conditional

Advertise SI PCI IDs only when they are not supported by amdgpu.
Use the CONFIG_DRM_AMDGPU_SI to check so that a single option in the kernel 
config keeps both drivers in sync.

This is the simplest possible change. A more complete solution may want to 
conditionally disable more SI-specific code in the Radeon driver.

Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/radeon/Kconfig  |  12 +++
 drivers/gpu/drm/radeon/radeon_drv.c |   3 +
 include/drm/drm_pciids.h| 146 ++--
 3 files changed, 89 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig 
index 86bbac8..2e66198 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -1,3 +1,15 @@
+config DRM_RADEON_FORCE_SI
+   bool "Enable radeon support for SI parts even when amdgpu supports it"
+   depends on DRM_RADEON && DRM_AMDGPU_SI
+   default n
+   help
+ Choose this option if you want to enable support for SI ASICs
+ in the radeon driver even when CONFIG_DRM_AMDGPU_SI is enabled.
+
+ This option is meant for experimentation and testing. In a
+ production system only one driver should claim support for the
+ same ASIC.
+
 config DRM_RADEON_FORCE_CIK
bool "Enable radeon support for CIK parts even when amdgpu supports it"
depends on DRM_RADEON && DRM_AMDGPU_CIK diff --git 
a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index fda6411..235c324 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -305,6 +305,9 @@ static inline void radeon_unregister_atpx_handler(void) {}  
#if !defined(CONFIG_DRM_AMDGPU_CIK) || defined(CONFIG_DRM_RADEON_FORCE_CIK)
radeon_CIK_PCI_IDS,
 #endif
+#if !defined(CONFIG_DRM_AMDGPU_SI) || defined(CONFIG_DRM_RADEON_FORCE_SI)
+   radeon_CIK_PCI_IDS,
+#endif
radeon_PCI_IDS
 };
 
diff --git a/include/drm/drm_pciids.h b/include/drm/drm_pciids.h index 
cf17901..b42179f 100644
--- a/include/drm/drm_pciids.h
+++ b/include/drm/drm_pciids.h
@@ -1,3 +1,77 @@
+#define radeon_SI_PCI_IDS \
+   {0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6784, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6788, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x678A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6790, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6791, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6792, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6798, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6799, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x679A, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x679B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x679E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x679F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_TAHITI|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6800, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_IS_MOBILITY|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6801, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_IS_MOBILITY|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6802, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_IS_MOBILITY|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6806, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6808, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6809, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6810, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6811, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6816, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6817, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6818, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
CHIP_PITCAIRN|RADEON_NEW_MEMMAP}, \
+   {0x1002, 0x6819, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 

RE: [PATCH] Revert "drm/amd/amdgpu: Disable GFX_PG on Carrizo until compute issues solved"

[Li, Samuel]

Patches is:
Reviewed-by: Samuel  Li <samuel...@amd.com>

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of Alex 
Deucher
Sent: Friday, March 17, 2017 3:29 PM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>
Subject: [PATCH] Revert "drm/amd/amdgpu: Disable GFX_PG on Carrizo until 
compute issues solved"

Re-enable GFX PG.  It's working properly with MEC now that KIQ is enabled.

This reverts commit e9ef19aa1bdeac380662a112f1d03a7c3477527f.
---
 drivers/gpu/drm/amd/amdgpu/vi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c 
index 4ecfaa1..7e5f9de 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -1062,7 +1062,7 @@ static int vi_common_early_init(void *handle)
/* rev0 hardware requires workarounds to support PG */
adev->pg_flags = 0;
if (adev->rev_id != 0x00 || 
CZ_REV_BRISTOL(adev->pdev->revision)) {
-   adev->pg_flags |=
+   adev->pg_flags |= AMD_PG_SUPPORT_GFX_PG |
AMD_PG_SUPPORT_GFX_SMG |
AMD_PG_SUPPORT_GFX_PIPELINE |
AMD_PG_SUPPORT_CP |
--
2.5.5

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx