Re: [PATCH 4/5] drm/xe: Rename internal vram helper function

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

++ maintainers

On 29.05.2024 00:15, Matthew Brost wrote:
> On Tue, May 28, 2024 at 02:35:15PM -0700, Matt Roper wrote:
>> On Mon, May 27, 2024 at 07:35:53PM +0200, Michal Wajdeczko wrote:
>>> Drop no longer applicable "xe_mmio_" prefix and downgrade the
>>> existing kernel-doc for internal function to normal comment.
>>
>> As noted on the previous patch, there are several other functions with
>> "xe_" prefixes even though they're static (xe_resize_vram_bar,
>> xe_pci_resource_valid, xe_determine_lmem_bar_size, and the
>> xe_mmio_tile_vram_size you're updating here).  Maybe we should drop the
>> "xe_" prefix from all of them (and "xe_mmio" from this one) before the
>> movement of VRAM code to a new file?

agree on doing all renames for consistency, but this time I just didn't
want to make too many cleanups in one shot, my focus was to do clear
code separation first

>>
> 
> We have talked about this before and I think the consensous was "xe_"
> prefixes for static functions are fine, perhaps even desired. I can't

IMO use of "xe_foo" prefix for static functions is inconsistent (as its
name suggests that it is public function while OTOH it is declared as
static) and may mislead that it could be used in other compilation unit.

thus use of "xe_" (for static) should be rather discouraged but also at
the same time it shouldn't be treated as showstopper (with the hope
better name will be provided later)

> find a reference but I do recall a discussion on the list about this.
> 
> I think the maintainers should make / document a rule wrt to "xe_"
> prefixes before we start making changes in this area as it is not clear.

before we start writing rules for static functions, better to focus only
on rules for public naming convention for the components, like:

files:
	GOOD
		xe_foo.ch
		xe_foo_types.h
		xe_foo_helpers.h
	FINE
		xe_gt_foo.ch
	BAD
		foo.ch

types:
	GOOD
		struct xe_foo_xxx
		enum xe_foo_yyy
		typedef xe_foo_zzz
	BAD
		struct foo
		struct xe_bar

functions:
	GOOD
		xe_foo_bar(struct xe_foo *foo, ...)
	FINE
		xe_foo_bar(struct xe_device *xe, ...)
		xe_gt_foo_bar(struct xe_gt *gt, ...)
	BAD
		xe_foo_bar(..., struct xe_foo *foo)
		xe_bar_foo(struct xe_foo *foo, ...)

> 
> Matt
> 
>>
>> Matt
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_vram.c | 12 ++++++------
>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
>>> index d8b81e4e050c..411e8d23fd4d 100644
>>> --- a/drivers/gpu/drm/xe/xe_vram.c
>>> +++ b/drivers/gpu/drm/xe/xe_vram.c
>>> @@ -192,8 +192,8 @@ static inline u64 get_flat_ccs_offset(struct xe_gt *gt, u64 tile_size)
>>>  	return offset;
>>>  }
>>>  
>>> -/**
>>> - * xe_mmio_tile_vram_size() - Collect vram size and offset information
>>> +/*
>>> + * tile_vram_size() - Collect vram size and offset information
>>>   * @tile: tile to get info for
>>>   * @vram_size: available vram (size - device reserved portions)
>>>   * @tile_size: actual vram size
>>> @@ -211,8 +211,8 @@ static inline u64 get_flat_ccs_offset(struct xe_gt *gt, u64 tile_size)
>>>   * NOTE: multi-tile bases will include the tile offset.
>>>   *
>>>   */
>>> -static int xe_mmio_tile_vram_size(struct xe_tile *tile, u64 *vram_size,
>>> -				  u64 *tile_size, u64 *tile_offset)
>>> +static int tile_vram_size(struct xe_tile *tile, u64 *vram_size,
>>> +			  u64 *tile_size, u64 *tile_offset)
>>>  {
>>>  	struct xe_device *xe = tile_to_xe(tile);
>>>  	struct xe_gt *gt = tile->primary_gt;
>>> @@ -287,7 +287,7 @@ int xe_vram_probe(struct xe_device *xe)
>>>  
>>>  	/* Get the size of the root tile's vram for later accessibility comparison */
>>>  	tile = xe_device_get_root_tile(xe);
>>> -	err = xe_mmio_tile_vram_size(tile, &vram_size, &tile_size, &tile_offset);
>>> +	err = tile_vram_size(tile, &vram_size, &tile_size, &tile_offset);
>>>  	if (err)
>>>  		return err;
>>>  
>>> @@ -302,7 +302,7 @@ int xe_vram_probe(struct xe_device *xe)
>>>  
>>>  	/* tile specific ranges */
>>>  	for_each_tile(tile, xe, id) {
>>> -		err = xe_mmio_tile_vram_size(tile, &vram_size, &tile_size, &tile_offset);
>>> +		err = tile_vram_size(tile, &vram_size, &tile_size, &tile_offset);
>>>  		if (err)
>>>  			return err;
>>>  
>>> -- 
>>> 2.43.0
>>>
>>>
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation