[01/18] drm/amdgpu: abstract gart table initialization for gfxhub/mmhub

Submitted by Huang, Ray on May 31, 2017, 4:14 p.m.

Details

Message ID 1496247293-16429-2-git-send-email-ray.huang@amd.com
State New
Headers show
Series "Vega10 S3 following up" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Huang, Ray May 31, 2017, 4:14 p.m.
Signed-off-by: Huang Rui <ray.huang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 33 +++++++++++++++++++-------------
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 33 +++++++++++++++++++-------------
 2 files changed, 40 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index 7c430c4..8cf30b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -31,6 +31,24 @@ 
 
 #include "soc15_common.h"
 
+static void gfxhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
+{
+	uint64_t value;
+
+	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
+	value = adev->gart.table_addr - adev->mc.vram_start
+		+ adev->vm_manager.vram_base_offset;
+	value &= 0x0000FFFFFFFFF000ULL;
+	value |= 0x1; /*valid bit*/
+
+	WREG32(SOC15_REG_OFFSET(GC, 0,
+				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
+		(u32)value);
+	WREG32(SOC15_REG_OFFSET(GC, 0,
+				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
+		(u32)(value >> 32));
+}
+
 int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
 {
 	u32 tmp;
@@ -38,6 +56,8 @@  int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
 	u32 i;
 
 	/* Program MC. */
+	gfxhub_v1_0_init_pt_regs(adev);
+
 	/* Update configuration */
 	WREG32(SOC15_REG_OFFSET(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
 		adev->mc.vram_start >> 18);
@@ -154,19 +174,6 @@  int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
 				mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
 		(u32)(adev->mc.gtt_end >> 44));
 
-	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
-	value = adev->gart.table_addr - adev->mc.vram_start
-		+ adev->vm_manager.vram_base_offset;
-	value &= 0x0000FFFFFFFFF000ULL;
-	value |= 0x1; /*valid bit*/
-
-	WREG32(SOC15_REG_OFFSET(GC, 0,
-				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
-		(u32)value);
-	WREG32(SOC15_REG_OFFSET(GC, 0,
-				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
-		(u32)(value >> 32));
-
 	WREG32(SOC15_REG_OFFSET(GC, 0,
 				mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
 		(u32)(adev->dummy_page.addr >> 12));
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
index afd9d85..84eb3a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
@@ -44,6 +44,24 @@  u64 mmhub_v1_0_get_fb_location(struct amdgpu_device *adev)
 	return base;
 }
 
+static void mmhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
+{
+	uint64_t value;
+
+	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
+	value = adev->gart.table_addr - adev->mc.vram_start +
+		adev->vm_manager.vram_base_offset;
+	value &= 0x0000FFFFFFFFF000ULL;
+	value |= 0x1; /* valid bit */
+
+	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
+				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
+		(u32)value);
+	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
+				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
+		(u32)(value >> 32));
+}
+
 int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
 {
 	u32 tmp;
@@ -52,6 +70,8 @@  int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
 	u32 i;
 
 	/* Program MC. */
+	mmhub_v1_0_init_pt_regs(adev);
+
 	/* Update configuration */
 	DRM_INFO("%s -- in\n", __func__);
 	WREG32(SOC15_REG_OFFSET(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
@@ -168,19 +188,6 @@  int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
 				mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
 		(u32)(adev->mc.gtt_end >> 44));
 
-	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
-	value = adev->gart.table_addr - adev->mc.vram_start +
-		adev->vm_manager.vram_base_offset;
-	value &= 0x0000FFFFFFFFF000ULL;
-	value |= 0x1; /* valid bit */
-
-	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
-				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
-		(u32)value);
-	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
-				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
-		(u32)(value >> 32));
-
 	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
 				mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
 		(u32)(adev->dummy_page.addr >> 12));

Comments

Am 31.05.2017 um 18:14 schrieb Huang Rui:
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 33 +++++++++++++++++++-------------
>   drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 33 +++++++++++++++++++-------------
>   2 files changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> index 7c430c4..8cf30b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> @@ -31,6 +31,24 @@
>   
>   #include "soc15_common.h"
>   
> +static void gfxhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> +{
> +	uint64_t value;
> +
> +	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> +	value = adev->gart.table_addr - adev->mc.vram_start
> +		+ adev->vm_manager.vram_base_offset;
> +	value &= 0x0000FFFFFFFFF000ULL;
> +	value |= 0x1; /*valid bit*/
> +
> +	WREG32(SOC15_REG_OFFSET(GC, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> +		(u32)value);
> +	WREG32(SOC15_REG_OFFSET(GC, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> +		(u32)(value >> 32));
> +}
> +
>   int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   {
>   	u32 tmp;
> @@ -38,6 +56,8 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   	u32 i;
>   
>   	/* Program MC. */
> +	gfxhub_v1_0_init_pt_regs(adev);
> +
>   	/* Update configuration */
>   	WREG32(SOC15_REG_OFFSET(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
>   		adev->mc.vram_start >> 18);
> @@ -154,19 +174,6 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   				mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
>   		(u32)(adev->mc.gtt_end >> 44));
>   
> -	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> -	value = adev->gart.table_addr - adev->mc.vram_start
> -		+ adev->vm_manager.vram_base_offset;
> -	value &= 0x0000FFFFFFFFF000ULL;
> -	value |= 0x1; /*valid bit*/
> -
> -	WREG32(SOC15_REG_OFFSET(GC, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> -		(u32)value);
> -	WREG32(SOC15_REG_OFFSET(GC, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> -		(u32)(value >> 32));
> -
>   	WREG32(SOC15_REG_OFFSET(GC, 0,
>   				mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
>   		(u32)(adev->dummy_page.addr >> 12));
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> index afd9d85..84eb3a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> @@ -44,6 +44,24 @@ u64 mmhub_v1_0_get_fb_location(struct amdgpu_device *adev)
>   	return base;
>   }
>   
> +static void mmhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> +{
> +	uint64_t value;
> +
> +	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> +	value = adev->gart.table_addr - adev->mc.vram_start +
> +		adev->vm_manager.vram_base_offset;
> +	value &= 0x0000FFFFFFFFF000ULL;
> +	value |= 0x1; /* valid bit */
> +
> +	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> +		(u32)value);
> +	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> +		(u32)(value >> 32));

While at it please use the upper_32_bits() and lower_32_bits() macros 
for this.

Additional to that since you cleanup the code anyway (which is very 
appreciated!) please use the WREG32_SOC15() macro instead.

Regards,
Christian.

> +}
> +
>   int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   {
>   	u32 tmp;
> @@ -52,6 +70,8 @@ int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   	u32 i;
>   
>   	/* Program MC. */
> +	mmhub_v1_0_init_pt_regs(adev);
> +
>   	/* Update configuration */
>   	DRM_INFO("%s -- in\n", __func__);
>   	WREG32(SOC15_REG_OFFSET(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
> @@ -168,19 +188,6 @@ int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   				mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
>   		(u32)(adev->mc.gtt_end >> 44));
>   
> -	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> -	value = adev->gart.table_addr - adev->mc.vram_start +
> -		adev->vm_manager.vram_base_offset;
> -	value &= 0x0000FFFFFFFFF000ULL;
> -	value |= 0x1; /* valid bit */
> -
> -	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> -		(u32)value);
> -	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> -		(u32)(value >> 32));
> -
>   	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
>   				mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
>   		(u32)(adev->dummy_page.addr >> 12));
On Thu, Jun 01, 2017 at 01:02:18AM +0800, Koenig, Christian wrote:
> Am 31.05.2017 um 18:14 schrieb Huang Rui:
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 33
> +++++++++++++++++++-------------
> >   drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 33
> +++++++++++++++++++-------------
> >   2 files changed, 40 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/
> amdgpu/gfxhub_v1_0.c
> > index 7c430c4..8cf30b7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> > @@ -31,6 +31,24 @@
> >  
> >   #include "soc15_common.h"
> >  
> > +static void gfxhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> > +{
> > +     uint64_t value;
> > +
> > +     BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> > +     value = adev->gart.table_addr - adev->mc.vram_start
> > +             + adev->vm_manager.vram_base_offset;
> > +     value &= 0x0000FFFFFFFFF000ULL;
> > +     value |= 0x1; /*valid bit*/
> > +
> > +     WREG32(SOC15_REG_OFFSET(GC, 0,
> > +                             mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> > +             (u32)value);
> > +     WREG32(SOC15_REG_OFFSET(GC, 0,
> > +                             mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> > +             (u32)(value >> 32));
> > +}
> > +
> >   int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
> >   {
> >        u32 tmp;
> > @@ -38,6 +56,8 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
> >        u32 i;
> >  
> >        /* Program MC. */
> > +     gfxhub_v1_0_init_pt_regs(adev);
> > +
> >        /* Update configuration */
> >        WREG32(SOC15_REG_OFFSET(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
> >                adev->mc.vram_start >> 18);
> > @@ -154,19 +174,6 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
> >                                mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
> >                (u32)(adev->mc.gtt_end >> 44));
> >  
> > -     BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> > -     value = adev->gart.table_addr - adev->mc.vram_start
> > -             + adev->vm_manager.vram_base_offset;
> > -     value &= 0x0000FFFFFFFFF000ULL;
> > -     value |= 0x1; /*valid bit*/
> > -
> > -     WREG32(SOC15_REG_OFFSET(GC, 0,
> > -                             mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> > -             (u32)value);
> > -     WREG32(SOC15_REG_OFFSET(GC, 0,
> > -                             mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> > -             (u32)(value >> 32));
> > -
> >        WREG32(SOC15_REG_OFFSET(GC, 0,
> >                                mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
> >                (u32)(adev->dummy_page.addr >> 12));
> > diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/
> amdgpu/mmhub_v1_0.c
> > index afd9d85..84eb3a3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> > @@ -44,6 +44,24 @@ u64 mmhub_v1_0_get_fb_location(struct amdgpu_device *adev)
> >        return base;
> >   }
> >  
> > +static void mmhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> > +{
> > +     uint64_t value;
> > +
> > +     BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> > +     value = adev->gart.table_addr - adev->mc.vram_start +
> > +             adev->vm_manager.vram_base_offset;
> > +     value &= 0x0000FFFFFFFFF000ULL;
> > +     value |= 0x1; /* valid bit */
> > +
> > +     WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> > +                             mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> > +             (u32)value);
> > +     WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> > +                             mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> > +             (u32)(value >> 32));
> 
> While at it please use the upper_32_bits() and lower_32_bits() macros
> for this.
> 
> Additional to that since you cleanup the code anyway (which is very
> appreciated!) please use the WREG32_SOC15() macro instead.
> 

My pleasure. I found original mmhub/gfxhub programming sequence was really
hard to read when I was debugging S3 issue. So I have to break it down to
make it clearly. 

Could I add one more patches at top of patch set to use
WREG32_SOC15/REG32_SOC15 instead for the whole GMC, GFXHUB, and MMHUB
blocks.

Thanks,
Ray
On 2017年06月01日 00:14, Huang Rui wrote:
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 33 +++++++++++++++++++-------------
>   drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 33 +++++++++++++++++++-------------
>   2 files changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> index 7c430c4..8cf30b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> @@ -31,6 +31,24 @@
>   
>   #include "soc15_common.h"
>   
> +static void gfxhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> +{
> +	uint64_t value;
> +
> +	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> +	value = adev->gart.table_addr - adev->mc.vram_start
> +		+ adev->vm_manager.vram_base_offset;
> +	value &= 0x0000FFFFFFFFF000ULL;
> +	value |= 0x1; /*valid bit*/
> +
> +	WREG32(SOC15_REG_OFFSET(GC, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> +		(u32)value);
> +	WREG32(SOC15_REG_OFFSET(GC, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> +		(u32)(value >> 32));
> +}
> +
>   int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   {
>   	u32 tmp;
> @@ -38,6 +56,8 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   	u32 i;
>   
>   	/* Program MC. */
> +	gfxhub_v1_0_init_pt_regs(adev);
> +
abstraction is fine, but why you change code location?

Regards,
David Zhou
>   	/* Update configuration */
>   	WREG32(SOC15_REG_OFFSET(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
>   		adev->mc.vram_start >> 18);
> @@ -154,19 +174,6 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   				mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
>   		(u32)(adev->mc.gtt_end >> 44));
>   
> -	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> -	value = adev->gart.table_addr - adev->mc.vram_start
> -		+ adev->vm_manager.vram_base_offset;
> -	value &= 0x0000FFFFFFFFF000ULL;
> -	value |= 0x1; /*valid bit*/
> -
> -	WREG32(SOC15_REG_OFFSET(GC, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> -		(u32)value);
> -	WREG32(SOC15_REG_OFFSET(GC, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> -		(u32)(value >> 32));
> -
>   	WREG32(SOC15_REG_OFFSET(GC, 0,
>   				mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
>   		(u32)(adev->dummy_page.addr >> 12));
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> index afd9d85..84eb3a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> @@ -44,6 +44,24 @@ u64 mmhub_v1_0_get_fb_location(struct amdgpu_device *adev)
>   	return base;
>   }
>   
> +static void mmhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> +{
> +	uint64_t value;
> +
> +	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> +	value = adev->gart.table_addr - adev->mc.vram_start +
> +		adev->vm_manager.vram_base_offset;
> +	value &= 0x0000FFFFFFFFF000ULL;
> +	value |= 0x1; /* valid bit */
> +
> +	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> +		(u32)value);
> +	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> +		(u32)(value >> 32));
> +}
> +
>   int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   {
>   	u32 tmp;
> @@ -52,6 +70,8 @@ int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   	u32 i;
>   
>   	/* Program MC. */
> +	mmhub_v1_0_init_pt_regs(adev);
> +
>   	/* Update configuration */
>   	DRM_INFO("%s -- in\n", __func__);
>   	WREG32(SOC15_REG_OFFSET(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR),
> @@ -168,19 +188,6 @@ int mmhub_v1_0_gart_enable(struct amdgpu_device *adev)
>   				mmVM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32),
>   		(u32)(adev->mc.gtt_end >> 44));
>   
> -	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> -	value = adev->gart.table_addr - adev->mc.vram_start +
> -		adev->vm_manager.vram_base_offset;
> -	value &= 0x0000FFFFFFFFF000ULL;
> -	value |= 0x1; /* valid bit */
> -
> -	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> -		(u32)value);
> -	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
> -				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> -		(u32)(value >> 32));
> -
>   	WREG32(SOC15_REG_OFFSET(MMHUB, 0,
>   				mmVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32),
>   		(u32)(adev->dummy_page.addr >> 12));
On Thu, Jun 01, 2017 at 12:19:19PM +0800, zhoucm1 wrote:
> 
> 
> On 2017年06月01日 00:14, Huang Rui wrote:
> >Signed-off-by: Huang Rui <ray.huang@amd.com>
> >---
> >  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 33 +++++++++++++++++++-------------
> >  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 33 +++++++++++++++++++-------------
> >  2 files changed, 40 insertions(+), 26 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> >index 7c430c4..8cf30b7 100644
> >--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> >+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> >@@ -31,6 +31,24 @@
> >  #include "soc15_common.h"
> >+static void gfxhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
> >+{
> >+	uint64_t value;
> >+
> >+	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
> >+	value = adev->gart.table_addr - adev->mc.vram_start
> >+		+ adev->vm_manager.vram_base_offset;
> >+	value &= 0x0000FFFFFFFFF000ULL;
> >+	value |= 0x1; /*valid bit*/
> >+
> >+	WREG32(SOC15_REG_OFFSET(GC, 0,
> >+				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
> >+		(u32)value);
> >+	WREG32(SOC15_REG_OFFSET(GC, 0,
> >+				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
> >+		(u32)(value >> 32));
> >+}
> >+
> >  int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
> >  {
> >  	u32 tmp;
> >@@ -38,6 +56,8 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
> >  	u32 i;
> >  	/* Program MC. */
> >+	gfxhub_v1_0_init_pt_regs(adev);
> >+
> abstraction is fine, but why you change code location?
> 

Because we would better to align the programming sequence with windows
part. That's helpful to debug in future.

Thanks,
Ray
On 2017年06月01日 13:29, Huang Rui wrote:
> On Thu, Jun 01, 2017 at 12:19:19PM +0800, zhoucm1 wrote:
>>
>> On 2017年06月01日 00:14, Huang Rui wrote:
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 33 +++++++++++++++++++-------------
>>>   drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 33 +++++++++++++++++++-------------
>>>   2 files changed, 40 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> index 7c430c4..8cf30b7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> @@ -31,6 +31,24 @@
>>>   #include "soc15_common.h"
>>> +static void gfxhub_v1_0_init_pt_regs(struct amdgpu_device *adev)
>>> +{
>>> +	uint64_t value;
>>> +
>>> +	BUG_ON(adev->gart.table_addr & (~0x0000FFFFFFFFF000ULL));
>>> +	value = adev->gart.table_addr - adev->mc.vram_start
>>> +		+ adev->vm_manager.vram_base_offset;
>>> +	value &= 0x0000FFFFFFFFF000ULL;
>>> +	value |= 0x1; /*valid bit*/
>>> +
>>> +	WREG32(SOC15_REG_OFFSET(GC, 0,
>>> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_LO32),
>>> +		(u32)value);
>>> +	WREG32(SOC15_REG_OFFSET(GC, 0,
>>> +				mmVM_CONTEXT0_PAGE_TABLE_BASE_ADDR_HI32),
>>> +		(u32)(value >> 32));
>>> +}
>>> +
>>>   int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>>>   {
>>>   	u32 tmp;
>>> @@ -38,6 +56,8 @@ int gfxhub_v1_0_gart_enable(struct amdgpu_device *adev)
>>>   	u32 i;
>>>   	/* Program MC. */
>>> +	gfxhub_v1_0_init_pt_regs(adev);
>>> +
>> abstraction is fine, but why you change code location?
>>
> Because we would better to align the programming sequence with windows
> part.
If you make sure it aligns with windows, then that's good. Otherwise we 
should keep same with previous.

Regards,
David Zhou
> That's helpful to debug in future.
>
> Thanks,
> Ray