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

[Luben Tuikov <luben.tuikov@xxxxxxx>]

On 2020-03-05 08:29, Nirmoy Das wrote:
> Calculate GPU offset in radeon_bo_gpu_offset without depending on
> bo->offset.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> Reviewed-and-tested-by: Christian König <christian.koenig@xxxxxxx>
> ---
>  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..60275b822f79 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;
>  }

You're removing a "return bo->tbo.offset" and adding a
switch-case statement. So, then, now instead of an instant
lookup, you're adding branching. You're adding comparison
and branching. Do you think that's better? Faster? Smaller?

I've written before about this for this patch: Why not create a map,
whose index is "mem_type" which references the desired
address? No comparison, no branching. Just an index-dereference
and a value:

return rdev->mc.mem_start_map[bo->tbo.mem.mem_type];

Obviously, you'll have to create "mem_start_map".

That's a NAK from me on this patch using comparison
and branching to return static data lookup value.

Regards,
Luben

> 
>  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;
> --
> 2.25.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7Cca6004a5ac7a400a030708d7c108bcde%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637190115619487827&amp;sdata=EkSy4vpUIbTE%2B75CSO37JWiULKbRTYbcZUSEtRpcrTk%3D&amp;reserved=0
> 

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