Re: About CTX_CONTEXT_CONTROL initialization in populate_lr_context() intel_lrc.c

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: "Intel-gfx@lists.freedesktop.org" <Intel-gfx@lists.freedesktop.org>
Subject: Re: About CTX_CONTEXT_CONTROL initialization in populate_lr_context() intel_lrc.c
Date: Tue, 10 Feb 2015 23:18:47 +0100	[thread overview]
Message-ID: <20150210221847.GO24485@phenom.ffwll.local> (raw)
In-Reply-To: <54DA30F0.1040707@intel.com>

On Tue, Feb 10, 2015 at 04:25:20PM +0000, Dave Gordon wrote:
> On 09/02/15 15:05, Wang, Zhi A wrote:
> > Hi Gurus:
> > Forgive my junior HW knowledge, I just found that in execlist
> > context initialization function populate_lr_context(), this line:
> >   
> >           reg_state[CTX_CONTEXT_CONTROL+1] =
> >                           _MASKED_BIT_ENABLE((1<<3) | MI_RESTORE_INHIBIT);
> >   
> > It seems the "Inhibit Synchronous Context Switch" bit is not set
> > here, so when HW is trying to wait some events, e.g. semaphore, according to
> > Bspec, per my basic understanding, it will switch out current context
> > with some reason bit set. Here comes my question, I think if this
> > situation happen, should i915 remember this context and try to
> > re-schedule it in a proper time, e.g. before submitting a new context
> > until the context_completed bit in CSB is set? I don't find a register
> > that will remember the context switched out by waiting event, so I think
> > it should be i915 to handle this situation or just set "Inhibit
> > Synchronous Context Switch" bit here?...
> 
> But that's exactly what it does already ... it sets bit 3, which is the
> "Inhibit Synchronous Context Switch" bit you refer to.
> 
> What's wrong here is that it should use a #define for this bit, and for
> the "Restore Inhibit" bit -- for which it's actually using a value
> defined in a completely different context (no pun intended), namely the
> bits in a MI_SET_CONTEXT *instruction*. It just happens to work because
> they have the same numerical value. So, what we need is:
> 
> <intel_lrc.h>
> 
> #define RING_CONTEXT_CONTROL(ring)   	   ((ring)->mmio_base+0x244)
> #define	  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH	(1 << 3)
> #define	  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT	(1 << 0)
> 
> and then change the line you pointed out to use the correct symbolic names.

Can I have this in patch form please? I.e. convert to diff, paste the
above into commit message and add sob line.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx