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

[Rob Clark]

On Mon, Aug 31, 2015 at 1:15 AM, Archit Taneja <architt at codeaurora.org> wrote:
>
>
> On 08/28/2015 11:48 PM, Rob Clark wrote:
>>
>> On Fri, Aug 28, 2015 at 3:56 AM, Archit Taneja <architt at codeaurora.org>
>> wrote:
>>>
>>>
>>>
>>> On 08/27/2015 10:36 AM, Archit Taneja wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On 08/26/2015 08:42 PM, hali at codeaurora.org wrote:
>>>>>>
>>>>>>
>>>>>> 2015-08-26 9:55 GMT-04:00  <hali at 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 at 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.
>
>
> I meant modetest here*
>
>>>
>>> Looking at debug logs, it looks like the issue happens when the ioctl
>>> RMFB and drm_release race with each other.
>>
>>
>> hmm, this seems a bit strange.. since to do the RMFB ioctl the device
>> must still be open.. do we end up w/ the RMFB being an async commit
>> somehow?  (Although in case of flip w/ gpu rendering still pending,
>> somewhere we probably want to block on previous async commit?)
>
>
> It isn't a race condition as I thought before. The RMFB ioctl isn't
> asynchronous either.
>
> The problem has to do with how msm_atomic_commit behaves when the
> state we want to commit involves disabling the crtc:
>
> In this case, we grab clocks once (in mdp5_prepare_commit), but
> disable them twice (first, via mdp5_crtc_disable() in disable_outputs(),
> and then in mdp5_complete_commit())

We need a better way in disable path to wait until scanout has
finished..  I'm not sure if there is a good way to do this.  I seem to
recall seeing some code in the downstream android driver which would
spin on some (iirc) clk status bits, or something like that.
(Although presumably that involves disabling clk first.)

There is a slightly related issue that, since we don't know when
scanout completes, we end up unmapping scanout buffers too early and
getting some iommu faults (which so far have been mostly harmless, but
ugly)

If there was a non-racy way to wait for last vblank irq, I suppose we
could do that.  Or maybe we should just msleep() the expected amount
of time given the refresh rate.

BR,
-R

> Whatever comes next will crash if it requires clocks. In this case,
> drm_release's call to mdp5_crtc_cancel_pending_flip() is the first
> thing that needs clocks, and it crashes there.
>
>>
>>> 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.
>>
>>
>> I guess it is a reasonable thing to do.. but on that topic, it would
>> be nice if someone had some time to look and the pending atomic
>> suspend/resume/runtime-pm stuff.  I haven't really had time to follow
>> that, but I guess it is a good time to revisit the mdpN_enable/disable
>> stuff?
>
>
> Yeah, that'd be more ideal. We're looking at runtime pm adaptation now.
>
> Thanks,
>
> Archit
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project