| Message ID | 20200309195216.31042-14-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: mxsfb: Add i.MX7 support | expand |
On 2020-03-09 20:52, Laurent Pinchart wrote: > The driver attempts agressive power management by enabling and disabling > the AXI clock around register accesses. This results in attempts to > enable and disable the clock in the IRQ handler, which is a no-go as > preparing or unpreparing the clock may sleep. > > On the other hand, the driver enables the AXI clock when enabling the > CRTC and keeps it enabled until the CRTC is disabled. This is correct, > and renders the power management attempt pointless, as interrupts are > not supposed to occur when the CRTC is off. > > The same reasoning can be applied to the CRTC .enable_vblank() and > .disable_vblank() that are not supposed to be called when the CRTC off > and thus don't require manual handling of the AXI clock. Furthermore, > vblank handling is never enabled, which results in the vblank enable and > disable handlers never being called. > > To fix this, remove the manual clock handling in the IRQ, the CRTC > .enable_vblank() and .disable_vblank() handlers and the plane > .atomic_update() handler. We however need to handle the clock manually > in mxsfb_irq_disable() as is calls .disable_vblank() manually and is > used both at probe and remove time. > > The clock disabling is also moved to the last step of the > mxsfb_crtc_atomic_disable() function, to prepare for enabling vblank > handling. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Looks good to me! Reviewed-by: Stefan Agner <stefan@agner.ch> > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 6 ++---- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 15 ++++----------- > 2 files changed, 6 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index a8da92976d13..e324bd2a63a5 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -231,7 +231,9 @@ static void mxsfb_irq_disable(struct drm_device *drm) > { > struct mxsfb_drm_private *mxsfb = drm->dev_private; > > + mxsfb_enable_axi_clk(mxsfb); > mxsfb->crtc.funcs->disable_vblank(&mxsfb->crtc); > + mxsfb_disable_axi_clk(mxsfb); > } > > static irqreturn_t mxsfb_irq_handler(int irq, void *data) > @@ -240,8 +242,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) > struct mxsfb_drm_private *mxsfb = drm->dev_private; > u32 reg; > > - mxsfb_enable_axi_clk(mxsfb); > - > reg = readl(mxsfb->base + LCDC_CTRL1); > > if (reg & CTRL1_CUR_FRAME_DONE_IRQ) > @@ -249,8 +249,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) > > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > > - mxsfb_disable_axi_clk(mxsfb); > - > return IRQ_HANDLED; > } > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index ebe0785694cb..ac2696c8483d 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -344,9 +344,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, > struct drm_pending_vblank_event *event; > > mxsfb_disable_controller(mxsfb); > - mxsfb_disable_axi_clk(mxsfb); > - > - pm_runtime_put_sync(drm->dev); > > spin_lock_irq(&drm->event_lock); > event = crtc->state->event; > @@ -355,6 +352,9 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, > drm_crtc_send_vblank_event(crtc, event); > } > spin_unlock_irq(&drm->event_lock); > + > + mxsfb_disable_axi_clk(mxsfb); > + pm_runtime_put_sync(drm->dev); > } > > static int mxsfb_crtc_enable_vblank(struct drm_crtc *crtc) > @@ -362,10 +362,8 @@ static int mxsfb_crtc_enable_vblank(struct drm_crtc *crtc) > struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev); > > /* Clear and enable VBLANK IRQ */ > - mxsfb_enable_axi_clk(mxsfb); > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_SET); > - mxsfb_disable_axi_clk(mxsfb); > > return 0; > } > @@ -375,10 +373,8 @@ static void mxsfb_crtc_disable_vblank(struct > drm_crtc *crtc) > struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev); > > /* Disable and clear VBLANK IRQ */ > - mxsfb_enable_axi_clk(mxsfb); > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > - mxsfb_disable_axi_clk(mxsfb); > } > > static const struct drm_crtc_helper_funcs mxsfb_crtc_helper_funcs = { > @@ -433,11 +429,8 @@ static void mxsfb_plane_atomic_update(struct > drm_plane *plane, > dma_addr_t paddr; > > paddr = mxsfb_get_fb_paddr(mxsfb); > - if (paddr) { > - mxsfb_enable_axi_clk(mxsfb); > + if (paddr) > writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > - mxsfb_disable_axi_clk(mxsfb); > - } > } > > static const struct drm_plane_helper_funcs mxsfb_plane_helper_funcs = {
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index a8da92976d13..e324bd2a63a5 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -231,7 +231,9 @@ static void mxsfb_irq_disable(struct drm_device *drm) { struct mxsfb_drm_private *mxsfb = drm->dev_private; + mxsfb_enable_axi_clk(mxsfb); mxsfb->crtc.funcs->disable_vblank(&mxsfb->crtc); + mxsfb_disable_axi_clk(mxsfb); } static irqreturn_t mxsfb_irq_handler(int irq, void *data) @@ -240,8 +242,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) struct mxsfb_drm_private *mxsfb = drm->dev_private; u32 reg; - mxsfb_enable_axi_clk(mxsfb); - reg = readl(mxsfb->base + LCDC_CTRL1); if (reg & CTRL1_CUR_FRAME_DONE_IRQ) @@ -249,8 +249,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); - mxsfb_disable_axi_clk(mxsfb); - return IRQ_HANDLED; } diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index ebe0785694cb..ac2696c8483d 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -344,9 +344,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_pending_vblank_event *event; mxsfb_disable_controller(mxsfb); - mxsfb_disable_axi_clk(mxsfb); - - pm_runtime_put_sync(drm->dev); spin_lock_irq(&drm->event_lock); event = crtc->state->event; @@ -355,6 +352,9 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, drm_crtc_send_vblank_event(crtc, event); } spin_unlock_irq(&drm->event_lock); + + mxsfb_disable_axi_clk(mxsfb); + pm_runtime_put_sync(drm->dev); } static int mxsfb_crtc_enable_vblank(struct drm_crtc *crtc) @@ -362,10 +362,8 @@ static int mxsfb_crtc_enable_vblank(struct drm_crtc *crtc) struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev); /* Clear and enable VBLANK IRQ */ - mxsfb_enable_axi_clk(mxsfb); writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_SET); - mxsfb_disable_axi_clk(mxsfb); return 0; } @@ -375,10 +373,8 @@ static void mxsfb_crtc_disable_vblank(struct drm_crtc *crtc) struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev); /* Disable and clear VBLANK IRQ */ - mxsfb_enable_axi_clk(mxsfb); writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); - mxsfb_disable_axi_clk(mxsfb); } static const struct drm_crtc_helper_funcs mxsfb_crtc_helper_funcs = { @@ -433,11 +429,8 @@ static void mxsfb_plane_atomic_update(struct drm_plane *plane, dma_addr_t paddr; paddr = mxsfb_get_fb_paddr(mxsfb); - if (paddr) { - mxsfb_enable_axi_clk(mxsfb); + if (paddr) writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); - mxsfb_disable_axi_clk(mxsfb); - } } static const struct drm_plane_helper_funcs mxsfb_plane_helper_funcs = {
The driver attempts agressive power management by enabling and disabling the AXI clock around register accesses. This results in attempts to enable and disable the clock in the IRQ handler, which is a no-go as preparing or unpreparing the clock may sleep. On the other hand, the driver enables the AXI clock when enabling the CRTC and keeps it enabled until the CRTC is disabled. This is correct, and renders the power management attempt pointless, as interrupts are not supposed to occur when the CRTC is off. The same reasoning can be applied to the CRTC .enable_vblank() and .disable_vblank() that are not supposed to be called when the CRTC off and thus don't require manual handling of the AXI clock. Furthermore, vblank handling is never enabled, which results in the vblank enable and disable handlers never being called. To fix this, remove the manual clock handling in the IRQ, the CRTC .enable_vblank() and .disable_vblank() handlers and the plane .atomic_update() handler. We however need to handle the clock manually in mxsfb_irq_disable() as is calls .disable_vblank() manually and is used both at probe and remove time. The clock disabling is also moved to the last step of the mxsfb_crtc_atomic_disable() function, to prepare for enabling vblank handling. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 6 ++---- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 15 ++++----------- 2 files changed, 6 insertions(+), 15 deletions(-)