From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask Date: Mon, 31 Aug 2015 10:45:40 +0530 Message-ID: <55E3E2FC.1070207@codeaurora.org> References: <1440568263-674-1-git-send-email-architt@codeaurora.org> <9475e562be69b5c0ec26c4c63e0f83e0.squirrel@www.codeaurora.org> <0b855d85f2e9ef386b8ed67d89dbcc33.squirrel@www.codeaurora.org> <55DE9ABE.1050409@codeaurora.org> <55E01442.6040608@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:37973 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750771AbbHaFPr (ORCPT ); Mon, 31 Aug 2015 01:15:47 -0400 In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Rob Clark Cc: Hai Li , linux-arm-msm , "dri-devel@lists.freedesktop.org" , Stephane Viau On 08/28/2015 11:48 PM, Rob Clark wrote: > On Fri, Aug 28, 2015 at 3:56 AM, Archit Taneja wrote: >> >> >> 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 : >>>>>> >>>>>> 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 >>>>>>> --- >>>>>>> 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()) 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