Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask

[Archit Taneja <architt@codeaurora.org>]

On 08/27/2015 10:36 AM, Archit Taneja wrote:
>
>
> On 08/26/2015 08:42 PM, hali@codeaurora.org wrote:
>>> 2015-08-26 9:55 GMT-04:00  <hali@codeaurora.org>:
>>>> Hi Archit,
>>>>
>>>>> mdp5_hw_init and mdp5_set_irqmask configure registers but may not have
>>>>> clocks enabled.
>>>>>
>>>>> Add mdp5_enable/disable calls in these funcs to ensure clocks are
>>>>> enabled. We need this until we get proper runtime pm support.
>>>>>
>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 10 ++++++++--
>>>>>   drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c |  2 ++
>>>>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>>>> index b1f73be..9fabfca 100644
>>>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
>>>>> @@ -24,9 +24,15 @@
>>>>>   void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>>>>>                uint32_t old_irqmask)
>>>>>   {
>>>>> -     mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_CLEAR(0),
>>>>> +     struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>>>>> +
>>>>> +     mdp5_enable(mdp5_kms);
>>>>> +
>>>>> +     mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_CLEAR(0),
>>>>>                irqmask ^ (irqmask & old_irqmask));
>>>>> -     mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_MDP_INTR_EN(0),
>>>>> irqmask);
>>>>> +     mdp5_write(mdp5_kms, REG_MDP5_MDP_INTR_EN(0), irqmask);
>>>>> +
>>>>> +     mdp5_disable(mdp5_kms);
>>>>>   }
>>>>
>>>> mdp5_set_irqmask() can be invoked in atomic context, clk_prepare() is
>>>> not
>>>> allowed in this function because it may cause process to sleep. We can
>>>> enable the clocks in the caller at initialization.
>
> Oh, oops. I missed that.
>
>>>
>>> iirc, it will be called with at least one spinlock held..
>>>
>>> We do already move the enable/disable_vblank() paths off to a worker
>>> so that we can ensure things are enabled before we get into
>>> update_irq()..  the only other path to update_irq() should be when
>>> driver code does mdp_irq_register/unregister().. so maybe we should
>>> just require that the mdp4/mdp5 kms code only calls those when clk's
>>> are already enabled (which should be mostly true already, I think)
>>>
>>> BR,
>>> -R
>>
>> Yes, the only case that not been covered is mdp5_irq_postinstall(). We
>> can
>> enable clocks in this function. Actually, this is what we are doing in
>> downstream test.
>
> It works fine if I put it in postinstall. I'll update the patch and resend.

So, I hit an issue in both the approaches.

When I try modeset, I get a watchdog reset once the app closes down.

Looking at debug logs, it looks like the issue happens when the ioctl
RMFB and drm_release race with each other.

Within the the msm driver, this maps to mdp5_complete_commit
(drm_mode_rmfb path) being called before mdp5_crtc_cancel_pending_flip
(drm_release path). mdp5_complete_commit disables clocks, and the other
patch calls complete_flip, which requires clocks.

If I wrap around complete_flip with mdp5_enable/disable calls, things
work fine. Although, that's not an ideal fix.

Any suggestions?

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project