Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state

[Dmitry Baryshkov]

On 25/10/2023 15:44, Maxime Ripard wrote:
> On Thu, Oct 19, 2023 at 02:19:51PM +0300, Dmitry Baryshkov wrote:
> > 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.
>
> 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.

That's not the only driver with strange peculiarities. For example, see 
commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming 
from pre-enable to enable"), which was one of the issues that actually 
prompted me to send this this patchset (after my previous version of 
this patch being rejected because of sunxi).

> > 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.

In the mentioned discussion it was more about 'DSI host should not 
assume panel driver features', like the panel sending commands in 
pre_enable or not, or having pre_enable_prev_first.

So the pre_enable_prev_first clearly lacks feature negotiation.


> 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?

Well, we do not support DSI sublinks, do we?

> 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.

Yes, quite unfortunately. Another approach that I have in mind is to add 
two callbacks to mipi_dsi_device. This way the DSI host will call into 
the device to initialise it once the link has been powered up and just 
before tearing it down. We solve a lot of problems this way, no 
boilerplate and the panel / bridge are in control of the initialisation 
procedure. WDYT?

> 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?

There is not that much in the DSI spec (or maybe I do not understand the 
question). The spec is more about the power states and the commands. Our 
problem is that this doesn't fully match kernel expectations.

--
With best wishes
Dmitry