From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754634AbbBBCaW (ORCPT ); Sun, 1 Feb 2015 21:30:22 -0500 Received: from regular1.263xmail.com ([211.150.99.139]:54765 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752423AbbBBCaU (ORCPT ); Sun, 1 Feb 2015 21:30:20 -0500 X-263anti-spam: KSV:0;BIG:0;ABS:1;DNS:0;ATT:0;SPF:S; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ADDR-CHECKED: 0 X-RL-SENDER: mark.yao@rock-chips.com X-FST-TO: djkurtz@chromium.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <14f53be52e91c167d1bbfb5c42da5722> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <54CEE131.1020600@rock-chips.com> Date: Mon, 02 Feb 2015 10:30:09 +0800 From: Mark yao User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Daniel Kurtz CC: David Airlie , Daniel Vetter , Rob Clark , Philipp Zabel , dri-devel , "linux-kernel@vger.kernel.org" , "open list:ARM/Rockchip SoC..." Subject: Re: [PATCH] drm/rockchip: vop: power off until vop standby take effect References: <1422693698-21944-1-git-send-email-mark.yao@rock-chips.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015年02月02日 10:07, Daniel Kurtz wrote: > 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. In this way, you are right, if vop clocks is disabled, write vop register will hang system, and the WARN_ON crash dump may be have no chance to print. > >> + >> + 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" OK >> + * 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? Right, the DSP_HOLD_VALID_INTR only happen when standby complete, one standby one irq. Mark > 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 >> >> > >