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@xxxxxxx> > > --- > > 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); _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
