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