Re: [PATCH v3 1/4] drm/mxsfb: Fix initial corrupt frame when activating display

On 06.08.2018 21:31, Leonard Crestez wrote:

LCDIF will repeatedly display data from CUR_BUF and set CUR_BUF to NEXT_BUF when done. Since we are only ever writing to NEXT_BUF the display will show an initial corrupt frame.

Fix by writing the FB paddr to both CUR_BUF and NEXT_BUF when activating the CRTC.

Really like how this patch set evolves! Only some minor thing below...

Signed-off-by: Leonard Crestez leonard.crestez@nxp.com

drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 42 +++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c index 0abe77675b76..db3ff5bde122 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,10 +284,16 @@ 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);
  

• }

Traditionally mxsfb_crtc_mode_set_nofb() was used as a callback to mode_set_nofb. And that callback should not fiddle with the fb (as the name says).

So I rather prefer to move the new bits to mxsfb_crtc_enable(), between mxsfb_crtc_mode_set_nofb() and mxsfb_enable_controller(). It is only cosmetics, but I'd rather prefer to have it in the right place.

Currently the AXI clock is disabled between those functions... I suggest to enable the AXI clock once in mxsfb_crtc_enable(). This puts an end to the unnecessary enable/disable/enable sequence in mxsfb_crtc_enable().

So I suggest to create a patch "drm/mxsfb: simplify AXI clock handling"

--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c @@ -129,7 +129,6 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) if (mxsfb->clk_disp_axi) clk_prepare_enable(mxsfb->clk_disp_axi); clk_prepare_enable(mxsfb->clk); - mxsfb_enable_axi_clk(mxsfb);

/* If it was disabled, re-enable the mode again */ writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); @@ -159,8 +158,6 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb) reg &= ~VDCTRL4_SYNC_SIGNALS_ON; writel(reg, mxsfb->base + LCDC_VDCTRL4);

- mxsfb_disable_axi_clk(mxsfb); - clk_disable_unprepare(mxsfb->clk); if (mxsfb->clk_disp_axi) clk_disable_unprepare(mxsfb->clk_disp_axi); @@ -208,7 +205,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) * running. This may lead to shifted pictures (FIFO issue?), so * first stop the controller and drain its FIFOs. */ - mxsfb_enable_axi_clk(mxsfb);

/* Mandatory eLCDIF reset as per the Reference Manual */ err = mxsfb_reset_block(mxsfb->base); @@ -269,12 +265,11 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)

writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay), mxsfb->base + LCDC_VDCTRL4); - - mxsfb_disable_axi_clk(mxsfb); }

void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) { + mxsfb_enable_axi_clk(mxsfb); mxsfb_crtc_mode_set_nofb(mxsfb); mxsfb_enable_controller(mxsfb); } @@ -282,6 +277,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) { mxsfb_disable_controller(mxsfb); + mxsfb_disable_axi_clk(mxsfb); }

Then move the above section between mxsfb_crtc_mode_set_nofb() and mxsfb_enable_controller().

-- Stefan

mxsfb_disable_axi_clk(mxsfb); }

void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) { @@ -287,16 +309,12 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) 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 +325,12 @@ 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)

• 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);
  

• }

}