[Intel-gfx] [PATCH] drm/i915: prefer INTEL_GEN(dev_priv) to INTEL_INFO(dev)->gen

[Dave Gordon]

On 13/06/16 10:28, Tvrtko Ursulin wrote:
>
> On 10/06/16 18:35, Dave Gordon wrote:
>> More Coccinellery ...
>>
>> Wherever we find "INTEL_INFO(dev)->gen", and have a suitable
>> "dev_priv" in scope, replace it with "INTEL_GEN(dev_priv)".
>> At this time, we've found 189 instances, and each replacement
>> saves one memory cycle at runtime and two bytes of codespace
>> (~360 bytes total text size reduction).
>>
>> @dev_priv_param@
>> function FUNC;
>> idexpression struct drm_device *DEV;
>> identifier DEV_PRIV;
>> @@
>> FUNC(..., struct drm_i915_private *DEV_PRIV, ...)
>> {
>>      <...
>> -   INTEL_INFO(DEV)->gen
>> +   INTEL_GEN(DEV_PRIV)
>>      ...>
>> }
>>
>> @dev_priv_local@
>> idexpression struct drm_device *DEV;
>> identifier DEV_PRIV;
>> expression E;
>> @@
>> {
>>      ...
>> (
>>      struct drm_i915_private *DEV_PRIV;
>> |
>>      struct drm_i915_private *DEV_PRIV = E;
>> )
>>      <...
>> -   INTEL_INFO(DEV)->gen
>> +   INTEL_GEN(DEV_PRIV)
>>      ...>
>> }
>>
>> Plus manual deletion of one now-unused local "dev".
>>
>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c        |  52 +++++++-------
>>   drivers/gpu/drm/i915/i915_dma.c            |  16 ++---
>>   drivers/gpu/drm/i915/i915_drv.c            |   2 +-
>>   drivers/gpu/drm/i915/i915_gem.c            |   4 +-
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   8 +--
>>   drivers/gpu/drm/i915/i915_gem_fence.c      |   8 +--
>>   drivers/gpu/drm/i915/i915_gem_gtt.c        |  12 ++--
>>   drivers/gpu/drm/i915/i915_gem_stolen.c     |   6 +-
>>   drivers/gpu/drm/i915/i915_gpu_error.c      |  14 ++--
>>   drivers/gpu/drm/i915/i915_irq.c            |  12 ++--
>>   drivers/gpu/drm/i915/i915_suspend.c        |  20 +++---
>>   drivers/gpu/drm/i915/intel_color.c         |   2 +-
>>   drivers/gpu/drm/i915/intel_crt.c           |   6 +-
>>   drivers/gpu/drm/i915/intel_ddi.c           |   4 +-
>>   drivers/gpu/drm/i915/intel_display.c       | 111 ++++++++++++++---------------
>>   drivers/gpu/drm/i915/intel_dp.c            |  20 +++---
>>   drivers/gpu/drm/i915/intel_dpll_mgr.c      |   2 +-
>>   drivers/gpu/drm/i915/intel_lvds.c          |   4 +-
>>   drivers/gpu/drm/i915/intel_pm.c            |  18 ++---
>>   drivers/gpu/drm/i915/intel_psr.c           |   4 +-
>>   drivers/gpu/drm/i915/intel_sdvo.c          |   8 +--
>>   drivers/gpu/drm/i915/intel_tv.c            |   2 +-
>>   22 files changed, 167 insertions(+), 168 deletions(-)

[snip]

>> @@ -553,7 +553,7 @@ void i915_gem_restore_fences(struct drm_device *dev)
>>       uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
>>       uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
>>
>> -    if (INTEL_INFO(dev)->gen >= 8 || IS_VALLEYVIEW(dev)) {
>> +    if (INTEL_GEN(dev_priv) >= 8 || IS_VALLEYVIEW(dev)) {
>
> While we are churning :), could looks for both dev_priv and dev in the
> same condition like here and tidy those.

[snip]

> A few instances of the small comment I made above, which I think would
> be a good opportunity. Otherwise all looks OK.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> But needs more acks, since last time we discussed this conclusion was it
> was too much disruption for the benefit.
>
> Regards,
> Tvrtko

Yes, that's why this was a minimal set for replacement of one specific 
but very common idiom that has a definite benefit. At the other extreme, 
I have a Cocci patch to replace *all* INTEL_INFO()->gen with INTEL_GEN() 
and then replace the 'dev' parameter to INTEL_INFO(), INTEL_GEN() and 
all the IS_X() macros wherever possible; but that really is a lot of 
churn for proportionately less benefit. Or we could pick replacements on 
a file-by-file basis rather than according to which macro they use. Or 
maybe one patch each for the big targets (debugfs.c, display.c) and 
another one that does all the files where there are only a few lines of 
changes.

Opinions, anybody?

.Dave.