<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - Internal Distribution Only] <div> <div style="font-family: Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);"> 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.)</div> <div style="font-family: Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);"> </div> <div style="font-family: Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);"> Kenny</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Tuikov, Luben <Luben.Tuikov@amd.com> Sent: Thursday, March 5, 2020 2:19 PM To: Nirmoy Das <nirmoy.aiemd@gmail.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; thellstrom@vmware.com <thellstrom@vmware.com>; airlied@linux.ie <airlied@linux.ie>; Ho, Kenny <Kenny.Ho@amd.com>; brian.welty@intel.com <brian.welty@intel.com>; maarten.lankhorst@linux.intel.com <maarten.lankhorst@linux.intel.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Das, Nirmoy <Nirmoy.Das@amd.com>; linux-graphics-maintainer@vmware.com <linux-graphics-maintainer@vmware.com>; bskeggs@redhat.com <bskeggs@redhat.com>; daniel@ffwll.ch <daniel@ffwll.ch>; Deucher, Alexander <Alexander.Deucher@amd.com>; sean@poorly.run <sean@poorly.run>; Koenig, Christian <Christian.Koenig@amd.com>; kraxel@redhat.com <kraxel@redhat.com> Subject: Re: [PATCH 2/8] drm/radeon: don't use ttm bo->offset <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">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@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..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@lists.freedesktop.org > <a href="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"> 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</a> > </div> </div> </div> </body> </html>