[Intel-gfx] [PATCH v2] drm/i915: Rework workaround init functions for BDW and CHV

[Damien Lespiau]

On Tue, Sep 02, 2014 at 10:14:17AM +0100, Arun Siluvery wrote:
> This rework is based on suggestion from Chris.
> Now w/a are organized in an array and all of them are emitted in
> single fn instead of sending them individually. This approach is
> clean and new w/a can be added with minimal changes.
> The same array can be used when exporting them to debugfs and the
> temporary array in the current implementation is not required.
> 
> v2: As suggested by Damien a new w/a reg definition is added.
> This helps in initializing w/a that are unmasked registers.
> 
> Signed-off-by: Arun Siluvery <arun.siluvery at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 158 ++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  26 ++++++
>  2 files changed, 93 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c5e4dc7..3ed0ad5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -650,87 +650,80 @@ err:
>  	return ret;
>  }
>  
> -static inline void intel_ring_emit_wa(struct intel_engine_cs *ring,
> -				       u32 addr, u32 value)
> +static int i915_request_emit_lri(struct intel_engine_cs *ring,
> +				 int num_registers,
> +				 const struct intel_wa_reg *lri_list)
>  {

[...]

> +	for (i = 0; i < num_registers; ++i) {
> +		u32 value;
> +		const struct intel_wa_reg *p = (lri_list + i);
>
> +		if (p->masked_register)
> +			value = (p->mask << 16) | p->value;
> +		else
> +			value = (I915_READ(p->addr) & ~p->mask) | p->value;

There's one case when this won't work, when several WAs for a single
'normal' register are defined. The read done here means only the last of
those W/As will end up being applied (because the last LRI to that
register will be the value that ends up in the register. We'll probably
need to coalesce all W/A defined for a single normal register into one
write.

Considering that, and the comment about the define, maybe the simplest
way to go forward with the patch is to forget the non-masked case for
the moment, especially as it's not used in this patch.

> +#define INIT_MASKED_WA(_addr, _value) \
> +	INIT_WA_REG(1, _addr, ((_value) >> 16), ((_value) & 0xFFFF))

Those outer '()' look unnecessary

> +#define INIT_UNMASKED_WA(_addr, _value) \
> +	INIT_WA_REG(0, _addr, ~(_value), _value)
> +

~value is not a mask. Consider values with one or more bits defined to 0.

-- 
Damien