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? 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
