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