diff mbox series

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

Message ID 20201215024542.18888-2-zev@bewilderbeest.net (mailing list archive)
State New, archived
Headers show
Series aspeed-video: extend spurious interrupt handling | expand

Commit Message

Zev Weiss Dec. 15, 2020, 2:45 a.m. UTC
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(+)

Comments

Joel Stanley Dec. 22, 2020, 4:34 a.m. UTC | #1
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
>
Zev Weiss Dec. 22, 2020, 7:11 p.m. UTC | #2
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
diff mbox series

Patch

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;
 }