Re: [PATCH 4/4 v2] drm/mcde: Enable the DSI link with display

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Aug 09, 2020 at 12:31:22AM +0200, Linus Walleij wrote:
> The MCDE DSI link hardware which is modeled like a bridge
> in DRM, connected further to the panel bridge, creating
> a pipeline.
> 
> We have been using the .pre_enable(), .enable(),
> .disable() and .post_disable() callbacks from the bridge
> to set this up in a chained manner: first the display
> controller goes online and then in successive order
> each bridge in the pipeline. Inside DRM it works
> like this:
> 
> drm_atomic_helper_commit_tail()
>   drm_atomic_helper_commit_modeset_enables()
>     struct drm_crtc_helper_funcs .atomic_enable()
>       struct drm_simple_display_pipe_funcs .enable()
>         MCDE display enable call
>     drm_atomic_bridge_chain_enable()
>       struct drm_bridge_funcs .pre_enable()
>         mcde_dsi_bridge_pre_enable()
>         panel_bridge_pre_enable()
>           struct drm_panel_funcs .prepare()
>       struct drm_bridge_funcs .enable()
>         mcde_dsi_bridge_enable()
>         panel_bridge_enable()
>           struct drm_panel_funcs .enable()
> 
> A similar sequence is executed for disabling.
> 
> Unfortunately this is not what the hardware needs: at
> a certain stage in the enablement of the display
> controller the DSI link needs to come up to support
> video mode, else something (like a FIFO flow) locks
> up the hardware and we never get picture.
> 
> Fix this by simply leaving the pre|enable and
> post|disable callbacks unused, and establish two
> cross-calls from the display controller to bring up
> the DSI link at the right place in the display
> bring-up sequence and vice versa in the shutdown
> sequence.
> 
> For command mode displays, it works just fine to
> also enable the display flow early. The only time
> we hold it back right now is in one-shot mode,
> on-demand display updates.
> 
> When combined with the previous patch and some patches
> for the S6E63M0 display controller to support DSI
> mode, this gives working display on the Samsung
> GT-I8190 (Golden) phone. It has also been tested working
> on the Samsung GT-S7710 (Skomer) phone.
> 
> Cc: newbytee@xxxxxxxxxxxxxx
> Cc: Stephan Gerhold <stephan@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

So the idea kinda was that if you need this, you'd write your own modeset
code. That's why atomic helpers are fairly modular. Then you can place the
various drm_bridge_enable/disable calls exactly where you need them to
make your upstream blocks happy.

You could even do that with simple pipe helpers by overwriting the
atomic_commit_tail function with your special enable/disable sequence.
Since you have simple hw this would look something like:

static void mcde_helper_commit_tail(struct drm_atomic_state *old_state)
{
	struct drm_device *dev = old_state->dev;
	bool fence_cookie = dma_fence_begin_signalling();

	mcde_display_disable(...)

	drm_atomic_helper_commit_planes(dev, old_state, 0);

	mcde_display_enable(....)

	drm_atomic_helper_fake_vblank(old_state);

	drm_atomic_helper_commit_hw_done(old_state);

	dma_fence_end_signalling(fence_cookie);

	drm_atomic_helper_wait_for_vblanks(dev, old_state);

	drm_atomic_helper_cleanup_planes(dev, old_state);
}

And mcde_display_enable/disable would then call into bridges and anything
else you need to be called.

Anyway just an idea, for patches 2-4:

Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> ---
>  drivers/gpu/drm/mcde/mcde_display.c | 36 +++++++++++++++++------
>  drivers/gpu/drm/mcde/mcde_drm.h     |  2 ++
>  drivers/gpu/drm/mcde/mcde_dsi.c     | 44 +++++++++++------------------
>  3 files changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index d608cc894e01..c271e5bf042e 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
> @@ -999,6 +999,16 @@ static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
>  	mcde_configure_fifo(mcde, MCDE_FIFO_A, MCDE_DSI_FORMATTER_0,
>  			    fifo_wtrmrk);
>  
> +	/*
> +	 * This brings up the DSI bridge which is tightly connected
> +	 * to the MCDE DSI formatter.
> +	 *
> +	 * FIXME: if we want to use another formatter, such as DPI,
> +	 * we need to be more elaborate here and select the appropriate
> +	 * bridge.
> +	 */
> +	mcde_dsi_enable(mcde->bridge);
> +
>  	/* Configure the DSI formatter 0 for the DSI panel output */
>  	mcde_configure_dsi_formatter(mcde, MCDE_DSI_FORMATTER_0,
>  				     formatter_frame, pkt_size);
> @@ -1025,16 +1035,20 @@ static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
>  
>  	drm_crtc_vblank_on(crtc);
>  
> -	if (mcde_flow_is_video(mcde)) {
> -		/*
> -		 * Keep FIFO permanently enabled in video mode,
> -		 * otherwise MCDE will stop feeding data to the panel.
> -		 */
> +	/*
> +	 * If we're using oneshot mode we don't start the flow
> +	 * until each time the display is given an update, and
> +	 * then we disable it immediately after. For all other
> +	 * modes (command or video) we start the FIFO flow
> +	 * right here. This is necessary for the hardware to
> +	 * behave right.
> +	 */
> +	if (mcde->flow_mode != MCDE_COMMAND_ONESHOT_FLOW) {
>  		mcde_enable_fifo(mcde, MCDE_FIFO_A);
>  		dev_dbg(mcde->dev, "started MCDE video FIFO flow\n");
>  	}
>  
> -	/* Enable automatic clock gating */
> +	/* Enable MCDE with automatic clock gating */
>  	val = readl(mcde->regs + MCDE_CR);
>  	val |= MCDE_CR_MCDEEN | MCDE_CR_AUTOCLKG_EN;
>  	writel(val, mcde->regs + MCDE_CR);
> @@ -1055,6 +1069,9 @@ static void mcde_display_disable(struct drm_simple_display_pipe *pipe)
>  	/* Disable FIFO A flow */
>  	mcde_disable_fifo(mcde, MCDE_FIFO_A, true);
>  
> +	/* This disables the DSI bridge */
> +	mcde_dsi_disable(mcde->bridge);
> +
>  	event = crtc->state->event;
>  	if (event) {
>  		crtc->state->event = NULL;
> @@ -1164,8 +1181,11 @@ static void mcde_display_update(struct drm_simple_display_pipe *pipe,
>  	if (fb) {
>  		mcde_set_extsrc(mcde, drm_fb_cma_get_gem_addr(fb, pstate, 0));
>  		dev_info_once(mcde->dev, "first update of display contents\n");
> -		/* The flow is already active in video mode */
> -		if (!mcde_flow_is_video(mcde) && mcde->flow_active == 0)
> +		/*
> +		 * Usually the flow is already active, unless we are in
> +		 * oneshot mode, then we need to kick the flow right here.
> +		 */
> +		if (mcde->flow_active == 0)
>  			mcde_start_flow(mcde);
>  	} else {
>  		/*
> diff --git a/drivers/gpu/drm/mcde/mcde_drm.h b/drivers/gpu/drm/mcde/mcde_drm.h
> index 9f197f4e9ced..8253e2f9993e 100644
> --- a/drivers/gpu/drm/mcde/mcde_drm.h
> +++ b/drivers/gpu/drm/mcde/mcde_drm.h
> @@ -97,6 +97,8 @@ static inline bool mcde_flow_is_video(struct mcde *mcde)
>  
>  bool mcde_dsi_irq(struct mipi_dsi_device *mdsi);
>  void mcde_dsi_te_request(struct mipi_dsi_device *mdsi);
> +void mcde_dsi_enable(struct drm_bridge *bridge);
> +void mcde_dsi_disable(struct drm_bridge *bridge);
>  extern struct platform_driver mcde_dsi_driver;
>  
>  void mcde_display_irq(struct mcde *mcde);
> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index 0d7ebb59b727..a46a45c5cd52 100644
> --- a/drivers/gpu/drm/mcde/mcde_dsi.c
> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
> @@ -844,23 +844,11 @@ static void mcde_dsi_start(struct mcde_dsi *d)
>  	dev_info(d->dev, "DSI link enabled\n");
>  }
>  
> -
> -static void mcde_dsi_bridge_enable(struct drm_bridge *bridge)
> -{
> -	struct mcde_dsi *d = bridge_to_mcde_dsi(bridge);
> -	u32 val;
> -
> -	if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> -		/* Enable video mode */
> -		val = readl(d->regs + DSI_MCTL_MAIN_DATA_CTL);
> -		val |= DSI_MCTL_MAIN_DATA_CTL_VID_EN;
> -		writel(val, d->regs + DSI_MCTL_MAIN_DATA_CTL);
> -	}
> -
> -	dev_info(d->dev, "enable DSI master\n");
> -};
> -
> -static void mcde_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +/*
> + * Notice that this is called from inside the display controller
> + * and not from the bridge callbacks.
> + */
> +void mcde_dsi_enable(struct drm_bridge *bridge)
>  {
>  	struct mcde_dsi *d = bridge_to_mcde_dsi(bridge);
>  	unsigned long hs_freq, lp_freq;
> @@ -938,6 +926,11 @@ static void mcde_dsi_bridge_pre_enable(struct drm_bridge *bridge)
>  		val |= DSI_VID_MODE_STS_CTL_ERR_MISSING_VSYNC;
>  		val |= DSI_VID_MODE_STS_CTL_ERR_MISSING_DATA;
>  		writel(val, d->regs + DSI_VID_MODE_STS_CTL);
> +
> +		/* Enable video mode */
> +		val = readl(d->regs + DSI_MCTL_MAIN_DATA_CTL);
> +		val |= DSI_MCTL_MAIN_DATA_CTL_VID_EN;
> +		writel(val, d->regs + DSI_MCTL_MAIN_DATA_CTL);
>  	} else {
>  		/* Command mode, clear IF1 ID */
>  		val = readl(d->regs + DSI_CMD_MODE_CTL);
> @@ -950,6 +943,8 @@ static void mcde_dsi_bridge_pre_enable(struct drm_bridge *bridge)
>  		val &= ~DSI_CMD_MODE_CTL_IF1_ID_MASK;
>  		writel(val, d->regs + DSI_CMD_MODE_CTL);
>  	}
> +
> +	dev_info(d->dev, "enabled MCDE DSI master\n");
>  }
>  
>  static void mcde_dsi_bridge_mode_set(struct drm_bridge *bridge,
> @@ -1012,7 +1007,11 @@ static void mcde_dsi_wait_for_video_mode_stop(struct mcde_dsi *d)
>  	}
>  }
>  
> -static void mcde_dsi_bridge_disable(struct drm_bridge *bridge)
> +/*
> + * Notice that this is called from inside the display controller
> + * and not from the bridge callbacks.
> + */
> +void mcde_dsi_disable(struct drm_bridge *bridge)
>  {
>  	struct mcde_dsi *d = bridge_to_mcde_dsi(bridge);
>  	u32 val;
> @@ -1027,11 +1026,6 @@ static void mcde_dsi_bridge_disable(struct drm_bridge *bridge)
>  		/* Stop command mode */
>  		mcde_dsi_wait_for_command_mode_stop(d);
>  	}
> -}
> -
> -static void mcde_dsi_bridge_post_disable(struct drm_bridge *bridge)
> -{
> -	struct mcde_dsi *d = bridge_to_mcde_dsi(bridge);
>  
>  	/*
>  	 * Stop clocks and terminate any DSI traffic here so the panel can
> @@ -1070,10 +1064,6 @@ static int mcde_dsi_bridge_attach(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs mcde_dsi_bridge_funcs = {
>  	.attach = mcde_dsi_bridge_attach,
>  	.mode_set = mcde_dsi_bridge_mode_set,
> -	.disable = mcde_dsi_bridge_disable,
> -	.enable = mcde_dsi_bridge_enable,
> -	.pre_enable = mcde_dsi_bridge_pre_enable,
> -	.post_disable = mcde_dsi_bridge_post_disable,
>  };
>  
>  static int mcde_dsi_bind(struct device *dev, struct device *master,
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux