[PATCH] drm/xe/rtp: Drop sentinels from arg to xe_rtp_process_to_sr()

[Lucas De Marchi]

On Fri, Mar 07, 2025 at 05:00:16PM +0000, Tvrtko Ursulin wrote:
>
>On 07/03/2025 15:57, Lucas De Marchi wrote:
>>On Fri, Mar 07, 2025 at 12:00:14PM +0000, Tvrtko Ursulin wrote:
>>>
>>>On 07/03/2025 04:00, Lucas De Marchi wrote:
>>>>There's a mismatch on API: while xe_rtp_process_to_sr() processes
>>>>entries until an entry without name, the active tracking with
>>>>xe_rtp_process_ctx_enable_active_tracking() needs to use the number of
>>>>elements. The number of elements is taken everywhere using ARRAY_SIZE(),
>>>>but that will have one entry too many. This leads to the following
>>>>warning, as reported by lkp:
>>>>
>>>>   drivers/gpu/drm/xe/xe_tuning.c: In function 'xe_tuning_dump':
>>>>>>include/drm/drm_print.h:228:31: warning: '%s' directive 
>>>>>>argument is null [-Wformat-overflow=]
>>>>     228 |         drm_printf((printer), "%.*s" fmt, (indent), 
>>>>"\t\t\t\t\tX", ##__VA_ARGS__)
>>>>         |                               ^~~~~~
>>>>   drivers/gpu/drm/xe/xe_tuning.c:226:17: note: in expansion of 
>>>>macro 'drm_printf_indent'
>>>>     226 |                 drm_printf_indent(p, 1, "%s\n", 
>>>>engine_tunings[idx].name);
>>>>         |                 ^~~~~~~~~~~~~~~~~
>>>>
>>>>That's because it will still process the last entry when tracking the
>>>>active tunings. The same issue exists in the WAs. Change
>>>>xe_rtp_process_to_sr() to also take the number of elements so the empty
>>>>entry can be removed and the warning should go away. Fixing on the
>>>>active-tracking side would more fragile as the it would need a `- 1`
>>>>everywhere and continue to use a different approach for number of
>>>>elements.
>>>
>>>Patch looks fine, and dropping the terminators is good. But I 
>>>can't say I understand the warning. How it will process the last 
>>>entry if entry->name is NULL? Hence bitmap will not have the bit 
>>>set, hence the dump cannot iterate over it. I must be missing 
>>>something, I wanted to say obvious, but I hope it really isn't. 
>>>:))
>>
>>yeah... I could reproduce it only with the compiler provided by 0day,
>>for microblaze arch. Even the same gcc version for x86 didn't reproduce
>>it.
>>
>>>
>>>Regards,
>>>
>>>Tvrtko
>>>
>>>>Aside from the warning, it's a non-issue as there would always be enough
>>>>bits allocated and the last entry would never be active since
>>>>xe_rtp_process_to_sr() stops on the sentinel.
>>
>>^ this paragraph tries to explain why it's harmless, which made me not
>>even add a Fixes trailer. Indeed xe_rtp_process_to_sr() will never
>>process it, hence the bit will never be set. The I can see is that later
>>when we are dumping, we are using for_each_bit_set(), passing as max the
>>bit related to the empty entry. So maybe there's a diff in bitops for
>>that arch that makes the compiler **think** the last element could be
>>accessed. The alternative fix I started was to pass `ARRAY_SIZE(...) - 1`
>>in the for_each_bit_set(). But that looked ugly.
>>
>>Note that it's a build time warning - if that really happens in runtime
>>there's probably something terribly broken with that arch.
>
>Hah.. go figure.
>
>Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>

thanks, applied to drm-xe-next,

[1/1] drm/xe/rtp: Drop sentinels from arg to xe_rtp_process_to_sr()    
       commit: 8aa8c2d4214e1771c32101d70740002662d31bb7                 


Lucas De Marchi