Re: [PATCH] drm/i915: Emit even number of dwords when emitting LRIs

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 23, 2014 at 05:42:47PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Thu, Oct 23, 2014 at 01:42:38PM +0100, Damien Lespiau wrote:
> >> On Thu, Oct 23, 2014 at 02:21:02PM +0200, Daniel Vetter wrote:
> >> > On Wed, Oct 22, 2014 at 06:59:52PM +0100, Arun Siluvery wrote:
> >> > > The number of DWords should be even when doing ring emits as
> >> > > command sequences require QWord alignment.
> >> > > 
> >> > > v2: user LRI variant that can write multiple regs in one go (Damien).
> >> > > We can simply insert one NOP at the end instead of one per register write.
> >> > > 
> >> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >> > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++--
> >> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> >> > > 
> >> > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> > > index 497b836..a8f72e8 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> > > @@ -680,15 +680,16 @@ static int intel_ring_workarounds_emit(struct intel_engine_cs *ring)
> >> > >  	if (ret)
> >> > >  		return ret;
> >> > >  
> >> > > -	ret = intel_ring_begin(ring, w->count * 3);
> >> > > +	ret = intel_ring_begin(ring, (w->count * 2 + 2));
> >> > >  	if (ret)
> >> > >  		return ret;
> >> > >  
> >> > > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(w->count));
> >> > 
> >> > Afaik there's a limit to the size of an MI_LRI. Where's the check for
> >> > that (probably with a WARN_ON for now to avoid unecessary complexity)?
> >> 
> >> I guess there's always the size of the length field, I don't see any
> >> other indication. Note that I can find the documentation of the
> >> multi-registers version of LRI either. So, well, we probably should
> >> double check it does work.
> >
> > It does work. The max is around 60 iirc (the max length of the
> > command).
> > -Chris
> >
> 
> I did some test with bdw:
> 
> The maximum is 128 writes, resulting the 8 bit length 
> field of the command being 0xff, thus following the spec.
> The 128'th write went through.
> 
> Perhaps the max command length is then less in older gens?
> 
> Perhaps WARN_ON(x > 128) in MI_LOAD_REGISTER_IMM would be in place
> but one needs minor tweak to command parser a bit also then.
> 
> #define I915_MAX_WA_REGS 16
> 
> keeps us safe for now atleast.

Ok, that's good enough I think. I've summarized the discussion a bit in
the commit message and merged the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch