Re: [PATCH] drm/xe/tests: Add legacy IP descriptors to param generators

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 02 Jul 2025, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> On 02.07.2025 10:35, Jani Nikula wrote:
>> On Tue, 01 Jul 2025, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> On Fri, Jun 27, 2025 at 08:11:10PM +0200, Michal Wajdeczko wrote:
>>>> Recently introduced kunit parameter generators where based on
>>>> the existing arrays which have only GDMID-bsaed IP and didn't
>>>> take into account legacy IP definitions from pre-GMDID era.
>>>>
>>>> Add test only arrays with legacy IPs (as those will not change)
>>>> and extend param generators to start iterating over them.
>>>>
>>>> [ ] =================== xe_pci (2 subtests) ====================
>>>> [ ] ==================== check_graphics_ip  ====================
>>>> [ ] [PASSED] 12.00 Xe_LP
>>>> [ ] [PASSED] 12.10 Xe_LP+
>>>> [ ] [PASSED] 12.55 Xe_HPG
>>>> [ ] [PASSED] 12.60 Xe_HPC
>>>> [ ] [PASSED] 12.70 Xe_LPG
>>>> [ ] [PASSED] 12.71 Xe_LPG
>>>> [ ] [PASSED] 12.74 Xe_LPG+
>>>> [ ] [PASSED] 20.01 Xe2_HPG
>>>> [ ] [PASSED] 20.02 Xe2_HPG
>>>> [ ] [PASSED] 20.04 Xe2_LPG
>>>> [ ] [PASSED] 30.00 Xe3_LPG
>>>> [ ] [PASSED] 30.01 Xe3_LPG
>>>> [ ] [PASSED] 30.03 Xe3_LPG
>>>> [ ] ================ [PASSED] check_graphics_ip ================
>>>> [ ] ===================== check_media_ip  ======================
>>>> [ ] [PASSED] 12.00 Xe_M
>>>> [ ] [PASSED] 12.55 Xe_HPM
>>>> [ ] [PASSED] 13.00 Xe_LPM+
>>>> [ ] [PASSED] 13.01 Xe2_HPM
>>>> [ ] [PASSED] 20.00 Xe2_LPM
>>>> [ ] [PASSED] 30.00 Xe3_LPM
>>>> [ ] [PASSED] 30.02 Xe3_LPM
>>>> [ ] ================= [PASSED] check_media_ip ==================
>>>> [ ] ===================== [PASSED] xe_pci ======================
>>>>
>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/tests/xe_pci.c | 33 +++++++++++++++++++++++++++++++
>>>> 1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/tests/xe_pci.c b/drivers/gpu/drm/xe/tests/xe_pci.c
>>>> index baccb657bd05..c5acee52b458 100644
>>>> --- a/drivers/gpu/drm/xe/tests/xe_pci.c
>>>> +++ b/drivers/gpu/drm/xe/tests/xe_pci.c
>>>> @@ -12,12 +12,31 @@
>>>> #include <kunit/test-bug.h>
>>>> #include <kunit/visibility.h>
>>>>
>
> /**
>  * IS_ARRAY_ROW - checks if @row pointer points to entry in the @array
>  * @arr: the array
>  * @row: the pointer to the row in the array
>  */
>
>>>> +#define IS_ARRAY_ROW(arr, row) ((typeof(&(arr)[0]))(row) - (arr) < ARRAY_SIZE(arr))
>
> ...
>
>>>> const void *xe_pci_graphics_ip_gen_param(const void *prev, char *desc)
>>>> {
>>>> +	const void *next = legacy_graphics_ip_gen_params(prev, desc);
>>>> +
>>>> +	if (next)
>>>> +		return next;
>>>> +	if (IS_ARRAY_ROW(legacy_graphics_ips, prev))
>> 
>> I don't understand what "is array row" means.
>
> do you want me to include some kind of the kernel-doc (see above) for
> this helper macro used only here as a trick to combine two gen_params?
>
> or maybe do you prefer 'entry' instead of 'row' name?

Lucas already reviewed it, so who am I to argue...

...but I think it's a bit clumsy check if 'row' points anywhere in the
memory occupied by 'arr', not just at an element. And it completely
casts away the type of row, you could pass it, say, 5. Something like
this feels safer:

	const typeof(&(arr)[0]) __p = row;

But up to Lucas, really.


BR,
Jani.

-- 
Jani Nikula, Intel