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

On 18.06.2018 12:28, 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

I did not see the whole driver/pipeline, but it seems the point of this patch is to perform the same work on the slave as on the master in case of dual mode. I think DSI should not be a place for it, DSI masters usually are stupid devices from display stack PoV, they just convert video streams, in dual mode also. In this case the panel and/or crtc adds complications so they should be responsible for handling it. Panel should: - register its both mipi interfaces with proper mode_flags (maybe some dual-mode indication flags should be added if necessary), - register drm_panel for both interfaces (it requires change in drm_panel api), and provide video mode timings. - in case it needs perform transfers perform it to master/slave/both interfaces according to its needs,

I am not sure about DRM pipeline, it should model, maybe it could be done this way: CRTC -->ENCODER0(dsi master) --> CONNECTOR0 (panel interface 0)       |---> ENCODER1(dsi slave) --> CONNECTOR1 (panel interface 1)

But I am not sure if it is not reserved only for mirroring.

For me more tempting solution is to create meta-encoder-connector let's call it dual-encoder (maybe it could be even generic), which is visible to userspace as single pipeline and encapsulates both dsi bridges/panel inputs. So its every callback will be translated usually to sequence of callbacks to 1st and 2nd dsi, or in case of get_modes it should return mode which represent sum of modes in both panels. Maybe it looks more complicated, but it can be more universal - you can use it with different bridges/panels even two single-panels if necessary.

Of course I do not see the whole picture, or I can be just wrong, or just freaking purist :). If there are arguments against my vision please provide them. I am also not strongly against your solution, I just want to show alternatives, which could be better/more generic.

Regards Andrzej

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;

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

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

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

+} +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);

#endif /* __DW_MIPI_DSI__ */