Re: [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs

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

On Mon, Sep 01, 2014 at 03:45:29PM +0100, Siluvery, Arun wrote:
> >>+		/*
> >>+		 * 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.

I don't think we can use mask == 0 when it's an unmasked register. In
this case, we still need to be able to have a mask that tells us which
are the interesting bits in the value. If we want something generic that
can be applied to masked and unmasked register, we'll something like:

struct reg_wa {
	unsigned int masked_register : 1;
	u32 addr;
	u32 mask;
	u32 value;
};

So:

1. masked register case:

  - masked_register = 1
  - addr
  - mask (selects the lower 16 bits that are coding for the W/A value)
  - value (it only makes sense to some of the lower 16 bits set here)

2. 'normal' register case

  - masked_register = 0
  - addr
  - mask
  - value

>From that generic description you can cover all cases, eg.

1. the write for masked registers becomes: mask << 16 | value
2. the write for normal registers becomes: (read(addr) & ~mask) | value
2. check a W/A has been applied (both normal and masked register):
	read(addr) & mask == value

We seem to have only masked registers atm, so it's fine to handle just
that case, but if you envision that we'll need more than masked
registers, we need a better W/A definition.
  
-- 
Damien