| Message ID | 1422693698-21944-1-git-send-email-mark.yao@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mark, Am Samstag, 31. Januar 2015, 16:41:38 schrieb Mark Yao: > 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 <mark.yao@rock-chips.com> > --- > 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); BUG_ON is generally not well liked in general error handling - i.e. in the !vop->is_enabled bad things may happen, but this may not always be the case. And BUG_ON effectively kills the machine. You could simply use a WARN_ON, so people see that something really strange is going on but can still try to recover things. > + > + 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); same here > + > + 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); except the above Reviewed-by: Heiko Stuebner <heiko@sntech.de> [but take it with a grain of salt, as I'm still new to drm-land :-) ] Heiko
On 2015?01?31? 20:49, Heiko Stübner wrote: > Hi Mark, > > Am Samstag, 31. Januar 2015, 16:41:38 schrieb Mark Yao: >> 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 <mark.yao@rock-chips.com> >> --- >> 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); > BUG_ON is generally not well liked in general error handling - i.e. in the > !vop->is_enabled > bad things may happen, but this may not always be the case. And BUG_ON > effectively kills the machine. > > You could simply use a WARN_ON, so people see that something really strange is > going on but can still try to recover things. Right, got it. > >> + >> + 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); > same here > >> + >> + 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); > > except the above > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > [but take it with a grain of salt, as I'm still new to drm-land :-) ] Good, I saw your drm patches, I'm interested in those, very good. Mark > > Heiko > > >
Hi Mark, Heiko, On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao <mark.yao@rock-chips.com> 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 <mark.yao@rock-chips.com> > --- > 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 <djkurtz@chromium.org> > + 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 > >
On 2015?02?02? 10:07, Daniel Kurtz wrote: > Hi Mark, Heiko, > > On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao <mark.yao@rock-chips.com> 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 <mark.yao@rock-chips.com> >> --- >> 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 <djkurtz@chromium.org> > >> + 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 >> >> > >
On Mon, Feb 02, 2015 at 10:30:09AM +0800, Mark yao wrote: > On 2015?02?02? 10:07, Daniel Kurtz wrote: > >Hi Mark, Heiko, > > > >On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao <mark.yao@rock-chips.com> 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 <mark.yao@rock-chips.com> > >>--- > >> 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 if (WARN_ON(cond)) return; is what we commonly use in i915. Because it's really not any good to use BUG_ON in drm drivers, at least if you expect anyone to use fbcon on top of it (i.e. anything even remotely resembling a normal distro): Then the intial modeset is all done under console_lock, which means that _none_ of your BUG_ON will ever get out over serial console. The problem is fbcon locking, but no fool yet signed up to fix it (it's horrible, I looked ...). -Daniel
On 2015?02?02? 15:53, Daniel Vetter wrote: > On Mon, Feb 02, 2015 at 10:30:09AM +0800, Mark yao wrote: >> On 2015?02?02? 10:07, Daniel Kurtz wrote: >>> Hi Mark, Heiko, >>> >>> On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao <mark.yao@rock-chips.com> 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 <mark.yao@rock-chips.com> >>>> --- >>>> 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 > if (WARN_ON(cond)) > return; > > is what we commonly use in i915. Because it's really not any good to use > BUG_ON in drm drivers, at least if you expect anyone to use fbcon on top > of it (i.e. anything even remotely resembling a normal distro): Then the > intial modeset is all done under console_lock, which means that _none_ of > your BUG_ON will ever get out over serial console. > > The problem is fbcon locking, but no fool yet signed up to fix it (it's > horrible, I looked ...). > -Daniel I know this problem, if fbcon locking, and BUG_ON, we can't get any log from serial console, Trouble me many times. So know, if no drm log output, the first thing I do is remove the fbcon lock, and then we can see the crash dump. But here, I expect to use BUG_ON, because if BUG happen, vop register will hang system, and WARN_ON may be have no chance to output. Mark
On Feb 4, 2015 11:38 AM, "Mark yao" <mark.yao@rock-chips.com> wrote: > > On 2015?02?02? 15:53, Daniel Vetter wrote: >> >> On Mon, Feb 02, 2015 at 10:30:09AM +0800, Mark yao wrote: >>> >>> On 2015?02?02? 10:07, Daniel Kurtz wrote: >>>> >>>> Hi Mark, Heiko, >>>> >>>> On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao <mark.yao@rock-chips.com> 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 <mark.yao@rock-chips.com> >>>>> --- >>>>> 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 >> >> if (WARN_ON(cond)) >> return; >> >> is what we commonly use in i915. Because it's really not any good to use >> BUG_ON in drm drivers, at least if you expect anyone to use fbcon on top >> of it (i.e. anything even remotely resembling a normal distro): Then the >> intial modeset is all done under console_lock, which means that _none_ of >> your BUG_ON will ever get out over serial console. >> >> The problem is fbcon locking, but no fool yet signed up to fix it (it's >> horrible, I looked ...). >> -Daniel > > I know this problem, if fbcon locking, and BUG_ON, we can't get any log from serial console, Trouble > me many times. So know, if no drm log output, the first thing I do is remove the fbcon lock, and then > we can see the crash dump. > > But here, I expect to use BUG_ON, because if BUG happen, vop register will hang system, and WARN_ON > may be have no chance to output. Yeah, but danvet is suggesting we just return immediately on the WARN_ON condition, to avoid accessing the vop registers and hanging. Thus, the WARN_ON will have a chance to put out (unless the early return path goes off and finds some other unprotected path goes to access unlocked registers). -Dan > > Mark >
On 2015?02?04? 11:48, Daniel Kurtz wrote: > On Feb 4, 2015 11:38 AM, "Mark yao" <mark.yao@rock-chips.com> wrote: >> On 2015?02?02? 15:53, Daniel Vetter wrote: >>> On Mon, Feb 02, 2015 at 10:30:09AM +0800, Mark yao wrote: >>>> On 2015?02?02? 10:07, Daniel Kurtz wrote: >>>>> Hi Mark, Heiko, >>>>> >>>>> On Sat, Jan 31, 2015 at 4:41 PM, Mark Yao <mark.yao@rock-chips.com> 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 <mark.yao@rock-chips.com> >>>>>> --- >>>>>> 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 >>> if (WARN_ON(cond)) >>> return; >>> >>> is what we commonly use in i915. Because it's really not any good to use >>> BUG_ON in drm drivers, at least if you expect anyone to use fbcon on top >>> of it (i.e. anything even remotely resembling a normal distro): Then the >>> intial modeset is all done under console_lock, which means that _none_ of >>> your BUG_ON will ever get out over serial console. >>> >>> The problem is fbcon locking, but no fool yet signed up to fix it (it's >>> horrible, I looked ...). >>> -Daniel >> I know this problem, if fbcon locking, and BUG_ON, we can't get any log from serial console, Trouble >> me many times. So know, if no drm log output, the first thing I do is remove the fbcon lock, and then >> we can see the crash dump. >> >> But here, I expect to use BUG_ON, because if BUG happen, vop register will hang system, and WARN_ON >> may be have no chance to output. > Yeah, but danvet is suggesting we just return immediately on the > WARN_ON condition, to avoid accessing the vop registers and hanging. > > Thus, the WARN_ON will have a chance to put out (unless the early > return path goes off and finds some other unprotected path goes to > access unlocked registers). > > -Dan All right, I got it. >> Mark >> > >
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); + + 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, + * 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)) + 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);
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 <mark.yao@rock-chips.com> --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 76 ++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 13 deletions(-)