Re: [PATCH 2/2] drm/i915/bdw: Export workaround data to debugfs

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

On 30/08/2014 16:10, Damien Lespiau wrote:
> On Tue, Aug 26, 2014 at 02:44:51PM +0100, Arun Siluvery wrote:
>> The workarounds that are applied are exported to a debugfs file;
>> this is used to verify their state after the test case (reset or
>> suspend/resume etc). This patch is only required to support i-g-t.
>
> I'm really, really confused. Please bear with me.
I have reworked all the patches hopefully things will be more clear this 
time :)

>
> 1. We only deal with masked registers AFAICS. Those registers have the
> high 16 bits masking the writes.
>
> 2. The values given to intel_ring_emit_wa() are the actual values we're going
> to write in the register, so they include those mask bits. say:
>
>      intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2,
>                         _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
>
> 3. We then record in intel_wa_regs the reg address and two fields named mask
> and value.
>
> 3. a) mask
>
>    intel_wa_regs[dev_priv->num_wa_regs].mask = value & 0xFFFF;
>
> You're selecting the low 16bits and put it in mask. But the masked bits are the
> upper 16bits? This may work when the W/A is about setting bits, but we have a
> bug if we ever have a W/A that is about clearing a bit. It would seem better to
> me to grab the upper bits which are, after all, the bitmask we're interested in.
>
it is a valid issue, changed to use upper 16-bits as mask.

> 3. b) value
>
>     /* value is updated with the status of remaining bits of this
>      * register when it is read from debugfs file
>      */
>    dev_priv->intel_wa_regs[dev_priv->num_wa_regs].value = value;
>
> I don't understand what the comment explains. The *why* we need to do that is
> missing and, frankly, having to update the reference values we capture at
> intel_ring_emit_wa() time sounds like a bug to me.
>
> I also take a note that, at this, point, intel_wa_regs.value contains both the
> value and the mask. Weird.
>
I agree the why part is missing.
The idea is value represents the status of other bits in this register 
besides w/a bit; this is actually redundant here, I guess I added 
because I wanted to initialize all members.
This change is not applicable in the new patches.

> 4. Time to expose that intel_wa_regs array to user space
>
> 4. a) mask
>
>     mask = dev_priv->intel_wa_regs[i].mask
>
> Straigthforward enough, except that these are still the lower 16 bits, so the
> value really.
>
> 4. b) value
>
>    dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask;
>
> Hum? This really started my journey to dig futher. So we:
>
>    - override the reference value from intel_ring_emit_wa() with whatever we
>      have in the register at that moment
>
>    - Or it with a mask that's not really a mask (but the reference value)
>
> 5. igt test
>
>    So you grab those mask and value fields from the debugs file and read the
> register through mapped MMIO. and then
>
>     status = (current_wa[i].value & current_wa[i].mask) !=
>              (wa_regs[i].value & wa_regs[i].mask);
>
> So that's where I'm starting to put things back together and understand what
> the intention is. I still think that's not quite right, especially how we get
> the mask and why we read back the register in the debugfs file.
>
We read the register value after the test case (eg reset) and compare it 
with a known value that is exported to debugfs file.

regards
Arun

> Or am I just missing something? In any case, having to spend that much time
> trying to understand what's going on is a maintainability problem, we need code
> that a least looks straightforward.
>
> HTH,
>