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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
