On Thu, 2021-06-17 at 17:02 -0500, Zev Weiss wrote:
> As partially addressed in commit 65d270acb2d6 ("media: aspeed: clear
> garbage interrupts"), the ASpeed video engine sometimes asserts
> interrupts that the driver hasn't enabled. In addition to the
> CAPTURE_COMPLETE and FRAME_COMPLETE interrupts dealt with in that
> patch, COMP_READY has also been observed. Instead of playing
> whack-a-mole with each one individually, we can instead just blanket
> ignore everything we haven't explicitly enabled.
Suspect this will fix an intermittent problem on AST2500 with
screensaver. Change looks good, thanks!
Reviewed-by: Eddie James <[email protected]>
>
> Signed-off-by: Zev Weiss <[email protected]>
> ---
>
> Changes since v2 [1]:
> - minor commit message improvements
>
> Changes since v1 [0]:
> - dropped error message
> - switched to a blanket-ignore approach as suggested by Ryan
>
> [0]
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
> [1]
> https://lore.kernel.org/openbmc/[email protected]/
>
> drivers/media/platform/aspeed-video.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c
> b/drivers/media/platform/aspeed-video.c
> index 7bb6babdcade..77611c296a25 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -563,6 +563,12 @@ static irqreturn_t aspeed_video_irq(int irq,
> void *arg)
> struct aspeed_video *video = arg;
> u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
>
> + /*
> + * Hardware sometimes asserts interrupts that we haven't
> actually
> + * enabled; ignore them if so.
> + */
> + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);
> +
> /*
> * Resolution changed or signal was lost; reset the engine and
> * re-initialize
> @@ -629,16 +635,6 @@ 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;
> -
> return sts ? IRQ_NONE : IRQ_HANDLED;
> }
>
> -----Original Message-----
> From: Eddie James <[email protected]>
> Sent: Wednesday, September 1, 2021 11:07 PM
> To: Zev Weiss <[email protected]>
> Cc: Jae Hyun Yoo <[email protected]>; Ryan Chen
> <[email protected]>; [email protected]; Andrew Jeffery
> <[email protected]>; [email protected]; [email protected];
> Mauro Carvalho Chehab <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH v3] media: aspeed-video: ignore interrupts that aren't
> enabled
>
> On Thu, 2021-06-17 at 17:02 -0500, Zev Weiss wrote:
> > As partially addressed in commit 65d270acb2d6 ("media: aspeed: clear
> > garbage interrupts"), the ASpeed video engine sometimes asserts
> > interrupts that the driver hasn't enabled. In addition to the
> > CAPTURE_COMPLETE and FRAME_COMPLETE interrupts dealt with in that
> > patch, COMP_READY has also been observed. Instead of playing
> > whack-a-mole with each one individually, we can instead just blanket
> > ignore everything we haven't explicitly enabled.
>
> Suspect this will fix an intermittent problem on AST2500 with screensaver.
> Change looks good, thanks!
>
> Reviewed-by: Eddie James <[email protected]>
>
Reviewed-by: Ryan Chen <[email protected]>
> >
> > Signed-off-by: Zev Weiss <[email protected]>
> > ---
> >
> > Changes since v2 [1]:
> > - minor commit message improvements
> >
> > Changes since v1 [0]:
> > - dropped error message
> > - switched to a blanket-ignore approach as suggested by Ryan
> >
> > [0]
> > https://lore.kernel.org/linux-arm-kernel/20201215024542.18888-1-zev@be
> > wilderbeest.net/
> > [1]
> >
> https://lore.kernel.org/openbmc/20210506234048.3214-1-zev@bewilderbees
> > t.net/
> >
> > drivers/media/platform/aspeed-video.c | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/platform/aspeed-video.c
> > b/drivers/media/platform/aspeed-video.c
> > index 7bb6babdcade..77611c296a25 100644
> > --- a/drivers/media/platform/aspeed-video.c
> > +++ b/drivers/media/platform/aspeed-video.c
> > @@ -563,6 +563,12 @@ static irqreturn_t aspeed_video_irq(int irq, void
> > *arg)
> > struct aspeed_video *video = arg;
> > u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
> >
> > + /*
> > + * Hardware sometimes asserts interrupts that we haven't
> > actually
> > + * enabled; ignore them if so.
> > + */
> > + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);
> > +
> > /*
> > * Resolution changed or signal was lost; reset the engine and
> > * re-initialize
> > @@ -629,16 +635,6 @@ 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;
> > -
> > return sts ? IRQ_NONE : IRQ_HANDLED; }
> >
Hi Mauro,
On Mon, 6 Sept 2021 at 10:10, Ryan Chen <[email protected]> wrote:
> > Subject: Re: [PATCH v3] media: aspeed-video: ignore interrupts that aren't
> > enabled
> >
> > On Thu, 2021-06-17 at 17:02 -0500, Zev Weiss wrote:
> > > As partially addressed in commit 65d270acb2d6 ("media: aspeed: clear
> > > garbage interrupts"), the ASpeed video engine sometimes asserts
> > > interrupts that the driver hasn't enabled. In addition to the
> > > CAPTURE_COMPLETE and FRAME_COMPLETE interrupts dealt with in that
> > > patch, COMP_READY has also been observed. Instead of playing
> > > whack-a-mole with each one individually, we can instead just blanket
> > > ignore everything we haven't explicitly enabled.
> >
> > Suspect this will fix an intermittent problem on AST2500 with screensaver.
> > Change looks good, thanks!
> >
> > Reviewed-by: Eddie James <[email protected]>
> >
> Reviewed-by: Ryan Chen <[email protected]>
> > >
> > > Signed-off-by: Zev Weiss <[email protected]>
I notice this wasn't merged in the pull request you sent for v5.15.
Would you be able to ensure it makes it in the next pull request you
send?
It can have some fixes tags too:
Fixes: 65d270acb2d6 ("media: aspeed: clear garbage interrupts")
Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
Acked-by: Joel Stanley <[email protected]>
Cheers,
Joel
> > > ---
> > >
> > > Changes since v2 [1]:
> > > - minor commit message improvements
> > >
> > > Changes since v1 [0]:
> > > - dropped error message
> > > - switched to a blanket-ignore approach as suggested by Ryan
> > >
> > > [0]
> > > https://lore.kernel.org/linux-arm-kernel/20201215024542.18888-1-zev@be
> > > wilderbeest.net/
> > > [1]
> > >
> > https://lore.kernel.org/openbmc/20210506234048.3214-1-zev@bewilderbees
> > > t.net/
> > >
> > > drivers/media/platform/aspeed-video.c | 16 ++++++----------
> > > 1 file changed, 6 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/aspeed-video.c
> > > b/drivers/media/platform/aspeed-video.c
> > > index 7bb6babdcade..77611c296a25 100644
> > > --- a/drivers/media/platform/aspeed-video.c
> > > +++ b/drivers/media/platform/aspeed-video.c
> > > @@ -563,6 +563,12 @@ static irqreturn_t aspeed_video_irq(int irq, void
> > > *arg)
> > > struct aspeed_video *video = arg;
> > > u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
> > >
> > > + /*
> > > + * Hardware sometimes asserts interrupts that we haven't
> > > actually
> > > + * enabled; ignore them if so.
> > > + */
> > > + sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);
> > > +
> > > /*
> > > * Resolution changed or signal was lost; reset the engine and
> > > * re-initialize
> > > @@ -629,16 +635,6 @@ 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;
> > > -
> > > return sts ? IRQ_NONE : IRQ_HANDLED; }
> > >
>
On Mon, Sep 06, 2021 at 04:47:14AM PDT, Joel Stanley wrote:
>Hi Mauro,
>
>On Mon, 6 Sept 2021 at 10:10, Ryan Chen <[email protected]> wrote:
>> > Subject: Re: [PATCH v3] media: aspeed-video: ignore interrupts that aren't
>> > enabled
>> >
>> > On Thu, 2021-06-17 at 17:02 -0500, Zev Weiss wrote:
>> > > As partially addressed in commit 65d270acb2d6 ("media: aspeed: clear
>> > > garbage interrupts"), the ASpeed video engine sometimes asserts
>> > > interrupts that the driver hasn't enabled. In addition to the
>> > > CAPTURE_COMPLETE and FRAME_COMPLETE interrupts dealt with in that
>> > > patch, COMP_READY has also been observed. Instead of playing
>> > > whack-a-mole with each one individually, we can instead just blanket
>> > > ignore everything we haven't explicitly enabled.
>> >
>> > Suspect this will fix an intermittent problem on AST2500 with screensaver.
>> > Change looks good, thanks!
>> >
>> > Reviewed-by: Eddie James <[email protected]>
>> >
>> Reviewed-by: Ryan Chen <[email protected]>
>> > >
>> > > Signed-off-by: Zev Weiss <[email protected]>
>
>I notice this wasn't merged in the pull request you sent for v5.15.
>Would you be able to ensure it makes it in the next pull request you
>send?
>
>It can have some fixes tags too:
>
>Fixes: 65d270acb2d6 ("media: aspeed: clear garbage interrupts")
>Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
>Acked-by: Joel Stanley <[email protected]>
>
>Cheers,
>
>Joel
>
Also not in the recent merge of the media/v5.15-2 tag it looks like -- ping?
Thanks,
Zev
Hi Greg,
On Tue, 28 Sept 2021 at 18:57, Zev Weiss <[email protected]> wrote:
>
> On Mon, Sep 06, 2021 at 04:47:14AM PDT, Joel Stanley wrote:
> >Hi Mauro,
> >
> >On Mon, 6 Sept 2021 at 10:10, Ryan Chen <[email protected]> wrote:
> >> > Subject: Re: [PATCH v3] media: aspeed-video: ignore interrupts that aren't
> >> > enabled
> >> >
> >> > On Thu, 2021-06-17 at 17:02 -0500, Zev Weiss wrote:
> >> > > As partially addressed in commit 65d270acb2d6 ("media: aspeed: clear
> >> > > garbage interrupts"), the ASpeed video engine sometimes asserts
> >> > > interrupts that the driver hasn't enabled. In addition to the
> >> > > CAPTURE_COMPLETE and FRAME_COMPLETE interrupts dealt with in that
> >> > > patch, COMP_READY has also been observed. Instead of playing
> >> > > whack-a-mole with each one individually, we can instead just blanket
> >> > > ignore everything we haven't explicitly enabled.
> >> >
> >> > Suspect this will fix an intermittent problem on AST2500 with screensaver.
> >> > Change looks good, thanks!
> >> >
> >> > Reviewed-by: Eddie James <[email protected]>
> >> >
> >> Reviewed-by: Ryan Chen <[email protected]>
> >> > >
> >> > > Signed-off-by: Zev Weiss <[email protected]>
> >
> >I notice this wasn't merged in the pull request you sent for v5.15.
> >Would you be able to ensure it makes it in the next pull request you
> >send?
> >
> >It can have some fixes tags too:
> >
> >Fixes: 65d270acb2d6 ("media: aspeed: clear garbage interrupts")
> >Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
> >Acked-by: Joel Stanley <[email protected]>
> >
> >Cheers,
> >
> >Joel
> >
>
> Also not in the recent merge of the media/v5.15-2 tag it looks like -- ping?
We're struggling to get a hold of the media maintainers to merge this
fix. Have you heard from them recently?
Cheers,
Joel
On Fri, Oct 08, 2021 at 04:43:24AM +0000, Joel Stanley wrote:
> Hi Greg,
>
> On Tue, 28 Sept 2021 at 18:57, Zev Weiss <[email protected]> wrote:
> >
> > On Mon, Sep 06, 2021 at 04:47:14AM PDT, Joel Stanley wrote:
> > >Hi Mauro,
> > >
> > >On Mon, 6 Sept 2021 at 10:10, Ryan Chen <[email protected]> wrote:
> > >> > Subject: Re: [PATCH v3] media: aspeed-video: ignore interrupts that aren't
> > >> > enabled
> > >> >
> > >> > On Thu, 2021-06-17 at 17:02 -0500, Zev Weiss wrote:
> > >> > > As partially addressed in commit 65d270acb2d6 ("media: aspeed: clear
> > >> > > garbage interrupts"), the ASpeed video engine sometimes asserts
> > >> > > interrupts that the driver hasn't enabled. In addition to the
> > >> > > CAPTURE_COMPLETE and FRAME_COMPLETE interrupts dealt with in that
> > >> > > patch, COMP_READY has also been observed. Instead of playing
> > >> > > whack-a-mole with each one individually, we can instead just blanket
> > >> > > ignore everything we haven't explicitly enabled.
> > >> >
> > >> > Suspect this will fix an intermittent problem on AST2500 with screensaver.
> > >> > Change looks good, thanks!
> > >> >
> > >> > Reviewed-by: Eddie James <[email protected]>
> > >> >
> > >> Reviewed-by: Ryan Chen <[email protected]>
> > >> > >
> > >> > > Signed-off-by: Zev Weiss <[email protected]>
> > >
> > >I notice this wasn't merged in the pull request you sent for v5.15.
> > >Would you be able to ensure it makes it in the next pull request you
> > >send?
> > >
> > >It can have some fixes tags too:
> > >
> > >Fixes: 65d270acb2d6 ("media: aspeed: clear garbage interrupts")
> > >Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
> > >Acked-by: Joel Stanley <[email protected]>
> > >
> > >Cheers,
> > >
> > >Joel
> > >
> >
> > Also not in the recent merge of the media/v5.15-2 tag it looks like -- ping?
>
> We're struggling to get a hold of the media maintainers to merge this
> fix. Have you heard from them recently?
I have seen them on the mailing list, yes.
good luck!
greg k-h