Messages in this thread | | | Date | Thu, 12 May 2022 09:02:12 -0700 | Subject | Re: [PATCH v5] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() | From | Kuogee Hsieh <> |
| |
On 5/11/2022 6:03 PM, Dmitry Baryshkov wrote: > On Thu, 12 May 2022 at 04:01, Stephen Boyd <swboyd@chromium.org> wrote: >> Quoting Dmitry Baryshkov (2022-05-11 17:41:50) >>> On 12/05/2022 03:02, Kuogee Hsieh wrote: >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c >>>> index af7a80c..f3e333e 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >>>> @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable) >>>> >>>> dp_catalog_ctrl_reset(ctrl->catalog); >>>> >>>> - if (enable) >>>> - dp_catalog_ctrl_enable_irq(ctrl->catalog, enable); >>>> + /* >>>> + * all dp controller programmable registers will not >>>> + * be reset to default value after DP_SW_RESET >>>> + * therefore interrupt mask bits have to be updated >>>> + * to enable/disable interrupts >>>> + */ >>>> + dp_catalog_ctrl_enable_irq(ctrl->catalog, enable); >>>> } >>>> >>>> void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl) >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index c388323..79439b8 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -98,6 +98,8 @@ struct dp_display_private { >>>> struct dp_ctrl *ctrl; >>>> struct dp_debug *debug; >>>> >>>> + atomic_t suspended; >>> I think it'd be better to protect it with event_lock rather than using >>> atomics. >> Agreed. I think the concern is that the event queue will have "stuff" in >> it. If the event queue was all a threaded irq we could simply call >> synchronize_irq() after disabling the irq bit in the DP hardware and >> then we would know it is safe to power down the DP logic. Unfortunately >> the event queue is a kthread so we can't do that and we have to rewrite >> synchronize_irq() by checking that the event queue is empty and waiting >> for it to empty out otherwise. It's not safe enough to simply do the >> power operations underneath the event_lock because there's a queue in >> the kthread that might be waiting to grab the event_lock to process. > This sounds like a good reason to rewrite event_thread to use > threaded_irq and/or workqueue.
ok, i will do
1) protect suspended flag with event_lock to prevent new event be added
2) disable interrupts
2) wait for event_q empty before turn off power
| |