Re: [RFC] drm/i915/bdw: Initialize BDW workarounds in render ring init fn

Daniel Vetter
2014-07-28T19:22:08Z

On Mon, Jul 28, 2014 at 08:00:39PM +0300, Ville Syrjälä wrote:
> On Mon, Jul 28, 2014 at 05:31:46PM +0100, arun.siluvery@linux.intel.com wrote:
> > From: Arun Siluvery <arun.siluvery@linux.intel.com>
> >
> > The workarounds at the moment are initialized in init_clock_gating() but
> > they are lost during reset; In case of execlists some workarounds modify
> > registers that are part of register state context, since these are not
> > initialized until init_clock_gating() default context ends up with
> > incorrect values as render context is restored and saved before updated
> > by workarounds hence move them to render ring init fn. This should be
> > ok as these workarounds are not related to display clock gating.
> >
> > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 46 ++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_pm.c | 59 ----------------------------
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 68 +++++++++++++++++++++++++++++++++
> > 3 files changed, 114 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 083683c..cf7da30 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2397,20 +2397,65 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
> > seq_printf(m, " dpll_md: 0x%08x\n", pll->hw_state.dpll_md);
> > seq_printf(m, " fp0: 0x%08x\n", pll->hw_state.fp0);
> > seq_printf(m, " fp1: 0x%08x\n", pll->hw_state.fp1);
> > seq_printf(m, " wrpll: 0x%08x\n", pll->hw_state.wrpll);
> > }
> > drm_modeset_unlock_all(dev);
> >
> > return 0;
> > }
> >
> > +static int i915_workaround_info(struct seq_file *m, void *unused)
> > +{
> > + struct drm_info_node *node = (struct drm_info_node *) m->private;
> > + struct drm_device *dev = node->minor->dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + int ret;
> > +
> > + ret = mutex_lock_interruptible(&dev->struct_mutex);
> > + if (ret)
> > + return ret;
> > +
> > + if (IS_BROADWELL(dev)) {
> > + seq_printf(m, "GEN8_ROW_CHICKEN:\t0x%08x\n",
> > + I915_READ(GEN8_ROW_CHICKEN));
> > + seq_printf(m, "HALF_SLICE_CHICKEN3:\t0x%08x\n",
> > + I915_READ(HALF_SLICE_CHICKEN3));
> > + seq_printf(m, "GAMTARBMODE:\t0x%08x\n", I915_READ(GAMTARBMODE));
> > + seq_printf(m, "_3D_CHICKEN3:\t0x%08x\n",
> > + I915_READ(_3D_CHICKEN3));
> > + seq_printf(m, "COMMON_SLICE_CHICKEN2:\t0x%08x\n",
> > + I915_READ(COMMON_SLICE_CHICKEN2));
> > + seq_printf(m, "GEN7_HALF_SLICE_CHICKEN1:\t0x%08x\n",
> > + I915_READ(GEN7_HALF_SLICE_CHICKEN1));
> > + seq_printf(m, "GEN7_ROW_CHICKEN2:\t0x%08x\n",
> > + I915_READ(GEN7_ROW_CHICKEN2));
> > + seq_printf(m, "GAM_ECOCHK:\t0x%08x\n",
> > + I915_READ(GAM_ECOCHK));
> > + seq_printf(m, "HDC_CHICKEN0:\t0x%08x\n",
> > + I915_READ(HDC_CHICKEN0));
> > + seq_printf(m, "GEN7_FF_THREAD_MODE:\t0x%08x\n",
> > + I915_READ(GEN7_FF_THREAD_MODE));
> > + seq_printf(m, "GEN8_UCGCTL6:\t0x%08x\n",
> > + I915_READ(GEN8_UCGCTL6));
> > + seq_printf(m, "GEN6_RC_SLEEP_PSMI_CONTROL:\t0x%08x\n",
> > + I915_READ(GEN6_RC_SLEEP_PSMI_CONTROL));
> > + seq_printf(m, "CACHE_MODE_1:\t0x%08x\n",
> > + I915_READ(CACHE_MODE_1));
> > + } else
> > + DRM_DEBUG_DRIVER("Not available for Gen%d\n",
> > + INTEL_INFO(dev)->gen);
> > +
> > + mutex_unlock(&dev->struct_mutex);
> > + return 0;
> > +}
> > +
>
> This smells like a separate patch. But I'm not sure we want at all since
> intel_reg_read will provide the same information.

Yeah, debugfs files that just do what intel_reg_read does are just an
additional maintaince burden. I know that we have a few that dump lots of
registers, but most of them dump a lot of other information, too.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch