[Intel-gfx] About CTX_CONTEXT_CONTROL initialization in populate_lr_context() intel_lrc.c

[Wang, Zhi A]

Hi Daniel & Gordon & Thomas:
    Just found that, my bad. Thanks for giving so much introduction for me, a beginner. :)  Let me cook a patch for it. 
   BTW: CTXT_SR_CTRL register looks like a register needs mask. Does the bit 0 restore inhibit also needs _MASKED_BIT_ENABLE here? It looks like bit 0 is not set with _MASKED_BIT_ENABLE().

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, February 11, 2015 6:19 AM
To: Gordon, David S
Cc: Wang, Zhi A; Daniel, Thomas; Intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] About CTX_CONTEXT_CONTROL initialization in populate_lr_context() intel_lrc.c

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