[Intel-gfx] [PATCH] drm/i915: Add haswell_pcode_write function
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Nov 6 19:18:46 CET 2014
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 at intel.com wrote:
> > > From: Tom O'Rourke <Tom.O'Rourke at 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 at 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 at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list