Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block
On 02.08.2018 12:17, Philipp Zabel wrote: > On Tue, 2018-07-31 at 12:17 +, 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
Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block
On 02.08.2018 12:17, Philipp Zabel wrote: > On Tue, 2018-07-31 at 12:17 +, 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
Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block
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
Tested-by: Philipp Zabel
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 = >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 = >pipe;
> struct drm_crtc *crtc = >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(>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(>dev->event_lock);
>
> - if
Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block
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
Tested-by: Philipp Zabel
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 = >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 = >pipe;
> struct drm_crtc *crtc = >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(>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(>dev->event_lock);
>
> - if
