| Message ID | 20180612121537.31223-3-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Heiko, On 12/06/18 13:15, 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 > > 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> > --- > 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 9a1f272e41c7..ae8a69793aed 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 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 (WARN_ON(vop_core_clks_enable(vop))) > + goto out; As I mentioned before, a WARN_ON() in an interrupt handler is a good way to make a bad problem even worse, and will give information (full register and stack dump) that is mostly useless to the context at hand. Turning it to a dev_warn_ratelimited() (or DRM_ERROR_RATELIMITED if you want to be DRM compliant) would be a better approach, IMHO. > + > /* > * 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; > + } A couple of nits: ret is already set to IRQ_NONE at this stage, and you could simply rewrite it as: if (!active_irq) goto out_disable; > > 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); > with the "out_disable" label placed here. > + 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: > Thanks, M.
Hi Heiko, On 06/12/2018 08:15 PM, 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. hmmm, i think this patch actually fixes another stall case by removing the unpaired disable_irq() in vop_bind(). if we do disable_irq() in vop_bind() without enable it again in vop_unbind(), the irq_shutdown() called after vop_unbind()(when releasing devres) will confuse the irq depth, so the irq will stay disabled and could never be enabled again. > > 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 > > 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> > --- > 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 9a1f272e41c7..ae8a69793aed 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 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 (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: >
Am Dienstag, 12. Juni 2018, 14:39:03 CEST schrieb Marc Zyngier: > Hi Heiko, > > On 12/06/18 13:15, 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 > > > > 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> > > --- > > 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 9a1f272e41c7..ae8a69793aed 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 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 (WARN_ON(vop_core_clks_enable(vop))) > > + goto out; > > As I mentioned before, a WARN_ON() in an interrupt handler is a good way > to make a bad problem even worse, and will give information (full > register and stack dump) that is mostly useless to the context at hand. > Turning it to a dev_warn_ratelimited() (or DRM_ERROR_RATELIMITED if you > want to be DRM compliant) would be a better approach, IMHO. Gah, sorry that I forgot to address your comment from v2 and thanks for the reminder. > > + > > /* > > * 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; > > + } > > A couple of nits: ret is already set to IRQ_NONE at this stage, and you > could simply rewrite it as: > > if (!active_irq) > goto out_disable; That's only one nit :-P ... but will change the patch accordingly. Heiko
Hi Heiko, On Tue, Jun 12, 2018 at 9:15 PM Heiko Stuebner <heiko@sntech.de> 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 [snip] > @@ -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); nit: If we're adding "out:", couldn't we also add "out_clks:" and move the call to vop_core_clks_disable() there? > + 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; > } Other than that: Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
Am Montag, 18. Juni 2018, 10:44:58 CEST schrieb Tomasz Figa: > Hi Heiko, > > On Tue, Jun 12, 2018 at 9:15 PM Heiko Stuebner <heiko@sntech.de> 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 > [snip] > > @@ -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); > > nit: If we're adding "out:", couldn't we also add "out_clks:" and move > the call to vop_core_clks_disable() there? > > > + 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; > > } > > Other than that: > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> That's similar to what Marc suggested and thus already part of v4 posted last tuesday, so I'll just carry over your Reviewed-by. Could you possibly also give patch1 a nod of approval? So I can honor the strong suggestion in the drm-misc documentation? ;-) Thanks Heiko
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 9a1f272e41c7..ae8a69793aed 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 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 (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: