Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Wed, 23 Jun 2010 02:29:00 +0200 ext Ohad Ben-Cohen o...@wizery.com wrote: On Wed, Jun 16, 2010 at 8:50 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Ohad Ben-Cohen o...@wizery.com Thanks, I'll prepare them and resubmit You can use the following branch which has accumulateed unmerged mailbox patches. git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-mailbox Done. Thansk. [PATCH 1-3/4] are queued on the above branch and will be pulled soon. The following should be taken care of dspbridge: [PATCH 4/4] dspbridge: use mailbox API to set rx callback -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Wed, Jun 16, 2010 at 8:50 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Ohad Ben-Cohen o...@wizery.com Thanks, I'll prepare them and resubmit You can use the following branch which has accumulateed unmerged mailbox patches. git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-mailbox Done. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: ext C.A, Subramaniam subramaniam...@ti.com Subject: RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Mon, 14 Jun 2010 17:56:35 +0200 Hi Ohad/Hiroshi, Good to see the new mailbox requirements. While we are at it can we add some more :)? * Make the mailbox as a generic driver. Meaning, users can request for a pair of mailbox rather than using a set of pre-defined ones. For eg: Instead of doing an omap_mbox_get(mailbox-1) we can have the user specify the mailbox pair that he needs ( omap_mbox_get(int tx_mbox, int rx_mbox) or similar API ). This also means that we remove the bulk of the data structures like omap2_mbox_1_priv and mbox_1_info and so on. Additional checks needs to be done so that consequtive requests to the driver does not re-configure the rx-tx pairs. Rationale: The pre-configured structures does make sense in case of DSP Bridge, since it is the only user of mailbox. However, for Syslink for example, (or for any other IPC or user of mailbox) it would be good for the user of the mailbox to request the pair (or just tx/rx) from user/kernel side. Fair enough. * Provide debug support for the mailboxes (relevant for OMAP4) Since OMAP4 has support to read the RAW registers we might as well add an API for the user to read the status from RAW registers. This could be debugfs. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Ohad, From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 15 Jun 2010 06:43:21 +0200 Hi Hiroshi, On Mon, Jun 14, 2010 at 3:58 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: Does dspbridge really need its own defered work for sending mailbox messages? We do, according to http://permalink.gmane.org/gmane.linux.ports.arm.omap/38240 I am wondering that this defered work may not be necessary just for the serialization. (thanks Rene for the explanation). 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. Agree 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. Not sure how much QoS is an issue with mailbox (do we really have latency issues ?), but I guess adding such interface can't hurt. This solves the one case of lockdep issue. I think that workqueue is only necessary when it has to sleep, otherwise tasklet is prefered. I must say I disagree; tasklets are really not friendly to the whole system's responsiveness and should be used only if really needed (there was even an attempt to remove them entirely in the past: http://lwn.net/Articles/239633/). Evoking a workqueue may be overkilling just for putting several bytes into registers since large part of contents is usually passed through shared memory. I'd like to allow mutiple listners in some way, as the flexibility of mailbox functionalty. This can be achieved using notifier chains of multiple listeners callback, but I agree with Felipe: do we really want this functionality ? I just rather not add code that no one is going to use. What do you say we start with enforcing only 1 listener and later, if the use case of several listeners shows up, we'd add that support (promise! :) ? Fair enough. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Tue, Jun 15, 2010 at 3:04 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: Fair enough. Thanks, I'll prepare them and resubmit -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Ohad, From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Wed, 16 Jun 2010 07:09:13 +0200 On Tue, Jun 15, 2010 at 3:04 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: Fair enough. Thanks, I'll prepare them and resubmit You can use the following branch which has accumulateed unmerged mailbox patches. git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-mailbox -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Ohad, From: ext Ohad Ben-Cohen o...@wizery.com 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.d...@nokia.com 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-omapm=127601655325416w=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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Ohad/Hiroshi, Good to see the new mailbox requirements. While we are at it can we add some more :)? * Make the mailbox as a generic driver. Meaning, users can request for a pair of mailbox rather than using a set of pre-defined ones. For eg: Instead of doing an omap_mbox_get(mailbox-1) we can have the user specify the mailbox pair that he needs ( omap_mbox_get(int tx_mbox, int rx_mbox) or similar API ). This also means that we remove the bulk of the data structures like omap2_mbox_1_priv and mbox_1_info and so on. Additional checks needs to be done so that consequtive requests to the driver does not re-configure the rx-tx pairs. Rationale: The pre-configured structures does make sense in case of DSP Bridge, since it is the only user of mailbox. However, for Syslink for example, (or for any other IPC or user of mailbox) it would be good for the user of the mailbox to request the pair (or just tx/rx) from user/kernel side. * Provide debug support for the mailboxes (relevant for OMAP4) Since OMAP4 has support to read the RAW registers we might as well add an API for the user to read the status from RAW registers. Please provide your feedback! Thank you and Regards Subbu -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Hiroshi DOYU Sent: Monday, June 14, 2010 3:58 AM To: Guzman Lugo, Fernando; o...@wizery.com Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux-omap@vger.kernel.org Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Hi Ohad, From: ext Ohad Ben-Cohen o...@wizery.com 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.d...@nokia.com 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-omapm=127601655325416w=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
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Hiroshi -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Hiroshi DOYU Sent: Monday, June 14, 2010 3:58 AM To: Guzman Lugo, Fernando; o...@wizery.com Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux- o...@vger.kernel.org Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Hi Ohad, From: ext Ohad Ben-Cohen o...@wizery.com 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.d...@nokia.com 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-omapm=127601655325416w=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
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Mon, Jun 14, 2010 at 11:58 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Ohad Ben-Cohen o...@wizery.com * 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. Can you explain a use case? If there's no good argument for complexity, I would rather go for simplicity. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Hiroshi, On Mon, Jun 14, 2010 at 3:58 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: Does dspbridge really need its own defered work for sending mailbox messages? We do, according to http://permalink.gmane.org/gmane.linux.ports.arm.omap/38240 (thanks Rene for the explanation). 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. Agree 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. Not sure how much QoS is an issue with mailbox (do we really have latency issues ?), but I guess adding such interface can't hurt. I think that workqueue is only necessary when it has to sleep, otherwise tasklet is prefered. I must say I disagree; tasklets are really not friendly to the whole system's responsiveness and should be used only if really needed (there was even an attempt to remove them entirely in the past: http://lwn.net/Articles/239633/). I'd like to allow mutiple listners in some way, as the flexibility of mailbox functionalty. This can be achieved using notifier chains of multiple listeners callback, but I agree with Felipe: do we really want this functionality ? I just rather not add code that no one is going to use. What do you say we start with enforcing only 1 listener and later, if the use case of several listeners shows up, we'd add that support (promise! :) ? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Subbu, On Mon, Jun 14, 2010 at 10:56 AM, C.A, Subramaniam subramaniam...@ti.com wrote: Hi Ohad/Hiroshi, Good to see the new mailbox requirements. While we are at it can we add some more :)? ... Please provide your feedback! Feel free to send the patches ;) Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU hiroshi.d...@nokia.com 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.
(btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we
still need a mailbox queuing mechanism at all ? :)
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
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 ?
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).
* 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:
From 353d36d7fd6ec76c81689893338dfd4d33c8c89a Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen o...@wizery.com
Date: Sun, 13 Jun 2010 18:32:22 -0500
Subject: [PATCH] mailbox: enforce sane usage
Signed-off-by: Ohad Ben-Cohen o...@wizery.com
---
I can also be contacted via ohadb at ti dot com
arch/arm/plat-omap/include/plat/mailbox.h |5 ++-
arch/arm/plat-omap/mailbox.c | 38 +++-
2 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
b/arch/arm/plat-omap/include/plat/mailbox.h
index 729166b..0a5bf19 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -45,7 +45,6 @@ struct omap_mbox_queue {
struct request_queue*queue;
struct work_struct work;
struct tasklet_struct tasklet;
- int (*callback)(void *);
struct omap_mbox*mbox;
};
@@ -65,12 +64,14 @@ struct omap_mbox {
void*priv;
void(*err_notify)(void);
+ int (*callback)(void *);
+ atomic_tcount;
};
int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
void omap_mbox_init_seq(struct omap_mbox *);
-struct omap_mbox *omap_mbox_get(const char *);
+struct omap_mbox *omap_mbox_get(const char *, int (*)(void *));
void omap_mbox_put(struct omap_mbox *);
int omap_mbox_register(struct device *parent, struct omap_mbox *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 08a2df7..5f29ea4 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -33,8 +33,6 @@ static struct workqueue_struct *mboxd;
static struct omap_mbox *mboxes;
static DEFINE_RWLOCK(mboxes_lock);
-static int mbox_configured;
-
/* Mailbox FIFO handle functions */
static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
{
@@ -146,7 +144,7 @@ static void mbox_rx_work(struct work_struct *work)
msg = (mbox_msg_t)rq-special;
blk_end_request_all(rq, 0);
- mbox-rxq-callback((void *)msg);
+ mbox-callback((void *)msg);
}
}
@@ -249,16 +247,10 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
struct omap_mbox_queue *mq;
if (likely(mbox-ops-startup)) {
- write_lock(mboxes_lock);
- if (!mbox_configured)
- ret = mbox-ops-startup(mbox);
-
+ ret = mbox-ops-startup(mbox);
if (unlikely(ret)) {
- write_unlock(mboxes_lock);
return ret;
}
- mbox_configured++;
-
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Tue, Jun 8, 2010 at 6:46 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Guzman Lugo, Fernando fernando.l...@ti.com I think the best thing to do here is remove the spinlock, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. That maybe in this moment it is not called but it could be needed in the future. The caller of omap_mbox_msg_send should be take care of that function can not be call simultaneously. What about adding another function, omap_mbox_msg_send_sync()? That's exactly what I was thinking about when I heard the problem. Would this function send all the msgs already queued, and then wait for the current one actually be sent? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Tue, Jun 8, 2010 at 6:55 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 8 Jun 2010 05:11:50 +0200 Hi Deepak, On Mon, Jun 7, 2010 at 6:27 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: Ohad Ben-Cohen wrote: Somewhat relevant note about mailbox performance: omap_mbox_msg_send often (i.e. when the kfifo is empty) can just send the message directly, without triggering the tasklet to do that. Applying such a change will substantially improve the mailbox performance, and will make the shift to workqueues even more reasonable. I've got a patch for that, I'll post it soon. tasklet is run in atomic context,so I wonder how mailbox performance will increase if you switch from tasklet to workqueue? In general moving from tasklet to workqueues will probably increase some scheduling latencies. Nevertheless, if we'll send mailbox messages directly whenever possible, without triggering any deferred context, we'll be improving mailbox performance on average, because most of the time we will get rid of the scheduling latencies entirely. I think that this depends on the situation. 1) too many messages are coming, buffering and tasklet works efficiently at onece, for good throughput. 2) too few messages, the latency won't be a problem? Or some PM case may be concerned. I guess that most of the case, we just care about the throughput of encoding and decoding, but I am afraid that data themselves are sent via shared memory and the traffic of mailbox message may not be so big. I'd like to see the actual data for practical usecases. Does TI have some typical test pattern to measure throughput? Or Felipe? What exactly are you interested in? The amount of messages being sent on typical video encoding/decoding scenarios? My guess is that it would be one or two messages per frame (60 msgs/s tops). -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: ext Felipe Contreras felipe.contre...@gmail.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 8 Jun 2010 11:43:28 +0200 On Tue, Jun 8, 2010 at 6:46 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Guzman Lugo, Fernando fernando.l...@ti.com I think the best thing to do here is remove the spinlock, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. That maybe in this moment it is not called but it could be needed in the future. The caller of omap_mbox_msg_send should be take care of that function can not be call simultaneously. What about adding another function, omap_mbox_msg_send_sync()? That's exactly what I was thinking about when I heard the problem. Would this function send all the msgs already queued, and then wait for the current one actually be sent? Could be as you said. I haven't thought that much. What I just thought was to bypass kfifo and tasklet and to put a message directly to H/W fifo. Maily just for the above case which Fernando described above. Fernando, what do you think on Felipes? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
-Original Message- From: Hiroshi DOYU [mailto:hiroshi.d...@nokia.com] Sent: Monday, June 07, 2010 10:40 PM To: o...@wizery.com Cc: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar; Kanigeri, Hari; linux-omap@vger.kernel.org Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Mon, 7 Jun 2010 23:40:34 +0200 Hi Deepak, On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: With this patch I observed inconsistent lock state warning. Thanks for the report! Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() functions.In order to protect this critical section we need to protect by using spin_lock_bh() so that the tasklet cannot preempt omap_mobx_msg_send(). This is actually not the problem: it's ok if mbox_tx_tasklet preempts omap_mbox_msg_send. In fact, such a use case is even ok if we don't take a spinlock at all: kfifo is designed in a way that if you have only 1 consumer and 1 producer, they can both access kfifo simultaneously without any locking. That's why we don't take the spinlock in the mbox_tx_tasklet. The reason, btw, that we take a spinlock in omap_mbox_msg_send is to allow multiple simultaneous sending contexts (taking a spinlock there ensures serialization of those multiple simultaneous sending contexts). The problem here lies in the fact (that I've just learnt) that dspbridge also sends mbox messages from a tasklet context (dpc_tasklet). Lockdep identifies that the spinlock is taken in a softirq context (dspbridge's dpc_tasklet) after it was previously taken in a softirq-enabled context. Your proposed fix will solve the lockdep issue here, but: Do we really want to have tasklets in dspbridge ? Are we that performance critical ? Can't a whole contents of bridge_msg_get()/bridge_msg_put() be replaced with omap mailbox APIs? It seems that dspbridge does the same message queueing and deferred execution as omap mailbox does in the above 2 functions. At least bridge_msg_put() looks similar to omap_mbox_msg_send(). If this works, we can reduce the size of dspbridge again;) Yes, it has a pretty similar method to send the messages. However the messages send by bridge_msg_put() is a shared memory message, which has a cmd fiel and 2 arguments fields and the bridge used a mailbox message just to indicate to the DSP that it has some pending shared memory messages. Regards, Fernando. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
-Original Message-
From: Hiroshi DOYU [mailto:hiroshi.d...@nokia.com]
Sent: Tuesday, June 08, 2010 4:56 AM
To: felipe.contre...@gmail.com
Cc: Guzman Lugo, Fernando; o...@wizery.com; Chitriki Rudramuni, Deepak;
Ramirez Luna, Omar; Kanigeri, Hari; linux-omap@vger.kernel.org
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: ext Felipe Contreras felipe.contre...@gmail.com
Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Date: Tue, 8 Jun 2010 11:43:28 +0200
On Tue, Jun 8, 2010 at 6:46 AM, Hiroshi DOYU hiroshi.d...@nokia.com
wrote:
From: ext Guzman Lugo, Fernando fernando.l...@ti.com
I think the best thing to do here is remove the spinlock, if not,
you are preventing that omap_mbox_msg_send be executed from a
tasklet or isr context. That maybe in this moment it is not called
but it could be needed in the future. The caller of
omap_mbox_msg_send should be take care of that function can not be
call simultaneously.
What about adding another function, omap_mbox_msg_send_sync()?
That's exactly what I was thinking about when I heard the problem.
Would this function send all the msgs already queued, and then wait
for the current one actually be sent?
A function like that could be helpful in some cases I think. Maybe and other
one to flush (discard) all pending messages would be good too. So that I can
clean the mailbox and would not be needed to do a mbox_put and mbox_get.
Could be as you said. I haven't thought that much.
What I just thought was to bypass kfifo and tasklet and to put a
message directly to H/W fifo. Maily just for the above case which
Fernando described above.
Fernando, what do you think on Felipes?
Why don't just bypass the tasklet? I think the kfifo is ok in order to not
discard the message in case of full hw fifo. I was thinking in the
omap_mbox_msg_send just check if we have messajes in the kfifo or if the hw
fifo is full. If so, store the message en the kfifo and enable not full
interrupt. Otherwise write the message to hw fifo.
if (kfifo_len(mq-fifo) || mbox_fifo_full(mbox)) {
kfifo_in(mq-fifo, (unsigned char *)msg, sizeof(msg));
omap_mbox_enable_irq(mbox, IRQ_TX);
} else
mbox_fifo_write(mbox, msg);
and in the tx isr just send the messages store in the kfifo, reding the
messages from the kfifo is very fast and at last we will be able to send 4
messages (most of the cases we will be able to send 1 message), which I think
is even faster than just doing the tasklet schedule.
while (kfifo_len(mq-fifo) !mbox_fifo_full(mbox)) {
kfifo_out(mq-fifo, (unsigned char *)msg, sizeof(msg));
mbox_fifo_write(mbox, msg);
}
If (!mbox_fifo_full(mbox))
omap_mbox_disable_irq(mbox, IRQ_TX);
Regards,
Fernando.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
On Mon, Jun 7, 2010 at 6:14 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: I think the best thing to do here is remove the spinlock I'm afraid we can't do that: we need it to support OMAP4 syslink IPC use cases which have multiple simultaneous sending contexts. , if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. Right. But this is pretty standard - you can't call every kernel API from every possible context - the allowed calling context is part of the API. If later we decide we need to add additional calling contexts, it's ok - it's just like changing the API. Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)? hmm I'm not following here - will you please elaborate on this a bit more ? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 8 Jun 2010 04:54:55 +0200 On Mon, Jun 7, 2010 at 6:14 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: I think the best thing to do here is remove the spinlock I'm afraid we can't do that: we need it to support OMAP4 syslink IPC use cases which have multiple simultaneous sending contexts. , if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. Right. But this is pretty standard - you can't call every kernel API from every possible context - the allowed calling context is part of the API. If later we decide we need to add additional calling contexts, it's ok - it's just like changing the API. Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)? hmm I'm not following here - will you please elaborate on this a bit more ? The abvoe could be something like below: Modified arch/arm/plat-omap/mailbox.c 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); } static irqreturn_t mbox_interrupt(int irq, void *p) The user/client of mbox can register its own callback function in advance and do its own job(tasklet, workqueue, just func whatever) later by itself. It makes more sense to me than the current workqueue as default. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: Hiroshi DOYU [hiroshi.d...@nokia.com] Sent: Wednesday, June 09, 2010 12:07 AM To: o...@wizery.com Cc: Guzman Lugo, Fernando; Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux-omap@vger.kernel.org Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 8 Jun 2010 04:54:55 +0200 On Mon, Jun 7, 2010 at 6:14 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: I think the best thing to do here is remove the spinlock I'm afraid we can't do that: we need it to support OMAP4 syslink IPC use cases which have multiple simultaneous sending contexts. , if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. Right. But this is pretty standard - you can't call every kernel API from every possible context - the allowed calling context is part of the API. If later we decide we need to add additional calling contexts, it's ok - it's just like changing the API. Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)? hmm I'm not following here - will you please elaborate on this a bit more ? The abvoe could be something like below: Modified arch/arm/plat-omap/mailbox.c 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); } static irqreturn_t mbox_interrupt(int irq, void *p) The user/client of mbox can register its own callback function in advance and do its own job(tasklet, workqueue, just func whatever) later by itself. It makes more sense to me than the current workqueue as default. Yes, this is exactly what I was talking about. Regards, Fernando. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Ohad Ben-Cohen wrote:
The underlying buffering implementation of mailbox
is converted from block API to kfifo due to the simplicity
and speed of kfifo.
The default size of the kfifo buffer is set to 256 bytes.
This value is configurable at compile time (via
CONFIG_OMAP_MBOX_KFIFO_SIZE), and can be changed at
runtime (via the mbox_kfifo_size module parameter).
Signed-off-by: Ohad Ben-Cohen o...@wizery.com
Signed-off-by: Hari Kanigeri h-kanige...@ti.com
---
If you want, you can also reach me at ohadb at ti dot com .
arch/arm/plat-omap/Kconfig|9 +++
arch/arm/plat-omap/include/plat/mailbox.h |4 +-
arch/arm/plat-omap/mailbox.c | 112 +---
3 files changed, 63 insertions(+), 62 deletions(-)
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 6da796e..f4b0029 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -106,6 +106,15 @@ config OMAP_MBOX_FWK
Say Y here if you want to use OMAP Mailbox framework support for
DSP, IVA1.0 and IVA2 in OMAP1/2/3.
+config OMAP_MBOX_KFIFO_SIZE
+ int Mailbox kfifo default buffer size (bytes)
+ depends on OMAP_MBOX_FWK
+ default 256
+ help
+ Specify the default size of mailbox's kfifo buffers (bytes).
+ This can also be changed at runtime (via the mbox_kfifo_size
+ module parameter).
+
config OMAP_IOMMU
tristate
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 729166b..0c3c4a5 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -5,8 +5,8 @@
#include linux/wait.h
#include linux/workqueue.h
-#include linux/blkdev.h
#include linux/interrupt.h
+#include linux/kfifo.h
typedef u32 mbox_msg_t;
struct omap_mbox;
@@ -42,7 +42,7 @@ struct omap_mbox_ops {
struct omap_mbox_queue {
spinlock_t lock;
- struct request_queue*queue;
+ struct kfifofifo;
struct work_struct work;
struct tasklet_struct tasklet;
int (*callback)(void *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 7c60550..908d9d2 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -21,11 +21,14 @@
*
*/
+#include linux/kernel.h
#include linux/module.h
#include linux/interrupt.h
#include linux/device.h
#include linux/delay.h
#include linux/slab.h
+#include linux/kfifo.h
+#include linux/err.h
#include plat/mailbox.h
@@ -35,6 +38,10 @@ static DEFINE_SPINLOCK(mboxes_lock);
static int mbox_configured;
+static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE;
+module_param(mbox_kfifo_size, uint, S_IRUGO);
+MODULE_PARM_DESC(mbox_kfifo_size, Size of omap's mailbox kfifo (bytes));
+
/* Mailbox FIFO handle functions */
static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
{
@@ -67,7 +74,7 @@ static inline int is_mbox_irq(struct omap_mbox *mbox,
omap_mbox_irq_t irq)
/*
* message sender
*/
-static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
+static int __mbox_poll_for_space(struct omap_mbox *mbox)
{
int ret = 0, i = 1000;
@@ -78,49 +85,50 @@ static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
return -1;
udelay(1);
}
- mbox_fifo_write(mbox, msg);
return ret;
}
-
int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
{
+ struct omap_mbox_queue *mq = mbox-txq;
+ int ret = 0, len;
- struct request *rq;
- struct request_queue *q = mbox-txq-queue;
+ spin_lock(mq-lock);
- rq = blk_get_request(q, WRITE, GFP_ATOMIC);
- if (unlikely(!rq))
- return -ENOMEM;
+ if (kfifo_avail(mq-fifo) sizeof(msg)) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ len = kfifo_in(mq-fifo, (unsigned char *)msg, sizeof(msg));
+ WARN_ON(len != sizeof(msg));
- blk_insert_request(q, rq, 0, (void *) msg);
tasklet_schedule(mbox-txq-tasklet);
- return 0;
+out:
+ spin_unlock(mq-lock);
+ return ret;
}
EXPORT_SYMBOL(omap_mbox_msg_send);
static void mbox_tx_tasklet(unsigned long tx_data)
{
- int ret;
- struct request *rq;
struct omap_mbox *mbox = (struct omap_mbox *)tx_data;
- struct request_queue *q = mbox-txq-queue;
-
- while (1) {
-
- rq = blk_fetch_request(q);
-
- if (!rq)
- break;
+ struct omap_mbox_queue *mq = mbox-txq;
+ mbox_msg_t msg;
+ int ret;
- ret = __mbox_msg_send(mbox, (mbox_msg_t)rq-special);
- if (ret) {
+ while (kfifo_len(mq-fifo)) {
+ if (__mbox_poll_for_space(mbox)) {
omap_mbox_enable_irq(mbox, IRQ_TX);
- blk_requeue_request(q, rq);
-
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Deepak, On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: With this patch I observed inconsistent lock state warning. Thanks for the report! Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() functions.In order to protect this critical section we need to protect by using spin_lock_bh() so that the tasklet cannot preempt omap_mobx_msg_send(). This is actually not the problem: it's ok if mbox_tx_tasklet preempts omap_mbox_msg_send. In fact, such a use case is even ok if we don't take a spinlock at all: kfifo is designed in a way that if you have only 1 consumer and 1 producer, they can both access kfifo simultaneously without any locking. That's why we don't take the spinlock in the mbox_tx_tasklet. The reason, btw, that we take a spinlock in omap_mbox_msg_send is to allow multiple simultaneous sending contexts (taking a spinlock there ensures serialization of those multiple simultaneous sending contexts). The problem here lies in the fact (that I've just learnt) that dspbridge also sends mbox messages from a tasklet context (dpc_tasklet). Lockdep identifies that the spinlock is taken in a softirq context (dspbridge's dpc_tasklet) after it was previously taken in a softirq-enabled context. Your proposed fix will solve the lockdep issue here, but: Do we really want to have tasklets in dspbridge ? Are we that performance critical ? In general I'd prefer to switch to workqueues in both dspbridge and mailbox. I'm really not convinced we have to use tasklets in those modules, and workqueues are much more system friendly. This way we would also not have to stop all bottom halves when sending a mailbox message. Somewhat relevant note about mailbox performance: omap_mbox_msg_send often (i.e. when the kfifo is empty) can just send the message directly, without triggering the tasklet to do that. Applying such a change will substantially improve the mailbox performance, and will make the shift to workqueues even more reasonable. I've got a patch for that, I'll post it soon. What do you think (looping in Fernando, Omar and Hari) ? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi, -Original Message- From: Ohad Ben-Cohen [mailto:o...@wizery.com] Sent: Monday, June 07, 2010 4:41 PM To: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar; Kanigeri, Hari Cc: linux-omap@vger.kernel.org; Hiroshi Doyu Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Hi Deepak, On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: With this patch I observed inconsistent lock state warning. Thanks for the report! Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() functions.In order to protect this critical section we need to protect by using spin_lock_bh() so that the tasklet cannot preempt omap_mobx_msg_send(). This is actually not the problem: it's ok if mbox_tx_tasklet preempts omap_mbox_msg_send. In fact, such a use case is even ok if we don't take a spinlock at all: kfifo is designed in a way that if you have only 1 consumer and 1 producer, they can both access kfifo simultaneously without any locking. That's why we don't take the spinlock in the mbox_tx_tasklet. The reason, btw, that we take a spinlock in omap_mbox_msg_send is to allow multiple simultaneous sending contexts (taking a spinlock there ensures serialization of those multiple simultaneous sending contexts). The problem here lies in the fact (that I've just learnt) that dspbridge also sends mbox messages from a tasklet context (dpc_tasklet). Lockdep identifies that the spinlock is taken in a softirq context (dspbridge's dpc_tasklet) after it was previously taken in a softirq-enabled context. Your proposed fix will solve the lockdep issue here, but: I think the best thing to do here is remove the spinlock, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. That maybe in this moment it is not called but it could be needed in the future. The caller of omap_mbox_msg_send should be take care of that function can not be call simultaneously. Do we really want to have tasklets in dspbridge ? Are we that performance critical ? We are facing some issues when the DSP send the request for hibernation and we get that message and start processing the request (because it is done in a workqueue and there some latency). I issue had been manage in other ways but maybe we would have that issue it the hibernation request could be handle immediately. Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)? In general I'd prefer to switch to workqueues in both dspbridge and mailbox. I'm really not convinced we have to use tasklets in those modules, and workqueues are much more system friendly. This way we would also not have to stop all bottom halves when sending a mailbox message. Somewhat relevant note about mailbox performance: omap_mbox_msg_send often (i.e. when the kfifo is empty) can just send the message directly, without triggering the tasklet to do that. Applying such a change will substantially improve the mailbox performance, and will make the shift to workqueues even more reasonable. I've got a patch for that, I'll post it soon. What would be really good! Regards, Fernando. What do you think (looping in Fernando, Omar and Hari) ? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Ohad Ben-Cohen wrote: Hi Deepak, On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: With this patch I observed inconsistent lock state warning. Thanks for the report! Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() functions.In order to protect this critical section we need to protect by using spin_lock_bh() so that the tasklet cannot preempt omap_mobx_msg_send(). This is actually not the problem: it's ok if mbox_tx_tasklet preempts omap_mbox_msg_send. In fact, such a use case is even ok if we don't take a spinlock at all: kfifo is designed in a way that if you have only 1 consumer and 1 producer, they can both access kfifo simultaneously without any locking. That's why we don't take the spinlock in the mbox_tx_tasklet. The reason, btw, that we take a spinlock in omap_mbox_msg_send is to allow multiple simultaneous sending contexts (taking a spinlock there ensures serialization of those multiple simultaneous sending contexts). The problem here lies in the fact (that I've just learnt) that dspbridge also sends mbox messages from a tasklet context (dpc_tasklet). Lockdep identifies that the spinlock is taken in a softirq context (dspbridge's dpc_tasklet) after it was previously taken in a softirq-enabled context. Your proposed fix will solve the lockdep issue here, but: Do we really want to have tasklets in dspbridge ? Are we that performance critical ? In general I'd prefer to switch to workqueues in both dspbridge and mailbox. I'm really not convinced we have to use tasklets in those modules, and workqueues are much more system friendly. This way we would also not have to stop all bottom halves when sending a mailbox message. Somewhat relevant note about mailbox performance: omap_mbox_msg_send often (i.e. when the kfifo is empty) can just send the message directly, without triggering the tasklet to do that. Applying such a change will substantially improve the mailbox performance, and will make the shift to workqueues even more reasonable. I've got a patch for that, I'll post it soon. tasklet is run in atomic context,so I wonder how mailbox performance will increase if you switch from tasklet to workqueue? What do you think (looping in Fernando, Omar and Hari) ? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Deepak, On Mon, Jun 7, 2010 at 6:27 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: Ohad Ben-Cohen wrote: Somewhat relevant note about mailbox performance: omap_mbox_msg_send often (i.e. when the kfifo is empty) can just send the message directly, without triggering the tasklet to do that. Applying such a change will substantially improve the mailbox performance, and will make the shift to workqueues even more reasonable. I've got a patch for that, I'll post it soon. tasklet is run in atomic context,so I wonder how mailbox performance will increase if you switch from tasklet to workqueue? In general moving from tasklet to workqueues will probably increase some scheduling latencies. Nevertheless, if we'll send mailbox messages directly whenever possible, without triggering any deferred context, we'll be improving mailbox performance on average, because most of the time we will get rid of the scheduling latencies entirely. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: ext Guzman Lugo, Fernando fernando.l...@ti.com Subject: RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 8 Jun 2010 01:14:41 +0200 Hi, -Original Message- From: Ohad Ben-Cohen [mailto:o...@wizery.com] Sent: Monday, June 07, 2010 4:41 PM To: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar; Kanigeri, Hari Cc: linux-omap@vger.kernel.org; Hiroshi Doyu Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Hi Deepak, On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: With this patch I observed inconsistent lock state warning. Thanks for the report! Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() functions.In order to protect this critical section we need to protect by using spin_lock_bh() so that the tasklet cannot preempt omap_mobx_msg_send(). This is actually not the problem: it's ok if mbox_tx_tasklet preempts omap_mbox_msg_send. In fact, such a use case is even ok if we don't take a spinlock at all: kfifo is designed in a way that if you have only 1 consumer and 1 producer, they can both access kfifo simultaneously without any locking. That's why we don't take the spinlock in the mbox_tx_tasklet. The reason, btw, that we take a spinlock in omap_mbox_msg_send is to allow multiple simultaneous sending contexts (taking a spinlock there ensures serialization of those multiple simultaneous sending contexts). The problem here lies in the fact (that I've just learnt) that dspbridge also sends mbox messages from a tasklet context (dpc_tasklet). Lockdep identifies that the spinlock is taken in a softirq context (dspbridge's dpc_tasklet) after it was previously taken in a softirq-enabled context. Your proposed fix will solve the lockdep issue here, but: I think the best thing to do here is remove the spinlock, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. That maybe in this moment it is not called but it could be needed in the future. The caller of omap_mbox_msg_send should be take care of that function can not be call simultaneously. What about adding another function, omap_mbox_msg_send_sync()? I don't want to limit the ability of having multiple senders. The above would send the message with bypassing tasklet. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Tue, 8 Jun 2010 05:11:50 +0200 Hi Deepak, On Mon, Jun 7, 2010 at 6:27 PM, Deepak Chitriki deepak.chitr...@ti.com wrote: Ohad Ben-Cohen wrote: Somewhat relevant note about mailbox performance: omap_mbox_msg_send often (i.e. when the kfifo is empty) can just send the message directly, without triggering the tasklet to do that. Applying such a change will substantially improve the mailbox performance, and will make the shift to workqueues even more reasonable. I've got a patch for that, I'll post it soon. tasklet is run in atomic context,so I wonder how mailbox performance will increase if you switch from tasklet to workqueue? In general moving from tasklet to workqueues will probably increase some scheduling latencies. Nevertheless, if we'll send mailbox messages directly whenever possible, without triggering any deferred context, we'll be improving mailbox performance on average, because most of the time we will get rid of the scheduling latencies entirely. I think that this depends on the situation. 1) too many messages are coming, buffering and tasklet works efficiently at onece, for good throughput. 2) too few messages, the latency won't be a problem? Or some PM case may be concerned. I guess that most of the case, we just care about the throughput of encoding and decoding, but I am afraid that data themselves are sent via shared memory and the traffic of mailbox message may not be so big. I'd like to see the actual data for practical usecases. Does TI have some typical test pattern to measure throughput? Or Felipe? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] omap: mailbox: convert block api to kfifo
The underlying buffering implementation of mailbox
is converted from block API to kfifo due to the simplicity
and speed of kfifo.
The default size of the kfifo buffer is set to 256 bytes.
This value is configurable at compile time (via
CONFIG_OMAP_MBOX_KFIFO_SIZE), and can be changed at
runtime (via the mbox_kfifo_size module parameter).
Signed-off-by: Ohad Ben-Cohen o...@wizery.com
Signed-off-by: Hari Kanigeri h-kanige...@ti.com
---
If you want, you can also reach me at ohadb at ti dot com .
arch/arm/plat-omap/Kconfig|9 +++
arch/arm/plat-omap/include/plat/mailbox.h |4 +-
arch/arm/plat-omap/mailbox.c | 112 +---
3 files changed, 63 insertions(+), 62 deletions(-)
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 6da796e..f4b0029 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -106,6 +106,15 @@ config OMAP_MBOX_FWK
Say Y here if you want to use OMAP Mailbox framework support for
DSP, IVA1.0 and IVA2 in OMAP1/2/3.
+config OMAP_MBOX_KFIFO_SIZE
+ int Mailbox kfifo default buffer size (bytes)
+ depends on OMAP_MBOX_FWK
+ default 256
+ help
+ Specify the default size of mailbox's kfifo buffers (bytes).
+ This can also be changed at runtime (via the mbox_kfifo_size
+ module parameter).
+
config OMAP_IOMMU
tristate
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h
b/arch/arm/plat-omap/include/plat/mailbox.h
index 729166b..0c3c4a5 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -5,8 +5,8 @@
#include linux/wait.h
#include linux/workqueue.h
-#include linux/blkdev.h
#include linux/interrupt.h
+#include linux/kfifo.h
typedef u32 mbox_msg_t;
struct omap_mbox;
@@ -42,7 +42,7 @@ struct omap_mbox_ops {
struct omap_mbox_queue {
spinlock_t lock;
- struct request_queue*queue;
+ struct kfifofifo;
struct work_struct work;
struct tasklet_struct tasklet;
int (*callback)(void *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 7c60550..908d9d2 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -21,11 +21,14 @@
*
*/
+#include linux/kernel.h
#include linux/module.h
#include linux/interrupt.h
#include linux/device.h
#include linux/delay.h
#include linux/slab.h
+#include linux/kfifo.h
+#include linux/err.h
#include plat/mailbox.h
@@ -35,6 +38,10 @@ static DEFINE_SPINLOCK(mboxes_lock);
static int mbox_configured;
+static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE;
+module_param(mbox_kfifo_size, uint, S_IRUGO);
+MODULE_PARM_DESC(mbox_kfifo_size, Size of omap's mailbox kfifo (bytes));
+
/* Mailbox FIFO handle functions */
static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
{
@@ -67,7 +74,7 @@ static inline int is_mbox_irq(struct omap_mbox *mbox,
omap_mbox_irq_t irq)
/*
* message sender
*/
-static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
+static int __mbox_poll_for_space(struct omap_mbox *mbox)
{
int ret = 0, i = 1000;
@@ -78,49 +85,50 @@ static int __mbox_msg_send(struct omap_mbox *mbox,
mbox_msg_t msg)
return -1;
udelay(1);
}
- mbox_fifo_write(mbox, msg);
return ret;
}
-
int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
{
+ struct omap_mbox_queue *mq = mbox-txq;
+ int ret = 0, len;
- struct request *rq;
- struct request_queue *q = mbox-txq-queue;
+ spin_lock(mq-lock);
- rq = blk_get_request(q, WRITE, GFP_ATOMIC);
- if (unlikely(!rq))
- return -ENOMEM;
+ if (kfifo_avail(mq-fifo) sizeof(msg)) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ len = kfifo_in(mq-fifo, (unsigned char *)msg, sizeof(msg));
+ WARN_ON(len != sizeof(msg));
- blk_insert_request(q, rq, 0, (void *) msg);
tasklet_schedule(mbox-txq-tasklet);
- return 0;
+out:
+ spin_unlock(mq-lock);
+ return ret;
}
EXPORT_SYMBOL(omap_mbox_msg_send);
static void mbox_tx_tasklet(unsigned long tx_data)
{
- int ret;
- struct request *rq;
struct omap_mbox *mbox = (struct omap_mbox *)tx_data;
- struct request_queue *q = mbox-txq-queue;
-
- while (1) {
-
- rq = blk_fetch_request(q);
-
- if (!rq)
- break;
+ struct omap_mbox_queue *mq = mbox-txq;
+ mbox_msg_t msg;
+ int ret;
- ret = __mbox_msg_send(mbox, (mbox_msg_t)rq-special);
- if (ret) {
+ while (kfifo_len(mq-fifo)) {
+ if (__mbox_poll_for_space(mbox)) {
omap_mbox_enable_irq(mbox, IRQ_TX);
- blk_requeue_request(q, rq);
-
