Re: ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7)

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Aug 22, 2019 at 06:21:23PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Engine relative MMIO (rev7)
> URL   : https://patchwork.freedesktop.org/series/57117/
> State : warning
> 
> == Summary ==
> 
> $ dim checkpatch origin/drm-tip
> 2eab059bc87e drm/i915: Engine relative MMIO
> -:108: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
> #108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
> +#define MI_LOAD_REGISTER_IMM(engine, count)	\
> +					(engine)->get_lri_cmd((engine), (count))
> 
> -:108: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'engine' - possible side-effects?
> #108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
> +#define MI_LOAD_REGISTER_IMM(engine, count)	\
> +					(engine)->get_lri_cmd((engine), (count))
> 
> -:203: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
> +#define __MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>                                   	                ^
> 
> -:203: CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
> +#define __MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>                                   	                    ^
> 
> -:205: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> #205: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
> +#define   MI_LRI_ADD_CS_MMIO_START_GEN11	(1<<19)
>                                          	  ^
> 
> -:491: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
> #491: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:1719:
> +	*cs++ = MI_LOAD_REGISTER_IMM(rq->engine, GEN7_L3LOG_SIZE/4);
>  	                                                        ^
> 
> -:522: CHECK:CAMELCASE: Avoid CamelCase: <regLRI>
> #522: FILE: drivers/gpu/drm/i915/gt/selftest_workarounds.c:483:
> +		u32 regLRI = i915_get_lri_reg(engine,
> 
> -:618: ERROR:SPACING: space prohibited after that open parenthesis '('
> #618: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:225:
> +	CMD(  __MI_LOAD_REGISTER_IMM(1),        SMI,   !F,  0xFF,   W,
> 
> -:619: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
> #619: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:226:
> +	CMD(  __MI_LOAD_REGISTER_IMM(1),        SMI,   !F,  0xFF,   W,
>  	      .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 }    ),
> 
> -:629: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
> #629: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:1187:
> +				if (desc->cmd.value == __MI_LOAD_REGISTER_IMM(1)
> +				    && (offset + 2 > length ||
> 
> total: 2 errors, 0 warnings, 8 checks, 531 lines checked
> 901081d701fe drm/i915: Engine relative MMIO for Gen12
> -:182: CHECK:BRACES: braces {} should be used on all arms of this statement
> #182: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:407:
> +	if (i915_engine_has_relative_lri(engine)) {
> [...]
> +	} else
> [...]
> 
> -:183: CHECK:BRACES: braces {} should be used on all arms of this statement
> #183: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:408:
> +		if (INTEL_GEN(engine->i915) < 12)
> [...]
> +		else {
> [...]
> 
> -:185: CHECK:BRACES: Unbalanced braces around else statement
> #185: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:410:
> +		else {
> 
> -:191: CHECK:BRACES: Unbalanced braces around else statement
> #191: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:416:
> +	} else
> 
> -:271: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> #271: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
> +#define   MI_LRI_MMIO_REMAP_GEN12		(1<<17)
>                                   		  ^
> 
> total: 0 errors, 0 warnings, 5 checks, 252 lines checked

I looks like we need to fix many (most) of the issues here before proceed.

Also, did you check the naming with Tvrtko. If I remember correctly
his preference was for "MI_LOAD_REGISTER_IMM_REL for 
usage on relevant (new) paths."

and don't touch the old ones.

Also I'm not sure if I like _MI_LOAD for the old and MI_LOAD for the new.
Tvrtko original's suggestion makes the distinction clean.

> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx