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

[Noguchi, Masato]

> 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);
> 
> 
> 
> 
>