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

[Daniel Vetter <daniel@ffwll.ch>]

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