[Intel-gfx] [PATCH] drm/i915: Add haswell_pcode_write function
O'Rourke, Tom
Tom.O'Rourke at intel.com
Thu Nov 6 21:21:26 CET 2014
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 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.
>
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 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).
>
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 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