Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754563AbbBBCHo (ORCPT ); Sun, 1 Feb 2015 21:07:44 -0500 Received: from mail-vc0-f181.google.com ([209.85.220.181]:48551 "EHLO mail-vc0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752277AbbBBCHm (ORCPT ); Sun, 1 Feb 2015 21:07:42 -0500 MIME-Version: 1.0 In-Reply-To: <1422693698-21944-1-git-send-email-mark.yao@rock-chips.com> References: <1422693698-21944-1-git-send-email-mark.yao@rock-chips.com> From: Daniel Kurtz Date: Mon, 2 Feb 2015 10:07:21 +0800 X-Google-Sender-Auth: x_lUKmMZHviYKOV87Xl-RTf0Rvg Message-ID: Subject: Re: [PATCH] drm/rockchip: vop: power off until vop standby take effect To: Mark Yao Cc: David Airlie , Daniel Vetter , Rob Clark , Philipp Zabel , dri-devel , "linux-kernel@vger.kernel.org" , "open list:ARM/Rockchip SoC..." Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5987 Lines: 180 Hi Mark, Heiko, On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao wrote: > Vop standby will take effect end of current frame, > if dsp_hold_valid_irq happen, it means vop standby complete. > > we must wait standby complete when we want to disable aclk, > if not, memory bus maybe dead. > > Signed-off-by: Mark Yao > --- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 76 ++++++++++++++++++++++----- > 1 file changed, 63 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index fb25836..47ea61f 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -89,6 +89,7 @@ struct vop { > /* mutex vsync_ work */ > struct mutex vsync_mutex; > bool vsync_work_pending; > + struct completion dsp_hold_completion; > > const struct vop_data *data; > > @@ -382,6 +383,34 @@ static bool is_alpha_support(uint32_t format) > } > } > > +static void vop_dsp_hold_valid_irq_enable(struct vop *vop) > +{ > + unsigned long flags; > + > + BUG_ON(!vop->is_enabled); Re: Heiko "use a WARN_ON": If the VOP clock is off, then the system will just hang when trying to write the VOP register so in this case, BUG_ON gives a more reliable crash dump than the hang. > + > + spin_lock_irqsave(&vop->irq_lock, flags); > + > + vop_mask_write(vop, INTR_CTRL0, DSP_HOLD_VALID_INTR_MASK, > + DSP_HOLD_VALID_INTR_EN(1)); > + > + spin_unlock_irqrestore(&vop->irq_lock, flags); > +} > + > +static void vop_dsp_hold_valid_irq_disable(struct vop *vop) > +{ > + unsigned long flags; > + > + BUG_ON(!vop->is_enabled); > + > + spin_lock_irqsave(&vop->irq_lock, flags); > + > + vop_mask_write(vop, INTR_CTRL0, DSP_HOLD_VALID_INTR_MASK, > + DSP_HOLD_VALID_INTR_EN(0)); > + > + spin_unlock_irqrestore(&vop->irq_lock, flags); > +} > + > static void vop_enable(struct drm_crtc *crtc) > { > struct vop *vop = to_vop(crtc); > @@ -454,26 +483,36 @@ static void vop_disable(struct drm_crtc *crtc) > > drm_vblank_off(crtc->dev, vop->pipe); > > - disable_irq(vop->irq); > - > /* > - * TODO: Since standby doesn't take effect until the next vblank, > - * when we turn off dclk below, the vop is probably still active. > + * Vop standby will take effect until end of current frame, "Vop standby will take effect at end of current frame" > + * if dsp hold valid irq happen, it means standby complete. > + * > + * we must wait standby complete when we want to disable aclk, > + * if not, memory bus maybe dead. > */ > + reinit_completion(&vop->dsp_hold_completion); > + vop_dsp_hold_valid_irq_enable(vop); > + > spin_lock(&vop->reg_lock); > > VOP_CTRL_SET(vop, standby, 1); > > spin_unlock(&vop->reg_lock); > > + wait_for_completion(&vop->dsp_hold_completion); > + > + vop_dsp_hold_valid_irq_disable(vop); > + > + disable_irq(vop->irq); > + > vop->is_enabled = false; > + > /* > - * disable dclk to stop frame scan, so we can safely detach iommu, > + * vop standby complete, so iommu detach is safe. > */ > - clk_disable(vop->dclk); > - > rockchip_drm_dma_detach_device(vop->drm_dev, vop->dev); > > + clk_disable(vop->dclk); > clk_disable(vop->aclk); > clk_disable(vop->hclk); > } > @@ -1086,6 +1125,7 @@ static irqreturn_t vop_isr(int irq, void *data) > struct vop *vop = data; > uint32_t intr0_reg, active_irqs; > unsigned long flags; > + int ret = IRQ_NONE; > > /* > * INTR_CTRL0 register has interrupt status, enable and clear bits, we > @@ -1104,15 +1144,24 @@ static irqreturn_t vop_isr(int irq, void *data) > if (!active_irqs) > return IRQ_NONE; > > - /* Only Frame Start Interrupt is enabled; other irqs are spurious. */ > - if (!(active_irqs & FS_INTR)) { > - DRM_ERROR("Unknown VOP IRQs: %#02x\n", active_irqs); > - return IRQ_NONE; > + if (active_irqs & DSP_HOLD_VALID_INTR) { > + if (!completion_done(&vop->dsp_hold_completion)) Why is this "completion_done" check needed? I guess it doesn't help, but isn't strictly needed, since the dsp_hold_completion is a one shot, right? In any case, this should work as it is, so: Reviewed-by: Daniel Kurtz > + complete(&vop->dsp_hold_completion); > + active_irqs &= ~DSP_HOLD_VALID_INTR; > + ret = IRQ_HANDLED; > } > > - drm_handle_vblank(vop->drm_dev, vop->pipe); > + if (active_irqs & FS_INTR) { > + drm_handle_vblank(vop->drm_dev, vop->pipe); > + active_irqs &= ~FS_INTR; > + ret = (vop->vsync_work_pending) ? IRQ_WAKE_THREAD : IRQ_HANDLED; > + } > > - return (vop->vsync_work_pending) ? IRQ_WAKE_THREAD : IRQ_HANDLED; > + /* Unhandled irqs are spurious. */ > + if (active_irqs) > + DRM_ERROR("Unknown VOP IRQs: %#02x\n", active_irqs); > + > + return ret; > } > > static int vop_create_crtc(struct vop *vop) > @@ -1194,6 +1243,7 @@ static int vop_create_crtc(struct vop *vop) > goto err_cleanup_crtc; > } > > + init_completion(&vop->dsp_hold_completion); > crtc->port = port; > vop->pipe = drm_crtc_index(crtc); > rockchip_register_crtc_funcs(drm_dev, &private_crtc_funcs, vop->pipe); > -- > 1.7.9.5 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/