[PATCH] drm/i915: Add haswell_pcode_write function

[PATCH] drm/i915: Add haswell_pcode_write function

[Tom.O'Rourke]

From: Tom O'Rourke <Tom.O'Rourke@intel.com>

Based on sandybridge_pcode_write, haswell_pcode_write has an
additional field for address control.

Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f00e58..fd8b550 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
+int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
 int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
 
 /* intel_sideband.c */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6fbfdec..b674050 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6011,6 +6011,7 @@ enum punit_power_well {
 #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
 #define   DISPLAY_IPS_CONTROL			0x19
 #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
+#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
 #define GEN6_PCODE_DATA				0x138128
 #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
 #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1244ff8..9c47bc8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
 	return 0;
 }
 
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
+int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
 	}
 
 	I915_WRITE(GEN6_PCODE_DATA, val);
-	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
+	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
 
 	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
 		     500)) {
@@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
 	return 0;
 }
 
+int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
+{
+	return haswell_pcode_write(dev_priv, mbox, val, 0);
+}
+
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	int div;
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Add haswell_pcode_write function

[Ville Syrjälä]

On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> 
> Based on sandybridge_pcode_write, haswell_pcode_write has an
> additional field for address control.

It's already there in snb.

Do you have an actual use case for this? If so I wonder if we should
just change the mbox parameter to u32 and allow the caller to specify
it all?

> 
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>  drivers/gpu/drm/i915/i915_reg.h |    1 +
>  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0f00e58..fd8b550 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
>  
>  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
>  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
>  
>  /* intel_sideband.c */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6fbfdec..b674050 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6011,6 +6011,7 @@ enum punit_power_well {
>  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
>  #define GEN6_PCODE_DATA				0x138128
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1244ff8..9c47bc8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
>  	return 0;
>  }
>  
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
>  {
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
>  	}
>  
>  	I915_WRITE(GEN6_PCODE_DATA, val);
> -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
>  
>  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
>  		     500)) {
> @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
>  	return 0;
>  }
>  
> +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> +{
> +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> +}
> +
>  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
>  	int div;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add haswell_pcode_write function

[O'Rourke, Tom]

On Thu, Nov 06, 2014 at 10:28:53AM +0200, Ville Syrjälä wrote:
> On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > 
> > Based on sandybridge_pcode_write, haswell_pcode_write has an
> > additional field for address control.
> 
> It's already there in snb.
If the address control field is already there for SNB,
then I would prefer to modify sandybridge_pcode_write
instead of adding a haswell_pcode_write.

I based this patch on the Haswell documentation for
PCU_CR_GTDRIVER_MAILBOX_INTERFACE_0_2_0_GTTMMADR on
page 101 of
https://01.org/linuxgraphics/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-pcie-config-registers.pdf

Can you point me to the SNB documentation for the
address control field?   I do not have documentation
for address control for earlier than Haswell.

> 
> Do you have an actual use case for this? If so I wonder if we should
> just change the mbox parameter to u32 and allow the caller to specify
> it all?
> 
No, I do not have an actual use case for this yet.
I am working on something that will use it but that
is not ready yet.

Yes, we could change mbox to u32 and shift the work
to the caller.  The caller would need to combine the
u8 mailbox command with the address control field.
I do not think that would be better (or worse) but
I can make that change if others want it.

> > 
> > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> >  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
> >  3 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 0f00e58..fd8b550 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
> >  
> >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
> >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> >  
> >  /* intel_sideband.c */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 6fbfdec..b674050 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6011,6 +6011,7 @@ enum punit_power_well {
> >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> >  #define   DISPLAY_IPS_CONTROL			0x19
> >  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
> >  #define GEN6_PCODE_DATA				0x138128
> >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1244ff8..9c47bc8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> >  	return 0;
> >  }
> >  
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
> >  {
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  
> > @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> >  	}
> >  
> >  	I915_WRITE(GEN6_PCODE_DATA, val);
> > -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
> >  
> >  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> >  		     500)) {
> > @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> >  	return 0;
> >  }
> >  
> > +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > +{
> > +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> > +}
> > +
> >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> >  	int div;
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add haswell_pcode_write function

[Ville Syrjälä]

On Thu, Nov 06, 2014 at 09:35:48AM -0800, O'Rourke, Tom wrote:
> On Thu, Nov 06, 2014 at 10:28:53AM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> > > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > 
> > > Based on sandybridge_pcode_write, haswell_pcode_write has an
> > > additional field for address control.
> > 
> > It's already there in snb.
> If the address control field is already there for SNB,
> then I would prefer to modify sandybridge_pcode_write
> instead of adding a haswell_pcode_write.

Agreed.

> 
> I based this patch on the Haswell documentation for
> PCU_CR_GTDRIVER_MAILBOX_INTERFACE_0_2_0_GTTMMADR on
> page 101 of
> https://01.org/linuxgraphics/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-pcie-config-registers.pdf
> 
> Can you point me to the SNB documentation for the
> address control field?   I do not have documentation
> for address control for earlier than Haswell.

I found it in BSpec. Not sure it can be found in the PRMs.

> 
> > 
> > Do you have an actual use case for this? If so I wonder if we should
> > just change the mbox parameter to u32 and allow the caller to specify
> > it all?
> > 
> No, I do not have an actual use case for this yet.
> I am working on something that will use it but that
> is not ready yet.
> 
> Yes, we could change mbox to u32 and shift the work
> to the caller.  The caller would need to combine the
> u8 mailbox command with the address control field.
> I do not think that would be better (or worse) but
> I can make that change if others want it.

Well, I don't really mind either way. So unless anyone else objects to
one of the approaches feel free to pick either one.

> 
> > > 
> > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> > >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> > >  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
> > >  3 files changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0f00e58..fd8b550 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> > >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
> > >  
> > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
> > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> > >  
> > >  /* intel_sideband.c */
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 6fbfdec..b674050 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6011,6 +6011,7 @@ enum punit_power_well {
> > >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> > >  #define   DISPLAY_IPS_CONTROL			0x19
> > >  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > > +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)

I don't think we need the mask here. We generally don't have it, with some
exceptions like the csc stuff I did recently which deals in signed values,
and so needs it to avoid the sign bits from clobbering everything above.

But I have been dreaming occasionally about having such macros defined
in a way that would allow us to verify that we don't overflow anything
by accident. Eg. something like this:

#if CONFIG_I915_CHECK_REGISTERS
#define _BITFIELD(value,offset,size) ({ WARN_ON((value) & ~((1 << (size)) - 1)); ((value) << (shift)) })
#else
#define _BITFIELD(value,offset,size) ((value) << (shift))
#endif

#define PCODE_ADDR_CNTL(x) _BITFIELD((x), 8, 21)

But converting everything would be a big task, especially as we don't
have such macros for everything. Sometimes we just have a shift value,
and sometimes not even that and we calculate a shift using some case
specific magic, and sometimes the shift isn't strictly needed (eg.
it's 0, or just happens to match naturally the data we stuff in there
like in the case of page aligned addresses).

> > >  #define GEN6_PCODE_DATA				0x138128
> > >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> > >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 1244ff8..9c47bc8 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> > >  	return 0;
> > >  }
> > >  
> > > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
> > >  {
> > >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > >  
> > > @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > >  	}
> > >  
> > >  	I915_WRITE(GEN6_PCODE_DATA, val);
> > > -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > > +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
> > >  
> > >  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> > >  		     500)) {
> > > @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > >  	return 0;
> > >  }
> > >  
> > > +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > +{
> > > +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> > > +}
> > > +
> > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > >  {
> > >  	int div;
> > > -- 
> > > 1.7.9.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add haswell_pcode_write function

[O'Rourke, Tom]

On Thu, Nov 06, 2014 at 08:18:46PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 06, 2014 at 09:35:48AM -0800, O'Rourke, Tom wrote:
> > On Thu, Nov 06, 2014 at 10:28:53AM +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> > > > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > 
> > > > Based on sandybridge_pcode_write, haswell_pcode_write has an
> > > > additional field for address control.
> > > 
> > > It's already there in snb.
> > If the address control field is already there for SNB,
> > then I would prefer to modify sandybridge_pcode_write
> > instead of adding a haswell_pcode_write.
> 
> Agreed.
> 
> > 
> > I based this patch on the Haswell documentation for
> > PCU_CR_GTDRIVER_MAILBOX_INTERFACE_0_2_0_GTTMMADR on
> > page 101 of
> > https://01.org/linuxgraphics/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-pcie-config-registers.pdf
> > 
> > Can you point me to the SNB documentation for the
> > address control field?   I do not have documentation
> > for address control for earlier than Haswell.
> 
> I found it in BSpec. Not sure it can be found in the PRMs.
>
Thank you for the pointer.  I will rewrite the patch.
This current patch should be abandoned.

> > 
> > > 
> > > Do you have an actual use case for this? If so I wonder if we should
> > > just change the mbox parameter to u32 and allow the caller to specify
> > > it all?
> > > 
> > No, I do not have an actual use case for this yet.
> > I am working on something that will use it but that
> > is not ready yet.
> > 
> > Yes, we could change mbox to u32 and shift the work
> > to the caller.  The caller would need to combine the
> > u8 mailbox command with the address control field.
> > I do not think that would be better (or worse) but
> > I can make that change if others want it.
> 
> Well, I don't really mind either way. So unless anyone else objects to
> one of the approaches feel free to pick either one.
> 
> > 
> > > > 
> > > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> > > >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> > > >  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
> > > >  3 files changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0f00e58..fd8b550 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> > > >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
> > > >  
> > > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> > > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
> > > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> > > >  
> > > >  /* intel_sideband.c */
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 6fbfdec..b674050 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6011,6 +6011,7 @@ enum punit_power_well {
> > > >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> > > >  #define   DISPLAY_IPS_CONTROL			0x19
> > > >  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > > > +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
> 
> I don't think we need the mask here. We generally don't have it, with some
> exceptions like the csc stuff I did recently which deals in signed values,
> and so needs it to avoid the sign bits from clobbering everything above.
> 
> But I have been dreaming occasionally about having such macros defined
> in a way that would allow us to verify that we don't overflow anything
> by accident. Eg. something like this:
> 
> #if CONFIG_I915_CHECK_REGISTERS
> #define _BITFIELD(value,offset,size) ({ WARN_ON((value) & ~((1 << (size)) - 1)); ((value) << (shift)) })
> #else
> #define _BITFIELD(value,offset,size) ((value) << (shift))
> #endif
> 
> #define PCODE_ADDR_CNTL(x) _BITFIELD((x), 8, 21)
> 
> But converting everything would be a big task, especially as we don't
> have such macros for everything. Sometimes we just have a shift value,
> and sometimes not even that and we calculate a shift using some case
> specific magic, and sometimes the shift isn't strictly needed (eg.
> it's 0, or just happens to match naturally the data we stuff in there
> like in the case of page aligned addresses).
> 
I like the CONFIG_I915_CHECK_REGISTERS idea.  Until we
have something like that, I would like to keep the mask.
In addition to adding a little safety, the mask makes it
clear in the code how big the field should be.

> > > >  #define GEN6_PCODE_DATA				0x138128
> > > >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> > > >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 1244ff8..9c47bc8 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
> > > >  {
> > > >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > > >  
> > > > @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > >  	}
> > > >  
> > > >  	I915_WRITE(GEN6_PCODE_DATA, val);
> > > > -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > > > +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
> > > >  
> > > >  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> > > >  		     500)) {
> > > > @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > > +{
> > > > +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> > > > +}
> > > > +
> > > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > > >  {
> > > >  	int div;
> > > > -- 
> > > > 1.7.9.5
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Extend pcode mailbox interface

[Tom.O'Rourke]

From: Tom O'Rourke <Tom.O'Rourke@intel.com>

In sandybridge_pcode_read and sandybridge_pcode_write,
extend the mbox parameter from u8 to u32.

On Haswell and Sandybridge, bits 7:0 encode the mailbox
command and bits 28:8 are used for address control for
specific commands.

Based on suggestion from Ville Syrjälä.

Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    4 ++--
 drivers/gpu/drm/i915/intel_pm.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3f3035c..4dea835 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2954,8 +2954,8 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
 
-int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
+int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
+int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
 
 /* intel_sideband.c */
 u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9e87265..21faa92 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7154,7 +7154,7 @@ void intel_init_pm(struct drm_device *dev)
 	}
 }
 
-int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
+int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -7180,7 +7180,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
 	return 0;
 }
 
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
+int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Extend pcode mailbox interface

[Ville Syrjälä]

On Thu, Nov 13, 2014 at 06:50:10PM -0800, Tom.O'Rourke@intel.com wrote:
> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> 
> In sandybridge_pcode_read and sandybridge_pcode_write,
> extend the mbox parameter from u8 to u32.
> 
> On Haswell and Sandybridge, bits 7:0 encode the mailbox
> command and bits 28:8 are used for address control for
> specific commands.
> 
> Based on suggestion from Ville Syrjälä.
> 
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>

Not sure what we're going to do with this, but the spec does allow
passing some stuff in the high bits, so

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h |    4 ++--
>  drivers/gpu/drm/i915/intel_pm.c |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3f3035c..4dea835 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2954,8 +2954,8 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
>  void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
>  
> -int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> +int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
>  
>  /* intel_sideband.c */
>  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9e87265..21faa92 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7154,7 +7154,7 @@ void intel_init_pm(struct drm_device *dev)
>  	}
>  }
>  
> -int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> +int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
>  {
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> @@ -7180,7 +7180,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
>  	return 0;
>  }
>  
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val)
>  {
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Extend pcode mailbox interface

[Daniel Vetter]

On Tue, Nov 18, 2014 at 09:24:26PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 13, 2014 at 06:50:10PM -0800, Tom.O'Rourke@intel.com wrote:
> > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > 
> > In sandybridge_pcode_read and sandybridge_pcode_write,
> > extend the mbox parameter from u8 to u32.
> > 
> > On Haswell and Sandybridge, bits 7:0 encode the mailbox
> > command and bits 28:8 are used for address control for
> > specific commands.
> > 
> > Based on suggestion from Ville Syrjälä.
> > 
> > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> 
> Not sure what we're going to do with this, but the spec does allow
> passing some stuff in the high bits, so
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx