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

["Siluvery, Arun" <arun.siluvery@linux.intel.com>]

On 28/07/2014 20:22, Daniel Vetter wrote:
> 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
>

I've added this mainly for testing workarounds which can be extended 
further as we move WAs for other chipsets but I agree it can be done 
with intel_reg_read.

regards
Arun