[PATCH 2/8] drm/radeon: don't use ttm bo->offset
Ho, Kenny
Kenny.Ho at amd.com
Thu Mar 5 19:37:52 UTC 2020
[AMD Official Use Only - Internal Distribution Only]
I believe bo->tbo.mem.mem_type is of uint32_t type and not an enum, is the index lookup method safe? (i.e., how do you deal with the possibility of having value TTM_PL_PRIV or above or are you suggesting those are not possible for this function.)
Kenny
________________________________
From: Tuikov, Luben <Luben.Tuikov at amd.com>
Sent: Thursday, March 5, 2020 2:19 PM
To: Nirmoy Das <nirmoy.aiemd at gmail.com>; dri-devel at lists.freedesktop.org <dri-devel at lists.freedesktop.org>
Cc: Zhou, David(ChunMing) <David1.Zhou at amd.com>; thellstrom at vmware.com <thellstrom at vmware.com>; airlied at linux.ie <airlied at linux.ie>; Ho, Kenny <Kenny.Ho at amd.com>; brian.welty at intel.com <brian.welty at intel.com>; maarten.lankhorst at linux.intel.com <maarten.lankhorst at linux.intel.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>; Das, Nirmoy <Nirmoy.Das at amd.com>; linux-graphics-maintainer at vmware.com <linux-graphics-maintainer at vmware.com>; bskeggs at redhat.com <bskeggs at redhat.com>; daniel at ffwll.ch <daniel at ffwll.ch>; Deucher, Alexander <Alexander.Deucher at amd.com>; sean at poorly.run <sean at poorly.run>; Koenig, Christian <Christian.Koenig at amd.com>; kraxel at redhat.com <kraxel at redhat.com>
Subject: Re: [PATCH 2/8] drm/radeon: don't use ttm bo->offset
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 at amd.com>
> Reviewed-and-tested-by: Christian König <christian.koenig at 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..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 at lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cca6004a5ac7a400a030708d7c108bcde%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637190115619487827&sdata=EkSy4vpUIbTE%2B75CSO37JWiULKbRTYbcZUSEtRpcrTk%3D&reserved=0
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200305/7b8aad04/attachment-0001.htm>
More information about the amd-gfx
mailing list