Re: [PATCH 09/10] omap: mailbox: convert block api to kfifo
On Tue, Aug 10, 2010 at 6:00 PM, Sapiens, Rene wrote: > Hi Ohad, > > Sure I will do it. Thanks a lot, Rene ! Ohad. > > Regards, > Rene > >> -Original Message- >> From: Ohad Ben-Cohen [mailto:o...@wizery.com] >> Sent: Tuesday, August 10, 2010 9:43 AM >> To: Guzman Lugo, Fernando; Sapiens, Rene >> Cc: Hiroshi DOYU; linux-omap@vger.kernel.org; linux-arm- >> ker...@lists.infradead.org; Kanigeri, Hari >> Subject: Re: [PATCH 09/10] omap: mailbox: convert block api to kfifo >> >> Hi Rene, >> >> >> On Wed, Jun 9, 2010 at 8:38 AM, Guzman Lugo, Fernando >> wrote: >> >>On Tue, Jun 8, 2010 at 7:16 PM, Sapiens, Rene >> wrote: >> >>> In mbox_rx_work() you are removing the lines that enable back >> the mbox irq for the RX case, but inside __mbox_rx_interrupt() this >> interrupt is disabled in the case that the kfifo for Rx >mailbox gets >> full. So I think that we need to enable it back as soon as there is space >> in this kfifo. >> >> >> >> >> >>Actually these irq on/off lines are not part of my patch; they are >> >>introduced by patch 05/10 on top of which my patches were rebased. >> >> >> >>Nevertheless I agree with you - the kfifo migration patch should not >> >>affect that irq on/off behavior. It's probably just a rebase gotcha. >> >> >> >>But now that you point me to this irq on/off thing, it looks a bit >> >>broken in terms of multiple concurrent mbox support since it relies on >> >>a global rq_full state. I guess it'd be better to hold that rq_full >> >>state in the relevant mbox queue state itself. >> >> >> >>Fernando what do you think ? >> > >> > Yes, you are right Ohad. Only should be disable the "new message" >> interrupt of the mailbox which kfifo is full. >> >> >> >> Once Fernando's fix will get thru, we will be able to fix the rebase >> error that you pointed out. >> >> Unfortunately I will not have any email access in the next 3 weeks, >> and I was hoping maybe you could submit a fix for this once Fernando's >> fix is accepted ? I would really like us to fix this early in the days >> of 2.6.36, maybe even during the merge window. >> >> Thanks a lot, >> Ohad. >> >> > >> > regards, >> > Fernando. >> > >> >> >> >>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 09/10] omap: mailbox: convert block api to kfifo
Hi Ohad, Sure I will do it. Regards, Rene > -Original Message- > From: Ohad Ben-Cohen [mailto:o...@wizery.com] > Sent: Tuesday, August 10, 2010 9:43 AM > To: Guzman Lugo, Fernando; Sapiens, Rene > Cc: Hiroshi DOYU; linux-omap@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; Kanigeri, Hari > Subject: Re: [PATCH 09/10] omap: mailbox: convert block api to kfifo > > Hi Rene, > > > On Wed, Jun 9, 2010 at 8:38 AM, Guzman Lugo, Fernando > wrote: > >>On Tue, Jun 8, 2010 at 7:16 PM, Sapiens, Rene > wrote: > >>> In mbox_rx_work() you are removing the lines that enable back > the mbox irq for the RX case, but inside __mbox_rx_interrupt() this > interrupt is disabled in the case that the kfifo for Rx >mailbox gets > full. So I think that we need to enable it back as soon as there is space > in this kfifo. > >> > >> > >>Actually these irq on/off lines are not part of my patch; they are > >>introduced by patch 05/10 on top of which my patches were rebased. > >> > >>Nevertheless I agree with you - the kfifo migration patch should not > >>affect that irq on/off behavior. It's probably just a rebase gotcha. > >> > >>But now that you point me to this irq on/off thing, it looks a bit > >>broken in terms of multiple concurrent mbox support since it relies on > >>a global rq_full state. I guess it'd be better to hold that rq_full > >>state in the relevant mbox queue state itself. > >> > >>Fernando what do you think ? > > > > Yes, you are right Ohad. Only should be disable the "new message" > interrupt of the mailbox which kfifo is full. > > > > Once Fernando's fix will get thru, we will be able to fix the rebase > error that you pointed out. > > Unfortunately I will not have any email access in the next 3 weeks, > and I was hoping maybe you could submit a fix for this once Fernando's > fix is accepted ? I would really like us to fix this early in the days > of 2.6.36, maybe even during the merge window. > > Thanks a lot, > Ohad. > > > > > regards, > > Fernando. > > > >> > >>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 09/10] omap: mailbox: convert block api to kfifo
Hi Rene, On Wed, Jun 9, 2010 at 8:38 AM, Guzman Lugo, Fernando wrote: >>On Tue, Jun 8, 2010 at 7:16 PM, Sapiens, Rene wrote: >>> In mbox_rx_work() you are removing the lines that enable back the mbox irq >>> for the RX case, but inside __mbox_rx_interrupt() this interrupt is >>> disabled in the case that the kfifo for Rx >mailbox gets full. So I think >>> that we need to enable it back as soon as there is space in this kfifo. >> >> >>Actually these irq on/off lines are not part of my patch; they are >>introduced by patch 05/10 on top of which my patches were rebased. >> >>Nevertheless I agree with you - the kfifo migration patch should not >>affect that irq on/off behavior. It's probably just a rebase gotcha. >> >>But now that you point me to this irq on/off thing, it looks a bit >>broken in terms of multiple concurrent mbox support since it relies on >>a global rq_full state. I guess it'd be better to hold that rq_full >>state in the relevant mbox queue state itself. >> >>Fernando what do you think ? > > Yes, you are right Ohad. Only should be disable the "new message" interrupt > of the mailbox which kfifo is full. Once Fernando's fix will get thru, we will be able to fix the rebase error that you pointed out. Unfortunately I will not have any email access in the next 3 weeks, and I was hoping maybe you could submit a fix for this once Fernando's fix is accepted ? I would really like us to fix this early in the days of 2.6.36, maybe even during the merge window. Thanks a lot, Ohad. > > regards, > Fernando. > >> >>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 09/10] omap: mailbox: convert block api to kfifo
On Wed, Jun 9, 2010 at 12:38 AM, Guzman Lugo, Fernando wrote: >>From: Ohad Ben-Cohen [o...@wizery.com] >>But now that you point me to this irq on/off thing, it looks a bit >>broken in terms of multiple concurrent mbox support since it relies on >>a global rq_full state. I guess it'd be better to hold that rq_full >>state in the relevant mbox queue state itself. > > Yes, you are right Ohad. Only should be disable the "new message" interrupt > of the mailbox which kfifo is full. Great! do you plan to submit a fix to patch 05/10 then ? Thanks, Ohad. > > regards, > Fernando. > >> >>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 09/10] omap: mailbox: convert block api to kfifo
Hi Ohad, >From: Ohad Ben-Cohen [o...@wizery.com] >Sent: Tuesday, June 08, 2010 9:35 PM >To: Sapiens, Rene; Guzman Lugo, Fernando >Cc: Hiroshi DOYU; linux-omap@vger.kernel.org; >linux-arm-ker...@lists.infradead.org; Kanigeri, Hari >Subject: Re: [PATCH 09/10] omap: mailbox: convert block api to kfifo > >Hi Rene, > >On Tue, Jun 8, 2010 at 7:16 PM, Sapiens, Rene wrote: >> In mbox_rx_work() you are removing the lines that enable back the mbox irq >> for the RX case, but inside __mbox_rx_interrupt() this interrupt is >> disabled in the case that the kfifo for Rx >mailbox gets full. So I think >> that we need to enable it back as soon as there is space in this kfifo. > > > >Actually these irq on/off lines are not part of my patch; they are >introduced by patch 05/10 on top of which my patches were rebased. > >Nevertheless I agree with you - the kfifo migration patch should not >affect that irq on/off behavior. It's probably just a rebase gotcha. > >But now that you point me to this irq on/off thing, it looks a bit >broken in terms of multiple concurrent mbox support since it relies on >a global rq_full state. I guess it'd be better to hold that rq_full >state in the relevant mbox queue state itself. > >Fernando what do you think ? Yes, you are right Ohad. Only should be disable the "new message" interrupt of the mailbox which kfifo is full. regards, Fernando. > >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 09/10] omap: mailbox: convert block api to kfifo
Hi Rene, On Tue, Jun 8, 2010 at 7:16 PM, Sapiens, Rene wrote: > In mbox_rx_work() you are removing the lines that enable back the mbox irq > for the RX case, but inside __mbox_rx_interrupt() this interrupt is > disabled in the case that the kfifo for Rx mailbox gets full. So I think that > we need to enable it back as soon as there is space in this kfifo. Actually these irq on/off lines are not part of my patch; they are introduced by patch 05/10 on top of which my patches were rebased. Nevertheless I agree with you - the kfifo migration patch should not affect that irq on/off behavior. It's probably just a rebase gotcha. But now that you point me to this irq on/off thing, it looks a bit broken in terms of multiple concurrent mbox support since it relies on a global rq_full state. I guess it'd be better to hold that rq_full state in the relevant mbox queue state itself. Fernando what do you think ? 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 09/10] omap: mailbox: convert block api to kfifo
Hi Ohad,
In mbox_rx_work() you are removing the lines that enable back the mbox irq for
the RX case, but inside __mbox_rx_interrupt() this interrupt is disabled in
the case that the kfifo for Rx mailbox gets full. So I think that we need to
enable it back as soon as there is space in this kfifo.
Regards,
Rene
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Hiroshi DOYU
> Sent: Thursday, June 03, 2010 1:34 AM
> To: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Cc: Ohad Ben-Cohen; Kanigeri, Hari; Hiroshi DOYU
> Subject: [PATCH 09/10] omap: mailbox: convert block api to kfifo
>
> From: Ohad Ben-Cohen
>
> 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
> Signed-off-by: Hari Kanigeri
> Signed-off-by: Hiroshi DOYU
> ---
> arch/arm/plat-omap/Kconfig|9 ++
> arch/arm/plat-omap/include/plat/mailbox.h |4 +-
> arch/arm/plat-omap/mailbox.c | 119 +---
> -
> 3 files changed, 64 insertions(+), 68 deletions(-)
>
> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
> index 78b49a6..111d39a 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
> #include
> -#include
> #include
> +#include
>
> 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 81076b5..ec0e159 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -21,11 +21,14 @@
> *
> */
>
> +#include
> #include
> #include
> #include
> #include
> #include
> +#include
> +#include
>
> #include
>
> @@ -37,6 +40,10 @@ static bool rq_full;
> static int mbox_configured;
> static DEFINE_MUTEX(mbox_configured_lock);
>
> +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)
> {
> @@ -69,7 +76,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;
>
> @@ -80,49 +87,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(uns
