drm/msm/mdp5: enable clocks in hw_init and set_irqmask
PATCH 1/1 drm/msm/mdp5: enable clocks in hw_init and set_irqmask
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.
--- 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(-)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +8 -2
@@ -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)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +2 -0
@@ -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 ;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Hi Archit,
Quoted 23 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +1
-1@@ -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);
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 36
Quoted 10 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +7
-1+ +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.
Quoted 25 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +0
-0static void mdp5_irq_error_handler (struct mdp_irq *irq,uint32_t irqstatus)drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +2
-1@@ -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 ; --
Quoted 4 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 31 lines
Hi Archit,
Quoted 29 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +5
-2@@ -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);
Quoted 8 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 36
Quoted 4 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +3
-0+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
Quoted 31 lines
Quoted 29 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +0
-0static void mdp5_irq_error_handler (struct mdp_irq *irq,uint32_t irqstatus)drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +2
-1@@ -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
Quoted 52 lines
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 33 lines
Hi Archit,
Quoted 31 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +6
-2@@ -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);Quoted 8 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 36
Quoted 3 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +2
-0+ +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 toupdate_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)
Quoted 2 lines
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.
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +0
-0static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_tirqstatus)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +2
-0@@ -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);
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +0
-1return 0 ; --The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 34 lines · On 08/26/2015 08:42 PM, [email protected] wrote:
Quoted 34 lines
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 33 lines
Hi Archit,
Quoted 31 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +6
-2@@ -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);
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 36
Quoted 3 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +2
-0+ +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.
Quoted 10 lines
Quoted 10 lines
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 toupdate_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)
Quoted 6 lines
Quoted 2 lines
BR,
-RYes, 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
Quoted 41 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +0
-0static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_tirqstatus)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +2
-0@@ -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);Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +0
-1return 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 61 lines · Archit Taneja
Quoted 34 lines · On 08/26/2015 08:42 PM, [email protected] wrote:
Quoted 34 lines
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 33 lines
Hi Archit,
Quoted 31 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +6
-2@@ -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);Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 36
Quoted 3 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +2
-0+ +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.
Quoted 13 lines
Quoted 12 lines
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 toupdate_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
Quoted 6 lines
Quoted 4 lines
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
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 80 lines · Archit Taneja
Quoted 67 lines · Archit Taneja
Quoted 37 lines · On 08/26/2015 08:42 PM, [email protected] wrote:
Quoted 37 lines
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 35 lines
Hi Archit,
Quoted 32 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +7
-2@@ -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); +Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 36
Quoted 2 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +1
-0+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.
Quoted 14 lines
Quoted 12 lines
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 toupdate_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,
-RQuoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
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_releaserace 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?)
Quoted 7 lines
Within the the msm driver, this maps to
mdp5_complete_commit(drm_mode_rmfbpath) being called beforemdp5_crtc_cancel_pending_flip(drm_releasepath).mdp5_complete_commitdisables clocks, and the other patch callscomplete_flip, which requires clocks.If I wrap around
complete_flipwith 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
Quoted 8 lines
Any suggestions?
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 77 lines · Rob Clark
Quoted 76 lines · Archit Taneja
Quoted 73 lines · Archit Taneja
Quoted 37 lines · On 08/26/2015 08:42 PM, [email protected] wrote:
Quoted 37 lines
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 35 lines
Hi Archit,
Quoted 32 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +7
-2@@ -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); +Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c line 37
Quoted 2 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +1
-0+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.
Quoted 18 lines
Quoted 10 lines
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 toupdate_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)Quoted 2 lines
BR,
-RYes, 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.
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
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*
Quoted 8 lines
Quoted 3 lines
Looking at debug logs, it looks like the issue happens when the ioctl RMFB and
drm_releaserace 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.
Quoted 6 lines
Quoted 6 lines
Within the the msm driver, this maps to
mdp5_complete_commit(drm_mode_rmfbpath) being called beforemdp5_crtc_cancel_pending_flip(drm_releasepath).mdp5_complete_commitdisables clocks, and the other patch callscomplete_flip, which requires clocks.
Quoted 8 lines
Quoted 2 lines
If I wrap around
complete_flipwith 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
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 118 lines · Archit Taneja
Quoted 89 lines · Rob Clark
Quoted 86 lines · Archit Taneja
Quoted 82 lines · Archit Taneja
Quoted 42 lines · On 08/26/2015 08:42 PM, [email protected] wrote:
Quoted 42 lines
2015-08-26 9:55 GMT-04:00 <[email protected]>:
Quoted 39 lines
Hi Archit,
Quoted 35 lines
mdp5_hw_initandmdp5_set_irqmaskconfigure 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 <[email protected]>--- 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(-)drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c +8
-2@@ -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); }Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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.
Quoted 19 lines
Quoted 10 lines
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 toupdate_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)Quoted 2 lines
BR,
-RYes, 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.Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
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*
Quoted 9 lines
Quoted 3 lines
Looking at debug logs, it looks like the issue happens when the ioctl RMFB and
drm_releaserace 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.
Quoted 6 lines
The problem has to do with how
msm_atomic_commitbehaves 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, viamdp5_crtc_disable()indisable_outputs(), and then inmdp5_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
Quoted 31 lines
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.Quoted 10 lines
Quoted 8 lines
Within the the msm driver, this maps to
mdp5_complete_commit(drm_mode_rmfbpath) being called beforemdp5_crtc_cancel_pending_flip(drm_releasepath).mdp5_complete_commitdisables clocks, and the other patch callscomplete_flip, which requires clocks.If I wrap around
complete_flipwith mdp5_enable/disable calls, things work fine. Although, that's not an ideal fix.Quoted 5 lines
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