Message-ID: <CAA8EJpqB6eMCEEg3Ptg531mTiOsnP=3qE_C6dyA1_TO4ZbJAbw@mail.gmail.com>
Date:   Mon, 23 Oct 2023 10:40:05 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     neil.armstrong@...aro.org
Cc:     Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        Robert Foss <rfoss@...nel.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Douglas Anderson <dianders@...omium.org>,
        Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Jessica Zhang <quic_jesszhan@...cinc.com>,
        Marek Vasut <marex@...x.de>, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        freedreno@...ts.freedesktop.org
Subject: Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over
 the DSI link power state

On Mon, 23 Oct 2023 at 10:35, Neil Armstrong <neil.armstrong@...aro.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@...aro.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

Powered by blists - more mailing lists