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
