mbox series

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

Message ID 20231016165355.1327217-1-dmitry.baryshkov@linaro.org
Headers show
Series drm/mipi-dsi: another attempt at sorting out DSI link powerup | expand

Message

Dmitry Baryshkov Oct. 16, 2023, 4:53 p.m. UTC
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(-)

Comments

Alexander Stein Oct. 19, 2023, 11:42 a.m. UTC | #1
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
Dmitry Baryshkov Oct. 22, 2023, 10:49 a.m. UTC | #2
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/
>
>
Alexander Stein Oct. 23, 2023, 6:52 a.m. UTC | #3
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/
Dmitry Baryshkov Oct. 23, 2023, 7:34 a.m. UTC | #4
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
Alexander Stein Oct. 23, 2023, 8:14 a.m. UTC | #5
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
Dmitry Baryshkov Oct. 23, 2023, 8:40 a.m. UTC | #6
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/
>
>