Re: [Intel-gfx] [PATCH 1/5] drm/i915: Turn __raw_i915_read8() & co. in to inline functions

[Ville Syrjälä]

On Thu, Oct 22, 2015 at 03:32:11PM +0200, Daniel Vetter wrote:
> On Thu, Oct 22, 2015 at 03:34:56PM +0300, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > There's no need for __raw_i915_read8() & co. bot be macros, so make them
> 
> s/bot/be

Actually s/bot/to/ :)

> 
> > inline funcitons. To avoid typo mistakes generate the inline functions
> 
> s/funcitons/functions/
> 
> > using preprocessor templates.
> > 
> > We have a few users of the raw register acces functions outside
> > intel_uncore.c, so let's also move the functions into intel_drv.h.
> > 
> > While doing that switch I915_READ_FW() & co. to use the
> > __raw_i915_read() functions, and use the _FW macros everywhere
> > outside intel_uncore.c where we want to read registers without
> > grabbing forcewake and whatnot. The only exception is
> > i915_check_vgpu() which itself gets called from intel_uncore.c,
> > so using the __raw_i915_read stuff there seems appropriate.
> > 
> > v2: Squash in the intel_uncore.c->i915_drv.h move
> > Convert I915_READ_FW() to use __raw_i915_read(), and use
> > I915_READ_FW() outside of intel_uncore.c (Chris)
> > 
> > Signed-off-by: Ville Syrjälä 
> 
> Reviewed-by: Daniel Vetter 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h | 30 --
> >  drivers/gpu/drm/i915/i915_irq.c | 10 --
> >  drivers/gpu/drm/i915/i915_vgpu.c|  6 +++---
> >  drivers/gpu/drm/i915/intel_uncore.c | 14 +-
> >  5 files changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index eca94d0..89ba549 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1523,7 +1523,7 @@ static int gen6_drpc_info(struct seq_file *m)
> > seq_printf(m, "RC information accurate: %s\n", yesno(count < 
> > 51));
> > }
> >  
> > -   gt_core_status = readl(dev_priv->regs + GEN6_GT_CORE_STATUS);
> > +   gt_core_status = I915_READ_FW(GEN6_GT_CORE_STATUS);
> > trace_i915_reg_rw(false, GEN6_GT_CORE_STATUS, gt_core_status, 4, true);
> >  
> > rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 9a15ebe..01fdc70 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3452,6 +3452,32 @@ int intel_freq_opcode(struct drm_i915_private 
> > *dev_priv, int val);
> >  #define POSTING_READ(reg)  (void)I915_READ_NOTRACE(reg)
> >  #define POSTING_READ16(reg)(void)I915_READ16_NOTRACE(reg)
> >  
> > +#define __raw_read(x, s) \
> > +static inline uint##x##_t __raw_i915_read##x(struct drm_i915_private 
> > *dev_priv, \
> > +uint32_t reg) \
> > +{ \
> > +   return read##s(dev_priv->regs + reg); \
> > +}
> > +
> > +#define __raw_write(x, s) \
> > +static inline void __raw_i915_write##x(struct drm_i915_private *dev_priv, \
> > +  uint32_t reg, uint##x##_t val) \
> > +{ \
> > +   write##s(val, dev_priv->regs + reg); \
> > +}
> > +__raw_read(8, b)
> > +__raw_read(16, w)
> > +__raw_read(32, l)
> > +__raw_read(64, q)
> > +
> > +__raw_write(8, b)
> > +__raw_write(16, w)
> > +__raw_write(32, l)
> > +__raw_write(64, q)
> > +
> > +#undef __raw_read
> > +#undef __raw_write
> > +
> >  /* These are untraced mmio-accessors that are only valid to be used inside
> >   * criticial sections inside IRQ handlers where forcewake is explicitly
> >   * controlled.
> > @@ -3459,8 +3485,8 @@ int intel_freq_opcode(struct drm_i915_private 
> > *dev_priv, int val);
> >   * Note: Should only be used between intel_uncore_forcewake_irqlock() and
> >   * intel_uncore_forcewake_irqunlock().
> >   */
> > -#define I915_READ_FW(reg__) readl(dev_priv->regs + (reg__))
> > -#define I915_WRITE_FW(reg__, val__) writel(val__, dev_priv->regs + (reg__))
> > +#define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__))
> > +#define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), 
> > (val__))
> >  #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__)
> >  
> >  /* "Broadcast RGB" property */
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 9caf22c..a0ed58d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -717,9 +717,7 @@ static u32 g4x_get_vblank_counter(struct drm_device 
> > *dev, unsigned int pipe)
> > return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
> >  }
> >  
> > -/* raw reads, only for fast reads of display block, no need for forcewake 
> > etc. */
> > -#define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + 
> > (reg__))
> > -
> > +/* I915_READ_FW, only for fast reads of display block, no need for 
> > 

[Intel-gfx] [PATCH 1/5] drm/i915: Turn __raw_i915_read8() & co. in to inline functions

[ville . syrjala]

From: Ville Syrjälä 

There's no need for __raw_i915_read8() & co. bot be macros, so make them
inline funcitons. To avoid typo mistakes generate the inline functions
using preprocessor templates.

We have a few users of the raw register acces functions outside
intel_uncore.c, so let's also move the functions into intel_drv.h.

While doing that switch I915_READ_FW() & co. to use the
__raw_i915_read() functions, and use the _FW macros everywhere
outside intel_uncore.c where we want to read registers without
grabbing forcewake and whatnot. The only exception is
i915_check_vgpu() which itself gets called from intel_uncore.c,
so using the __raw_i915_read stuff there seems appropriate.

v2: Squash in the intel_uncore.c->i915_drv.h move
Convert I915_READ_FW() to use __raw_i915_read(), and use
I915_READ_FW() outside of intel_uncore.c (Chris)

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.h | 30 --
 drivers/gpu/drm/i915/i915_irq.c | 10 --
 drivers/gpu/drm/i915/i915_vgpu.c|  6 +++---
 drivers/gpu/drm/i915/intel_uncore.c | 14 +-
 5 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index eca94d0..89ba549 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1523,7 +1523,7 @@ static int gen6_drpc_info(struct seq_file *m)
seq_printf(m, "RC information accurate: %s\n", yesno(count < 
51));
}
 
-   gt_core_status = readl(dev_priv->regs + GEN6_GT_CORE_STATUS);
+   gt_core_status = I915_READ_FW(GEN6_GT_CORE_STATUS);
trace_i915_reg_rw(false, GEN6_GT_CORE_STATUS, gt_core_status, 4, true);
 
rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9a15ebe..01fdc70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3452,6 +3452,32 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, 
int val);
 #define POSTING_READ(reg)  (void)I915_READ_NOTRACE(reg)
 #define POSTING_READ16(reg)(void)I915_READ16_NOTRACE(reg)
 
+#define __raw_read(x, s) \
+static inline uint##x##_t __raw_i915_read##x(struct drm_i915_private 
*dev_priv, \
+uint32_t reg) \
+{ \
+   return read##s(dev_priv->regs + reg); \
+}
+
+#define __raw_write(x, s) \
+static inline void __raw_i915_write##x(struct drm_i915_private *dev_priv, \
+  uint32_t reg, uint##x##_t val) \
+{ \
+   write##s(val, dev_priv->regs + reg); \
+}
+__raw_read(8, b)
+__raw_read(16, w)
+__raw_read(32, l)
+__raw_read(64, q)
+
+__raw_write(8, b)
+__raw_write(16, w)
+__raw_write(32, l)
+__raw_write(64, q)
+
+#undef __raw_read
+#undef __raw_write
+
 /* These are untraced mmio-accessors that are only valid to be used inside
  * criticial sections inside IRQ handlers where forcewake is explicitly
  * controlled.
@@ -3459,8 +3485,8 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, 
int val);
  * Note: Should only be used between intel_uncore_forcewake_irqlock() and
  * intel_uncore_forcewake_irqunlock().
  */
-#define I915_READ_FW(reg__) readl(dev_priv->regs + (reg__))
-#define I915_WRITE_FW(reg__, val__) writel(val__, dev_priv->regs + (reg__))
+#define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__))
+#define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), 
(val__))
 #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__)
 
 /* "Broadcast RGB" property */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9caf22c..a0ed58d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -717,9 +717,7 @@ static u32 g4x_get_vblank_counter(struct drm_device *dev, 
unsigned int pipe)
return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
 }
 
-/* raw reads, only for fast reads of display block, no need for forcewake etc. 
*/
-#define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + 
(reg__))
-
+/* I915_READ_FW, only for fast reads of display block, no need for forcewake 
etc. */
 static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
struct drm_device *dev = crtc->base.dev;
@@ -733,9 +731,9 @@ static int __intel_get_crtc_scanline(struct intel_crtc 
*crtc)
vtotal /= 2;
 
if (IS_GEN2(dev))
-   position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & 
DSL_LINEMASK_GEN2;
+   position = I915_READ_FW(PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
else
-   position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & 
DSL_LINEMASK_GEN3;
+   position = I915_READ_FW(PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
/*
 * On HSW, the DSL reg (0x7) appears to 

Re: [Intel-gfx] [PATCH 1/5] drm/i915: Turn __raw_i915_read8() & co. in to inline functions

[Daniel Vetter]

On Thu, Oct 22, 2015 at 03:34:56PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> There's no need for __raw_i915_read8() & co. bot be macros, so make them

s/bot/be

> inline funcitons. To avoid typo mistakes generate the inline functions

s/funcitons/functions/

> using preprocessor templates.
> 
> We have a few users of the raw register acces functions outside
> intel_uncore.c, so let's also move the functions into intel_drv.h.
> 
> While doing that switch I915_READ_FW() & co. to use the
> __raw_i915_read() functions, and use the _FW macros everywhere
> outside intel_uncore.c where we want to read registers without
> grabbing forcewake and whatnot. The only exception is
> i915_check_vgpu() which itself gets called from intel_uncore.c,
> so using the __raw_i915_read stuff there seems appropriate.
> 
> v2: Squash in the intel_uncore.c->i915_drv.h move
> Convert I915_READ_FW() to use __raw_i915_read(), and use
> I915_READ_FW() outside of intel_uncore.c (Chris)
> 
> Signed-off-by: Ville Syrjälä 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h | 30 --
>  drivers/gpu/drm/i915/i915_irq.c | 10 --
>  drivers/gpu/drm/i915/i915_vgpu.c|  6 +++---
>  drivers/gpu/drm/i915/intel_uncore.c | 14 +-
>  5 files changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index eca94d0..89ba549 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1523,7 +1523,7 @@ static int gen6_drpc_info(struct seq_file *m)
>   seq_printf(m, "RC information accurate: %s\n", yesno(count < 
> 51));
>   }
>  
> - gt_core_status = readl(dev_priv->regs + GEN6_GT_CORE_STATUS);
> + gt_core_status = I915_READ_FW(GEN6_GT_CORE_STATUS);
>   trace_i915_reg_rw(false, GEN6_GT_CORE_STATUS, gt_core_status, 4, true);
>  
>   rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9a15ebe..01fdc70 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3452,6 +3452,32 @@ int intel_freq_opcode(struct drm_i915_private 
> *dev_priv, int val);
>  #define POSTING_READ(reg)(void)I915_READ_NOTRACE(reg)
>  #define POSTING_READ16(reg)  (void)I915_READ16_NOTRACE(reg)
>  
> +#define __raw_read(x, s) \
> +static inline uint##x##_t __raw_i915_read##x(struct drm_i915_private 
> *dev_priv, \
> +  uint32_t reg) \
> +{ \
> + return read##s(dev_priv->regs + reg); \
> +}
> +
> +#define __raw_write(x, s) \
> +static inline void __raw_i915_write##x(struct drm_i915_private *dev_priv, \
> +uint32_t reg, uint##x##_t val) \
> +{ \
> + write##s(val, dev_priv->regs + reg); \
> +}
> +__raw_read(8, b)
> +__raw_read(16, w)
> +__raw_read(32, l)
> +__raw_read(64, q)
> +
> +__raw_write(8, b)
> +__raw_write(16, w)
> +__raw_write(32, l)
> +__raw_write(64, q)
> +
> +#undef __raw_read
> +#undef __raw_write
> +
>  /* These are untraced mmio-accessors that are only valid to be used inside
>   * criticial sections inside IRQ handlers where forcewake is explicitly
>   * controlled.
> @@ -3459,8 +3485,8 @@ int intel_freq_opcode(struct drm_i915_private 
> *dev_priv, int val);
>   * Note: Should only be used between intel_uncore_forcewake_irqlock() and
>   * intel_uncore_forcewake_irqunlock().
>   */
> -#define I915_READ_FW(reg__) readl(dev_priv->regs + (reg__))
> -#define I915_WRITE_FW(reg__, val__) writel(val__, dev_priv->regs + (reg__))
> +#define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__))
> +#define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), 
> (val__))
>  #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__)
>  
>  /* "Broadcast RGB" property */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9caf22c..a0ed58d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -717,9 +717,7 @@ static u32 g4x_get_vblank_counter(struct drm_device *dev, 
> unsigned int pipe)
>   return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
>  }
>  
> -/* raw reads, only for fast reads of display block, no need for forcewake 
> etc. */
> -#define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + 
> (reg__))
> -
> +/* I915_READ_FW, only for fast reads of display block, no need for forcewake 
> etc. */
>  static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  {
>   struct drm_device *dev = crtc->base.dev;
> @@ -733,9 +731,9 @@ static int __intel_get_crtc_scanline(struct intel_crtc 
> *crtc)
>   vtotal /= 2;
>  
>   if (IS_GEN2(dev))
> - position =