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(-)
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 call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation from the LCDIF block already and this is called in mxsfb_load() before request_irq(), so explicitly disabling IRQ using custom function like mxsfb_irq_disable() is not needed, remove it. The request_irq() call would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove the unnecessary check. Finally, remove both mxsfb_irq_install() and mxsfb_irq_uninstall() as well, since they are no longer useful.
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 | 38 +++++++------------------------ 1 file changed, 8 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index c790aeff0a6f0..94cafff7f68d5 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -157,33 +157,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) return IRQ_HANDLED; }
-static void mxsfb_irq_disable(struct drm_device *drm) -{ - struct mxsfb_drm_private *mxsfb = drm->dev_private; - - /* 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); -} - -static int mxsfb_irq_install(struct drm_device *dev, int irq) -{ - if (irq == IRQ_NOTCONNECTED) - return -ENOTCONN; - - mxsfb_irq_disable(dev); - - return request_irq(irq, mxsfb_irq_handler, 0, dev->driver->name, dev); -} - -static void mxsfb_irq_uninstall(struct drm_device *dev) -{ - struct mxsfb_drm_private *mxsfb = dev->dev_private; - - mxsfb_irq_disable(dev); - free_irq(mxsfb->irq, dev); -} - static int mxsfb_load(struct drm_device *drm, const struct mxsfb_devdata *devdata) { @@ -259,7 +232,8 @@ static int mxsfb_load(struct drm_device *drm, return ret; mxsfb->irq = ret;
- ret = mxsfb_irq_install(drm, mxsfb->irq); + ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0, + drm->driver->name, drm); if (ret < 0) { dev_err(drm->dev, "Failed to install IRQ handler\n"); return ret; @@ -276,16 +250,20 @@ static int mxsfb_load(struct drm_device *drm,
static void mxsfb_unload(struct drm_device *drm) { + struct mxsfb_drm_private *mxsfb = drm->dev_private; + pm_runtime_get_sync(drm->dev);
+ drm_crtc_vblank_off(&mxsfb->crtc); + drm_kms_helper_poll_fini(drm); drm_mode_config_cleanup(drm);
- mxsfb_irq_uninstall(drm); - pm_runtime_put_sync(drm->dev); pm_runtime_disable(drm->dev);
+ free_irq(mxsfb->irq, drm->dev); + drm->dev_private = NULL; }
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation from the LCDIF block already and this is called in mxsfb_load() before request_irq(), so explicitly disabling IRQ using custom function like mxsfb_irq_disable() is not needed, remove it.
Have you checked that the drm_vblank_off in probe actually results in a call to mxsfb_crtc_disable_vblank? From my reading of the core code, this should be a no-op without a previous drm_vblank_on, so it would not result in the desired masking before the IRQ is requested.
Regards, Lucas
The request_irq() call would return -ENOTCONN if IRQ is IRQ_NOTCONNECTED already, so remove the unnecessary check. Finally, remove both mxsfb_irq_install() and mxsfb_irq_uninstall() as well, since they are no longer useful.
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 | 38 +++++++------------------------ 1 file changed, 8 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index c790aeff0a6f0..94cafff7f68d5 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -157,33 +157,6 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) return IRQ_HANDLED; }
-static void mxsfb_irq_disable(struct drm_device *drm) -{
- struct mxsfb_drm_private *mxsfb = drm->dev_private;
- /* 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);
-}
-static int mxsfb_irq_install(struct drm_device *dev, int irq) -{
- if (irq == IRQ_NOTCONNECTED)
return -ENOTCONN;- mxsfb_irq_disable(dev);
- return request_irq(irq, mxsfb_irq_handler, 0, dev->driver->name, dev);
-}
-static void mxsfb_irq_uninstall(struct drm_device *dev) -{
- struct mxsfb_drm_private *mxsfb = dev->dev_private;
- mxsfb_irq_disable(dev);
- free_irq(mxsfb->irq, dev);
-}
static int mxsfb_load(struct drm_device *drm, const struct mxsfb_devdata *devdata) { @@ -259,7 +232,8 @@ static int mxsfb_load(struct drm_device *drm, return ret; mxsfb->irq = ret;
- ret = mxsfb_irq_install(drm, mxsfb->irq);
- ret = request_irq(mxsfb->irq, mxsfb_irq_handler, 0,
drm->driver->name, drm);@@ -276,16 +250,20 @@ static int mxsfb_load(struct drm_device *drm,
static void mxsfb_unload(struct drm_device *drm) {
struct mxsfb_drm_private *mxsfb = drm->dev_private;
pm_runtime_get_sync(drm->dev);
drm_crtc_vblank_off(&mxsfb->crtc);
drm_kms_helper_poll_fini(drm); drm_mode_config_cleanup(drm);
- mxsfb_irq_uninstall(drm);
- pm_runtime_put_sync(drm->dev); pm_runtime_disable(drm->dev);
- free_irq(mxsfb->irq, drm->dev);
- drm->dev_private = NULL;
}
On 4/6/22 21:41, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation from the LCDIF block already and this is called in mxsfb_load() before request_irq(), so explicitly disabling IRQ using custom function like mxsfb_irq_disable() is not needed, remove it.
Have you checked that the drm_vblank_off in probe actually results in a call to mxsfb_crtc_disable_vblank? From my reading of the core code, this should be a no-op without a previous drm_vblank_on, so it would not result in the desired masking before the IRQ is requested.
I must've missed the vblank->enabled check, but then, I am also not getting any interrupts, so presumably they are already disabled after the IP is reset.
Am Donnerstag, dem 07.04.2022 um 00:03 +0200 schrieb Marek Vasut:
On 4/6/22 21:41, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation from the LCDIF block already and this is called in mxsfb_load() before request_irq(), so explicitly disabling IRQ using custom function like mxsfb_irq_disable() is not needed, remove it.
Have you checked that the drm_vblank_off in probe actually results in a call to mxsfb_crtc_disable_vblank? From my reading of the core code, this should be a no-op without a previous drm_vblank_on, so it would not result in the desired masking before the IRQ is requested.
I must've missed the vblank->enabled check, but then, I am also not getting any interrupts, so presumably they are already disabled after the IP is reset.
Yep, it should be the default for every peripheral to not send any unsolicited interrupts. But then I don't see explicit reset of the IP in the driver probe. So maybe this is a workaround for situation where something running before Linux has already enabled the display controller and for whatever reason also enabled the interrupt requests?
Either we should have a proper reset of the block in the probe path, or this interrupt masking must be kept in one form or the other. My vote would be on just masking the IRQs and dropping the useless drm_vblank_off.
Regards, Lucas
On 4/7/22 10:01, Lucas Stach wrote:
Am Donnerstag, dem 07.04.2022 um 00:03 +0200 schrieb Marek Vasut:
On 4/6/22 21:41, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
The call to drm_crtc_vblank_off(&lcdif->crtc); disables IRQ generation from the LCDIF block already and this is called in mxsfb_load() before request_irq(), so explicitly disabling IRQ using custom function like mxsfb_irq_disable() is not needed, remove it.
Have you checked that the drm_vblank_off in probe actually results in a call to mxsfb_crtc_disable_vblank? From my reading of the core code, this should be a no-op without a previous drm_vblank_on, so it would not result in the desired masking before the IRQ is requested.
I must've missed the vblank->enabled check, but then, I am also not getting any interrupts, so presumably they are already disabled after the IP is reset.
Yep, it should be the default for every peripheral to not send any unsolicited interrupts. But then I don't see explicit reset of the IP in the driver probe. So maybe this is a workaround for situation where something running before Linux has already enabled the display controller and for whatever reason also enabled the interrupt requests?
Either we should have a proper reset of the block in the probe path, or this interrupt masking must be kept in one form or the other. My vote would be on just masking the IRQs and dropping the useless drm_vblank_off.
I can just discard this patch, OK.
Wrap FIFO reset and comments into mxsfb_reset_block(), this is a clean up. No functional change.
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_kms.c | 36 +++++++++++++++++-------------- 1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 657b6afbbf1f9..015b289d93a3c 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -183,6 +183,12 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb) { int ret;
+ /* + * It seems, you can't re-program the controller if it is still + * running. This may lead to shifted pictures (FIFO issue?), so + * first stop the controller and drain its FIFOs. + */ + ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_SFTRST); if (ret) return ret; @@ -193,7 +199,20 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb) if (ret) return ret;
- return clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE); + ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE); + if (ret) + return ret; + + /* Clear the FIFOs */ + writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET); + readl(mxsfb->base + LCDC_CTRL1); + writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR); + readl(mxsfb->base + LCDC_CTRL1); + + if (mxsfb->devdata->has_overlay) + writel(0, mxsfb->base + LCDC_AS_CTRL); + + return 0; }
static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) @@ -220,26 +239,11 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; int err;
- /* - * It seems, you can't re-program the controller if it is still - * running. This may lead to shifted pictures (FIFO issue?), so - * first stop the controller and drain its FIFOs. - */ - /* Mandatory eLCDIF reset as per the Reference Manual */ err = mxsfb_reset_block(mxsfb); if (err) return;
- /* Clear the FIFOs */ - writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET); - readl(mxsfb->base + LCDC_CTRL1); - writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR); - readl(mxsfb->base + LCDC_CTRL1); - - if (mxsfb->devdata->has_overlay) - writel(0, mxsfb->base + LCDC_AS_CTRL); - mxsfb_set_formats(mxsfb, bus_format);
if (mxsfb->bridge && mxsfb->bridge->timings)
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Wrap FIFO reset and comments into mxsfb_reset_block(), this is a clean up. No functional change.
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
Reviewed-by: Lucas Stach l.stach@pengutronix.de
V2: No change
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 36 +++++++++++++++++-------------- 1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 657b6afbbf1f9..015b289d93a3c 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -183,6 +183,12 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb) { int ret;
- /*
* It seems, you can't re-program the controller if it is still* running. This may lead to shifted pictures (FIFO issue?), so* first stop the controller and drain its FIFOs.*/- ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_SFTRST); if (ret) return ret;
@@ -193,7 +199,20 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb) if (ret) return ret;
- return clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
- ret = clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
- if (ret)
return ret;- /* Clear the FIFOs */
- writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
- readl(mxsfb->base + LCDC_CTRL1);
- writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
- readl(mxsfb->base + LCDC_CTRL1);
- if (mxsfb->devdata->has_overlay)
writel(0, mxsfb->base + LCDC_AS_CTRL);- return 0;
}
static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) @@ -220,26 +239,11 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; int err;
/*
* It seems, you can't re-program the controller if it is still* running. This may lead to shifted pictures (FIFO issue?), so* first stop the controller and drain its FIFOs.*//* Mandatory eLCDIF reset as per the Reference Manual */ err = mxsfb_reset_block(mxsfb); if (err) return;
/* Clear the FIFOs */
writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
readl(mxsfb->base + LCDC_CTRL1);
writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_CLR);
readl(mxsfb->base + LCDC_CTRL1);
if (mxsfb->devdata->has_overlay)
writel(0, mxsfb->base + LCDC_AS_CTRL);mxsfb_set_formats(mxsfb, bus_format);
if (mxsfb->bridge && mxsfb->bridge->timings)
Move mxsfb_get_fb_paddr() out of the way, away from register IO functions. This is a clean up. No functional change.
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_kms.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 015b289d93a3c..7b0abd0472aae 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -43,6 +43,21 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val) mxsfb->devdata->hs_wdth_shift; }
+static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) +{ + struct drm_framebuffer *fb = plane->state->fb; + struct drm_gem_cma_object *gem; + + if (!fb) + return 0; + + gem = drm_fb_cma_get_gem_obj(fb, 0); + if (!gem) + return 0; + + return gem->paddr; +} + /* * Setup the MXSFB registers for decoding the pixels out of the framebuffer and * outputting them on the bus. @@ -215,21 +230,6 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb) return 0; }
-static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) -{ - struct drm_framebuffer *fb = plane->state->fb; - struct drm_gem_cma_object *gem; - - if (!fb) - return 0; - - gem = drm_fb_cma_get_gem_obj(fb, 0); - if (!gem) - return 0; - - return gem->paddr; -} - static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, const u32 bus_format) {
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Move mxsfb_get_fb_paddr() out of the way, away from register IO functions. This is a clean up. No functional change.
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
Hm, I don't see any real benefit, but I also fail to see why it shouldn't be done so: Reviewed-by: Lucas Stach l.stach@pengutronix.de
V2: No change
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 015b289d93a3c..7b0abd0472aae 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -43,6 +43,21 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val) mxsfb->devdata->hs_wdth_shift; }
+static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) +{
- struct drm_framebuffer *fb = plane->state->fb;
- struct drm_gem_cma_object *gem;
- if (!fb)
return 0;- gem = drm_fb_cma_get_gem_obj(fb, 0);
- if (!gem)
return 0;- return gem->paddr;
+}
/*
- Setup the MXSFB registers for decoding the pixels out of the framebuffer and
- outputting them on the bus.
@@ -215,21 +230,6 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb) return 0; }
-static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) -{
- struct drm_framebuffer *fb = plane->state->fb;
- struct drm_gem_cma_object *gem;
- if (!fb)
return 0;- gem = drm_fb_cma_get_gem_obj(fb, 0);
- if (!gem)
return 0;- return gem->paddr;
-}
static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, const u32 bus_format) {
On 4/6/22 21:45, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Move mxsfb_get_fb_paddr() out of the way, away from register IO functions. This is a clean up. No functional change.
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
Hm, I don't see any real benefit, but I also fail to see why it shouldn't be done so:
The entire point of this series is to clean up the mxsfb and isolate lcdif (the original lcdif) from any of the common code.
Am Donnerstag, dem 07.04.2022 um 00:05 +0200 schrieb Marek Vasut:
On 4/6/22 21:45, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Move mxsfb_get_fb_paddr() out of the way, away from register IO functions. This is a clean up. No functional change.
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
Hm, I don't see any real benefit, but I also fail to see why it shouldn't be done so:
The entire point of this series is to clean up the mxsfb and isolate lcdif (the original lcdif) from any of the common code.
Actually, just use drm_fb_cma_get_gem_addr() instead?
Regards, Lucas
On 4/7/22 11:47, Lucas Stach wrote:
Am Donnerstag, dem 07.04.2022 um 00:05 +0200 schrieb Marek Vasut:
On 4/6/22 21:45, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Move mxsfb_get_fb_paddr() out of the way, away from register IO functions. This is a clean up. No functional change.
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
Hm, I don't see any real benefit, but I also fail to see why it shouldn't be done so:
The entire point of this series is to clean up the mxsfb and isolate lcdif (the original lcdif) from any of the common code.
Actually, just use drm_fb_cma_get_gem_addr() instead?
That function seems to add only extra code that is executed, but does not do away with the !fb check anyway. So, why ? (Also, seems unrelated to this patch)
Am Montag, dem 11.04.2022 um 00:17 +0200 schrieb Marek Vasut:
On 4/7/22 11:47, Lucas Stach wrote:
Am Donnerstag, dem 07.04.2022 um 00:05 +0200 schrieb Marek Vasut:
On 4/6/22 21:45, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Move mxsfb_get_fb_paddr() out of the way, away from register IO functions. This is a clean up. No functional change.
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
Hm, I don't see any real benefit, but I also fail to see why it shouldn't be done so:
The entire point of this series is to clean up the mxsfb and isolate lcdif (the original lcdif) from any of the common code.
Actually, just use drm_fb_cma_get_gem_addr() instead?
That function seems to add only extra code that is executed,
Yep, and thus it is the correct way to do it, as it actually takes into account the FB offset parameter. Currently mxsfb seems to just do the wrong thing when the FB is not at offset 0 in the GEM object.
but does not do away with the !fb check anyway.
And that one seems bogus. If you have no FB there is no way you can reasonably start scanout or flip to the next buffer. What would you scan out in that case? Random memory? FB should never be NULL in those code paths.
So, why ? (Also, seems unrelated to this patch)
Because you aim to clean up the driver and make the code reusable, so why not use the reusable and correct DRM helper?
Regards, Lucas
Pull mode registers programming from mxsfb_enable_controller() into dedicated function mxsfb_set_mode(). This is a clean up. No functional change.
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_kms.c | 96 +++++++++++++++++-------------- 1 file changed, 52 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 7b0abd0472aae..14f5cc590a51b 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -111,6 +111,57 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb, writel(ctrl, mxsfb->base + LCDC_CTRL); }
+static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags) +{ + struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode; + u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; + + writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) | + TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay), + mxsfb->base + mxsfb->devdata->transfer_count); + + vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start; + + vdctrl0 = VDCTRL0_ENABLE_PRESENT | /* Always in DOTCLOCK mode */ + VDCTRL0_VSYNC_PERIOD_UNIT | + VDCTRL0_VSYNC_PULSE_WIDTH_UNIT | + VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len); + if (m->flags & DRM_MODE_FLAG_PHSYNC) + vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH; + if (m->flags & DRM_MODE_FLAG_PVSYNC) + vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH; + /* Make sure Data Enable is high active by default */ + if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) + vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; + /* + * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric, + * controllers VDCTRL0_DOTCLK is display centric. + * Drive on positive edge -> display samples on falling edge + * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING + */ + if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) + vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; + + writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); + + /* Frame length in lines. */ + writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1); + + /* Line length in units of clocks or pixels. */ + hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start; + writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) | + VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal), + mxsfb->base + LCDC_VDCTRL2); + + writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) | + SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start), + mxsfb->base + LCDC_VDCTRL3); + + writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), + mxsfb->base + LCDC_VDCTRL4); + +} + static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) { u32 reg; @@ -236,7 +287,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, struct drm_device *drm = mxsfb->crtc.dev; struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode; u32 bus_flags = mxsfb->connector->display_info.bus_flags; - u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; int err;
/* Mandatory eLCDIF reset as per the Reference Manual */ @@ -256,49 +306,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, bus_flags); DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
- writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) | - TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay), - mxsfb->base + mxsfb->devdata->transfer_count); - - vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start; - - vdctrl0 = VDCTRL0_ENABLE_PRESENT | /* Always in DOTCLOCK mode */ - VDCTRL0_VSYNC_PERIOD_UNIT | - VDCTRL0_VSYNC_PULSE_WIDTH_UNIT | - VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len); - if (m->flags & DRM_MODE_FLAG_PHSYNC) - vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH; - if (m->flags & DRM_MODE_FLAG_PVSYNC) - vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH; - /* Make sure Data Enable is high active by default */ - if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) - vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; - /* - * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric, - * controllers VDCTRL0_DOTCLK is display centric. - * Drive on positive edge -> display samples on falling edge - * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING - */ - if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) - vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; - - writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); - - /* Frame length in lines. */ - writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1); - - /* Line length in units of clocks or pixels. */ - hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start; - writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) | - VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal), - mxsfb->base + LCDC_VDCTRL2); - - writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) | - SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start), - mxsfb->base + LCDC_VDCTRL3); - - writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), - mxsfb->base + LCDC_VDCTRL4); + mxsfb_set_mode(mxsfb, bus_flags); }
static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Pull mode registers programming from mxsfb_enable_controller() into dedicated function mxsfb_set_mode(). This is a clean up. No functional change.
This one however looks like over-factorization to me. Why pull out a mode_set function out of a mode_set function?
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_kms.c | 96 +++++++++++++++++-------------- 1 file changed, 52 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 7b0abd0472aae..14f5cc590a51b 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -111,6 +111,57 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb, writel(ctrl, mxsfb->base + LCDC_CTRL); }
+static void mxsfb_set_mode(struct mxsfb_drm_private *mxsfb, u32 bus_flags) +{
- struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
- u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
- writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),mxsfb->base + mxsfb->devdata->transfer_count);- vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
- vdctrl0 = VDCTRL0_ENABLE_PRESENT | /* Always in DOTCLOCK mode */
VDCTRL0_VSYNC_PERIOD_UNIT |VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);- if (m->flags & DRM_MODE_FLAG_PHSYNC)
vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;- if (m->flags & DRM_MODE_FLAG_PVSYNC)
vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;- /* Make sure Data Enable is high active by default */
- if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;- /*
* DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,* controllers VDCTRL0_DOTCLK is display centric.* Drive on positive edge -> display samples on falling edge* DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING*/- if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;- writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
- /* Frame length in lines. */
- writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
- /* Line length in units of clocks or pixels. */
- hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
- writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),mxsfb->base + LCDC_VDCTRL2);- writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),mxsfb->base + LCDC_VDCTRL3);- writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
mxsfb->base + LCDC_VDCTRL4);+}
static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) { u32 reg; @@ -236,7 +287,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, struct drm_device *drm = mxsfb->crtc.dev; struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode; u32 bus_flags = mxsfb->connector->display_info.bus_flags;
u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; int err;
/* Mandatory eLCDIF reset as per the Reference Manual */
@@ -256,49 +306,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, bus_flags); DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
- writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),mxsfb->base + mxsfb->devdata->transfer_count);- vsync_pulse_len = m->crtc_vsync_end - m->crtc_vsync_start;
- vdctrl0 = VDCTRL0_ENABLE_PRESENT | /* Always in DOTCLOCK mode */
VDCTRL0_VSYNC_PERIOD_UNIT |VDCTRL0_VSYNC_PULSE_WIDTH_UNIT |VDCTRL0_SET_VSYNC_PULSE_WIDTH(vsync_pulse_len);- if (m->flags & DRM_MODE_FLAG_PHSYNC)
vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;- if (m->flags & DRM_MODE_FLAG_PVSYNC)
vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;- /* Make sure Data Enable is high active by default */
- if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;- /*
* DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric,* controllers VDCTRL0_DOTCLK is display centric.* Drive on positive edge -> display samples on falling edge* DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING*/- if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING;- writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
- /* Frame length in lines. */
- writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1);
- /* Line length in units of clocks or pixels. */
- hsync_pulse_len = m->crtc_hsync_end - m->crtc_hsync_start;
- writel(set_hsync_pulse_width(mxsfb, hsync_pulse_len) |
VDCTRL2_SET_HSYNC_PERIOD(m->crtc_htotal),mxsfb->base + LCDC_VDCTRL2);- writel(SET_HOR_WAIT_CNT(m->crtc_htotal - m->crtc_hsync_start) |
SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),mxsfb->base + LCDC_VDCTRL3);- writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
mxsfb->base + LCDC_VDCTRL4);
- mxsfb_set_mode(mxsfb, bus_flags);
}
static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
On 4/6/22 21:47, Lucas Stach wrote:
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
Pull mode registers programming from mxsfb_enable_controller() into dedicated function mxsfb_set_mode(). This is a clean up. No functional change.
This one however looks like over-factorization to me. Why pull out a mode_set function out of a mode_set function?
The entire point of this series is to clean up the mxsfb and isolate lcdif (the original lcdif) from any of the common code. Then I can just replace those functions with lcdif mx8mp variant ones in the other lcdif driver, while keeping the common code in sync (until deduplication happens).
Reorder mxsfb_crtc_mode_set_nofb() such that all functions which perform register IO are called from one single location in this function. This is a clean up. No functional change.
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_kms.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 14f5cc590a51b..497603964add8 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -289,13 +289,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, u32 bus_flags = mxsfb->connector->display_info.bus_flags; int err;
- /* Mandatory eLCDIF reset as per the Reference Manual */ - err = mxsfb_reset_block(mxsfb); - if (err) - return; - - mxsfb_set_formats(mxsfb, bus_format); - if (mxsfb->bridge && mxsfb->bridge->timings) bus_flags = mxsfb->bridge->timings->input_bus_flags;
@@ -306,6 +299,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, bus_flags); DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
+ /* Mandatory eLCDIF reset as per the Reference Manual */ + err = mxsfb_reset_block(mxsfb); + if (err) + return; + + mxsfb_set_formats(mxsfb, bus_format); + mxsfb_set_mode(mxsfb, bus_flags); }
Am Freitag, dem 11.03.2022 um 18:06 +0100 schrieb Marek Vasut:
Reorder mxsfb_crtc_mode_set_nofb() such that all functions which perform register IO are called from one single location in this function. This is a clean up. No functional change.
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
Reviewed-by: Lucas Stach l.stach@pengutronix.de
V2: No change
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 14f5cc590a51b..497603964add8 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -289,13 +289,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, u32 bus_flags = mxsfb->connector->display_info.bus_flags; int err;
- /* Mandatory eLCDIF reset as per the Reference Manual */
- err = mxsfb_reset_block(mxsfb);
- if (err)
return;- mxsfb_set_formats(mxsfb, bus_format);
- if (mxsfb->bridge && mxsfb->bridge->timings) bus_flags = mxsfb->bridge->timings->input_bus_flags;
@@ -306,6 +299,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb, bus_flags); DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
- /* Mandatory eLCDIF reset as per the Reference Manual */
- err = mxsfb_reset_block(mxsfb);
- if (err)
return;- mxsfb_set_formats(mxsfb, bus_format);
- mxsfb_set_mode(mxsfb, bus_flags);
}
Pull functionality responsible for programming framebuffer address into the controller into dedicated function mxsfb_update_buffer(). This is a clean up. No functional change.
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_kms.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 497603964add8..4baa3db1f3d10 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -58,6 +58,22 @@ static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) return gem->paddr; }
+static void +mxsfb_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane, + bool both) +{ + dma_addr_t paddr; + + paddr = mxsfb_get_fb_paddr(plane); + if (!paddr) + return; + + if (both) + writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf); + + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); +} + /* * Setup the MXSFB registers for decoding the pixels out of the framebuffer and * outputting them on the bus. @@ -352,7 +368,6 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_bridge_state *bridge_state; struct drm_device *drm = mxsfb->drm; u32 bus_format = 0; - dma_addr_t paddr;
/* If there is a bridge attached to the LCDIF, use its bus format */ if (mxsfb->bridge) { @@ -387,11 +402,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
/* Write cur_buf as well to avoid an initial corrupt frame */ - paddr = mxsfb_get_fb_paddr(crtc->primary); - if (paddr) { - writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf); - writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); - } + mxsfb_update_buffer(mxsfb, crtc->primary, true);
mxsfb_enable_controller(mxsfb);
@@ -491,11 +502,8 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev); - dma_addr_t paddr;
- paddr = mxsfb_get_fb_paddr(plane); - if (paddr) - writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); + mxsfb_update_buffer(mxsfb, plane, false); }
static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
Am Freitag, dem 11.03.2022 um 18:06 +0100 schrieb Marek Vasut:
Pull functionality responsible for programming framebuffer address into the controller into dedicated function mxsfb_update_buffer(). This is a clean up. No functional change.
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
Reviewed-by: Lucas Stach l.stach@pengutronix.de
V2: No change
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 497603964add8..4baa3db1f3d10 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -58,6 +58,22 @@ static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane) return gem->paddr; }
+static void +mxsfb_update_buffer(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane,
bool both)+{
- dma_addr_t paddr;
- paddr = mxsfb_get_fb_paddr(plane);
- if (!paddr)
return;- if (both)
writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);- writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+}
/*
- Setup the MXSFB registers for decoding the pixels out of the framebuffer and
- outputting them on the bus.
@@ -352,7 +368,6 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_bridge_state *bridge_state; struct drm_device *drm = mxsfb->drm; u32 bus_format = 0;
dma_addr_t paddr;
/* If there is a bridge attached to the LCDIF, use its bus format */ if (mxsfb->bridge) {
@@ -387,11 +402,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
/* Write cur_buf as well to avoid an initial corrupt frame */
- paddr = mxsfb_get_fb_paddr(crtc->primary);
- if (paddr) {
writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);- }
mxsfb_update_buffer(mxsfb, crtc->primary, true);
mxsfb_enable_controller(mxsfb);
@@ -491,11 +502,8 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
dma_addr_t paddr;
paddr = mxsfb_get_fb_paddr(plane);
if (paddr)
writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
- mxsfb_update_buffer(mxsfb, plane, false);
}
static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
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;
- 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;
- 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;
- 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.
dri-devel@lists.freedesktop.org
-
Lucas Stach -
Marek Vasut