[Cbe-oss-dev] [PATCH 7/9] spufs: limit saving MFC_CNTL bits

[장현승]

OK. I see your point :)


HS Jang.

> -----Original Message-----
> From: Noguchi, Masato [mailto:Masato.Noguchi at jp.sony.com] 
> Sent: Thursday, August 23, 2007 5:02 PM
> To: hs8848.jang at samsung.com
> Cc: cbe-oss-dev at ozlabs.org
> Subject: RE: Re: [Cbe-oss-dev] [PATCH 7/9] spufs: limit 
> saving MFC_CNTL bits
> 
> 
> > From: HYEONSEUNG JANG [mailto:hs8848.jang at samsung.com]
> > Subject: Re: Re: [Cbe-oss-dev] [PATCH 7/9] spufs: limit saving
> MFC_CNTL bits
> > 
> > It's good but I personally think that it's better to save or modify
> CSA
> > MFC_CNTL only inside the save_mfc_cntl() in some way for code
> maintenance.
> 
> Actually, save_mfc_cntl() (Save step 8) is performed before stopping 
> SPU execution (save_spu_status(): Save step 11). Thus, SPU 
> may issue new DMA command and MFC_CNTL[Q] bit may be changed 
> from "empty" to "not empty" between save_mfc_cntl() and 
> save_mfc_queues()(Save step 19).
> 
> Also, MFC_CNTL[Dh] is saved at save_mfc_decr() (Save step 
> 12), because of same reason.
> 
> 
> M.Noguchi
> 
> 
> 
> 
> > HS Jang.
> > 
> > ------- Original Message -------
> > Sender : Masato Noguchi<Masato.Noguchi at jp.sony.com>
> > Date   : Aug 23, 2007 15:12
> > Title  : Re: [Cbe-oss-dev] [PATCH 7/9] spufs: limit saving MFC_CNTL
> bits
> > 
> > 
> > "HYEONSEUNG JANG" <hs8848.jang at samsung.com> wrote: 
> (2007/08/23 12:24)
> > 
> > >
> > >After applying the patch, It can causes the following behavior :
> > >
> > >  In the saving step, save_mfc_cntl() always sets CSA 
> MFC_CNTL[Q] to
> be '0',
> > >  which means 'not empty' *ignoring current SPU MFC_CNTL[Q]*.
> > >  save_mfc_queues() checks current SPU MFC_CNTL[Q] to 
> decide whether
> to
> > >  save MFC queue context or not.
> > >
> > >  In the restoring step, restore_mfc_queues() checks CSA 
> MFC_CNTL[Q]
> before
> > >  restoring MFC queue context.
> > >
> > >  For example, when current SPU MFC_CNTL[Q] is 1, 'empty', in the
> saving
> > >  step. it ends up restoring old, _garbage_, MFC queue in
> > >  restore_mfc_queues() because CSA MFC_CNTL[Q] is always 
> 'not empty'
> and
> > >  it can lead to wrong DMA being initiated after restoring step.
> > >
> > 
> > Surely. That's a problem.
> > Thank you for pointing out it.
> > 
> > >
> > >I think we must save at least MFC_CNTL_DMA_QUEUES_EMPTY field like
> the
> > following.
> > >( note that below patch should be applied after the original patch)
> > >
> > >
> > >diff --git a/arch/powerpc/platforms/cell/spufs/switch.c
> > b/arch/powerpc/platforms/cell/spufs/switch.c
> > >index 27ffdae..d222355 100644
> > >--- a/arch/powerpc/platforms/cell/spufs/switch.c
> > >+++ b/arch/powerpc/platforms/cell/spufs/switch.c
> > >@@ -194,6 +194,8 @@ static inline void 
> save_mfc_cntl(struct spu_state
> *csa,
> > struct spu *spu)
> > >                }
> > >                break;
> > >        }
> > >+
> > >+       csa->priv2.mfc_control_RW |= 
> in_be64(&priv2->mfc_control_RW)
> &
> > MFC_CNTL_DMA_QUEUES_EMPTY_MASK;
> > > }
> > >
> > > static inline void save_spu_runcntl(struct spu_state *csa, struct
> spu *spu)
> > >
> > >
> > 
> > 
> > Um, I feel MFC_CNTL[Q] in a CSA should indicate wheter a 
> DMA queue was 
> > saved to a CSA or not. How about this?
> > 
> > Signed-off-by: Masato Noguchi <Masato.Noguchi at jp.sony.com>
> > ---
> > diff --git a/arch/powerpc/platforms/cell/spufs/switch.c
> > b/arch/powerpc/platforms/cell/spufs/switch.c
> > index 27ffdae..4eea86f 100644
> > --- a/arch/powerpc/platforms/cell/spufs/switch.c
> > +++ b/arch/powerpc/platforms/cell/spufs/switch.c
> > @@ -329,10 +329,13 @@ static inline void save_mfc_queues(struct
> spu_state *csa,
> > struct spu *spu)
> >  	int i;
> > 
> >  	/* Save, Step 19:
> > -	 *     If MFC_Cntl[Se]=0 then save
> > +	 *     Update saved copy of CSA.MFC_CNTL[Q], and
> > +	 *     if MFC_Cntl[Q]=0 then save
> >  	 *     MFC command queues.
> >  	 */
> > -	if ((in_be64(&priv2->mfc_control_RW) &
> MFC_CNTL_DMA_QUEUES_EMPTY)
> > == 0) {
> > +	if (in_be64(&priv2->mfc_control_RW) & MFC_CNTL_DMA_QUEUES_EMPTY)
> {
> > +		csa->priv2.mfc_control_RW |= MFC_CNTL_DMA_QUEUES_EMPTY;
> > +	} else {
> >  		for (i = 0; i < 8; i++) {
> >  			csa->priv2.puq[i].mfc_cq_data0_RW =
> >  			    in_be64(&priv2->puq[i].mfc_cq_data0_RW);
> > 
> > 
> > 
> > 
> > 
> 
> 
> 
>