Messages in this thread | | | Date | Wed, 25 Oct 2023 18:16:14 +0300 | Subject | Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state | From | 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@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
| |