* [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask @ 2015-08-26 5:51 Archit Taneja 2015-08-26 13:55 ` hali 0 siblings, 1 reply; 9+ messages in thread From: Archit Taneja @ 2015-08-26 5:51 UTC (permalink / raw) To: dri-devel; +Cc: robdclark, linux-arm-msm, hali, sviau, Archit Taneja 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); } 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-26 5:51 [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask Archit Taneja @ 2015-08-26 13:55 ` hali 2015-08-26 14:18 ` Rob Clark 0 siblings, 1 reply; 9+ messages in thread From: hali @ 2015-08-26 13:55 UTC (permalink / raw) To: Archit Taneja; +Cc: linux-arm-msm, dri-devel 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 > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-26 13:55 ` hali @ 2015-08-26 14:18 ` Rob Clark 2015-08-26 15:12 ` hali 0 siblings, 1 reply; 9+ messages in thread From: Rob Clark @ 2015-08-26 14:18 UTC (permalink / raw) To: Hai Li Cc: Archit Taneja, dri-devel@lists.freedesktop.org, linux-arm-msm, Stephane Viau 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 >> >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-26 14:18 ` Rob Clark @ 2015-08-26 15:12 ` hali 2015-08-27 5:06 ` Archit Taneja 0 siblings, 1 reply; 9+ messages in thread From: hali @ 2015-08-26 15:12 UTC (permalink / raw) To: Rob Clark; +Cc: linux-arm-msm, dri-devel@lists.freedesktop.org > 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 >>> >>> >> >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-26 15:12 ` hali @ 2015-08-27 5:06 ` Archit Taneja 2015-08-28 7:56 ` Archit Taneja 0 siblings, 1 reply; 9+ messages in thread From: Archit Taneja @ 2015-08-27 5:06 UTC (permalink / raw) To: hali, Rob Clark Cc: dri-devel@lists.freedesktop.org, linux-arm-msm, Stephane Viau 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 > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-27 5:06 ` Archit Taneja @ 2015-08-28 7:56 ` Archit Taneja 2015-08-28 18:18 ` Rob Clark 0 siblings, 1 reply; 9+ messages in thread From: Archit Taneja @ 2015-08-28 7:56 UTC (permalink / raw) To: hali, Rob Clark Cc: linux-arm-msm, dri-devel@lists.freedesktop.org, Stephane Viau 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-28 7:56 ` Archit Taneja @ 2015-08-28 18:18 ` Rob Clark 2015-08-31 5:15 ` Archit Taneja 0 siblings, 1 reply; 9+ messages in thread From: Rob Clark @ 2015-08-28 18:18 UTC (permalink / raw) To: Archit Taneja Cc: Hai Li, linux-arm-msm, dri-devel@lists.freedesktop.org, Stephane Viau 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-28 18:18 ` Rob Clark @ 2015-08-31 5:15 ` Archit Taneja 2015-08-31 11:38 ` Rob Clark 0 siblings, 1 reply; 9+ messages in thread From: Archit Taneja @ 2015-08-31 5:15 UTC (permalink / raw) 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 <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 -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask 2015-08-31 5:15 ` Archit Taneja @ 2015-08-31 11:38 ` Rob Clark 0 siblings, 0 replies; 9+ messages in thread From: Rob Clark @ 2015-08-31 11:38 UTC (permalink / raw) To: Archit Taneja Cc: Hai Li, linux-arm-msm, dri-devel@lists.freedesktop.org, Stephane Viau 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-08-31 11:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-26 5:51 [PATCH] drm/msm/mdp5: enable clocks in hw_init and set_irqmask Archit Taneja 2015-08-26 13:55 ` hali 2015-08-26 14:18 ` Rob Clark 2015-08-26 15:12 ` hali 2015-08-27 5:06 ` Archit Taneja 2015-08-28 7:56 ` Archit Taneja 2015-08-28 18:18 ` Rob Clark 2015-08-31 5:15 ` Archit Taneja 2015-08-31 11:38 ` Rob Clark
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox