[PATCH 0/3] drm/rockchip: VOP interrupt fixes

[PATCH 0/3] drm/rockchip: VOP interrupt fixes

[Marc Zyngier]

This small series fixes a number of issues that I found while trying
to get kexec working on the Chromebook Plus (aka rk3399-gru-kevin) in
order to use it as some sort of interactive bootloader.

The main issue is that the vop driver expects the interrupts to be
cleared and disabled when booting. Nothing could be more wrong. The
device should be expected to be alive and screaming, and it is the
driver's job to put it back into a sane state.

This is what the first patch does, making sure the interrupt is
requested only when the device has been put back into a known
state. Given that this is an observable bug that has been around for a
while, I've tagged it with a Cc: stable.

The two following patches are less important: Using memcpy on MMIO
ranges is plain wrong, and using spin_lock_irqsave in irq context is
slightly pointless.

With these patches in, I'm able to get kexec to work. There is still
some funny issues at the iommu level, but that's for another day.

Patches on top of 4.16-rc2.

Marc Zyngier (3):
  drm/rockchip: Clear all interrupts before requesting the IRQ
  drm/rockchip: Do not use memcpy for MMIO addresses
  drm/rockchip: Don't use spin_lock_irqsave in interrupt context

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 44 +++++++++++++++--------------
 1 file changed, 23 insertions(+), 21 deletions(-)

-- 
2.14.2

[PATCH 1/3] drm/rockchip: Clear all interrupts before requesting the IRQ

[Marc Zyngier]

Calling request_irq() followed by disable_irq() is usually a bad idea,
specially if the interrupt can be pending, and you're not yet in a
position to handle it.

This is exactly what happens on my kevin system when rebooting in a
second kernel using kexec: Some interrupt is left pending from
the previous kernel, and we take it too early, before disable_irq()
could do anything.

Let's clear the pending interrupts as we initialize the HW, and move
the interrupt request after that point. This ensures that we're in
a sane state when the interrupt is requested.

Cc: stable at vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index ba7505292b78..7b224e08cbf1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1414,6 +1414,9 @@ static int vop_initial(struct vop *vop)
 	usleep_range(10, 20);
 	reset_control_deassert(ahb_rst);
 
+	VOP_INTR_SET_TYPE(vop, clear, INTR_MASK, 1);
+	VOP_INTR_SET_TYPE(vop, enable, INTR_MASK, 0);
+
 	memcpy(vop->regsbak, vop->regs, vop->len);
 
 	VOP_REG_SET(vop, misc, global_regdone_en, 1);
@@ -1569,17 +1572,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	mutex_init(&vop->vsync_mutex);
 
-	ret = devm_request_irq(dev, vop->irq, vop_isr,
-			       IRQF_SHARED, dev_name(dev), vop);
-	if (ret)
-		return ret;
-
-	/* IRQ is initially disabled; it gets enabled in power_on */
-	disable_irq(vop->irq);
-
 	ret = vop_create_crtc(vop);
 	if (ret)
-		goto err_enable_irq;
+		return ret;
 
 	pm_runtime_enable(&pdev->dev);
 
@@ -1590,13 +1585,19 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 		goto err_disable_pm_runtime;
 	}
 
+	ret = devm_request_irq(dev, vop->irq, vop_isr,
+			       IRQF_SHARED, dev_name(dev), vop);
+	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:
 	pm_runtime_disable(&pdev->dev);
 	vop_destroy_crtc(vop);
-err_enable_irq:
-	enable_irq(vop->irq); /* To balance out the disable_irq above */
 	return ret;
 }
 
-- 
2.14.2

[PATCH 2/3] drm/rockchip: Do not use memcpy for MMIO addresses

[Marc Zyngier]

memcpy is only meant to be used for memory, and only that.
MMIO accessors should be used to access MMIO regions, preferably
the ones that correspond to the size of the register accessed.

Let's convert the bulk register copy to writel/readl_relaxed,
which is the correct API.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 7b224e08cbf1..f2bde1c76bbe 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -528,7 +528,10 @@ static int vop_enable(struct drm_crtc *crtc)
 		goto err_disable_aclk;
 	}
 
-	memcpy(vop->regs, vop->regsbak, vop->len);
+	spin_lock(&vop->reg_lock);
+	for (i = 0; i < vop->len; i += 4)
+		writel_relaxed(vop->regsbak[i / 4], vop->regs + i);
+
 	/*
 	 * We need to make sure that all windows are disabled before we
 	 * enable the crtc. Otherwise we might try to scan from a destroyed
@@ -538,10 +541,9 @@ static int vop_enable(struct drm_crtc *crtc)
 		struct vop_win *vop_win = &vop->win[i];
 		const struct vop_win_data *win = vop_win->data;
 
-		spin_lock(&vop->reg_lock);
 		VOP_WIN_SET(vop, win, enable, 0);
-		spin_unlock(&vop->reg_lock);
 	}
+	spin_unlock(&vop->reg_lock);
 
 	vop_cfg_done(vop);
 
@@ -1417,7 +1419,8 @@ static int vop_initial(struct vop *vop)
 	VOP_INTR_SET_TYPE(vop, clear, INTR_MASK, 1);
 	VOP_INTR_SET_TYPE(vop, enable, INTR_MASK, 0);
 
-	memcpy(vop->regsbak, vop->regs, vop->len);
+	for (i = 0; i < vop->len; i += sizeof(u32))
+		vop->regsbak[i / 4] = readl_relaxed(vop->regs + i);
 
 	VOP_REG_SET(vop, misc, global_regdone_en, 1);
 	VOP_REG_SET(vop, common, dsp_blank, 0);
-- 
2.14.2

[PATCH 3/3] drm/rockchip: Don't use spin_lock_irqsave in interrupt context

[Marc Zyngier]

The rockchip DRM driver is quite careful to disable interrupts
when taking a lock that is also taken in interrupt context,
which is a good thing.

What is a bit over the top is to use spin_lock_irqsave when
already in interrupt context, as you cannot take another
interrupt again, and disabling interrupt is just pure
overhead.

Switching to the non _irqsave version in interrupt context is
more logical, and less heavy handed.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f2bde1c76bbe..48d27f6fb16d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1160,15 +1160,14 @@ static void vop_handle_vblank(struct vop *vop)
 {
 	struct drm_device *drm = vop->drm_dev;
 	struct drm_crtc *crtc = &vop->crtc;
-	unsigned long flags;
 
-	spin_lock_irqsave(&drm->event_lock, flags);
+	spin_lock(&drm->event_lock);
 	if (vop->event) {
 		drm_crtc_send_vblank_event(crtc, vop->event);
 		drm_crtc_vblank_put(crtc);
 		vop->event = NULL;
 	}
-	spin_unlock_irqrestore(&drm->event_lock, flags);
+	spin_unlock(&drm->event_lock);
 
 	if (test_and_clear_bit(VOP_PENDING_FB_UNREF, &vop->pending))
 		drm_flip_work_commit(&vop->fb_unref_work, system_unbound_wq);
@@ -1179,21 +1178,20 @@ static irqreturn_t vop_isr(int irq, void *data)
 	struct vop *vop = data;
 	struct drm_crtc *crtc = &vop->crtc;
 	uint32_t active_irqs;
-	unsigned long flags;
 	int ret = IRQ_NONE;
 
 	/*
 	 * interrupt register has interrupt status, enable and clear bits, we
 	 * must hold irq_lock to avoid a race with enable/disable_vblank().
 	*/
-	spin_lock_irqsave(&vop->irq_lock, flags);
+	spin_lock(&vop->irq_lock);
 
 	active_irqs = VOP_INTR_GET_TYPE(vop, status, INTR_MASK);
 	/* Clear all active interrupt sources */
 	if (active_irqs)
 		VOP_INTR_SET_TYPE(vop, clear, active_irqs, 1);
 
-	spin_unlock_irqrestore(&vop->irq_lock, flags);
+	spin_unlock(&vop->irq_lock);
 
 	/* This is expected for vop iommu irqs, since the irq is shared */
 	if (!active_irqs)
-- 
2.14.2

[PATCH 0/3] drm/rockchip: VOP interrupt fixes

[Heiko Stübner]

Am Dienstag, 20. Februar 2018, 14:01:17 CET schrieb Marc Zyngier:
> This small series fixes a number of issues that I found while trying
> to get kexec working on the Chromebook Plus (aka rk3399-gru-kevin) in
> order to use it as some sort of interactive bootloader.
> 
> The main issue is that the vop driver expects the interrupts to be
> cleared and disabled when booting. Nothing could be more wrong. The
> device should be expected to be alive and screaming, and it is the
> driver's job to put it back into a sane state.
> 
> This is what the first patch does, making sure the interrupt is
> requested only when the device has been put back into a known
> state. Given that this is an observable bug that has been around for a
> while, I've tagged it with a Cc: stable.
> 
> The two following patches are less important: Using memcpy on MMIO
> ranges is plain wrong, and using spin_lock_irqsave in irq context is
> slightly pointless.
> 
> With these patches in, I'm able to get kexec to work. There is still
> some funny issues at the iommu level, but that's for another day.
> 
> Patches on top of 4.16-rc2.
> 
> Marc Zyngier (3):
>   drm/rockchip: Clear all interrupts before requesting the IRQ
>   drm/rockchip: Do not use memcpy for MMIO addresses
>   drm/rockchip: Don't use spin_lock_irqsave in interrupt context

Tested on rk3036 (hdmi), rk3288 (hdmi+edp) and rk3399 (edp) and
applied to drm-misc-next after slightly fixing patch1 for a recent change
to the code around the old request_irq position, so it applies.


Thanks
Heiko