Re: [PATCH 1/6] drm/xe: Add helper to format SR-IOV function name

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

On 15.04.2024 09:56, Piotr Piórkowski wrote:
> Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com> wrote on pon [2024-kwi-15 09:17:27 +0530]:
>>
>> On 15-04-2024 00:31, Michal Wajdeczko wrote:
>>> While the GuC firmware and the Xe driver are using VF identifier
>>> VFID(0) to represent the Physical Function, we should avoid using
>>> "VF0" name and use proper "PF" name in all user facing messages
>>> related to the Physical Function and use "VFn" name only when
>>> referrinf to the true Virtual Function. Add simple helper to get
>>> properly formatted function name based on the function number.
>>>
>>> Signed-off-by: Michal Wajdeczko<michal.wajdeczko@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_sriov.c | 17 +++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_sriov.h |  1 +
>>>   2 files changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_sriov.c b/drivers/gpu/drm/xe/xe_sriov.c
>>> index 1b40f5de9ef5..1c3fa84b6adb 100644
>>> --- a/drivers/gpu/drm/xe/xe_sriov.c
>>> +++ b/drivers/gpu/drm/xe/xe_sriov.c
>>> @@ -122,3 +122,20 @@ void xe_sriov_print_info(struct xe_device *xe, struct drm_printer *p)
>>>   	drm_printf(p, "enabled: %s\n", str_yes_no(IS_SRIOV(xe)));
>>>   	drm_printf(p, "mode: %s\n", xe_sriov_mode_to_string(xe_device_sriov_mode(xe)));
>>>   }
>>> +
>>> +/**
>>> + * xe_sriov_function_name() - Get SR-IOV Function name.
>>> + * @n: the Function number (identifier) to get name of
>>> + * @buf: the buffer to format to
>>> + * @size: size of the buffer (shall be at least 5 bytes)
>>> + *
>>> + * Return: formatted function name ("PF" or "VF%u").
>>> + */
>>> +const char *xe_sriov_function_name(unsigned int n, char *buf, size_t size)
>>
>> nit: It is not necessary to return a |buf|, so |how about void instead of
>> const char * ?|

true, it could be done as void, but that looks as a waste of one of the
potential use which is mentioned below

besides, this is our Xe driver helper only, it's main goal is usability

otherwise it should be written as returning "int" to "return number of
characters of output that would have been written which may be greater
than len, if output was truncated)" like in string_get_size()

but that would be overkill IMO as it would be highly unlikely that we
ever check that ret value

>>
>> |With that LGTM.|
>>
>> |Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> |
> 
> I'm guessing that this function returns a pointer to be able to use this directly
> in printf kind of functions. I am ok with this:

yes, even in patch 6/6 I'm using that this way in pf_push_vf_cfg_klvs()

> 
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

thanks!

>>
>>> +{
>>> +	if (n)
>>> +		snprintf(buf, size, "VF%u", n);
>>> +	else
>>> +		strscpy(buf, "PF", size);
>>> +	return buf;
>>> +}
>>> diff --git a/drivers/gpu/drm/xe/xe_sriov.h b/drivers/gpu/drm/xe/xe_sriov.h
>>> index f9dec84d77e3..486bb21c3256 100644
>>> --- a/drivers/gpu/drm/xe/xe_sriov.h
>>> +++ b/drivers/gpu/drm/xe/xe_sriov.h
>>> @@ -13,6 +13,7 @@
>>>   struct drm_printer;
>>>   const char *xe_sriov_mode_to_string(enum xe_sriov_mode mode);
>>> +const char *xe_sriov_function_name(unsigned int n, char *buf, size_t len);
>>>   void xe_sriov_probe_early(struct xe_device *xe);
>>>   void xe_sriov_print_info(struct xe_device *xe, struct drm_printer *p);
>