Re: [Intel-xe] [PATCH] drm/xe: make compound literal initialization const

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

On Thu, 09 Mar 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Thu, Mar 09, 2023 at 02:17:46PM +0200, Jani Nikula wrote:
>> Be careful about having const in the compound literal initialization to
>> keep the initializers in rodata. Here, the impact is 1.8k of mutable
>> data moved to rodata.
>
> is there any static tool/script that we could run along with checkpatch
> to prevent these?

Glad you asked. Over the years I had accumulated a number of quick bash
scripts for checking various style issues in i915. I now have them all
gathered in a single python script at [1]. It's not the greatest, and I
kind of wish checkpatch was more accessible (i.e. not perl) and allowed
local plugins, but this is what I have.

It's really just a collection of simple git greps. And probably too many
false positives to add to CI. And you'd have to hack it to make it work
on patches, not the source tree.

Anyway, for global data it uses objdump. You need to build the driver
first, and then run 'i915-check data'. Or, for xe, 'i915-check
--path=drivers/gpu/drm/xe data'.

Try 'i915-check help' or 'i915-check --explain <check>' where <check> is
one of the listed checks.


BR,
Jani.


PS. Regarding the recent discussion whether module parameters should be
individual variables or gathered in a struct, the objdump is IMO in
favour of a struct. Contrast the scattered results in xe:

drivers/gpu/drm/xe/xe_module.o: .data	0000000000000008 xe_param_force_probe
drivers/gpu/drm/xe/xe_module.o: .data	0000000000000004 xe_guc_log_level
drivers/gpu/drm/xe/xe_module.o: .bss	0000000000000004 xe_force_vram_bar_size
drivers/gpu/drm/xe/xe_module.o: .data	0000000000000001 enable_display
drivers/gpu/drm/xe/xe_module.o: .data	0000000000000001 enable_guc

with i915:

drivers/gpu/drm/i915/i915_params.o: .data..read_mostly	00000000000000a0 i915_modparams



[1] https://gitlab.freedesktop.org/jani/i915-check


>
>> 
>> add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-1804 (-1804)
>> Data                                         old     new   delta
>> __compound_literal                          1804       -   -1804
>> Total: Before=42425, After=40621, chg -4.25%
>> add/remove: 0/0 grow/shrink: 1/0 up/down: 1804/0 (1804)
>> RO Data                                      old     new   delta
>> __compound_literal                          7696    9500   +1804
>> Total: Before=138535, After=140339, chg +1.30
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>
>> ---
>>  drivers/gpu/drm/xe/xe_rtp.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_rtp.h b/drivers/gpu/drm/xe/xe_rtp.h
>> index bd44fd8bbe05..8fc393ef7358 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp.h
>> +++ b/drivers/gpu/drm/xe/xe_rtp.h
>> @@ -363,7 +363,7 @@ struct xe_reg_sr;
>>   */
>>  #define XE_RTP_RULES(r1, ...)							\
>>  	.n_rules = COUNT_ARGS(r1, ##__VA_ARGS__),				\
>> -	.rules = (struct xe_rtp_rule[]) {					\
>> +	.rules = (const struct xe_rtp_rule[]) {					\
>>  		CALL_FOR_EACH(__ADD_XE_RTP_RULE_PREFIX, r1, ##__VA_ARGS__)	\
>>  	}
>>  
>> @@ -390,7 +390,7 @@ struct xe_reg_sr;
>>   */
>>  #define XE_RTP_ACTIONS(a1, ...)							\
>>  	.n_actions = COUNT_ARGS(a1, ##__VA_ARGS__),				\
>> -	.actions = (struct xe_rtp_action[]) {					\
>> +	.actions = (const struct xe_rtp_action[]) {				\
>>  		CALL_FOR_EACH(__ADD_XE_RTP_ACTION_PREFIX, a1, ##__VA_ARGS__)	\
>>  	}
>>  
>> -- 
>> 2.39.1
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center