[PATCH dev-5.0 4/4] media: aspeed: clear interrupt status flags immediately
Jae Hyun Yoo
jae.hyun.yoo at linux.intel.com
Thu May 2 02:20:50 AEST 2019
On 4/30/2019 6:29 PM, Andrew Jeffery wrote:
> 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;
Yeah, that would be neater. I'll change it in v2 like you suggested.
Thanks,
Jae
>> }
>>
>> static void aspeed_video_check_and_set_polarity(struct aspeed_video
>> *video)
>>
More information about the openbmc
mailing list