Re: [PATCH] drm/rockchip: vop: fix irq disabled after vop driver probed

[Tomasz Figa]

Hi Heiko, Sandy,

On Fri, May 25, 2018 at 7:07 AM Heiko StÃbner <heiko@xxxxxxxxx> wrote:

> From: Sandy Huang <hjc@xxxxxxxxxxxxxx>

> 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 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.

> So remove the enable/disable handling and add appropriate condition
> to the irq handler.

> Signed-off-by: Sandy Huang <hjc@xxxxxxxxxxxxxx>
> [added an actual commit message]
> Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> ---
> Hi Ezequiel,

> this patch came from a discussion I had with Rockchip people over the
> iommu changes and resulting issues back in april, but somehow was
> forgotten and not posted to the lists. Correcting that now.

> So removing the enable/disable voodoo on the shared interrupt is
> the preferred way.

>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 ++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 510cdf0..61493d4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -549,8 +549,6 @@ static int vop_enable(struct drm_crtc *crtc)

>          spin_unlock(&vop->reg_lock);

> -       enable_irq(vop->irq);
> -

While this one should be okay (+/- my comment for vop_isr()), because the
hardware is already powered on and clocked at this point...

>          drm_crtc_vblank_on(crtc);

>          return 0;
> @@ -596,8 +594,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;

...this one is more tricky. There might be an interrupt handler still
running at this point. disable_irq() waits for any running handler to
complete before disabling, so we might want to call synchronize_irq() after
setting is_enabled to false.


>          /*
> @@ -1168,6 +1164,13 @@ static irqreturn_t vop_isr(int irq, void *data)
>          int ret = IRQ_NONE;

>          /*
> +        * since the irq is shared with iommu, iommu irq is enabled
before vop
> +        * enable, so before vop enable we do nothing.
> +        */
> +       if (!vop->is_enabled)
> +               return IRQ_NONE;

This doesn't seem to be properly synchronized. We don't even call it under
a spinlock, so no barriers are issued. Perhaps we should make this atomic_t?

Best regards,
Tomasz