| Message ID | 20180612132028.27490-3-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/06/18 14:20, Heiko Stuebner wrote: > From: Sandy Huang <hjc@rock-chips.com> > > 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) > > Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()") > Cc: stable@vger.kernel.org > Signed-off-by: Sandy Huang <hjc@rock-chips.com> > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > Tested-by: Ezequiel Garcia <ezequiel@collabora.com> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
Hi Marc, Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier: > On 12/06/18 14:20, Heiko Stuebner wrote: > > From: Sandy Huang <hjc@rock-chips.com> > > > > 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) > > > > Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Sandy Huang <hjc@rock-chips.com> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > Tested-by: Ezequiel Garcia <ezequiel@collabora.com> > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> 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
On 18/06/18 09:19, Heiko Stübner wrote: > Hi Marc, > > Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier: >> On 12/06/18 14:20, Heiko Stuebner wrote: >>> From: Sandy Huang <hjc@rock-chips.com> >>> >>> 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) >>> >>> Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Sandy Huang <hjc@rock-chips.com> >>> Signed-off-by: Heiko Stuebner <heiko@sntech.de> >>> Tested-by: Ezequiel Garcia <ezequiel@collabora.com> >> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > > 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 Acked-by: Marc Zyngier <marc.zyngier@arm.com> to that one. Thanks, M.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 9a1f272e41c7..d105e984cf09 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -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: