drm/rockchip: try to fix vblank hang resulting from iommu irq change
Subsystems (5)
Heiko Stuebner
maintainerv4 cover drm/rockchip: try to fix vblank hang resulting from iommu irq change
This still tries to address the hang seen by Ezequiel Garcia on rk3288.
As Tomasz noted, trying to count enablement can run into concurrency issues, so instead we'll just check if the vop is runtime-enabled to see if it could be the source of the irq and then just do our own clk_enable in the isr to bridge the possible gap between pm_runtime_enable and clk_enable in the core vop_enable() function.
Display tested to still work on rk3328 and rk3399, but as I don't see the hang from Ezequiel I hope that this fixes it.
changes in v2:
- adapt approach ... don't try to count usage ourself, because of possible concurrency issues with vop enable/disable
changes in v3:
- fix comment in patch2
- add stable+fixes tags
changes in v4:
- address Marc's comments to patch2
Heiko Stuebner (1):
drm/rockchip: vop: split out core clock enablement into separate
functions
Sandy Huang (1):
drm/rockchip: vop: fix irq disabled after vop driver probed
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 69 ++++++++++++++------- 1 file changed, 48 insertions(+), 21 deletions(-)
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Heiko Stuebner
maintainerPATCH v4 1/2 drm/rockchip: vop: split out core clock enablement into separate functions
Judging from the iommu code, both the hclk and aclk are necessary for register access. Split them off into separate functions from the regular vop enablement, so that we can use them elsewhere as well.
--- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 44 +++++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-)
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +31 -13
@@ -486,6 +486,31 @@ static void vop_line_flag_irq_disable(struct vop *vop)spin_unlock_irqrestore (&vop->irq_lock, flags); } +static int vop_core_clks_enable (struct vop *vop) +{ +int ret; + + ret =clk_enable (vop->hclk); +if (ret <0 ) +return ret; + + ret =clk_enable (vop->aclk); +if (ret <0 ) + goto err_disable_hclk; + +return 0 ; + +err_disable_hclk: +clk_disable (vop->hclk); +return ret; +} + +static void vop_core_clks_disable (struct vop *vop) +{ +clk_disable (vop->aclk); +clk_disable (vop->hclk); +} +static int vop_enable (struct drm_crtc *crtc) {struct vop *vop =to_vop (crtc);
@@ -497,17 +522,13 @@ static int vop_enable(struct drm_crtc *crtc)return ret; } - ret =clk_enable (vop->hclk); + ret =vop_core_clks_enable (vop);if (WARN_ON (ret <0 )) goto err_put_pm_runtime; ret =clk_enable (vop->dclk);if (WARN_ON (ret <0 )) - goto err_disable_hclk; - - ret =clk_enable (vop->aclk); -if (WARN_ON (ret <0 )) - goto err_disable_dclk; + goto err_disable_core;/* * Slave iommu shares power, irq and clock with vop. It was associated
@@ -519,7 +540,7 @@ static int vop_enable(struct drm_crtc *crtc)if (ret) {DRM_DEV_ERROR (vop->dev,"failed to attach dma mapping, %d\n" , ret); - goto err_disable_aclk; + goto err_disable_dclk; }spin_lock (&vop->reg_lock);
@@ -558,12 +579,10 @@ static int vop_enable(struct drm_crtc *crtc)return 0 ; -err_disable_aclk: -clk_disable (vop->aclk); err_disable_dclk:clk_disable (vop->dclk); -err_disable_hclk: -clk_disable (vop->hclk); +err_disable_core: +vop_core_clks_disable (vop); err_put_pm_runtime:pm_runtime_put_sync (vop->dev);return ret;
@@ -609,8 +628,7 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,rockchip_drm_dma_detach_device (vop->drm_dev, vop->dev);clk_disable (vop->dclk); -clk_disable (vop->aclk); -clk_disable (vop->hclk); +vop_core_clks_disable (vop);pm_runtime_put (vop->dev);mutex_unlock (&vop->vop_lock);
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/2] drm/rockchip: vop: split out core clock enablement into separate functions
Hi Heiko,
Quoted 5 lines · Heiko Stuebner
Judging from the iommu code, both the hclk and aclk are necessary for register access. Split them off into separate functions from the regular vop enablement, so that we can use them elsewhere as well.
Quoted 7 lines
Fixes d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")Signed-off-by Heiko Stuebner <[email protected]>Tested-by Ezequiel Garcia <[email protected]>--- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 44 +++++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-)
Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Heiko Stuebner
maintainerPATCH v4 2/2 drm/rockchip: vop: fix irq disabled after vop driver probed
In reply to Heiko StuebnerFrom: Sandy Huang <[email protected]>
The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.
But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.
So remove the enable/disable handling and add appropriate condition to the irq handler.
changes in v2:
- move to just check the power-domain state
- add clock handling
changes in v3:
- clarify comment to speak of runtime-pm not power-domain changes in v4:
- address Marc's comments (clk-enable WARN_ON and style improvement)
--- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 25 ++++++++++++++------- 1 file changed, 17 insertions(+), 8 deletions(-)
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +17 -8
@@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)spin_unlock (&vop->reg_lock); -enable_irq (vop->irq); -drm_crtc_vblank_on (crtc);return 0 ;
@@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,vop_dsp_hold_valid_irq_disable (vop); -disable_irq (vop->irq); - vop->is_enabled = false;/*
@@ -1195,6 +1191,18 @@ static irqreturn_t vop_isr(int irq, void *data)uint32_t active_irqs;int ret =IRQ_NONE ; +/* +* The irq is shared with the iommu. If the runtime-pm state of the +* vop-device is disabled the irq has to be targetted at the iommu. +*/ +if (!pm_runtime_get_if_in_use (vop->dev)) +return IRQ_NONE ; + +if (vop_core_clks_enable (vop)) { +DRM_DEV_ERROR_RATELIMITED (vop->dev,"couldn't enable clocks\n" ); + goto out; + } +/* * interrupt register has interrupt status, enable and clear bits, we * must hold irq_lock to avoid a race with enable/disable_vblank().
@@ -1210,7 +1218,7 @@ static irqreturn_t vop_isr(int irq, void *data)/* This is expected for vop iommu irqs, since the irq is shared */ if (!active_irqs) -return IRQ_NONE ; + goto out_disable;if (active_irqs &DSP_HOLD_VALID_INTR ) {complete (&vop->dsp_hold_completion);
@@ -1236,6 +1244,10 @@ static irqreturn_t vop_isr(int irq, void *data)DRM_DEV_ERROR (vop->dev,"Unknown VOP IRQs: %#02x\n" , active_irqs); +out_disable: +vop_core_clks_disable (vop); +out: +pm_runtime_put (vop->dev);return ret; }
@@ -1614,9 +1626,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)if (ret) goto err_disable_pm_runtime; -/* IRQ is initially disabled; it gets enabled in power_on */ -disable_irq (vop->irq); -return 0 ; err_disable_pm_runtime:
Signature · 6 lines
2.17.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed
Quoted 23 lines · Heiko Stuebner
From: Sandy Huang <[email protected]>
The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.
But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.
So remove the enable/disable handling and add appropriate condition to the irq handler.
changes in v2:
- move to just check the power-domain state
- add clock handling
changes in v3:
- clarify comment to speak of runtime-pm not power-domain changes in v4:
- address Marc's comments (clk-enableWARN_ONand style improvement)
Quoted 5 lines
Fixes d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")Signed-off-by Sandy Huang <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>Tested-by Ezequiel Garcia <[email protected]>
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Heiko Stübner
maintainerRe: [PATCH v4 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed
Hi Marc,
Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier:
Quoted 24 lines · Heiko Stuebner
Quoted 23 lines
From: Sandy Huang <[email protected]>
The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.
But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.
So remove the enable/disable handling and add appropriate condition to the irq handler.
changes in v2:
- move to just check the power-domain state
- add clock handling
changes in v3:
- clarify comment to speak of runtime-pm not power-domain changes in v4:
- address Marc's comments (clk-enableWARN_ONand style improvement)
Quoted 7 lines
Quoted 5 lines
Fixes d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")Signed-off-by Sandy Huang <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>Tested-by Ezequiel Garcia <[email protected]>Reviewed-by Marc Zyngier <[email protected]>
could I ask you to also look at patch1 of this series, to give it an Ack or Review? drm-misc documentation very strongly suggests [0] to have at least another set of eyes on a patch and so far noone came forward ;-)
This of course also applies to everybody else in the Cc list :-D .
Thanks
Heiko
[0] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#merge-criteria
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Marc Zyngier
Acked-byRe: [PATCH v4 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed
Quoted 33 lines · Heiko Stübner
Hi Marc,
Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier:
Quoted 30 lines · Heiko Stuebner
Quoted 28 lines
From: Sandy Huang <[email protected]>
The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.
But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.
So remove the enable/disable handling and add appropriate condition to the irq handler.
changes in v2:
- move to just check the power-domain state
- add clock handling
changes in v3:
- clarify comment to speak of runtime-pm not power-domain changes in v4:
- address Marc's comments (clk-enableWARN_ONand style improvement)Fixes d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")Signed-off-by Sandy Huang <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>Tested-by Ezequiel Garcia <[email protected]>
Quoted 8 lines
Quoted 1 line
Reviewed-by Marc Zyngier <[email protected]>could I ask you to also look at patch1 of this series, to give it an Ack or Review? drm-misc documentation very strongly suggests [0] to have at least another set of eyes on a patch and so far noone came forward ;-)
This of course also applies to everybody else in the Cc list :-D
Please feel free to apply my
to that one.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Am Dienstag, 12. Juni 2018, 15:20:26 CEST schrieb Heiko Stuebner:
Quoted 22 lines
This still tries to address the hang seen by Ezequiel Garcia on rk3288.
As Tomasz noted, trying to count enablement can run into concurrency issues, so instead we'll just check if the vop is runtime-enabled to see if it could be the source of the irq and then just do our own
clk_enablein the isr to bridge the possible gap betweenpm_runtime_enableandclk_enablein the corevop_enable()function.Display tested to still work on rk3328 and rk3399, but as I don't see the hang from Ezequiel I hope that this fixes it.
changes in v2:
- adapt approach ... don't try to count usage ourself, because of possible concurrency issues with vop enable/disable
changes in v3:
- fix comment in patch2
- add stable+fixes tags
changes in v4:
- address Marc's comments to patch2
Quoted 6 lines
Heiko Stuebner (1):
drm/rockchip: vop: split out core clock enablement into separate
functionsSandy Huang (1):
drm/rockchip: vop: fix irq disabled after vop driver probed
applied both to drm-misc-next with the appropriate Review tags
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel