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

[HYEONSEUNG JANG]

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.


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