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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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