[PATCH 0/3] aspeed-video: extend spurious interrupt handling

[PATCH 0/3] aspeed-video: extend spurious interrupt handling

[Zev Weiss]

These patches build on commit 65d270acb2d6 to address a similar
problem we've observed with a different interrupt.  The first patch
adds an error message so that any others that are discovered in the
future are easier to diagnose (this one took a while to reproduce and
identify).

Zev Weiss (3):
  aspeed-video: add error message for unhandled interrupts
  aspeed-video: clear spurious interrupt bits unconditionally
  aspeed-video: add COMP_READY to VE_SPURIOUS_IRQS

 drivers/media/platform/aspeed-video.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

-- 
2.29.2

[PATCH 1/3] aspeed-video: add error message for unhandled interrupts

[Zev Weiss]

This device seems to have a propensity for asserting interrupts that
aren't enabled -- in addition to the CAPTURE_COMPLETE and FRAME_COMPLETE
interrupts squashed in commit 65d270acb2d662c3346793663ac3a759eb4491b8,
COMP_READY has also been observed.  Adding a message diagnosing what
happened in the event of unhandled interrupt status bits should
hopefully make the debugging process simpler for any others that pop up
in the future.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/media/platform/aspeed-video.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 7d98db1d9b52..eb02043532e3 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -562,6 +562,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 {
 	struct aspeed_video *video = arg;
 	u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
+	u32 orig_sts = sts;
 
 	/*
 	 * Resolution changed or signal was lost; reset the engine and
@@ -639,6 +640,10 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 	if (sts & VE_INTERRUPT_FRAME_COMPLETE)
 		sts &= ~VE_INTERRUPT_FRAME_COMPLETE;
 
+	if (sts)
+		dev_err_ratelimited(video->dev, "unexpected interrupt asserted:"
+				    " sts=%08x, orig_sts=%08x", sts, orig_sts);
+
 	return sts ? IRQ_NONE : IRQ_HANDLED;
 }
 
-- 
2.29.2

[PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Zev Weiss]

Instead of testing and conditionally clearing them one by one, we can
instead just unconditionally clear them all at once.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/media/platform/aspeed-video.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index eb02043532e3..218aae3be809 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -558,6 +558,14 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
 	schedule_delayed_work(&video->res_work, delay);
 }
 
+/*
+ * Interrupts that we don't use but have to explicitly ignore because the
+ * hardware asserts them even when they're disabled in the VE_INTERRUPT_CTRL
+ * register.
+ */
+#define VE_SPURIOUS_IRQS \
+	(VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE)
+
 static irqreturn_t aspeed_video_irq(int irq, void *arg)
 {
 	struct aspeed_video *video = arg;
@@ -630,15 +638,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
 			aspeed_video_start_frame(video);
 	}
 
-	/*
-	 * CAPTURE_COMPLETE and FRAME_COMPLETE interrupts come even when these
-	 * are disabled in the VE_INTERRUPT_CTRL register so clear them to
-	 * prevent unnecessary interrupt calls.
-	 */
-	if (sts & VE_INTERRUPT_CAPTURE_COMPLETE)
-		sts &= ~VE_INTERRUPT_CAPTURE_COMPLETE;
-	if (sts & VE_INTERRUPT_FRAME_COMPLETE)
-		sts &= ~VE_INTERRUPT_FRAME_COMPLETE;
+	/* Squash known bogus interrupts */
+	sts &= ~VE_SPURIOUS_IRQS;
 
 	if (sts)
 		dev_err_ratelimited(video->dev, "unexpected interrupt asserted:"
-- 
2.29.2

[PATCH 3/3] aspeed-video: add COMP_READY to VE_SPURIOUS_IRQS

[Zev Weiss]

This joins CAPTURE_COMPLETE and FRAME_COMPLETE in the set of interrupts
that have been seen asserted by the hardware even when disabled, leading
to the interrupt eventually getting disabled as described in commit
65d270acb2d662c3346793663ac3a759eb4491b8.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/media/platform/aspeed-video.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 218aae3be809..48c52bf91a1b 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -564,7 +564,8 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
  * register.
  */
 #define VE_SPURIOUS_IRQS \
-	(VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE)
+	(VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE \
+	 | VE_INTERRUPT_COMP_READY)
 
 static irqreturn_t aspeed_video_irq(int irq, void *arg)
 {
-- 
2.29.2

Re: [PATCH 1/3] aspeed-video: add error message for unhandled interrupts

[Joel Stanley]

On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This device seems to have a propensity for asserting interrupts that
> aren't enabled -- in addition to the CAPTURE_COMPLETE and FRAME_COMPLETE
> interrupts squashed in commit 65d270acb2d662c3346793663ac3a759eb4491b8,
> COMP_READY has also been observed.  Adding a message diagnosing what
> happened in the event of unhandled interrupt status bits should
> hopefully make the debugging process simpler for any others that pop up
> in the future.

Ryan, is this a known issue with the video engine hardware?

>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/media/platform/aspeed-video.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index 7d98db1d9b52..eb02043532e3 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -562,6 +562,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>  {
>         struct aspeed_video *video = arg;
>         u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
> +       u32 orig_sts = sts;
>
>         /*
>          * Resolution changed or signal was lost; reset the engine and
> @@ -639,6 +640,10 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>         if (sts & VE_INTERRUPT_FRAME_COMPLETE)
>                 sts &= ~VE_INTERRUPT_FRAME_COMPLETE;
>
> +       if (sts)
> +               dev_err_ratelimited(video->dev, "unexpected interrupt asserted:"
> +                                   " sts=%08x, orig_sts=%08x", sts, orig_sts);

Do you want to do this before clearing the FRAME and CAPTURE bits?

> +
>         return sts ? IRQ_NONE : IRQ_HANDLED;
>  }
>
> --
> 2.29.2
>

Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Joel Stanley]

On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> Instead of testing and conditionally clearing them one by one, we can
> instead just unconditionally clear them all at once.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

I had a poke at the assembly and it looks like GCC is clearing the
bits unconditionally anyway, so removing the tests provides no change.

Combining them is a good further optimization.

Reviewed-by: Joel Stanley <joel@jms.id.au>

A question unrelated to this patch: Do you know why the driver doesn't
clear the status bits in the interrupt handler? I would expect it to
write the value of sts back to the register to ack the pending
interrupt.

> ---
>  drivers/media/platform/aspeed-video.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index eb02043532e3..218aae3be809 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -558,6 +558,14 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
>         schedule_delayed_work(&video->res_work, delay);
>  }
>
> +/*
> + * Interrupts that we don't use but have to explicitly ignore because the
> + * hardware asserts them even when they're disabled in the VE_INTERRUPT_CTRL
> + * register.
> + */
> +#define VE_SPURIOUS_IRQS \
> +       (VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE)
> +
>  static irqreturn_t aspeed_video_irq(int irq, void *arg)
>  {
>         struct aspeed_video *video = arg;
> @@ -630,15 +638,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>                         aspeed_video_start_frame(video);
>         }
>
> -       /*
> -        * CAPTURE_COMPLETE and FRAME_COMPLETE interrupts come even when these
> -        * are disabled in the VE_INTERRUPT_CTRL register so clear them to
> -        * prevent unnecessary interrupt calls.
> -        */
> -       if (sts & VE_INTERRUPT_CAPTURE_COMPLETE)
> -               sts &= ~VE_INTERRUPT_CAPTURE_COMPLETE;
> -       if (sts & VE_INTERRUPT_FRAME_COMPLETE)
> -               sts &= ~VE_INTERRUPT_FRAME_COMPLETE;
> +       /* Squash known bogus interrupts */
> +       sts &= ~VE_SPURIOUS_IRQS;
>
>         if (sts)
>                 dev_err_ratelimited(video->dev, "unexpected interrupt asserted:"
> --
> 2.29.2
>

Re: [PATCH 3/3] aspeed-video: add COMP_READY to VE_SPURIOUS_IRQS

[Joel Stanley]

On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This joins CAPTURE_COMPLETE and FRAME_COMPLETE in the set of interrupts
> that have been seen asserted by the hardware even when disabled, leading
> to the interrupt eventually getting disabled as described in commit
> 65d270acb2d662c3346793663ac3a759eb4491b8.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

I have less experience with this part of the chip, so I defer to Jae
or Ryan for an ack.

> ---
>  drivers/media/platform/aspeed-video.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index 218aae3be809..48c52bf91a1b 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -564,7 +564,8 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
>   * register.
>   */
>  #define VE_SPURIOUS_IRQS \
> -       (VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE)
> +       (VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE \
> +        | VE_INTERRUPT_COMP_READY)
>
>  static irqreturn_t aspeed_video_irq(int irq, void *arg)
>  {
> --
> 2.29.2
>

Re: [PATCH 1/3] aspeed-video: add error message for unhandled interrupts

[Zev Weiss]

On Mon, Dec 21, 2020 at 10:34:26PM CST, Joel Stanley wrote:
>On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  drivers/media/platform/aspeed-video.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>> index 7d98db1d9b52..eb02043532e3 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -562,6 +562,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>>  {
>>         struct aspeed_video *video = arg;
>>         u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
>> +       u32 orig_sts = sts;
>>
>>         /*
>>          * Resolution changed or signal was lost; reset the engine and
>> @@ -639,6 +640,10 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>>         if (sts & VE_INTERRUPT_FRAME_COMPLETE)
>>                 sts &= ~VE_INTERRUPT_FRAME_COMPLETE;
>>
>> +       if (sts)
>> +               dev_err_ratelimited(video->dev, "unexpected interrupt asserted:"
>> +                                   " sts=%08x, orig_sts=%08x", sts, orig_sts);
>
>Do you want to do this before clearing the FRAME and CAPTURE bits?
>

My intent was to only issue the message for unexpectedly-asserted 
interrupts that aren't among the ones already known to happen despite 
being disabled -- basically just indicating that a new bit might need to 
be added to the spurious-interrupt mask added in the second patch.  (I 
included the orig_sts element in case there's any useful debugging 
information to be gleaned from what other interrupts got asserted along 
with it, which would also include FRAME, CAPTURE, and any others 
explicitly cleared.)

And incidentally, in the handful of instances I captured in which this 
problem arose, it seemed to be "sticky" in that it continued occurring 
on every frame until the device was reset, so it seems like it would be 
likely to lead to a fair amount of log spam for a condition where it's 
basically just "we're ignoring known misbehavior" and there's not much 
else to do about it.


Zev

Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Zev Weiss]

On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote:
>On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> Instead of testing and conditionally clearing them one by one, we can
>> instead just unconditionally clear them all at once.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>
>I had a poke at the assembly and it looks like GCC is clearing the
>bits unconditionally anyway, so removing the tests provides no change.
>
>Combining them is a good further optimization.
>
>Reviewed-by: Joel Stanley <joel@jms.id.au>
>
>A question unrelated to this patch: Do you know why the driver doesn't
>clear the status bits in the interrupt handler? I would expect it to
>write the value of sts back to the register to ack the pending
>interrupt.
>

No, I don't, and I was sort of wondering the same thing actually -- I'm 
not deeply familiar with this hardware or driver though, so I was a bit 
hesitant to start messing with things.  (Though maybe doing so would 
address the "stickiness" aspect when it does manifest.)  Perhaps Eddie 
or Jae can shed some light here?


Zev

Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Joel Stanley]

On Tue, 22 Dec 2020 at 19:14, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote:
> >On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
> >>
> >> Instead of testing and conditionally clearing them one by one, we can
> >> instead just unconditionally clear them all at once.
> >>
> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >
> >I had a poke at the assembly and it looks like GCC is clearing the
> >bits unconditionally anyway, so removing the tests provides no change.
> >
> >Combining them is a good further optimization.
> >
> >Reviewed-by: Joel Stanley <joel@jms.id.au>
> >
> >A question unrelated to this patch: Do you know why the driver doesn't
> >clear the status bits in the interrupt handler? I would expect it to
> >write the value of sts back to the register to ack the pending
> >interrupt.
> >
>
> No, I don't, and I was sort of wondering the same thing actually -- I'm
> not deeply familiar with this hardware or driver though, so I was a bit
> hesitant to start messing with things.  (Though maybe doing so would
> address the "stickiness" aspect when it does manifest.)  Perhaps Eddie
> or Jae can shed some light here?

I think you're onto something here - this would be why the status bits
seem to stick until the device is reset.

Until Aspeed can clarify if this is a hardware or software issue, I
suggest we ack the bits and log a message when we see them, instead of
always ignoring them without taking any action.

Can you write a patch that changes the interrupt handler to ack status
bits as it handles each of them?

>
>
> Zev
>

RE: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Ryan Chen]

> -----Original Message-----
> From: Joel Stanley <joel@jms.id.au>
> Sent: Wednesday, December 23, 2020 9:07 AM
> To: Zev Weiss <zev@bewilderbeest.net>; Ryan Chen
> <ryan_chen@aspeedtech.com>
> Cc: Eddie James <eajames@linux.ibm.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Andrew Jeffery <andrew@aj.id.au>;
> linux-media@vger.kernel.org; OpenBMC Maillist <openbmc@lists.ozlabs.org>;
> Linux ARM <linux-arm-kernel@lists.infradead.org>; linux-aspeed
> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits
> unconditionally
> 
> On Tue, 22 Dec 2020 at 19:14, Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> > On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote:
> > >On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
> > >>
> > >> Instead of testing and conditionally clearing them one by one, we
> > >> can instead just unconditionally clear them all at once.
> > >>
> > >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> > >
> > >I had a poke at the assembly and it looks like GCC is clearing the
> > >bits unconditionally anyway, so removing the tests provides no change.
> > >
> > >Combining them is a good further optimization.
> > >
> > >Reviewed-by: Joel Stanley <joel@jms.id.au>
> > >
> > >A question unrelated to this patch: Do you know why the driver
> > >doesn't clear the status bits in the interrupt handler? I would
> > >expect it to write the value of sts back to the register to ack the
> > >pending interrupt.
> > >
> >
> > No, I don't, and I was sort of wondering the same thing actually --
> > I'm not deeply familiar with this hardware or driver though, so I was
> > a bit hesitant to start messing with things.  (Though maybe doing so
> > would address the "stickiness" aspect when it does manifest.)  Perhaps
> > Eddie or Jae can shed some light here?
> 
> I think you're onto something here - this would be why the status bits seem to
> stick until the device is reset.
> 
> Until Aspeed can clarify if this is a hardware or software issue, I suggest we ack
> the bits and log a message when we see them, instead of always ignoring them
> without taking any action.
> 
> Can you write a patch that changes the interrupt handler to ack status bits as it
> handles each of them?
> 
Hello Zev, before the patch, do you met issue with irq handler? [continuous incoming?]

In aspeed_video_irq handler should only handle enable interrupt expected.
   u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
 + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);

Ryan

> >
> >
> > Zev
> >

Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Zev Weiss]

On Tue, Dec 22, 2020 at 08:53:33PM CST, Ryan Chen wrote:
>> -----Original Message-----
>> From: Joel Stanley <joel@jms.id.au>
>> Sent: Wednesday, December 23, 2020 9:07 AM
>> To: Zev Weiss <zev@bewilderbeest.net>; Ryan Chen
>> <ryan_chen@aspeedtech.com>
>> Cc: Eddie James <eajames@linux.ibm.com>; Mauro Carvalho Chehab
>> <mchehab@kernel.org>; Andrew Jeffery <andrew@aj.id.au>;
>> linux-media@vger.kernel.org; OpenBMC Maillist <openbmc@lists.ozlabs.org>;
>> Linux ARM <linux-arm-kernel@lists.infradead.org>; linux-aspeed
>> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
>> <linux-kernel@vger.kernel.org>; Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits
>> unconditionally
>>
>> On Tue, 22 Dec 2020 at 19:14, Zev Weiss <zev@bewilderbeest.net> wrote:
>> >
>> > On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote:
>> > >On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>> > >>
>> > >> Instead of testing and conditionally clearing them one by one, we
>> > >> can instead just unconditionally clear them all at once.
>> > >>
>> > >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> > >
>> > >I had a poke at the assembly and it looks like GCC is clearing the
>> > >bits unconditionally anyway, so removing the tests provides no change.
>> > >
>> > >Combining them is a good further optimization.
>> > >
>> > >Reviewed-by: Joel Stanley <joel@jms.id.au>
>> > >
>> > >A question unrelated to this patch: Do you know why the driver
>> > >doesn't clear the status bits in the interrupt handler? I would
>> > >expect it to write the value of sts back to the register to ack the
>> > >pending interrupt.
>> > >
>> >
>> > No, I don't, and I was sort of wondering the same thing actually --
>> > I'm not deeply familiar with this hardware or driver though, so I was
>> > a bit hesitant to start messing with things.  (Though maybe doing so
>> > would address the "stickiness" aspect when it does manifest.)  Perhaps
>> > Eddie or Jae can shed some light here?
>>
>> I think you're onto something here - this would be why the status bits seem to
>> stick until the device is reset.
>>
>> Until Aspeed can clarify if this is a hardware or software issue, I suggest we ack
>> the bits and log a message when we see them, instead of always ignoring them
>> without taking any action.
>>
>> Can you write a patch that changes the interrupt handler to ack status bits as it
>> handles each of them?
>>
>Hello Zev, before the patch, do you met issue with irq handler? [continuous incoming?]
>
>In aspeed_video_irq handler should only handle enable interrupt expected.
>   u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
> + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);
>
>Ryan
>

Hi Ryan,

Prior to any of these patches I encountered a problem pretty much 
exactly like what Jae described in his commit message in 65d270acb2d 
(but the kernel I was running included that patch).  Adding the 
diagnostic in patch #1 of this series showed that it was apparently the 
same problem, just with a different interrupt that Jae's patch didn't 
include.

 From what you wrote above, I gather that it is in fact expected for the 
hardware to assert interrupts that aren't enabled in VE_INTERRUPT_CTRL?  
If so, I guess something like that would obviate the need for both Jae's 
earlier patch and this whole series.

I think the question Joel raised is somewhat independent though -- if 
the VE_INTERRUPT_STATUS register asserts interrupts we're not actually 
using, should the driver acknowledge them anyway or just leave them 
alone?  (Though if we're just going to ignore them anyway maybe it 
doesn't ultimately matter very much.)


Zev

RE: [PATCH 2/3] aspeed-video: clear spurious interrupt bits unconditionally

[Ryan Chen]

> -----Original Message-----
> From: Zev Weiss <zev@bewilderbeest.net>
> Sent: Wednesday, December 23, 2020 11:54 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>
> Cc: Joel Stanley <joel@jms.id.au>; Eddie James <eajames@linux.ibm.com>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Andrew Jeffery
> <andrew@aj.id.au>; linux-media@vger.kernel.org; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>; Linux ARM
> <linux-arm-kernel@lists.infradead.org>; linux-aspeed
> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits
> unconditionally
> 
> On Tue, Dec 22, 2020 at 08:53:33PM CST, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: Joel Stanley <joel@jms.id.au>
> >> Sent: Wednesday, December 23, 2020 9:07 AM
> >> To: Zev Weiss <zev@bewilderbeest.net>; Ryan Chen
> >> <ryan_chen@aspeedtech.com>
> >> Cc: Eddie James <eajames@linux.ibm.com>; Mauro Carvalho Chehab
> >> <mchehab@kernel.org>; Andrew Jeffery <andrew@aj.id.au>;
> >> linux-media@vger.kernel.org; OpenBMC Maillist
> >> <openbmc@lists.ozlabs.org>; Linux ARM
> >> <linux-arm-kernel@lists.infradead.org>; linux-aspeed
> >> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
> >> <linux-kernel@vger.kernel.org>; Jae Hyun Yoo
> >> <jae.hyun.yoo@linux.intel.com>
> >> Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits
> >> unconditionally
> >>
> >> On Tue, 22 Dec 2020 at 19:14, Zev Weiss <zev@bewilderbeest.net> wrote:
> >> >
> >> > On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote:
> >> > >On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net>
> wrote:
> >> > >>
> >> > >> Instead of testing and conditionally clearing them one by one,
> >> > >> we can instead just unconditionally clear them all at once.
> >> > >>
> >> > >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >> > >
> >> > >I had a poke at the assembly and it looks like GCC is clearing the
> >> > >bits unconditionally anyway, so removing the tests provides no change.
> >> > >
> >> > >Combining them is a good further optimization.
> >> > >
> >> > >Reviewed-by: Joel Stanley <joel@jms.id.au>
> >> > >
> >> > >A question unrelated to this patch: Do you know why the driver
> >> > >doesn't clear the status bits in the interrupt handler? I would
> >> > >expect it to write the value of sts back to the register to ack
> >> > >the pending interrupt.
> >> > >
> >> >
> >> > No, I don't, and I was sort of wondering the same thing actually --
> >> > I'm not deeply familiar with this hardware or driver though, so I
> >> > was a bit hesitant to start messing with things.  (Though maybe
> >> > doing so would address the "stickiness" aspect when it does
> >> > manifest.)  Perhaps Eddie or Jae can shed some light here?
> >>
> >> I think you're onto something here - this would be why the status
> >> bits seem to stick until the device is reset.
> >>
> >> Until Aspeed can clarify if this is a hardware or software issue, I
> >> suggest we ack the bits and log a message when we see them, instead
> >> of always ignoring them without taking any action.
> >>
> >> Can you write a patch that changes the interrupt handler to ack
> >> status bits as it handles each of them?
> >>
> >Hello Zev, before the patch, do you met issue with irq handler?
> >[continuous incoming?]
> >
> >In aspeed_video_irq handler should only handle enable interrupt expected.
> >   u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
> > + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);
> >
> >Ryan
> >
> 
> Hi Ryan,
> 
> Prior to any of these patches I encountered a problem pretty much exactly like
> what Jae described in his commit message in 65d270acb2d (but the kernel I
> was running included that patch).  Adding the diagnostic in patch #1 of this
> series showed that it was apparently the same problem, just with a different
> interrupt that Jae's patch didn't include.
> 
>  From what you wrote above, I gather that it is in fact expected for the
> hardware to assert interrupts that aren't enabled in VE_INTERRUPT_CTRL?
> If so, I guess something like that would obviate the need for both Jae's earlier
> patch and this whole series.
> 
Yes, I expected handle enabled in VE_INTERRUPT_CTRL. 

> I think the question Joel raised is somewhat independent though -- if the
> VE_INTERRUPT_STATUS register asserts interrupts we're not actually using,
> should the driver acknowledge them anyway or just leave them alone?
My opinion will keep them alone, ignore them.

> (Though if we're just going to ignore them anyway maybe it doesn't ultimately
> matter very much.)
> 
> 
> Zev

Re: [PATCH 3/3] aspeed-video: add COMP_READY to VE_SPURIOUS_IRQS

[Jae Hyun Yoo]

On 12/21/2020 8:49 PM, Joel Stanley wrote:
> On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> This joins CAPTURE_COMPLETE and FRAME_COMPLETE in the set of interrupts
>> that have been seen asserted by the hardware even when disabled, leading
>> to the interrupt eventually getting disabled as described in commit
>> 65d270acb2d662c3346793663ac3a759eb4491b8.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> 
> I have less experience with this part of the chip, so I defer to Jae
> or Ryan for an ack.

I didn't see unexpected VE_INTERRUPT_COMP_READY events in my system but
it seems a fix good to have.

Acked-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

>> ---
>>   drivers/media/platform/aspeed-video.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>> index 218aae3be809..48c52bf91a1b 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -564,7 +564,8 @@ static void aspeed_video_irq_res_change(struct aspeed_video *video, ulong delay)
>>    * register.
>>    */
>>   #define VE_SPURIOUS_IRQS \
>> -       (VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE)
>> +       (VE_INTERRUPT_CAPTURE_COMPLETE | VE_INTERRUPT_FRAME_COMPLETE \
>> +        | VE_INTERRUPT_COMP_READY)
>>
>>   static irqreturn_t aspeed_video_irq(int irq, void *arg)
>>   {
>> --
>> 2.29.2
>>