Re: [Intel-gfx] [RFC] Prefer INTEL_INFO(dev_priv) to INTEL_INFO(dev)

2016-04-11 Thread Jani Nikula 
On Mon, 11 Apr 2016, Dave Gordon  wrote:
> For this reason, I'd like to recommend that anyone doing this sort of 
> bulk transformation with Cocci or awk or just sed should /always/ 
> include the transformation script to help those with patches in flight.

I think that's generally the expectation anyway for cocci changes.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] Prefer INTEL_INFO(dev_priv) to INTEL_INFO(dev)

2016-04-11 Thread Dave Gordon 

On 08/04/16 07:09, Joonas Lahtinen wrote:

On to, 2016-04-07 at 18:57 +0100, Dave Gordon wrote:

Where we have a suitable dev_priv pointer, we can use that rather than
'dev' for accessing INTEL_INFO().  This removes one level of memory
reference, decreasing code size a little and possibly making everything
a little faster. We could also do this for all the macros that implicitly
use INTEL_INFO e.g. IS_CHERRYVIEW().


If applied to all macros, sounds like the right thing to do. Although,
I would prefer to see s/dev_priv/i915/g and then i915->info.gen, i915-

info.is_cherryview etc. rather than dozens of macros.


Maybe, but not all callsites actually have a 'dev_priv' in scope at 
present, which is why this had to be done with Coccinelle rather than 
just sed.


I tried extending this to all IS_XXX() macros as well (but not as yet 
HAS_XXX()) and got:


 36 files changed, 741 insertions(+), 763 deletions(-)

the line count difference being due to 22 now-unused local variables 
deleted afterwards; the i915.ko binary shrank by about 2k - which 
represents quite a lot of instructions. So this isn't just a matter of 
style, like changing INTEL_INFO(dev)->gen with INTEL_GEN(dev), this 
transformation actually improves code size and presumably speed :)



It's going to cause a lot of rebasing, though. So depending on when we
apply dev_priv -> i915, might be or might not be worth the hassle now.

Regards, Joonas


The point of including the actual Cocci code in the commit message is 
that then anyone who has a set of not-yet-submitted changes can apply 
the Cocci script to their own codebase before attempting to rebase onto 
the new version of nightly, thus eliminating from the conflicts all 
those which are simply due to the patch applied to upstream.


For this reason, I'd like to recommend that anyone doing this sort of 
bulk transformation with Cocci or awk or just sed should /always/ 
include the transformation script to help those with patches in flight.


.Dave.


Coccinelle:

 @dev_priv_param@
   function FUNC;
   idexpression struct drm_device *DEV;
   identifier DEV_PRIV;
 @@
   FUNC(..., struct drm_i915_private *DEV_PRIV, ...)
   {
<...
INTEL_INFO
(
 - DEV
 + 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
 + DEV_PRIV
)
...>
   }

followed by manually deleting 6 now-unused "dev" locals.

Signed-off-by: Dave Gordon 
---
  drivers/gpu/drm/i915/i915_debugfs.c|  66 +++---
  drivers/gpu/drm/i915/i915_dma.c|  30 +++---
  drivers/gpu/drm/i915/i915_drv.c|   4 +-
  drivers/gpu/drm/i915/i915_gem.c|   6 +-
  drivers/gpu/drm/i915/i915_gem_context.c|   2 +-
  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|  14 +--
  drivers/gpu/drm/i915/i915_gem_stolen.c |   6 +-
  drivers/gpu/drm/i915/i915_gpu_error.c  |  32 +++
  drivers/gpu/drm/i915/i915_irq.c|  36 
  drivers/gpu/drm/i915/i915_suspend.c|  20 ++--
  drivers/gpu/drm/i915/intel_color.c |  16 ++--
  drivers/gpu/drm/i915/intel_crt.c   |   6 +-
  drivers/gpu/drm/i915/intel_ddi.c   |   4 +-
  drivers/gpu/drm/i915/intel_display.c   | 141 ++---
  drivers/gpu/drm/i915/intel_dp.c|  20 ++--
  drivers/gpu/drm/i915/intel_dpll_mgr.c  |   2 +-
  drivers/gpu/drm/i915/intel_fbdev.c |   4 +-
  drivers/gpu/drm/i915/intel_lrc.c   |   2 +-
  drivers/gpu/drm/i915/intel_lvds.c  |   4 +-
  drivers/gpu/drm/i915/intel_overlay.c   |   4 +-
  drivers/gpu/drm/i915/intel_pm.c|  50 +-
  drivers/gpu/drm/i915/intel_psr.c   |   6 +-
  drivers/gpu/drm/i915/intel_ringbuffer.c|  40 
  drivers/gpu/drm/i915/intel_sdvo.c  |   8 +-
  drivers/gpu/drm/i915/intel_tv.c|   2 +-
  drivers/gpu/drm/i915/intel_uncore.c|   6 +-
  28 files changed, 270 insertions(+), 277 deletions(-)


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

Re: [Intel-gfx] [RFC] Prefer INTEL_INFO(dev_priv) to INTEL_INFO(dev)

2016-04-08 Thread Joonas Lahtinen 
On to, 2016-04-07 at 18:57 +0100, Dave Gordon wrote:
> Where we have a suitable dev_priv pointer, we can use that rather than
> 'dev' for accessing INTEL_INFO().  This removes one level of memory
> reference, decreasing code size a little and possibly making everything
> a little faster. We could also do this for all the macros that implicitly
> use INTEL_INFO e.g. IS_CHERRYVIEW().

If applied to all macros, sounds like the right thing to do. Although,
I would prefer to see s/dev_priv/i915/g and then i915->info.gen, i915-
>info.is_cherryview etc. rather than dozens of macros.

It's going to cause a lot of rebasing, though. So depending on when we
apply dev_priv -> i915, might be or might not be worth the hassle now.

Regards, Joonas

> 
> Coccinelle:
> 
> @dev_priv_param@
>   function FUNC;
>   idexpression struct drm_device *DEV;
>   identifier DEV_PRIV;
> @@
>   FUNC(..., struct drm_i915_private *DEV_PRIV, ...)
>   {
>   <...
>   INTEL_INFO
>   (
> - DEV
> + 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
> + DEV_PRIV
>   )
>   ...>
>   }
> 
> followed by manually deleting 6 now-unused "dev" locals.
> 
> Signed-off-by: Dave Gordon 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c|  66 +++---
>  drivers/gpu/drm/i915/i915_dma.c|  30 +++---
>  drivers/gpu/drm/i915/i915_drv.c|   4 +-
>  drivers/gpu/drm/i915/i915_gem.c|   6 +-
>  drivers/gpu/drm/i915/i915_gem_context.c|   2 +-
>  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|  14 +--
>  drivers/gpu/drm/i915/i915_gem_stolen.c |   6 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c  |  32 +++
>  drivers/gpu/drm/i915/i915_irq.c|  36 
>  drivers/gpu/drm/i915/i915_suspend.c|  20 ++--
>  drivers/gpu/drm/i915/intel_color.c |  16 ++--
>  drivers/gpu/drm/i915/intel_crt.c   |   6 +-
>  drivers/gpu/drm/i915/intel_ddi.c   |   4 +-
>  drivers/gpu/drm/i915/intel_display.c   | 141 
> ++---
>  drivers/gpu/drm/i915/intel_dp.c|  20 ++--
>  drivers/gpu/drm/i915/intel_dpll_mgr.c  |   2 +-
>  drivers/gpu/drm/i915/intel_fbdev.c |   4 +-
>  drivers/gpu/drm/i915/intel_lrc.c   |   2 +-
>  drivers/gpu/drm/i915/intel_lvds.c  |   4 +-
>  drivers/gpu/drm/i915/intel_overlay.c   |   4 +-
>  drivers/gpu/drm/i915/intel_pm.c|  50 +-
>  drivers/gpu/drm/i915/intel_psr.c   |   6 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c|  40 
>  drivers/gpu/drm/i915/intel_sdvo.c  |   8 +-
>  drivers/gpu/drm/i915/intel_tv.c|   2 +-
>  drivers/gpu/drm/i915/intel_uncore.c|   6 +-
>  28 files changed, 270 insertions(+), 277 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index be4bcdc..d7f471b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -612,7 +612,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, 
> void *data)
>   seq_puts(m, "Stall check waiting for page flip 
> ioctl, ");
>   seq_printf(m, "%d prepares\n", 
> atomic_read(>pending));
>  
> - if (INTEL_INFO(dev)->gen >= 4)
> + if (INTEL_INFO(dev_priv)->gen >= 4)
>   addr = 
> I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
>   else
>   addr = I915_READ(DSPADDR(crtc->plane));
> @@ -809,7 +809,7 @@ static int i915_interrupt_info(struct seq_file *m, void 
> *data)
>      I915_READ(GEN8_PCU_IIR));
>   seq_printf(m, "PCU interrupt enable:\t%08x\n",
>      I915_READ(GEN8_PCU_IER));
> - } else if (INTEL_INFO(dev)->gen >= 8) {
> + } else if (INTEL_INFO(dev_priv)->gen >= 8) {
>   seq_printf(m, "Master Interrupt Control:\t%08x\n",
>      I915_READ(GEN8_MASTER_IRQ));
>  
> @@ -935,7 +935,7 @@ static int i915_interrupt_info(struct seq_file *m, void 
> *data)
>      I915_READ(GTIMR));
>   }
>   for_each_engine(engine, dev_priv) {
> - if (INTEL_INFO(dev)->gen >= 6) {
> + if (INTEL_INFO(dev_priv)->gen >= 6) {
>   seq_printf(m,
>      "Graphics Interrupt mask (%s):   %08x\n",
>      engine->name,

[Intel-gfx] [RFC] Prefer INTEL_INFO(dev_priv) to INTEL_INFO(dev)

2016-04-07 Thread Dave Gordon 
Where we have a suitable dev_priv pointer, we can use that rather than
'dev' for accessing INTEL_INFO().  This removes one level of memory
reference, decreasing code size a little and possibly making everything
a little faster. We could also do this for all the macros that implicitly
use INTEL_INFO e.g. IS_CHERRYVIEW().

Coccinelle:

@dev_priv_param@
  function FUNC;
  idexpression struct drm_device *DEV;
  identifier DEV_PRIV;
@@
  FUNC(..., struct drm_i915_private *DEV_PRIV, ...)
  {
<...
INTEL_INFO
(
- DEV
+ 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
+ DEV_PRIV
)
...>
  }

followed by manually deleting 6 now-unused "dev" locals.

Signed-off-by: Dave Gordon 
---
 drivers/gpu/drm/i915/i915_debugfs.c|  66 +++---
 drivers/gpu/drm/i915/i915_dma.c|  30 +++---
 drivers/gpu/drm/i915/i915_drv.c|   4 +-
 drivers/gpu/drm/i915/i915_gem.c|   6 +-
 drivers/gpu/drm/i915/i915_gem_context.c|   2 +-
 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|  14 +--
 drivers/gpu/drm/i915/i915_gem_stolen.c |   6 +-
 drivers/gpu/drm/i915/i915_gpu_error.c  |  32 +++
 drivers/gpu/drm/i915/i915_irq.c|  36 
 drivers/gpu/drm/i915/i915_suspend.c|  20 ++--
 drivers/gpu/drm/i915/intel_color.c |  16 ++--
 drivers/gpu/drm/i915/intel_crt.c   |   6 +-
 drivers/gpu/drm/i915/intel_ddi.c   |   4 +-
 drivers/gpu/drm/i915/intel_display.c   | 141 ++---
 drivers/gpu/drm/i915/intel_dp.c|  20 ++--
 drivers/gpu/drm/i915/intel_dpll_mgr.c  |   2 +-
 drivers/gpu/drm/i915/intel_fbdev.c |   4 +-
 drivers/gpu/drm/i915/intel_lrc.c   |   2 +-
 drivers/gpu/drm/i915/intel_lvds.c  |   4 +-
 drivers/gpu/drm/i915/intel_overlay.c   |   4 +-
 drivers/gpu/drm/i915/intel_pm.c|  50 +-
 drivers/gpu/drm/i915/intel_psr.c   |   6 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c|  40 
 drivers/gpu/drm/i915/intel_sdvo.c  |   8 +-
 drivers/gpu/drm/i915/intel_tv.c|   2 +-
 drivers/gpu/drm/i915/intel_uncore.c|   6 +-
 28 files changed, 270 insertions(+), 277 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index be4bcdc..d7f471b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -612,7 +612,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void 
*data)
seq_puts(m, "Stall check waiting for page flip 
ioctl, ");
seq_printf(m, "%d prepares\n", 
atomic_read(>pending));
 
-   if (INTEL_INFO(dev)->gen >= 4)
+   if (INTEL_INFO(dev_priv)->gen >= 4)
addr = 
I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
else
addr = I915_READ(DSPADDR(crtc->plane));
@@ -809,7 +809,7 @@ static int i915_interrupt_info(struct seq_file *m, void 
*data)
   I915_READ(GEN8_PCU_IIR));
seq_printf(m, "PCU interrupt enable:\t%08x\n",
   I915_READ(GEN8_PCU_IER));
-   } else if (INTEL_INFO(dev)->gen >= 8) {
+   } else if (INTEL_INFO(dev_priv)->gen >= 8) {
seq_printf(m, "Master Interrupt Control:\t%08x\n",
   I915_READ(GEN8_MASTER_IRQ));
 
@@ -935,7 +935,7 @@ static int i915_interrupt_info(struct seq_file *m, void 
*data)
   I915_READ(GTIMR));
}
for_each_engine(engine, dev_priv) {
-   if (INTEL_INFO(dev)->gen >= 6) {
+   if (INTEL_INFO(dev_priv)->gen >= 6) {
seq_printf(m,
   "Graphics Interrupt mask (%s):   %08x\n",
   engine->name, I915_READ_IMR(engine));
@@ -1172,7 +1172,7 @@ static int i915_frequency_info(struct seq_file *m, void 
*unused)
   "efficient (RPe) frequency: %d MHz\n",
   intel_gpu_freq(dev_priv, 
dev_priv->rps.efficient_freq));
mutex_unlock(_priv->rps.hw_lock);
-   } else if (INTEL_INFO(dev)->gen >= 6) {
+   } else if (INTEL_INFO(dev_priv)->gen >= 6) {
u32 rp_state_limits;
u32 gt_perf_status;
u32 rp_state_cap;
@@ -1680,7 +1680,7 @@ static int i915_fbc_fc_get(void *data, u64