On Thu, 19 Oct 2023 at 14:42, Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > --- > > > > > > > > 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
