All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/radeon: update comments to clarify VM setup
@ 2012-10-08 13:50 alexdeucher
  2012-10-08 15:29 ` Christian König
  0 siblings, 1 reply; 4+ messages in thread
From: alexdeucher @ 2012-10-08 13:50 UTC (permalink / raw)
  To: airlied, dri-devel; +Cc: Alex Deucher

From: Alex Deucher <alexander.deucher@amd.com>

The actual set up and assignment of VM page tables
is done on the fly in radeon_gart.c.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/radeon/ni.c            |    4 ++++
 drivers/gpu/drm/radeon/radeon_device.c |    3 +++
 drivers/gpu/drm/radeon/si.c            |    7 ++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index 48e2337..cfb9276 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -770,6 +770,10 @@ static int cayman_pcie_gart_enable(struct radeon_device *rdev)
 	WREG32(0x15DC, 0);
 
 	/* empty context1-7 */
+	/* Assign the pt base to something valid for now; the pts used for
+	 * the VMs are determined by the application and setup and assigned
+	 * on the fly in the vm part of radeon_gart.c
+	 */
 	for (i = 1; i < 8; i++) {
 		WREG32(VM_CONTEXT0_PAGE_TABLE_START_ADDR + (i << 2), 0);
 		WREG32(VM_CONTEXT0_PAGE_TABLE_END_ADDR + (i << 2), 0);
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 64a4264..1fad47e 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1018,6 +1018,9 @@ int radeon_device_init(struct radeon_device *rdev,
 		return r;
 	/* initialize vm here */
 	mutex_init(&rdev->vm_manager.lock);
+	/* FIXME start with 4G, once using 2 level pt switch to full
+	 * vm size space
+	 */
 	rdev->vm_manager.max_pfn = 1 << 20;
 	INIT_LIST_HEAD(&rdev->vm_manager.lru_vm);
 
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index c76825f..f272ead 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -2407,12 +2407,13 @@ static int si_pcie_gart_enable(struct radeon_device *rdev)
 	WREG32(0x15DC, 0);
 
 	/* empty context1-15 */
-	/* FIXME start with 4G, once using 2 level pt switch to full
-	 * vm size space
-	 */
 	/* set vm size, must be a multiple of 4 */
 	WREG32(VM_CONTEXT1_PAGE_TABLE_START_ADDR, 0);
 	WREG32(VM_CONTEXT1_PAGE_TABLE_END_ADDR, rdev->vm_manager.max_pfn);
+	/* Assign the pt base to something valid for now; the pts used for
+	 * the VMs are determined by the application and setup and assigned
+	 * on the fly in the vm part of radeon_gart.c
+	 */
 	for (i = 1; i < 16; i++) {
 		if (i < 8)
 			WREG32(VM_CONTEXT0_PAGE_TABLE_BASE_ADDR + (i << 2),
-- 
1.7.7.5

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/radeon: update comments to clarify VM setup
  2012-10-08 13:50 [PATCH] drm/radeon: update comments to clarify VM setup alexdeucher
@ 2012-10-08 15:29 ` Christian König
  2012-10-08 15:49   ` Alex Deucher
  0 siblings, 1 reply; 4+ messages in thread
From: Christian König @ 2012-10-08 15:29 UTC (permalink / raw)
  To: alexdeucher; +Cc: Alex Deucher, dri-devel

On 08.10.2012 15:50, alexdeucher@gmail.com wrote:
> From: Alex Deucher <alexander.deucher@amd.com>
>
> The actual set up and assignment of VM page tables
> is done on the fly in radeon_gart.c.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
One comment below, apart from that it's:

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/radeon/ni.c            |    4 ++++
>   drivers/gpu/drm/radeon/radeon_device.c |    3 +++
>   drivers/gpu/drm/radeon/si.c            |    7 ++++---
>   3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
> index 48e2337..cfb9276 100644
> --- a/drivers/gpu/drm/radeon/ni.c
> +++ b/drivers/gpu/drm/radeon/ni.c
> @@ -770,6 +770,10 @@ static int cayman_pcie_gart_enable(struct radeon_device *rdev)
>   	WREG32(0x15DC, 0);
>   
>   	/* empty context1-7 */
> +	/* Assign the pt base to something valid for now; the pts used for
> +	 * the VMs are determined by the application and setup and assigned
> +	 * on the fly in the vm part of radeon_gart.c
> +	 */
>   	for (i = 1; i < 8; i++) {
>   		WREG32(VM_CONTEXT0_PAGE_TABLE_START_ADDR + (i << 2), 0);
>   		WREG32(VM_CONTEXT0_PAGE_TABLE_END_ADDR + (i << 2), 0);
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index 64a4264..1fad47e 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1018,6 +1018,9 @@ int radeon_device_init(struct radeon_device *rdev,
>   		return r;
>   	/* initialize vm here */
>   	mutex_init(&rdev->vm_manager.lock);
> +	/* FIXME start with 4G, once using 2 level pt switch to full
> +	 * vm size space
> +	 */
Even with 2 level pt I don't think we want to switch to the full vm size 
space.

Having 40bits address space minus 12bits offsets gives us 28bits for the 
page directory/page table, assuming a 50/50 split between them we would 
get 2 ^ 14 * 8 = 128kb for each page, which in my eyes is a bit much 
(and that gets even worth when we get 48bits address space.....)

I would rather say to make max_pfn depend on the available memory, so 
that a single application can at least map all VRAM + GART memory.

>   	rdev->vm_manager.max_pfn = 1 << 20;
>   	INIT_LIST_HEAD(&rdev->vm_manager.lru_vm);
>   
> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
> index c76825f..f272ead 100644
> --- a/drivers/gpu/drm/radeon/si.c
> +++ b/drivers/gpu/drm/radeon/si.c
> @@ -2407,12 +2407,13 @@ static int si_pcie_gart_enable(struct radeon_device *rdev)
>   	WREG32(0x15DC, 0);
>   
>   	/* empty context1-15 */
> -	/* FIXME start with 4G, once using 2 level pt switch to full
> -	 * vm size space
> -	 */
>   	/* set vm size, must be a multiple of 4 */
>   	WREG32(VM_CONTEXT1_PAGE_TABLE_START_ADDR, 0);
>   	WREG32(VM_CONTEXT1_PAGE_TABLE_END_ADDR, rdev->vm_manager.max_pfn);
> +	/* Assign the pt base to something valid for now; the pts used for
> +	 * the VMs are determined by the application and setup and assigned
> +	 * on the fly in the vm part of radeon_gart.c
> +	 */
>   	for (i = 1; i < 16; i++) {
>   		if (i < 8)
>   			WREG32(VM_CONTEXT0_PAGE_TABLE_BASE_ADDR + (i << 2),

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/radeon: update comments to clarify VM setup
  2012-10-08 15:29 ` Christian König
@ 2012-10-08 15:49   ` Alex Deucher
  2012-10-08 16:02     ` [PATCH 1/2] drm/radeon: update comments to clarify VM setup (v2) alexdeucher
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Deucher @ 2012-10-08 15:49 UTC (permalink / raw)
  To: Christian König; +Cc: Alex Deucher, dri-devel

On Mon, Oct 8, 2012 at 11:29 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 08.10.2012 15:50, alexdeucher@gmail.com wrote:
>>
>> From: Alex Deucher <alexander.deucher@amd.com>
>>
>> The actual set up and assignment of VM page tables
>> is done on the fly in radeon_gart.c.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>
> One comment below, apart from that it's:
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
>
>> ---
>>   drivers/gpu/drm/radeon/ni.c            |    4 ++++
>>   drivers/gpu/drm/radeon/radeon_device.c |    3 +++
>>   drivers/gpu/drm/radeon/si.c            |    7 ++++---
>>   3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
>> index 48e2337..cfb9276 100644
>> --- a/drivers/gpu/drm/radeon/ni.c
>> +++ b/drivers/gpu/drm/radeon/ni.c
>> @@ -770,6 +770,10 @@ static int cayman_pcie_gart_enable(struct
>> radeon_device *rdev)
>>         WREG32(0x15DC, 0);
>>         /* empty context1-7 */
>> +       /* Assign the pt base to something valid for now; the pts used for
>> +        * the VMs are determined by the application and setup and
>> assigned
>> +        * on the fly in the vm part of radeon_gart.c
>> +        */
>>         for (i = 1; i < 8; i++) {
>>                 WREG32(VM_CONTEXT0_PAGE_TABLE_START_ADDR + (i << 2), 0);
>>                 WREG32(VM_CONTEXT0_PAGE_TABLE_END_ADDR + (i << 2), 0);
>> diff --git a/drivers/gpu/drm/radeon/radeon_device.c
>> b/drivers/gpu/drm/radeon/radeon_device.c
>> index 64a4264..1fad47e 100644
>> --- a/drivers/gpu/drm/radeon/radeon_device.c
>> +++ b/drivers/gpu/drm/radeon/radeon_device.c
>> @@ -1018,6 +1018,9 @@ int radeon_device_init(struct radeon_device *rdev,
>>                 return r;
>>         /* initialize vm here */
>>         mutex_init(&rdev->vm_manager.lock);
>> +       /* FIXME start with 4G, once using 2 level pt switch to full
>> +        * vm size space
>> +        */
>
> Even with 2 level pt I don't think we want to switch to the full vm size
> space.
>
> Having 40bits address space minus 12bits offsets gives us 28bits for the
> page directory/page table, assuming a 50/50 split between them we would get
> 2 ^ 14 * 8 = 128kb for each page, which in my eyes is a bit much (and that
> gets even worth when we get 48bits address space.....)
>
> I would rather say to make max_pfn depend on the available memory, so that a
> single application can at least map all VRAM + GART memory.

I'll make the comment a bit clearer.  I was mostly just moving the
comment to be near the code that sets that value.

Alex

>
>
>>         rdev->vm_manager.max_pfn = 1 << 20;
>>         INIT_LIST_HEAD(&rdev->vm_manager.lru_vm);
>>   diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
>> index c76825f..f272ead 100644
>> --- a/drivers/gpu/drm/radeon/si.c
>> +++ b/drivers/gpu/drm/radeon/si.c
>> @@ -2407,12 +2407,13 @@ static int si_pcie_gart_enable(struct
>> radeon_device *rdev)
>>         WREG32(0x15DC, 0);
>>         /* empty context1-15 */
>> -       /* FIXME start with 4G, once using 2 level pt switch to full
>> -        * vm size space
>> -        */
>>         /* set vm size, must be a multiple of 4 */
>>         WREG32(VM_CONTEXT1_PAGE_TABLE_START_ADDR, 0);
>>         WREG32(VM_CONTEXT1_PAGE_TABLE_END_ADDR, rdev->vm_manager.max_pfn);
>> +       /* Assign the pt base to something valid for now; the pts used for
>> +        * the VMs are determined by the application and setup and
>> assigned
>> +        * on the fly in the vm part of radeon_gart.c
>> +        */
>>         for (i = 1; i < 16; i++) {
>>                 if (i < 8)
>>                         WREG32(VM_CONTEXT0_PAGE_TABLE_BASE_ADDR + (i <<
>> 2),
>
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH 1/2] drm/radeon: update comments to clarify VM setup (v2)
  2012-10-08 15:49   ` Alex Deucher
@ 2012-10-08 16:02     ` alexdeucher
  0 siblings, 0 replies; 4+ messages in thread
From: alexdeucher @ 2012-10-08 16:02 UTC (permalink / raw)
  To: airlied, dri-devel; +Cc: Alex Deucher

From: Alex Deucher <alexander.deucher@amd.com>

The actual set up and assignment of VM page tables
is done on the fly in radeon_gart.c.

v2: update vm size comments

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/ni.c            |    4 ++++
 drivers/gpu/drm/radeon/radeon_device.c |    4 ++++
 drivers/gpu/drm/radeon/si.c            |    7 ++++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index 9a46f7d..6883e14 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -770,6 +770,10 @@ static int cayman_pcie_gart_enable(struct radeon_device *rdev)
 	WREG32(0x15DC, 0);
 
 	/* empty context1-7 */
+	/* Assign the pt base to something valid for now; the pts used for
+	 * the VMs are determined by the application and setup and assigned
+	 * on the fly in the vm part of radeon_gart.c
+	 */
 	for (i = 1; i < 8; i++) {
 		WREG32(VM_CONTEXT0_PAGE_TABLE_START_ADDR + (i << 2), 0);
 		WREG32(VM_CONTEXT0_PAGE_TABLE_END_ADDR + (i << 2), 0);
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 64a4264..bd13ca0 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1018,6 +1018,10 @@ int radeon_device_init(struct radeon_device *rdev,
 		return r;
 	/* initialize vm here */
 	mutex_init(&rdev->vm_manager.lock);
+	/* Adjust VM size here.
+	 * Currently set to 4GB ((1 << 20) 4k pages).
+	 * Max GPUVM size for cayman and SI is 40 bits.
+	 */
 	rdev->vm_manager.max_pfn = 1 << 20;
 	INIT_LIST_HEAD(&rdev->vm_manager.lru_vm);
 
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index c76825f..f272ead 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -2407,12 +2407,13 @@ static int si_pcie_gart_enable(struct radeon_device *rdev)
 	WREG32(0x15DC, 0);
 
 	/* empty context1-15 */
-	/* FIXME start with 4G, once using 2 level pt switch to full
-	 * vm size space
-	 */
 	/* set vm size, must be a multiple of 4 */
 	WREG32(VM_CONTEXT1_PAGE_TABLE_START_ADDR, 0);
 	WREG32(VM_CONTEXT1_PAGE_TABLE_END_ADDR, rdev->vm_manager.max_pfn);
+	/* Assign the pt base to something valid for now; the pts used for
+	 * the VMs are determined by the application and setup and assigned
+	 * on the fly in the vm part of radeon_gart.c
+	 */
 	for (i = 1; i < 16; i++) {
 		if (i < 8)
 			WREG32(VM_CONTEXT0_PAGE_TABLE_BASE_ADDR + (i << 2),
-- 
1.7.7.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-10-08 16:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-08 13:50 [PATCH] drm/radeon: update comments to clarify VM setup alexdeucher
2012-10-08 15:29 ` Christian König
2012-10-08 15:49   ` Alex Deucher
2012-10-08 16:02     ` [PATCH 1/2] drm/radeon: update comments to clarify VM setup (v2) alexdeucher

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.