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

[Damien Lespiau <damien.lespiau@intel.com>]

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.

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.

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.

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.

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,

-- 
Damien