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(-)
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@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; }
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);
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)
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
dri-devel@lists.freedesktop.org
-
Heiko Stübner -
Marc Zyngier