Re: Plan: BO move throttling for visible VRAM evictions

2017-05-14 Thread zhoucm1



On 2017年05月14日 05:31, Marek Olšák wrote:

On Mon, Apr 17, 2017 at 11:55 AM, Michel Dänzer  wrote:

On 17/04/17 07:58 AM, Marek Olšák wrote:

On Fri, Apr 14, 2017 at 12:14 PM, Michel Dänzer  wrote:

On 04/04/17 05:11 AM, Marek Olšák wrote:

On Fri, Mar 31, 2017 at 5:24 AM, Michel Dänzer  wrote:

On 30/03/17 07:03 PM, Michel Dänzer wrote:

On 25/03/17 01:33 AM, Marek Olšák wrote:

Hi,

I'm sharing this idea here, because it's something that has been
decreasing our performance a lot recently, for example:
http://openbenchmarking.org/prospect/1703011-RI-RADEONDIR06/7b7668cfc109d1c3dc27e871c8aea71ca13f23fa

The attached proof-of-concept patch (on top of Christian's "CPU mapping
of split VRAM buffers" series, ported from radeon) results in 145.05 fps
on my Tonga.

I get the same result without my or Christian's patches though, with
4.11 based DRM or amd-staging-4.9. So I guess I just can't reproduce the
problem with this test. Are there any other tests for it?

It's random. Sometimes the benchmark runs OK, other times it's slow.
You can easily see the difference but observing how smooth it is. The
visible VRAM evictions result in constant 100-200ms stalls but not
every frame, which feels like the frame rate is much lower than it
actually is.

Make sure your graphics details are maxed out. The best score I can
get with my rig is 70 fps. (Fiji & Core i5 3570)

I'm getting around 53-54 fps at Ultra with Tonga, both with Mesa 13.0.6
and Git.

Have you tried if Christian's patches for CPU access to split VRAM
buffers help? I can imagine that forcing contiguous VRAM buffers for CPU
access could cause lots of other BOs to be unnecessarily evicted from
VRAM, if at least one of their fragments happens to be in the CPU
visible part of VRAM.

I've finally tested latest amd-staging-4.9 and I'm very pleased. For
the first time, the Deus Ex benchmark has almost no hiccups. I've
never seen it so smooth. At one point, the MB/s BO move rate increase
to 200MB/s, stayed there for a couple of seconds, and then it dropped
to 0 again. The frame rate was OK-ish, so I guess the moves didn't
happen all at once. I also tested DiRT Rally and I haven't been able
to reproduce the low FPS with the consistently-high BO move rate that
I saw several months ago.

We could do some move throttling there for sure, but it's much better
than it ever was.

That's great to hear. If you get a chance, it would be interesting if
the attached updated patch improves things even more for you. (The patch
I attached previously couldn't work as intended, this one at least might :)

Frogging101 on IRC noticed that we get a ton of TTM BO moves due to
visible VRAM thrashing and Michel's patch doesn't help. His kernel is
up to date with amd-staging. It looks like the only option left is my
original plan: BO move throttling for visible VRAM by redirecting
mapped buffers to GTT and not allowing them to go back to VRAM if some
counter is too high.
I agree on this opinion, from our performance tuning experiment, this 
case indeed often happen especially under vram memory pressure. 
redirecting to GTT is better than heavy eviction between VRAM and GTT.
But we should get a condition for redirecting (eviction counter?), 
otherwise, BO have no change back to prefer domain.


Regards,
David Zhou


Opinions?

Marek
___
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: cleanup adjust_mc_addr handling

2017-05-14 Thread zhoucm1



On 2017年05月13日 02:57, Felix Kuehling wrote:

Hi Christian,

One comment inline [FK]. If this is not a problem, then feel free to add
my R-B for the whole series.

Kent, when we adopt this change, we need to convert the PDE back to an
address, because KFD needs to fill just the page directory base address
into the map_process HIQ packet. I think the existing
amdgpu_amdkfd_gpuvm_get_process_page_dir already takes care of that just
by right-shifting the address.

Regards,
   Felix

On 17-05-12 09:48 AM, Christian König wrote:

From: Christian König 

Rename adjust_mc_addr to get_vm_pde, check the address bits in one place and
move setting the valid bit in there as well.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h|  5 +++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 --
  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  |  5 +
  drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c  |  6 ++
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  |  8 +++-
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  |  8 +++-
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 10 ++
  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c |  5 +
  drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c  | 10 ++
  drivers/gpu/drm/amd/amdgpu/vce_v4_0.c  |  5 +
  10 files changed, 46 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index fadeb55..bc089eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -309,8 +309,8 @@ struct amdgpu_gart_funcs {
/* set pte flags based per asic */
uint64_t (*get_vm_pte_flags)(struct amdgpu_device *adev,
 uint32_t flags);
-   /* adjust mc addr in fb for APU case */
-   u64 (*adjust_mc_addr)(struct amdgpu_device *adev, u64 addr);
+   /* get the pde for a given mc addr */
+   u64 (*get_vm_pde)(struct amdgpu_device *adev, u64 addr);
uint32_t (*get_invalidate_req)(unsigned int vm_id);
  };
  
@@ -1816,6 +1816,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)

  #define amdgpu_asic_get_config_memsize(adev) 
(adev)->asic_funcs->get_config_memsize((adev))
  #define amdgpu_gart_flush_gpu_tlb(adev, vmid) 
(adev)->gart.gart_funcs->flush_gpu_tlb((adev), (vmid))
  #define amdgpu_gart_set_pte_pde(adev, pt, idx, addr, flags) 
(adev)->gart.gart_funcs->set_pte_pde((adev), (pt), (idx), (addr), (flags))
+#define amdgpu_gart_get_vm_pde(adev, addr) 
(adev)->gart.gart_funcs->get_vm_pde((adev), (addr))
  #define amdgpu_vm_copy_pte(adev, ib, pe, src, count) 
((adev)->vm_manager.vm_pte_funcs->copy_pte((ib), (pe), (src), (count)))
  #define amdgpu_vm_write_pte(adev, ib, pe, value, count, incr) 
((adev)->vm_manager.vm_pte_funcs->write_pte((ib), (pe), (value), (count), 
(incr)))
  #define amdgpu_vm_set_pte_pde(adev, ib, pe, addr, count, incr, flags) 
((adev)->vm_manager.vm_pte_funcs->set_pte_pde((ib), (pe), (addr), (count), 
(incr), (flags)))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 88420dc..c10f3ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -682,16 +682,6 @@ static bool amdgpu_vm_ring_has_compute_vm_bug(struct 
amdgpu_ring *ring)
return false;
  }
  
-static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)

-{
-   u64 addr = mc_addr;
-
-   if (adev->gart.gart_funcs->adjust_mc_addr)
-   addr = adev->gart.gart_funcs->adjust_mc_addr(adev, addr);
-
-   return addr;
-}
-
  bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
  struct amdgpu_job *job)
  {
@@ -1034,19 +1024,17 @@ static int amdgpu_vm_update_level(struct amdgpu_device 
*adev,
(count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
  
  			if (count) {

-   uint64_t pt_addr =
-   amdgpu_vm_adjust_mc_addr(adev, last_pt);
+   uint64_t entry;
  
+entry = amdgpu_gart_get_vm_pde(adev, last_pt);

if (shadow)
amdgpu_vm_do_set_ptes(,
  last_shadow,
- pt_addr, count,
- incr,
- AMDGPU_PTE_VALID);
+ entry, count,
+ incr, 0);

[FK] For count >=3 this would result in an SDMA_OP_PTEPDE packet with
flags=0 and the flags included in the address. Could SDMA mask out the
flags bits from the address before it applies the flags? The SDMA 4.0
MAS includes pseudo code that looks like it 

Re: [PATCH 3/3] drm/amdgpu: cleanup adjust_mc_addr handling

2017-05-14 Thread zhoucm1

the series is Reviewed-by: Chunming Zhou 

On 2017年05月12日 21:48, Christian König wrote:

From: Christian König 

Rename adjust_mc_addr to get_vm_pde, check the address bits in one place and
move setting the valid bit in there as well.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h|  5 +++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 --
  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  |  5 +
  drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c  |  6 ++
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c  |  8 +++-
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c  |  8 +++-
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 10 ++
  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c |  5 +
  drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c  | 10 ++
  drivers/gpu/drm/amd/amdgpu/vce_v4_0.c  |  5 +
  10 files changed, 46 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index fadeb55..bc089eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -309,8 +309,8 @@ struct amdgpu_gart_funcs {
/* set pte flags based per asic */
uint64_t (*get_vm_pte_flags)(struct amdgpu_device *adev,
 uint32_t flags);
-   /* adjust mc addr in fb for APU case */
-   u64 (*adjust_mc_addr)(struct amdgpu_device *adev, u64 addr);
+   /* get the pde for a given mc addr */
+   u64 (*get_vm_pde)(struct amdgpu_device *adev, u64 addr);
uint32_t (*get_invalidate_req)(unsigned int vm_id);
  };
  
@@ -1816,6 +1816,7 @@ amdgpu_get_sdma_instance(struct amdgpu_ring *ring)

  #define amdgpu_asic_get_config_memsize(adev) 
(adev)->asic_funcs->get_config_memsize((adev))
  #define amdgpu_gart_flush_gpu_tlb(adev, vmid) 
(adev)->gart.gart_funcs->flush_gpu_tlb((adev), (vmid))
  #define amdgpu_gart_set_pte_pde(adev, pt, idx, addr, flags) 
(adev)->gart.gart_funcs->set_pte_pde((adev), (pt), (idx), (addr), (flags))
+#define amdgpu_gart_get_vm_pde(adev, addr) 
(adev)->gart.gart_funcs->get_vm_pde((adev), (addr))
  #define amdgpu_vm_copy_pte(adev, ib, pe, src, count) 
((adev)->vm_manager.vm_pte_funcs->copy_pte((ib), (pe), (src), (count)))
  #define amdgpu_vm_write_pte(adev, ib, pe, value, count, incr) 
((adev)->vm_manager.vm_pte_funcs->write_pte((ib), (pe), (value), (count), 
(incr)))
  #define amdgpu_vm_set_pte_pde(adev, ib, pe, addr, count, incr, flags) 
((adev)->vm_manager.vm_pte_funcs->set_pte_pde((ib), (pe), (addr), (count), 
(incr), (flags)))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 88420dc..c10f3ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -682,16 +682,6 @@ static bool amdgpu_vm_ring_has_compute_vm_bug(struct 
amdgpu_ring *ring)
return false;
  }
  
-static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)

-{
-   u64 addr = mc_addr;
-
-   if (adev->gart.gart_funcs->adjust_mc_addr)
-   addr = adev->gart.gart_funcs->adjust_mc_addr(adev, addr);
-
-   return addr;
-}
-
  bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
  struct amdgpu_job *job)
  {
@@ -1034,19 +1024,17 @@ static int amdgpu_vm_update_level(struct amdgpu_device 
*adev,
(count == AMDGPU_VM_MAX_UPDATE_SIZE)) {
  
  			if (count) {

-   uint64_t pt_addr =
-   amdgpu_vm_adjust_mc_addr(adev, last_pt);
+   uint64_t entry;
  
+entry = amdgpu_gart_get_vm_pde(adev, last_pt);

if (shadow)
amdgpu_vm_do_set_ptes(,
  last_shadow,
- pt_addr, count,
- incr,
- AMDGPU_PTE_VALID);
+ entry, count,
+ incr, 0);
  
  amdgpu_vm_do_set_ptes(, last_pde,

- pt_addr, count, incr,
- AMDGPU_PTE_VALID);
+ entry, count, incr, 0);
}
  
  			count = 1;

@@ -1059,14 +1047,16 @@ static int amdgpu_vm_update_level(struct amdgpu_device 
*adev,
}
  
  	if (count) {

-   uint64_t pt_addr = amdgpu_vm_adjust_mc_addr(adev, last_pt);
+   uint64_t entry;
+
+   entry = amdgpu_gart_get_vm_pde(adev, last_pt);
  
  		if (vm->root.bo->shadow)

-   

Re: [PATCH libdrm] amdgpu: add missing extern "C" headers

2017-05-14 Thread Xie, AlexBin
Hi Michel,


This patch was from Nicolai.


Though there are two amdgpu.h, one header is in kernel and the other header is 
in libdrm. But their content are irrelevant. They are not synchronized.


I think include/drm/README is not applicable to this case.


Thanks,

Alex Bin Xie


From: Michel Dänzer 
Sent: Sunday, May 14, 2017 10:09 PM
To: Xie, AlexBin; Haehnle, Nicolai
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH libdrm] amdgpu: add missing extern "C" headers

On 15/05/17 10:10 AM, Xie, AlexBin wrote:
>  amdgpu/amdgpu.h | 8 
>  1 file changed, 8 insertions(+)

This header is synchronized with the kernel, see include/drm/README.


BTW, don't send patches for review as HTML. Ideally use git send-email.


--
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] amdgpu: add missing extern "C" headers

2017-05-14 Thread Xie, AlexBin
Hi Michel,


It is fine. There is a glitch in my email client. The same email was sent 
twice, probably because I pressed a special key.


I am new to open source review method. Perhaps I did not make thing clear 
enough.



-Alex Bin Xie


From: Michel Dänzer 
Sent: Sunday, May 14, 2017 10:11 PM
To: Xie, AlexBin; Haehnle, Nicolai
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH libdrm] amdgpu: add missing extern "C" headers

On 15/05/17 11:09 AM, Michel Dänzer wrote:
>
> BTW, don't send patches for review as HTML. Ideally use git send-email.

Sorry Alex, I was confused and thought you sent the patch.


--
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: Plan: BO move throttling for visible VRAM evictions

2017-05-14 Thread Michel Dänzer
On 14/05/17 06:31 AM, Marek Olšák wrote:
> On Mon, Apr 17, 2017 at 11:55 AM, Michel Dänzer  wrote:
>> On 17/04/17 07:58 AM, Marek Olšák wrote:
>>> On Fri, Apr 14, 2017 at 12:14 PM, Michel Dänzer  wrote:
 On 04/04/17 05:11 AM, Marek Olšák wrote:
> On Fri, Mar 31, 2017 at 5:24 AM, Michel Dänzer  wrote:
>> On 30/03/17 07:03 PM, Michel Dänzer wrote:
>>> On 25/03/17 01:33 AM, Marek Olšák wrote:
 Hi,

 I'm sharing this idea here, because it's something that has been
 decreasing our performance a lot recently, for example:
 http://openbenchmarking.org/prospect/1703011-RI-RADEONDIR06/7b7668cfc109d1c3dc27e871c8aea71ca13f23fa
>>>
>>> The attached proof-of-concept patch (on top of Christian's "CPU mapping
>>> of split VRAM buffers" series, ported from radeon) results in 145.05 fps
>>> on my Tonga.
>>
>> I get the same result without my or Christian's patches though, with
>> 4.11 based DRM or amd-staging-4.9. So I guess I just can't reproduce the
>> problem with this test. Are there any other tests for it?
>
> It's random. Sometimes the benchmark runs OK, other times it's slow.
> You can easily see the difference but observing how smooth it is. The
> visible VRAM evictions result in constant 100-200ms stalls but not
> every frame, which feels like the frame rate is much lower than it
> actually is.
>
> Make sure your graphics details are maxed out. The best score I can
> get with my rig is 70 fps. (Fiji & Core i5 3570)

 I'm getting around 53-54 fps at Ultra with Tonga, both with Mesa 13.0.6
 and Git.

 Have you tried if Christian's patches for CPU access to split VRAM
 buffers help? I can imagine that forcing contiguous VRAM buffers for CPU
 access could cause lots of other BOs to be unnecessarily evicted from
 VRAM, if at least one of their fragments happens to be in the CPU
 visible part of VRAM.
>>>
>>> I've finally tested latest amd-staging-4.9 and I'm very pleased. For
>>> the first time, the Deus Ex benchmark has almost no hiccups. I've
>>> never seen it so smooth. At one point, the MB/s BO move rate increase
>>> to 200MB/s, stayed there for a couple of seconds, and then it dropped
>>> to 0 again. The frame rate was OK-ish, so I guess the moves didn't
>>> happen all at once. I also tested DiRT Rally and I haven't been able
>>> to reproduce the low FPS with the consistently-high BO move rate that
>>> I saw several months ago.
>>>
>>> We could do some move throttling there for sure, but it's much better
>>> than it ever was.
>>
>> That's great to hear. If you get a chance, it would be interesting if
>> the attached updated patch improves things even more for you. (The patch
>> I attached previously couldn't work as intended, this one at least might :)
> 
> Frogging101 on IRC noticed that we get a ton of TTM BO moves due to
> visible VRAM thrashing and Michel's patch doesn't help. His kernel is
> up to date with amd-staging. It looks like the only option left is my
> original plan: BO move throttling for visible VRAM by redirecting
> mapped buffers to GTT and not allowing them to go back to VRAM if some
> counter is too high.
> 
> Opinions?

I think the next step should be to make radeonsi keep track of how much
VRAM it's trying to use that's expected to be accessed by the CPU, and
to use GTT instead when that exceeds a threshold (probably derived from
vram_vis_size).


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

2017-05-14 Thread Michel Dänzer
On 13/05/17 12:21 AM, Li, Samuel wrote:
> My understanding is this is actually a data file. Similar to amdgpu
> firmware, which is also separate from the kernel source code.

I don't think the reasons for the linux-firmware repository being
separate from linux apply to this file.

Please provide specific reasons why this file cannot be in the libdrm
repository upstream.


-- 
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] amdgpu: add missing extern "C" headers

2017-05-14 Thread Michel Dänzer
On 15/05/17 11:09 AM, Michel Dänzer wrote:
> 
> BTW, don't send patches for review as HTML. Ideally use git send-email.

Sorry Alex, I was confused and thought you sent the patch.


-- 
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] amdgpu: add missing extern "C" headers

2017-05-14 Thread Michel Dänzer
On 15/05/17 10:10 AM, Xie, AlexBin wrote:
>  amdgpu/amdgpu.h | 8 
>  1 file changed, 8 insertions(+)

This header is synchronized with the kernel, see include/drm/README.


BTW, don't send patches for review as HTML. Ideally use git send-email.


-- 
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] amdgpu: add missing extern "C" headers

2017-05-14 Thread Xie, AlexBin
 amdgpu/amdgpu.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index fdea905..1901fa8 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -30,20 +30,24 @@
  * User wanted to use libdrm_amdgpu functionality must include
  * this file.
  *
  */
 #ifndef _AMDGPU_H_
 #define _AMDGPU_H_

 #include 
 #include 

+#ifdef __cplusplus
+extern "C" {
+#endif
+
 struct drm_amdgpu_info_hw_ip;

 /*--*/
 /* --- Defines  */
 /*--*/

 /**
  * Define max. number of Command Buffers (IB) which could be sent to the single
  * hardware IP to accommodate CE/DE requirements
  *
@@ -1317,11 +1321,15 @@ int amdgpu_cs_destroy_semaphore(amdgpu_semaphore_handle 
sem);
 /**
  *  Get the ASIC marketing name
  *
  * \param   dev - \c [in] Device handle. See 
#amdgpu_device_initialize()
  *
  * \return  the constant string of the marketing name
  *  "NULL" means the ASIC is not found
 */
 const char *amdgpu_get_marketing_name(amdgpu_device_handle dev);

+#ifdef __cplusplus
+}
+#endif
+
 #endif /* #ifdef _AMDGPU_H_ */
--
2.9.3


Reviewed by: Alex Xie 


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


Re: [PATCH libdrm] amdgpu: add missing extern "C" headers

2017-05-14 Thread Xie, AlexBin
 amdgpu/amdgpu.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index fdea905..1901fa8 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -30,20 +30,24 @@
  * User wanted to use libdrm_amdgpu functionality must include
  * this file.
  *
  */
 #ifndef _AMDGPU_H_
 #define _AMDGPU_H_

 #include 
 #include 

+#ifdef __cplusplus
+extern "C" {
+#endif
+
 struct drm_amdgpu_info_hw_ip;

 /*--*/
 /* --- Defines  */
 /*--*/

 /**
  * Define max. number of Command Buffers (IB) which could be sent to the single
  * hardware IP to accommodate CE/DE requirements
  *
@@ -1317,11 +1321,15 @@ int amdgpu_cs_destroy_semaphore(amdgpu_semaphore_handle 
sem);
 /**
  *  Get the ASIC marketing name
  *
  * \param   dev - \c [in] Device handle. See 
#amdgpu_device_initialize()
  *
  * \return  the constant string of the marketing name
  *  "NULL" means the ASIC is not found
 */
 const char *amdgpu_get_marketing_name(amdgpu_device_handle dev);

+#ifdef __cplusplus
+}
+#endif
+
 #endif /* #ifdef _AMDGPU_H_ */
--
2.9.3


Reviewed by: Alex Xie 


___
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

2017-05-14 Thread Grazvydas Ignotas
On Sat, May 13, 2017 at 12:50 AM, Li, Samuel  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 
> Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie 
> 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  wrote:
>> From: Xiaojie Yuan 
>>
>> v2: fix an off by one error and leading white spaces
>>
>> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
>> Reviewed-by: Junwei Zhang 
>> Signed-off-by: Samuel Li 
>> ---
>>  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);
>> +   if (!buf)
>> +   return -ENOMEM;
>> +
>> +   /* ignore empty line and commented line */
>> +   if (strlen(line) == 0 || line[0] == '#') {
>> +   r = -EAGAIN;
>> +   goto out;
>> +