Hi Hiroshi > -----Original Message----- > From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap- > owner@xxxxxxxxxxxxxxx] On Behalf Of Hiroshi DOYU > Sent: Monday, June 14, 2010 3:58 AM > To: Guzman Lugo, Fernando; ohad@xxxxxxxxxx > Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux- > omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo > > Hi Ohad, > > From: ext Ohad Ben-Cohen <ohad@xxxxxxxxxx> > Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo > Date: Mon, 14 Jun 2010 01:52:16 +0200 > > > On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> > wrote: > >> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat- > omap/mailbox.c > >> index 87e0cde..1b79b32 100644 > >> --- a/arch/arm/plat-omap/mailbox.c > >> +++ b/arch/arm/plat-omap/mailbox.c > >> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox > *mbox) > >> /* no more messages in the fifo. clear IRQ source. */ > >> ack_mbox_irq(mbox, IRQ_RX); > >> nomem: > >> - queue_work(mboxd, &mbox->rxq->work); > >> + mbox->callback(mbox); > >> } > > > > I like this ! > > > > It will allow us to easily plug in new IPC mechanisms in the future. > > Agree. > > > (btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we > > still need a mailbox queuing mechanism at all ? :) > > I think that "queuing"(can be called as S/W fifo/buffer?) is basically > necessary it can compensate the shortage of H/W fifo slots under high > load or emergency case. It has only 4 slots. I guess that, DSP usually > responds quickly and 4 H/W slots may be enough, but it might be > safer/more robust to avoid the assumption which depends on other > entity/DSP. From latecy perspective, "s/w fifo" + "tasklet" would be > enough short. > > omap_mbox_msg_send_sync() can handle the case for a special message, > like PM, which has to respond at the higher priority than the normal > ones. > > > Having said that, this is not going to solve the lockdep warning > > reported by Deepak - that was caused because of dspbridge's sending > > context (and not because of the receiving context). To eliminate that > > Does dspbridge really need its own defered work for sending mailbox > messages? > > For me, the problem here is the unneccesary duplication of tasklet, or > can be said, the unnecessary use of tasklet for _sending_ mailbox > messages in dspbridge. > > http://marc.info/?l=linux-omap&m=127601655325416&w=2 > > I thought that bridge_msg_put()/bridge_msg_get() can use omap mbox > APIs directly, with getting rid of its use of its own defered > work/tasklet as pointed out in the above link. Fernando? > Actually the intention of that deferred work is not to send the Mbox messages. Bridge shares a section of physical memory with the DSP and communication is established through this one. Messages (not mailbox messages) coming from users go into bridge_msg_put(), those are more significant messages containing more arguments that are understandable by a DSP task. If the Shared memory(SHM) is occupied by a previous message, messages are queued inside bridge_msg_put() and the tasklet to send them is scheduled (so that we don't loose any) due that there can only be a single message going out and one coming back at a time in this Shared memory. The usage of the Mailbox messages is to notify to the DSP (actually to interrupt it and make it go to check the SHM) that there is a message with more information in that shared memory. The tasklet is needed to serialize the upper communication that is done in the SHM level and every time that we want to send out a message previously queued we need to interrupt the DSP (use the Mailbox messages). On the other hand PM messages are sent directly by using the Mailbox messages where we directly call omap_mbox_msg_send(). > For recieving, its defered work(tasklet) can be trigered directly in > the above proposed callback, that callback can triger its own > workqueue if necessary, then. I think that, for recieving, some PM > command may has to be sent back immedieately inside of > tasklet. omap_mbox_msg_send_sync() may handle this case. > > What do you think? > > > issue, I prefer fixing dspbridge to use work queues rather than using > > spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves > > just to send a mbox msg sounds unjustified (unless bridge really needs > > to use tasklets instead of work queues, which I slightly doubt). What > > do you think ? > > I think that workqueue is only necessary when it has to sleep, > otherwise tasklet is prefered. For _sending_ a message inside of > dspbridge, I haven't found any reasonable reason to use any defered > work(softirq, tasklet, workqueue) so far. > > > Speaking of mailbox I'd like to address some issues that are code > related: > > > > * Let's add mailbox API to set the callback pointer (it feels wrong to > > let users directly manipulate the mbox structure). > > > > * We can also safely move the callback field to the main mbox > > structure, since it has no usage in the TX mq; it's pretty global to > > the whole mbox. > > > > * Let's make sure no one accidentally registers two receivers on the > > same time (which would result in one overwriting the other's callback > > field). > > I'd like to allow mutiple listners in some way, as the flexibility of > mailbox functionalty. > > > * mbox_configured is a global variable and that breaks support of > > multiple concurrent mailbox instances. Let's move this logic into the > > instance of mbox. > > > > * If we make sure there are no two user of the same mailbox instance > > (see 3rd *), we can eliminate mbox_configured and its locking (not > > that doesn't mean there can't be two concurrent sending contexts, it > > just means they are somewhat related, they have a common receiver, > > which was registered using only a single call to get). > > > > Something like this, pls tell me what you think: > > For the above 5 proposals, excpet the multiple listeners issue, they > seems ok. > -- > 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 -- 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
