* [PATCH] drm/i915: Add haswell_pcode_write function @ 2014-11-06 1:32 Tom.O'Rourke 2014-11-06 8:28 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Tom.O'Rourke @ 2014-11-06 1:32 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Add haswell_pcode_write function 2014-11-06 1:32 [PATCH] drm/i915: Add haswell_pcode_write function Tom.O'Rourke @ 2014-11-06 8:28 ` Ville Syrjälä 2014-11-06 17:35 ` O'Rourke, Tom 2014-11-14 2:50 ` [PATCH] drm/i915: Extend pcode mailbox interface Tom.O'Rourke 0 siblings, 2 replies; 8+ messages in thread From: Ville Syrjälä @ 2014-11-06 8:28 UTC (permalink / raw) To: Tom.O'Rourke; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Add haswell_pcode_write function 2014-11-06 8:28 ` Ville Syrjälä @ 2014-11-06 17:35 ` O'Rourke, Tom 2014-11-06 18:18 ` Ville Syrjälä 2014-11-14 2:50 ` [PATCH] drm/i915: Extend pcode mailbox interface Tom.O'Rourke 1 sibling, 1 reply; 8+ messages in thread From: O'Rourke, Tom @ 2014-11-06 17:35 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Add haswell_pcode_write function 2014-11-06 17:35 ` O'Rourke, Tom @ 2014-11-06 18:18 ` Ville Syrjälä 2014-11-06 20:21 ` O'Rourke, Tom 0 siblings, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2014-11-06 18:18 UTC (permalink / raw) To: O'Rourke, Tom; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Add haswell_pcode_write function 2014-11-06 18:18 ` Ville Syrjälä @ 2014-11-06 20:21 ` O'Rourke, Tom 0 siblings, 0 replies; 8+ messages in thread From: O'Rourke, Tom @ 2014-11-06 20:21 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/i915: Extend pcode mailbox interface 2014-11-06 8:28 ` Ville Syrjälä 2014-11-06 17:35 ` O'Rourke, Tom @ 2014-11-14 2:50 ` Tom.O'Rourke 2014-11-18 19:24 ` Ville Syrjälä 1 sibling, 1 reply; 8+ messages in thread From: Tom.O'Rourke @ 2014-11-14 2:50 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Extend pcode mailbox interface 2014-11-14 2:50 ` [PATCH] drm/i915: Extend pcode mailbox interface Tom.O'Rourke @ 2014-11-18 19:24 ` Ville Syrjälä 2014-11-19 13:40 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2014-11-18 19:24 UTC (permalink / raw) To: Tom.O'Rourke; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Extend pcode mailbox interface 2014-11-18 19:24 ` Ville Syrjälä @ 2014-11-19 13:40 ` Daniel Vetter 0 siblings, 0 replies; 8+ messages in thread From: Daniel Vetter @ 2014-11-19 13:40 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-11-19 13:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-06 1:32 [PATCH] drm/i915: Add haswell_pcode_write function Tom.O'Rourke 2014-11-06 8:28 ` Ville Syrjälä 2014-11-06 17:35 ` O'Rourke, Tom 2014-11-06 18:18 ` Ville Syrjälä 2014-11-06 20:21 ` O'Rourke, Tom 2014-11-14 2:50 ` [PATCH] drm/i915: Extend pcode mailbox interface Tom.O'Rourke 2014-11-18 19:24 ` Ville Syrjälä 2014-11-19 13:40 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox