lkml.org 
[lkml]   [2023]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state
    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

    \
     
     \ /
      Last update: 2023-10-19 13:23    [W:4.780 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site