[Cbe-oss-dev] [PATCH] spufs: Avoid restarting MFC in context saving

[Kazunori Asayama]

Jeremy Kerr <jk at ozlabs.org> wrote:
> Asayama-san,
> 
> Nice catch!
> 
> > The current SPU context saving procedure in SPUFS unexpectedly
> > restarts MFC when halting decrementer, because MFC_CNTL[Dh] is set
> > without MFC_CNTL[Sm].
> 
> >From reading the CBEA docs, the Dh bit is independent from the Sm bit, 
> so I think we should keep them separate, rather than setting both in 
> halt_mfc_decr(). Perhaps this bit should be set in suspend_mfc(). Or we 
> could just set it in init_priv() and leave it set.

I think that you misunderstand the meaning of Sc bit (and Sm
bit). Writing "Sc == 0" does *NOT* mean "Suspend MFC command queue
operation is not requested", but means "Request normal MFC command
queue operation".

So writing "Sc == 0" with "Sm == 0" to MFC control register causes
restarting MFC regardless of other bits of the register, and we must
always set Sm bit when we do *NOT* want to request "Normal MFC command
queue operation".

Therefore:

  - we must set both of Dh and Sm in halt_mfc_decr() to ignore "Sc == 0".
  - we must *NOT* set Sm bit in suspend_mfc().

See the beginning of the section "15.10 MFC Control Register" in CBEA.

> 
> > This bug causes, for example, saving broken DMA  queues.
> 
> Could you elaborate on what you've seen happen? I assume the DMA queues 
> have continued to be processed during the context switch?

Yes, right.

> 
> Do you have a testcase that I can use to confirm the fix?

Yes, I have. I'll submit it to this ML later.

> 
> > +#define MFC_CNTL_SUSPEND_DMA_QUEUE_DISABLED	(1ull << 4)
> 
> The docs call this a 'Suspend Mask' (setting this bit doesn't 
> disbale/suspend the queue, it's the mask for the 0x1 bit). How about 
> MFC_CNTL_SUSPEND_MASK?

Sounds good.

> 
> Cheers,
> 
> 
> Jeremy

--
(ASAYAMA Kazunori
  (asayama at sm.sony.co.jp))
t