diff mbox

[B,1/3] tidspbridge: don't flood the mailbox on MemUnMap

Message ID 1237339605-20697-2-git-send-email-felipe.contreras@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Felipe Contreras March 18, 2009, 1:26 a.m. UTC 
From: Felipe Contreras <felipe.contreras@nokia.com>

Impact: improves performance and reliability

Check the hardware state of the DSP before sending the command to wake
it up thus avoiding to flood the mailbox unnecessarily.

By doing that the mailbox is free most of the time which dramatically
decreases CPU usage since the check for empty slots is done in a busy
loop.

Also, a free mailbox means less timeouts, so now most messages are
posted which improves reliability.

MemMap is doing the flush properly, so this patch also refactors the
code into a common flush_all function.

The problem can be triggered by doing many memmap/unmaps, just like TI's
OpenMAX IL implementation.

Signed-off-by: Felipe Contreras <felipe.contreras@nokia.com>
---
 drivers/dsp/bridge/wmd/tiomap3430.c |   42 +++++++++++++++++++---------------
 1 files changed, 23 insertions(+), 19 deletions(-)

Comments

Kanigeri, Hari March 18, 2009, 1:06 p.m. UTC | #1 
Felipe,

> Check the hardware state of the DSP before sending the command to wake
> it up thus avoiding to flood the mailbox unnecessarily.
>
-- So this check was missing in Unmap part. Good catch.


> -	u32 temp = 0;
> -	struct CFG_HOSTRES resources;
>  	u32 *pPhysAddrPageTbl = NULL;
>  	struct vm_area_struct *vma;
>  	struct mm_struct *mm = current->mm;
> +	u32 temp = 0;
>

-- Is temp variable needed ?

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Tuesday, March 17, 2009 8:27 PM
> To: linux-omap@vger.kernel.org
> Cc: Kanigeri, Hari; Hiroshi DOYU; Ameya Palande; Guzman Lugo, Fernando;
> Felipe Contreras
> Subject: [PATCH B 1/3] tidspbridge: don't flood the mailbox on MemUnMap
> 
> From: Felipe Contreras <felipe.contreras@nokia.com>
> 
> Impact: improves performance and reliability
> 
> Check the hardware state of the DSP before sending the command to wake
> it up thus avoiding to flood the mailbox unnecessarily.
> 
> By doing that the mailbox is free most of the time which dramatically
> decreases CPU usage since the check for empty slots is done in a busy
> loop.
> 
> Also, a free mailbox means less timeouts, so now most messages are
> posted which improves reliability.
> 
> MemMap is doing the flush properly, so this patch also refactors the
> code into a common flush_all function.
> 
> The problem can be triggered by doing many memmap/unmaps, just like TI's
> OpenMAX IL implementation.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@nokia.com>
> ---
>  drivers/dsp/bridge/wmd/tiomap3430.c |   42 +++++++++++++++++++-----------
> ----
>  1 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c
> b/drivers/dsp/bridge/wmd/tiomap3430.c
> index c0f4ffc..b538ef7 100644
> --- a/drivers/dsp/bridge/wmd/tiomap3430.c
> +++ b/drivers/dsp/bridge/wmd/tiomap3430.c
> @@ -235,6 +235,25 @@ static struct WMD_DRV_INTERFACE drvInterfaceFxns = {
>  	WMD_MSG_SetQueueId,
>  };
> 
> +static inline void flush_all(struct WMD_DEV_CONTEXT *pDevContext)
> +{
> +	struct CFG_HOSTRES resources;
> +	u32 temp = 0;
> +
> +	CFG_GetHostResources((struct CFG_DEVNODE
> *)DRV_GetFirstDevExtension(), &resources);
> +	HW_PWRST_IVA2RegGet(resources.dwPrmBase, &temp);
> +
> +	if ((temp & HW_PWR_STATE_ON) == HW_PWR_STATE_OFF) {
> +		/* IVA domain is not in ON state*/
> +		DBG_Trace(DBG_LEVEL7, "temp value is 0x%x\n", temp);
> +		CLK_Enable(SERVICESCLK_iva2_ck);
> +		WakeDSP(pDevContext, NULL);
> +		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
> +		CLK_Disable(SERVICESCLK_iva2_ck);
> +	} else
> +		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
> +}
> +
>  /*
>   *  ======== WMD_DRV_Entry ========
>   *  purpose:
> @@ -1283,17 +1302,14 @@ static DSP_STATUS WMD_BRD_MemMap(struct
> WMD_DEV_CONTEXT *hDevContext,
>  	struct WMD_DEV_CONTEXT *pDevContext = hDevContext;
>  	struct HW_MMUMapAttrs_t hwAttrs;
>  	u32 numOfActualTabEntries = 0;
> -	u32 temp = 0;
> -	struct CFG_HOSTRES resources;
>  	u32 *pPhysAddrPageTbl = NULL;
>  	struct vm_area_struct *vma;
>  	struct mm_struct *mm = current->mm;
> +	u32 temp = 0;
> 
>  	DBG_Trace(DBG_ENTER, "> WMD_BRD_MemMap hDevContext %x, pa %x, va %x,
> "
>  		 "size %x, ulMapAttr %x\n", hDevContext, ulMpuAddr,
> ulVirtAddr,
>  		 ulNumBytes, ulMapAttr);
> -	status = CFG_GetHostResources(
> -		 (struct CFG_DEVNODE *)DRV_GetFirstDevExtension(),
> &resources);
>  	if (ulNumBytes == 0)
>  		return DSP_EINVALIDARG;
> 
> @@ -1421,16 +1437,7 @@ func_cont:
>  	 * This is called from here instead from PteUpdate to avoid
> unnecessary
>  	 * repetition while mapping non-contiguous physical regions of a
> virtual
>  	 * region */
> -	HW_PWRST_IVA2RegGet(resources.dwPrmBase, &temp);
> -	if ((temp & HW_PWR_STATE_ON) == HW_PWR_STATE_OFF) {
> -		/* IVA domain is not in ON state*/
> -		DBG_Trace(DBG_LEVEL7, "temp value is 0x%x\n", temp);
> -		CLK_Enable(SERVICESCLK_iva2_ck);
> -		WakeDSP(pDevContext, NULL);
> -		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
> -		CLK_Disable(SERVICESCLK_iva2_ck);
> -	} else
> -		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
> +	flush_all(pDevContext);
>  	DBG_Trace(DBG_ENTER, "< WMD_BRD_MemMap status %x\n", status);
>  	return status;
>  }
> @@ -1571,8 +1578,7 @@ static DSP_STATUS WMD_BRD_MemUnMap(struct
> WMD_DEV_CONTEXT *hDevContext,
>  	 /* It is better to flush the TLB here, so that any stale old
> entries
>  	 * get flushed */
>  EXIT_LOOP:
> -	CHNLSM_InterruptDSP2(pDevContext, MBX_PM_DSPWAKEUP);
> -	HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
> +	flush_all(pDevContext);
>  	DBG_Trace(DBG_LEVEL1, "WMD_BRD_MemUnMap vaCurr %x, pteAddrL1 %x "
>  		  "pteAddrL2 %x\n", vaCurr, pteAddrL1, pteAddrL2);
>  	DBG_Trace(DBG_ENTER, "< WMD_BRD_MemUnMap status %x remBytes %x, "
> @@ -2055,9 +2061,7 @@ func_cont:
>  	 * This is called from here instead from PteUpdate to avoid
> unnecessary
>  	 * repetition while mapping non-contiguous physical regions of a
> virtual
>  	 * region */
> -	/* Waking up DSP before calling TLB Flush */
> -	CHNLSM_InterruptDSP2(pDevContext, MBX_PM_DSPWAKEUP);
> -	HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
> +	flush_all(pDevContext);
>  	DBG_Trace(DBG_LEVEL7, "< WMD_BRD_MemMap  at end status %x\n",
> status);
>  	return status;
>  }
> --
> 1.6.2.1.287.g9a8be
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Contreras March 18, 2009, 1:29 p.m. UTC | #2 
On Wed, Mar 18, 2009 at 3:06 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Felipe,
>
>> Check the hardware state of the DSP before sending the command to wake
>> it up thus avoiding to flood the mailbox unnecessarily.
>>
> -- So this check was missing in Unmap part. Good catch.
>
>
>> -     u32 temp = 0;
>> -     struct CFG_HOSTRES resources;
>>       u32 *pPhysAddrPageTbl = NULL;
>>       struct vm_area_struct *vma;
>>       struct mm_struct *mm = current->mm;
>> +     u32 temp = 0;
>>
>
> -- Is temp variable needed ?

You tell me :)

I just moved code around. I don't truly understand what it's doing.
Maybe the read is needed for some kind of sync?

I've removed the tlb flush all completely and I didn't notice any
problems, is it really needed?
Kanigeri, Hari March 18, 2009, 1:49 p.m. UTC | #3 
> You tell me :)
> 
> I just moved code around. I don't truly understand what it's doing.
> Maybe the read is needed for some kind of sync?
> 
> I've removed the tlb flush all completely and I didn't notice any
> problems, is it really needed?
>

-- Ignore my previous comment. I think temp is used for other purpose in the same function. I thought temp was used by only the part of the code section that you stripped out.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Wednesday, March 18, 2009 8:30 AM
> To: Kanigeri, Hari
> Cc: linux-omap@vger.kernel.org; Hiroshi DOYU; Ameya Palande; Guzman Lugo,
> Fernando; Felipe Contreras
> Subject: Re: [PATCH B 1/3] tidspbridge: don't flood the mailbox on
> MemUnMap
> 
> On Wed, Mar 18, 2009 at 3:06 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > Felipe,
> >
> >> Check the hardware state of the DSP before sending the command to wake
> >> it up thus avoiding to flood the mailbox unnecessarily.
> >>
> > -- So this check was missing in Unmap part. Good catch.
> >
> >
> >> -     u32 temp = 0;
> >> -     struct CFG_HOSTRES resources;
> >>       u32 *pPhysAddrPageTbl = NULL;
> >>       struct vm_area_struct *vma;
> >>       struct mm_struct *mm = current->mm;
> >> +     u32 temp = 0;
> >>
> >
> > -- Is temp variable needed ?
> 
> You tell me :)
> 
> I just moved code around. I don't truly understand what it's doing.
> Maybe the read is needed for some kind of sync?
> 
> I've removed the tlb flush all completely and I didn't notice any
> problems, is it really needed?
> 
> --
> Felipe Contreras

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c b/drivers/dsp/bridge/wmd/tiomap3430.c
index c0f4ffc..b538ef7 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430.c
@@ -235,6 +235,25 @@  static struct WMD_DRV_INTERFACE drvInterfaceFxns = {
 	WMD_MSG_SetQueueId,
 };
 
+static inline void flush_all(struct WMD_DEV_CONTEXT *pDevContext)
+{
+	struct CFG_HOSTRES resources;
+	u32 temp = 0;
+
+	CFG_GetHostResources((struct CFG_DEVNODE *)DRV_GetFirstDevExtension(), &resources);
+	HW_PWRST_IVA2RegGet(resources.dwPrmBase, &temp);
+
+	if ((temp & HW_PWR_STATE_ON) == HW_PWR_STATE_OFF) {
+		/* IVA domain is not in ON state*/
+		DBG_Trace(DBG_LEVEL7, "temp value is 0x%x\n", temp);
+		CLK_Enable(SERVICESCLK_iva2_ck);
+		WakeDSP(pDevContext, NULL);
+		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
+		CLK_Disable(SERVICESCLK_iva2_ck);
+	} else
+		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
+}
+
 /*
  *  ======== WMD_DRV_Entry ========
  *  purpose:
@@ -1283,17 +1302,14 @@  static DSP_STATUS WMD_BRD_MemMap(struct WMD_DEV_CONTEXT *hDevContext,
 	struct WMD_DEV_CONTEXT *pDevContext = hDevContext;
 	struct HW_MMUMapAttrs_t hwAttrs;
 	u32 numOfActualTabEntries = 0;
-	u32 temp = 0;
-	struct CFG_HOSTRES resources;
 	u32 *pPhysAddrPageTbl = NULL;
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
+	u32 temp = 0;
 
 	DBG_Trace(DBG_ENTER, "> WMD_BRD_MemMap hDevContext %x, pa %x, va %x, "
 		 "size %x, ulMapAttr %x\n", hDevContext, ulMpuAddr, ulVirtAddr,
 		 ulNumBytes, ulMapAttr);
-	status = CFG_GetHostResources(
-		 (struct CFG_DEVNODE *)DRV_GetFirstDevExtension(), &resources);
 	if (ulNumBytes == 0)
 		return DSP_EINVALIDARG;
 
@@ -1421,16 +1437,7 @@  func_cont:
 	 * This is called from here instead from PteUpdate to avoid unnecessary
 	 * repetition while mapping non-contiguous physical regions of a virtual
 	 * region */
-	HW_PWRST_IVA2RegGet(resources.dwPrmBase, &temp);
-	if ((temp & HW_PWR_STATE_ON) == HW_PWR_STATE_OFF) {
-		/* IVA domain is not in ON state*/
-		DBG_Trace(DBG_LEVEL7, "temp value is 0x%x\n", temp);
-		CLK_Enable(SERVICESCLK_iva2_ck);
-		WakeDSP(pDevContext, NULL);
-		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
-		CLK_Disable(SERVICESCLK_iva2_ck);
-	} else
-		HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
+	flush_all(pDevContext);
 	DBG_Trace(DBG_ENTER, "< WMD_BRD_MemMap status %x\n", status);
 	return status;
 }
@@ -1571,8 +1578,7 @@  static DSP_STATUS WMD_BRD_MemUnMap(struct WMD_DEV_CONTEXT *hDevContext,
 	 /* It is better to flush the TLB here, so that any stale old entries
 	 * get flushed */
 EXIT_LOOP:
-	CHNLSM_InterruptDSP2(pDevContext, MBX_PM_DSPWAKEUP);
-	HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
+	flush_all(pDevContext);
 	DBG_Trace(DBG_LEVEL1, "WMD_BRD_MemUnMap vaCurr %x, pteAddrL1 %x "
 		  "pteAddrL2 %x\n", vaCurr, pteAddrL1, pteAddrL2);
 	DBG_Trace(DBG_ENTER, "< WMD_BRD_MemUnMap status %x remBytes %x, "
@@ -2055,9 +2061,7 @@  func_cont:
 	 * This is called from here instead from PteUpdate to avoid unnecessary
 	 * repetition while mapping non-contiguous physical regions of a virtual
 	 * region */
-	/* Waking up DSP before calling TLB Flush */
-	CHNLSM_InterruptDSP2(pDevContext, MBX_PM_DSPWAKEUP);
-	HW_MMU_TLBFlushAll(pDevContext->dwDSPMmuBase);
+	flush_all(pDevContext);
 	DBG_Trace(DBG_LEVEL7, "< WMD_BRD_MemMap  at end status %x\n", status);
 	return status;
 }