[Intel-gfx] [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs

[Siluvery, Arun]

On 01/09/2014 15:06, Damien Lespiau wrote:
> On Mon, Sep 01, 2014 at 02:28:53PM +0100, Arun Siluvery wrote:
>> Now w/a are organized in an array so we know exactly how many of them
>> are applied; use the same array while exporting data to debugfs and
>> remove the temporary array we currently have in driver priv structure.
>>
>> Signed-off-by: Arun Siluvery <arun.siluvery at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c     | 41 +++++++++++++++++++++++----------
>>   drivers/gpu/drm/i915/i915_drv.h         | 14 -----------
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 15 ++++++++++++
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  8 +++++++
>>   4 files changed, 52 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 2727bda..bab0408 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2465,6 +2465,14 @@ static int i915_wa_registers(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;
>> +	struct intel_ring_context_rodata ro_data;
>> +
>> +	ret = ring_context_rodata(dev, &ro_data);
>> +	if (ret) {
>> +		seq_printf(m, "Workarounds applied: 0\n");
>
> seq_puts()
>
>> +		DRM_DEBUG_DRIVER("Workaround table not available !!\n");
>> +		return -EINVAL;
>> +	}
>>
>>   	ret = mutex_lock_interruptible(&dev->struct_mutex);
>>   	if (ret)
>> @@ -2472,18 +2480,27 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>>
>>   	intel_runtime_pm_get(dev_priv);
>>
>> -	seq_printf(m, "Workarounds applied: %d\n", dev_priv->num_wa_regs);
>> -	for (i = 0; i < dev_priv->num_wa_regs; ++i) {
>> -		u32 addr, mask;
>> -
>> -		addr = dev_priv->intel_wa_regs[i].addr;
>> -		mask = dev_priv->intel_wa_regs[i].mask;
>> -		dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask;
>> -		if (dev_priv->intel_wa_regs[i].addr)
>> -			seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
>> -				   dev_priv->intel_wa_regs[i].addr,
>> -				   dev_priv->intel_wa_regs[i].value,
>> -				   dev_priv->intel_wa_regs[i].mask);
>> +	seq_printf(m, "Workarounds applied: %d\n", ro_data.num_items/2);
>> +	for (i = 0; i < ro_data.num_items; i += 2) {
>> +		u32 addr, mask, value;
>> +
>> +		addr = ro_data.init_context[i];
>> +		/*
>> +		 * Most of workarounds are  masked registers;
>> +		 * to set a bit in lower 16-bits we set a mask bit in
>> +		 * upper 16-bits so we can take either of them as mask but
>> +		 * it doesn't work if the w/a is about clearing a bit so
>> +		 * use upper 16-bits to cover both cases.
>> +		 */
>> +		mask = ro_data.init_context[i+1] >> 16;
>
> "Most" doesn't seem good here. Either it's "all" and we're happy, or we
> need a generic way to describe the W/A (masked register or not). value +
> mask is generic enough to code for both cases.
>
It seems some of them could be unmasked registers.
We can use 'mask' itself to determine whether it is a masked/unmasked 
register. mask == 0 if it is an unmasked register.
>> +
>> +		/*
>> +		 * value represents the status of other bits in the
>> +		 * register besides w/a bits
>> +		 */
>> +		value  = I915_READ(addr) | mask;
>> +		seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
>> +			   addr, value, mask);
>>   	}
>
> I still don't get it. 'value' is supposed to be the reference value for
> the W/A, but you're or'ing the mask here, so you treat the mask as if it
> were the reference value. This won't work if the W/A is about setting
> multi-bits fields or about clearing a bit.
>
> The comment is still not clear enough. You're saying "other bits besides
> the w/a bits", but or'ing the mask doesn't do that.
>
> Why do we care about the "other bits" in the reference value? they don't
> matter. Why use something else than (ro_data.init_context[i+1] & 0xffff)
> for the value here (as long we're talking about masked registers)?
>
I have always considered value as the register value (remaining bits of 
the register and w/a bits) and now I see your point.
Yes lower 16-bits can be used as reference value, depending on whether 
it is a masked/unmasked we can use/not use the mask in conjunction with 
value in the test.

regards
Arun