diff mbox series

[2/8] drm/radeon: don't use ttm bo->offset

Message ID 20200219135322.56463-3-nirmoy.das@amd.com (mailing list archive)
State New, archived
Headers show
Series do not store GPU address in TTM | expand

Commit Message

Nirmoy Das Feb. 19, 2020, 1:53 p.m. UTC 
Calculate GPU offset in radeon_bo_gpu_offset without depending on
bo->offset

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Reviewed-and-tested-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/radeon.h        |  1 +
 drivers/gpu/drm/radeon/radeon_object.h | 16 +++++++++++++++-
 drivers/gpu/drm/radeon/radeon_ttm.c    |  4 +---
 3 files changed, 17 insertions(+), 4 deletions(-)

Comments

Luben Tuikov Feb. 21, 2020, 1:19 a.m. UTC | #1 
On 2020-02-19 08:53, Nirmoy Das wrote:
> Calculate GPU offset in radeon_bo_gpu_offset without depending on
> bo->offset
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> Reviewed-and-tested-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/radeon/radeon.h        |  1 +
>  drivers/gpu/drm/radeon/radeon_object.h | 16 +++++++++++++++-
>  drivers/gpu/drm/radeon/radeon_ttm.c    |  4 +---
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 30e32adc1fc6..b7c3fb2bfb54 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -2828,6 +2828,7 @@ extern void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size
>  extern void radeon_program_register_sequence(struct radeon_device *rdev,
>  					     const u32 *registers,
>  					     const u32 array_size);
> +struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev);
>  
>  /*
>   * vm
> diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h
> index d23f2ed4126e..4d37571c7ff5 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.h
> +++ b/drivers/gpu/drm/radeon/radeon_object.h
> @@ -90,7 +90,21 @@ static inline void radeon_bo_unreserve(struct radeon_bo *bo)
>   */
>  static inline u64 radeon_bo_gpu_offset(struct radeon_bo *bo)
>  {
> -	return bo->tbo.offset;
> +	struct radeon_device *rdev;
> +	u64 start = 0;
> +
> +	rdev = radeon_get_rdev(bo->tbo.bdev);
> +
> +	switch(bo->tbo.mem.mem_type) {

LKCS wants a space after a keyword, "switch (" .

> +	case TTM_PL_TT:
> +		start = rdev->mc.gtt_start;
> +		break;
> +	case TTM_PL_VRAM:
> +		start = rdev->mc.vram_start;
> +		break;
> +	}

Could this lookup have been parameterized by "mem_type"
to be looked up by an index (possibly "mem_type") to result
in something new like (pseudo-code):
     start = rdev->mc.mem_start_table[bo->tbo.mem.mem_type];
Where "mem_start_table" is a new table holding memory starts
of particular memories.
Then you don't need the switch-case.

Regards,
Luben

> +
> +	return (bo->tbo.mem.start << PAGE_SHIFT) + start;
>  }
>  
>  static inline unsigned long radeon_bo_size(struct radeon_bo *bo)
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index badf1b6d1549..1c8303468e8f 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -56,7 +56,7 @@
>  static int radeon_ttm_debugfs_init(struct radeon_device *rdev);
>  static void radeon_ttm_debugfs_fini(struct radeon_device *rdev);
>  
> -static struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
> +struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
>  {
>  	struct radeon_mman *mman;
>  	struct radeon_device *rdev;
> @@ -82,7 +82,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>  		break;
>  	case TTM_PL_TT:
>  		man->func = &ttm_bo_manager_func;
> -		man->gpu_offset = rdev->mc.gtt_start;
>  		man->available_caching = TTM_PL_MASK_CACHING;
>  		man->default_caching = TTM_PL_FLAG_CACHED;
>  		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA;
> @@ -104,7 +103,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>  	case TTM_PL_VRAM:
>  		/* "On-card" video ram */
>  		man->func = &ttm_bo_manager_func;
> -		man->gpu_offset = rdev->mc.vram_start;
>  		man->flags = TTM_MEMTYPE_FLAG_FIXED |
>  			     TTM_MEMTYPE_FLAG_MAPPABLE;
>  		man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
>
Nirmoy Feb. 21, 2020, 1:27 p.m. UTC | #2 
On 2/21/20 2:19 AM, Luben Tuikov wrote:
> On 2020-02-19 08:53, Nirmoy Das wrote:
>> Calculate GPU offset in radeon_bo_gpu_offset without depending on
>> bo->offset
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> Reviewed-and-tested-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon.h        |  1 +
>>   drivers/gpu/drm/radeon/radeon_object.h | 16 +++++++++++++++-
>>   drivers/gpu/drm/radeon/radeon_ttm.c    |  4 +---
>>   3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
>> index 30e32adc1fc6..b7c3fb2bfb54 100644
>> --- a/drivers/gpu/drm/radeon/radeon.h
>> +++ b/drivers/gpu/drm/radeon/radeon.h
>> @@ -2828,6 +2828,7 @@ extern void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size
>>   extern void radeon_program_register_sequence(struct radeon_device *rdev,
>>   					     const u32 *registers,
>>   					     const u32 array_size);
>> +struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev);
>>   
>>   /*
>>    * vm
>> diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h
>> index d23f2ed4126e..4d37571c7ff5 100644
>> --- a/drivers/gpu/drm/radeon/radeon_object.h
>> +++ b/drivers/gpu/drm/radeon/radeon_object.h
>> @@ -90,7 +90,21 @@ static inline void radeon_bo_unreserve(struct radeon_bo *bo)
>>    */
>>   static inline u64 radeon_bo_gpu_offset(struct radeon_bo *bo)
>>   {
>> -	return bo->tbo.offset;
>> +	struct radeon_device *rdev;
>> +	u64 start = 0;
>> +
>> +	rdev = radeon_get_rdev(bo->tbo.bdev);
>> +
>> +	switch(bo->tbo.mem.mem_type) {
> LKCS wants a space after a keyword, "switch (" .
Thanks Luben, will fix that for both radeon and amdgpu.
>
>> +	case TTM_PL_TT:
>> +		start = rdev->mc.gtt_start;
>> +		break;
>> +	case TTM_PL_VRAM:
>> +		start = rdev->mc.vram_start;
>> +		break;
>> +	}
> Could this lookup have been parameterized by "mem_type"
> to be looked up by an index (possibly "mem_type") to result
> in something new like (pseudo-code):
>       start = rdev->mc.mem_start_table[bo->tbo.mem.mem_type];
> Where "mem_start_table" is a new table holding memory starts
> of particular memories.
> Then you don't need the switch-case.

That would look good but I am afraid, amdgpu_ttm_domain_start() and 
radeon_bo_gpu_offset() will be the only consumer of /mem_start_table./

I think it is not worth to do a intrusive change for this patch series.

Regards,

Nirmoy
diff mbox series

Patch

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 30e32adc1fc6..b7c3fb2bfb54 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -2828,6 +2828,7 @@  extern void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size
 extern void radeon_program_register_sequence(struct radeon_device *rdev,
 					     const u32 *registers,
 					     const u32 array_size);
+struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev);
 
 /*
  * vm
diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h
index d23f2ed4126e..4d37571c7ff5 100644
--- a/drivers/gpu/drm/radeon/radeon_object.h
+++ b/drivers/gpu/drm/radeon/radeon_object.h
@@ -90,7 +90,21 @@  static inline void radeon_bo_unreserve(struct radeon_bo *bo)
  */
 static inline u64 radeon_bo_gpu_offset(struct radeon_bo *bo)
 {
-	return bo->tbo.offset;
+	struct radeon_device *rdev;
+	u64 start = 0;
+
+	rdev = radeon_get_rdev(bo->tbo.bdev);
+
+	switch(bo->tbo.mem.mem_type) {
+	case TTM_PL_TT:
+		start = rdev->mc.gtt_start;
+		break;
+	case TTM_PL_VRAM:
+		start = rdev->mc.vram_start;
+		break;
+	}
+
+	return (bo->tbo.mem.start << PAGE_SHIFT) + start;
 }
 
 static inline unsigned long radeon_bo_size(struct radeon_bo *bo)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index badf1b6d1549..1c8303468e8f 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -56,7 +56,7 @@ 
 static int radeon_ttm_debugfs_init(struct radeon_device *rdev);
 static void radeon_ttm_debugfs_fini(struct radeon_device *rdev);
 
-static struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
+struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
 {
 	struct radeon_mman *mman;
 	struct radeon_device *rdev;
@@ -82,7 +82,6 @@  static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 		break;
 	case TTM_PL_TT:
 		man->func = &ttm_bo_manager_func;
-		man->gpu_offset = rdev->mc.gtt_start;
 		man->available_caching = TTM_PL_MASK_CACHING;
 		man->default_caching = TTM_PL_FLAG_CACHED;
 		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA;
@@ -104,7 +103,6 @@  static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 	case TTM_PL_VRAM:
 		/* "On-card" video ram */
 		man->func = &ttm_bo_manager_func;
-		man->gpu_offset = rdev->mc.vram_start;
 		man->flags = TTM_MEMTYPE_FLAG_FIXED |
 			     TTM_MEMTYPE_FLAG_MAPPABLE;
 		man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;