| Message ID | f873c5592e9907beba905ce68358d5ae2f63a4d7.1531824173.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote: > Adding lcdif nodes to a power domain currently does work, it results in > black/corrupted screens or hangs. While the driver does enable runtime > pm it does not deal correctly with the block being unpowered. > > Ensure power is on when required by adding pm_runtime_get/put_sync to > mxsfb_pipe_enable/disable. > > Since power is lost on suspend implement PM_SLEEP_OPS using > drm_mode_config_helper_suspend/resume. > > The mxsfb_plane_atomic_update function can get called before > mxsfb_pipe_enable while the block is not yet powered. When this happens > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank > until a refresh. > > Fix this by not writing gem->paddr if the block is not enabled and > instead delaying the write until the next mxsfb_crtc_mode_set_nofb call. > At that point also update cur_buf to avoid an initial corrupt frame > after resume. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> This is a gentle reminder that this patch was sent two weeks ago without any reply. Since v1 went ~1 month without a reply it makes sense to send an explicit ping. Can somebody please take a look at this? > --- > > The purpose of this patch is to prepare for enabling power gating on > DISPLAY power domains. > > Changes since v1: > * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling > pm_runtime_get/put in pipe enable/disable is enough. > * Use drm_mode_config_helper_suspend/resume instead of attempting to > track state manually. > * Don't touch NEXT_BUF if atomic_update called with crtc disabled > * Also update CUR_BUF on enable, this avoids initial corrupt frames. > > Previous discussion: https://lkml.org/lkml/2018/7/17/329 > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++------- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 25 +++++++++++++++ > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 2 ++ > 3 files changed, 67 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 0abe77675b76..10153da77c40 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -194,15 +194,31 @@ static int mxsfb_reset_block(void __iomem *reset_addr) > return ret; > > return clear_poll_bit(reset_addr, MODULE_CLKGATE); > } > > +static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb) > +{ > + struct drm_framebuffer *fb = mxsfb->pipe.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) > { > struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; > const u32 bus_flags = mxsfb->connector.display_info.bus_flags; > u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; > + dma_addr_t paddr; > 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 > @@ -268,35 +284,47 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > mxsfb->base + LCDC_VDCTRL3); > > writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), > mxsfb->base + LCDC_VDCTRL4); > > + /* Update cur_buf as well to avoid an initial corrupt frame */ > + paddr = mxsfb_get_fb_paddr(mxsfb); > + if (paddr) { > + writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf); > + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + } > mxsfb_disable_axi_clk(mxsfb); > } > > void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > { > + if (mxsfb->enabled) > + return; > + > mxsfb_crtc_mode_set_nofb(mxsfb); > mxsfb_enable_controller(mxsfb); > + > + mxsfb->enabled = true; > } > > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > { > + if (!mxsfb->enabled) > + return; > + > mxsfb_disable_controller(mxsfb); > + > + mxsfb->enabled = false; > } > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > struct drm_plane_state *state) > { > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > struct drm_crtc *crtc = &pipe->crtc; > - struct drm_framebuffer *fb = pipe->plane.state->fb; > struct drm_pending_vblank_event *event; > - struct drm_gem_cma_object *gem; > - > - if (!crtc) > - return; > + dma_addr_t paddr; > > spin_lock_irq(&crtc->dev->event_lock); > event = crtc->state->event; > if (event) { > crtc->state->event = NULL; > @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > drm_crtc_send_vblank_event(crtc, event); > } > } > spin_unlock_irq(&crtc->dev->event_lock); > > - if (!fb) > + if (!mxsfb->enabled) > return; > > - gem = drm_fb_cma_get_gem_obj(fb, 0); > - > - mxsfb_enable_axi_clk(mxsfb); > - writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf); > - mxsfb_disable_axi_clk(mxsfb); > + paddr = mxsfb_get_fb_paddr(mxsfb); > + if (paddr) { > + mxsfb_enable_axi_clk(mxsfb); > + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + mxsfb_disable_axi_clk(mxsfb); > + } > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index dd1dd58e4956..a5269fccbed9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -101,23 +101,27 @@ static const struct drm_mode_config_funcs mxsfb_mode_config_funcs = { > static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + struct drm_device *drm = pipe->plane.dev; > > + pm_runtime_get_sync(drm->dev); > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > drm_panel_enable(mxsfb->panel); > } > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + struct drm_device *drm = pipe->plane.dev; > > drm_panel_disable(mxsfb->panel); > mxsfb_crtc_disable(mxsfb); > drm_panel_unprepare(mxsfb->panel); > + pm_runtime_put_sync(drm->dev); > } > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > struct drm_plane_state *plane_state) > { > @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev) > drm_dev_unref(drm); > > return 0; > } > > +#ifdef CONFIG_PM > +static int mxsfb_suspend(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + > + return drm_mode_config_helper_suspend(drm); > +} > + > +static int mxsfb_resume(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + > + return drm_mode_config_helper_resume(drm); > +} > +#endif > + > +static const struct dev_pm_ops mxsfb_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume) > +}; > + > static struct platform_driver mxsfb_platform_driver = { > .probe = mxsfb_probe, > .remove = mxsfb_remove, > .id_table = mxsfb_devtype, > .driver = { > .name = "mxsfb-drm", > .of_match_table = mxsfb_dt_ids, > + .pm = &mxsfb_pm_ops, > }, > }; > > module_platform_driver(mxsfb_platform_driver); > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 5d0883fc805b..e539d4b05c48 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -36,10 +36,12 @@ struct mxsfb_drm_private { > > struct drm_simple_display_pipe pipe; > struct drm_connector connector; > struct drm_panel *panel; > struct drm_fbdev_cma *fbdev; > + > + bool enabled; > }; > > int mxsfb_setup_crtc(struct drm_device *dev); > int mxsfb_create_output(struct drm_device *dev); >
On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote: > Adding lcdif nodes to a power domain currently does work, it results in > black/corrupted screens or hangs. While the driver does enable runtime > pm it does not deal correctly with the block being unpowered. > > Ensure power is on when required by adding pm_runtime_get/put_sync to > mxsfb_pipe_enable/disable. > > Since power is lost on suspend implement PM_SLEEP_OPS using > drm_mode_config_helper_suspend/resume. > > The mxsfb_plane_atomic_update function can get called before > mxsfb_pipe_enable while the block is not yet powered. When this happens > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank > until a refresh. Why does this happen? > Fix this by not writing gem->paddr if the block is not enabled and > instead delaying the write until the next mxsfb_crtc_mode_set_nofb call. > At that point also update cur_buf to avoid an initial corrupt frame > after resume. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> Tested-by: Philipp Zabel <p.zabel@pengutronix.de> on imx6ull-evk, this fixes the initial corrupt frame. regards Philipp > > --- > > The purpose of this patch is to prepare for enabling power gating on > DISPLAY power domains. > > Changes since v1: > * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling > pm_runtime_get/put in pipe enable/disable is enough. > * Use drm_mode_config_helper_suspend/resume instead of attempting to > track state manually. > * Don't touch NEXT_BUF if atomic_update called with crtc disabled > * Also update CUR_BUF on enable, this avoids initial corrupt frames. > > Previous discussion: https://lkml.org/lkml/2018/7/17/329 > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++------- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 25 +++++++++++++++ > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 2 ++ > 3 files changed, 67 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 0abe77675b76..10153da77c40 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -194,15 +194,31 @@ static int mxsfb_reset_block(void __iomem *reset_addr) > return ret; > > return clear_poll_bit(reset_addr, MODULE_CLKGATE); > } > > +static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb) > +{ > + struct drm_framebuffer *fb = mxsfb->pipe.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) > { > struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; > const u32 bus_flags = mxsfb->connector.display_info.bus_flags; > u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; > + dma_addr_t paddr; > 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 > @@ -268,35 +284,47 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > mxsfb->base + LCDC_VDCTRL3); > > writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), > mxsfb->base + LCDC_VDCTRL4); > > + /* Update cur_buf as well to avoid an initial corrupt frame */ > + paddr = mxsfb_get_fb_paddr(mxsfb); > + if (paddr) { > + writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf); > + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + } > mxsfb_disable_axi_clk(mxsfb); > } > > void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > { > + if (mxsfb->enabled) > + return; > + > mxsfb_crtc_mode_set_nofb(mxsfb); > mxsfb_enable_controller(mxsfb); > + > + mxsfb->enabled = true; > } > > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > { > + if (!mxsfb->enabled) > + return; > + > mxsfb_disable_controller(mxsfb); > + > + mxsfb->enabled = false; > } > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > struct drm_plane_state *state) > { > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > struct drm_crtc *crtc = &pipe->crtc; > - struct drm_framebuffer *fb = pipe->plane.state->fb; > struct drm_pending_vblank_event *event; > - struct drm_gem_cma_object *gem; > - > - if (!crtc) > - return; > + dma_addr_t paddr; > > spin_lock_irq(&crtc->dev->event_lock); > event = crtc->state->event; > if (event) { > crtc->state->event = NULL; > @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > drm_crtc_send_vblank_event(crtc, event); > } > } > spin_unlock_irq(&crtc->dev->event_lock); > > - if (!fb) > + if (!mxsfb->enabled) > return; > > - gem = drm_fb_cma_get_gem_obj(fb, 0); > - > - mxsfb_enable_axi_clk(mxsfb); > - writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf); > - mxsfb_disable_axi_clk(mxsfb); > + paddr = mxsfb_get_fb_paddr(mxsfb); > + if (paddr) { > + mxsfb_enable_axi_clk(mxsfb); > + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + mxsfb_disable_axi_clk(mxsfb); > + } > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index dd1dd58e4956..a5269fccbed9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -101,23 +101,27 @@ static const struct drm_mode_config_funcs mxsfb_mode_config_funcs = { > static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + struct drm_device *drm = pipe->plane.dev; > > + pm_runtime_get_sync(drm->dev); > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > drm_panel_enable(mxsfb->panel); > } > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + struct drm_device *drm = pipe->plane.dev; > > drm_panel_disable(mxsfb->panel); > mxsfb_crtc_disable(mxsfb); > drm_panel_unprepare(mxsfb->panel); > + pm_runtime_put_sync(drm->dev); > } > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > struct drm_plane_state *plane_state) > { > @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev) > drm_dev_unref(drm); > > return 0; > } > > +#ifdef CONFIG_PM > +static int mxsfb_suspend(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + > + return drm_mode_config_helper_suspend(drm); > +} > + > +static int mxsfb_resume(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + > + return drm_mode_config_helper_resume(drm); > +} > +#endif > + > +static const struct dev_pm_ops mxsfb_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume) > +}; > + > static struct platform_driver mxsfb_platform_driver = { > .probe = mxsfb_probe, > .remove = mxsfb_remove, > .id_table = mxsfb_devtype, > .driver = { > .name = "mxsfb-drm", > .of_match_table = mxsfb_dt_ids, > + .pm = &mxsfb_pm_ops, > }, > }; > > module_platform_driver(mxsfb_platform_driver); > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 5d0883fc805b..e539d4b05c48 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -36,10 +36,12 @@ struct mxsfb_drm_private { > > struct drm_simple_display_pipe pipe; > struct drm_connector connector; > struct drm_panel *panel; > struct drm_fbdev_cma *fbdev; > + > + bool enabled; > }; > > int mxsfb_setup_crtc(struct drm_device *dev); > int mxsfb_create_output(struct drm_device *dev); >
On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote: > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote: > > Adding lcdif nodes to a power domain currently does work, it results in > > black/corrupted screens or hangs. While the driver does enable runtime > > pm it does not deal correctly with the block being unpowered. > > > > Ensure power is on when required by adding pm_runtime_get/put_sync to > > mxsfb_pipe_enable/disable. > > > > Since power is lost on suspend implement PM_SLEEP_OPS using > > drm_mode_config_helper_suspend/resume. > > > > The mxsfb_plane_atomic_update function can get called before > > mxsfb_pipe_enable while the block is not yet powered. When this happens > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank > > until a refresh. > > Why does this happen? I'm not sure what you're asking but register writes to unpowered or unclocked blocks are not expected to "just work". Here the write is ignored/lost but I think on imx8 it can even cause a bus error. The approach here is to only set the framebuffer address as part of activating the display. > > Fix this by not writing gem->paddr if the block is not enabled and > > instead delaying the write until the next mxsfb_crtc_mode_set_nofb call. > > At that point also update cur_buf to avoid an initial corrupt frame > > after resume. > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > > on imx6ull-evk, this fixes the initial corrupt frame. Cool!
On 17.07.2018 12:48, Leonard Crestez wrote: > Adding lcdif nodes to a power domain currently does work, it results in > black/corrupted screens or hangs. While the driver does enable runtime > pm it does not deal correctly with the block being unpowered. > > Ensure power is on when required by adding pm_runtime_get/put_sync to > mxsfb_pipe_enable/disable. > > Since power is lost on suspend implement PM_SLEEP_OPS using > drm_mode_config_helper_suspend/resume. > > The mxsfb_plane_atomic_update function can get called before > mxsfb_pipe_enable while the block is not yet powered. When this happens > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank > until a refresh. > > Fix this by not writing gem->paddr if the block is not enabled and > instead delaying the write until the next mxsfb_crtc_mode_set_nofb call. > At that point also update cur_buf to avoid an initial corrupt frame > after resume. You seem to do two things in a single patch. I know they are related, but it still seems better if you split it up: 1. Introduce mxsfb_get_fb_paddr and set framebuffer pointers properly (this seems to have a positive effect even without PM as reported by Philipp) 2. Add PM callbacks I think in a follow up patch we can also use runtime pm to enable/disable the AXI clock. Add driver level runtime pm functions which enable disable the clocks. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > --- > > The purpose of this patch is to prepare for enabling power gating on > DISPLAY power domains. > > Changes since v1: > * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling > pm_runtime_get/put in pipe enable/disable is enough. > * Use drm_mode_config_helper_suspend/resume instead of attempting to > track state manually. > * Don't touch NEXT_BUF if atomic_update called with crtc disabled > * Also update CUR_BUF on enable, this avoids initial corrupt frames. > > Previous discussion: https://lkml.org/lkml/2018/7/17/329 > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++------- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 25 +++++++++++++++ > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 2 ++ > 3 files changed, 67 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 0abe77675b76..10153da77c40 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -194,15 +194,31 @@ static int mxsfb_reset_block(void __iomem *reset_addr) > return ret; > > return clear_poll_bit(reset_addr, MODULE_CLKGATE); > } > > +static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb) > +{ > + struct drm_framebuffer *fb = mxsfb->pipe.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) > { > struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; > const u32 bus_flags = mxsfb->connector.display_info.bus_flags; > u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; > + dma_addr_t paddr; > 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 > @@ -268,35 +284,47 @@ static void mxsfb_crtc_mode_set_nofb(struct > mxsfb_drm_private *mxsfb) > mxsfb->base + LCDC_VDCTRL3); > > writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), > mxsfb->base + LCDC_VDCTRL4); > > + /* Update cur_buf as well to avoid an initial corrupt frame */ > + paddr = mxsfb_get_fb_paddr(mxsfb); > + if (paddr) { > + writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf); > + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + } > mxsfb_disable_axi_clk(mxsfb); > } > > void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > { > + if (mxsfb->enabled) > + return; > + > mxsfb_crtc_mode_set_nofb(mxsfb); > mxsfb_enable_controller(mxsfb); > + > + mxsfb->enabled = true; > } > > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > { > + if (!mxsfb->enabled) > + return; > + > mxsfb_disable_controller(mxsfb); > + > + mxsfb->enabled = false; > } > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > struct drm_plane_state *state) > { > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > struct drm_crtc *crtc = &pipe->crtc; > - struct drm_framebuffer *fb = pipe->plane.state->fb; > struct drm_pending_vblank_event *event; > - struct drm_gem_cma_object *gem; > - > - if (!crtc) > - return; > + dma_addr_t paddr; > > spin_lock_irq(&crtc->dev->event_lock); > event = crtc->state->event; > if (event) { > crtc->state->event = NULL; > @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct > mxsfb_drm_private *mxsfb, > drm_crtc_send_vblank_event(crtc, event); > } > } > spin_unlock_irq(&crtc->dev->event_lock); > > - if (!fb) > + if (!mxsfb->enabled) > return; > I think this is the wrong thing to do. The simple KMS helper callback ->update() is called by the ->atomic_update() callback of drm_plane_helper_funcs. And the documentation of atomic_update() states: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_plane_helper_funcs "Note that the power state of the display pipe when this function is called depends upon the exact helpers and calling sequence the driver has picked. See drm_atomic_helper_commit_planes() for a discussion of the tradeoffs and variants of plane commit helpers." The documentation of drm_atomic_helper_commit_planes() then has more information: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_atomic_helper_commit_planes Bottom line, we should be using drm_atomic_helper_commit_tail_rpm() for runtime pm... So adding something like: static const struct drm_mode_config_helper_funcs mxsfb_drm_mode_config_helpers = { .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, }; And add something like this in mxsfb_load: drm->mode_config.funcs = &mxsfb_mode_config_funcs; + dev->mode_config.helper_private = &mxsfb_drm_mode_config_helpers; ... Should make the stack not calling update while the pipe is disabled. With that you do not have to keep state locally and can always apply the new state in ->enable(). > - gem = drm_fb_cma_get_gem_obj(fb, 0); > - > - mxsfb_enable_axi_clk(mxsfb); > - writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf); > - mxsfb_disable_axi_clk(mxsfb); > + paddr = mxsfb_get_fb_paddr(mxsfb); > + if (paddr) { > + mxsfb_enable_axi_clk(mxsfb); > + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + mxsfb_disable_axi_clk(mxsfb); > + } > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index dd1dd58e4956..a5269fccbed9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -101,23 +101,27 @@ static const struct drm_mode_config_funcs > mxsfb_mode_config_funcs = { > static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + struct drm_device *drm = pipe->plane.dev; > > + pm_runtime_get_sync(drm->dev); > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > drm_panel_enable(mxsfb->panel); > } > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + struct drm_device *drm = pipe->plane.dev; > > drm_panel_disable(mxsfb->panel); > mxsfb_crtc_disable(mxsfb); > drm_panel_unprepare(mxsfb->panel); > + pm_runtime_put_sync(drm->dev); > } > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > struct drm_plane_state *plane_state) > { > @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev) > drm_dev_unref(drm); > > return 0; > } > > +#ifdef CONFIG_PM This should be CONFIG_PM_SLEEP afaict. -- Stefan > +static int mxsfb_suspend(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + > + return drm_mode_config_helper_suspend(drm); > +} > + > +static int mxsfb_resume(struct device *dev) > +{ > + struct drm_device *drm = dev_get_drvdata(dev); > + > + return drm_mode_config_helper_resume(drm); > +} > +#endif > + > +static const struct dev_pm_ops mxsfb_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume) > +}; > + > static struct platform_driver mxsfb_platform_driver = { > .probe = mxsfb_probe, > .remove = mxsfb_remove, > .id_table = mxsfb_devtype, > .driver = { > .name = "mxsfb-drm", > .of_match_table = mxsfb_dt_ids, > + .pm = &mxsfb_pm_ops, > }, > }; > > module_platform_driver(mxsfb_platform_driver); > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 5d0883fc805b..e539d4b05c48 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -36,10 +36,12 @@ struct mxsfb_drm_private { > > struct drm_simple_display_pipe pipe; > struct drm_connector connector; > struct drm_panel *panel; > struct drm_fbdev_cma *fbdev; > + > + bool enabled; > }; > > int mxsfb_setup_crtc(struct drm_device *dev); > int mxsfb_create_output(struct drm_device *dev);
On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote: > On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote: > > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote: > > > Adding lcdif nodes to a power domain currently does work, it results in > > > black/corrupted screens or hangs. While the driver does enable runtime > > > pm it does not deal correctly with the block being unpowered. > > > > > > Ensure power is on when required by adding pm_runtime_get/put_sync to > > > mxsfb_pipe_enable/disable. > > > > > > Since power is lost on suspend implement PM_SLEEP_OPS using > > > drm_mode_config_helper_suspend/resume. > > > > > > The mxsfb_plane_atomic_update function can get called before > > > mxsfb_pipe_enable while the block is not yet powered. When this happens > > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank > > > until a refresh. > > > > Why does this happen? > > I'm not sure what you're asking but register writes to unpowered or > unclocked blocks are not expected to "just work". Here the write is > ignored/lost but I think on imx8 it can even cause a bus error. > > The approach here is to only set the framebuffer address as part of > activating the display. I wonder why atomic update is called at all while the pipe is not enabled. regards Philipp
On 02.08.2018 12:17, Philipp Zabel wrote: > On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote: >> On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote: >> > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote: >> > > Adding lcdif nodes to a power domain currently does work, it results in >> > > black/corrupted screens or hangs. While the driver does enable runtime >> > > pm it does not deal correctly with the block being unpowered. >> > > >> > > Ensure power is on when required by adding pm_runtime_get/put_sync to >> > > mxsfb_pipe_enable/disable. >> > > >> > > Since power is lost on suspend implement PM_SLEEP_OPS using >> > > drm_mode_config_helper_suspend/resume. >> > > >> > > The mxsfb_plane_atomic_update function can get called before >> > > mxsfb_pipe_enable while the block is not yet powered. When this happens >> > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank >> > > until a refresh. >> > >> > Why does this happen? >> >> I'm not sure what you're asking but register writes to unpowered or >> unclocked blocks are not expected to "just work". Here the write is >> ignored/lost but I think on imx8 it can even cause a bus error. >> >> The approach here is to only set the framebuffer address as part of >> activating the display. > > I wonder why atomic update is called at all while the pipe is not > enabled. It can be made to behave differently (see also my review). However, the default seems also a bit unfortunate to me. -- Stefan
On Thu, 2018-08-02 at 13:06 +0200, Stefan Agner wrote: > On 02.08.2018 12:17, Philipp Zabel wrote: > > On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote: > > > On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote: > > > > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote: > > > > > Adding lcdif nodes to a power domain currently does work, it results in > > > > > black/corrupted screens or hangs. While the driver does enable runtime > > > > > pm it does not deal correctly with the block being unpowered. > > > > > > > > > > Ensure power is on when required by adding pm_runtime_get/put_sync to > > > > > mxsfb_pipe_enable/disable. > > > > > > > > > > Since power is lost on suspend implement PM_SLEEP_OPS using > > > > > drm_mode_config_helper_suspend/resume. > > > > > > > > > > The mxsfb_plane_atomic_update function can get called before > > > > > mxsfb_pipe_enable while the block is not yet powered. When this happens > > > > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank > > > > > until a refresh. > > > > > > > > Why does this happen? > > > > > > I'm not sure what you're asking but register writes to unpowered or > > > unclocked blocks are not expected to "just work". Here the write is > > > ignored/lost but I think on imx8 it can even cause a bus error. > > > > > > The approach here is to only set the framebuffer address as part of > > > activating the display. > > > > I wonder why atomic update is called at all while the pipe is not > > enabled. > > It can be made to behave differently (see also my review). Setting the framebuffer before enabling the crtc makes sense to me, otherwise an initial corrupt frame would be impossible to avoid. It's just that it slightly complicates PM for simple devices. Maybe drm_simple_display_pipe could have prepare/unprepare callbacks for PM+modeset separate from just crtc enable/disable?
Hi Stefan, On Wed, 2018-08-01 at 12:00 +0200, Stefan Agner wrote: [...] > > @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct > > mxsfb_drm_private *mxsfb, > > drm_crtc_send_vblank_event(crtc, event); > > } > > } > > spin_unlock_irq(&crtc->dev->event_lock); > > > > - if (!fb) > > + if (!mxsfb->enabled) > > return; > > > > I think this is the wrong thing to do. > > The simple KMS helper callback ->update() is called by the > ->atomic_update() callback of drm_plane_helper_funcs. > > And the documentation of atomic_update() states: > https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_plane_helper_funcs > > "Note that the power state of the display pipe when this function is > called depends upon the exact helpers and calling sequence the driver > has picked. See drm_atomic_helper_commit_planes() for a discussion of > the tradeoffs and variants of plane commit helpers." > > The documentation of drm_atomic_helper_commit_planes() then has more > information: > https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_atomic_helper_commit_planes > > Bottom line, we should be using drm_atomic_helper_commit_tail_rpm() for > runtime pm... > > So adding something like: > > static const struct drm_mode_config_helper_funcs > mxsfb_drm_mode_config_helpers = { > .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > }; > > And add something like this in mxsfb_load: > > drm->mode_config.funcs = &mxsfb_mode_config_funcs; > + dev->mode_config.helper_private = &mxsfb_drm_mode_config_helpers; > ... > > Should make the stack not calling update while the pipe is disabled. > > With that you do not have to keep state locally and can always apply the > new state in ->enable(). Yes, thank you for the explanation. That is exactly what I would have expected. regards Philipp
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c index 0abe77675b76..10153da77c40 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c @@ -194,15 +194,31 @@ static int mxsfb_reset_block(void __iomem *reset_addr) return ret; return clear_poll_bit(reset_addr, MODULE_CLKGATE); } +static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb) +{ + struct drm_framebuffer *fb = mxsfb->pipe.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) { struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; const u32 bus_flags = mxsfb->connector.display_info.bus_flags; u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; + dma_addr_t paddr; 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 @@ -268,35 +284,47 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) mxsfb->base + LCDC_VDCTRL3); writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), mxsfb->base + LCDC_VDCTRL4); + /* Update cur_buf as well to avoid an initial corrupt frame */ + paddr = mxsfb_get_fb_paddr(mxsfb); + if (paddr) { + writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf); + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); + } mxsfb_disable_axi_clk(mxsfb); } void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) { + if (mxsfb->enabled) + return; + mxsfb_crtc_mode_set_nofb(mxsfb); mxsfb_enable_controller(mxsfb); + + mxsfb->enabled = true; } void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) { + if (!mxsfb->enabled) + return; + mxsfb_disable_controller(mxsfb); + + mxsfb->enabled = false; } void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, struct drm_plane_state *state) { struct drm_simple_display_pipe *pipe = &mxsfb->pipe; struct drm_crtc *crtc = &pipe->crtc; - struct drm_framebuffer *fb = pipe->plane.state->fb; struct drm_pending_vblank_event *event; - struct drm_gem_cma_object *gem; - - if (!crtc) - return; + dma_addr_t paddr; spin_lock_irq(&crtc->dev->event_lock); event = crtc->state->event; if (event) { crtc->state->event = NULL; @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, drm_crtc_send_vblank_event(crtc, event); } } spin_unlock_irq(&crtc->dev->event_lock); - if (!fb) + if (!mxsfb->enabled) return; - gem = drm_fb_cma_get_gem_obj(fb, 0); - - mxsfb_enable_axi_clk(mxsfb); - writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf); - mxsfb_disable_axi_clk(mxsfb); + paddr = mxsfb_get_fb_paddr(mxsfb); + if (paddr) { + mxsfb_enable_axi_clk(mxsfb); + writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); + mxsfb_disable_axi_clk(mxsfb); + } } diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index dd1dd58e4956..a5269fccbed9 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -101,23 +101,27 @@ static const struct drm_mode_config_funcs mxsfb_mode_config_funcs = { static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state, struct drm_plane_state *plane_state) { struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); + struct drm_device *drm = pipe->plane.dev; + pm_runtime_get_sync(drm->dev); drm_panel_prepare(mxsfb->panel); mxsfb_crtc_enable(mxsfb); drm_panel_enable(mxsfb->panel); } static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) { struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); + struct drm_device *drm = pipe->plane.dev; drm_panel_disable(mxsfb->panel); mxsfb_crtc_disable(mxsfb); drm_panel_unprepare(mxsfb->panel); + pm_runtime_put_sync(drm->dev); } static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *plane_state) { @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev) drm_dev_unref(drm); return 0; } +#ifdef CONFIG_PM +static int mxsfb_suspend(struct device *dev) +{ + struct drm_device *drm = dev_get_drvdata(dev); + + return drm_mode_config_helper_suspend(drm); +} + +static int mxsfb_resume(struct device *dev) +{ + struct drm_device *drm = dev_get_drvdata(dev); + + return drm_mode_config_helper_resume(drm); +} +#endif + +static const struct dev_pm_ops mxsfb_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume) +}; + static struct platform_driver mxsfb_platform_driver = { .probe = mxsfb_probe, .remove = mxsfb_remove, .id_table = mxsfb_devtype, .driver = { .name = "mxsfb-drm", .of_match_table = mxsfb_dt_ids, + .pm = &mxsfb_pm_ops, }, }; module_platform_driver(mxsfb_platform_driver); diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h index 5d0883fc805b..e539d4b05c48 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h @@ -36,10 +36,12 @@ struct mxsfb_drm_private { struct drm_simple_display_pipe pipe; struct drm_connector connector; struct drm_panel *panel; struct drm_fbdev_cma *fbdev; + + bool enabled; }; int mxsfb_setup_crtc(struct drm_device *dev); int mxsfb_create_output(struct drm_device *dev);
Adding lcdif nodes to a power domain currently does work, it results in black/corrupted screens or hangs. While the driver does enable runtime pm it does not deal correctly with the block being unpowered. Ensure power is on when required by adding pm_runtime_get/put_sync to mxsfb_pipe_enable/disable. Since power is lost on suspend implement PM_SLEEP_OPS using drm_mode_config_helper_suspend/resume. The mxsfb_plane_atomic_update function can get called before mxsfb_pipe_enable while the block is not yet powered. When this happens the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank until a refresh. Fix this by not writing gem->paddr if the block is not enabled and instead delaying the write until the next mxsfb_crtc_mode_set_nofb call. At that point also update cur_buf to avoid an initial corrupt frame after resume. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- The purpose of this patch is to prepare for enabling power gating on DISPLAY power domains. Changes since v1: * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling pm_runtime_get/put in pipe enable/disable is enough. * Use drm_mode_config_helper_suspend/resume instead of attempting to track state manually. * Don't touch NEXT_BUF if atomic_update called with crtc disabled * Also update CUR_BUF on enable, this avoids initial corrupt frames. Previous discussion: https://lkml.org/lkml/2018/7/17/329 --- drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++------- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 25 +++++++++++++++ drivers/gpu/drm/mxsfb/mxsfb_drv.h | 2 ++ 3 files changed, 67 insertions(+), 11 deletions(-)