[Intel-gfx] [PATCH] drm/i915: set initialised only when init_context callback is NULL

[Dong, Chuanxiao]

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Friday, May 12, 2017 6:11 PM
> To: Dong, Chuanxiao <chuanxiao.dong at intel.com>; intel-
> gfx at lists.freedesktop.org; intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: set initialised only when
> init_context callback is NULL
> 
> On Fri, May 12, 2017 at 10:49:24AM +0100, Chris Wilson wrote:
> > On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote:
> > > initialised is fixup by the GVT shadow context as true to avoid the
> > > init from the host because it cannot take the settings from the
> > > host. Add a check to let host driver only overwrite it when the init
> > > callback is NULL
> >
> > During execlist_context_deferred_alloc() we presumed that the context
> > is uninitialised (we only just allocated the state object for it!) and
> > chose to optimise away the later call to engine->init_context() if
> > engine->init_context were NULL. This breaks with GVT's contexts that
> > engine->are
> > marked as pre-initialised to avoid us annoyingly calling
> > engine->init_context(). The fix is to not override ce->initialised if
> > engine->it
> > is already true.

Thanks Chris, will use this as commit message

> >
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > index 319d9a8..d0e9b61 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -1956,7 +1956,8 @@ static int
> > > execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> > >
> > >  	ce->ring = ring;
> > >  	ce->state = vma;
> > > -	ce->initialised = engine->init_context == NULL;
> > > +	if (!engine->init_context)
> > > +		ce->initialised = true;
> >
> > Does the compiler generate a cmov?

Just get the assembly code of this change, seems no cmov.
if (!engine->init_context)
ffffffff8156b6f3:       49 83 bf c0 01 00 00    cmpq   $0x0,0x1c0(%r15)
ffffffff8156b6fa:       00
ffffffff8156b6fb:       0f 84 e5 01 00 00       je     ffffffff8156b8e6 <execlists_context_pin+0x486>
.........
        ce->initialised = true;
ffffffff8156b8e6:       4d 6b ed 28             imul   $0x28,%r13,%r13
ffffffff8156b8ea:       43 c6 44 2e 7c 01       movb   $0x1,0x7c(%r14,%r13,1)
ffffffff8156b8f0:       e9 0c fe ff ff          jmpq   ffffffff8156b701 <execlists_context_pin+0x2a1>

> 
> Test and jump, may as well just use ce->initialised |= init_context == NULL; -
> Chris

This is the new assembly code for this new change:

         ce->initialised |= engine->init_context == NULL;
ffffffff8156b6f3:       49 83 bf c0 01 00 00    cmpq   $0x0,0x1c0(%r15)
ffffffff8156b6fa:       00
ffffffff8156b6fb:       0f 94 c2                sete   %dl
ffffffff8156b6fe:       08 50 7c                or     %dl,0x7c(%rax)

looks your change is better because the compiler doesn't generate a jump. :)

Will send out v2 with your idea.

Thanks
Chuanxiao