| Message ID | 20180618102806.15650-8-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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__ */ >
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__ */
Am Dienstag, 3. Juli 2018, 19:07:02 CEST schrieb Andrzej Hajda: > 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. I cannot really claim to know the correct way to go forward, as these drm-internals are still very much unknown land for me, but current thought points I had on this: - strawman argument ;-) : this is the same way tegra handles dual-dsi in its ganged mode ... also with regards to panel handling With Thierry helming both panels and the tegra dual-dsi change - while the panel may expose two DSI data interfaces, there is still only one power-sequence and also only one init-command sequence. So I don't think you can really handle a dual-dsi panels as two fully separate panels, creating instead the need for both panel-instances to work together - Right now we have two data-points on dual-something voodoo (tegra+rockchip) So for me it sounds hard to design something generic that survives the first non-dsi use-case. The devicetree binding (see cover-letter) should be pretty stable, as a panel like this will need to be bound to one of the two dsi controllers so nothing hinders code changes later on if the need for a more generic solution comes up. Heiko
On 09.07.2018 15:45, Heiko Stuebner wrote: > Am Dienstag, 3. Juli 2018, 19:07:02 CEST schrieb Andrzej Hajda: >> 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. > I cannot really claim to know the correct way to go forward, as these > drm-internals are still very much unknown land for me, but current > thought points I had on this: > > - strawman argument ;-) : this is the same way tegra handles dual-dsi > in its ganged mode ... also with regards to panel handling > With Thierry helming both panels and the tegra dual-dsi change Yes, and I remember our disagreement on the subject :) > > - while the panel may expose two DSI data interfaces, there is still > only one power-sequence and also only one init-command sequence. As in case of almost all devices exposing two, or more interfaces. > So I don't think you can really handle a dual-dsi panels as two fully > separate panels, creating instead the need for both panel-instances > to work together The question is if they can work separately? Ie is it possible to use only one interface, and have picture only on left/top side, even/odd lines? Or to have connected interfaces to two independent display controllers (of course via some dsi bridges)? Btw I have not seen bindings/driver for the panel, what is the panel name? The answer for above questions would be helpful. If the only way of work is with both dsi interfaces, exposing one drm_panel maybe could be OK. The point is if the panel has two interfaces it should be his responsibility for handling it, not the poor DSI host, which is just for converting native video signal (RGB I guess) to DSI. The other responsible for the mess is CRTC which actually should negotiate with panel how the data is split between both channels (left/right, top/bottom, odd/even, ....). DSI-host here serves only as data transport, in fact it does not need to know if it works in dual mode or not (from HW point of view) - dual mode is just something which only CRTC and panel should be really aware. See what will happen to your solution if in next version of hardware vendor: 1. replace dual-panel with two panels glued together. 2. replace DSI bridges with different ones. 3. replace only one DSI briges - ie two different bridges working in dual mode. > > - Right now we have two data-points on dual-something voodoo > (tegra+rockchip) > So for me it sounds hard to design something generic that survives > the first non-dsi use-case. As I said I do not insist on the solution, but I want to show you it can be dead-end. On the other side maybe dual dsi solutions will die eventually soon, so there is no point in polishing it :) Regards Andrzej > > The devicetree binding (see cover-letter) should be pretty stable, as > a panel like this will need to be bound to one of the two dsi controllers > so nothing hinders code changes later on if the need for a more generic > solution comes up. > > > Heiko > > > > >
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__ */