mbox series

[0/3] IRQ initialization debloat and conversion to uncore

Message ID 20190409003729.20857-1-paulo.r.zanoni@intel.com (mailing list archive)
Headers show
Series IRQ initialization debloat and conversion to uncore | expand

Message

Zanoni, Paulo R April 9, 2019, 12:37 a.m. UTC 
The first patch is a simple refactor to try to debloat our IRQ
initialization and the second is a tiny conversion to the new
intel_uncore model. I'm not sure how much we want patch 3 right now,
but my understanding is that we want to move in that direction anyway,
so why not now.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Paulo Zanoni (3):
  drm/i915: refactor the IRQ init/reset macros
  drm/i915: convert the IRQ initialization functions to intel_uncore
  drm/i915: fully convert the IRQ initialization macros to intel_uncore

 drivers/gpu/drm/i915/i915_irq.c | 275 +++++++++++++++++++-------------
 1 file changed, 165 insertions(+), 110 deletions(-)

Comments

Zanoni, Paulo R April 9, 2019, 5:34 p.m. UTC | #1 
Em ter, 2019-04-09 às 00:44 +0000, Patchwork escreveu:
> == Series Details ==
> 
> Series: IRQ initialization debloat and conversion to uncore
> URL   : https://patchwork.freedesktop.org/series/59202/
> State : warning
> 
> == Summary ==
> 
> $ dim checkpatch origin/drm-tip
> 7f73d1fe31bb drm/i915: refactor the IRQ init/reset macros
> -:114: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> #114: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> +#define GEN8_IRQ_RESET_NDX(type, which) \
> +	gen3_irq_reset(dev_priv, GEN8_##type##_IMR(which), \
> +		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> 
> -:172: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> #172: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> +	gen3_irq_init(dev_priv, GEN8_##type##_IMR(which), \
> +		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> +		      imr_val, ier_val)
> 
> total: 0 errors, 0 warnings, 2 checks, 135 lines checked
> 82160241d80f drm/i915: convert the IRQ initialization functions to intel_uncore
> 8c1c76059a41 drm/i915: fully convert the IRQ initialization macros to intel_uncore
> -:24: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> #24: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> +#define GEN8_IRQ_RESET_NDX(uncore, type, which) \
> +	gen3_irq_reset((uncore), GEN8_##type##_IMR(which), \
>  		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> 
> -:46: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> #46: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> +#define GEN8_IRQ_INIT_NDX(uncore, type, which, imr_val, ier_val) \
> +	gen3_irq_init((uncore), GEN8_##type##_IMR(which), \
>  		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
>  		      imr_val, ier_val)

The whiches are not really a regression, but OK we can deal with them
to make the robots happy.

> 
> -:401: ERROR:SPACING: space prohibited before that close parenthesis ')'
> #401: FILE: drivers/gpu/drm/i915/i915_irq.c:4228:
> +	GEN2_IRQ_RESET(uncore, );
> 
> -:416: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> #416: FILE: drivers/gpu/drm/i915/i915_irq.c:4252:
> +	GEN2_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
>  	                      ^
> 
> -:433: ERROR:SPACING: space prohibited before that close parenthesis ')'
> #433: FILE: drivers/gpu/drm/i915/i915_irq.c:4397:
> +	GEN3_IRQ_RESET(uncore, );
> 
> -:448: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> #448: FILE: drivers/gpu/drm/i915/i915_irq.c:4430:
> +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
>  	                      ^
> 
> -:464: ERROR:SPACING: space prohibited before that close parenthesis ')'
> #464: FILE: drivers/gpu/drm/i915/i915_irq.c:4508:
> +	GEN3_IRQ_RESET(uncore, );
> 
> -:479: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> #479: FILE: drivers/gpu/drm/i915/i915_irq.c:4552:
> +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);

For these ones I really think the spaces help. I would love to read
some opinions. Perhaps some comment like /* paste token here */ would
help make the code more readable and could help silence checkpatch.
Opinions?

>  	                      ^
> 
> total: 6 errors, 0 warnings, 2 checks, 432 lines checked
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala April 9, 2019, 6:20 p.m. UTC | #2 
On Tue, Apr 09, 2019 at 10:34:22AM -0700, Paulo Zanoni wrote:
> Em ter, 2019-04-09 às 00:44 +0000, Patchwork escreveu:
> > == Series Details ==
> > 
> > Series: IRQ initialization debloat and conversion to uncore
> > URL   : https://patchwork.freedesktop.org/series/59202/
> > State : warning
> > 
> > == Summary ==
> > 
> > $ dim checkpatch origin/drm-tip
> > 7f73d1fe31bb drm/i915: refactor the IRQ init/reset macros
> > -:114: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > #114: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> > +#define GEN8_IRQ_RESET_NDX(type, which) \
> > +	gen3_irq_reset(dev_priv, GEN8_##type##_IMR(which), \
> > +		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> > 
> > -:172: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > #172: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> > +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> > +	gen3_irq_init(dev_priv, GEN8_##type##_IMR(which), \
> > +		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> > +		      imr_val, ier_val)
> > 
> > total: 0 errors, 0 warnings, 2 checks, 135 lines checked
> > 82160241d80f drm/i915: convert the IRQ initialization functions to intel_uncore
> > 8c1c76059a41 drm/i915: fully convert the IRQ initialization macros to intel_uncore
> > -:24: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > #24: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> > +#define GEN8_IRQ_RESET_NDX(uncore, type, which) \
> > +	gen3_irq_reset((uncore), GEN8_##type##_IMR(which), \
> >  		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> > 
> > -:46: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > #46: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> > +#define GEN8_IRQ_INIT_NDX(uncore, type, which, imr_val, ier_val) \
> > +	gen3_irq_init((uncore), GEN8_##type##_IMR(which), \
> >  		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> >  		      imr_val, ier_val)
> 
> The whiches are not really a regression, but OK we can deal with them
> to make the robots happy.
> 
> > 
> > -:401: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > #401: FILE: drivers/gpu/drm/i915/i915_irq.c:4228:
> > +	GEN2_IRQ_RESET(uncore, );
> > 
> > -:416: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > #416: FILE: drivers/gpu/drm/i915/i915_irq.c:4252:
> > +	GEN2_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> >  	                      ^
> > 
> > -:433: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > #433: FILE: drivers/gpu/drm/i915/i915_irq.c:4397:
> > +	GEN3_IRQ_RESET(uncore, );
> > 
> > -:448: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > #448: FILE: drivers/gpu/drm/i915/i915_irq.c:4430:
> > +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> >  	                      ^
> > 
> > -:464: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > #464: FILE: drivers/gpu/drm/i915/i915_irq.c:4508:
> > +	GEN3_IRQ_RESET(uncore, );
> > 
> > -:479: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > #479: FILE: drivers/gpu/drm/i915/i915_irq.c:4552:
> > +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> 
> For these ones I really think the spaces help. I would love to read
> some opinions. Perhaps some comment like /* paste token here */ would
> help make the code more readable and could help silence checkpatch.
> Opinions?

Or maybe rename the registers to eg. I9XX_IIR?

> 
> >  	                      ^
> > 
> > total: 6 errors, 0 warnings, 2 checks, 432 lines checked
> > 
> > _______________________________________________
> > 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
Zanoni, Paulo R April 10, 2019, 10:53 p.m. UTC | #3 
Em ter, 2019-04-09 às 21:20 +0300, Ville Syrjälä escreveu:
> On Tue, Apr 09, 2019 at 10:34:22AM -0700, Paulo Zanoni wrote:
> > Em ter, 2019-04-09 às 00:44 +0000, Patchwork escreveu:
> > > == Series Details ==
> > > 
> > > Series: IRQ initialization debloat and conversion to uncore
> > > URL   : https://patchwork.freedesktop.org/series/59202/
> > > State : warning
> > > 
> > > == Summary ==
> > > 
> > > $ dim checkpatch origin/drm-tip
> > > 7f73d1fe31bb drm/i915: refactor the IRQ init/reset macros
> > > -:114: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > #114: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> > > +#define GEN8_IRQ_RESET_NDX(type, which) \
> > > +	gen3_irq_reset(dev_priv, GEN8_##type##_IMR(which), \
> > > +		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> > > 
> > > -:172: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > #172: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> > > +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> > > +	gen3_irq_init(dev_priv, GEN8_##type##_IMR(which), \
> > > +		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> > > +		      imr_val, ier_val)
> > > 
> > > total: 0 errors, 0 warnings, 2 checks, 135 lines checked
> > > 82160241d80f drm/i915: convert the IRQ initialization functions to intel_uncore
> > > 8c1c76059a41 drm/i915: fully convert the IRQ initialization macros to intel_uncore
> > > -:24: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > #24: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> > > +#define GEN8_IRQ_RESET_NDX(uncore, type, which) \
> > > +	gen3_irq_reset((uncore), GEN8_##type##_IMR(which), \
> > >  		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> > > 
> > > -:46: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > #46: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> > > +#define GEN8_IRQ_INIT_NDX(uncore, type, which, imr_val, ier_val) \
> > > +	gen3_irq_init((uncore), GEN8_##type##_IMR(which), \
> > >  		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> > >  		      imr_val, ier_val)
> > 
> > The whiches are not really a regression, but OK we can deal with them
> > to make the robots happy.
> > 
> > > -:401: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > > #401: FILE: drivers/gpu/drm/i915/i915_irq.c:4228:
> > > +	GEN2_IRQ_RESET(uncore, );
> > > 
> > > -:416: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > > #416: FILE: drivers/gpu/drm/i915/i915_irq.c:4252:
> > > +	GEN2_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> > >  	                      ^
> > > 
> > > -:433: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > > #433: FILE: drivers/gpu/drm/i915/i915_irq.c:4397:
> > > +	GEN3_IRQ_RESET(uncore, );
> > > 
> > > -:448: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > > #448: FILE: drivers/gpu/drm/i915/i915_irq.c:4430:
> > > +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> > >  	                      ^
> > > 
> > > -:464: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > > #464: FILE: drivers/gpu/drm/i915/i915_irq.c:4508:
> > > +	GEN3_IRQ_RESET(uncore, );
> > > 
> > > -:479: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > > #479: FILE: drivers/gpu/drm/i915/i915_irq.c:4552:
> > > +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> > 
> > For these ones I really think the spaces help. I would love to read
> > some opinions. Perhaps some comment like /* paste token here */ would
> > help make the code more readable and could help silence checkpatch.
> > Opinions?
> 
> Or maybe rename the registers to eg. I9XX_IIR?

That makes more sense. We use these regs on gen2 too, so I suppose
I8XX_IIR (or GEN2_IIR) would make more sense. OTOH it would break our
current naming rule.

I'll submit v2 soon. Thanks.

> 
> > >  	                      ^
> > > 
> > > total: 6 errors, 0 warnings, 2 checks, 432 lines checked
> > > 
> > > _______________________________________________
> > > 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
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala April 11, 2019, 11:04 a.m. UTC | #4 
On Wed, Apr 10, 2019 at 03:53:05PM -0700, Paulo Zanoni wrote:
> Em ter, 2019-04-09 às 21:20 +0300, Ville Syrjälä escreveu:
> > On Tue, Apr 09, 2019 at 10:34:22AM -0700, Paulo Zanoni wrote:
> > > Em ter, 2019-04-09 às 00:44 +0000, Patchwork escreveu:
> > > > == Series Details ==
> > > > 
> > > > Series: IRQ initialization debloat and conversion to uncore
> > > > URL   : https://patchwork.freedesktop.org/series/59202/
> > > > State : warning
> > > > 
> > > > == Summary ==
> > > > 
> > > > $ dim checkpatch origin/drm-tip
> > > > 7f73d1fe31bb drm/i915: refactor the IRQ init/reset macros
> > > > -:114: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > > #114: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> > > > +#define GEN8_IRQ_RESET_NDX(type, which) \
> > > > +	gen3_irq_reset(dev_priv, GEN8_##type##_IMR(which), \
> > > > +		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> > > > 
> > > > -:172: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > > #172: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> > > > +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> > > > +	gen3_irq_init(dev_priv, GEN8_##type##_IMR(which), \
> > > > +		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> > > > +		      imr_val, ier_val)
> > > > 
> > > > total: 0 errors, 0 warnings, 2 checks, 135 lines checked
> > > > 82160241d80f drm/i915: convert the IRQ initialization functions to intel_uncore
> > > > 8c1c76059a41 drm/i915: fully convert the IRQ initialization macros to intel_uncore
> > > > -:24: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > > #24: FILE: drivers/gpu/drm/i915/i915_irq.c:169:
> > > > +#define GEN8_IRQ_RESET_NDX(uncore, type, which) \
> > > > +	gen3_irq_reset((uncore), GEN8_##type##_IMR(which), \
> > > >  		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> > > > 
> > > > -:46: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'which' - possible side-effects?
> > > > #46: FILE: drivers/gpu/drm/i915/i915_irq.c:236:
> > > > +#define GEN8_IRQ_INIT_NDX(uncore, type, which, imr_val, ier_val) \
> > > > +	gen3_irq_init((uncore), GEN8_##type##_IMR(which), \
> > > >  		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> > > >  		      imr_val, ier_val)
> > > 
> > > The whiches are not really a regression, but OK we can deal with them
> > > to make the robots happy.
> > > 
> > > > -:401: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > > > #401: FILE: drivers/gpu/drm/i915/i915_irq.c:4228:
> > > > +	GEN2_IRQ_RESET(uncore, );
> > > > 
> > > > -:416: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > > > #416: FILE: drivers/gpu/drm/i915/i915_irq.c:4252:
> > > > +	GEN2_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> > > >  	                      ^
> > > > 
> > > > -:433: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > > > #433: FILE: drivers/gpu/drm/i915/i915_irq.c:4397:
> > > > +	GEN3_IRQ_RESET(uncore, );
> > > > 
> > > > -:448: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > > > #448: FILE: drivers/gpu/drm/i915/i915_irq.c:4430:
> > > > +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> > > >  	                      ^
> > > > 
> > > > -:464: ERROR:SPACING: space prohibited before that close parenthesis ')'
> > > > #464: FILE: drivers/gpu/drm/i915/i915_irq.c:4508:
> > > > +	GEN3_IRQ_RESET(uncore, );
> > > > 
> > > > -:479: ERROR:SPACING: space prohibited before that ',' (ctx:WxW)
> > > > #479: FILE: drivers/gpu/drm/i915/i915_irq.c:4552:
> > > > +	GEN3_IRQ_INIT(uncore, , dev_priv->irq_mask, enable_mask);
> > > 
> > > For these ones I really think the spaces help. I would love to read
> > > some opinions. Perhaps some comment like /* paste token here */ would
> > > help make the code more readable and could help silence checkpatch.
> > > Opinions?
> > 
> > Or maybe rename the registers to eg. I9XX_IIR?
> 
> That makes more sense. We use these regs on gen2 too, so I suppose
> I8XX_IIR (or GEN2_IIR) would make more sense. OTOH it would break our
> current naming rule.

I tend to use i9xx to indicate anything gmch, and sometimes
it even means pre-skl :/ Not the best naming scheme perhaps
but I've not been able to come up with anything better.