Few reasons as why the check shouldn't be in DSP flush functions. - Clients might forget to call flush call - In some cases you are not required to call the flush calls as you might map to a non-cacheable memory for optimization purposes. - DSP Bridge flush functions will be removed from Driver. If we go with a check, the check should be in the Map function as this is a required call for all Bridge clients. Thank you, Best regards, Hari > -----Original Message----- > From: Hiroshi DOYU [mailto:Hiroshi.DOYU@xxxxxxxxx] > Sent: Wednesday, May 13, 2009 6:33 AM > To: felipe.contreras@xxxxxxxxx > Cc: Kanigeri, Hari; Menon, Nishanth; linux-omap@xxxxxxxxxxxxxxx; Menon, > Nishanth > Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for > dsp cache line size > > From: ext Felipe Contreras <felipe.contreras@xxxxxxxxx> > Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for > dsp cache line size > Date: Wed, 13 May 2009 13:13:35 +0200 > > > On Wed, May 13, 2009 at 12:10 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> > wrote: > > > From: ext Felipe Contreras <felipe.contreras@xxxxxxxxx> > > > Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment > for dsp cache line size > > > Date: Tue, 12 May 2009 23:41:04 +0200 > > > > > >> On Tue, May 12, 2009 at 8:38 AM, Hiroshi DOYU > <Hiroshi.DOYU@xxxxxxxxx> wrote: > > >> > From: "ext Kanigeri, Hari" <h-kanigeri2@xxxxxx> > > >> > Subject: RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment > for dsp cache line size > > >> > Date: Mon, 11 May 2009 20:26:11 +0200 > > >> > > > >> >> To summarize the discussion. > > >> > > > >> > Thanks > > >> > > > >> >> We need to have the check for 128 bytes alignment (upper and > > >> >> lower). The 2 places that this can be done are in flush function > or > > >> >> in Map function. > > >> >> > > >> >> - I prefer the check is done in Map function as Felipe > > >> >> mentioned this function is bound to be called by MM components as > > >> >> opposed to Flush function. > > >> > > > >> > Mapping is totally another thing from this problem. > > >> > > > >> > Any page mapping is being done, forcing PAGE_SIZE as below, because > > >> > PAGE_SIZE(4KB) is the mininum unit from (io)mmu H/W perspective. > This > > >> > is same as ARM too. They've been aligned on 4KB already. So no need > to > > >> > worry about 128-byte alignement for mapping. > > >> > > >> It's the same address, you cannot flush an address that has not been > > >> mapped. Look at the dsp-dummy code[1]. > > >> > > >> DSPProcessor_ReserveMemory(b->node, to_reserve, &b->reserve); > > >> DSPProcessor_Map(b->node, b->data, b->size, b->reserve, &b->map, 0); > > >> DSPProcessor_FlushMemory(b->node, b->data, b->size, 0); > > >> > > >> See? b->data is used for *both* Map and FlushMemory. The only > > >> difference is that you can skip FlushMemory, or flush half of the > > >> memory area: > > >> DSPProcessor_FlushMemory(b->node, b->data + (b->size / 2), b->size, > 0); > > >> > > >> The only operation you cannot avoid is Map. > > >> > > >> > DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 > ulSize, > > >> > void *pReqAddr, void **ppMapAddr, u32 ulMapAttr) > > >> > { > > >> > ..... > > >> > /* Calculate the page-aligned PA, VA and size */ > > >> > vaAlign = PG_ALIGN_LOW((u32) pReqAddr, PG_SIZE_4K); > > >> > paAlign = PG_ALIGN_LOW((u32) pMpuAddr, PG_SIZE_4K); > > >> > sizeAlign = PG_ALIGN_HIGH(ulSize + (u32)pMpuAddr - paAlign, > > >> > PG_SIZE_4K); > > >> > > > >> > The points which we should take care of are bridge cache > > >> > operations("PROC_Flush/Invalidate..()") and we should/can handle > this > > >> > problem independently. It would cause another dependency and > increase > > >> > another complexity again if we have to assume something(ex: > > >> > "read-only" or "write-only" buffer) on other modules or expecting > > >> > something on its "protocol". > > >> > > >> If we are not going to have a read-only/write-only flag then I'm > > >> against adding the alignment check. It will only force user-space to > > >> do memory copies unnecessarily. That would kill performance > > >> drastically. NAK! > > > > > > At least, better than allowing user to crash kernel. > > > > The kernel would crash even with that check because it's the software > > running in the *DSP* side that is causing the corruption. > > > > It sounds you are still not convinced by that so I'll write a test > > application to make sure my assumption is correct. > > You misunderstood. > > There are 2 patterns when kernel crashes from ARM cache flush and DSP > cache flush(*1). > > 1: https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
