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

[Chris Wilson]

On Tue, Sep 02, 2014 at 10:45:45AM +0100, Damien Lespiau wrote:
> 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.

If you are still considering an in-kernel table, despite that it can be
done in userspace, design it to track all w/a and not just the ring
specific fixups.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre