Re: [PATCH dev-5.0 4/4] media: aspeed: clear interrupt status flags immediately
Andrew Jeffery
andrew at aj.id.au
Wed May 1 11:29:14 AEST 2019
On Wed, 1 May 2019, at 05:30, Jae Hyun Yoo wrote:
> On 4/29/2019 11:01 PM, Joel Stanley wrote:
> > On Tue, 30 Apr 2019 at 03:04, Joel Stanley <joel at jms.id.au> wrote:
> >>
> >> On Mon, 29 Apr 2019 at 23:38, Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com> wrote:
> >>>
> >>> On 4/29/2019 3:29 PM, Eddie James wrote:
> >>>>
> >>>> On 4/25/19 5:20 PM, Jae Hyun Yoo wrote:
> >>>>> Interrupt status flags should be cleared immediately otherwise
> >>>>> interrupt handler will be called again and again until the flag
> >>>>> is cleared, but this driver clears some flags through a 500ms
> >>>>> delayed work which is a bad idea in interrupt handling, so this
> >>>>> commit makes the interrupt handler clear the status flags
> >>>>> immediately.
> >>>>>
> >>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com>
> >>>>> ---
> >>>>> drivers/media/platform/aspeed-video.c | 12 +++++++-----
> >>>>> 1 file changed, 7 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/platform/aspeed-video.c
> >>>>> b/drivers/media/platform/aspeed-video.c
> >>>>> index 77c209a472ca..e218f375b9f5 100644
> >>>>> --- a/drivers/media/platform/aspeed-video.c
> >>>>> +++ b/drivers/media/platform/aspeed-video.c
> >>>>> @@ -546,17 +546,18 @@ static irqreturn_t aspeed_video_irq(int irq,
> >>>>> void *arg)
> >>>>> * re-initialize
> >>>>> */
> >>>>> if (sts & VE_INTERRUPT_MODE_DETECT_WD) {
> >>>>> + aspeed_video_write(video, VE_INTERRUPT_STATUS,
> >>>>> + VE_INTERRUPT_MODE_DETECT_WD);
> >>>>
> >>>>
> >>>> aspeed_video_irq_res_change disables all IRQs and turns off the clocks.
> >>>> This shouldn't be necessary.
> >>>
> >>> In fact, this patch fixes a watch dog reset with printing out a stack
> >>> trace like below. This happens very rarely but it's critical because it
> >>> causes a BMC reset. In my experiments, interrupt flags should be cleared
> >>> even with the aspeed_video_write(video, VE_INTERRUPT_CTRL, 0) in
> >>> aspeed_video_off(), or we should add
> >>> apeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff)
> >>> before the disabling interrupt. I think the way in this patch is better.
> >>
> >> In general, a driver should certainly be clearing (acking) the
> >> interrupt bits in the interrupt handler before returning.
> >>
> >> Jae, it would be interesting to know the value of VE_INTERRUPT_STATUS
> >> in the soft lockup situation.
> >>
> >> I took a closer look at this function, and it should probably not
> >> return IRQ_HANDLED at the bottom, as it may have fallen through all of
> >> the if statements and not have handled any interrupt. Jae, can you
> >> take a look at this and send another patch if you think that is
> >> correct.
> >
> > Something like the patch below. It also made me wonder why you don't
> > return from the flags = VIDEO_RES_DETECT state?
>
> Issue is caused because VE_INTERRUPT_MODE_DETECT_WD or
> VE_INTERRUPT_MODE_DETECT is cleared by a 500ms delayed work when the
> handler calls aspeed_video_irq_res_change(). As Eddie said, it disables
> interrupt through aspeed_video_off() but it doesn't mean that the
> interrupt bit is cleared at that timing. Actually, the interrupt bit
> is cleared by aspeed_video_resolution_work() 500ms after it gets the
> interrupt request so this patch is going to clear the bits immediately.
>
> > I don't have a way to test. Is there a simple command I can run on a
> > BMC to test, or do I need the entire stack?
>
> The watch dog reset issue happens when video mode change is occurred. I
> use Ubuntu GUI mode and set screen saver to blank screen with 1 minute
> expiration time. And then, open bmcweb page and navigate to Server
> control -> KVM page. As long as a user stays in this page, KVM video
> will be off by the screen saver and then it will be awaken by KVM
> service so this sequence will be repeated infinitely. I usually see the
> issue at least once if I put my system under this overnight stress test.
>
> > --- a/drivers/media/platform/aspeed-video.c
> > +++ b/drivers/media/platform/aspeed-video.c
> > @@ -566,8 +566,8 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
> > * detection; reset the engine and re-initialize
> > */
> > aspeed_video_irq_res_change(video);
> > - return IRQ_HANDLED;
> > }
> > + return IRQ_HANDLED;
> > }
> >
> > if (sts & VE_INTERRUPT_COMP_COMPLETE) {
> > @@ -606,9 +606,13 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
> >
> > if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
> > aspeed_video_start_frame(video);
> > +
> > + return IRQ_HANDLED;
> > }
> >
> > - return IRQ_HANDLED;
> > + dev_dbg(video->dev, "unhandled states: %08x\n", sts);
> > +
> > + return IRQ_NONE;
> > }
> >
>
> Looks good but we need to consider cases that needs going thru all
> if statements other than cases call aspeed_video_irq_res_change().
>
> I made a new patch like below. It has worked well so far. Will submit
> v2 after making sufficient test using it.
>
>
> diff --git a/drivers/media/platform/aspeed-video.c
> b/drivers/media/platform/aspeed-video.c
> index 77c209a472ca..8096319733ee 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -488,6 +488,7 @@ static void aspeed_video_off(struct aspeed_video *video)
>
> /* Disable interrupts */
> aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
> + aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xffffffff);
>
> /* Turn off the relevant clocks */
> clk_disable(video->vclk);
> @@ -539,24 +540,23 @@ static void aspeed_video_irq_res_change(struct
> aspeed_video *video)
> 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 irq_received = aspeed_video_read(video, VE_INTERRUPT_STATUS);
> + u32 irq_handled = 0;
>
> /*
> * Resolution changed or signal was lost; reset the engine and
> * re-initialize
> */
> - if (sts & VE_INTERRUPT_MODE_DETECT_WD) {
> + if (irq_received & VE_INTERRUPT_MODE_DETECT_WD) {
> aspeed_video_irq_res_change(video);
> return IRQ_HANDLED;
> }
>
> - if (sts & VE_INTERRUPT_MODE_DETECT) {
> + if (irq_received & VE_INTERRUPT_MODE_DETECT) {
> if (test_bit(VIDEO_RES_DETECT, &video->flags)) {
> aspeed_video_update(video, VE_INTERRUPT_CTRL,
> VE_INTERRUPT_MODE_DETECT, 0);
> - aspeed_video_write(video, VE_INTERRUPT_STATUS,
> - VE_INTERRUPT_MODE_DETECT);
> -
> + irq_handled |= VE_INTERRUPT_MODE_DETECT;
> set_bit(VIDEO_MODE_DETECT_DONE, &video->flags);
> wake_up_interruptible_all(&video->wait);
> } else {
> @@ -569,7 +569,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
> }
> }
>
> - if (sts & VE_INTERRUPT_COMP_COMPLETE) {
> + if (irq_received & VE_INTERRUPT_COMP_COMPLETE) {
> struct aspeed_video_buffer *buf;
> u32 frame_size = aspeed_video_read(video,
> VE_OFFSET_COMP_STREAM);
> @@ -599,14 +599,15 @@ static irqreturn_t aspeed_video_irq(int irq, void
> *arg)
> VE_SEQ_CTRL_TRIG_COMP, 0);
> aspeed_video_update(video, VE_INTERRUPT_CTRL,
> VE_INTERRUPT_COMP_COMPLETE, 0);
> - aspeed_video_write(video, VE_INTERRUPT_STATUS,
> - VE_INTERRUPT_COMP_COMPLETE);
> -
> + irq_handled |= VE_INTERRUPT_COMP_COMPLETE;
> if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
> aspeed_video_start_frame(video);
> }
>
> - return IRQ_HANDLED;
> + if (irq_handled)
> + aspeed_video_write(video, VE_INTERRUPT_STATUS, irq_handled);
> +
> + return irq_received == irq_handled ? IRQ_HANDLED : IRQ_NONE;
I don't think we need two variables? We can clear bits out of sts as we go,
and then the return condition becomes:
return sts ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static void aspeed_video_check_and_set_polarity(struct aspeed_video
> *video)
>
More information about the openbmc
mailing list