On 29/05/18 13:17, Heiko St?bner wrote: > Am Dienstag, 29. Mai 2018, 13:59:42 CEST schrieb Robin Murphy: >> On 28/05/18 14:20, Heiko Stuebner wrote: >>> From: Sandy Huang <hjc at 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 >>> >>> Signed-off-by: Sandy Huang <hjc at rock-chips.com> >>> [add commit message, moved to pm_runtime_get_if_in_use] >>> Signed-off-by: Heiko Stuebner <heiko at sntech.de> >>> --- >>> >>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++------- >>> 1 file changed, 19 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index >>> b55156b8ba3b..615a5b44bfe9 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,16 @@ 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 power-domain is off >>> + * the irq has to be targetted at the iommu. >> >> Hmm, aren't the IOMMUs in the same power domain as their respective >> master, though? I would naively assume so, and it does look that way >> from the DTs in the BSP kernel. >> >> AFAICS the IOMMU usage count should never be greater than the VOP usage >> count (except before the VOP driver has probed, but I don't think that >> matters), so although this looks like a sensible change in general I >> can't help be a little bit puzzled at how and why the flow works. > > Ok, the comment might be misleading. It actually means to use the runtime-pm > state of the vop-_device_ as a check. > > I.e. in vop_initials(), Marc added the patch clearing and masking all vop > interrupts. In vop_enable() we have runtime_get_... + enablement of > vop interrupts, which get disabled in vop_disable again. > > That way, checking the runtime_pm state should be an indicator if the > irq is for the vop and not the iommu. Right, but whenever the VOP is nominally-disabled, the IOMMU should also be nominally-disabled, in which case if it's even possible for the IRQ to be asserted, both drivers would ignore it for the same reason (plus the IOMMU driver would also spew a WARN(), which I'm not sure is always appropriate...). That's what I couldn't quite make sense of. However, from serendipitously stumbling across [1] I see that the IOMMU is in fact going to get explicitly enabled by the driver core around probing the VOP, which does give a window during which the imbalance is present. I can imagine that the IOMMU reset via the VOP driver's dma_configure_call() might misbehave if e.g. the VOP was left running from a bootloader splash screen, but in that case I would expect to see various screaming from the IOMMU driver which wasn't apparent in Ezequiel's log. Oh well, as I said before the patch looks sane regardless of my ability to reason about it ;) Robin. [1] https://patchwork.kernel.org/patch/10408825/ >>> + */ >>> + if (!pm_runtime_get_if_in_use(vop->dev)) >>> + return IRQ_NONE; >>> + >>> + if (WARN_ON(vop_core_clks_enable(vop))) >>> + goto out; >>> + >>> >>> /* >>> >>> * interrupt register has interrupt status, enable and clear bits, we >>> * must hold irq_lock to avoid a race with enable/disable_vblank(). >>> >>> @@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data) >>> >>> spin_unlock(&vop->irq_lock); >>> >>> /* This is expected for vop iommu irqs, since the irq is shared */ >>> >>> - if (!active_irqs) >>> - return IRQ_NONE; >>> + if (!active_irqs) { >>> + ret = IRQ_NONE; >>> + vop_core_clks_disable(vop); >>> + goto out; >>> + } >>> >>> if (active_irqs & DSP_HOLD_VALID_INTR) { >>> >>> complete(&vop->dsp_hold_completion); >>> >>> @@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data) >>> >>> DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n", >>> >>> active_irqs); >>> >>> + vop_core_clks_disable(vop); >>> + >>> +out: >>> + pm_runtime_put(vop->dev); >>> >>> return ret; >>> >>> } >>> >>> @@ -1614,9 +1627,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: > > > >
