> -----Original Message----- > From: Chris Wilson [mailto:chris@xxxxxxxxxxxxxxxxxx] > Sent: Friday, May 12, 2017 6:11 PM > To: Dong, Chuanxiao <chuanxiao.dong@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxxxxxx> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@xxxxxxxxx> > > > --- > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
