Message-ID: <uj6rtlionmacnwlqxy6ejt5iaczgbbe5z54ipte5ffbixcx3p4@pps7fcr3uqhf>
Date:   Wed, 25 Oct 2023 14:44:59 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        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>,
        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 Thu, Oct 19, 2023 at 02:19:51PM +0300, Dmitry Baryshkov wrote:
> On Thu, 19 Oct 2023 at 12:26, Maxime Ripard <mripard@...nel.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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists