On 18.06.2018 12:28, Heiko Stuebner wrote:
> From: Nickey Yang <nickey.yang@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> ---
> 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__ */
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
