Re: [PATCH] drm/amdgpu: fix gtt_mgr bo's offset

2016-09-20 Thread Flora Cui 
On Wed, Sep 21, 2016 at 09:54:04AM +0800, zhoucm1 wrote:
> 
> 
> On 2016年09月20日 19:40, Christian König wrote:
> >Am 20.09.2016 um 13:16 schrieb Flora Cui:
> >>Does this change make sense?
> >
> >Yes, exactly. Patch is Reviewed-by: Christian König
> >.
> Hi Flora,
> Does this change work?
> 
> if (>mem == mem)
> this checking seems always be false, right?
mostly the check is true. eg
amdgpu_bo_pin_restricted->amdgpu_ttm_bind(), the mem is >tbo.mem
> 
> Regards,
> David Zhou
> >
> >But please be aware that there is at least one other bug I'm currently
> >investigating.
> >
> >Regards,
> >Christian.
> >
> >>
> >>On Tue, Sep 20, 2016 at 01:02:20PM +0200, Christian König wrote:
> >>>Am 20.09.2016 um 12:41 schrieb Flora Cui:
> On Tue, Sep 20, 2016 at 06:19:33PM +0800, zhoucm1 wrote:
> >On 2016年09月20日 17:09, Flora Cui wrote:
> >>Change-Id: I89e9be3d5c96d46655f3a977fb557b20b4d87609
> >>Signed-off-by: Flora Cui 
> >>---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>index 262e872..b266cf7 100644
> >>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>@@ -128,8 +128,8 @@ int amdgpu_gtt_mgr_alloc(struct
> >>ttm_mem_type_manager *man,
> >>  if (!r) {
> >>  mem->start = node->start;
> >>-tbo->offset = (tbo->mem.start << PAGE_SHIFT) +
> >>- tbo->bdev->man[tbo->mem.mem_type].gpu_offset;
> >>+tbo->offset = (mem->start << PAGE_SHIFT) +
> >>+ tbo->bdev->man[mem->mem_type].gpu_offset;
> >mem->mem_type seems not set yet as well if I am correct.
> >IIRC, I feel it's better that tbo->offset is set out of this
> >function after
> >get_node successfully.
> >
> how about change to tbo->bdev->man[TTM_PL_TT].gpu_offset? since
> gtt_mgr is for GTT only.
> >>>Not 100% correct either.
> >>>
> >>>The problem is that sometimes a GTT MM node will be bound for a BO
> >>>which is
> >>>still in VRAM (e.g. on eviction for example).
> >>>
> >>>So you need to check if (>mem == mem) and only then update the
> >>>tbo's
> >>>offset.
> >>>
> >>>Regards,
> >>>Christian.
> >>>
> >Regards,
> >David Zhou
> >
> >>  }
> >>  return r;
> ___
> 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] drm/amdgpu: fix gtt_mgr bo's offset

2016-09-20 Thread Flora Cui 
Does this change make sense? 

On Tue, Sep 20, 2016 at 01:02:20PM +0200, Christian König wrote:
> Am 20.09.2016 um 12:41 schrieb Flora Cui:
> >On Tue, Sep 20, 2016 at 06:19:33PM +0800, zhoucm1 wrote:
> >>
> >>On 2016年09月20日 17:09, Flora Cui wrote:
> >>>Change-Id: I89e9be3d5c96d46655f3a977fb557b20b4d87609
> >>>Signed-off-by: Flora Cui 
> >>>---
> >>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
> >>>b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>>index 262e872..b266cf7 100644
> >>>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >>>@@ -128,8 +128,8 @@ int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager 
> >>>*man,
> >>>   if (!r) {
> >>>   mem->start = node->start;
> >>>-  tbo->offset = (tbo->mem.start << PAGE_SHIFT) +
> >>>-  tbo->bdev->man[tbo->mem.mem_type].gpu_offset;
> >>>+  tbo->offset = (mem->start << PAGE_SHIFT) +
> >>>+  tbo->bdev->man[mem->mem_type].gpu_offset;
> >>mem->mem_type seems not set yet as well if I am correct.
> >>IIRC, I feel it's better that tbo->offset is set out of this function after
> >>get_node successfully.
> >>
> >how about change to tbo->bdev->man[TTM_PL_TT].gpu_offset? since
> >gtt_mgr is for GTT only.
> 
> Not 100% correct either.
> 
> The problem is that sometimes a GTT MM node will be bound for a BO which is
> still in VRAM (e.g. on eviction for example).
> 
> So you need to check if (>mem == mem) and only then update the tbo's
> offset.
> 
> Regards,
> Christian.
> 
> >
> >>Regards,
> >>David Zhou
> >>
> >>>   }
> >>>   return r;
> >___
> >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
>From 411f93d78e8f3691ce480146374c1e603c2c6db6 Mon Sep 17 00:00:00 2001
From: Flora Cui 
Date: Tue, 20 Sep 2016 17:07:31 +0800
Subject: [PATCH] drm/amdgpu: fix gtt_mgr bo's offset

Change-Id: I89e9be3d5c96d46655f3a977fb557b20b4d87609
Signed-off-by: Flora Cui 
Reviewed-by: Hawking Zhang 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 262e872..f86c844 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -128,8 +128,9 @@ int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager *man,
 
 	if (!r) {
 		mem->start = node->start;
-		tbo->offset = (tbo->mem.start << PAGE_SHIFT) +
-		tbo->bdev->man[tbo->mem.mem_type].gpu_offset;
+		if (>mem == mem)
+			tbo->offset = (tbo->mem.start << PAGE_SHIFT) +
+			tbo->bdev->man[tbo->mem.mem_type].gpu_offset;
 	}
 
 	return r;
-- 
2.7.4

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

Re: [PATCH] drm/amdgpu: fix gtt_mgr bo's offset

2016-09-20 Thread Flora Cui 
On Tue, Sep 20, 2016 at 06:19:33PM +0800, zhoucm1 wrote:
> 
> 
> On 2016年09月20日 17:09, Flora Cui wrote:
> >Change-Id: I89e9be3d5c96d46655f3a977fb557b20b4d87609
> >Signed-off-by: Flora Cui 
> >---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
> >b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >index 262e872..b266cf7 100644
> >--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> >@@ -128,8 +128,8 @@ int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager 
> >*man,
> > if (!r) {
> > mem->start = node->start;
> >-tbo->offset = (tbo->mem.start << PAGE_SHIFT) +
> >-tbo->bdev->man[tbo->mem.mem_type].gpu_offset;
> >+tbo->offset = (mem->start << PAGE_SHIFT) +
> >+tbo->bdev->man[mem->mem_type].gpu_offset;
> mem->mem_type seems not set yet as well if I am correct.
> IIRC, I feel it's better that tbo->offset is set out of this function after
> get_node successfully.
> 
how about change to tbo->bdev->man[TTM_PL_TT].gpu_offset? since
gtt_mgr is for GTT only.

> Regards,
> David Zhou
> 
> > }
> > return r;
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx