Re: [PATCH v2 7/8] drm/bridge/synopsys: dsi: add dual-dsi support

[Philippe CORNU <philippe.cornu@st.com>]

Hi Heiko,
Thank you for your patch,

On 06/18/2018 12:28 PM, Heiko Stuebner wrote:
> From: Nickey Yang <nickey.yang@rock-chips.com>
> 
> Allow to also drive a slave dw-mipi-dsi controller in a dual-dsi
> setup. This will require additional implementation-specific
> code to look up the slave instance and do specific setup.
> Also will probably need code in the specific crtcs as dual-dsi
> does not equal two separate dsi outputs.
> 
> To activate, the implementation-specific code should set the slave
> using dw_mipi_dsi_set_slave() before calling __dw_mipi_dsi_bind().
> 
> v2:
> - expect real interface number of lanes
> - keep links to both master and slave
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 93 +++++++++++++++++--
>   include/drm/bridge/dw_mipi_dsi.h              |  1 +
>   2 files changed, 86 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index bd503f000ed5..6a345d1dde25 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -230,9 +230,20 @@ struct dw_mipi_dsi {
>   	u32 format;
>   	unsigned long mode_flags;
>   
> +	struct dw_mipi_dsi *master; /* dual-dsi master ptr */
> +	struct dw_mipi_dsi *slave; /* dual-dsi slave ptr */
> +
>   	const struct dw_mipi_dsi_plat_data *plat_data;
>   };
>   
> +/*
> + * Check if either a link to a master or slave is present
> + */
> +static inline bool dw_mipi_is_dual_mode(struct dw_mipi_dsi *dsi)
> +{
> +	return dsi->slave || dsi->master;
> +}
> +
>   /*
>    * The controller should generate 2 frames before
>    * preparing the peripheral.
> @@ -273,18 +284,26 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
>   	struct drm_bridge *bridge;
>   	struct drm_panel *panel;
>   	int ret;
> +	int lanes = device->lanes;

I do not see a big interest here in adding the new var "lanes", I 
suggest to remove it or wait for more comments (not a strong opposition 
on my side:) just a preference).

>   
> -	if (device->lanes > dsi->plat_data->max_data_lanes) {
> +	if (lanes > dsi->plat_data->max_data_lanes) {
>   		dev_err(dsi->dev, "the number of data lanes(%u) is too many\n",
>   			device->lanes);

please use "lanes" has you have created it ;-) or keep it as it is if 
you decide to remove the var lanes :)


>   		return -EINVAL;
>   	}
>   
> -	dsi->lanes = device->lanes;
> +	dsi->lanes = lanes;
>   	dsi->channel = device->channel;
>   	dsi->format = device->format;
>   	dsi->mode_flags = device->mode_flags;
>   
> +	if (dsi->slave) {
> +		dsi->slave->lanes = dsi->lanes;
> +		dsi->slave->channel = dsi->channel;
> +		dsi->slave->format = dsi->format;
> +		dsi->slave->mode_flags = dsi->mode_flags;
> +	}
> +
>   	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0,
>   					  &panel, &bridge);
>   	if (ret)
> @@ -441,10 +460,17 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>   	}
>   
>   	dw_mipi_message_config(dsi, msg);
> +	if (dsi->slave)
> +		dw_mipi_message_config(dsi->slave, msg);
>   
>   	ret = dw_mipi_dsi_write(dsi, &packet);
>   	if (ret)
>   		return ret;
> +	if (dsi->slave) {
> +		ret = dw_mipi_dsi_write(dsi->slave, &packet);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	if (msg->rx_buf && msg->rx_len) {
>   		ret = dw_mipi_dsi_read(dsi, msg);
> @@ -583,7 +609,15 @@ static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
>   	 * DSI_VNPCR.NPSIZE... especially because this driver supports
>   	 * non-burst video modes, see dw_mipi_dsi_video_mode_config()...
>   	 */
> -	dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay));
> +
> +	int pkt_size;

I do not see any interest in adding this pkt_size, you can directly do 
the dsi_write (below) and remove minimum 4 lines in your patch then ... 
just my opinion : )

> +
> +	if (dw_mipi_is_dual_mode(dsi))
> +		pkt_size = VID_PKT_SIZE(mode->hdisplay / 2);
> +	else
> +		pkt_size = VID_PKT_SIZE(mode->hdisplay);
> +
> +	dsi_write(dsi, DSI_VID_PKT_SIZE, pkt_size);
>   }
>   
>   static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
> @@ -756,23 +790,39 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge)
>   	dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
>   
>   	dw_mipi_dsi_disable(dsi);
Maybe you should move dw_mipi_dsi_disable(dsi) call after the if 
(dsi->slave)...

> +	if (dsi->slave) {
> +		dw_mipi_dsi_disable(dsi->slave);
> +		clk_disable_unprepare(dsi->slave->pclk);
> +		pm_runtime_put(dsi->slave->dev);
> +	}
> +
>   	clk_disable_unprepare(dsi->pclk);
>   	pm_runtime_put(dsi->dev);
>   }
>   
> -static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
> -					struct drm_display_mode *mode,
> -					struct drm_display_mode *adjusted_mode)
> +static unsigned int dw_mipi_dsi_get_lanes(struct dw_mipi_dsi *dsi)
> +{
> +	if (dsi->master)
> +		return dsi->master->lanes + dsi->lanes;
> +
> +	if (dsi->slave)
> +		return dsi->lanes + dsi->slave->lanes;
maybe a short explanation for master, slave and single-dsi 3 cases could 
be nice here

> +
> +	return dsi->lanes;
> +}
> +
> +static void dw_mipi_dsi_mode_set(struct dw_mipi_dsi *dsi,
> +				struct drm_display_mode *adjusted_mode)
>   {
> -	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>   	const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops;
>   	void *priv_data = dsi->plat_data->priv_data;
>   	int ret;
> +	u32 lanes = dw_mipi_dsi_get_lanes(dsi);
>   
>   	clk_prepare_enable(dsi->pclk);
>   
>   	ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags,
> -				     dsi->lanes, dsi->format, &dsi->lane_mbps);
> +				     lanes, dsi->format, &dsi->lane_mbps);
>   	if (ret)
>   		DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n");
>   
> @@ -804,12 +854,25 @@ static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
>   	dw_mipi_dsi_set_mode(dsi, 0);
>   }
>   
> +static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
> +					struct drm_display_mode *mode,
> +					struct drm_display_mode *adjusted_mode)
> +{
> +	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	dw_mipi_dsi_mode_set(dsi, adjusted_mode);
> +	if (dsi->slave)
> +		dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode);
> +}
> +
>   static void dw_mipi_dsi_bridge_enable(struct drm_bridge *bridge)
>   {
>   	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>   
>   	/* Switch to video mode for panel-bridge enable & panel enable */
>   	dw_mipi_dsi_set_mode(dsi, MIPI_DSI_MODE_VIDEO);
> +	if (dsi->slave)
> +		dw_mipi_dsi_set_mode(dsi->slave, MIPI_DSI_MODE_VIDEO);
>   }
>   
>   static enum drm_mode_status
> @@ -949,6 +1012,20 @@ static void __dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi)
>   	pm_runtime_disable(dsi->dev);
>   }
>   
> +void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave)
> +{
> +	/* introduce controllers to each other */
> +	dsi->slave = slave;
> +	dsi->slave->master = dsi;
> +
> +	/* migrate settings for already attached displays */
> +	dsi->slave->lanes = dsi->lanes;
> +	dsi->slave->channel = dsi->channel;
> +	dsi->slave->format = dsi->format;
> +	dsi->slave->mode_flags = dsi->mode_flags;

I though it has been done earlier in dw_mipi_dsi_host_attach() function?

> +}
> +EXPORT_SYMBOL_GPL(dw_mipi_dsi_set_slave);
> +
>   /*
>    * Probe/remove API, used from platforms based on the DRM bridge API.
>    */
> diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
> index 6d7f8eb5d9f2..5fd997cdf281 100644
> --- a/include/drm/bridge/dw_mipi_dsi.h
> +++ b/include/drm/bridge/dw_mipi_dsi.h
> @@ -37,5 +37,6 @@ struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev,
>   void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
>   int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder);
>   void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
> +void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave);


Tested-by: Philippe Cornu <philippe.cornu@st.com> (only on stm32 with 
single dsi so dual-dsi has not been tested)

I wait for more comments & (minor) updates before giving my reviewed-by 
but you are close to have it : )

many thanks,
Philippe :-)

>   
>   #endif /* __DW_MIPI_DSI__ */
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel