All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jerome Glisse <j.glisse@gmail.com>
To: "Christian König" <deathsimple@vodafone.de>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/radeon: add proper support for RADEON_VM_BLOCK_SIZE
Date: Thu, 1 May 2014 13:17:52 -0400	[thread overview]
Message-ID: <20140501171751.GA3296@gmail.com> (raw)
In-Reply-To: <1398963153-1635-2-git-send-email-deathsimple@vodafone.de>

On Thu, May 01, 2014 at 06:52:33PM +0200, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> This patch makes it possible to decide how many address
> bits are spend on the page directory vs the page tables.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

NAK on the basis you are modifying default behavior and hardware
programming in same patch see below.

> ---
>  drivers/gpu/drm/radeon/cik.c       | 1 +
>  drivers/gpu/drm/radeon/cikd.h      | 1 +
>  drivers/gpu/drm/radeon/ni.c        | 1 +
>  drivers/gpu/drm/radeon/nid.h       | 1 +
>  drivers/gpu/drm/radeon/radeon.h    | 2 +-
>  drivers/gpu/drm/radeon/radeon_vm.c | 4 +++-
>  drivers/gpu/drm/radeon/si.c        | 1 +
>  drivers/gpu/drm/radeon/sid.h       | 1 +
>  8 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> index 38da9f3..f11a9ab 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -5445,6 +5445,7 @@ static int cik_pcie_gart_enable(struct radeon_device *rdev)
>  	       (u32)(rdev->dummy_page.addr >> 12));
>  	WREG32(VM_CONTEXT1_CNTL2, 4);
>  	WREG32(VM_CONTEXT1_CNTL, ENABLE_CONTEXT | PAGE_TABLE_DEPTH(1) |
> +				PAGE_TABLE_BLOCK_SIZE(RADEON_VM_BLOCK_SIZE - 9) |
>  				RANGE_PROTECTION_FAULT_ENABLE_INTERRUPT |
>  				RANGE_PROTECTION_FAULT_ENABLE_DEFAULT |
>  				DUMMY_PAGE_PROTECTION_FAULT_ENABLE_INTERRUPT |
> diff --git a/drivers/gpu/drm/radeon/cikd.h b/drivers/gpu/drm/radeon/cikd.h
> index dd79263..ae88660 100644
> --- a/drivers/gpu/drm/radeon/cikd.h
> +++ b/drivers/gpu/drm/radeon/cikd.h
> @@ -482,6 +482,7 @@
>  #define		READ_PROTECTION_FAULT_ENABLE_DEFAULT		(1 << 16)
>  #define		WRITE_PROTECTION_FAULT_ENABLE_INTERRUPT		(1 << 18)
>  #define		WRITE_PROTECTION_FAULT_ENABLE_DEFAULT		(1 << 19)
> +#define		PAGE_TABLE_BLOCK_SIZE(x)			(((x) & 0xF) << 24)
>  #define VM_CONTEXT1_CNTL				0x1414
>  #define VM_CONTEXT0_CNTL2				0x1430
>  #define VM_CONTEXT1_CNTL2				0x1434
> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
> index 5e8db9b..1d3209f 100644
> --- a/drivers/gpu/drm/radeon/ni.c
> +++ b/drivers/gpu/drm/radeon/ni.c
> @@ -1268,6 +1268,7 @@ static int cayman_pcie_gart_enable(struct radeon_device *rdev)
>  	       (u32)(rdev->dummy_page.addr >> 12));
>  	WREG32(VM_CONTEXT1_CNTL2, 4);
>  	WREG32(VM_CONTEXT1_CNTL, ENABLE_CONTEXT | PAGE_TABLE_DEPTH(1) |
> +				PAGE_TABLE_BLOCK_SIZE(RADEON_VM_BLOCK_SIZE - 9) |
>  				RANGE_PROTECTION_FAULT_ENABLE_INTERRUPT |
>  				RANGE_PROTECTION_FAULT_ENABLE_DEFAULT |
>  				DUMMY_PAGE_PROTECTION_FAULT_ENABLE_INTERRUPT |
> diff --git a/drivers/gpu/drm/radeon/nid.h b/drivers/gpu/drm/radeon/nid.h
> index d996033..2e12e4d 100644
> --- a/drivers/gpu/drm/radeon/nid.h
> +++ b/drivers/gpu/drm/radeon/nid.h
> @@ -128,6 +128,7 @@
>  #define		READ_PROTECTION_FAULT_ENABLE_DEFAULT		(1 << 16)
>  #define		WRITE_PROTECTION_FAULT_ENABLE_INTERRUPT		(1 << 18)
>  #define		WRITE_PROTECTION_FAULT_ENABLE_DEFAULT		(1 << 19)
> +#define		PAGE_TABLE_BLOCK_SIZE(x)			(((x) & 0xF) << 24)
>  #define VM_CONTEXT1_CNTL				0x1414
>  #define VM_CONTEXT0_CNTL2				0x1430
>  #define VM_CONTEXT1_CNTL2				0x1434
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index e3d6be3..2d80cf2 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -838,7 +838,7 @@ struct radeon_mec {
>  /* defines number of bits in page table versus page directory,
>   * a page is 4KB so we have 12 bits offset, 9 bits in the page
>   * table and the remaining 19 bits are in the page directory */
> -#define RADEON_VM_BLOCK_SIZE   9
> +#define RADEON_VM_BLOCK_SIZE   12

I would first do a patch that add all the code to set this bit inside the
vm context registers and then a patch that change the default current
value explaining why.

This would allow bisection to either find that the register setting is
guilty or that the default value change is the one introducing problem

But really i am being picky here.

Cheers,
Jérôme

>  
>  /* number of entries in page table */
>  #define RADEON_VM_PTE_COUNT (1 << RADEON_VM_BLOCK_SIZE)
> diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c
> index 6bf656e..3900ecf 100644
> --- a/drivers/gpu/drm/radeon/radeon_vm.c
> +++ b/drivers/gpu/drm/radeon/radeon_vm.c
> @@ -964,6 +964,8 @@ void radeon_vm_bo_invalidate(struct radeon_device *rdev,
>   */
>  int radeon_vm_init(struct radeon_device *rdev, struct radeon_vm *vm)
>  {
> +	const unsigned align = min(RADEON_VM_PTB_ALIGN_SIZE,
> +		RADEON_VM_PTE_COUNT * 8);
>  	unsigned pd_size, pd_entries, pts_size;
>  	int r;
>  
> @@ -985,7 +987,7 @@ int radeon_vm_init(struct radeon_device *rdev, struct radeon_vm *vm)
>  		return -ENOMEM;
>  	}
>  
> -	r = radeon_bo_create(rdev, pd_size, RADEON_VM_PTB_ALIGN_SIZE, false,
> +	r = radeon_bo_create(rdev, pd_size, align, false,
>  			     RADEON_GEM_DOMAIN_VRAM, NULL,
>  			     &vm->page_directory);
>  	if (r)
> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
> index dece3be..8fa6be5 100644
> --- a/drivers/gpu/drm/radeon/si.c
> +++ b/drivers/gpu/drm/radeon/si.c
> @@ -4095,6 +4095,7 @@ static int si_pcie_gart_enable(struct radeon_device *rdev)
>  	       (u32)(rdev->dummy_page.addr >> 12));
>  	WREG32(VM_CONTEXT1_CNTL2, 4);
>  	WREG32(VM_CONTEXT1_CNTL, ENABLE_CONTEXT | PAGE_TABLE_DEPTH(1) |
> +				PAGE_TABLE_BLOCK_SIZE(RADEON_VM_BLOCK_SIZE - 9) |
>  				RANGE_PROTECTION_FAULT_ENABLE_INTERRUPT |
>  				RANGE_PROTECTION_FAULT_ENABLE_DEFAULT |
>  				DUMMY_PAGE_PROTECTION_FAULT_ENABLE_INTERRUPT |
> diff --git a/drivers/gpu/drm/radeon/sid.h b/drivers/gpu/drm/radeon/sid.h
> index 683532f..da8f867 100644
> --- a/drivers/gpu/drm/radeon/sid.h
> +++ b/drivers/gpu/drm/radeon/sid.h
> @@ -362,6 +362,7 @@
>  #define		READ_PROTECTION_FAULT_ENABLE_DEFAULT		(1 << 16)
>  #define		WRITE_PROTECTION_FAULT_ENABLE_INTERRUPT		(1 << 18)
>  #define		WRITE_PROTECTION_FAULT_ENABLE_DEFAULT		(1 << 19)
> +#define		PAGE_TABLE_BLOCK_SIZE(x)			(((x) & 0xF) << 24)
>  #define VM_CONTEXT1_CNTL				0x1414
>  #define VM_CONTEXT0_CNTL2				0x1430
>  #define VM_CONTEXT1_CNTL2				0x1434
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2014-05-01 17:17 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-05-01 16:52 [PATCH 1/2] drm/radeon: add large PTE support for NI, SI and CIK v4 Christian König
2014-05-01 16:52 ` [PATCH 2/2] drm/radeon: add proper support for RADEON_VM_BLOCK_SIZE Christian König
2014-05-01 17:17   ` Jerome Glisse [this message]
2014-05-01 17:22     ` Christian König
2014-05-01 17:29 ` [PATCH 1/2] drm/radeon: add large PTE support for NI, SI and CIK v4 Jay Cornwall
2014-05-02 12:13   ` Christian König

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140501171751.GA3296@gmail.com \
    --to=j.glisse@gmail.com \
    --cc=deathsimple@vodafone.de \
    --cc=dri-devel@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.