[RFC PATCH 00/10] drm/mipi-dsi: another attempt at sorting out DSI link powerup

[RFC PATCH 00/10] drm/mipi-dsi: another attempt at sorting out DSI link powerup

[Dmitry Baryshkov]

It is well known that DSI dosn't fully fit into the DRM enable/disable
model thanks to the intermediate LP-11 state: (roughly) the link is already
up, but the video stream is not yet enabled.

Previously we have handled this by forcing DSI link powerup in the
mode_set callback. This worked, but it was not an ideal solution. For
example, it didn't play well with the atomicity part.

Then Dave attempted to solve the issue by adding pre_enable_prev_first.
It also seemed to work fine, until we stumbled at the issue of the
driver being unable to negotiate whether the bridge/panel didn't enable
pre_enable_prev_first because it is not updated yet or because it
doesn't need the callbacks to be inverted.

This series is yet another attempt at solving the DSI link powerup
story. It adds two flags for the DSI evice. One of them should trigger
implicit link powerup at the atomic_pre_enable / atomic_post_disable
callbacks. Another one requests excplicit DSI link power control.

Dmitry Baryshkov (10):
  Revert "drm/bridge: tc358762: Split register programming from
    pre-enable to enable"
  drm/mipi-dsi: document DSI hosts limitations
  drm/mipi-dsi: add API for manual control over the DSI link power state
  drm/msm/dsi: use dsi_mgr_bridge_power_off in
    dsi_mgr_bridge_post_disable
  drm/msm/dsi: implement manual power control
  drm/bridge: tc358762: add support for manual DSI power control
  drm/bridge: ps8640: require manual DSI power control
  drm/bridge: lt9611: mark for automatic DSI power control
  drm/bridge: lt9611uxc: implement automatic DSI power control
  drm/msm/dsi: drop (again) the ps8640 workaround

 drivers/gpu/drm/bridge/lontium-lt9611.c    |  2 +-
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c |  2 +-
 drivers/gpu/drm/bridge/parade-ps8640.c     | 14 ++++-
 drivers/gpu/drm/bridge/tc358762.c          | 24 +++++---
 drivers/gpu/drm/drm_mipi_dsi.c             | 31 ++++++++++
 drivers/gpu/drm/msm/dsi/dsi.h              |  4 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c         | 44 ++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi_manager.c      | 70 +++++++++++++---------
 include/drm/drm_mipi_dsi.h                 | 33 ++++++++--
 9 files changed, 180 insertions(+), 44 deletions(-)

-- 
2.42.0

[RFC PATCH 01/10] Revert "drm/bridge: tc358762: Split register programming from pre-enable to enable"

[Dmitry Baryshkov]

It has been pointed out (e.g. in [1]) that enable is not the best place
for sending the DSI commands. There are hosts (sunxi) that can not
support sending DSI commands once video stream has enabled.

Also most panel drivers send DSI commands in the prepare() callback
(which maps to drm_bridg_funcs::pre_enable()). Thus the DSIM host should
be fixed to be able to send DSI commands from pre_enable() state.

[1] https://lore.kernel.org/dri-devel/CAPY8ntBrhYAmsraDqJGuTrSL6VjGXBAMVoN7xweV7E4qZv+v3Q@mail.gmail.com/
[2] https://github.com/torvalds/lincux/blob/master/include/drm/drm_mipi_dsi.h#L84-L87

Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/tc358762.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 46198af9eebb..7b9f05f95fd1 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -185,17 +185,11 @@ static void tc358762_pre_enable(struct drm_bridge *bridge, struct drm_bridge_sta
 		usleep_range(5000, 10000);
 	}
 
-	ctx->pre_enabled = true;
-}
-
-static void tc358762_enable(struct drm_bridge *bridge, struct drm_bridge_state *state)
-{
-	struct tc358762 *ctx = bridge_to_tc358762(bridge);
-	int ret;
-
 	ret = tc358762_init(ctx);
 	if (ret < 0)
 		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
+
+	ctx->pre_enabled = true;
 }
 
 static int tc358762_attach(struct drm_bridge *bridge,
@@ -219,7 +213,6 @@ static void tc358762_bridge_mode_set(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs tc358762_bridge_funcs = {
 	.atomic_post_disable = tc358762_post_disable,
 	.atomic_pre_enable = tc358762_pre_enable,
-	.atomic_enable = tc358762_enable,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
-- 
2.42.0

[RFC PATCH 02/10] drm/mipi-dsi: document DSI hosts limitations

[Dmitry Baryshkov]

Document the known limititations of the DSI hosts vs commands transfers
in LP mode. For the details see sun6i_dsi_encoder_enable().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 include/drm/drm_mipi_dsi.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c9df0407980c..167742e579e3 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -85,6 +85,10 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
  * host is in. Drivers that need the underlying device to be powered to
  * perform these operations will first need to make sure it's been
  * properly enabled.
+ *
+ * Note: some hosts (sunxi) can not send LP commands between HS video
+ * packets. Thus all DSI transfers sent in LP mode should be limited to the
+ * drm_bridge_funcs::pre_enable() and drm_panel_funcs::prepare() callbacks.
  */
 struct mipi_dsi_host_ops {
 	int (*attach)(struct mipi_dsi_host *host,
-- 
2.42.0

[RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

The MIPI DSI links do not fully fall into the DRM callbacks model. The
drm_bridge_funcs abstraction. Instead of having just two states (off and
on) the DSI hosts have separate LP-11 state. In this state the host is
on, but the video stream is not yet enabled.

Introduce API that allows DSI bridges / panels to control the DSI host
power up state.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 14201f73aab1..c467162cb7d8 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
 
+bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
+{
+	const struct mipi_dsi_host_ops *ops = host->ops;
+
+	return ops && ops->power_up;
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
+
+int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
+{
+	const struct mipi_dsi_host_ops *ops = host->ops;
+
+	if (!mipi_dsi_host_power_control_available(host))
+		return -EOPNOTSUPP;
+
+	return ops->power_up ? ops->power_up(host) : 0;
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
+
+void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
+{
+	const struct mipi_dsi_host_ops *ops = host->ops;
+
+	if (!mipi_dsi_host_power_control_available(host))
+		return;
+
+	if (ops->power_down)
+		ops->power_down(host);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
+
 static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi,
 					struct mipi_dsi_msg *msg)
 {
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 167742e579e3..e503c3e4d057 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -68,6 +68,8 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
  * @attach: attach DSI device to DSI host
  * @detach: detach DSI device from DSI host
  * @transfer: transmit a DSI packet
+ * @power_up: enable DSI link and bring it to the LP-11 state
+ * @power_down: fully disable DSI link
  *
  * DSI packets transmitted by .transfer() are passed in as mipi_dsi_msg
  * structures. This structure contains information about the type of packet
@@ -81,10 +83,18 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
  * function will seldomly return anything other than the number of bytes
  * contained in the transmit buffer on success.
  *
- * Also note that those callbacks can be called no matter the state the
- * host is in. Drivers that need the underlying device to be powered to
- * perform these operations will first need to make sure it's been
- * properly enabled.
+ * Note: currently there are two modes of DSI power control. Legacy drivers
+ * will call those callbacks no matter the state the host is in. DSI host
+ * drivers that need the underlying device to be powered to perform these
+ * operations will first need to make sure it's been properly enabled.
+ *
+ * Newer drivers will set the @MIPI_DSI_MANUAL_POWERUP flag to indicate that
+ * they will call @mipi_dsi_power_up() and @mipi_dsi_power_down() to control
+ * the link state of the DSI host or they will set @MIPI_DSI_AUTO_POWERUP to
+ * indicate that the driver is fine with the link being powered up in DSI
+ * host's (atomic_)pre_enable() callback and then being disabled in the
+ * (atomic_)post_disable() callback. The transfer callback must only be called
+ * if the DSI host has been powered up and was not brought down.
  *
  * Note: some hosts (sunxi) can not send LP commands between HS video
  * packets. Thus all DSI transfers sent in LP mode should be limited to the
@@ -97,6 +107,8 @@ struct mipi_dsi_host_ops {
 		      struct mipi_dsi_device *dsi);
 	ssize_t (*transfer)(struct mipi_dsi_host *host,
 			    const struct mipi_dsi_msg *msg);
+	int (*power_up)(struct mipi_dsi_host *host);
+	void (*power_down)(struct mipi_dsi_host *host);
 };
 
 /**
@@ -143,6 +155,10 @@ struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
 #define MIPI_DSI_MODE_LPM		BIT(11)
 /* transmit data ending at the same time for all lanes within one hsync */
 #define MIPI_DSI_HS_PKT_END_ALIGNED	BIT(12)
+/* DSI peripheral driver manually controls DSI link powerup */
+#define MIPI_DSI_MANUAL_POWERUP		BIT(13)
+/* DSI peripheral driver is fine with automatic DSI link power control */
+#define MIPI_DSI_AUTO_POWERUP		BIT(14)
 
 enum mipi_dsi_pixel_format {
 	MIPI_DSI_FMT_RGB888,
@@ -235,6 +251,11 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
 struct mipi_dsi_device *
 devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *host,
 				   const struct mipi_dsi_device_info *info);
+
+bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host);
+int mipi_dsi_host_power_up(struct mipi_dsi_host *host);
+void mipi_dsi_host_power_down(struct mipi_dsi_host *host);
+
 struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
 int mipi_dsi_attach(struct mipi_dsi_device *dsi);
 int mipi_dsi_detach(struct mipi_dsi_device *dsi);
-- 
2.42.0

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Maxime Ripard]

On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> The MIPI DSI links do not fully fall into the DRM callbacks model.

Explaining why would help

> The drm_bridge_funcs abstraction.

Is there a typo or missing words?

> Instead of having just two states (off and on) the DSI hosts have
> separate LP-11 state. In this state the host is on, but the video
> stream is not yet enabled.
> 
> Introduce API that allows DSI bridges / panels to control the DSI host
> power up state.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
>  include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
>  2 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 14201f73aab1..c467162cb7d8 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
>  
> +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
> +{
> +	const struct mipi_dsi_host_ops *ops = host->ops;
> +
> +	return ops && ops->power_up;
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> +
> +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> +{
> +	const struct mipi_dsi_host_ops *ops = host->ops;
> +
> +	if (!mipi_dsi_host_power_control_available(host))
> +		return -EOPNOTSUPP;
> +
> +	return ops->power_up ? ops->power_up(host) : 0;
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> +
> +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> +{
> +	const struct mipi_dsi_host_ops *ops = host->ops;
> +
> +	if (!mipi_dsi_host_power_control_available(host))
> +		return;
> +
> +	if (ops->power_down)
> +		ops->power_down(host);
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> +

If this API is supposed to be used by DSI devices, it should probably
take a mipi_dsi_device pointer as a parameter?

We should probably make sure it hasn't been powered on already too?

Maxime

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > The MIPI DSI links do not fully fall into the DRM callbacks model.
>
> Explaining why would help

A kind of explanation comes afterwards, but probably I should change
the order of the phrases and expand it:

The atomic_pre_enable / atomic_enable and correspondingly
atomic_disable / atomic_post_disable expect that the bridge links
follow a simple paradigm: either it is off, or it is on and streaming
video. Thus, it is fine to just enable the link at the enable time,
doing some preparations during the pre_enable.

But then it causes several issues with DSI. First, some of the DSI
bridges and most of the DSI panels would like to send commands over
the DSI link to setup the device. Next, some of the DSI hosts have
limitations on sending the commands. The proverbial sunxi DSI host can
not send DSI commands after the video stream has started. Thus most of
the panels have opted to send all DSI commands from pre_enable (or
prepare) callback (before the video stream has started).

However this leaves no good place for the DSI host to power up the DSI
link. By default the host's pre_enable callback is called after the
DSI bridge's pre_enable. For quite some time we were powering up the
DSI link from mode_set. This doesn't look fully correct. And also we
got into the issue with ps8640 bridge, which requires for the DSI link
to be quiet / unpowered at the bridge's reset time.

Dave has come with the idea of pre_enable_prev_first /
prepare_prev_first flags, which attempt to solve the issue by
reversing the order of pre_enable callbacks. This mostly solves the
issue. However during this cycle it became obvious that this approach
is not ideal too. There is no way for the DSI host to know whether the
DSI panel / bridge has been updated to use this flag or not, see the
discussion at [1].

Thus comes this proposal. It allows for the panels to explicitly bring
the link up and down at the correct time, it supports automatic use
case, where no special handling is required. And last, but not least,
it allows the DSI host to note that the bridge / panel were not
updated to follow new protocol and thus the link should be powered on
at the mode_set time. This leaves us with the possibility of dropping
support for this workaround once all in-kernel drivers are updated.

> > The drm_bridge_funcs abstraction.
>
> Is there a typo or missing words?

missing comma in front of The

>
> > Instead of having just two states (off and on) the DSI hosts have
> > separate LP-11 state. In this state the host is on, but the video
> > stream is not yet enabled.
> >
> > Introduce API that allows DSI bridges / panels to control the DSI host
> > power up state.

[1] https://lore.kernel.org/dri-devel/6c0dd9fd-5d8e-537c-804f-7a03d5899a07@linaro.org/

> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
> >  include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
> >  2 files changed, 56 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index 14201f73aab1..c467162cb7d8 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
> >
> > +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
> > +{
> > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > +
> > +     return ops && ops->power_up;
> > +}
> > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> > +
> > +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> > +{
> > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > +
> > +     if (!mipi_dsi_host_power_control_available(host))
> > +             return -EOPNOTSUPP;
> > +
> > +     return ops->power_up ? ops->power_up(host) : 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> > +
> > +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> > +{
> > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > +
> > +     if (!mipi_dsi_host_power_control_available(host))
> > +             return;
> > +
> > +     if (ops->power_down)
> > +             ops->power_down(host);
> > +}
> > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> > +
>
> If this API is supposed to be used by DSI devices, it should probably
> take a mipi_dsi_device pointer as a parameter?

Ack.

>
> We should probably make sure it hasn't been powered on already too?

Ack, I can add an atomic count there and a WARN_ON. However I don't
think that it is a real problem.

>
> Maxime



--
With best wishes

Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Alexander Stein]

Hi,

Am Donnerstag, 19. Oktober 2023, 13:19:51 CEST schrieb Dmitry Baryshkov:
> On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> > 
> > Explaining why would help
> 
> A kind of explanation comes afterwards, but probably I should change
> the order of the phrases and expand it:
> 
> The atomic_pre_enable / atomic_enable and correspondingly
> atomic_disable / atomic_post_disable expect that the bridge links
> follow a simple paradigm: either it is off, or it is on and streaming
> video. Thus, it is fine to just enable the link at the enable time,
> doing some preparations during the pre_enable.
> 
> But then it causes several issues with DSI. First, some of the DSI
> bridges and most of the DSI panels would like to send commands over
> the DSI link to setup the device. Next, some of the DSI hosts have
> limitations on sending the commands. The proverbial sunxi DSI host can
> not send DSI commands after the video stream has started. Thus most of
> the panels have opted to send all DSI commands from pre_enable (or
> prepare) callback (before the video stream has started).
> 
> However this leaves no good place for the DSI host to power up the DSI
> link. By default the host's pre_enable callback is called after the
> DSI bridge's pre_enable. For quite some time we were powering up the
> DSI link from mode_set. This doesn't look fully correct. And also we
> got into the issue with ps8640 bridge, which requires for the DSI link
> to be quiet / unpowered at the bridge's reset time.

There are also bridges (e.g. tc358767) which require DSI-LP11 upon bridge 
reset. And additionally this DSI-(e)DP bridge requires LP11 while accessing 
DP-AUX channel, e.g. reading EDID. So bridges need at least some control over 
DSI line state.

> Dave has come with the idea of pre_enable_prev_first /
> prepare_prev_first flags, which attempt to solve the issue by
> reversing the order of pre_enable callbacks. This mostly solves the
> issue. However during this cycle it became obvious that this approach
> is not ideal too. There is no way for the DSI host to know whether the
> DSI panel / bridge has been updated to use this flag or not, see the
> discussion at [1].
> 
> Thus comes this proposal. It allows for the panels to explicitly bring
> the link up and down at the correct time, it supports automatic use
> case, where no special handling is required. And last, but not least,
> it allows the DSI host to note that the bridge / panel were not
> updated to follow new protocol and thus the link should be powered on
> at the mode_set time. This leaves us with the possibility of dropping
> support for this workaround once all in-kernel drivers are updated.

I want to use this series to support tc358767 properly, because 
pre_enable_prev_first is not enough, see AUX channel above. I hope I'll find 
any time soon.

Best regards,
Alexander

> 
> > > The drm_bridge_funcs abstraction.
> > 
> > Is there a typo or missing words?
> 
> missing comma in front of The
> 
> > > Instead of having just two states (off and on) the DSI hosts have
> > > separate LP-11 state. In this state the host is on, but the video
> > > stream is not yet enabled.
> > > 
> > > Introduce API that allows DSI bridges / panels to control the DSI host
> > > power up state.
> 
> [1]
> https://lore.kernel.org/dri-devel/6c0dd9fd-5d8e-537c-804f-7a03d5899a07@lina
> ro.org/
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
> > >  include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
> > >  2 files changed, 56 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
> > > b/drivers/gpu/drm/drm_mipi_dsi.c index 14201f73aab1..c467162cb7d8
> > > 100644
> > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
> > > 
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
> > > 
> > > +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
> > > +{
> > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > +
> > > +     return ops && ops->power_up;
> > > +}
> > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> > > +
> > > +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> > > +{
> > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > +
> > > +     if (!mipi_dsi_host_power_control_available(host))
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     return ops->power_up ? ops->power_up(host) : 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> > > +
> > > +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> > > +{
> > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > +
> > > +     if (!mipi_dsi_host_power_control_available(host))
> > > +             return;
> > > +
> > > +     if (ops->power_down)
> > > +             ops->power_down(host);
> > > +}
> > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> > > +
> > 
> > If this API is supposed to be used by DSI devices, it should probably
> > take a mipi_dsi_device pointer as a parameter?
> 
> Ack.
> 
> > We should probably make sure it hasn't been powered on already too?
> 
> Ack, I can add an atomic count there and a WARN_ON. However I don't
> think that it is a real problem.
> 
> > Maxime
> 
> --
> With best wishes
> 
> Dmitry


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Thu, 19 Oct 2023 at 14:42, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Donnerstag, 19. Oktober 2023, 13:19:51 CEST schrieb Dmitry Baryshkov:
> > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> > >
> > > Explaining why would help
> >
> > A kind of explanation comes afterwards, but probably I should change
> > the order of the phrases and expand it:
> >
> > The atomic_pre_enable / atomic_enable and correspondingly
> > atomic_disable / atomic_post_disable expect that the bridge links
> > follow a simple paradigm: either it is off, or it is on and streaming
> > video. Thus, it is fine to just enable the link at the enable time,
> > doing some preparations during the pre_enable.
> >
> > But then it causes several issues with DSI. First, some of the DSI
> > bridges and most of the DSI panels would like to send commands over
> > the DSI link to setup the device. Next, some of the DSI hosts have
> > limitations on sending the commands. The proverbial sunxi DSI host can
> > not send DSI commands after the video stream has started. Thus most of
> > the panels have opted to send all DSI commands from pre_enable (or
> > prepare) callback (before the video stream has started).
> >
> > However this leaves no good place for the DSI host to power up the DSI
> > link. By default the host's pre_enable callback is called after the
> > DSI bridge's pre_enable. For quite some time we were powering up the
> > DSI link from mode_set. This doesn't look fully correct. And also we
> > got into the issue with ps8640 bridge, which requires for the DSI link
> > to be quiet / unpowered at the bridge's reset time.
>
> There are also bridges (e.g. tc358767) which require DSI-LP11 upon bridge
> reset. And additionally this DSI-(e)DP bridge requires LP11 while accessing
> DP-AUX channel, e.g. reading EDID. So bridges need at least some control over
> DSI line state.

For sending commands in LP11 it is typical to toggle the
MIPI_DSI_MODE_LPM flag, for example see panel-=jdi-lt070me05000.c or
some other drives. It might be a good idea to make that more explicit.
All suggestions here would be appreciated.

>
> > Dave has come with the idea of pre_enable_prev_first /
> > prepare_prev_first flags, which attempt to solve the issue by
> > reversing the order of pre_enable callbacks. This mostly solves the
> > issue. However during this cycle it became obvious that this approach
> > is not ideal too. There is no way for the DSI host to know whether the
> > DSI panel / bridge has been updated to use this flag or not, see the
> > discussion at [1].
> >
> > Thus comes this proposal. It allows for the panels to explicitly bring
> > the link up and down at the correct time, it supports automatic use
> > case, where no special handling is required. And last, but not least,
> > it allows the DSI host to note that the bridge / panel were not
> > updated to follow new protocol and thus the link should be powered on
> > at the mode_set time. This leaves us with the possibility of dropping
> > support for this workaround once all in-kernel drivers are updated.
>
> I want to use this series to support tc358767 properly, because
> pre_enable_prev_first is not enough, see AUX channel above. I hope I'll find
> any time soon.
>
> Best regards,
> Alexander
>
> >
> > > > The drm_bridge_funcs abstraction.
> > >
> > > Is there a typo or missing words?
> >
> > missing comma in front of The
> >
> > > > Instead of having just two states (off and on) the DSI hosts have
> > > > separate LP-11 state. In this state the host is on, but the video
> > > > stream is not yet enabled.
> > > >
> > > > Introduce API that allows DSI bridges / panels to control the DSI host
> > > > power up state.
> >
> > [1]
> > https://lore.kernel.org/dri-devel/6c0dd9fd-5d8e-537c-804f-7a03d5899a07@lina
> > ro.org/
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >
> > > >  drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
> > > >  include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
> > > >  2 files changed, 56 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
> > > > b/drivers/gpu/drm/drm_mipi_dsi.c index 14201f73aab1..c467162cb7d8
> > > > 100644
> > > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > > @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
> > > >
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
> > > >
> > > > +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
> > > > +{
> > > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > > +
> > > > +     return ops && ops->power_up;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> > > > +
> > > > +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> > > > +{
> > > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > > +
> > > > +     if (!mipi_dsi_host_power_control_available(host))
> > > > +             return -EOPNOTSUPP;
> > > > +
> > > > +     return ops->power_up ? ops->power_up(host) : 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> > > > +
> > > > +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> > > > +{
> > > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > > +
> > > > +     if (!mipi_dsi_host_power_control_available(host))
> > > > +             return;
> > > > +
> > > > +     if (ops->power_down)
> > > > +             ops->power_down(host);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> > > > +
> > >
> > > If this API is supposed to be used by DSI devices, it should probably
> > > take a mipi_dsi_device pointer as a parameter?
> >
> > Ack.
> >
> > > We should probably make sure it hasn't been powered on already too?
> >
> > Ack, I can add an atomic count there and a WARN_ON. However I don't
> > think that it is a real problem.
> >
> > > Maxime
> >
> > --
> > With best wishes
> >
> > Dmitry
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Alexander Stein]

Hi Dmitry,

Am Sonntag, 22. Oktober 2023, 12:49:41 CEST schrieb Dmitry Baryshkov:
> On Thu, 19 Oct 2023 at 14:42, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi,
> > 
> > Am Donnerstag, 19. Oktober 2023, 13:19:51 CEST schrieb Dmitry Baryshkov:
> > > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> > > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> > > > 
> > > > Explaining why would help
> > > 
> > > A kind of explanation comes afterwards, but probably I should change
> > > the order of the phrases and expand it:
> > > 
> > > The atomic_pre_enable / atomic_enable and correspondingly
> > > atomic_disable / atomic_post_disable expect that the bridge links
> > > follow a simple paradigm: either it is off, or it is on and streaming
> > > video. Thus, it is fine to just enable the link at the enable time,
> > > doing some preparations during the pre_enable.
> > > 
> > > But then it causes several issues with DSI. First, some of the DSI
> > > bridges and most of the DSI panels would like to send commands over
> > > the DSI link to setup the device. Next, some of the DSI hosts have
> > > limitations on sending the commands. The proverbial sunxi DSI host can
> > > not send DSI commands after the video stream has started. Thus most of
> > > the panels have opted to send all DSI commands from pre_enable (or
> > > prepare) callback (before the video stream has started).
> > > 
> > > However this leaves no good place for the DSI host to power up the DSI
> > > link. By default the host's pre_enable callback is called after the
> > > DSI bridge's pre_enable. For quite some time we were powering up the
> > > DSI link from mode_set. This doesn't look fully correct. And also we
> > > got into the issue with ps8640 bridge, which requires for the DSI link
> > > to be quiet / unpowered at the bridge's reset time.
> > 
> > There are also bridges (e.g. tc358767) which require DSI-LP11 upon bridge
> > reset. And additionally this DSI-(e)DP bridge requires LP11 while
> > accessing
> > DP-AUX channel, e.g. reading EDID. So bridges need at least some control
> > over DSI line state.
> 
> For sending commands in LP11 it is typical to toggle the
> MIPI_DSI_MODE_LPM flag, for example see panel-=jdi-lt070me05000.c or
> some other drives. It might be a good idea to make that more explicit.
> All suggestions here would be appreciated.

The biggest difference between that display and the tc358767 bridge is that 
the display uses DSI commands, while the bridge is using i2c transfer to issue 
DP-AUX commands. There is no host_transfer [1] which would enable LP-11.
It seems this DSI-DP bridge requires LP-11/HS on DSI lanes all times. This 
contradicts current Linux behaviour.

Best regards,
Alexander

[1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

> 
> > > Dave has come with the idea of pre_enable_prev_first /
> > > prepare_prev_first flags, which attempt to solve the issue by
> > > reversing the order of pre_enable callbacks. This mostly solves the
> > > issue. However during this cycle it became obvious that this approach
> > > is not ideal too. There is no way for the DSI host to know whether the
> > > DSI panel / bridge has been updated to use this flag or not, see the
> > > discussion at [1].
> > > 
> > > Thus comes this proposal. It allows for the panels to explicitly bring
> > > the link up and down at the correct time, it supports automatic use
> > > case, where no special handling is required. And last, but not least,
> > > it allows the DSI host to note that the bridge / panel were not
> > > updated to follow new protocol and thus the link should be powered on
> > > at the mode_set time. This leaves us with the possibility of dropping
> > > support for this workaround once all in-kernel drivers are updated.
> > 
> > I want to use this series to support tc358767 properly, because
> > pre_enable_prev_first is not enough, see AUX channel above. I hope I'll
> > find any time soon.
> > 
> > Best regards,
> > Alexander
> > 
> > > > > The drm_bridge_funcs abstraction.
> > > > 
> > > > Is there a typo or missing words?
> > > 
> > > missing comma in front of The
> > > 
> > > > > Instead of having just two states (off and on) the DSI hosts have
> > > > > separate LP-11 state. In this state the host is on, but the video
> > > > > stream is not yet enabled.
> > > > > 
> > > > > Introduce API that allows DSI bridges / panels to control the DSI
> > > > > host
> > > > > power up state.
> > > 
> > > [1]
> > > https://lore.kernel.org/dri-devel/6c0dd9fd-5d8e-537c-804f-7a03d5899a07@l
> > > ina
> > > ro.org/
> > > 
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > > 
> > > > >  drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
> > > > >  2 files changed, 56 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
> > > > > b/drivers/gpu/drm/drm_mipi_dsi.c index 14201f73aab1..c467162cb7d8
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > > > @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
> > > > > 
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
> > > > > 
> > > > > +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host
> > > > > *host)
> > > > > +{
> > > > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > > > +
> > > > > +     return ops && ops->power_up;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> > > > > +
> > > > > +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> > > > > +{
> > > > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > > > +
> > > > > +     if (!mipi_dsi_host_power_control_available(host))
> > > > > +             return -EOPNOTSUPP;
> > > > > +
> > > > > +     return ops->power_up ? ops->power_up(host) : 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> > > > > +
> > > > > +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> > > > > +{
> > > > > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > > > > +
> > > > > +     if (!mipi_dsi_host_power_control_available(host))
> > > > > +             return;
> > > > > +
> > > > > +     if (ops->power_down)
> > > > > +             ops->power_down(host);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> > > > > +
> > > > 
> > > > If this API is supposed to be used by DSI devices, it should probably
> > > > take a mipi_dsi_device pointer as a parameter?
> > > 
> > > Ack.
> > > 
> > > > We should probably make sure it hasn't been powered on already too?
> > > 
> > > Ack, I can add an atomic count there and a WARN_ON. However I don't
> > > think that it is a real problem.
> > > 
> > > > Maxime
> > > 
> > > --
> > > With best wishes
> > > 
> > > Dmitry
> > 
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Mon, 23 Oct 2023 at 09:52, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Dmitry,
>
> Am Sonntag, 22. Oktober 2023, 12:49:41 CEST schrieb Dmitry Baryshkov:
> > On Thu, 19 Oct 2023 at 14:42, Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi,
> > >
> > > Am Donnerstag, 19. Oktober 2023, 13:19:51 CEST schrieb Dmitry Baryshkov:
> > > > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> > > > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> > > > >
> > > > > Explaining why would help
> > > >
> > > > A kind of explanation comes afterwards, but probably I should change
> > > > the order of the phrases and expand it:
> > > >
> > > > The atomic_pre_enable / atomic_enable and correspondingly
> > > > atomic_disable / atomic_post_disable expect that the bridge links
> > > > follow a simple paradigm: either it is off, or it is on and streaming
> > > > video. Thus, it is fine to just enable the link at the enable time,
> > > > doing some preparations during the pre_enable.
> > > >
> > > > But then it causes several issues with DSI. First, some of the DSI
> > > > bridges and most of the DSI panels would like to send commands over
> > > > the DSI link to setup the device. Next, some of the DSI hosts have
> > > > limitations on sending the commands. The proverbial sunxi DSI host can
> > > > not send DSI commands after the video stream has started. Thus most of
> > > > the panels have opted to send all DSI commands from pre_enable (or
> > > > prepare) callback (before the video stream has started).
> > > >
> > > > However this leaves no good place for the DSI host to power up the DSI
> > > > link. By default the host's pre_enable callback is called after the
> > > > DSI bridge's pre_enable. For quite some time we were powering up the
> > > > DSI link from mode_set. This doesn't look fully correct. And also we
> > > > got into the issue with ps8640 bridge, which requires for the DSI link
> > > > to be quiet / unpowered at the bridge's reset time.
> > >
> > > There are also bridges (e.g. tc358767) which require DSI-LP11 upon bridge
> > > reset. And additionally this DSI-(e)DP bridge requires LP11 while
> > > accessing
> > > DP-AUX channel, e.g. reading EDID. So bridges need at least some control
> > > over DSI line state.
> >
> > For sending commands in LP11 it is typical to toggle the
> > MIPI_DSI_MODE_LPM flag, for example see panel-=jdi-lt070me05000.c or
> > some other drives. It might be a good idea to make that more explicit.
> > All suggestions here would be appreciated.
>
> The biggest difference between that display and the tc358767 bridge is that
> the display uses DSI commands, while the bridge is using i2c transfer to issue
> DP-AUX commands. There is no host_transfer [1] which would enable LP-11.
> It seems this DSI-DP bridge requires LP-11/HS on DSI lanes all times. This
> contradicts current Linux behaviour.

I see. I took a quick glance at the driver. Does the device mark AUX
as busy when there is a HS transfer?
Because otherwise it might be pretty hard to synchronise DP-AUX
transfers with the DSI link state. We might need to add an API for
this, if the DSI hosts actually can signal the blanking / DSI LP.

Side note: the driver needs some care. It doesn't support the aux-bus
bindings for eDP panels, it doesn't support other bridges on top of DP
connectors (but there can be e..g. dp-connector device).

>
> Best regards,
> Alexander
>
> [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation


-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Alexander Stein]

Am Montag, 23. Oktober 2023, 09:34:42 CEST schrieb Dmitry Baryshkov:
> On Mon, 23 Oct 2023 at 09:52, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi Dmitry,
> > 
> > Am Sonntag, 22. Oktober 2023, 12:49:41 CEST schrieb Dmitry Baryshkov:
> > > On Thu, 19 Oct 2023 at 14:42, Alexander Stein
> > > 
> > > <alexander.stein@ew.tq-group.com> wrote:
> > > > Hi,
> > > > 
> > > > Am Donnerstag, 19. Oktober 2023, 13:19:51 CEST schrieb Dmitry 
Baryshkov:
> > > > > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> 
wrote:
> > > > > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > > > > The MIPI DSI links do not fully fall into the DRM callbacks
> > > > > > > model.
> > > > > > 
> > > > > > Explaining why would help
> > > > > 
> > > > > A kind of explanation comes afterwards, but probably I should change
> > > > > the order of the phrases and expand it:
> > > > > 
> > > > > The atomic_pre_enable / atomic_enable and correspondingly
> > > > > atomic_disable / atomic_post_disable expect that the bridge links
> > > > > follow a simple paradigm: either it is off, or it is on and
> > > > > streaming
> > > > > video. Thus, it is fine to just enable the link at the enable time,
> > > > > doing some preparations during the pre_enable.
> > > > > 
> > > > > But then it causes several issues with DSI. First, some of the DSI
> > > > > bridges and most of the DSI panels would like to send commands over
> > > > > the DSI link to setup the device. Next, some of the DSI hosts have
> > > > > limitations on sending the commands. The proverbial sunxi DSI host
> > > > > can
> > > > > not send DSI commands after the video stream has started. Thus most
> > > > > of
> > > > > the panels have opted to send all DSI commands from pre_enable (or
> > > > > prepare) callback (before the video stream has started).
> > > > > 
> > > > > However this leaves no good place for the DSI host to power up the
> > > > > DSI
> > > > > link. By default the host's pre_enable callback is called after the
> > > > > DSI bridge's pre_enable. For quite some time we were powering up the
> > > > > DSI link from mode_set. This doesn't look fully correct. And also we
> > > > > got into the issue with ps8640 bridge, which requires for the DSI
> > > > > link
> > > > > to be quiet / unpowered at the bridge's reset time.
> > > > 
> > > > There are also bridges (e.g. tc358767) which require DSI-LP11 upon
> > > > bridge
> > > > reset. And additionally this DSI-(e)DP bridge requires LP11 while
> > > > accessing
> > > > DP-AUX channel, e.g. reading EDID. So bridges need at least some
> > > > control
> > > > over DSI line state.
> > > 
> > > For sending commands in LP11 it is typical to toggle the
> > > MIPI_DSI_MODE_LPM flag, for example see panel-=jdi-lt070me05000.c or
> > > some other drives. It might be a good idea to make that more explicit.
> > > All suggestions here would be appreciated.
> > 
> > The biggest difference between that display and the tc358767 bridge is
> > that
> > the display uses DSI commands, while the bridge is using i2c transfer to
> > issue DP-AUX commands. There is no host_transfer [1] which would enable
> > LP-11. It seems this DSI-DP bridge requires LP-11/HS on DSI lanes all
> > times. This contradicts current Linux behaviour.
> 
> I see. I took a quick glance at the driver. Does the device mark AUX
> as busy when there is a HS transfer?
> Because otherwise it might be pretty hard to synchronise DP-AUX
> transfers with the DSI link state. We might need to add an API for
> this, if the DSI hosts actually can signal the blanking / DSI LP.

I don't see that a synchronization would be required. AUX should be 
independent from DSI transfers. ASFAICS the bridge internals just requires DSI 
lines to be LP-00 or HS for AUX channel to be functioning.

> 
> Side note: the driver needs some care. It doesn't support the aux-bus
> bindings for eDP panels, it doesn't support other bridges on top of DP
> connectors (but there can be e..g. dp-connector device).

I don't think that this is necessary as you add an optional endpoint to port2 
which will then add an eDP display panel bridge. This should then handle aux-
bus bindings.

Best regards,
Alexander

> > Best regards,
> > Alexander
> > 
> > [1]
> > https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-> > bridge-operation


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Mon, 23 Oct 2023 at 11:14, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Am Montag, 23. Oktober 2023, 09:34:42 CEST schrieb Dmitry Baryshkov:
> > On Mon, 23 Oct 2023 at 09:52, Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Dmitry,
> > >
> > > Am Sonntag, 22. Oktober 2023, 12:49:41 CEST schrieb Dmitry Baryshkov:
> > > > On Thu, 19 Oct 2023 at 14:42, Alexander Stein
> > > >
> > > > <alexander.stein@ew.tq-group.com> wrote:
> > > > > Hi,
> > > > >
> > > > > Am Donnerstag, 19. Oktober 2023, 13:19:51 CEST schrieb Dmitry
> Baryshkov:
> > > > > > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org>
> wrote:
> > > > > > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > The MIPI DSI links do not fully fall into the DRM callbacks
> > > > > > > > model.
> > > > > > >
> > > > > > > Explaining why would help
> > > > > >
> > > > > > A kind of explanation comes afterwards, but probably I should change
> > > > > > the order of the phrases and expand it:
> > > > > >
> > > > > > The atomic_pre_enable / atomic_enable and correspondingly
> > > > > > atomic_disable / atomic_post_disable expect that the bridge links
> > > > > > follow a simple paradigm: either it is off, or it is on and
> > > > > > streaming
> > > > > > video. Thus, it is fine to just enable the link at the enable time,
> > > > > > doing some preparations during the pre_enable.
> > > > > >
> > > > > > But then it causes several issues with DSI. First, some of the DSI
> > > > > > bridges and most of the DSI panels would like to send commands over
> > > > > > the DSI link to setup the device. Next, some of the DSI hosts have
> > > > > > limitations on sending the commands. The proverbial sunxi DSI host
> > > > > > can
> > > > > > not send DSI commands after the video stream has started. Thus most
> > > > > > of
> > > > > > the panels have opted to send all DSI commands from pre_enable (or
> > > > > > prepare) callback (before the video stream has started).
> > > > > >
> > > > > > However this leaves no good place for the DSI host to power up the
> > > > > > DSI
> > > > > > link. By default the host's pre_enable callback is called after the
> > > > > > DSI bridge's pre_enable. For quite some time we were powering up the
> > > > > > DSI link from mode_set. This doesn't look fully correct. And also we
> > > > > > got into the issue with ps8640 bridge, which requires for the DSI
> > > > > > link
> > > > > > to be quiet / unpowered at the bridge's reset time.
> > > > >
> > > > > There are also bridges (e.g. tc358767) which require DSI-LP11 upon
> > > > > bridge
> > > > > reset. And additionally this DSI-(e)DP bridge requires LP11 while
> > > > > accessing
> > > > > DP-AUX channel, e.g. reading EDID. So bridges need at least some
> > > > > control
> > > > > over DSI line state.
> > > >
> > > > For sending commands in LP11 it is typical to toggle the
> > > > MIPI_DSI_MODE_LPM flag, for example see panel-=jdi-lt070me05000.c or
> > > > some other drives. It might be a good idea to make that more explicit.
> > > > All suggestions here would be appreciated.
> > >
> > > The biggest difference between that display and the tc358767 bridge is
> > > that
> > > the display uses DSI commands, while the bridge is using i2c transfer to
> > > issue DP-AUX commands. There is no host_transfer [1] which would enable
> > > LP-11. It seems this DSI-DP bridge requires LP-11/HS on DSI lanes all
> > > times. This contradicts current Linux behaviour.
> >
> > I see. I took a quick glance at the driver. Does the device mark AUX
> > as busy when there is a HS transfer?
> > Because otherwise it might be pretty hard to synchronise DP-AUX
> > transfers with the DSI link state. We might need to add an API for
> > this, if the DSI hosts actually can signal the blanking / DSI LP.
>
> I don't see that a synchronization would be required. AUX should be
> independent from DSI transfers. ASFAICS the bridge internals just requires DSI
> lines to be LP-00 or HS for AUX channel to be functioning.

Ah, LP or HS. Then it should be fine. I probably misread your original
email. I thought that AUX transfers work only in the LP mode.

>
> >
> > Side note: the driver needs some care. It doesn't support the aux-bus
> > bindings for eDP panels, it doesn't support other bridges on top of DP
> > connectors (but there can be e..g. dp-connector device).
>
> I don't think that this is necessary as you add an optional endpoint to port2
> which will then add an eDP display panel bridge. This should then handle aux-
> bus bindings.

Not quite, see Documentation/devicetree/bindings/display/dp-aux-bus.yaml
and devm_of_dp_aux_populate_bus().

It is expected that eDP panels are to be placed under the edp_bridge /
aux-bus device node. But this is a separate topic, I just wanted to
point out other missing pieces.

>
> Best regards,
> Alexander
>
> > > Best regards,
> > > Alexander
> > >
> > > [1]
> > > https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-> > bridge-operation
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4462 bytes --]

On Thu, Oct 19, 2023 at 02:19:51PM +0300, Dmitry Baryshkov wrote:
> On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> >
> > Explaining why would help
> 
> A kind of explanation comes afterwards, but probably I should change
> the order of the phrases and expand it:
> 
> The atomic_pre_enable / atomic_enable and correspondingly
> atomic_disable / atomic_post_disable expect that the bridge links
> follow a simple paradigm: either it is off, or it is on and streaming
> video. Thus, it is fine to just enable the link at the enable time,
> doing some preparations during the pre_enable.
> 
> But then it causes several issues with DSI. First, some of the DSI
> bridges and most of the DSI panels would like to send commands over
> the DSI link to setup the device.

What prevent them from doing it in enable when the link is enabled?

> Next, some of the DSI hosts have limitations on sending the commands.
> The proverbial sunxi DSI host can not send DSI commands after the
> video stream has started. Thus most of the panels have opted to send
> all DSI commands from pre_enable (or prepare) callback (before the
> video stream has started).

I'm not sure we should account for a single driver when designing a
framework. We should focus on designing something sound, and then making
that driver work with whatever we designed, but not the other way
around. And if we can't, we should get rid of that driver because it's
de-facto unmaintainable. And I'm saying that as the author of that
driver.

> However this leaves no good place for the DSI host to power up the DSI
> link. By default the host's pre_enable callback is called after the
> DSI bridge's pre_enable. For quite some time we were powering up the
> DSI link from mode_set. This doesn't look fully correct.

Yeah, it's not.

> And also we got into the issue with ps8640 bridge, which requires for
> the DSI link to be quiet / unpowered at the bridge's reset time.
> 
> Dave has come with the idea of pre_enable_prev_first /
> prepare_prev_first flags, which attempt to solve the issue by
> reversing the order of pre_enable callbacks. This mostly solves the
> issue. However during this cycle it became obvious that this approach
> is not ideal too. There is no way for the DSI host to know whether the
> DSI panel / bridge has been updated to use this flag or not, see the
> discussion at [1].

Yeah. Well, that happens. I kind of disagree with Neil here though when
he says that "A panel driver should not depend on features of a DSI
controller". Panels definitely rely on particular features, like the
number of lanes, the modes supported, etc.

Panels shouldn't depend on a particular driver *behaviour*. But the
features are fine.

For our particular discussion, I think that that kind of discussion is a
dead-end, and we just shouldn't worry about it. Yes, some panels have
not yet been updated to take the new flags into account. However, the
proper thing to do is to update them if we see a problem with that (and
thus move forward to the ideal solution), not revert the beginning of
that feature enablement (thus moving away from where we want to end up
in).

> Thus comes this proposal. It allows for the panels to explicitly bring
> the link up and down at the correct time, it supports automatic use
> case, where no special handling is required. And last, but not least,
> it allows the DSI host to note that the bridge / panel were not
> updated to follow new protocol and thus the link should be powered on
> at the mode_set time. This leaves us with the possibility of dropping
> support for this workaround once all in-kernel drivers are updated.

I'm kind of skeptical for these kind of claims that everything will be
automatic and will be handled fine. What if we have conflicting
requirements, for example two bridges drivers that would request the
power up at different times?

Also, we would still need to update every single panel driver, which is
going to create a lot of boilerplate that people might get wrong.

I have the feeling that we should lay out the problem without talking
about any existing code base first. So, what does the MIPI-DSI spec
requires and what does panels and bridges expect?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On 25/10/2023 15:44, Maxime Ripard wrote:
> On Thu, Oct 19, 2023 at 02:19:51PM +0300, Dmitry Baryshkov wrote:
>> On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
>>>
>>> On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
>>>> The MIPI DSI links do not fully fall into the DRM callbacks model.
>>>
>>> Explaining why would help
>>
>> A kind of explanation comes afterwards, but probably I should change
>> the order of the phrases and expand it:
>>
>> The atomic_pre_enable / atomic_enable and correspondingly
>> atomic_disable / atomic_post_disable expect that the bridge links
>> follow a simple paradigm: either it is off, or it is on and streaming
>> video. Thus, it is fine to just enable the link at the enable time,
>> doing some preparations during the pre_enable.
>>
>> But then it causes several issues with DSI. First, some of the DSI
>> bridges and most of the DSI panels would like to send commands over
>> the DSI link to setup the device.
> 
> What prevent them from doing it in enable when the link is enabled?
> 
>> Next, some of the DSI hosts have limitations on sending the commands.
>> The proverbial sunxi DSI host can not send DSI commands after the
>> video stream has started. Thus most of the panels have opted to send
>> all DSI commands from pre_enable (or prepare) callback (before the
>> video stream has started).
> 
> I'm not sure we should account for a single driver when designing a
> framework. We should focus on designing something sound, and then making
> that driver work with whatever we designed, but not the other way
> around. And if we can't, we should get rid of that driver because it's
> de-facto unmaintainable. And I'm saying that as the author of that
> driver.

That's not the only driver with strange peculiarities. For example, see 
commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming 
from pre-enable to enable"), which was one of the issues that actually 
prompted me to send this this patchset (after my previous version of 
this patch being rejected because of sunxi).

> 
>> However this leaves no good place for the DSI host to power up the DSI
>> link. By default the host's pre_enable callback is called after the
>> DSI bridge's pre_enable. For quite some time we were powering up the
>> DSI link from mode_set. This doesn't look fully correct.
> 
> Yeah, it's not.
> 
>> And also we got into the issue with ps8640 bridge, which requires for
>> the DSI link to be quiet / unpowered at the bridge's reset time.
>>
>> Dave has come with the idea of pre_enable_prev_first /
>> prepare_prev_first flags, which attempt to solve the issue by
>> reversing the order of pre_enable callbacks. This mostly solves the
>> issue. However during this cycle it became obvious that this approach
>> is not ideal too. There is no way for the DSI host to know whether the
>> DSI panel / bridge has been updated to use this flag or not, see the
>> discussion at [1].
> 
> Yeah. Well, that happens. I kind of disagree with Neil here though when
> he says that "A panel driver should not depend on features of a DSI
> controller". Panels definitely rely on particular features, like the
> number of lanes, the modes supported, etc.

In the mentioned discussion it was more about 'DSI host should not 
assume panel driver features', like the panel sending commands in 
pre_enable or not, or having pre_enable_prev_first.

So the pre_enable_prev_first clearly lacks feature negotiation.


> 
> Panels shouldn't depend on a particular driver *behaviour*. But the
> features are fine.
> 
> For our particular discussion, I think that that kind of discussion is a
> dead-end, and we just shouldn't worry about it. Yes, some panels have
> not yet been updated to take the new flags into account. However, the
> proper thing to do is to update them if we see a problem with that (and
> thus move forward to the ideal solution), not revert the beginning of
> that feature enablement (thus moving away from where we want to end up
> in).
> 
>> Thus comes this proposal. It allows for the panels to explicitly bring
>> the link up and down at the correct time, it supports automatic use
>> case, where no special handling is required. And last, but not least,
>> it allows the DSI host to note that the bridge / panel were not
>> updated to follow new protocol and thus the link should be powered on
>> at the mode_set time. This leaves us with the possibility of dropping
>> support for this workaround once all in-kernel drivers are updated.
> 
> I'm kind of skeptical for these kind of claims that everything will be
> automatic and will be handled fine. What if we have conflicting
> requirements, for example two bridges drivers that would request the
> power up at different times?

Well, we do not support DSI sublinks, do we?

> 
> Also, we would still need to update every single panel driver, which is
> going to create a lot of boilerplate that people might get wrong.

Yes, quite unfortunately. Another approach that I have in mind is to add 
two callbacks to mipi_dsi_device. This way the DSI host will call into 
the device to initialise it once the link has been powered up and just 
before tearing it down. We solve a lot of problems this way, no 
boilerplate and the panel / bridge are in control of the initialisation 
procedure. WDYT?

> I have the feeling that we should lay out the problem without talking
> about any existing code base first. So, what does the MIPI-DSI spec
> requires and what does panels and bridges expect?

There is not that much in the DSI spec (or maybe I do not understand the 
question). The spec is more about the power states and the commands. Our 
problem is that this doesn't fully match kernel expectations.

-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7214 bytes --]

On Wed, Oct 25, 2023 at 06:16:14PM +0300, Dmitry Baryshkov wrote:
> On 25/10/2023 15:44, Maxime Ripard wrote:
> > On Thu, Oct 19, 2023 at 02:19:51PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> > > > 
> > > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> > > > 
> > > > Explaining why would help
> > > 
> > > A kind of explanation comes afterwards, but probably I should change
> > > the order of the phrases and expand it:
> > > 
> > > The atomic_pre_enable / atomic_enable and correspondingly
> > > atomic_disable / atomic_post_disable expect that the bridge links
> > > follow a simple paradigm: either it is off, or it is on and streaming
> > > video. Thus, it is fine to just enable the link at the enable time,
> > > doing some preparations during the pre_enable.
> > > 
> > > But then it causes several issues with DSI. First, some of the DSI
> > > bridges and most of the DSI panels would like to send commands over
> > > the DSI link to setup the device.
> > 
> > What prevent them from doing it in enable when the link is enabled?
> > 
> > > Next, some of the DSI hosts have limitations on sending the commands.
> > > The proverbial sunxi DSI host can not send DSI commands after the
> > > video stream has started. Thus most of the panels have opted to send
> > > all DSI commands from pre_enable (or prepare) callback (before the
> > > video stream has started).
> > 
> > I'm not sure we should account for a single driver when designing a
> > framework. We should focus on designing something sound, and then making
> > that driver work with whatever we designed, but not the other way
> > around. And if we can't, we should get rid of that driver because it's
> > de-facto unmaintainable. And I'm saying that as the author of that
> > driver.
> 
> That's not the only driver with strange peculiarities. For example, see
> commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming from
> pre-enable to enable"), which was one of the issues that actually prompted
> me to send this this patchset (after my previous version of this patch being
> rejected because of sunxi).

The datasheet for that bridge is available so at least we can try to fix
it (and bridges are much simpler than controllers anyway). It's not
something we can do with the sunxi driver.

> > > However this leaves no good place for the DSI host to power up the DSI
> > > link. By default the host's pre_enable callback is called after the
> > > DSI bridge's pre_enable. For quite some time we were powering up the
> > > DSI link from mode_set. This doesn't look fully correct.
> > 
> > Yeah, it's not.
> > 
> > > And also we got into the issue with ps8640 bridge, which requires for
> > > the DSI link to be quiet / unpowered at the bridge's reset time.
> > > 
> > > Dave has come with the idea of pre_enable_prev_first /
> > > prepare_prev_first flags, which attempt to solve the issue by
> > > reversing the order of pre_enable callbacks. This mostly solves the
> > > issue. However during this cycle it became obvious that this approach
> > > is not ideal too. There is no way for the DSI host to know whether the
> > > DSI panel / bridge has been updated to use this flag or not, see the
> > > discussion at [1].
> > 
> > Yeah. Well, that happens. I kind of disagree with Neil here though when
> > he says that "A panel driver should not depend on features of a DSI
> > controller". Panels definitely rely on particular features, like the
> > number of lanes, the modes supported, etc.
> 
> In the mentioned discussion it was more about 'DSI host should not assume
> panel driver features', like the panel sending commands in pre_enable or
> not, or having pre_enable_prev_first.
> 
> So the pre_enable_prev_first clearly lacks feature negotiation.
> 
> > Panels shouldn't depend on a particular driver *behaviour*. But the
> > features are fine.
> > 
> > For our particular discussion, I think that that kind of discussion is a
> > dead-end, and we just shouldn't worry about it. Yes, some panels have
> > not yet been updated to take the new flags into account. However, the
> > proper thing to do is to update them if we see a problem with that (and
> > thus move forward to the ideal solution), not revert the beginning of
> > that feature enablement (thus moving away from where we want to end up
> > in).
> > 
> > > Thus comes this proposal. It allows for the panels to explicitly bring
> > > the link up and down at the correct time, it supports automatic use
> > > case, where no special handling is required. And last, but not least,
> > > it allows the DSI host to note that the bridge / panel were not
> > > updated to follow new protocol and thus the link should be powered on
> > > at the mode_set time. This leaves us with the possibility of dropping
> > > support for this workaround once all in-kernel drivers are updated.
> > 
> > I'm kind of skeptical for these kind of claims that everything will be
> > automatic and will be handled fine. What if we have conflicting
> > requirements, for example two bridges drivers that would request the
> > power up at different times?
> 
> Well, we do not support DSI sublinks, do we?

No, but we start to consider adding support for muxes for example. A DSI
mux + a DSI bridge behind it might trigger that behaviour, even if we
don't support sublinks.

> > Also, we would still need to update every single panel driver, which is
> > going to create a lot of boilerplate that people might get wrong.
> 
> Yes, quite unfortunately. Another approach that I have in mind is to add two
> callbacks to mipi_dsi_device. This way the DSI host will call into the
> device to initialise it once the link has been powered up and just before
> tearing it down. We solve a lot of problems this way, no boilerplate and the
> panel / bridge are in control of the initialisation procedure. WDYT?
> 
> > I have the feeling that we should lay out the problem without talking
> > about any existing code base first. So, what does the MIPI-DSI spec
> > requires and what does panels and bridges expect?
> 
> There is not that much in the DSI spec (or maybe I do not understand the
> question). The spec is more about the power states and the commands. Our
> problem is that this doesn't fully match kernel expectations.

You're explicitly asking for comments on that series. How can we provide
any comment if you're dead-set on a particular implementation and not
explain what the problem you are trying to solve is?

Thinking more about it, I'm even more skeptical about the general
approach that this should be implemented at the bridge level (or in
KMS).

It looks to me that this is very much a bus problem. USB device drivers
also require the bus to be powered and generally available to send data
to their device, and you don't fix that up in the HID or storage
drivers, you make the bus behave that way.

What prevents us from fixing it at the bus level?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Thu, 26 Oct 2023 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Oct 25, 2023 at 06:16:14PM +0300, Dmitry Baryshkov wrote:
> > On 25/10/2023 15:44, Maxime Ripard wrote:
> > > On Thu, Oct 19, 2023 at 02:19:51PM +0300, Dmitry Baryshkov wrote:
> > > > On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Mon, Oct 16, 2023 at 07:53:48PM +0300, Dmitry Baryshkov wrote:
> > > > > > The MIPI DSI links do not fully fall into the DRM callbacks model.
> > > > >
> > > > > Explaining why would help
> > > >
> > > > A kind of explanation comes afterwards, but probably I should change
> > > > the order of the phrases and expand it:
> > > >
> > > > The atomic_pre_enable / atomic_enable and correspondingly
> > > > atomic_disable / atomic_post_disable expect that the bridge links
> > > > follow a simple paradigm: either it is off, or it is on and streaming
> > > > video. Thus, it is fine to just enable the link at the enable time,
> > > > doing some preparations during the pre_enable.
> > > >
> > > > But then it causes several issues with DSI. First, some of the DSI
> > > > bridges and most of the DSI panels would like to send commands over
> > > > the DSI link to setup the device.
> > >
> > > What prevent them from doing it in enable when the link is enabled?
> > >
> > > > Next, some of the DSI hosts have limitations on sending the commands.
> > > > The proverbial sunxi DSI host can not send DSI commands after the
> > > > video stream has started. Thus most of the panels have opted to send
> > > > all DSI commands from pre_enable (or prepare) callback (before the
> > > > video stream has started).
> > >
> > > I'm not sure we should account for a single driver when designing a
> > > framework. We should focus on designing something sound, and then making
> > > that driver work with whatever we designed, but not the other way
> > > around. And if we can't, we should get rid of that driver because it's
> > > de-facto unmaintainable. And I'm saying that as the author of that
> > > driver.
> >
> > That's not the only driver with strange peculiarities. For example, see
> > commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming from
> > pre-enable to enable"), which was one of the issues that actually prompted
> > me to send this this patchset (after my previous version of this patch being
> > rejected because of sunxi).
>
> The datasheet for that bridge is available so at least we can try to fix
> it (and bridges are much simpler than controllers anyway). It's not
> something we can do with the sunxi driver.
>
> > > > However this leaves no good place for the DSI host to power up the DSI
> > > > link. By default the host's pre_enable callback is called after the
> > > > DSI bridge's pre_enable. For quite some time we were powering up the
> > > > DSI link from mode_set. This doesn't look fully correct.
> > >
> > > Yeah, it's not.
> > >
> > > > And also we got into the issue with ps8640 bridge, which requires for
> > > > the DSI link to be quiet / unpowered at the bridge's reset time.
> > > >
> > > > Dave has come with the idea of pre_enable_prev_first /
> > > > prepare_prev_first flags, which attempt to solve the issue by
> > > > reversing the order of pre_enable callbacks. This mostly solves the
> > > > issue. However during this cycle it became obvious that this approach
> > > > is not ideal too. There is no way for the DSI host to know whether the
> > > > DSI panel / bridge has been updated to use this flag or not, see the
> > > > discussion at [1].
> > >
> > > Yeah. Well, that happens. I kind of disagree with Neil here though when
> > > he says that "A panel driver should not depend on features of a DSI
> > > controller". Panels definitely rely on particular features, like the
> > > number of lanes, the modes supported, etc.
> >
> > In the mentioned discussion it was more about 'DSI host should not assume
> > panel driver features', like the panel sending commands in pre_enable or
> > not, or having pre_enable_prev_first.
> >
> > So the pre_enable_prev_first clearly lacks feature negotiation.
> >
> > > Panels shouldn't depend on a particular driver *behaviour*. But the
> > > features are fine.
> > >
> > > For our particular discussion, I think that that kind of discussion is a
> > > dead-end, and we just shouldn't worry about it. Yes, some panels have
> > > not yet been updated to take the new flags into account. However, the
> > > proper thing to do is to update them if we see a problem with that (and
> > > thus move forward to the ideal solution), not revert the beginning of
> > > that feature enablement (thus moving away from where we want to end up
> > > in).
> > >
> > > > Thus comes this proposal. It allows for the panels to explicitly bring
> > > > the link up and down at the correct time, it supports automatic use
> > > > case, where no special handling is required. And last, but not least,
> > > > it allows the DSI host to note that the bridge / panel were not
> > > > updated to follow new protocol and thus the link should be powered on
> > > > at the mode_set time. This leaves us with the possibility of dropping
> > > > support for this workaround once all in-kernel drivers are updated.
> > >
> > > I'm kind of skeptical for these kind of claims that everything will be
> > > automatic and will be handled fine. What if we have conflicting
> > > requirements, for example two bridges drivers that would request the
> > > power up at different times?
> >
> > Well, we do not support DSI sublinks, do we?
>
> No, but we start to consider adding support for muxes for example. A DSI
> mux + a DSI bridge behind it might trigger that behaviour, even if we
> don't support sublinks.

Ack.

>
> > > Also, we would still need to update every single panel driver, which is
> > > going to create a lot of boilerplate that people might get wrong.
> >
> > Yes, quite unfortunately. Another approach that I have in mind is to add two
> > callbacks to mipi_dsi_device. This way the DSI host will call into the
> > device to initialise it once the link has been powered up and just before
> > tearing it down. We solve a lot of problems this way, no boilerplate and the
> > panel / bridge are in control of the initialisation procedure. WDYT?
> >
> > > I have the feeling that we should lay out the problem without talking
> > > about any existing code base first. So, what does the MIPI-DSI spec
> > > requires and what does panels and bridges expect?
> >
> > There is not that much in the DSI spec (or maybe I do not understand the
> > question). The spec is more about the power states and the commands. Our
> > problem is that this doesn't fully match kernel expectations.
>
> You're explicitly asking for comments on that series. How can we provide
> any comment if you're dead-set on a particular implementation and not
> explain what the problem you are trying to solve is?

Ah, excuse me. I thought that I explained that in the cover letter.

DSI device lifetime has three different stages:
1. before the DSI link being powered up and clocking,
2. when the DSI link is in LP state (for the purpose of this question,
this is the time between the DSI link being powered up and the video
stream start)
3. when the DSI link is in HS state (while streaming the video).

Different DSI bridges have different requirements with respect to the
code being executed at stages 1 and 2. For example several DSI-to-eDP
bridges (ps8640, tc358767 require for the link to be quiet during
reset time.
The DSI-controlled bridges and DSI panels need to send some commands
in stage 2, before starting up video

In the DRM subsystem stage 3 naturally maps to the
drm_bridge_funcs::enable, stage 1 also naturally maps to the
drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
the DRM call chain.
Earlier we attempted to solve that using the pre_enable_prev_first,
which remapped pre-enable callback execution order. However it has led
us to the two issues. First, at the DSI host driver we do not know
whether the panel / bridge were updated to use pre_enable_prev_first
or not. Second, if the bridge has to perform steps during both stages
1 and 2, it can not do that.

I'm trying to find a way to express the difference between stages 1
and 2 in the generic code, so that we do not to worry about particular
DSI host and DSI bridge / panel peculiarities when implementing the
DSI host and/or DSI panel driver.

Last, but not least, we currently document that it is fine to call DSI
transfer functions at any point during the driver's life time (at
least that was the interpretation that we have agreed in the
DSI-related threads). It has its own drawbacks for the DSI host
drivers. The hosts have to deal with the DSI commands being sent at
the different times, when the host is fully powered down, when it is
running in the LP mode and when it is fully running and streaming
video. By defining DSI lifetime more precisely, we can limit the
period when the DSI commands can be legitimately sent, simplifying DSI
host drives.

> Thinking more about it, I'm even more skeptical about the general
> approach that this should be implemented at the bridge level (or in
> KMS).
>
> It looks to me that this is very much a bus problem. USB device drivers
> also require the bus to be powered and generally available to send data
> to their device, and you don't fix that up in the HID or storage
> drivers, you make the bus behave that way.
>
> What prevents us from fixing it at the bus level?

Yes, this can also be possible. Do you mean adding code / callbacks to
struct mipi_dsi_device ?

-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4719 bytes --]

+GKH

On Thu, Oct 26, 2023 at 11:41:34AM +0300, Dmitry Baryshkov wrote:
> > > > Also, we would still need to update every single panel driver, which is
> > > > going to create a lot of boilerplate that people might get wrong.
> > >
> > > Yes, quite unfortunately. Another approach that I have in mind is to add two
> > > callbacks to mipi_dsi_device. This way the DSI host will call into the
> > > device to initialise it once the link has been powered up and just before
> > > tearing it down. We solve a lot of problems this way, no boilerplate and the
> > > panel / bridge are in control of the initialisation procedure. WDYT?
> > >
> > > > I have the feeling that we should lay out the problem without talking
> > > > about any existing code base first. So, what does the MIPI-DSI spec
> > > > requires and what does panels and bridges expect?
> > >
> > > There is not that much in the DSI spec (or maybe I do not understand the
> > > question). The spec is more about the power states and the commands. Our
> > > problem is that this doesn't fully match kernel expectations.
> >
> > You're explicitly asking for comments on that series. How can we provide
> > any comment if you're dead-set on a particular implementation and not
> > explain what the problem you are trying to solve is?
> 
> Ah, excuse me. I thought that I explained that in the cover letter.
> 
> DSI device lifetime has three different stages:
> 1. before the DSI link being powered up and clocking,
> 2. when the DSI link is in LP state (for the purpose of this question,
> this is the time between the DSI link being powered up and the video
> stream start)
> 3. when the DSI link is in HS state (while streaming the video).
> 
> Different DSI bridges have different requirements with respect to the
> code being executed at stages 1 and 2. For example several DSI-to-eDP
> bridges (ps8640, tc358767 require for the link to be quiet during
> reset time.
> The DSI-controlled bridges and DSI panels need to send some commands
> in stage 2, before starting up video
> 
> In the DRM subsystem stage 3 naturally maps to the
> drm_bridge_funcs::enable, stage 1 also naturally maps to the
> drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
> the DRM call chain.
> Earlier we attempted to solve that using the pre_enable_prev_first,
> which remapped pre-enable callback execution order. However it has led
> us to the two issues. First, at the DSI host driver we do not know
> whether the panel / bridge were updated to use pre_enable_prev_first
> or not. Second, if the bridge has to perform steps during both stages
> 1 and 2, it can not do that.
> 
> I'm trying to find a way to express the difference between stages 1
> and 2 in the generic code, so that we do not to worry about particular
> DSI host and DSI bridge / panel peculiarities when implementing the
> DSI host and/or DSI panel driver.
> 
> Last, but not least, we currently document that it is fine to call DSI
> transfer functions at any point during the driver's life time (at
> least that was the interpretation that we have agreed in the
> DSI-related threads). It has its own drawbacks for the DSI host
> drivers. The hosts have to deal with the DSI commands being sent at
> the different times, when the host is fully powered down, when it is
> running in the LP mode and when it is fully running and streaming
> video. By defining DSI lifetime more precisely, we can limit the
> period when the DSI commands can be legitimately sent, simplifying DSI
> host drives.

Thanks for writing this :)

> > Thinking more about it, I'm even more skeptical about the general
> > approach that this should be implemented at the bridge level (or in
> > KMS).
> >
> > It looks to me that this is very much a bus problem. USB device drivers
> > also require the bus to be powered and generally available to send data
> > to their device, and you don't fix that up in the HID or storage
> > drivers, you make the bus behave that way.
> >
> > What prevents us from fixing it at the bus level?
> 
> Yes, this can also be possible. Do you mean adding code / callbacks to
> struct mipi_dsi_device ?

Yes, even more so with your summary above, I really think this should be
dealt with at the bus layer.

To put it in a different way, if we had an (imaginary, probably)
MIPI-DSI device that didn't fit into KMS but in any other framework, we
would still have all the constraints you list above. It really is a bus
matter, not something that bridges need to express.

I had a look at the other buses we have in the kernel and it looks like
HSI might have the same requirements?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote:
> +GKH

Why?  I don't see a question for me here, sorry.

greg k-h

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On Tue, Nov 07, 2023 at 12:22:21PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote:
> > +GKH
> 
> Why?  I don't see a question for me here, sorry.

I guess the question is: we have a bus with various power states
(powered off, low power, high speed)

low power is typically used to send commands to a device, high speed to
transmit pixels, but still allows to send commands.

Depending on the devices, there's different requirements about the state
devices expect the bus to be in to send commands. Some will need to send
all the commands in the low power state, some don't care, etc. See
the mail I was replying too for more details.

We've tried so far to model that in KMS itself, so the framework the
drivers would register too, but we're kind of reaching the limits of
what we can do there. It also feels to me that "the driver can't access
its device" is more of a problem for the bus to solve rather than the
framework.

Do you agree? Are you aware of any other bus in Linux with similar
requirements we could look at? Or any suggestion on how to solve it?

Thanks
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 01:18:14PM +0100, Maxime Ripard wrote:
> On Tue, Nov 07, 2023 at 12:22:21PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote:
> > > +GKH
> > 
> > Why?  I don't see a question for me here, sorry.
> 
> I guess the question is: we have a bus with various power states
> (powered off, low power, high speed)

Great, have fun!  And is this per-device or per-bus-instance?

> low power is typically used to send commands to a device, high speed to
> transmit pixels, but still allows to send commands.
> 
> Depending on the devices, there's different requirements about the state
> devices expect the bus to be in to send commands. Some will need to send
> all the commands in the low power state, some don't care, etc. See
> the mail I was replying too for more details.
> 
> We've tried so far to model that in KMS itself, so the framework the
> drivers would register too, but we're kind of reaching the limits of
> what we can do there. It also feels to me that "the driver can't access
> its device" is more of a problem for the bus to solve rather than the
> framework.

This is up to the specific bus to resolve, there's nothing special
needed in the driver core for it, right?

> Do you agree? Are you aware of any other bus in Linux with similar
> requirements we could look at? Or any suggestion on how to solve it?

There might be others, yes, look at how the dynamic power management
works for different devices on most busses, that might help you out
here.

good luck!

greg k-h

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

Hi,

Thanks for your answer

On Tue, Nov 07, 2023 at 04:26:34PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 07, 2023 at 01:18:14PM +0100, Maxime Ripard wrote:
> > On Tue, Nov 07, 2023 at 12:22:21PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote:
> > > > +GKH
> > > 
> > > Why?  I don't see a question for me here, sorry.
> > 
> > I guess the question is: we have a bus with various power states
> > (powered off, low power, high speed)
> 
> Great, have fun!  And is this per-device or per-bus-instance?

Per bus instance

> > low power is typically used to send commands to a device, high speed to
> > transmit pixels, but still allows to send commands.
> > 
> > Depending on the devices, there's different requirements about the state
> > devices expect the bus to be in to send commands. Some will need to send
> > all the commands in the low power state, some don't care, etc. See
> > the mail I was replying too for more details.
> > 
> > We've tried so far to model that in KMS itself, so the framework the
> > drivers would register too, but we're kind of reaching the limits of
> > what we can do there. It also feels to me that "the driver can't access
> > its device" is more of a problem for the bus to solve rather than the
> > framework.
> 
> This is up to the specific bus to resolve, there's nothing special
> needed in the driver core for it, right?

Yeah, we weren't really looking to handle this into the driver core, but
rather if there was a set of guidelines or feedback on implementing
those kind of features for a bus.

> > Do you agree? Are you aware of any other bus in Linux with similar
> > requirements we could look at? Or any suggestion on how to solve it?
> 
> There might be others, yes, look at how the dynamic power management
> works for different devices on most busses, that might help you out
> here.

Thanks for the pointers, we'll have a look
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Laurent Pinchart]

On Wed, Nov 08, 2023 at 04:34:39PM +0100, Maxime Ripard wrote:
> On Tue, Nov 07, 2023 at 04:26:34PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 07, 2023 at 01:18:14PM +0100, Maxime Ripard wrote:
> > > On Tue, Nov 07, 2023 at 12:22:21PM +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote:
> > > > > +GKH
> > > > 
> > > > Why?  I don't see a question for me here, sorry.
> > > 
> > > I guess the question is: we have a bus with various power states
> > > (powered off, low power, high speed)
> > 
> > Great, have fun!  And is this per-device or per-bus-instance?
> 
> Per bus instance

To be precise, those power states are link states. They don't
necessarily translate directly to device power states, and they're not
so much about power management than speed (and bus turn-around for
reads) management.

Also, while DSI allows for multiple peripherals on a bus, the link is
point-to-point, with the peripherals being all behind a single DSI RX.

> > > low power is typically used to send commands to a device, high speed to
> > > transmit pixels, but still allows to send commands.

Low power (LP) is a link state where commands can be transmitted at a
low speed, as opposed to the high speed (HS) link state that is used to
transmit both video data and commands at high speed. Any device-to-host
data transfer (in response to read commands) occurs exclusively in LP
mode (at least with DSI v1.3, I don't have acces to newer
specifications).

> > > Depending on the devices, there's different requirements about the state
> > > devices expect the bus to be in to send commands. Some will need to send
> > > all the commands in the low power state, some don't care, etc. See
> > > the mail I was replying too for more details.
> > > 
> > > We've tried so far to model that in KMS itself, so the framework the
> > > drivers would register too, but we're kind of reaching the limits of
> > > what we can do there. It also feels to me that "the driver can't access
> > > its device" is more of a problem for the bus to solve rather than the
> > > framework.
> > 
> > This is up to the specific bus to resolve, there's nothing special
> > needed in the driver core for it, right?
> 
> Yeah, we weren't really looking to handle this into the driver core, but
> rather if there was a set of guidelines or feedback on implementing
> those kind of features for a bus.
> 
> > > Do you agree? Are you aware of any other bus in Linux with similar
> > > requirements we could look at? Or any suggestion on how to solve it?
> > 
> > There might be others, yes, look at how the dynamic power management
> > works for different devices on most busses, that might help you out
> > here.
> 
> Thanks for the pointers, we'll have a look

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Neil Armstrong]

On 08/11/2023 16:58, Laurent Pinchart wrote:
> On Wed, Nov 08, 2023 at 04:34:39PM +0100, Maxime Ripard wrote:
>> On Tue, Nov 07, 2023 at 04:26:34PM +0100, Greg Kroah-Hartman wrote:
>>> On Tue, Nov 07, 2023 at 01:18:14PM +0100, Maxime Ripard wrote:
>>>> On Tue, Nov 07, 2023 at 12:22:21PM +0100, Greg Kroah-Hartman wrote:
>>>>> On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote:
>>>>>> +GKH
>>>>>
>>>>> Why?  I don't see a question for me here, sorry.
>>>>
>>>> I guess the question is: we have a bus with various power states
>>>> (powered off, low power, high speed)
>>>
>>> Great, have fun!  And is this per-device or per-bus-instance?
>>
>> Per bus instance
> 
> To be precise, those power states are link states. They don't
> necessarily translate directly to device power states, and they're not
> so much about power management than speed (and bus turn-around for
> reads) management.

So the DSI core should support handling and tracking the current DSI
link state, and DSI devices should be able to request for a particular
link state.

> 
> Also, while DSI allows for multiple peripherals on a bus, the link is
> point-to-point, with the peripherals being all behind a single DSI RX. >
>>>> low power is typically used to send commands to a device, high speed to
>>>> transmit pixels, but still allows to send commands.
> 
> Low power (LP) is a link state where commands can be transmitted at a
> low speed, as opposed to the high speed (HS) link state that is used to
> transmit both video data and commands at high speed. Any device-to-host
> data transfer (in response to read commands) occurs exclusively in LP
> mode (at least with DSI v1.3, I don't have acces to newer
> specifications).
> 
>>>> Depending on the devices, there's different requirements about the state
>>>> devices expect the bus to be in to send commands. Some will need to send
>>>> all the commands in the low power state, some don't care, etc. See
>>>> the mail I was replying too for more details.
>>>>
>>>> We've tried so far to model that in KMS itself, so the framework the
>>>> drivers would register too, but we're kind of reaching the limits of
>>>> what we can do there. It also feels to me that "the driver can't access
>>>> its device" is more of a problem for the bus to solve rather than the
>>>> framework.
>>>
>>> This is up to the specific bus to resolve, there's nothing special
>>> needed in the driver core for it, right?
>>
>> Yeah, we weren't really looking to handle this into the driver core, but
>> rather if there was a set of guidelines or feedback on implementing
>> those kind of features for a bus.
>>
>>>> Do you agree? Are you aware of any other bus in Linux with similar
>>>> requirements we could look at? Or any suggestion on how to solve it?
>>>
>>> There might be others, yes, look at how the dynamic power management
>>> works for different devices on most busses, that might help you out
>>> here.
>>
>> Thanks for the pointers, we'll have a look
> 

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Michael Walle]

Hi,

> DSI device lifetime has three different stages:
> 1. before the DSI link being powered up and clocking,
> 2. when the DSI link is in LP state (for the purpose of this question,
> this is the time between the DSI link being powered up and the video
> stream start)
> 3. when the DSI link is in HS state (while streaming the video).

It's not clear to me what (2) is. What is the state of the clock and
data lanes?

I'm facing similar issues with the tc358775 bridge. This bridge needs
to release its reset while both clock and data lanes are in LP-11 mode.
But then it needs to be configured (via I2C) while the clock lane is
in enabled (HS mode), but the data lanes are still in LP-11 mode.

To me it looks like there is a fouth case then:
1. unpowered
2. DSI clock and data are in LP-11
3. DSI clock is in HS and data are in LP-11
4. DSI clock is in HS and data is in HS

(And of course the bridge needs continuous clock mode).

> Different DSI bridges have different requirements with respect to the
> code being executed at stages 1 and 2. For example several DSI-to-eDP
> bridges (ps8640, tc358767 require for the link to be quiet during
> reset time.
> The DSI-controlled bridges and DSI panels need to send some commands
> in stage 2, before starting up video
> 
> In the DRM subsystem stage 3 naturally maps to the
> drm_bridge_funcs::enable, stage 1 also naturally maps to the
> drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
> the DRM call chain.
> Earlier we attempted to solve that using the pre_enable_prev_first,
> which remapped pre-enable callback execution order. However it has led
> us to the two issues. First, at the DSI host driver we do not know
> whether the panel / bridge were updated to use pre_enable_prev_first
> or not. Second, if the bridge has to perform steps during both stages
> 1 and 2, it can not do that.
> 
> I'm trying to find a way to express the difference between stages 1
> and 2 in the generic code, so that we do not to worry about particular
> DSI host and DSI bridge / panel peculiarities when implementing the
> DSI host and/or DSI panel driver.

For now, I have a rather hacky ".dsi_lp11_notify" callback in
drm_bridge_funcs which is supposed to be called by the DSI host while the
clock and data lanes are in LP-11 mode. But that is rather an RFC and me
needing something to get the driver for this bridge working. Because it's
badly broken. FWIW, you can find my work-in-progress patches at
https://github.com/mwalle/linux/tree/feature-tc358775-fixes

-michael

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Mon, 27 Nov 2023 at 18:07, Michael Walle <mwalle@kernel.org> wrote:
>
> Hi,
>
> > DSI device lifetime has three different stages:
> > 1. before the DSI link being powered up and clocking,
> > 2. when the DSI link is in LP state (for the purpose of this question,
> > this is the time between the DSI link being powered up and the video
> > stream start)
> > 3. when the DSI link is in HS state (while streaming the video).
>
> It's not clear to me what (2) is. What is the state of the clock and
> data lanes?

Clk an Data0 should be in the LP mode, ready for LP Data Transfer.

I don't think we support ULPS currently.


>
> I'm facing similar issues with the tc358775 bridge. This bridge needs
> to release its reset while both clock and data lanes are in LP-11 mode.
> But then it needs to be configured (via I2C) while the clock lane is
> in enabled (HS mode), but the data lanes are still in LP-11 mode.
>
> To me it looks like there is a fouth case then:
> 1. unpowered
> 2. DSI clock and data are in LP-11
> 3. DSI clock is in HS and data are in LP-11
> 4. DSI clock is in HS and data is in HS
>
> (And of course the bridge needs continuous clock mode).
>
> > Different DSI bridges have different requirements with respect to the
> > code being executed at stages 1 and 2. For example several DSI-to-eDP
> > bridges (ps8640, tc358767 require for the link to be quiet during
> > reset time.
> > The DSI-controlled bridges and DSI panels need to send some commands
> > in stage 2, before starting up video
> >
> > In the DRM subsystem stage 3 naturally maps to the
> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
> > the DRM call chain.
> > Earlier we attempted to solve that using the pre_enable_prev_first,
> > which remapped pre-enable callback execution order. However it has led
> > us to the two issues. First, at the DSI host driver we do not know
> > whether the panel / bridge were updated to use pre_enable_prev_first
> > or not. Second, if the bridge has to perform steps during both stages
> > 1 and 2, it can not do that.
> >
> > I'm trying to find a way to express the difference between stages 1
> > and 2 in the generic code, so that we do not to worry about particular
> > DSI host and DSI bridge / panel peculiarities when implementing the
> > DSI host and/or DSI panel driver.
>
> For now, I have a rather hacky ".dsi_lp11_notify" callback in
> drm_bridge_funcs which is supposed to be called by the DSI host while the
> clock and data lanes are in LP-11 mode. But that is rather an RFC and me
> needing something to get the driver for this bridge working. Because it's
> badly broken. FWIW, you can find my work-in-progress patches at
> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
>
> -michael
>


--
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Michael Walle]

>> > DSI device lifetime has three different stages:
>> > 1. before the DSI link being powered up and clocking,
>> > 2. when the DSI link is in LP state (for the purpose of this question,
>> > this is the time between the DSI link being powered up and the video
>> > stream start)
>> > 3. when the DSI link is in HS state (while streaming the video).
>> 
>> It's not clear to me what (2) is. What is the state of the clock and
>> data lanes?
> 
> Clk an Data0 should be in the LP mode, ready for LP Data Transfer.

Then this is somehow missing
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

   A DSI host should keep the PHY powered down until the pre_enable 
operation
   is called. All lanes are in an undefined idle state up to this point, 
and
   it must not be assumed that it is LP-11. pre_enable should initialise 
the
   PHY, set the data lanes to LP-11, and the clock lane to either LP-11 
or HS
   depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.

So I don't think these three states are sufficient, see below, that 
there
should be at least four.

-michael

> 
> I don't think we support ULPS currently.
> 
> 
>> 
>> I'm facing similar issues with the tc358775 bridge. This bridge needs
>> to release its reset while both clock and data lanes are in LP-11 
>> mode.
>> But then it needs to be configured (via I2C) while the clock lane is
>> in enabled (HS mode), but the data lanes are still in LP-11 mode.
>> 
>> To me it looks like there is a fouth case then:
>> 1. unpowered
>> 2. DSI clock and data are in LP-11
>> 3. DSI clock is in HS and data are in LP-11
>> 4. DSI clock is in HS and data is in HS
>> 
>> (And of course the bridge needs continuous clock mode).
>> 
>> > Different DSI bridges have different requirements with respect to the
>> > code being executed at stages 1 and 2. For example several DSI-to-eDP
>> > bridges (ps8640, tc358767 require for the link to be quiet during
>> > reset time.
>> > The DSI-controlled bridges and DSI panels need to send some commands
>> > in stage 2, before starting up video
>> >
>> > In the DRM subsystem stage 3 naturally maps to the
>> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
>> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
>> > the DRM call chain.
>> > Earlier we attempted to solve that using the pre_enable_prev_first,
>> > which remapped pre-enable callback execution order. However it has led
>> > us to the two issues. First, at the DSI host driver we do not know
>> > whether the panel / bridge were updated to use pre_enable_prev_first
>> > or not. Second, if the bridge has to perform steps during both stages
>> > 1 and 2, it can not do that.
>> >
>> > I'm trying to find a way to express the difference between stages 1
>> > and 2 in the generic code, so that we do not to worry about particular
>> > DSI host and DSI bridge / panel peculiarities when implementing the
>> > DSI host and/or DSI panel driver.
>> 
>> For now, I have a rather hacky ".dsi_lp11_notify" callback in
>> drm_bridge_funcs which is supposed to be called by the DSI host while 
>> the
>> clock and data lanes are in LP-11 mode. But that is rather an RFC and 
>> me
>> needing something to get the driver for this bridge working. Because 
>> it's
>> badly broken. FWIW, you can find my work-in-progress patches at
>> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
>> 
>> -michael
>> 
> 
> 
> --
> With best wishes
> Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Tue, 28 Nov 2023 at 18:56, Michael Walle <mwalle@kernel.org> wrote:
>
> >> > DSI device lifetime has three different stages:
> >> > 1. before the DSI link being powered up and clocking,
> >> > 2. when the DSI link is in LP state (for the purpose of this question,
> >> > this is the time between the DSI link being powered up and the video
> >> > stream start)
> >> > 3. when the DSI link is in HS state (while streaming the video).
> >>
> >> It's not clear to me what (2) is. What is the state of the clock and
> >> data lanes?
> >
> > Clk an Data0 should be in the LP mode, ready for LP Data Transfer.
>
> Then this is somehow missing
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>
>    A DSI host should keep the PHY powered down until the pre_enable
> operation
>    is called. All lanes are in an undefined idle state up to this point,
> and
>    it must not be assumed that it is LP-11. pre_enable should initialise
> the
>    PHY, set the data lanes to LP-11, and the clock lane to either LP-11
> or HS
>    depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.
>
> So I don't think these three states are sufficient, see below, that
> there
> should be at least four.

Which one is #4?

>
> -michael
>
> >
> > I don't think we support ULPS currently.
> >
> >
> >>
> >> I'm facing similar issues with the tc358775 bridge. This bridge needs
> >> to release its reset while both clock and data lanes are in LP-11
> >> mode.
> >> But then it needs to be configured (via I2C) while the clock lane is
> >> in enabled (HS mode), but the data lanes are still in LP-11 mode.
> >>
> >> To me it looks like there is a fouth case then:
> >> 1. unpowered
> >> 2. DSI clock and data are in LP-11
> >> 3. DSI clock is in HS and data are in LP-11
> >> 4. DSI clock is in HS and data is in HS
> >>
> >> (And of course the bridge needs continuous clock mode).
> >>
> >> > Different DSI bridges have different requirements with respect to the
> >> > code being executed at stages 1 and 2. For example several DSI-to-eDP
> >> > bridges (ps8640, tc358767 require for the link to be quiet during
> >> > reset time.
> >> > The DSI-controlled bridges and DSI panels need to send some commands
> >> > in stage 2, before starting up video
> >> >
> >> > In the DRM subsystem stage 3 naturally maps to the
> >> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
> >> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
> >> > the DRM call chain.
> >> > Earlier we attempted to solve that using the pre_enable_prev_first,
> >> > which remapped pre-enable callback execution order. However it has led
> >> > us to the two issues. First, at the DSI host driver we do not know
> >> > whether the panel / bridge were updated to use pre_enable_prev_first
> >> > or not. Second, if the bridge has to perform steps during both stages
> >> > 1 and 2, it can not do that.
> >> >
> >> > I'm trying to find a way to express the difference between stages 1
> >> > and 2 in the generic code, so that we do not to worry about particular
> >> > DSI host and DSI bridge / panel peculiarities when implementing the
> >> > DSI host and/or DSI panel driver.
> >>
> >> For now, I have a rather hacky ".dsi_lp11_notify" callback in
> >> drm_bridge_funcs which is supposed to be called by the DSI host while
> >> the
> >> clock and data lanes are in LP-11 mode. But that is rather an RFC and
> >> me
> >> needing something to get the driver for this bridge working. Because
> >> it's
> >> badly broken. FWIW, you can find my work-in-progress patches at
> >> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
> >>
> >> -michael
> >>
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Michael Walle]

>> >> > DSI device lifetime has three different stages:
>> >> > 1. before the DSI link being powered up and clocking,
>> >> > 2. when the DSI link is in LP state (for the purpose of this question,
>> >> > this is the time between the DSI link being powered up and the video
>> >> > stream start)
>> >> > 3. when the DSI link is in HS state (while streaming the video).
>> >>
>> >> It's not clear to me what (2) is. What is the state of the clock and
>> >> data lanes?
>> >
>> > Clk an Data0 should be in the LP mode, ready for LP Data Transfer.
>>
>> Then this is somehow missing
>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>
>>    A DSI host should keep the PHY powered down until the pre_enable
>> operation
>>    is called. All lanes are in an undefined idle state up to this point,
>> and
>>    it must not be assumed that it is LP-11. pre_enable should initialise
>> the
>>    PHY, set the data lanes to LP-11, and the clock lane to either LP-11
>> or HS
>>    depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.
>>
>> So I don't think these three states are sufficient, see below, that
>> there
>> should be at least four.
>
>Which one is #4?

enabled clock lane (HS mode), data lanes in LP-11

-michael

>>
>> >
>> > I don't think we support ULPS currently.
>> >
>> >
>> >>
>> >> I'm facing similar issues with the tc358775 bridge. This bridge needs
>> >> to release its reset while both clock and data lanes are in LP-11
>> >> mode.
>> >> But then it needs to be configured (via I2C) while the clock lane is
>> >> in enabled (HS mode), but the data lanes are still in LP-11 mode.
>> >>
>> >> To me it looks like there is a fouth case then:
>> >> 1. unpowered
>> >> 2. DSI clock and data are in LP-11
>> >> 3. DSI clock is in HS and data are in LP-11
>> >> 4. DSI clock is in HS and data is in HS
>> >>
>> >> (And of course the bridge needs continuous clock mode).
>> >>
>> >> > Different DSI bridges have different requirements with respect to the
>> >> > code being executed at stages 1 and 2. For example several DSI-to-eDP
>> >> > bridges (ps8640, tc358767 require for the link to be quiet during
>> >> > reset time.
>> >> > The DSI-controlled bridges and DSI panels need to send some commands
>> >> > in stage 2, before starting up video
>> >> >
>> >> > In the DRM subsystem stage 3 naturally maps to the
>> >> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
>> >> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
>> >> > the DRM call chain.
>> >> > Earlier we attempted to solve that using the pre_enable_prev_first,
>> >> > which remapped pre-enable callback execution order. However it has led
>> >> > us to the two issues. First, at the DSI host driver we do not know
>> >> > whether the panel / bridge were updated to use pre_enable_prev_first
>> >> > or not. Second, if the bridge has to perform steps during both stages
>> >> > 1 and 2, it can not do that.
>> >> >
>> >> > I'm trying to find a way to express the difference between stages 1
>> >> > and 2 in the generic code, so that we do not to worry about particular
>> >> > DSI host and DSI bridge / panel peculiarities when implementing the
>> >> > DSI host and/or DSI panel driver.
>> >>
>> >> For now, I have a rather hacky ".dsi_lp11_notify" callback in
>> >> drm_bridge_funcs which is supposed to be called by the DSI host while
>> >> the
>> >> clock and data lanes are in LP-11 mode. But that is rather an RFC and
>> >> me
>> >> needing something to get the driver for this bridge working. Because
>> >> it's
>> >> badly broken. FWIW, you can find my work-in-progress patches at
>> >> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
>> >>
>> >> -michael
>> >>
>> >
>> >
>> > --
>> > With best wishes
>> > Dmitry
>
>
>

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Tue, 28 Nov 2023 at 21:50, Michael Walle <michael@walle.cc> wrote:
>
> >> >> > DSI device lifetime has three different stages:
> >> >> > 1. before the DSI link being powered up and clocking,
> >> >> > 2. when the DSI link is in LP state (for the purpose of this question,
> >> >> > this is the time between the DSI link being powered up and the video
> >> >> > stream start)
> >> >> > 3. when the DSI link is in HS state (while streaming the video).
> >> >>
> >> >> It's not clear to me what (2) is. What is the state of the clock and
> >> >> data lanes?
> >> >
> >> > Clk an Data0 should be in the LP mode, ready for LP Data Transfer.
> >>
> >> Then this is somehow missing
> >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >>
> >>    A DSI host should keep the PHY powered down until the pre_enable
> >> operation
> >>    is called. All lanes are in an undefined idle state up to this point,
> >> and
> >>    it must not be assumed that it is LP-11. pre_enable should initialise
> >> the
> >>    PHY, set the data lanes to LP-11, and the clock lane to either LP-11
> >> or HS
> >>    depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.
> >>
> >> So I don't think these three states are sufficient, see below, that
> >> there
> >> should be at least four.
> >
> >Which one is #4?
>
> enabled clock lane (HS mode), data lanes in LP-11

What is the purpose of such a mode?

>
> -michael
>
> >>
> >> >
> >> > I don't think we support ULPS currently.
> >> >
> >> >
> >> >>
> >> >> I'm facing similar issues with the tc358775 bridge. This bridge needs
> >> >> to release its reset while both clock and data lanes are in LP-11
> >> >> mode.
> >> >> But then it needs to be configured (via I2C) while the clock lane is
> >> >> in enabled (HS mode), but the data lanes are still in LP-11 mode.
> >> >>
> >> >> To me it looks like there is a fouth case then:
> >> >> 1. unpowered
> >> >> 2. DSI clock and data are in LP-11
> >> >> 3. DSI clock is in HS and data are in LP-11
> >> >> 4. DSI clock is in HS and data is in HS
> >> >>
> >> >> (And of course the bridge needs continuous clock mode).
> >> >>
> >> >> > Different DSI bridges have different requirements with respect to the
> >> >> > code being executed at stages 1 and 2. For example several DSI-to-eDP
> >> >> > bridges (ps8640, tc358767 require for the link to be quiet during
> >> >> > reset time.
> >> >> > The DSI-controlled bridges and DSI panels need to send some commands
> >> >> > in stage 2, before starting up video
> >> >> >
> >> >> > In the DRM subsystem stage 3 naturally maps to the
> >> >> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
> >> >> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
> >> >> > the DRM call chain.
> >> >> > Earlier we attempted to solve that using the pre_enable_prev_first,
> >> >> > which remapped pre-enable callback execution order. However it has led
> >> >> > us to the two issues. First, at the DSI host driver we do not know
> >> >> > whether the panel / bridge were updated to use pre_enable_prev_first
> >> >> > or not. Second, if the bridge has to perform steps during both stages
> >> >> > 1 and 2, it can not do that.
> >> >> >
> >> >> > I'm trying to find a way to express the difference between stages 1
> >> >> > and 2 in the generic code, so that we do not to worry about particular
> >> >> > DSI host and DSI bridge / panel peculiarities when implementing the
> >> >> > DSI host and/or DSI panel driver.
> >> >>
> >> >> For now, I have a rather hacky ".dsi_lp11_notify" callback in
> >> >> drm_bridge_funcs which is supposed to be called by the DSI host while
> >> >> the
> >> >> clock and data lanes are in LP-11 mode. But that is rather an RFC and
> >> >> me
> >> >> needing something to get the driver for this bridge working. Because
> >> >> it's
> >> >> badly broken. FWIW, you can find my work-in-progress patches at
> >> >> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
> >> >>
> >> >> -michael
> >> >>
> >> >
> >> >
> >> > --
> >> > With best wishes
> >> > Dmitry
> >
> >
> >
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Michael Walle]

>> >> >> > DSI device lifetime has three different stages:
>> >> >> > 1. before the DSI link being powered up and clocking,
>> >> >> > 2. when the DSI link is in LP state (for the purpose of this question,
>> >> >> > this is the time between the DSI link being powered up and the video
>> >> >> > stream start)
>> >> >> > 3. when the DSI link is in HS state (while streaming the video).
>> >> >>
>> >> >> It's not clear to me what (2) is. What is the state of the clock and
>> >> >> data lanes?
>> >> >
>> >> > Clk an Data0 should be in the LP mode, ready for LP Data Transfer.
>> >>
>> >> Then this is somehow missing
>> >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>> >>
>> >>    A DSI host should keep the PHY powered down until the pre_enable
>> >> operation
>> >>    is called. All lanes are in an undefined idle state up to this point,
>> >> and
>> >>    it must not be assumed that it is LP-11. pre_enable should initialise
>> >> the
>> >>    PHY, set the data lanes to LP-11, and the clock lane to either LP-11
>> >> or HS
>> >>    depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.
>> >>
>> >> So I don't think these three states are sufficient, see below, that
>> >> there
>> >> should be at least four.
>> >
>> >Which one is #4?
>> 
>> enabled clock lane (HS mode), data lanes in LP-11
> 
> What is the purpose of such a mode?

To repeat my first mail:

I'm facing similar issues with the tc358775 bridge. This bridge needs
to release its reset while both clock and data lanes are in LP-11
mode.
But then it needs to be configured (via I2C) while the clock lane is
in enabled (HS mode), but the data lanes are still in LP-11 mode.

Therefore, for the correct init sequence is:
(1) dsi host enables lanes, that is clock and data are in lp-11
(2) dsi bridge driver releases reset of the bridge
(3) dsi host enables clock lane, leaves data lanes in lp-11
(4) dsi bridge driver configures the bridge
(5) dsi host enables the video stream
(6) dsi bridge enables the output port of the bridge

-michael

>> >> > I don't think we support ULPS currently.
>> >> >
>> >> >
>> >> >>
>> >> >> I'm facing similar issues with the tc358775 bridge. This bridge needs
>> >> >> to release its reset while both clock and data lanes are in LP-11
>> >> >> mode.
>> >> >> But then it needs to be configured (via I2C) while the clock lane is
>> >> >> in enabled (HS mode), but the data lanes are still in LP-11 mode.
>> >> >>
>> >> >> To me it looks like there is a fouth case then:
>> >> >> 1. unpowered
>> >> >> 2. DSI clock and data are in LP-11
>> >> >> 3. DSI clock is in HS and data are in LP-11
>> >> >> 4. DSI clock is in HS and data is in HS
>> >> >>
>> >> >> (And of course the bridge needs continuous clock mode).
>> >> >>
>> >> >> > Different DSI bridges have different requirements with respect to the
>> >> >> > code being executed at stages 1 and 2. For example several DSI-to-eDP
>> >> >> > bridges (ps8640, tc358767 require for the link to be quiet during
>> >> >> > reset time.
>> >> >> > The DSI-controlled bridges and DSI panels need to send some commands
>> >> >> > in stage 2, before starting up video
>> >> >> >
>> >> >> > In the DRM subsystem stage 3 naturally maps to the
>> >> >> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
>> >> >> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
>> >> >> > the DRM call chain.
>> >> >> > Earlier we attempted to solve that using the pre_enable_prev_first,
>> >> >> > which remapped pre-enable callback execution order. However it has led
>> >> >> > us to the two issues. First, at the DSI host driver we do not know
>> >> >> > whether the panel / bridge were updated to use pre_enable_prev_first
>> >> >> > or not. Second, if the bridge has to perform steps during both stages
>> >> >> > 1 and 2, it can not do that.
>> >> >> >
>> >> >> > I'm trying to find a way to express the difference between stages 1
>> >> >> > and 2 in the generic code, so that we do not to worry about particular
>> >> >> > DSI host and DSI bridge / panel peculiarities when implementing the
>> >> >> > DSI host and/or DSI panel driver.
>> >> >>
>> >> >> For now, I have a rather hacky ".dsi_lp11_notify" callback in
>> >> >> drm_bridge_funcs which is supposed to be called by the DSI host while
>> >> >> the
>> >> >> clock and data lanes are in LP-11 mode. But that is rather an RFC and
>> >> >> me
>> >> >> needing something to get the driver for this bridge working. Because
>> >> >> it's
>> >> >> badly broken. FWIW, you can find my work-in-progress patches at
>> >> >> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
>> >> >>
>> >> >> -michael
>> >> >>
>> >> >
>> >> >
>> >> > --
>> >> > With best wishes
>> >> > Dmitry
>> >
>> >
>> >
>> 

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Michael Walle]

[sorry I fat fingered my former reply and converted all CCs to BCCs..]

>> >> >> > DSI device lifetime has three different stages:
>> >> >> > 1. before the DSI link being powered up and clocking,
>> >> >> > 2. when the DSI link is in LP state (for the purpose of this question,
>> >> >> > this is the time between the DSI link being powered up and the video
>> >> >> > stream start)
>> >> >> > 3. when the DSI link is in HS state (while streaming the video).
>> >> >>
>> >> >> It's not clear to me what (2) is. What is the state of the clock and
>> >> >> data lanes?
>> >> >
>> >> > Clk an Data0 should be in the LP mode, ready for LP Data Transfer.
>> >>
>> >> Then this is somehow missing
>> >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>> >>
>> >>    A DSI host should keep the PHY powered down until the pre_enable
>> >> operation
>> >>    is called. All lanes are in an undefined idle state up to this point,
>> >> and
>> >>    it must not be assumed that it is LP-11. pre_enable should initialise
>> >> the
>> >>    PHY, set the data lanes to LP-11, and the clock lane to either LP-11
>> >> or HS
>> >>    depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.
>> >>
>> >> So I don't think these three states are sufficient, see below, that
>> >> there
>> >> should be at least four.
>> >
>> >Which one is #4?
>> 
>> enabled clock lane (HS mode), data lanes in LP-11
> 
> What is the purpose of such a mode?

To repeat my first mail:

I'm facing similar issues with the tc358775 bridge. This bridge needs
to release its reset while both clock and data lanes are in LP-11
mode.
But then it needs to be configured (via I2C) while the clock lane is
in enabled (HS mode), but the data lanes are still in LP-11 mode.

Therefore, for the correct init sequence is:
(1) dsi host enables lanes, that is clock and data are in lp-11
(2) dsi bridge driver releases reset of the bridge
(3) dsi host enables clock lane, leaves data lanes in lp-11
(4) dsi bridge driver configures the bridge
(5) dsi host enables the video stream
(6) dsi bridge enables the output port of the bridge

-michael

>> >> > I don't think we support ULPS currently.
>> >> >
>> >> >
>> >> >>
>> >> >> I'm facing similar issues with the tc358775 bridge. This bridge needs
>> >> >> to release its reset while both clock and data lanes are in LP-11
>> >> >> mode.
>> >> >> But then it needs to be configured (via I2C) while the clock lane is
>> >> >> in enabled (HS mode), but the data lanes are still in LP-11 mode.
>> >> >>
>> >> >> To me it looks like there is a fouth case then:
>> >> >> 1. unpowered
>> >> >> 2. DSI clock and data are in LP-11
>> >> >> 3. DSI clock is in HS and data are in LP-11
>> >> >> 4. DSI clock is in HS and data is in HS
>> >> >>
>> >> >> (And of course the bridge needs continuous clock mode).
>> >> >>
>> >> >> > Different DSI bridges have different requirements with respect to the
>> >> >> > code being executed at stages 1 and 2. For example several DSI-to-eDP
>> >> >> > bridges (ps8640, tc358767 require for the link to be quiet during
>> >> >> > reset time.
>> >> >> > The DSI-controlled bridges and DSI panels need to send some commands
>> >> >> > in stage 2, before starting up video
>> >> >> >
>> >> >> > In the DRM subsystem stage 3 naturally maps to the
>> >> >> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
>> >> >> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
>> >> >> > the DRM call chain.
>> >> >> > Earlier we attempted to solve that using the pre_enable_prev_first,
>> >> >> > which remapped pre-enable callback execution order. However it has led
>> >> >> > us to the two issues. First, at the DSI host driver we do not know
>> >> >> > whether the panel / bridge were updated to use pre_enable_prev_first
>> >> >> > or not. Second, if the bridge has to perform steps during both stages
>> >> >> > 1 and 2, it can not do that.
>> >> >> >
>> >> >> > I'm trying to find a way to express the difference between stages 1
>> >> >> > and 2 in the generic code, so that we do not to worry about particular
>> >> >> > DSI host and DSI bridge / panel peculiarities when implementing the
>> >> >> > DSI host and/or DSI panel driver.
>> >> >>
>> >> >> For now, I have a rather hacky ".dsi_lp11_notify" callback in
>> >> >> drm_bridge_funcs which is supposed to be called by the DSI host while
>> >> >> the
>> >> >> clock and data lanes are in LP-11 mode. But that is rather an RFC and
>> >> >> me
>> >> >> needing something to get the driver for this bridge working. Because
>> >> >> it's
>> >> >> badly broken. FWIW, you can find my work-in-progress patches at
>> >> >> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
>> >> >>
>> >> >> -michael
>> >> >>
>> >> >
>> >> >
>> >> > --
>> >> > With best wishes
>> >> > Dmitry
>> >
>> >
>> >
>> 

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Wed, 29 Nov 2023 at 00:20, Michael Walle <michael@walle.cc> wrote:
>
> [sorry I fat fingered my former reply and converted all CCs to BCCs..]
>
> >> >> >> > DSI device lifetime has three different stages:
> >> >> >> > 1. before the DSI link being powered up and clocking,
> >> >> >> > 2. when the DSI link is in LP state (for the purpose of this question,
> >> >> >> > this is the time between the DSI link being powered up and the video
> >> >> >> > stream start)
> >> >> >> > 3. when the DSI link is in HS state (while streaming the video).
> >> >> >>
> >> >> >> It's not clear to me what (2) is. What is the state of the clock and
> >> >> >> data lanes?
> >> >> >
> >> >> > Clk an Data0 should be in the LP mode, ready for LP Data Transfer.
> >> >>
> >> >> Then this is somehow missing
> >> >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >> >>
> >> >>    A DSI host should keep the PHY powered down until the pre_enable
> >> >> operation
> >> >>    is called. All lanes are in an undefined idle state up to this point,
> >> >> and
> >> >>    it must not be assumed that it is LP-11. pre_enable should initialise
> >> >> the
> >> >>    PHY, set the data lanes to LP-11, and the clock lane to either LP-11
> >> >> or HS
> >> >>    depending on the mode_flag MIPI_DSI_CLOCK_NON_CONTINUOUS.
> >> >>
> >> >> So I don't think these three states are sufficient, see below, that
> >> >> there
> >> >> should be at least four.
> >> >
> >> >Which one is #4?
> >>
> >> enabled clock lane (HS mode), data lanes in LP-11
> >
> > What is the purpose of such a mode?
>
> To repeat my first mail:

Excuse me please, I either missed it, or forgot it.

>
> I'm facing similar issues with the tc358775 bridge. This bridge needs
> to release its reset while both clock and data lanes are in LP-11
> mode.
> But then it needs to be configured (via I2C) while the clock lane is
> in enabled (HS mode), but the data lanes are still in LP-11 mode.

This is quite an interesting requirement. For example, I'm not 100%
sure whether we can get that done on our (msm) hosts. I need to double
check that.
What frequency is expected on the CLK lane? Can it be an arbitrary
frequency or it should be the same freq as the one used later for the
video transfer?

>
> Therefore, for the correct init sequence is:
> (1) dsi host enables lanes, that is clock and data are in lp-11
> (2) dsi bridge driver releases reset of the bridge
> (3) dsi host enables clock lane, leaves data lanes in lp-11
> (4) dsi bridge driver configures the bridge
> (5) dsi host enables the video stream
> (6) dsi bridge enables the output port of the bridge
>
> -michael
>
> >> >> > I don't think we support ULPS currently.
> >> >> >
> >> >> >
> >> >> >>
> >> >> >> I'm facing similar issues with the tc358775 bridge. This bridge needs
> >> >> >> to release its reset while both clock and data lanes are in LP-11
> >> >> >> mode.
> >> >> >> But then it needs to be configured (via I2C) while the clock lane is
> >> >> >> in enabled (HS mode), but the data lanes are still in LP-11 mode.
> >> >> >>
> >> >> >> To me it looks like there is a fouth case then:
> >> >> >> 1. unpowered
> >> >> >> 2. DSI clock and data are in LP-11
> >> >> >> 3. DSI clock is in HS and data are in LP-11
> >> >> >> 4. DSI clock is in HS and data is in HS
> >> >> >>
> >> >> >> (And of course the bridge needs continuous clock mode).
> >> >> >>
> >> >> >> > Different DSI bridges have different requirements with respect to the
> >> >> >> > code being executed at stages 1 and 2. For example several DSI-to-eDP
> >> >> >> > bridges (ps8640, tc358767 require for the link to be quiet during
> >> >> >> > reset time.
> >> >> >> > The DSI-controlled bridges and DSI panels need to send some commands
> >> >> >> > in stage 2, before starting up video
> >> >> >> >
> >> >> >> > In the DRM subsystem stage 3 naturally maps to the
> >> >> >> > drm_bridge_funcs::enable, stage 1 also naturally maps to the
> >> >> >> > drm_bridge_funcs::pre_enable. Stage 2 doesn't have its own place in
> >> >> >> > the DRM call chain.
> >> >> >> > Earlier we attempted to solve that using the pre_enable_prev_first,
> >> >> >> > which remapped pre-enable callback execution order. However it has led
> >> >> >> > us to the two issues. First, at the DSI host driver we do not know
> >> >> >> > whether the panel / bridge were updated to use pre_enable_prev_first
> >> >> >> > or not. Second, if the bridge has to perform steps during both stages
> >> >> >> > 1 and 2, it can not do that.
> >> >> >> >
> >> >> >> > I'm trying to find a way to express the difference between stages 1
> >> >> >> > and 2 in the generic code, so that we do not to worry about particular
> >> >> >> > DSI host and DSI bridge / panel peculiarities when implementing the
> >> >> >> > DSI host and/or DSI panel driver.
> >> >> >>
> >> >> >> For now, I have a rather hacky ".dsi_lp11_notify" callback in
> >> >> >> drm_bridge_funcs which is supposed to be called by the DSI host while
> >> >> >> the
> >> >> >> clock and data lanes are in LP-11 mode. But that is rather an RFC and
> >> >> >> me
> >> >> >> needing something to get the driver for this bridge working. Because
> >> >> >> it's
> >> >> >> badly broken. FWIW, you can find my work-in-progress patches at
> >> >> >> https://github.com/mwalle/linux/tree/feature-tc358775-fixes
> >> >> >>
> >> >> >> -michael
> >> >> >>
> >> >> >
> >> >> >
> >> >> > --
> >> >> > With best wishes
> >> >> > Dmitry
> >> >
> >> >
> >> >
> >>



-- 
With best wishes
Dmitry

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Michael Walle]

>> I'm facing similar issues with the tc358775 bridge. This bridge needs
>> to release its reset while both clock and data lanes are in LP-11
>> mode.
>> But then it needs to be configured (via I2C) while the clock lane is
>> in enabled (HS mode), but the data lanes are still in LP-11 mode.
> 
> This is quite an interesting requirement. For example, I'm not 100%
> sure whether we can get that done on our (msm) hosts. I need to double
> check that.
> What frequency is expected on the CLK lane? Can it be an arbitrary
> frequency or it should be the same freq as the one used later for the
> video transfer?

I presume it has to be the same frequency as the video stream later.
That's a least what I have successfully tested.
The datasheet doesn't mention if a frequency switch is allowed on the
clock lane (which would need a brief switch to LP mode, I presume). I'd 
say
it's not allowed/supported as the bridge is very picky regarding the 
init
sequence in general.

I'm using the Mediatek DSI host, where that sequence is possible. I.e. 
you
just enable the clock and data lanes in continuous clock mode, but don't
enable the video stream, which should leave the data lanes in LP-11 
mode.

Sometimes you also have a command mode (instead of a video mode). And if
you don't send any commands, the data lanes are in LP-11 mode, too.

-michael

>> Therefore, for the correct init sequence is:
>> (1) dsi host enables lanes, that is clock and data are in lp-11
>> (2) dsi bridge driver releases reset of the bridge
>> (3) dsi host enables clock lane, leaves data lanes in lp-11
>> (4) dsi bridge driver configures the bridge
>> (5) dsi host enables the video stream
>> (6) dsi bridge enables the output port of the bridge

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Neil Armstrong]

On 16/10/2023 18:53, Dmitry Baryshkov wrote:
> The MIPI DSI links do not fully fall into the DRM callbacks model. The
> drm_bridge_funcs abstraction. Instead of having just two states (off and
> on) the DSI hosts have separate LP-11 state. In this state the host is
> on, but the video stream is not yet enabled.
> 
> Introduce API that allows DSI bridges / panels to control the DSI host
> power up state.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
>   include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
>   2 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 14201f73aab1..c467162cb7d8 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
>   
> +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
> +{
> +	const struct mipi_dsi_host_ops *ops = host->ops;
> +
> +	return ops && ops->power_up;
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> +
> +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> +{
> +	const struct mipi_dsi_host_ops *ops = host->ops;
> +
> +	if (!mipi_dsi_host_power_control_available(host))
> +		return -EOPNOTSUPP;
> +
> +	return ops->power_up ? ops->power_up(host) : 0;
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> +
> +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> +{
> +	const struct mipi_dsi_host_ops *ops = host->ops;
> +
> +	if (!mipi_dsi_host_power_control_available(host))
> +		return;
> +
> +	if (ops->power_down)
> +		ops->power_down(host);
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> +
>   static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi,
>   					struct mipi_dsi_msg *msg)
>   {
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 167742e579e3..e503c3e4d057 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -68,6 +68,8 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
>    * @attach: attach DSI device to DSI host
>    * @detach: detach DSI device from DSI host
>    * @transfer: transmit a DSI packet
> + * @power_up: enable DSI link and bring it to the LP-11 state
> + * @power_down: fully disable DSI link
>    *
>    * DSI packets transmitted by .transfer() are passed in as mipi_dsi_msg
>    * structures. This structure contains information about the type of packet
> @@ -81,10 +83,18 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
>    * function will seldomly return anything other than the number of bytes
>    * contained in the transmit buffer on success.
>    *
> - * Also note that those callbacks can be called no matter the state the
> - * host is in. Drivers that need the underlying device to be powered to
> - * perform these operations will first need to make sure it's been
> - * properly enabled.
> + * Note: currently there are two modes of DSI power control. Legacy drivers
> + * will call those callbacks no matter the state the host is in. DSI host
> + * drivers that need the underlying device to be powered to perform these
> + * operations will first need to make sure it's been properly enabled.
> + *
> + * Newer drivers will set the @MIPI_DSI_MANUAL_POWERUP flag to indicate that
> + * they will call @mipi_dsi_power_up() and @mipi_dsi_power_down() to control
> + * the link state of the DSI host or they will set @MIPI_DSI_AUTO_POWERUP to
> + * indicate that the driver is fine with the link being powered up in DSI
> + * host's (atomic_)pre_enable() callback and then being disabled in the
> + * (atomic_)post_disable() callback. The transfer callback must only be called
> + * if the DSI host has been powered up and was not brought down.
>    *
>    * Note: some hosts (sunxi) can not send LP commands between HS video
>    * packets. Thus all DSI transfers sent in LP mode should be limited to the
> @@ -97,6 +107,8 @@ struct mipi_dsi_host_ops {
>   		      struct mipi_dsi_device *dsi);
>   	ssize_t (*transfer)(struct mipi_dsi_host *host,
>   			    const struct mipi_dsi_msg *msg);
> +	int (*power_up)(struct mipi_dsi_host *host);
> +	void (*power_down)(struct mipi_dsi_host *host);
>   };
>   
>   /**
> @@ -143,6 +155,10 @@ struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
>   #define MIPI_DSI_MODE_LPM		BIT(11)
>   /* transmit data ending at the same time for all lanes within one hsync */
>   #define MIPI_DSI_HS_PKT_END_ALIGNED	BIT(12)
> +/* DSI peripheral driver manually controls DSI link powerup */
> +#define MIPI_DSI_MANUAL_POWERUP		BIT(13)
> +/* DSI peripheral driver is fine with automatic DSI link power control */
> +#define MIPI_DSI_AUTO_POWERUP		BIT(14)

What happens if none of the bits are in the flags ?

Can't we implement "opportunistic power-up" on the first DSI command sent?

If a bridge/panel sends a DSI command, and if it happens before the DSI host enable, then
the DSI host will "pre-enable" the host and put the link in LP-11.

This would be simpler and would work whatever the pre_enable order.

But this won't work for the tc358767, except if we add a dummy DSI host command
which powers up the DSI link.

This won't fix the PS8640 either who also needs a disabled DSI link to initialize.

Neil

>   
>   enum mipi_dsi_pixel_format {
>   	MIPI_DSI_FMT_RGB888,
> @@ -235,6 +251,11 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
>   struct mipi_dsi_device *
>   devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *host,
>   				   const struct mipi_dsi_device_info *info);
> +
> +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host);
> +int mipi_dsi_host_power_up(struct mipi_dsi_host *host);
> +void mipi_dsi_host_power_down(struct mipi_dsi_host *host);
> +
>   struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
>   int mipi_dsi_attach(struct mipi_dsi_device *dsi);
>   int mipi_dsi_detach(struct mipi_dsi_device *dsi);

Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On Mon, 23 Oct 2023 at 10:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 16/10/2023 18:53, Dmitry Baryshkov wrote:
> > The MIPI DSI links do not fully fall into the DRM callbacks model. The
> > drm_bridge_funcs abstraction. Instead of having just two states (off and
> > on) the DSI hosts have separate LP-11 state. In this state the host is
> > on, but the video stream is not yet enabled.
> >
> > Introduce API that allows DSI bridges / panels to control the DSI host
> > power up state.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/drm_mipi_dsi.c | 31 +++++++++++++++++++++++++++++++
> >   include/drm/drm_mipi_dsi.h     | 29 +++++++++++++++++++++++++----
> >   2 files changed, 56 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index 14201f73aab1..c467162cb7d8 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -428,6 +428,37 @@ int devm_mipi_dsi_attach(struct device *dev,
> >   }
> >   EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
> >
> > +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host)
> > +{
> > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > +
> > +     return ops && ops->power_up;
> > +}
> > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_control_available);
> > +
> > +int mipi_dsi_host_power_up(struct mipi_dsi_host *host)
> > +{
> > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > +
> > +     if (!mipi_dsi_host_power_control_available(host))
> > +             return -EOPNOTSUPP;
> > +
> > +     return ops->power_up ? ops->power_up(host) : 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_up);
> > +
> > +void mipi_dsi_host_power_down(struct mipi_dsi_host *host)
> > +{
> > +     const struct mipi_dsi_host_ops *ops = host->ops;
> > +
> > +     if (!mipi_dsi_host_power_control_available(host))
> > +             return;
> > +
> > +     if (ops->power_down)
> > +             ops->power_down(host);
> > +}
> > +EXPORT_SYMBOL_GPL(mipi_dsi_host_power_down);
> > +
> >   static ssize_t mipi_dsi_device_transfer(struct mipi_dsi_device *dsi,
> >                                       struct mipi_dsi_msg *msg)
> >   {
> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > index 167742e579e3..e503c3e4d057 100644
> > --- a/include/drm/drm_mipi_dsi.h
> > +++ b/include/drm/drm_mipi_dsi.h
> > @@ -68,6 +68,8 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> >    * @attach: attach DSI device to DSI host
> >    * @detach: detach DSI device from DSI host
> >    * @transfer: transmit a DSI packet
> > + * @power_up: enable DSI link and bring it to the LP-11 state
> > + * @power_down: fully disable DSI link
> >    *
> >    * DSI packets transmitted by .transfer() are passed in as mipi_dsi_msg
> >    * structures. This structure contains information about the type of packet
> > @@ -81,10 +83,18 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> >    * function will seldomly return anything other than the number of bytes
> >    * contained in the transmit buffer on success.
> >    *
> > - * Also note that those callbacks can be called no matter the state the
> > - * host is in. Drivers that need the underlying device to be powered to
> > - * perform these operations will first need to make sure it's been
> > - * properly enabled.
> > + * Note: currently there are two modes of DSI power control. Legacy drivers
> > + * will call those callbacks no matter the state the host is in. DSI host
> > + * drivers that need the underlying device to be powered to perform these
> > + * operations will first need to make sure it's been properly enabled.
> > + *
> > + * Newer drivers will set the @MIPI_DSI_MANUAL_POWERUP flag to indicate that
> > + * they will call @mipi_dsi_power_up() and @mipi_dsi_power_down() to control
> > + * the link state of the DSI host or they will set @MIPI_DSI_AUTO_POWERUP to
> > + * indicate that the driver is fine with the link being powered up in DSI
> > + * host's (atomic_)pre_enable() callback and then being disabled in the
> > + * (atomic_)post_disable() callback. The transfer callback must only be called
> > + * if the DSI host has been powered up and was not brought down.
> >    *
> >    * Note: some hosts (sunxi) can not send LP commands between HS video
> >    * packets. Thus all DSI transfers sent in LP mode should be limited to the
> > @@ -97,6 +107,8 @@ struct mipi_dsi_host_ops {
> >                     struct mipi_dsi_device *dsi);
> >       ssize_t (*transfer)(struct mipi_dsi_host *host,
> >                           const struct mipi_dsi_msg *msg);
> > +     int (*power_up)(struct mipi_dsi_host *host);
> > +     void (*power_down)(struct mipi_dsi_host *host);
> >   };
> >
> >   /**
> > @@ -143,6 +155,10 @@ struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
> >   #define MIPI_DSI_MODE_LPM           BIT(11)
> >   /* transmit data ending at the same time for all lanes within one hsync */
> >   #define MIPI_DSI_HS_PKT_END_ALIGNED BIT(12)
> > +/* DSI peripheral driver manually controls DSI link powerup */
> > +#define MIPI_DSI_MANUAL_POWERUP              BIT(13)
> > +/* DSI peripheral driver is fine with automatic DSI link power control */
> > +#define MIPI_DSI_AUTO_POWERUP                BIT(14)
>
> What happens if none of the bits are in the flags ?
>
> Can't we implement "opportunistic power-up" on the first DSI command sent?

Not really. Such an opportunistic power up was expected to be there
and ... it failed, as you can see from the pre_enable_prev_first and
then by this series.

If the device doesn't set either of these flags, the DSI host can not
make any guesses about the time to power up the link. So, it should
follow the previous approach of enabling the DSI link no later than
mode_set. Otherwise the DSI sink might not be able to send DSI
commands from pre_enable callback.

>
> If a bridge/panel sends a DSI command, and if it happens before the DSI host enable, then
> the DSI host will "pre-enable" the host and put the link in LP-11.
>
> This would be simpler and would work whatever the pre_enable order.
>
> But this won't work for the tc358767, except if we add a dummy DSI host command
> which powers up the DSI link.
>
> This won't fix the PS8640 either who also needs a disabled DSI link to initialize.

Well, you have said it. The automatic enabling doesn't work if the DSI
host has no information about the DSI sink.

>
> Neil
>
> >
> >   enum mipi_dsi_pixel_format {
> >       MIPI_DSI_FMT_RGB888,
> > @@ -235,6 +251,11 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
> >   struct mipi_dsi_device *
> >   devm_mipi_dsi_device_register_full(struct device *dev, struct mipi_dsi_host *host,
> >                                  const struct mipi_dsi_device_info *info);
> > +
> > +bool mipi_dsi_host_power_control_available(struct mipi_dsi_host *host);
> > +int mipi_dsi_host_power_up(struct mipi_dsi_host *host);
> > +void mipi_dsi_host_power_down(struct mipi_dsi_host *host);
> > +
> >   struct mipi_dsi_device *of_find_mipi_dsi_device_by_node(struct device_node *np);
> >   int mipi_dsi_attach(struct mipi_dsi_device *dsi);
> >   int mipi_dsi_detach(struct mipi_dsi_device *dsi);
>


-- 
With best wishes
Dmitry

[RFC PATCH 04/10] drm/msm/dsi: use dsi_mgr_bridge_power_off in dsi_mgr_bridge_post_disable

[Dmitry Baryshkov]

Simplify dsi_mgr_bridge_post_disable() by using
dsi_mgr_bridge_power_off() instead of hand-coding the same call
sequence.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 896f369fdd53..9fa1f29ec11a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -372,8 +372,10 @@ static void dsi_mgr_bridge_post_disable(struct drm_bridge *bridge)
 	 * It is safe to call dsi_mgr_phy_disable() here because a single PHY
 	 * won't be diabled until both PHYs request disable.
 	 */
-	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
-		goto disable_phy;
+	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) {
+		dsi_mgr_phy_disable(id);
+		return;
+	}
 
 	ret = msm_dsi_host_disable(host);
 	if (ret)
@@ -385,26 +387,10 @@ static void dsi_mgr_bridge_post_disable(struct drm_bridge *bridge)
 			pr_err("%s: host1 disable failed, %d\n", __func__, ret);
 	}
 
-	msm_dsi_host_disable_irq(host);
-	if (is_bonded_dsi && msm_dsi1)
-		msm_dsi_host_disable_irq(msm_dsi1->host);
-
 	/* Save PHY status if it is a clock source */
 	msm_dsi_phy_pll_save_state(msm_dsi->phy);
 
-	ret = msm_dsi_host_power_off(host);
-	if (ret)
-		pr_err("%s: host %d power off failed,%d\n", __func__, id, ret);
-
-	if (is_bonded_dsi && msm_dsi1) {
-		ret = msm_dsi_host_power_off(msm_dsi1->host);
-		if (ret)
-			pr_err("%s: host1 power off failed, %d\n",
-								__func__, ret);
-	}
-
-disable_phy:
-	dsi_mgr_phy_disable(id);
+	dsi_mgr_bridge_power_off(bridge);
 }
 
 static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
-- 
2.42.0

[RFC PATCH 05/10] drm/msm/dsi: implement manual power control

[Dmitry Baryshkov]

Implement new API for tight control over the DSI link's power state.
This allows bridge and panel drivers to send DSI commands at a proper
time.

Note, this also brings back the ps8640 workaround (to be removed later,
once ps8640 driver sets up the MIPI_DSI_MANUAL_POWERUP flag).. We have
to make sure that the DSI link stays disabled in case of this bridge,
otherwise it will not work.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h         |  4 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c    | 44 +++++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++++++++++++++++++++----
 3 files changed, 106 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index a01c326774a6..e0ae2cb144de 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -63,6 +63,8 @@ bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
 int msm_dsi_manager_register(struct msm_dsi *msm_dsi);
 void msm_dsi_manager_unregister(struct msm_dsi *msm_dsi);
 void msm_dsi_manager_tpg_enable(void);
+int msm_dsi_manager_power_up(int id);
+void msm_dsi_manager_power_down(int id);
 
 /* msm dsi */
 static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
@@ -108,6 +110,8 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
 void msm_dsi_host_destroy(struct mipi_dsi_host *host);
 int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
 					struct drm_device *dev);
+bool msm_dsi_host_auto_powerup(struct mipi_dsi_host *host);
+bool msm_dsi_host_early_powerup(struct mipi_dsi_host *host);
 int msm_dsi_host_init(struct msm_dsi *msm_dsi);
 int msm_dsi_runtime_suspend(struct device *dev);
 int msm_dsi_runtime_resume(struct device *dev);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index fd2201cb62db..7010a6f17c5f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1671,10 +1671,54 @@ static ssize_t dsi_host_transfer(struct mipi_dsi_host *host,
 	return ret;
 }
 
+static bool msm_dsi_host_manual_powerup(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	return msm_host->mode_flags & MIPI_DSI_MANUAL_POWERUP;
+}
+
+bool msm_dsi_host_auto_powerup(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	return msm_host->mode_flags & MIPI_DSI_AUTO_POWERUP;
+}
+
+bool msm_dsi_host_early_powerup(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	return !msm_dsi_host_manual_powerup(host) &&
+		!msm_dsi_host_auto_powerup(host);
+}
+
+static int msm_dsi_host_power_up(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	if (msm_dsi_host_auto_powerup(host))
+		return 0;
+
+	return msm_dsi_manager_power_up(msm_host->id);
+}
+
+static void msm_dsi_host_power_down(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	if (!msm_dsi_host_manual_powerup(host))
+		return;
+
+	msm_dsi_manager_power_down(msm_host->id);
+}
+
 static const struct mipi_dsi_host_ops dsi_host_ops = {
 	.attach = dsi_host_attach,
 	.detach = dsi_host_detach,
 	.transfer = dsi_host_transfer,
+	.power_up = msm_dsi_host_power_up,
+	.power_down = msm_dsi_host_power_down,
 };
 
 /*
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 9fa1f29ec11a..2d7040d21239 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -198,6 +198,44 @@ static int dsi_mgr_bridge_get_id(struct drm_bridge *bridge)
 	return dsi_bridge->id;
 }
 
+/*
+ * If the next bridge in the chain is the Parade ps8640 bridge chip then don't
+ * power on early since it seems to violate the expectations of the firmware
+ * that the bridge chip is running.
+ */
+static bool dsi_mgr_next_is_ps8640(struct drm_bridge *bridge)
+{
+	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
+
+	return next_bridge &&
+		next_bridge->of_node &&
+		of_device_is_compatible(next_bridge->of_node, "parade,ps8640");
+}
+
+static bool dsi_mgr_auto_powerup(struct drm_bridge *bridge)
+{
+	int id = dsi_mgr_bridge_get_id(bridge);
+	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+	struct mipi_dsi_host *host = msm_dsi->host;
+
+	if (dsi_mgr_next_is_ps8640(bridge))
+		return true;
+
+	return msm_dsi_host_auto_powerup(host);
+}
+
+static bool dsi_mgr_early_powerup(struct drm_bridge *bridge)
+{
+	int id = dsi_mgr_bridge_get_id(bridge);
+	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+	struct mipi_dsi_host *host = msm_dsi->host;
+
+	if (dsi_mgr_next_is_ps8640(bridge))
+		return false;
+
+	return msm_dsi_host_early_powerup(host);
+}
+
 static void msm_dsi_manager_set_split_display(u8 id)
 {
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
@@ -228,9 +266,8 @@ static void msm_dsi_manager_set_split_display(u8 id)
 	}
 }
 
-static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
+int msm_dsi_manager_power_up(int id)
 {
-	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
 	struct mipi_dsi_host *host = msm_dsi->host;
@@ -240,6 +277,10 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 
 	DBG("id=%d", id);
 
+	/* Do nothing with the host if it is slave-DSI in case of bonded DSI */
+	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
+		return 0;
+
 	ret = dsi_mgr_phy_enable(id, phy_shared_timings);
 	if (ret)
 		goto phy_en_fail;
@@ -278,9 +319,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 	return ret;
 }
 
-static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
+void msm_dsi_manager_power_down(int id)
 {
-	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
 	struct mipi_dsi_host *host = msm_dsi->host;
@@ -312,10 +352,12 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
-	ret = dsi_mgr_bridge_power_on(bridge);
-	if (ret) {
-		dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret);
-		return;
+	if (dsi_mgr_auto_powerup(bridge)) {
+		ret = msm_dsi_manager_power_up(id);
+		if (ret) {
+			dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret);
+			return;
+		}
 	}
 
 	ret = msm_dsi_host_enable(host);
@@ -337,7 +379,8 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 host1_en_fail:
 	msm_dsi_host_disable(host);
 host_en_fail:
-	dsi_mgr_bridge_power_off(bridge);
+	if (dsi_mgr_auto_powerup(bridge))
+		msm_dsi_manager_power_down(id);
 }
 
 void msm_dsi_manager_tpg_enable(void)
@@ -390,7 +433,9 @@ static void dsi_mgr_bridge_post_disable(struct drm_bridge *bridge)
 	/* Save PHY status if it is a clock source */
 	msm_dsi_phy_pll_save_state(msm_dsi->phy);
 
-	dsi_mgr_bridge_power_off(bridge);
+	if (dsi_mgr_auto_powerup(bridge) ||
+	    dsi_mgr_early_powerup(bridge))
+		msm_dsi_manager_power_down(id);
 }
 
 static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
@@ -411,6 +456,9 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
 	msm_dsi_host_set_display_mode(host, adjusted_mode);
 	if (is_bonded_dsi && other_dsi)
 		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
+
+	if (dsi_mgr_early_powerup(bridge))
+		msm_dsi_manager_power_up(id);
 }
 
 static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
-- 
2.42.0

[RFC PATCH 06/10] drm/bridge: tc358762: add support for manual DSI power control

[Dmitry Baryshkov]

Start migrating tc358762 bridge to new manual DSI power control API. If
the tight power control is not available, default to the existing
pre_enable_prev_first flag.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/tc358762.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 7b9f05f95fd1..efb1da4487c9 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -72,6 +72,7 @@ struct tc358762 {
 	struct device *dev;
 	struct drm_bridge bridge;
 	struct regulator *regulator;
+	struct mipi_dsi_host *dsi_host;
 	struct drm_bridge *panel_bridge;
 	struct gpio_desc *reset_gpio;
 	struct drm_display_mode mode;
@@ -163,6 +164,8 @@ static void tc358762_post_disable(struct drm_bridge *bridge, struct drm_bridge_s
 
 	ctx->pre_enabled = false;
 
+	mipi_dsi_host_power_down(ctx->dsi_host);
+
 	if (ctx->reset_gpio)
 		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
 
@@ -185,6 +188,10 @@ static void tc358762_pre_enable(struct drm_bridge *bridge, struct drm_bridge_sta
 		usleep_range(5000, 10000);
 	}
 
+	ret = mipi_dsi_host_power_up(ctx->dsi_host);
+	if (ret < 0)
+		dev_err(ctx->dev, "error powering up the DSI host (%d)\n", ret);
+
 	ret = tc358762_init(ctx);
 	if (ret < 0)
 		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
@@ -277,10 +284,16 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
 	if (ret < 0)
 		return ret;
 
+	ctx->dsi_host = dsi->host;
+
 	ctx->bridge.funcs = &tc358762_bridge_funcs;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
 	ctx->bridge.of_node = dev->of_node;
-	ctx->bridge.pre_enable_prev_first = true;
+
+	if (mipi_dsi_host_power_control_available(dsi->host))
+		dsi->mode_flags |= MIPI_DSI_MANUAL_POWERUP;
+	else
+		ctx->bridge.pre_enable_prev_first = true;
 
 	drm_bridge_add(&ctx->bridge);
 
-- 
2.42.0

[RFC PATCH 07/10] drm/bridge: ps8640: require manual DSI power control

[Dmitry Baryshkov]

The Parade PS8640 bridge will fail to start if the DSI link is enabled
when the bridge is being reset / powered up (even to the LP-11 state).
To ensure that the DSI link is powered down, require manual control over
the DSI link state.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8161b1a1a4b1..6c5daaa70cb7 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -458,6 +458,10 @@ static void ps8640_atomic_pre_enable(struct drm_bridge *bridge,
 
 	ps8640_bridge_vdo_control(ps_bridge, ENABLE);
 
+	ret = mipi_dsi_host_power_up(ps_bridge->dsi->host);
+	if (ret < 0)
+		dev_warn(dev, "failed to power up DSI host: %d\n", ret);
+
 	ps_bridge->pre_enabled = true;
 }
 
@@ -468,6 +472,8 @@ static void ps8640_atomic_post_disable(struct drm_bridge *bridge,
 
 	ps_bridge->pre_enabled = false;
 
+	mipi_dsi_host_power_down(ps_bridge->dsi->host);
+
 	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
 	pm_runtime_put_sync_suspend(&ps_bridge->page[PAGE0_DP_CNTL]->dev);
 }
@@ -562,6 +568,11 @@ static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps
 	if (!host)
 		return -EPROBE_DEFER;
 
+	if (!mipi_dsi_host_power_control_available(host)) {
+		dev_err(dev, "MIPI DSI host doesn't provide tight power control\n");
+		return -ENODEV;
+	}
+
 	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
 	if (IS_ERR(dsi)) {
 		dev_err(dev, "failed to create dsi device\n");
@@ -572,7 +583,8 @@ static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps
 
 	dsi->host = host;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
-			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			  MIPI_DSI_MANUAL_POWERUP;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->lanes = NUM_MIPI_LANES;
 
-- 
2.42.0

[RFC PATCH 08/10] drm/bridge: lt9611: mark for automatic DSI power control

[Dmitry Baryshkov]

The Lontium LT9611 driver doesn't need to control DSI power
lines manually. Mark it for automatic power control.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/lontium-lt9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 9663601ce098..10b7093bd5c5 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -774,7 +774,7 @@ static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611,
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
-			  MIPI_DSI_MODE_VIDEO_HSE;
+			  MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_AUTO_POWERUP;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
-- 
2.42.0

[RFC PATCH 09/10] drm/bridge: lt9611uxc: implement automatic DSI power control

[Dmitry Baryshkov]

The Lontium LT9611UXC driver doesn't need to control DSI power
lines manually. Mark it for automatic power control.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index 7835738a532e..ace7a6ee890b 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -279,7 +279,7 @@ static struct mipi_dsi_device *lt9611uxc_attach_dsi(struct lt9611uxc *lt9611uxc,
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
-			  MIPI_DSI_MODE_VIDEO_HSE;
+			  MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_AUTO_POWERUP;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
-- 
2.42.0

[RFC PATCH 10/10] drm/msm/dsi: drop (again) the ps8640 workaround

[Dmitry Baryshkov]

Now as the Parade PS8640 driver sets the MIPI_DSI_MANUAL_POWERUP flag,
drop the workaround enforcing the late DSI link powerup in the case the
next bridge is ps8640.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 2d7040d21239..b6b8171cf382 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -198,29 +198,12 @@ static int dsi_mgr_bridge_get_id(struct drm_bridge *bridge)
 	return dsi_bridge->id;
 }
 
-/*
- * If the next bridge in the chain is the Parade ps8640 bridge chip then don't
- * power on early since it seems to violate the expectations of the firmware
- * that the bridge chip is running.
- */
-static bool dsi_mgr_next_is_ps8640(struct drm_bridge *bridge)
-{
-	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
-
-	return next_bridge &&
-		next_bridge->of_node &&
-		of_device_is_compatible(next_bridge->of_node, "parade,ps8640");
-}
-
 static bool dsi_mgr_auto_powerup(struct drm_bridge *bridge)
 {
 	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct mipi_dsi_host *host = msm_dsi->host;
 
-	if (dsi_mgr_next_is_ps8640(bridge))
-		return true;
-
 	return msm_dsi_host_auto_powerup(host);
 }
 
@@ -230,9 +213,6 @@ static bool dsi_mgr_early_powerup(struct drm_bridge *bridge)
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct mipi_dsi_host *host = msm_dsi->host;
 
-	if (dsi_mgr_next_is_ps8640(bridge))
-		return false;
-
 	return msm_dsi_host_early_powerup(host);
 }
 
-- 
2.42.0