Re: Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Mao, David]

Sounds good!
One thing to confirm, If the original location is already in the invisible, 
will the notifier callback to move the bo from invisible to visible?  if it is 
true and the logic is already available in the kernel, can we use NO_CPU_ACCESS 
flag by default to accomplish the similar purpose for now?
It also reminds me of another related topic, can we always take visible heap as 
priority against to the remote in this case?
So far, kernel don’t have the heap priority.
IIRC, if the LFB bo moved to GTT, it will never be moved back since GTT is also 
its preferred heap. (Kernel seems to add the GTT even if the UMD only ask for 
LFB).

Thanks.
Best Regards,
David
On 30 Jun 2017, at 11:36 AM, Michel Dänzer 
> wrote:

On 30/06/17 10:55 AM, Mao, David wrote:
Vulkan allows the application to decide whether it wants the allocation
to be host visible and device local.
If we drop the flag, what will happen if we did not set the
NO_CPU_ACCESS flag?
Will it fail the map in case the allocation could be placed in invisible
heap then?

No, it'll work just as well. On attempted CPU access,
amdgpu_bo_fault_reserve_notify will ensure that it's CPU accessible.

The difference is that it'll allow BOs which aren't being actively
accessed by the CPU to be in CPU invisible VRAM, reducing pressure on
CPU visible VRAM.


--
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: Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Michel Dänzer]

On 30/06/17 10:55 AM, Mao, David wrote:
> Vulkan allows the application to decide whether it wants the allocation
> to be host visible and device local. 
> If we drop the flag, what will happen if we did not set the
> NO_CPU_ACCESS flag?
> Will it fail the map in case the allocation could be placed in invisible
> heap then?

No, it'll work just as well. On attempted CPU access,
amdgpu_bo_fault_reserve_notify will ensure that it's CPU accessible.

The difference is that it'll allow BOs which aren't being actively
accessed by the CPU to be in CPU invisible VRAM, reducing pressure on
CPU visible VRAM.


-- 
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: Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Mao, David]

Vulkan allows the application to decide whether it wants the allocation to be 
host visible and device local.
If we drop the flag, what will happen if we did not set the NO_CPU_ACCESS flag?
Will it fail the map in case the allocation could be placed in invisible heap 
then?

Thanks.
Best Regards,
David
On 29 Jun 2017, at 11:03 PM, Marek Olšák 
> wrote:

Do you have any concern if we also stop using the CPU_ACCESS flag on radeon?

Thanks,
Marek

On Thu, Jun 29, 2017 at 4:51 PM, Christian König
> wrote:
Yeah, I was thinking something similar.

See the intention behind CPU_ACCESS_REQUIRED is to always guarantee that CPU
access is immediately possible.

If you ask me that is not really useful for the UMD and was never meant to
be used by Mesa (only the closed source UMD and some kernel internal use
cases).

I would like to keep the behavior in the kernel driver as it is, but we
should really stop using this as a hint in Mesa.

Regards,
Christian.


Am 29.06.2017 um 16:41 schrieb Marek Olšák:

Hi,

Given how our memory manager works and the guesswork that UMDs have to
do to determine whether to set the flag, I think the flag isn't
useful.

I'm proposing that CPU_ACCESS_REQUIRED:
- will be deprecated.
- It will remain to be accepted by the kernel driver, but it will
either not have any effect, or it will serve as a hint that might or
might not be followed.
- The only flag that UMDs are expected to set with regard to CPU
access is NO_CPU_ACCESS.

The main motivation is the reduction of "virtual" heaps for UMD buffer
suballocators and reusable buffer pools. A higher number of heaps
means that more memory can be wasted by UMDs.

Opinions?

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

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

Re: [PATCH 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[Michel Dänzer]

On 29/06/17 07:05 PM, Daniel Vetter wrote:
> On Thu, Jun 29, 2017 at 06:58:05PM +0900, Michel Dänzer wrote:
>> On 29/06/17 05:23 PM, Christian König wrote:
>>> Am 29.06.2017 um 04:35 schrieb Michel Dänzer:
 On 29/06/17 08:26 AM, John Brooks wrote:
> On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:
>>>
>>> Instead of the flag being set in stone at BO creation, set the flag
>>> when a
>>> page fault occurs so that it goes somewhere CPU-visible, and clear
>>> it when
>>> the BO is requested by the GPU.
>>>
>>> However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
>>> invisible VRAM, where they may promptly generate another page
>>> fault. When
>>> BOs are constantly moved back and forth like this, it is highly
>>> detrimental
>>> to performance. Only clear the flag on CS if:
>>>
>>> - The BO wasn't page faulted for a certain amount of time
>>> (currently 10
>>>seconds), and
>>> - its last page fault didn't occur too soon (currently 500ms) after
>>> its
>>>last CS request, or vice versa.
>>>
>>> Setting the flag in amdgpu_fault_reserve_notify() also means that
>>> we can
>>> remove the loop to restrict lpfn to the end of visible VRAM, because
>>> amdgpu_ttm_placement_init() will do it for us.
>> I'm fine with the general approach, but I'm still absolutely not
>> keen about
>> clearing the flag when userspace has originally specified it.
 Is there any specific concern you have about that?
>>>
>>> Yeah, quite a bunch actually. We want to use this flag for P2P buffer
>>> sharing in the future as well and I don't intent to add another one like
>>> CPU_ACCESS_REALLY_REQUIRED or something like this.
>>
>> Won't a BO need to be pinned while it's being shared with another device?
> 
> That's an artifact of the current kernel implementation, I think we could
> do better (but for current use-cases where we share a bunch of scanouts
> and maybe a few pixmaps it's pointless). I wouldn't bet uapi on this never
> changing.

Surely there will need to be some kind of transaction though to let the
driver know when a BO starts/stops being shared with another device?
Either via the existing dma-buf callbacks, or something similar. We
can't rely on userspace setting a "CPU access" flag to make sure a BO
can be shared with other devices?


-- 
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 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[John Brooks]

On 2017-06-29 09:56 PM, John Brooks wrote:

On Thu, Jun 29, 2017 at 11:35:53AM +0900, Michel Dänzer wrote:

On 29/06/17 08:26 AM, John Brooks wrote:

On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:

Am 28.06.2017 um 04:33 schrieb John Brooks:

When the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag is given by userspace,
it should only be treated as a hint to initially place a BO somewhere CPU
accessible, rather than having a permanent effect on BO placement.

Instead of the flag being set in stone at BO creation, set the flag when a
page fault occurs so that it goes somewhere CPU-visible, and clear it when
the BO is requested by the GPU.

However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
invisible VRAM, where they may promptly generate another page fault. When
BOs are constantly moved back and forth like this, it is highly detrimental
to performance. Only clear the flag on CS if:

- The BO wasn't page faulted for a certain amount of time (currently 10
   seconds), and
- its last page fault didn't occur too soon (currently 500ms) after its
   last CS request, or vice versa.

Setting the flag in amdgpu_fault_reserve_notify() also means that we can
remove the loop to restrict lpfn to the end of visible VRAM, because
amdgpu_ttm_placement_init() will do it for us.

I'm fine with the general approach, but I'm still absolutely not keen about
clearing the flag when userspace has originally specified it.

Is there any specific concern you have about that?



Please add a new flag something like AMDGPU_GEM_CREATE_CPU_ACCESS_HINT or
something like this.

Is it the fact that we clear a flag that userspace expects not to have changed
if it queries it later? I think that's the only effect of this that's directly
visible to userspace code.

I don't see any way for userspace to query that.

I was looking at amdgpu_gem_metadata_ioctl(). It looked like it was possible to
query a BO's flags through that method. I don't know if it actually matters; it
was just a stab in the dark for any possibly tangible effect on userspace that
might arise from the kernel changing the flag.

John

And it looks like I am not correct about this as I misread it.
It exposes bo->metadata_flags, not bo->flags.

John




As for a new "hint" flag, I assume this new flag would be an alternative to the
existing CPU_ACCESS_REQUIRED flag, and we'd change Mesa et al to use it in
situations where the kernel *should* place a BO somewhere CPU-accessible, but
is permitted to move it elsewhere. Is that correct?

That seems silly. The userspace flag could never be more than a hint.
Unfortunately we named it to suggest differently, but we have to live
with that.

If we do need a new hint flag internally in the driver, we should simply
translate AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED to the new flag in
amdgpu_gem_create_ioctl, and not expose the new flag to userspace.


But other than the question in my followup to the cover letter, this
patch looks good to me as is.


--
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 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[John Brooks]

On Thu, Jun 29, 2017 at 11:35:53AM +0900, Michel Dänzer wrote:
> On 29/06/17 08:26 AM, John Brooks wrote:
> > On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:
> >> Am 28.06.2017 um 04:33 schrieb John Brooks:
> >>> When the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag is given by userspace,
> >>> it should only be treated as a hint to initially place a BO somewhere CPU
> >>> accessible, rather than having a permanent effect on BO placement.
> >>>
> >>> Instead of the flag being set in stone at BO creation, set the flag when a
> >>> page fault occurs so that it goes somewhere CPU-visible, and clear it when
> >>> the BO is requested by the GPU.
> >>>
> >>> However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
> >>> invisible VRAM, where they may promptly generate another page fault. When
> >>> BOs are constantly moved back and forth like this, it is highly 
> >>> detrimental
> >>> to performance. Only clear the flag on CS if:
> >>>
> >>> - The BO wasn't page faulted for a certain amount of time (currently 10
> >>>   seconds), and
> >>> - its last page fault didn't occur too soon (currently 500ms) after its
> >>>   last CS request, or vice versa.
> >>>
> >>> Setting the flag in amdgpu_fault_reserve_notify() also means that we can
> >>> remove the loop to restrict lpfn to the end of visible VRAM, because
> >>> amdgpu_ttm_placement_init() will do it for us.
> >>
> >> I'm fine with the general approach, but I'm still absolutely not keen about
> >> clearing the flag when userspace has originally specified it.
> 
> Is there any specific concern you have about that?
> 
> 
> >> Please add a new flag something like AMDGPU_GEM_CREATE_CPU_ACCESS_HINT or
> >> something like this.
> > 
> > Is it the fact that we clear a flag that userspace expects not to have 
> > changed
> > if it queries it later? I think that's the only effect of this that's 
> > directly
> > visible to userspace code.
> 
> I don't see any way for userspace to query that.

I was looking at amdgpu_gem_metadata_ioctl(). It looked like it was possible to
query a BO's flags through that method. I don't know if it actually matters; it
was just a stab in the dark for any possibly tangible effect on userspace that
might arise from the kernel changing the flag.

John

> 
> 
> > As for a new "hint" flag, I assume this new flag would be an alternative to 
> > the
> > existing CPU_ACCESS_REQUIRED flag, and we'd change Mesa et al to use it in
> > situations where the kernel *should* place a BO somewhere CPU-accessible, 
> > but
> > is permitted to move it elsewhere. Is that correct?
> 
> That seems silly. The userspace flag could never be more than a hint.
> Unfortunately we named it to suggest differently, but we have to live
> with that.
> 
> If we do need a new hint flag internally in the driver, we should simply
> translate AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED to the new flag in
> amdgpu_gem_create_ioctl, and not expose the new flag to userspace.
> 
> 
> But other than the question in my followup to the cover letter, this
> patch looks good to me as is.
> 
> 
> -- 
> 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: Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Michel Dänzer]

On 30/06/17 12:03 AM, Marek Olšák wrote:
> Do you have any concern if we also stop using the CPU_ACCESS flag on radeon?

No concern from my side for radeon.


> On Thu, Jun 29, 2017 at 4:51 PM, Christian König
>  wrote:
>> Yeah, I was thinking something similar.
>>
>> See the intention behind CPU_ACCESS_REQUIRED is to always guarantee that CPU
>> access is immediately possible.
>>
>> If you ask me that is not really useful for the UMD and was never meant to
>> be used by Mesa (only the closed source UMD and some kernel internal use
>> cases).
>>
>> I would like to keep the behavior in the kernel driver as it is, but we
>> should really stop using this as a hint in Mesa.

So we'd have a flag in the userspace ABI which is only used by closed
source userspace, and which can be used to artificially create pressure
on CPU visible VRAM. I know somebody who would argue vehemently against
adding something like that. :)

What does the closed source UMD use the flag for?


-- 
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: fix vblank_time when displays are off

[Michel Dänzer]

On 30/06/17 05:16 AM, Alex Deucher wrote:
> If the displays are off, set the vblank time to max to make
> sure mclk switching is enabled.  Avoid mclk getting set
> to high when no displays are attached.
> 
> bug: https://bugs.freedesktop.org/show_bug.cgi?id=101528
> fixes: 09be4a5219 (drm/amd/powerplay/smu7: add vblank check for mclk 
> switching (v2))
> Signed-off-by: Alex Deucher 
> Cc: sta...@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> index c6dba1e..8b8eda7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
> @@ -838,6 +838,9 @@ static int amdgpu_cgs_get_active_displays_info(struct 
> cgs_device *cgs_device,
>   return -EINVAL;
>  
>   mode_info = info->mode_info;
> + if (mode_info)
> + /* if the displays are off, vblank time is max */
> + mode_info->vblank_time_us = 0x;
>  
>   if (adev->mode_info.num_crtc && 
> adev->mode_info.mode_config_initialized) {
>   list_for_each_entry(crtc,
> 

Reviewed-by: Michel Dänzer 


-- 
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: Make KIQ read/write register routine be atomic

[Michel Dänzer]

On 30/06/17 06:08 AM, Shaoyun Liu wrote:
> 1. Use spin lock instead of mutex in KIQ
> 2. Directly write to KIQ fence address instead of using fence_emit()
> 3. Disable the interrupt for KIQ read/write and use CPU polling

This list indicates that this patch should be split up in at least three
patches. :)


-- 
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: Resizeable PCI BAR support V5

[Dieter Nützel]

Hello Christian,

I've running this since you've sent it on-top of amd-staging-4.11. But I 
think my poor little FUJITSU PRIMERGY TX150 S7, Xeon X3470 (Nehalem), 
PCIe 2.0, 24 GB is to old for this stuff...


[1.066475] pci :05:00.0: VF(n) BAR0 space: [mem 
0x-0x0003 64bit] (contains BAR0 for 16 VFs)
[1.066489] pci :05:00.0: VF(n) BAR2 space: [mem 
0x-0x003f 64bit] (contains BAR2 for 16 VFs)
[1.121656] pci :00:1c.0: BAR 15: assigned [mem 
0x8000-0x801f 64bit pref]
[1.121659] pci :00:1c.6: BAR 15: assigned [mem 
0x8020-0x803f 64bit pref]
[1.121662] pci :01:00.0: BAR 6: assigned [mem 
0xb012-0xb013 pref]
[1.121681] pci :05:00.0: BAR 6: assigned [mem 
0xb028-0xb02f pref]
[1.121683] pci :05:00.0: BAR 9: no space for [mem size 
0x0040 64bit]
[1.121684] pci :05:00.0: BAR 9: failed to assign [mem size 
0x0040 64bit]
[1.121685] pci :05:00.0: BAR 7: no space for [mem size 
0x0004 64bit]
[1.121687] pci :05:00.0: BAR 7: failed to assign [mem size 
0x0004 64bit]
[3.874180] amdgpu :01:00.0: BAR 0: releasing [mem 
0xc000-0xcfff 64bit pref]
[3.874182] amdgpu :01:00.0: BAR 2: releasing [mem 
0xb040-0xb05f 64bit pref]
[3.874198] pcieport :00:03.0: BAR 15: releasing [mem 
0xb040-0xcfff 64bit pref]
[3.874215] pcieport :00:03.0: BAR 15: no space for [mem size 
0x3 64bit pref]
[3.874217] pcieport :00:03.0: BAR 15: failed to assign [mem size 
0x3 64bit pref]
[3.874221] amdgpu :01:00.0: BAR 0: no space for [mem size 
0x2 64bit pref]
[3.874223] amdgpu :01:00.0: BAR 0: failed to assign [mem size 
0x2 64bit pref]
[3.874226] amdgpu :01:00.0: BAR 2: no space for [mem size 
0x0020 64bit pref]
[3.874227] amdgpu :01:00.0: BAR 2: failed to assign [mem size 
0x0020 64bit pref]

[3.874258] [drm] Not enough PCI address space for a large BAR.
[3.874261] amdgpu :01:00.0: BAR 0: assigned [mem 
0xc000-0xcfff 64bit pref]
[3.874269] amdgpu :01:00.0: BAR 2: assigned [mem 
0xb040-0xb05f 64bit pref]

[3.874288] [drm] Detected VRAM RAM=8192M, BAR=256M

Anyway rebase for current amd-staging-4.11 needed.
Find attached dmesg-amd-staging-4.11-1.g7262353-default+.log.xz

Greetings,
Dieter

Am 09.06.2017 10:59, schrieb Christian König:

Hi everyone,

This is the fith incarnation of this set of patches. It enables device
drivers to resize and most likely also relocate the PCI BAR of devices
they manage to allow the CPU to access all of the device local memory 
at once.


This is very useful for GFX device drivers where the default PCI BAR is 
only
about 256MB in size for compatibility reasons, but the device easily 
have

multiple gigabyte of local memory.

Some changes since V4:
1. Rebased on 4.11.
2. added the rb from Andy Shevchenko to patches which look complete 
now.
3. Move releasing the BAR and reallocating it on error to the driver 
side.

4. Add amdgpu support for GMC V6 hardware generation as well.

Please review and/or comment,
Christian.

___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

dmesg-amd-staging-4.11-1.g7262353-default+.log.xz
Description: application/xz
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: Make KIQ read/write register routine be atomic

[Shaoyun Liu]

1. Use spin lock instead of mutex in KIQ
2. Directly write to KIQ fence address instead of using fence_emit()
3. Disable the interrupt for KIQ read/write and use CPU polling

Change-Id: Id3693a2878ce1338f55aee3def6e7fc0f6b81996
Signed-off-by: Shaoyun Liu 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 57 ++--
 3 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ab1dad2..a155206 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -955,7 +955,7 @@ struct amdgpu_mec {
 struct amdgpu_kiq {
u64 eop_gpu_addr;
struct amdgpu_bo*eop_obj;
-   struct mutexring_mutex;
+   spinlock_t  ring_lock;
struct amdgpu_ring  ring;
struct amdgpu_irq_src   irq;
 };
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index e26108a..e5e5541 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -184,7 +184,7 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev,
struct amdgpu_kiq *kiq = >gfx.kiq;
int r = 0;
 
-   mutex_init(>ring_mutex);
+   spin_lock_init(>ring_lock);
 
r = amdgpu_wb_get(adev, >virt.reg_val_offs);
if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 8a081e1..95b4975 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -116,24 +116,33 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, 
uint32_t reg)
 {
signed long r;
uint32_t val;
-   struct dma_fence *f;
struct amdgpu_kiq *kiq = >gfx.kiq;
struct amdgpu_ring *ring = >ring;
+   unsigned long end_jiffies;
+   uint32_t seq;
+   volatile uint32_t *f;
 
BUG_ON(!ring->funcs->emit_rreg);
 
-   mutex_lock(>ring_mutex);
+   spin_lock(>ring_lock);
amdgpu_ring_alloc(ring, 32);
amdgpu_ring_emit_rreg(ring, reg);
-   amdgpu_fence_emit(ring, );
+   f = ring->fence_drv.cpu_addr;
+   *f = 0;
+   seq = ++ring->fence_drv.sync_seq;
+   amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, seq, 0);
amdgpu_ring_commit(ring);
-   mutex_unlock(>ring_mutex);
-
-   r = dma_fence_wait_timeout(f, false, 
msecs_to_jiffies(MAX_KIQ_REG_WAIT));
-   dma_fence_put(f);
-   if (r < 1) {
-   DRM_ERROR("wait for kiq fence error: %ld.\n", r);
-   return ~0;
+   spin_unlock(>ring_lock);
+
+   end_jiffies = (MAX_KIQ_REG_WAIT * HZ / 1000) + jiffies;
+   while (true) {
+   if (*f >= seq)
+   break;
+   if (time_after(jiffies, end_jiffies)) {
+   DRM_ERROR("wait for kiq fence error: %ld.\n", r);
+   return ~0;
+   }
+   cpu_relax();
}
 
val = adev->wb.wb[adev->virt.reg_val_offs];
@@ -144,23 +153,35 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, 
uint32_t reg)
 void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
 {
signed long r;
-   struct dma_fence *f;
struct amdgpu_kiq *kiq = >gfx.kiq;
struct amdgpu_ring *ring = >ring;
+   unsigned long end_jiffies;
+   uint32_t seq;
+   volatile uint32_t *f;
 
BUG_ON(!ring->funcs->emit_wreg);
 
-   mutex_lock(>ring_mutex);
+   spin_lock(>ring_lock);
amdgpu_ring_alloc(ring, 32);
amdgpu_ring_emit_wreg(ring, reg, v);
-   amdgpu_fence_emit(ring, );
+   f = ring->fence_drv.cpu_addr;
+   *f = 0;
+   seq = ++ring->fence_drv.sync_seq;
+   amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, seq, 0);
amdgpu_ring_commit(ring);
-   mutex_unlock(>ring_mutex);
+   spin_unlock(>ring_lock);
+
+   end_jiffies = (MAX_KIQ_REG_WAIT * HZ / 1000) + jiffies;
+   while (true) {
+   if (*f >= seq)
+   break;
+   if (time_after(jiffies, end_jiffies)) {
+   DRM_ERROR("wait for kiq fence error: %ld.\n", r);
+   return;
+   }
+   cpu_relax();
+   }
 
-   r = dma_fence_wait_timeout(f, false, 
msecs_to_jiffies(MAX_KIQ_REG_WAIT));
-   if (r < 1)
-   DRM_ERROR("wait for kiq fence error: %ld.\n", r);
-   dma_fence_put(f);
 }
 
 /**
-- 
1.9.1

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

Re: [PATCH] drm/radeon: add header comment for clarification to vce_v2_0_enable_mgcg()

[Gustavo A. R. Silva]

Quoting Alex Deucher :


On Thu, Jun 29, 2017 at 1:38 PM, Gustavo A. R. Silva
 wrote:

Add function header comment to make it clear that local variable sw_cg
is used for debugging and it should not be removed.

Addresses-Coverity-ID: 1198635
Cc: Alex Deucher 
Signed-off-by: Gustavo A. R. Silva 


Applied.  thanks!



Great, glad to help :)

Thanks
--
Gustavo A. R. Silva






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

Re: [PATCH] drm/radeon: add header comment for clarification to vce_v2_0_enable_mgcg()

[Alex Deucher]

On Thu, Jun 29, 2017 at 1:38 PM, Gustavo A. R. Silva
 wrote:
> Add function header comment to make it clear that local variable sw_cg
> is used for debugging and it should not be removed.
>
> Addresses-Coverity-ID: 1198635
> Cc: Alex Deucher 
> Signed-off-by: Gustavo A. R. Silva 

Applied.  thanks!

Alex

> ---
>  drivers/gpu/drm/radeon/vce_v2_0.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/radeon/vce_v2_0.c 
> b/drivers/gpu/drm/radeon/vce_v2_0.c
> index fce2144..b0a43b6 100644
> --- a/drivers/gpu/drm/radeon/vce_v2_0.c
> +++ b/drivers/gpu/drm/radeon/vce_v2_0.c
> @@ -104,6 +104,10 @@ static void vce_v2_0_disable_cg(struct radeon_device 
> *rdev)
> WREG32(VCE_CGTT_CLK_OVERRIDE, 7);
>  }
>
> +/*
> + * Local variable sw_cg is used for debugging purposes, in case we
> + * ran into problems with dynamic clock gating. Don't remove it.
> + */
>  void vce_v2_0_enable_mgcg(struct radeon_device *rdev, bool enable)
>  {
> bool sw_cg = false;
> --
> 2.5.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/radeon: add header comment for clarification to vce_v2_0_enable_mgcg()

[Gustavo A. R. Silva]

Add function header comment to make it clear that local variable sw_cg
is used for debugging and it should not be removed.

Addresses-Coverity-ID: 1198635
Cc: Alex Deucher 
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/gpu/drm/radeon/vce_v2_0.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/radeon/vce_v2_0.c 
b/drivers/gpu/drm/radeon/vce_v2_0.c
index fce2144..b0a43b6 100644
--- a/drivers/gpu/drm/radeon/vce_v2_0.c
+++ b/drivers/gpu/drm/radeon/vce_v2_0.c
@@ -104,6 +104,10 @@ static void vce_v2_0_disable_cg(struct radeon_device *rdev)
WREG32(VCE_CGTT_CLK_OVERRIDE, 7);
 }
 
+/*
+ * Local variable sw_cg is used for debugging purposes, in case we
+ * ran into problems with dynamic clock gating. Don't remove it.
+ */
 void vce_v2_0_enable_mgcg(struct radeon_device *rdev, bool enable)
 {
bool sw_cg = false;
-- 
2.5.0

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

Re: Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Marek Olšák]

Do you have any concern if we also stop using the CPU_ACCESS flag on radeon?

Thanks,
Marek

On Thu, Jun 29, 2017 at 4:51 PM, Christian König
 wrote:
> Yeah, I was thinking something similar.
>
> See the intention behind CPU_ACCESS_REQUIRED is to always guarantee that CPU
> access is immediately possible.
>
> If you ask me that is not really useful for the UMD and was never meant to
> be used by Mesa (only the closed source UMD and some kernel internal use
> cases).
>
> I would like to keep the behavior in the kernel driver as it is, but we
> should really stop using this as a hint in Mesa.
>
> Regards,
> Christian.
>
>
> Am 29.06.2017 um 16:41 schrieb Marek Olšák:
>>
>> Hi,
>>
>> Given how our memory manager works and the guesswork that UMDs have to
>> do to determine whether to set the flag, I think the flag isn't
>> useful.
>>
>> I'm proposing that CPU_ACCESS_REQUIRED:
>> - will be deprecated.
>> - It will remain to be accepted by the kernel driver, but it will
>> either not have any effect, or it will serve as a hint that might or
>> might not be followed.
>> - The only flag that UMDs are expected to set with regard to CPU
>> access is NO_CPU_ACCESS.
>>
>> The main motivation is the reduction of "virtual" heaps for UMD buffer
>> suballocators and reusable buffer pools. A higher number of heaps
>> means that more memory can be wasted by UMDs.
>>
>> Opinions?
>>
>> Thanks,
>> 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: Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Christian König]

Yeah, I was thinking something similar.

See the intention behind CPU_ACCESS_REQUIRED is to always guarantee that 
CPU access is immediately possible.


If you ask me that is not really useful for the UMD and was never meant 
to be used by Mesa (only the closed source UMD and some kernel internal 
use cases).


I would like to keep the behavior in the kernel driver as it is, but we 
should really stop using this as a hint in Mesa.


Regards,
Christian.

Am 29.06.2017 um 16:41 schrieb Marek Olšák:

Hi,

Given how our memory manager works and the guesswork that UMDs have to
do to determine whether to set the flag, I think the flag isn't
useful.

I'm proposing that CPU_ACCESS_REQUIRED:
- will be deprecated.
- It will remain to be accepted by the kernel driver, but it will
either not have any effect, or it will serve as a hint that might or
might not be followed.
- The only flag that UMDs are expected to set with regard to CPU
access is NO_CPU_ACCESS.

The main motivation is the reduction of "virtual" heaps for UMD buffer
suballocators and reusable buffer pools. A higher number of heaps
means that more memory can be wasted by UMDs.

Opinions?

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

Deprecation of AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED

[Marek Olšák]

Hi,

Given how our memory manager works and the guesswork that UMDs have to
do to determine whether to set the flag, I think the flag isn't
useful.

I'm proposing that CPU_ACCESS_REQUIRED:
- will be deprecated.
- It will remain to be accepted by the kernel driver, but it will
either not have any effect, or it will serve as a hint that might or
might not be followed.
- The only flag that UMDs are expected to set with regard to CPU
access is NO_CPU_ACCESS.

The main motivation is the reduction of "virtual" heaps for UMD buffer
suballocators and reusable buffer pools. A higher number of heaps
means that more memory can be wasted by UMDs.

Opinions?

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

RE: [PATCH v2] drm/amdgpu: fix the memory corruption on S3

[Deucher, Alexander]

> -Original Message-
> From: Christian König [mailto:deathsim...@vodafone.de]
> Sent: Thursday, June 29, 2017 4:17 AM
> To: Huang, Ray; amd-gfx@lists.freedesktop.org; Deucher, Alexander; Koenig,
> Christian
> Cc: Huan, Alvin; Qiao, Joe(Markham); Jiang, Sonny; Wang, Ken; Yuan, Xiaojie
> Subject: Re: [PATCH v2] drm/amdgpu: fix the memory corruption on S3
> 
> Am 29.06.2017 um 10:09 schrieb Huang Rui:
> > psp->cmd will be used on resume phase, so we can not free it on hw_init.
> > Otherwise, a memory corruption will be triggered.
> >
> > Signed-off-by: Huang Rui 
> > ---
> >
> > V1 -> V2:
> > - remove "cmd" variable.
> > - fix typo of check.
> >
> > Alex, Christian,
> >
> > This is the final fix for vega10 S3. The random memory corruption issue is
> root
> > caused.
> >
> > Thanks,
> > Ray
> >
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 +
> >   1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > index 5bed483..711476792 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > @@ -330,14 +330,11 @@ static int psp_load_fw(struct amdgpu_device
> *adev)
> >   {
> > int ret;
> > struct psp_context *psp = >psp;
> > -   struct psp_gfx_cmd_resp *cmd;
> >
> > -   cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
> > -   if (!cmd)
> > +   psp->cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
> > +   if (!psp->cmd)
> > return -ENOMEM;
> >
> > -   psp->cmd = cmd;
> > -
> > ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
> >   AMDGPU_GEM_DOMAIN_GTT,
> >   >fw_pri_bo,
> > @@ -376,8 +373,6 @@ static int psp_load_fw(struct amdgpu_device
> *adev)
> > if (ret)
> > goto failed_mem;
> >
> > -   kfree(cmd);
> > -
> > return 0;
> >
> >   failed_mem:
> > @@ -387,7 +382,8 @@ static int psp_load_fw(struct amdgpu_device
> *adev)
> > amdgpu_bo_free_kernel(>fw_pri_bo,
> >   >fw_pri_mc_addr, >fw_pri_buf);
> >   failed:
> > -   kfree(cmd);
> > +   kfree(psp->cmd);
> > +   psp->cmd = NULL;
> > return ret;
> >   }
> >
> > @@ -447,6 +443,11 @@ static int psp_hw_fini(void *handle)
> > amdgpu_bo_free_kernel(>fence_buf_bo,
> >   >fence_buf_mc_addr, 
> >fence_buf);
> >
> > +   if (psp->cmd) {
> 
> As Michel noted as well please drop this extra check, kfree(NULL) is
> perfectly save.
> 
> With that fixed the patch is Reviewed-by: Christian König
>  for now, but I still think we could do better
> by only allocating the temporary command buffer when it is needed.

Yes, nice find Ray!  Glad to finally have this one solved!  With the extra 
check fixed:
Reviewed-by: Alex Deucher 

> 
> Regards,
> Christian.
> 
> > +   kfree(psp->cmd);
> > +   psp->cmd = NULL;
> > +   }
> > +
> > return 0;
> >   }
> >
> 

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

RE: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code

[Mukunda, Vijendar]

-Original Message-
From: Mark Brown [mailto:broo...@kernel.org] 
Sent: Wednesday, June 28, 2017 11:36 PM
To: Alex Deucher
Cc: dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; 
alsa-de...@alsa-project.org; airl...@gmail.com; Mukunda, Vijendar; 
rajeevkumar.li...@gmail.com; lgirdw...@gmail.com; ti...@suse.de; 
pe...@perex.cz; Deucher, Alexander
Subject: Re: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code

On Fri, Jun 23, 2017 at 12:35:02PM -0400, Alex Deucher wrote:

> Reviewed-by: Alex Deucher 


> index dcbf997..e48ae5d 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -34,6 +34,8 @@
>  
>  #define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * 
> PLAYBACK_MAX_NUM_PERIODS)  #define MIN_BUFFER MAX_BUFFER
> +#define CHIP_STONEY 14
> +#define CHIP_CARRIZO 13

>These defines are being added in the middle of a file but CHIP_STONEY is also 
>used in another file in the previous patch (and apparently extensively 
>throughout the DRM driver already).  This is obviously not good, >we shouldn't 
>have multiple copies of the definition.

> - } else {
> + if (adata->asic_type == CHIP_CARRIZO) {
> + for (bank = 1; bank <= 4; bank++)
> + acp_set_sram_bank_state(adata->acp_mmio, bank,
> + false);

> I'm not seeing any poweroff cases for other chips being added, and again 
> switch statements please.

We will modify code to use single definition for CHIP_STONEY and CHIP_CARRIZO.
There are only two chip sets based on ACP 2.x design(Carrizo and Stoney).
Our future Chip sets going to use different design based on next ACP IP version.

In the current patch, Condition checks added for Carrizo for setting SRAM BANK 
state.
Memory Gating is disabled in Stoney,i.e SRAM Bank's won't be turned off. The 
default state for SRAM banks is ON.
As Memory Gating is disabled, there is no need to add condition checks for 
Stoney to set SRAM Bank state.

Apart from Memory Gating, there are  few more differences between Stoney and 
Carrizo chip sets.
Stoney specific DMA driver changes submitted in a separate patch.

Vijendar



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

Re: [gpu-drm-radeon] question about potential dead code in vce_v2_0_enable_mgcg()

[Alex Deucher]

On Wed, Jun 28, 2017 at 7:08 PM, Gustavo A. R. Silva
 wrote:
> Hi Alex,
>
> Quoting "Deucher, Alexander" :
>
>>> -Original Message-
>>> From: Gustavo A. R. Silva [mailto:garsi...@embeddedor.com]
>>> Sent: Wednesday, June 28, 2017 10:22 AM
>>> To: Deucher, Alexander; Koenig, Christian; David Airlie
>>> Cc: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
>>> linux-
>>> ker...@vger.kernel.org
>>> Subject: [gpu-drm-radeon] question about potential dead code in
>>> vce_v2_0_enable_mgcg()
>>>
>>>
>>> Hello everybody,
>>>
>>> While looking into Coverity ID 1198635 I ran into the following piece
>>> of code at drivers/gpu/drm/radeon/vce_v2_0.c:107:
>>>
>>> 107void vce_v2_0_enable_mgcg(struct radeon_device *rdev, bool enable)
>>> 108{
>>> 109bool sw_cg = false;
>>> 110
>>> 111if (enable && (rdev->cg_flags &
>>> RADEON_CG_SUPPORT_VCE_MGCG)) {
>>> 112if (sw_cg)
>>> 113vce_v2_0_set_sw_cg(rdev, true);
>>> 114else
>>> 115vce_v2_0_set_dyn_cg(rdev, true);
>>> 116} else {
>>> 117vce_v2_0_disable_cg(rdev);
>>> 118
>>> 119if (sw_cg)
>>> 120vce_v2_0_set_sw_cg(rdev, false);
>>> 121else
>>> 122vce_v2_0_set_dyn_cg(rdev, false);
>>> 123}
>>> 124}
>>>
>>> The issue here is that local variable sw_cg is never updated again
>>> after its initialization; which cause some code to be logically dead.
>>>
>>> My question here is if such variable is there for testing purposes or
>>> if it is a sort of an old code leftover that should be removed?
>>>
>>> In any case I can send a patch to add a comment or remove the dead code.
>>>
>>> I'd really appreciate any comments on this.
>>
>>
>> I wanted to leave the code in for debugging if we ran into problems with
>> dynamic clockgating.
>>
>
> Do you mind if I send a patch to add such comment and make it clear the
> purpose of that variable?

Sure.  Thanks.

Alex

>
> --- a/drivers/gpu/drm/radeon/vce_v2_0.c
> +++ b/drivers/gpu/drm/radeon/vce_v2_0.c
> @@ -104,6 +104,10 @@ static void vce_v2_0_disable_cg(struct radeon_device
> *rdev)
> WREG32(VCE_CGTT_CLK_OVERRIDE, 7);
>  }
>
> +/*
> + * Local variable sw_cg is used for debugging purposes, in case we
> + * ran into problems with dynamic clock gating. Don't remove it.
> + */
>  void vce_v2_0_enable_mgcg(struct radeon_device *rdev, bool enable)
>  {
> bool sw_cg = false;
>
>
> Thanks for clarifying!
> --
> Gustavo A. R. Silva
>
>
>
>
>
>
> ___
> 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 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[Daniel Vetter]

On Thu, Jun 29, 2017 at 06:58:05PM +0900, Michel Dänzer wrote:
> On 29/06/17 05:23 PM, Christian König wrote:
> > Am 29.06.2017 um 04:35 schrieb Michel Dänzer:
> >> On 29/06/17 08:26 AM, John Brooks wrote:
> >>> On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:
> >
> > Instead of the flag being set in stone at BO creation, set the flag
> > when a
> > page fault occurs so that it goes somewhere CPU-visible, and clear
> > it when
> > the BO is requested by the GPU.
> >
> > However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
> > invisible VRAM, where they may promptly generate another page
> > fault. When
> > BOs are constantly moved back and forth like this, it is highly
> > detrimental
> > to performance. Only clear the flag on CS if:
> >
> > - The BO wasn't page faulted for a certain amount of time
> > (currently 10
> >seconds), and
> > - its last page fault didn't occur too soon (currently 500ms) after
> > its
> >last CS request, or vice versa.
> >
> > Setting the flag in amdgpu_fault_reserve_notify() also means that
> > we can
> > remove the loop to restrict lpfn to the end of visible VRAM, because
> > amdgpu_ttm_placement_init() will do it for us.
>  I'm fine with the general approach, but I'm still absolutely not
>  keen about
>  clearing the flag when userspace has originally specified it.
> >> Is there any specific concern you have about that?
> > 
> > Yeah, quite a bunch actually. We want to use this flag for P2P buffer
> > sharing in the future as well and I don't intent to add another one like
> > CPU_ACCESS_REALLY_REQUIRED or something like this.
> 
> Won't a BO need to be pinned while it's being shared with another device?

That's an artifact of the current kernel implementation, I think we could
do better (but for current use-cases where we share a bunch of scanouts
and maybe a few pixmaps it's pointless). I wouldn't bet uapi on this never
changing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[Michel Dänzer]

On 29/06/17 05:23 PM, Christian König wrote:
> Am 29.06.2017 um 04:35 schrieb Michel Dänzer:
>> On 29/06/17 08:26 AM, John Brooks wrote:
>>> On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:
>
> Instead of the flag being set in stone at BO creation, set the flag
> when a
> page fault occurs so that it goes somewhere CPU-visible, and clear
> it when
> the BO is requested by the GPU.
>
> However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
> invisible VRAM, where they may promptly generate another page
> fault. When
> BOs are constantly moved back and forth like this, it is highly
> detrimental
> to performance. Only clear the flag on CS if:
>
> - The BO wasn't page faulted for a certain amount of time
> (currently 10
>seconds), and
> - its last page fault didn't occur too soon (currently 500ms) after
> its
>last CS request, or vice versa.
>
> Setting the flag in amdgpu_fault_reserve_notify() also means that
> we can
> remove the loop to restrict lpfn to the end of visible VRAM, because
> amdgpu_ttm_placement_init() will do it for us.
 I'm fine with the general approach, but I'm still absolutely not
 keen about
 clearing the flag when userspace has originally specified it.
>> Is there any specific concern you have about that?
> 
> Yeah, quite a bunch actually. We want to use this flag for P2P buffer
> sharing in the future as well and I don't intent to add another one like
> CPU_ACCESS_REALLY_REQUIRED or something like this.

Won't a BO need to be pinned while it's being shared with another device?


 Please add a new flag something like
 AMDGPU_GEM_CREATE_CPU_ACCESS_HINT or
 something like this.
>>> Is it the fact that we clear a flag that userspace expects not to
>>> have changed
>>> if it queries it later? I think that's the only effect of this that's
>>> directly
>>> visible to userspace code.
>> I don't see any way for userspace to query that.
>>
>>
>>> As for a new "hint" flag, I assume this new flag would be an
>>> alternative to the
>>> existing CPU_ACCESS_REQUIRED flag, and we'd change Mesa et al to use
>>> it in
>>> situations where the kernel *should* place a BO somewhere
>>> CPU-accessible, but
>>> is permitted to move it elsewhere. Is that correct?
>> That seems silly. The userspace flag could never be more than a hint.
>> Unfortunately we named it to suggest differently, but we have to live
>> with that.
> 
> No, just the other way around. The CPU_ACCESS_REQUIRED flag was
> introduced to note that it is MANDATORY to always have CPU access to the
> buffer.
> 
> It's just mesa which uses the flag as a hint to we could get CPU access.

Can you describe a specific scenario where userspace would actually need
the strict semantics? Otherwise I'm afraid what you're demanding boils
down to userspace churn for no benefit.


-- 
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 v2] drm/amdgpu: fix the memory corruption on S3

[Huang Rui]

On Thu, Jun 29, 2017 at 04:16:53PM +0800, Christian König wrote:
> Am 29.06.2017 um 10:09 schrieb Huang Rui:
> > psp->cmd will be used on resume phase, so we can not free it on hw_init.
> > Otherwise, a memory corruption will be triggered.
> >
> > Signed-off-by: Huang Rui 
> > ---
> >
> > V1 -> V2:
> > - remove "cmd" variable.
> > - fix typo of check.
> >
> > Alex, Christian,
> >
> > This is the final fix for vega10 S3. The random memory corruption issue is
> root
> > caused.
> >
> > Thanks,
> > Ray
> >
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 +
> >   1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/
> amdgpu/amdgpu_psp.c
> > index 5bed483..711476792 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > @@ -330,14 +330,11 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >   {
> >int ret;
> >struct psp_context *psp = >psp;
> > - struct psp_gfx_cmd_resp *cmd;
> >  
> > - cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
> > - if (!cmd)
> > + psp->cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
> > + if (!psp->cmd)
> >return -ENOMEM;
> >  
> > - psp->cmd = cmd;
> > -
> >ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
> >  AMDGPU_GEM_DOMAIN_GTT,
> >  >fw_pri_bo,
> > @@ -376,8 +373,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >if (ret)
> >goto failed_mem;
> >  
> > - kfree(cmd);
> > -
> >return 0;
> >  
> >   failed_mem:
> > @@ -387,7 +382,8 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >amdgpu_bo_free_kernel(>fw_pri_bo,
> >  >fw_pri_mc_addr, >fw_pri_buf);
> >   failed:
> > - kfree(cmd);
> > + kfree(psp->cmd);
> > + psp->cmd = NULL;
> >return ret;
> >   }
> >  
> > @@ -447,6 +443,11 @@ static int psp_hw_fini(void *handle)
> >amdgpu_bo_free_kernel(>fence_buf_bo,
> >  >fence_buf_mc_addr, >
> fence_buf);
> >  
> > + if (psp->cmd) {
> 
> As Michel noted as well please drop this extra check, kfree(NULL) is
> perfectly save.
> 
> With that fixed the patch is Reviewed-by: Christian König
>  for now, but I still think we could do better
> by only allocating the temporary command buffer when it is needed.
> 

Thanks. This is the quick fix for release. You know, it was a tragedy till
I found the root cause for S3 suspend/resume and make it stable, now it's
able to enter S3 more than 30+ cycles and never crash. 

I am planning to refine the psp codes, any suggestions are warm for me. I
will refer the comments such as fence and "temporary command buffter" to
modify it in following days. :-)

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

Re: [PATCH 4/5] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

[Christian König]

Am 29.06.2017 um 04:35 schrieb Michel Dänzer:

On 29/06/17 08:26 AM, John Brooks wrote:

On Wed, Jun 28, 2017 at 03:05:32PM +0200, Christian König wrote:

Am 28.06.2017 um 04:33 schrieb John Brooks:

When the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag is given by userspace,
it should only be treated as a hint to initially place a BO somewhere CPU
accessible, rather than having a permanent effect on BO placement.

Instead of the flag being set in stone at BO creation, set the flag when a
page fault occurs so that it goes somewhere CPU-visible, and clear it when
the BO is requested by the GPU.

However, clearing the CPU_ACCESS_REQUIRED flag may move BOs in GTT to
invisible VRAM, where they may promptly generate another page fault. When
BOs are constantly moved back and forth like this, it is highly detrimental
to performance. Only clear the flag on CS if:

- The BO wasn't page faulted for a certain amount of time (currently 10
   seconds), and
- its last page fault didn't occur too soon (currently 500ms) after its
   last CS request, or vice versa.

Setting the flag in amdgpu_fault_reserve_notify() also means that we can
remove the loop to restrict lpfn to the end of visible VRAM, because
amdgpu_ttm_placement_init() will do it for us.

I'm fine with the general approach, but I'm still absolutely not keen about
clearing the flag when userspace has originally specified it.

Is there any specific concern you have about that?


Yeah, quite a bunch actually. We want to use this flag for P2P buffer 
sharing in the future as well and I don't intent to add another one like 
CPU_ACCESS_REALLY_REQUIRED or something like this.



Please add a new flag something like AMDGPU_GEM_CREATE_CPU_ACCESS_HINT or
something like this.

Is it the fact that we clear a flag that userspace expects not to have changed
if it queries it later? I think that's the only effect of this that's directly
visible to userspace code.

I don't see any way for userspace to query that.



As for a new "hint" flag, I assume this new flag would be an alternative to the
existing CPU_ACCESS_REQUIRED flag, and we'd change Mesa et al to use it in
situations where the kernel *should* place a BO somewhere CPU-accessible, but
is permitted to move it elsewhere. Is that correct?

That seems silly. The userspace flag could never be more than a hint.
Unfortunately we named it to suggest differently, but we have to live
with that.


No, just the other way around. The CPU_ACCESS_REQUIRED flag was 
introduced to note that it is MANDATORY to always have CPU access to the 
buffer.


It's just mesa which uses the flag as a hint to we could get CPU access.

Regards,
Christian.


If we do need a new hint flag internally in the driver, we should simply
translate AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED to the new flag in
amdgpu_gem_create_ioctl, and not expose the new flag to userspace.


But other than the question in my followup to the cover letter, this
patch looks good to me as is.



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

Re: [PATCH v2] drm/amdgpu: fix the memory corruption on S3

[Christian König]

Am 29.06.2017 um 10:09 schrieb Huang Rui:

psp->cmd will be used on resume phase, so we can not free it on hw_init.
Otherwise, a memory corruption will be triggered.

Signed-off-by: Huang Rui 
---

V1 -> V2:
- remove "cmd" variable.
- fix typo of check.

Alex, Christian,

This is the final fix for vega10 S3. The random memory corruption issue is root
caused.

Thanks,
Ray

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 5bed483..711476792 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -330,14 +330,11 @@ static int psp_load_fw(struct amdgpu_device *adev)
  {
int ret;
struct psp_context *psp = >psp;
-   struct psp_gfx_cmd_resp *cmd;
  
-	cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);

-   if (!cmd)
+   psp->cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
+   if (!psp->cmd)
return -ENOMEM;
  
-	psp->cmd = cmd;

-
ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
  AMDGPU_GEM_DOMAIN_GTT,
  >fw_pri_bo,
@@ -376,8 +373,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
if (ret)
goto failed_mem;
  
-	kfree(cmd);

-
return 0;
  
  failed_mem:

@@ -387,7 +382,8 @@ static int psp_load_fw(struct amdgpu_device *adev)
amdgpu_bo_free_kernel(>fw_pri_bo,
  >fw_pri_mc_addr, >fw_pri_buf);
  failed:
-   kfree(cmd);
+   kfree(psp->cmd);
+   psp->cmd = NULL;
return ret;
  }
  
@@ -447,6 +443,11 @@ static int psp_hw_fini(void *handle)

amdgpu_bo_free_kernel(>fence_buf_bo,
  >fence_buf_mc_addr, >fence_buf);
  
+	if (psp->cmd) {


As Michel noted as well please drop this extra check, kfree(NULL) is 
perfectly save.


With that fixed the patch is Reviewed-by: Christian König 
 for now, but I still think we could do better 
by only allocating the temporary command buffer when it is needed.


Regards,
Christian.


+   kfree(psp->cmd);
+   psp->cmd = NULL;
+   }
+
return 0;
  }
  



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

Re: [PATCH] drm/amdgpu: fix the memory corruption on S3

[Huang Rui]

On Thu, Jun 29, 2017 at 04:07:33PM +0800, Michel Dänzer wrote:
> On 29/06/17 04:59 PM, Huang Rui wrote:
> > On Thu, Jun 29, 2017 at 03:34:57PM +0800, Michel Dänzer wrote:
> >> On 29/06/17 04:03 PM, Huang Rui wrote:
> >>> psp->cmd will be used on resume phase, so we can not free it on hw_init.
> >>> Otherwise, a memory corruption will be triggered.
> >>>
> >>> Signed-off-by: Huang Rui 
> >>> ---
> >>>
> >>> Alex, Christian,
> >>>
> >>> This is the final fix for vega10 S3. The random memory corruption issue is
> >> root
> >>> caused.
> >>>
> >>> Thanks,
> >>> Ray
> >>>
> >>> ---
> >>>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++--
> >>>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
> >>> b/drivers/gpu/drm/amd/
> >> amdgpu/amdgpu_psp.c
> >>> index 5041073..fcdd542 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> >>> @@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >>>if (ret)
> >>>goto failed_mem;
> >>>
> >>> - kfree(cmd);
> >>> -
> >>>return 0;
> >>
> >> This looks like a good catch.
> >>
> >>
> >>> @@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >>>  >fw_pri_mc_addr, >fw_pri_buf);
> >>>  failed:
> >>>kfree(cmd);
> >>> + cmd = NULL;
> >>
> >> This should probably be
> >>
> >> psp->cmd = NULL;
> >>
> >> instead?
> >>
> >
> > Actually, we set psp->cmd = cmd before.
> >
> > But anyway, we needn't "cmd" member any more.
> 
> You should probably still set psp->cmd = NULL here, otherwise psp->cmd
> still contains the pointer to the memory that is freed here, which could
> result in use-after-free somewhere else.
> 

Right, I already found it and update it in V2, please take a look.

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

Re: [PATCH 3/5] drm/amdgpu: Track time of last page fault and last CS move in struct amdgpu_bo

[Christian König]

Am 29.06.2017 um 00:59 schrieb John Brooks:

On Wed, Jun 28, 2017 at 03:06:47PM +0200, Christian König wrote:

Am 28.06.2017 um 04:33 schrieb John Brooks:

Signed-off-by: John Brooks 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h| 3 +++
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 ++
  3 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 7366115..34c293a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -428,6 +428,9 @@ struct amdgpu_bo {
void*metadata;
u32 metadata_size;
unsignedprime_shared_count;
+   unsigned long   last_page_fault_jiffies;
+   unsigned long   last_cs_move_jiffies;

Please use jiffies64 here, apart from that the patch looks good to me.

Christian.


I'm not sure I understand. Do you mean change these variables to u64 and use
get_jiffies_64() instead of the plain jiffies variable below?


Yes, exactly.


I believe jiffies_64 can be slower than jiffies also.


Yeah, but it doesn't matter on 64bit systems and they don't wrap around 
every 49 days on 32bit systems :)


Christian.



John


+
/* list of all virtual address to which this bo
 * is associated to
 */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 1dfa847..071b592 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -335,6 +335,7 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
u64 initial_bytes_moved, bytes_moved;
uint32_t domain;
+   uint32_t old_mem;
int r;
if (bo->pin_count)
@@ -364,6 +365,7 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
  retry:
amdgpu_ttm_placement_from_domain(bo, domain);
initial_bytes_moved = atomic64_read(>num_bytes_moved);
+   old_mem = bo->tbo.mem.mem_type;
r = ttm_bo_validate(>tbo, >placement, true, false);
bytes_moved = atomic64_read(>num_bytes_moved) -
  initial_bytes_moved;
@@ -377,6 +379,9 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
goto retry;
}
+   if (bo->tbo.mem.mem_type != old_mem)
+   bo->last_cs_move_jiffies = jiffies;
+
return r;
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index dcf1ddb..b71775c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -953,6 +953,8 @@ int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object 
*bo)
if (bo->mem.mem_type != TTM_PL_VRAM)
return 0;
+   abo->last_page_fault_jiffies = jiffies;
+
size = bo->mem.num_pages << PAGE_SHIFT;
offset = bo->mem.start << PAGE_SHIFT;
/* TODO: figure out how to map scattered VRAM to the CPU */




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

[PATCH v2] drm/amdgpu: fix the memory corruption on S3

[Huang Rui]

psp->cmd will be used on resume phase, so we can not free it on hw_init.
Otherwise, a memory corruption will be triggered.

Signed-off-by: Huang Rui 
---

V1 -> V2:
- remove "cmd" variable.
- fix typo of check.

Alex, Christian,

This is the final fix for vega10 S3. The random memory corruption issue is root
caused.

Thanks,
Ray

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 5bed483..711476792 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -330,14 +330,11 @@ static int psp_load_fw(struct amdgpu_device *adev)
 {
int ret;
struct psp_context *psp = >psp;
-   struct psp_gfx_cmd_resp *cmd;
 
-   cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
-   if (!cmd)
+   psp->cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
+   if (!psp->cmd)
return -ENOMEM;
 
-   psp->cmd = cmd;
-
ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
  AMDGPU_GEM_DOMAIN_GTT,
  >fw_pri_bo,
@@ -376,8 +373,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
if (ret)
goto failed_mem;
 
-   kfree(cmd);
-
return 0;
 
 failed_mem:
@@ -387,7 +382,8 @@ static int psp_load_fw(struct amdgpu_device *adev)
amdgpu_bo_free_kernel(>fw_pri_bo,
  >fw_pri_mc_addr, >fw_pri_buf);
 failed:
-   kfree(cmd);
+   kfree(psp->cmd);
+   psp->cmd = NULL;
return ret;
 }
 
@@ -447,6 +443,11 @@ static int psp_hw_fini(void *handle)
amdgpu_bo_free_kernel(>fence_buf_bo,
  >fence_buf_mc_addr, >fence_buf);
 
+   if (psp->cmd) {
+   kfree(psp->cmd);
+   psp->cmd = NULL;
+   }
+
return 0;
 }
 
-- 
2.7.4

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

Re: drm/amdgpu: Fix vram_page_split parameter description

[Christian König]

Reviewed-by: Christian König .

Regards,
Christian.

Am 28.06.2017 um 21:26 schrieb Kent Russell:

The default was changed to 512 from 1024, but the default in the
description wasn't updated.

 Kent



___
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] drm/amdgpu: fix the memory corruption on S3

[Michel Dänzer]

On 29/06/17 04:59 PM, Huang Rui wrote:
> On Thu, Jun 29, 2017 at 03:34:57PM +0800, Michel Dänzer wrote:
>> On 29/06/17 04:03 PM, Huang Rui wrote:
>>> psp->cmd will be used on resume phase, so we can not free it on hw_init.
>>> Otherwise, a memory corruption will be triggered.
>>>
>>> Signed-off-by: Huang Rui 
>>> ---
>>>
>>> Alex, Christian,
>>>
>>> This is the final fix for vega10 S3. The random memory corruption issue is
>> root
>>> caused.
>>>
>>> Thanks,
>>> Ray
>>>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/
>> amdgpu/amdgpu_psp.c
>>> index 5041073..fcdd542 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> @@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
>>>if (ret)
>>>goto failed_mem;
>>>
>>> - kfree(cmd);
>>> -
>>>return 0;
>>
>> This looks like a good catch.
>>
>>
>>> @@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
>>>  >fw_pri_mc_addr, >fw_pri_buf);
>>>  failed:
>>>kfree(cmd);
>>> + cmd = NULL;
>>
>> This should probably be
>>
>> psp->cmd = NULL;
>>
>> instead?
>>
> 
> Actually, we set psp->cmd = cmd before.
> 
> But anyway, we needn't "cmd" member any more.

You should probably still set psp->cmd = NULL here, otherwise psp->cmd
still contains the pointer to the memory that is freed here, which could
result in use-after-free somewhere else.


-- 
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: fix the memory corruption on S3

[Christian König]

Indeed a nice catch.

Just skimming over the PSP code, wouldn't it be simpler to temporary 
allocate the cmd buffer in psp_np_fw_load()?


Doesn't looks like that is used outside that function. Might even be 
possible to just allocate the buffer on the stack.


Regards,
Christian.

Am 29.06.2017 um 09:59 schrieb Huang Rui:

On Thu, Jun 29, 2017 at 03:34:57PM +0800, Michel Dänzer wrote:

On 29/06/17 04:03 PM, Huang Rui wrote:

psp->cmd will be used on resume phase, so we can not free it on hw_init.
Otherwise, a memory corruption will be triggered.

Signed-off-by: Huang Rui 
---

Alex, Christian,

This is the final fix for vega10 S3. The random memory corruption issue is

root

caused.

Thanks,
Ray

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/

amdgpu/amdgpu_psp.c

index 5041073..fcdd542 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
if (ret)
goto failed_mem;

- kfree(cmd);
-
return 0;

This looks like a good catch.



@@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
  >fw_pri_mc_addr, >fw_pri_buf);
  failed:
kfree(cmd);
+ cmd = NULL;

This should probably be

 psp->cmd = NULL;

instead?


Actually, we set psp->cmd = cmd before.

But anyway, we needn't "cmd" member any more.


@@ -443,6 +442,11 @@ static int psp_hw_fini(void *handle)
amdgpu_bo_free_kernel(>fence_buf_bo,
  >fence_buf_mc_addr, >

fence_buf);

+ if (!psp->cmd) {
+ kfree(psp->cmd);
+ psp->cmd = NULL;
+ }

This should probably be

 if (psp->cmd) {

? If so, you could skip the if altogether, since kfree(NULL) is safe.

Nice catch. My typo. ;-)

You're right. Will update it in V2.

Thanks,
Rui



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

Re: [PATCH] drm/amdgpu: fix the memory corruption on S3

[Huang Rui]

On Thu, Jun 29, 2017 at 03:34:57PM +0800, Michel Dänzer wrote:
> On 29/06/17 04:03 PM, Huang Rui wrote:
> > psp->cmd will be used on resume phase, so we can not free it on hw_init.
> > Otherwise, a memory corruption will be triggered.
> >
> > Signed-off-by: Huang Rui 
> > ---
> >
> > Alex, Christian,
> >
> > This is the final fix for vega10 S3. The random memory corruption issue is
> root
> > caused.
> >
> > Thanks,
> > Ray
> >
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/
> amdgpu/amdgpu_psp.c
> > index 5041073..fcdd542 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > @@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >if (ret)
> >goto failed_mem;
> > 
> > - kfree(cmd);
> > -
> >return 0;
> 
> This looks like a good catch.
> 
> 
> > @@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >  >fw_pri_mc_addr, >fw_pri_buf);
> >  failed:
> >kfree(cmd);
> > + cmd = NULL;
> 
> This should probably be
> 
> psp->cmd = NULL;
> 
> instead?
> 

Actually, we set psp->cmd = cmd before.

But anyway, we needn't "cmd" member any more.

> 
> > @@ -443,6 +442,11 @@ static int psp_hw_fini(void *handle)
> >amdgpu_bo_free_kernel(>fence_buf_bo,
> >  >fence_buf_mc_addr, >
> fence_buf);
> > 
> > + if (!psp->cmd) {
> > + kfree(psp->cmd);
> > + psp->cmd = NULL;
> > + }
> 
> This should probably be
> 
> if (psp->cmd) {
> 
> ? If so, you could skip the if altogether, since kfree(NULL) is safe.

Nice catch. My typo. ;-)

You're right. Will update it in V2.

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

Re: [PATCH] drm/amdgpu: fix the memory corruption on S3

[Michel Dänzer]

On 29/06/17 04:03 PM, Huang Rui wrote:
> psp->cmd will be used on resume phase, so we can not free it on hw_init.
> Otherwise, a memory corruption will be triggered.
> 
> Signed-off-by: Huang Rui 
> ---
> 
> Alex, Christian,
> 
> This is the final fix for vega10 S3. The random memory corruption issue is 
> root
> caused.
> 
> Thanks,
> Ray
> 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 5041073..fcdd542 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
>   if (ret)
>   goto failed_mem;
>  
> - kfree(cmd);
> -
>   return 0;

This looks like a good catch.


> @@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >fw_pri_mc_addr, >fw_pri_buf);
>  failed:
>   kfree(cmd);
> + cmd = NULL;

This should probably be

psp->cmd = NULL;

instead?


> @@ -443,6 +442,11 @@ static int psp_hw_fini(void *handle)
>   amdgpu_bo_free_kernel(>fence_buf_bo,
> >fence_buf_mc_addr, >fence_buf);
>  
> + if (!psp->cmd) {
> + kfree(psp->cmd);
> + psp->cmd = NULL;
> + }

This should probably be

if (psp->cmd) {

? If so, you could skip the if altogether, since kfree(NULL) is safe.


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

[PATCH] drm/amdgpu: fix the memory corruption on S3

[Huang Rui]

psp->cmd will be used on resume phase, so we can not free it on hw_init.
Otherwise, a memory corruption will be triggered.

Signed-off-by: Huang Rui 
---

Alex, Christian,

This is the final fix for vega10 S3. The random memory corruption issue is root
caused.

Thanks,
Ray

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 5041073..fcdd542 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
if (ret)
goto failed_mem;
 
-   kfree(cmd);
-
return 0;
 
 failed_mem:
@@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
  >fw_pri_mc_addr, >fw_pri_buf);
 failed:
kfree(cmd);
+   cmd = NULL;
return ret;
 }
 
@@ -443,6 +442,11 @@ static int psp_hw_fini(void *handle)
amdgpu_bo_free_kernel(>fence_buf_bo,
  >fence_buf_mc_addr, >fence_buf);
 
+   if (!psp->cmd) {
+   kfree(psp->cmd);
+   psp->cmd = NULL;
+   }
+
return 0;
 }
 
-- 
2.7.4

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