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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx wrote:
> > > > From: Tom O'Rourke <Tom.O'Rourke@xxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxxxxxxxxxxxx
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux