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

[Luben Tuikov <luben.tuikov@amd.com>]

On 2020-03-05 14:37, Ho, Kenny wrote:
> [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.)

Hi Kenny,

Good question. I'm not suggesting that those are not possible
with this function. The driver provides this map.
The format of the map is set by the infrastructure (TTM),
it could be as simple as "u64 *mem_start_map; int map_size;" in an embedding
struture or something more fancy.
However the map array and values themselves are set by the driver when it answers
about its capabilities, etc, when it first registers with TTM.

Notice how badly the switch statment below is written. (Inverview question.)
It's missing a "default:" label! (it's why switch-case is powerful).

If you add a "default:" label, you see that the rest of the values
in the map are 0s.

While TTM knows only of TTM_PL_PRIV number of maps (3), the driver
can add more, allocate and initialize the map, and return it back
to TTM, guaranteeing at least TTM_PL_PRIV maps in indices 0, 1, and 2.

I think it is not a good idea to use a compare-branch for a
simple lookup like this. One naturally asks, "why isn't this
a simple map lookup?"

Consider the following thought experiment: we continue to add switch-case
and if-else for all sorts of simple things as we've seen being suggested
in recent patches. In a few years, we'll not be able to understand the code.

Regards,
Luben
P.S. Code obfuscation isn't necessarily a bad thing--it causes a complete
rewrite! :-D

> 
> Kenny
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *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
>  
> 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
>> 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
>> 
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx