| Message ID | 20220311170601.50995-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/7] drm: mxsfb: Simplify LCDIF clock handling | expand |
Hi Marek, Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: > The current clock handling in the LCDIF driver is a convoluted mess. Here we agree... > Implement runtime PM ops which turn the clock ON and OFF and let the > pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable > and .atomic_disable callbacks turn the clock ON and OFF at the right > time. > > This requires slight reordering in mxsfb_crtc_atomic_enable() and > mxsfb_crtc_atomic_disable(), since all the register accesses must > happen only with clock enabled and clock frequency configuration > must happen with clock disabled. > ... on that one I don't. Please don't move the pixel clock into the RPM calls. We have a very well defined point between atomic enable/disable where the pixel clock is actually needed. All the other configuration accesses don't need the pixel clock to be active. > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Robby Cai <robby.cai@nxp.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Stefan Agner <stefan@agner.ch> > --- > V2: No change > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 104 +++++++++++++++++------------- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 27 +++----- > 2 files changed, 67 insertions(+), 64 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 9d71c55a31c07..c790aeff0a6f0 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -73,18 +73,6 @@ static const struct mxsfb_devdata mxsfb_devdata[] = { > }, > }; > > -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb) > -{ > - if (mxsfb->clk_axi) > - clk_prepare_enable(mxsfb->clk_axi); > -} > - > -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb) > -{ > - if (mxsfb->clk_axi) > - clk_disable_unprepare(mxsfb->clk_axi); > -} > - > static struct drm_framebuffer * > mxsfb_fb_create(struct drm_device *dev, struct drm_file *file_priv, > const struct drm_mode_fb_cmd2 *mode_cmd) > @@ -173,13 +161,9 @@ static void mxsfb_irq_disable(struct drm_device *drm) > { > struct mxsfb_drm_private *mxsfb = drm->dev_private; > > - mxsfb_enable_axi_clk(mxsfb); > - > /* Disable and clear VBLANK IRQ */ > 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 int mxsfb_irq_install(struct drm_device *dev, int irq) > @@ -225,43 +209,41 @@ static int mxsfb_load(struct drm_device *drm, > if (IS_ERR(mxsfb->clk)) > return PTR_ERR(mxsfb->clk); > > - mxsfb->clk_axi = devm_clk_get(drm->dev, "axi"); > + mxsfb->clk_axi = devm_clk_get_optional(drm->dev, "axi"); > if (IS_ERR(mxsfb->clk_axi)) > - mxsfb->clk_axi = NULL; > + return PTR_ERR(mxsfb->clk_axi); > > - mxsfb->clk_disp_axi = devm_clk_get(drm->dev, "disp_axi"); > + mxsfb->clk_disp_axi = devm_clk_get_optional(drm->dev, "disp_axi"); > if (IS_ERR(mxsfb->clk_disp_axi)) > - mxsfb->clk_disp_axi = NULL; > + return PTR_ERR(mxsfb->clk_disp_axi); > + > + platform_set_drvdata(pdev, drm); > > ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32)); > if (ret) > return ret; > > - pm_runtime_enable(drm->dev); > - > /* Modeset init */ > drm_mode_config_init(drm); > > ret = mxsfb_kms_init(mxsfb); > if (ret < 0) { > dev_err(drm->dev, "Failed to initialize KMS pipeline\n"); > - goto err_vblank; > + return ret; > } > > ret = drm_vblank_init(drm, drm->mode_config.num_crtc); > if (ret < 0) { > dev_err(drm->dev, "Failed to initialise vblank\n"); > - goto err_vblank; > + return ret; > } > > /* Start with vertical blanking interrupt reporting disabled. */ > drm_crtc_vblank_off(&mxsfb->crtc); > > ret = mxsfb_attach_bridge(mxsfb); > - if (ret) { > - dev_err_probe(drm->dev, ret, "Cannot connect bridge\n"); > - goto err_vblank; > - } > + if (ret) > + return dev_err_probe(drm->dev, ret, "Cannot connect bridge\n"); > > drm->mode_config.min_width = MXSFB_MIN_XRES; > drm->mode_config.min_height = MXSFB_MIN_YRES; > @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm, > > ret = platform_get_irq(pdev, 0); > if (ret < 0) > - goto err_vblank; > + return ret; > mxsfb->irq = ret; > > - pm_runtime_get_sync(drm->dev); > ret = mxsfb_irq_install(drm, mxsfb->irq); > - pm_runtime_put_sync(drm->dev); > - Here you do a bunch of stuff resulting in register access without enabling the clocks. I don't really see how this works, maybe because the clocks are still on when you run the probe? Better enable the register access clocks here and then tell RPM about the fact that the device is running by calling pm_runtime_set_active before pm_runtime_enable. This way theoretically allows to build a kernel without CONFIG_PM and things still work, even if the RPM calls are stubs. > if (ret < 0) { > dev_err(drm->dev, "Failed to install IRQ handler\n"); > - goto err_vblank; > + return ret; > } > > drm_kms_helper_poll_init(drm); > > - platform_set_drvdata(pdev, drm); > - > drm_helper_hpd_irq_event(drm); > > - return 0; > - > -err_vblank: > - pm_runtime_disable(drm->dev); > + pm_runtime_enable(drm->dev); > > - return ret; > + return 0; > } > > static void mxsfb_unload(struct drm_device *drm) > { > + pm_runtime_get_sync(drm->dev); > + > drm_kms_helper_poll_fini(drm); > drm_mode_config_cleanup(drm); > > - pm_runtime_get_sync(drm->dev); > mxsfb_irq_uninstall(drm); > + > pm_runtime_put_sync(drm->dev); > + pm_runtime_disable(drm->dev); > > drm->dev_private = NULL; > - > - pm_runtime_disable(drm->dev); > } > > DEFINE_DRM_GEM_CMA_FOPS(fops); > @@ -388,23 +363,60 @@ static void mxsfb_shutdown(struct platform_device *pdev) > drm_atomic_helper_shutdown(drm); > } > > -#ifdef CONFIG_PM_SLEEP > +static int mxsfb_rpm_suspend(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + struct mxsfb_drm_private *mxsfb = drm->dev_private; > + > + /* These clock supply the DISPLAY CLOCK Domain */ > + clk_disable_unprepare(mxsfb->clk); > + /* These clock supply the System Bus, AXI, Write Path, LFIFO */ > + clk_disable_unprepare(mxsfb->clk_disp_axi); > + /* These clock supply the Control Bus, APB, APBH Ctrl Registers */ > + clk_disable_unprepare(mxsfb->clk_axi); > + > + return 0; > +} > + > +static int mxsfb_rpm_resume(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + struct mxsfb_drm_private *mxsfb = drm->dev_private; > + > + /* These clock supply the Control Bus, APB, APBH Ctrl Registers */ > + clk_prepare_enable(mxsfb->clk_axi); > + /* These clock supply the System Bus, AXI, Write Path, LFIFO */ > + clk_prepare_enable(mxsfb->clk_disp_axi); > + /* These clock supply the DISPLAY CLOCK Domain */ > + clk_prepare_enable(mxsfb->clk); > + > + return 0; > +} > + > static int mxsfb_suspend(struct device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > + int ret; > > - return drm_mode_config_helper_suspend(drm); > + ret = drm_mode_config_helper_suspend(drm); > + if (ret) > + return ret; > + > + return mxsfb_rpm_suspend(dev); > } > > static int mxsfb_resume(struct device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > > + mxsfb_rpm_resume(dev); > + > return drm_mode_config_helper_resume(drm); > } > -#endif > > static const struct dev_pm_ops mxsfb_pm_ops = { > + .runtime_suspend = mxsfb_rpm_suspend, > + .runtime_resume = mxsfb_rpm_resume, SET_RUNTIME_PM_OPS(mxsfb_rpm_suspend, mxsfb_rpm_resume, NULL) > SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume) > }; > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index 4cfb6c0016799..657b6afbbf1f9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -100,10 +100,6 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > { > u32 reg; > > - if (mxsfb->clk_disp_axi) > - clk_prepare_enable(mxsfb->clk_disp_axi); > - clk_prepare_enable(mxsfb->clk); > - > /* Increase number of outstanding requests on all supported IPs */ > if (mxsfb->devdata->has_ctrl2) { > reg = readl(mxsfb->base + LCDC_V4_CTRL2); > @@ -168,10 +164,6 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb) > reg = readl(mxsfb->base + LCDC_VDCTRL4); > reg &= ~VDCTRL4_SYNC_SIGNALS_ON; > writel(reg, mxsfb->base + LCDC_VDCTRL4); > - > - clk_disable_unprepare(mxsfb->clk); > - if (mxsfb->clk_disp_axi) > - clk_disable_unprepare(mxsfb->clk_disp_axi); > } > > /* > @@ -250,8 +242,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, > > mxsfb_set_formats(mxsfb, bus_format); > > - clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); > - > if (mxsfb->bridge && mxsfb->bridge->timings) > bus_flags = mxsfb->bridge->timings->input_bus_flags; > > @@ -346,16 +336,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_atomic_state *state) > { > struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev); > + struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode; > struct drm_bridge_state *bridge_state; > struct drm_device *drm = mxsfb->drm; > u32 bus_format = 0; > dma_addr_t paddr; > > - pm_runtime_get_sync(drm->dev); > - mxsfb_enable_axi_clk(mxsfb); > - > - drm_crtc_vblank_on(crtc); > - > /* If there is a bridge attached to the LCDIF, use its bus format */ > if (mxsfb->bridge) { > bridge_state = > @@ -382,6 +368,10 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > if (!bus_format) > bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > + clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); > + > + pm_runtime_get_sync(drm->dev); > + > mxsfb_crtc_mode_set_nofb(mxsfb, bus_format); > > /* Write cur_buf as well to avoid an initial corrupt frame */ > @@ -392,6 +382,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > } > > mxsfb_enable_controller(mxsfb); > + > + drm_crtc_vblank_on(crtc); > } > > static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, > @@ -401,6 +393,8 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, > struct drm_device *drm = mxsfb->drm; > struct drm_pending_vblank_event *event; > > + drm_crtc_vblank_off(crtc); > + > mxsfb_disable_controller(mxsfb); > > spin_lock_irq(&drm->event_lock); > @@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, > } > spin_unlock_irq(&drm->event_lock); > > - drm_crtc_vblank_off(crtc); > - > - mxsfb_disable_axi_clk(mxsfb); > pm_runtime_put_sync(drm->dev); > Not the fault of your patch, but why is this a _sync call? Regards, Lucas
On 4/6/22 21:32, Lucas Stach wrote: > Hi Marek, Hi, > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: >> The current clock handling in the LCDIF driver is a convoluted mess. > > Here we agree... > >> Implement runtime PM ops which turn the clock ON and OFF and let the >> pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable >> and .atomic_disable callbacks turn the clock ON and OFF at the right >> time. >> >> This requires slight reordering in mxsfb_crtc_atomic_enable() and >> mxsfb_crtc_atomic_disable(), since all the register accesses must >> happen only with clock enabled and clock frequency configuration >> must happen with clock disabled. >> > ... on that one I don't. Please don't move the pixel clock into the RPM > calls. We have a very well defined point between atomic enable/disable > where the pixel clock is actually needed. All the other configuration > accesses don't need the pixel clock to be active. On the other hand, "all the other" configuration happens during probe, at which point all the clock are enabled anyway. And then during runtime, the pixel clock of this IP are either enabled or this IP is completely shut down. So, where exactly does this patch make the clock handling any worse than it currently is ? [...] >> @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm, >> >> ret = platform_get_irq(pdev, 0); >> if (ret < 0) >> - goto err_vblank; >> + return ret; >> mxsfb->irq = ret; >> >> - pm_runtime_get_sync(drm->dev); >> ret = mxsfb_irq_install(drm, mxsfb->irq); >> - pm_runtime_put_sync(drm->dev); >> - > Here you do a bunch of stuff resulting in register access without > enabling the clocks. I don't really see how this works, maybe because > the clocks are still on when you run the probe? Right > Better enable the register access clocks here and then tell RPM about > the fact that the device is running by calling pm_runtime_set_active > before pm_runtime_enable. This way theoretically allows to build a > kernel without CONFIG_PM and things still work, even if the RPM calls > are stubs. I would much rather move this driver to RPM and have RPM handle the clock altogether in one place. [...] >> @@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, >> } >> spin_unlock_irq(&drm->event_lock); >> >> - drm_crtc_vblank_off(crtc); >> - >> - mxsfb_disable_axi_clk(mxsfb); >> pm_runtime_put_sync(drm->dev); >> > Not the fault of your patch, but why is this a _sync call? See 4201f4e848450 ("drm/mxsfb: Add pm_runtime calls to pipe_enable/disable"), likely the intention was for this call to complete before existing the atomic_disable.
Am Mittwoch, dem 06.04.2022 um 23:45 +0200 schrieb Marek Vasut: > On 4/6/22 21:32, Lucas Stach wrote: > > Hi Marek, > > Hi, > > > Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: > > > The current clock handling in the LCDIF driver is a convoluted mess. > > > > Here we agree... > > > > > Implement runtime PM ops which turn the clock ON and OFF and let the > > > pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable > > > and .atomic_disable callbacks turn the clock ON and OFF at the right > > > time. > > > > > > This requires slight reordering in mxsfb_crtc_atomic_enable() and > > > mxsfb_crtc_atomic_disable(), since all the register accesses must > > > happen only with clock enabled and clock frequency configuration > > > must happen with clock disabled. > > > > > ... on that one I don't. Please don't move the pixel clock into the RPM > > calls. We have a very well defined point between atomic enable/disable > > where the pixel clock is actually needed. All the other configuration > > accesses don't need the pixel clock to be active. > > On the other hand, "all the other" configuration happens during probe, > at which point all the clock are enabled anyway. And then during > runtime, the pixel clock of this IP are either enabled or this IP is > completely shut down. > > So, where exactly does this patch make the clock handling any worse than > it currently is ? > There is an even stronger argument: runtime PM does not guarantee that the runtime_suspend is actually called after you put your last reference. A simple "echo on > /sys/your-device/power/control" will prevent the device from ever entering runtime suspend. So if you have a clock like the pixel clock that _needs_ to be disabled for configuration purposes you _must_ not handle this clock via RPM, but via explicit clock handling in the driver. > [...] > > > > @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm, > > > > > > ret = platform_get_irq(pdev, 0); > > > if (ret < 0) > > > - goto err_vblank; > > > + return ret; > > > mxsfb->irq = ret; > > > > > > - pm_runtime_get_sync(drm->dev); > > > ret = mxsfb_irq_install(drm, mxsfb->irq); > > > - pm_runtime_put_sync(drm->dev); > > > - > > Here you do a bunch of stuff resulting in register access without > > enabling the clocks. I don't really see how this works, maybe because > > the clocks are still on when you run the probe? > > Right > > > Better enable the register access clocks here and then tell RPM about > > the fact that the device is running by calling pm_runtime_set_active > > before pm_runtime_enable. This way theoretically allows to build a > > kernel without CONFIG_PM and things still work, even if the RPM calls > > are stubs. > > I would much rather move this driver to RPM and have RPM handle the > clock altogether in one place. > See above. Same argument applies. The driver should work without RPM support. > [...] > > > > @@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, > > > } > > > spin_unlock_irq(&drm->event_lock); > > > > > > - drm_crtc_vblank_off(crtc); > > > - > > > - mxsfb_disable_axi_clk(mxsfb); > > > pm_runtime_put_sync(drm->dev); > > > > > Not the fault of your patch, but why is this a _sync call? > > See 4201f4e848450 ("drm/mxsfb: Add pm_runtime calls to > pipe_enable/disable"), likely the intention was for this call to > complete before existing the atomic_disable. Hm, still don't see why this would be necessary. But as this was just a passing comment, we should revisit this later, not in the context of this patch. Regards, Lucas
On 4/7/22 09:57, Lucas Stach wrote: > Am Mittwoch, dem 06.04.2022 um 23:45 +0200 schrieb Marek Vasut: >> On 4/6/22 21:32, Lucas Stach wrote: >>> Hi Marek, >> >> Hi, >> >>> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut: >>>> The current clock handling in the LCDIF driver is a convoluted mess. >>> >>> Here we agree... >>> >>>> Implement runtime PM ops which turn the clock ON and OFF and let the >>>> pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable >>>> and .atomic_disable callbacks turn the clock ON and OFF at the right >>>> time. >>>> >>>> This requires slight reordering in mxsfb_crtc_atomic_enable() and >>>> mxsfb_crtc_atomic_disable(), since all the register accesses must >>>> happen only with clock enabled and clock frequency configuration >>>> must happen with clock disabled. >>>> >>> ... on that one I don't. Please don't move the pixel clock into the RPM >>> calls. We have a very well defined point between atomic enable/disable >>> where the pixel clock is actually needed. All the other configuration >>> accesses don't need the pixel clock to be active. >> >> On the other hand, "all the other" configuration happens during probe, >> at which point all the clock are enabled anyway. And then during >> runtime, the pixel clock of this IP are either enabled or this IP is >> completely shut down. >> >> So, where exactly does this patch make the clock handling any worse than >> it currently is ? >> > There is an even stronger argument: runtime PM does not guarantee that > the runtime_suspend is actually called after you put your last > reference. A simple "echo on > /sys/your-device/power/control" will > prevent the device from ever entering runtime suspend. So if you have a > clock like the pixel clock that _needs_ to be disabled for > configuration purposes you _must_ not handle this clock via RPM, but > via explicit clock handling in the driver. OK, patch discarded.
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 9d71c55a31c07..c790aeff0a6f0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -73,18 +73,6 @@ static const struct mxsfb_devdata mxsfb_devdata[] = { }, }; -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb) -{ - if (mxsfb->clk_axi) - clk_prepare_enable(mxsfb->clk_axi); -} - -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb) -{ - if (mxsfb->clk_axi) - clk_disable_unprepare(mxsfb->clk_axi); -} - static struct drm_framebuffer * mxsfb_fb_create(struct drm_device *dev, struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd) @@ -173,13 +161,9 @@ static void mxsfb_irq_disable(struct drm_device *drm) { struct mxsfb_drm_private *mxsfb = drm->dev_private; - mxsfb_enable_axi_clk(mxsfb); - /* Disable and clear VBLANK IRQ */ 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 int mxsfb_irq_install(struct drm_device *dev, int irq) @@ -225,43 +209,41 @@ static int mxsfb_load(struct drm_device *drm, if (IS_ERR(mxsfb->clk)) return PTR_ERR(mxsfb->clk); - mxsfb->clk_axi = devm_clk_get(drm->dev, "axi"); + mxsfb->clk_axi = devm_clk_get_optional(drm->dev, "axi"); if (IS_ERR(mxsfb->clk_axi)) - mxsfb->clk_axi = NULL; + return PTR_ERR(mxsfb->clk_axi); - mxsfb->clk_disp_axi = devm_clk_get(drm->dev, "disp_axi"); + mxsfb->clk_disp_axi = devm_clk_get_optional(drm->dev, "disp_axi"); if (IS_ERR(mxsfb->clk_disp_axi)) - mxsfb->clk_disp_axi = NULL; + return PTR_ERR(mxsfb->clk_disp_axi); + + platform_set_drvdata(pdev, drm); ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32)); if (ret) return ret; - pm_runtime_enable(drm->dev); - /* Modeset init */ drm_mode_config_init(drm); ret = mxsfb_kms_init(mxsfb); if (ret < 0) { dev_err(drm->dev, "Failed to initialize KMS pipeline\n"); - goto err_vblank; + return ret; } ret = drm_vblank_init(drm, drm->mode_config.num_crtc); if (ret < 0) { dev_err(drm->dev, "Failed to initialise vblank\n"); - goto err_vblank; + return ret; } /* Start with vertical blanking interrupt reporting disabled. */ drm_crtc_vblank_off(&mxsfb->crtc); ret = mxsfb_attach_bridge(mxsfb); - if (ret) { - dev_err_probe(drm->dev, ret, "Cannot connect bridge\n"); - goto err_vblank; - } + if (ret) + return dev_err_probe(drm->dev, ret, "Cannot connect bridge\n"); drm->mode_config.min_width = MXSFB_MIN_XRES; drm->mode_config.min_height = MXSFB_MIN_YRES; @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm, ret = platform_get_irq(pdev, 0); if (ret < 0) - goto err_vblank; + return ret; mxsfb->irq = ret; - pm_runtime_get_sync(drm->dev); ret = mxsfb_irq_install(drm, mxsfb->irq); - pm_runtime_put_sync(drm->dev); - if (ret < 0) { dev_err(drm->dev, "Failed to install IRQ handler\n"); - goto err_vblank; + return ret; } drm_kms_helper_poll_init(drm); - platform_set_drvdata(pdev, drm); - drm_helper_hpd_irq_event(drm); - return 0; - -err_vblank: - pm_runtime_disable(drm->dev); + pm_runtime_enable(drm->dev); - return ret; + return 0; } static void mxsfb_unload(struct drm_device *drm) { + pm_runtime_get_sync(drm->dev); + drm_kms_helper_poll_fini(drm); drm_mode_config_cleanup(drm); - pm_runtime_get_sync(drm->dev); mxsfb_irq_uninstall(drm); + pm_runtime_put_sync(drm->dev); + pm_runtime_disable(drm->dev); drm->dev_private = NULL; - - pm_runtime_disable(drm->dev); } DEFINE_DRM_GEM_CMA_FOPS(fops); @@ -388,23 +363,60 @@ static void mxsfb_shutdown(struct platform_device *pdev) drm_atomic_helper_shutdown(drm); } -#ifdef CONFIG_PM_SLEEP +static int mxsfb_rpm_suspend(struct device *dev) +{ + struct drm_device *drm = dev_get_drvdata(dev); + struct mxsfb_drm_private *mxsfb = drm->dev_private; + + /* These clock supply the DISPLAY CLOCK Domain */ + clk_disable_unprepare(mxsfb->clk); + /* These clock supply the System Bus, AXI, Write Path, LFIFO */ + clk_disable_unprepare(mxsfb->clk_disp_axi); + /* These clock supply the Control Bus, APB, APBH Ctrl Registers */ + clk_disable_unprepare(mxsfb->clk_axi); + + return 0; +} + +static int mxsfb_rpm_resume(struct device *dev) +{ + struct drm_device *drm = dev_get_drvdata(dev); + struct mxsfb_drm_private *mxsfb = drm->dev_private; + + /* These clock supply the Control Bus, APB, APBH Ctrl Registers */ + clk_prepare_enable(mxsfb->clk_axi); + /* These clock supply the System Bus, AXI, Write Path, LFIFO */ + clk_prepare_enable(mxsfb->clk_disp_axi); + /* These clock supply the DISPLAY CLOCK Domain */ + clk_prepare_enable(mxsfb->clk); + + return 0; +} + static int mxsfb_suspend(struct device *dev) { struct drm_device *drm = dev_get_drvdata(dev); + int ret; - return drm_mode_config_helper_suspend(drm); + ret = drm_mode_config_helper_suspend(drm); + if (ret) + return ret; + + return mxsfb_rpm_suspend(dev); } static int mxsfb_resume(struct device *dev) { struct drm_device *drm = dev_get_drvdata(dev); + mxsfb_rpm_resume(dev); + return drm_mode_config_helper_resume(drm); } -#endif static const struct dev_pm_ops mxsfb_pm_ops = { + .runtime_suspend = mxsfb_rpm_suspend, + .runtime_resume = mxsfb_rpm_resume, SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume) }; diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 4cfb6c0016799..657b6afbbf1f9 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -100,10 +100,6 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) { u32 reg; - if (mxsfb->clk_disp_axi) - clk_prepare_enable(mxsfb->clk_disp_axi); - clk_prepare_enable(mxsfb->clk); - /* Increase number of outstanding requests on all supported IPs */ if (mxsfb->devdata->has_ctrl2) { reg = readl(mxsfb->base + LCDC_V4_CTRL2); @@ -168,10 +164,6 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb) reg = readl(mxsfb->base + LCDC_VDCTRL4); reg &= ~VDCTRL4_SYNC_SIGNALS_ON; writel(reg, mxsfb->base + LCDC_VDCTRL4); - - clk_disable_unprepare(mxsfb->clk); - if (mxsfb->clk_disp_axi) - clk_disable_unprepare(mxsfb->clk_disp_axi); } /* @@ -250,8 +242,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, mxsfb_set_formats(mxsfb, bus_format); - clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); - if (mxsfb->bridge && mxsfb->bridge->timings) bus_flags = mxsfb->bridge->timings->input_bus_flags; @@ -346,16 +336,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) { struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev); + struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode; struct drm_bridge_state *bridge_state; struct drm_device *drm = mxsfb->drm; u32 bus_format = 0; dma_addr_t paddr; - pm_runtime_get_sync(drm->dev); - mxsfb_enable_axi_clk(mxsfb); - - drm_crtc_vblank_on(crtc); - /* If there is a bridge attached to the LCDIF, use its bus format */ if (mxsfb->bridge) { bridge_state = @@ -382,6 +368,10 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, if (!bus_format) bus_format = MEDIA_BUS_FMT_RGB888_1X24; + clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); + + pm_runtime_get_sync(drm->dev); + mxsfb_crtc_mode_set_nofb(mxsfb, bus_format); /* Write cur_buf as well to avoid an initial corrupt frame */ @@ -392,6 +382,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, } mxsfb_enable_controller(mxsfb); + + drm_crtc_vblank_on(crtc); } static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, @@ -401,6 +393,8 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_device *drm = mxsfb->drm; struct drm_pending_vblank_event *event; + drm_crtc_vblank_off(crtc); + mxsfb_disable_controller(mxsfb); spin_lock_irq(&drm->event_lock); @@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, } spin_unlock_irq(&drm->event_lock); - drm_crtc_vblank_off(crtc); - - mxsfb_disable_axi_clk(mxsfb); pm_runtime_put_sync(drm->dev); }
The current clock handling in the LCDIF driver is a convoluted mess. Implement runtime PM ops which turn the clock ON and OFF and let the pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable and .atomic_disable callbacks turn the clock ON and OFF at the right time. This requires slight reordering in mxsfb_crtc_atomic_enable() and mxsfb_crtc_atomic_disable(), since all the register accesses must happen only with clock enabled and clock frequency configuration must happen with clock disabled. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Peng Fan <peng.fan@nxp.com> Cc: Robby Cai <robby.cai@nxp.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Stefan Agner <stefan@agner.ch> --- V2: No change --- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 104 +++++++++++++++++------------- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 27 +++----- 2 files changed, 67 insertions(+), 64 deletions(-)