| Message ID | 1440568263-674-1-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t > irqstatus) > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > index 047cb04..2b760f5 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms) > unsigned long flags; > > pm_runtime_get_sync(dev->dev); > + mdp5_enable(mdp5_kms); > > /* Magic unknown register writes: > * > @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms) > > mdp5_ctlm_hw_reset(mdp5_kms->ctlm); > > + mdp5_disable(mdp5_kms); > pm_runtime_put_sync(dev->dev); > > return 0; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation > >
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. 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 >> >> static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t >> irqstatus) >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >> index 047cb04..2b760f5 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms) >> unsigned long flags; >> >> pm_runtime_get_sync(dev->dev); >> + mdp5_enable(mdp5_kms); >> >> /* Magic unknown register writes: >> * >> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms) >> >> mdp5_ctlm_hw_reset(mdp5_kms->ctlm); >> >> + mdp5_disable(mdp5_kms); >> pm_runtime_put_sync(dev->dev); >> >> return 0; >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> hosted by The Linux Foundation >> >> > >
> 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. > > 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. > >>> >>> static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t >>> irqstatus) >>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>> index 047cb04..2b760f5 100644 >>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms) >>> unsigned long flags; >>> >>> pm_runtime_get_sync(dev->dev); >>> + mdp5_enable(mdp5_kms); >>> >>> /* Magic unknown register writes: >>> * >>> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms) >>> >>> mdp5_ctlm_hw_reset(mdp5_kms->ctlm); >>> >>> + mdp5_disable(mdp5_kms); >>> pm_runtime_put_sync(dev->dev); >>> >>> return 0; >>> -- >>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>> Forum, >>> hosted by The Linux Foundation >>> >>> >> >> >
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. Thanks, Archit >> >>>> >>>> static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t >>>> irqstatus) >>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>>> index 047cb04..2b760f5 100644 >>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c >>>> @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms) >>>> unsigned long flags; >>>> >>>> pm_runtime_get_sync(dev->dev); >>>> + mdp5_enable(mdp5_kms); >>>> >>>> /* Magic unknown register writes: >>>> * >>>> @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms) >>>> >>>> mdp5_ctlm_hw_reset(mdp5_kms->ctlm); >>>> >>>> + mdp5_disable(mdp5_kms); >>>> pm_runtime_put_sync(dev->dev); >>>> >>>> return 0; >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>>> Forum, >>>> hosted by The Linux Foundation >>>> >>>> >>> >>> >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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
On Fri, Aug 28, 2015 at 3:56 AM, Archit Taneja <architt@codeaurora.org> 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 <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. 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?) > 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? BR, -R > Any suggestions? > > Archit > > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
On 08/28/2015 11:48 PM, Rob Clark wrote: > On Fri, Aug 28, 2015 at 3:56 AM, Archit Taneja <architt@codeaurora.org> 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 <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. 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
On Mon, Aug 31, 2015 at 1:15 AM, Archit Taneja <architt@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@codeaurora.org> >> 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 <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. > > > 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
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); } static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t irqstatus) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c index 047cb04..2b760f5 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c @@ -32,6 +32,7 @@ static int mdp5_hw_init(struct msm_kms *kms) unsigned long flags; pm_runtime_get_sync(dev->dev); + mdp5_enable(mdp5_kms); /* Magic unknown register writes: * @@ -63,6 +64,7 @@ static int mdp5_hw_init(struct msm_kms *kms) mdp5_ctlm_hw_reset(mdp5_kms->ctlm); + mdp5_disable(mdp5_kms); pm_runtime_put_sync(dev->dev); return 0;
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(-)