[Intel-gfx] [PATCH] drm/i915/vlv: Take forcewake on media engine writes
Since commit 940aece471bd ("drm/i915/vlv: Valleyview support for forcewake Individual power wells.") we have only taken media engine forcewake correctly on reads, but only taken render engine forcewake on media engine writes and omitted the media domain. This asymmetry might have caused unstable behaviour on media ring access. Fix is to take media engine forcewake symmetrically to writes. References: https://bugs.freedesktop.org/show_bug.cgi?id=88012 Cc: Deepak SCc: Jesse Barnes Cc: Chris Wilson Signed-off-by: Mika Kuoppala --- drivers/gpu/drm/i915/intel_uncore.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 277e60ae0e47..a2e204088aa5 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -902,6 +902,23 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t GEN6_WRITE_FOOTER; \ } +#define __vlv_write(x) \ +static void \ +vlv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \ + enum forcewake_domains fw_engine = 0; \ + GEN6_WRITE_HEADER; \ + if (!NEEDS_FORCE_WAKE(offset)) \ + fw_engine = 0; \ + else if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(offset)) \ + fw_engine = FORCEWAKE_RENDER; \ + else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \ + fw_engine = FORCEWAKE_MEDIA; \ + if (fw_engine) \ + __force_wake_get(dev_priv, fw_engine); \ + __raw_i915_write##x(dev_priv, reg, val); \ + GEN6_WRITE_FOOTER; \ +} + static const i915_reg_t gen8_shadowed_regs[] = { FORCEWAKE_MT, GEN6_RPNSWREQ, @@ -1019,6 +1036,10 @@ __gen8_write(8) __gen8_write(16) __gen8_write(32) __gen8_write(64) +__vlv_write(8) +__vlv_write(16) +__vlv_write(32) +__vlv_write(64) __hsw_write(8) __hsw_write(16) __hsw_write(32) @@ -1031,6 +1052,7 @@ __gen6_write(64) #undef __gen9_write #undef __chv_write #undef __gen8_write +#undef __vlv_write #undef __hsw_write #undef __gen6_write #undef GEN6_WRITE_FOOTER @@ -1243,6 +1265,8 @@ void intel_uncore_init(struct drm_device *dev) case 6: if (IS_HASWELL(dev)) { ASSIGN_WRITE_MMIO_VFUNCS(hsw); + } else if (IS_VALLEYVIEW(dev)) { + ASSIGN_WRITE_MMIO_VFUNCS(vlv); } else { ASSIGN_WRITE_MMIO_VFUNCS(gen6); } -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: Take forcewake on media engine writes
On Thu, Dec 17, 2015 at 05:14:54PM +0200, Mika Kuoppala wrote: > Since commit 940aece471bd ("drm/i915/vlv: Valleyview support > for forcewake Individual power wells.") we have only taken > media engine forcewake correctly on reads, but only taken render > engine forcewake on media engine writes and omitted the media > domain. > > This asymmetry might have caused unstable behaviour on > media ring access. > > Fix is to take media engine forcewake symmetrically to writes. We don't take any forcewake on writes pre-gen8. That's what the wake FIFO is for. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=88012 > Cc: Deepak S> Cc: Jesse Barnes > Cc: Chris Wilson > Signed-off-by: Mika Kuoppala > --- > drivers/gpu/drm/i915/intel_uncore.c | 24 > 1 file changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c > b/drivers/gpu/drm/i915/intel_uncore.c > index 277e60ae0e47..a2e204088aa5 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -902,6 +902,23 @@ hsw_write##x(struct drm_i915_private *dev_priv, > i915_reg_t reg, u##x val, bool t > GEN6_WRITE_FOOTER; \ > } > > +#define __vlv_write(x) \ > +static void \ > +vlv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, > bool trace) { \ > + enum forcewake_domains fw_engine = 0; \ > + GEN6_WRITE_HEADER; \ > + if (!NEEDS_FORCE_WAKE(offset)) \ > + fw_engine = 0; \ > + else if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(offset)) \ > + fw_engine = FORCEWAKE_RENDER; \ > + else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \ > + fw_engine = FORCEWAKE_MEDIA; \ > + if (fw_engine) \ > + __force_wake_get(dev_priv, fw_engine); \ > + __raw_i915_write##x(dev_priv, reg, val); \ > + GEN6_WRITE_FOOTER; \ > +} > + > static const i915_reg_t gen8_shadowed_regs[] = { > FORCEWAKE_MT, > GEN6_RPNSWREQ, > @@ -1019,6 +1036,10 @@ __gen8_write(8) > __gen8_write(16) > __gen8_write(32) > __gen8_write(64) > +__vlv_write(8) > +__vlv_write(16) > +__vlv_write(32) > +__vlv_write(64) > __hsw_write(8) > __hsw_write(16) > __hsw_write(32) > @@ -1031,6 +1052,7 @@ __gen6_write(64) > #undef __gen9_write > #undef __chv_write > #undef __gen8_write > +#undef __vlv_write > #undef __hsw_write > #undef __gen6_write > #undef GEN6_WRITE_FOOTER > @@ -1243,6 +1265,8 @@ void intel_uncore_init(struct drm_device *dev) > case 6: > if (IS_HASWELL(dev)) { > ASSIGN_WRITE_MMIO_VFUNCS(hsw); > + } else if (IS_VALLEYVIEW(dev)) { > + ASSIGN_WRITE_MMIO_VFUNCS(vlv); > } else { > ASSIGN_WRITE_MMIO_VFUNCS(gen6); > } > -- > 2.5.0 > > ___ > 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
Re: [Intel-gfx] [PATCH] drm/i915/vlv: Take forcewake on media engine writes
On Thu, Dec 17, 2015 at 05:39:39PM +0200, Ville Syrjälä wrote: > On Thu, Dec 17, 2015 at 05:14:54PM +0200, Mika Kuoppala wrote: > > Since commit 940aece471bd ("drm/i915/vlv: Valleyview support > > for forcewake Individual power wells.") we have only taken > > media engine forcewake correctly on reads, but only taken render > > engine forcewake on media engine writes and omitted the media > > domain. > > > > This asymmetry might have caused unstable behaviour on > > media ring access. > > > > Fix is to take media engine forcewake symmetrically to writes. > > We don't take any forcewake on writes pre-gen8. That's what the > wake FIFO is for. On the other hand, if something magically starts working, I think we can conclude that our FIFO handling or the FIFO is broken. One or the other. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: Take forcewake on media engine writes
On 12/17/2015 07:14 AM, Mika Kuoppala wrote: > Since commit 940aece471bd ("drm/i915/vlv: Valleyview support > for forcewake Individual power wells.") we have only taken > media engine forcewake correctly on reads, but only taken render > engine forcewake on media engine writes and omitted the media > domain. > > This asymmetry might have caused unstable behaviour on > media ring access. > > Fix is to take media engine forcewake symmetrically to writes. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=88012 > Cc: Deepak S> Cc: Jesse Barnes > Cc: Chris Wilson > Signed-off-by: Mika Kuoppala > --- > drivers/gpu/drm/i915/intel_uncore.c | 24 > 1 file changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c > b/drivers/gpu/drm/i915/intel_uncore.c > index 277e60ae0e47..a2e204088aa5 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -902,6 +902,23 @@ hsw_write##x(struct drm_i915_private *dev_priv, > i915_reg_t reg, u##x val, bool t > GEN6_WRITE_FOOTER; \ > } > > +#define __vlv_write(x) \ > +static void \ > +vlv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, > bool trace) { \ > + enum forcewake_domains fw_engine = 0; \ > + GEN6_WRITE_HEADER; \ > + if (!NEEDS_FORCE_WAKE(offset)) \ > + fw_engine = 0; \ > + else if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(offset)) \ > + fw_engine = FORCEWAKE_RENDER; \ > + else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \ > + fw_engine = FORCEWAKE_MEDIA; \ > + if (fw_engine) \ > + __force_wake_get(dev_priv, fw_engine); \ > + __raw_i915_write##x(dev_priv, reg, val); \ > + GEN6_WRITE_FOOTER; \ > +} > + > static const i915_reg_t gen8_shadowed_regs[] = { > FORCEWAKE_MT, > GEN6_RPNSWREQ, > @@ -1019,6 +1036,10 @@ __gen8_write(8) > __gen8_write(16) > __gen8_write(32) > __gen8_write(64) > +__vlv_write(8) > +__vlv_write(16) > +__vlv_write(32) > +__vlv_write(64) > __hsw_write(8) > __hsw_write(16) > __hsw_write(32) > @@ -1031,6 +1052,7 @@ __gen6_write(64) > #undef __gen9_write > #undef __chv_write > #undef __gen8_write > +#undef __vlv_write > #undef __hsw_write > #undef __gen6_write > #undef GEN6_WRITE_FOOTER > @@ -1243,6 +1265,8 @@ void intel_uncore_init(struct drm_device *dev) > case 6: > if (IS_HASWELL(dev)) { > ASSIGN_WRITE_MMIO_VFUNCS(hsw); > + } else if (IS_VALLEYVIEW(dev)) { > + ASSIGN_WRITE_MMIO_VFUNCS(vlv); > } else { > ASSIGN_WRITE_MMIO_VFUNCS(gen6); > } > Looks good. Looks like we also have it on chv, so I guess it was just an oversight. Reviewed-by: Jesse Barnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx