Re: SOLO6x10: fix a race in IRQ handler.
On Fri, 14 Nov 2014 13:35:06 +0100
khal...@piap.pl (Krzysztof Hałasa) wrote:
> The IRQs have to be acknowledged before they are serviced, otherwise
> some events may be skipped. Also, acknowledging IRQs just before
> returning from the handler doesn't leave enough time for the device
> to deassert the INTx line, and for bridges to propagate this change.
> This resulted in twice the IRQ rate on ARMv6 dual core CPU.
>
> Signed-off-by: Krzysztof Hałasa
>
> --- a/drivers/media/pci/solo6x10/solo6x10-core.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-core.c
> @@ -105,11 +105,8 @@ static irqreturn_t solo_isr(int irq, void *data)
> if (!status)
> return IRQ_NONE;
>
> - if (status & ~solo_dev->irq_mask) {
> - solo_reg_write(solo_dev, SOLO_IRQ_STAT,
> -status & ~solo_dev->irq_mask);
> - status &= solo_dev->irq_mask;
> - }
> + /* Acknowledge all interrupts immediately */
> + solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
>
> if (status & SOLO_IRQ_PCI_ERR)
> solo_p2m_error_isr(solo_dev);
> @@ -132,9 +129,6 @@ static irqreturn_t solo_isr(int irq, void *data)
> if (status & SOLO_IRQ_G723)
> solo_g723_isr(solo_dev);
>
> - /* Clear all interrupts handled */
> - solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
> -
> return IRQ_HANDLED;
> }
>
>
Signed-off-by: Ismael Luceno
pgpM1nfzSn1Y5.pgp
Description: OpenPGP digital signature
Re: patchwork on solo6x10: fix a race in IRQ handler
On 11/17/2014 05:47 PM, Andrey Utkin wrote: > Dear linux-media maintainers, I fail to do `git am` on mbox-formatted > patch downloadable from https://patchwork.linuxtv.org/patch/26970/ > so i worry if the Krzyztof's patch i resubmitted is well-formed, and > whether you are fine with integration of this patch to media_tree and > further to upstream. Please let me know if there you experience any > issues with that. > I plan to merge it Friday. It's in my TODO list. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
patchwork on solo6x10: fix a race in IRQ handler
Dear linux-media maintainers, I fail to do `git am` on mbox-formatted patch downloadable from https://patchwork.linuxtv.org/patch/26970/ so i worry if the Krzyztof's patch i resubmitted is well-formed, and whether you are fine with integration of this patch to media_tree and further to upstream. Please let me know if there you experience any issues with that. -- Andrey Utkin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] solo6x10: fix a race in IRQ handler
From: Krzysztof Hałasa
The IRQs have to be acknowledged before they are serviced, otherwise some events
may be skipped. Also, acknowledging IRQs just before returning from the handler
doesn't leave enough time for the device to deassert the INTx line, and for
bridges to propagate this change. This resulted in twice the IRQ rate on ARMv6
dual core CPU.
Signed-off-by: Krzysztof Hałasa
Acked-by: Andrey Utkin
Tested-by: Andrey Utkin
--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -105,11 +105,8 @@ static irqreturn_t solo_isr(int irq, void *data)
if (!status)
return IRQ_NONE;
- if (status & ~solo_dev->irq_mask) {
- solo_reg_write(solo_dev, SOLO_IRQ_STAT,
- status & ~solo_dev->irq_mask);
- status &= solo_dev->irq_mask;
- }
+ /* Acknowledge all interrupts immediately */
+ solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
if (status & SOLO_IRQ_PCI_ERR)
solo_p2m_error_isr(solo_dev);
@@ -132,9 +129,6 @@ static irqreturn_t solo_isr(int irq, void *data)
if (status & SOLO_IRQ_G723)
solo_g723_isr(solo_dev);
- /* Clear all interrupts handled */
- solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
-
return IRQ_HANDLED;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SOLO6x10: fix a race in IRQ handler.
Hi Andrey,
Please always prefix the subject line with [PATCH] when you post a patch. That
way it
will be picked up by patchwork
(https://patchwork.linuxtv.org/project/linux-media/list/)
and the patch won't be lost.
Can you repost with such a prefix?
Thanks!
Hans
On 11/15/2014 11:34 AM, Andrey Utkin wrote:
> From: khal...@piap.pl (Krzysztof =?utf-8?Q?Ha=C5=82asa?=)
>
> The IRQs have to be acknowledged before they are serviced, otherwise some
> events
> may be skipped. Also, acknowledging IRQs just before returning from the
> handler
> doesn't leave enough time for the device to deassert the INTx line, and for
> bridges to propagate this change. This resulted in twice the IRQ rate on ARMv6
> dual core CPU.
>
> Signed-off-by: Krzysztof Hałasa
> Acked-by: Andrey Utkin
> Tested-by: Andrey Utkin
>
> --- a/drivers/media/pci/solo6x10/solo6x10-core.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-core.c
> @@ -105,11 +105,8 @@ static irqreturn_t solo_isr(int irq, void *data)
> if (!status)
> return IRQ_NONE;
>
> - if (status & ~solo_dev->irq_mask) {
> - solo_reg_write(solo_dev, SOLO_IRQ_STAT,
> -status & ~solo_dev->irq_mask);
> - status &= solo_dev->irq_mask;
> - }
> + /* Acknowledge all interrupts immediately */
> + solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
>
> if (status & SOLO_IRQ_PCI_ERR)
> solo_p2m_error_isr(solo_dev);
> @@ -132,9 +129,6 @@ static irqreturn_t solo_isr(int irq, void *data)
> if (status & SOLO_IRQ_G723)
> solo_g723_isr(solo_dev);
>
> - /* Clear all interrupts handled */
> - solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
> -
> return IRQ_HANDLED;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SOLO6x10: fix a race in IRQ handler.
Krzysztof, it seems to really help. The host is working stable for 2.5 hours at the moment, with original framerate of 2 fps. Thank you very much. -- Andrey Utkin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SOLO6x10: fix a race in IRQ handler.
2014-11-15 1:33 GMT+04:00 Krzysztof Hałasa : > The SOLO IRQ controller does the common thing, all drivers (for chips > using the relatively modern "write 1 to clear") have to follow this > sequence: first ACK the interrupts sources (so they are deasserted, > though they can be asserted again if new events arrive), and only then > service the chip. Thanks for explanation. > I think my patch does exactly this, merges both writes. Ah right, sorry. -- Andrey Utkin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SOLO6x10: fix a race in IRQ handler.
Andrey Utkin writes: > could you please point to some reading which explains this moment? Or > you just know this from experience? The solo device specs are very > terse about this, so I considered that it should work fine without > regard to how fast we write back to that register. The SOLO IRQ controller does the common thing, all drivers (for chips using the relatively modern "write 1 to clear") have to follow this sequence: first ACK the interrupts sources (so they are deasserted, though they can be asserted again if new events arrive), and only then service the chip. > Also while you're at it, and if this really makes sense, you could > merge these two writes (unrecognized bits, then recognized bits) to > one write act. I think my patch does exactly this, merges both writes. -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SOLO6x10: fix a race in IRQ handler.
Also while you're at it, and if this really makes sense, you could merge these two writes (unrecognized bits, then recognized bits) to one write act. -- Andrey Utkin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SOLO6x10: fix a race in IRQ handler.
2014-11-14 16:35 GMT+04:00 Krzysztof Hałasa : > The IRQs have to be acknowledged before they are serviced, otherwise some > events > may be skipped. Also, acknowledging IRQs just before returning from the > handler > doesn't leave enough time for the device to deassert the INTx line, and for > bridges to propagate this change. This resulted in twice the IRQ rate on ARMv6 > dual core CPU. Thanks! I'm not experienced in interaction with hardware in this regard... could you please point to some reading which explains this moment? Or you just know this from experience? The solo device specs are very terse about this, so I considered that it should work fine without regard to how fast we write back to that register. -- Andrey Utkin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SOLO6x10: fix a race in IRQ handler.
The IRQs have to be acknowledged before they are serviced, otherwise some events
may be skipped. Also, acknowledging IRQs just before returning from the handler
doesn't leave enough time for the device to deassert the INTx line, and for
bridges to propagate this change. This resulted in twice the IRQ rate on ARMv6
dual core CPU.
Signed-off-by: Krzysztof Hałasa
--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -105,11 +105,8 @@ static irqreturn_t solo_isr(int irq, void *data)
if (!status)
return IRQ_NONE;
- if (status & ~solo_dev->irq_mask) {
- solo_reg_write(solo_dev, SOLO_IRQ_STAT,
- status & ~solo_dev->irq_mask);
- status &= solo_dev->irq_mask;
- }
+ /* Acknowledge all interrupts immediately */
+ solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
if (status & SOLO_IRQ_PCI_ERR)
solo_p2m_error_isr(solo_dev);
@@ -132,9 +129,6 @@ static irqreturn_t solo_isr(int irq, void *data)
if (status & SOLO_IRQ_G723)
solo_g723_isr(solo_dev);
- /* Clear all interrupts handled */
- solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
-
return IRQ_HANDLED;
}
--
Krzysztof Halasa
Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
