Questions over DSI within DRM.

[Laurent Pinchart]

Hello,

Reviving a bit of an old thread.

On Thu, Jul 15, 2021 at 11:50:22AM +0200, Maxime Ripard wrote:
> On Tue, Jul 06, 2021 at 05:44:58PM +0100, Dave Stevenson wrote:
> > On Tue, 6 Jul 2021 at 16:13, Maxime Ripard wrote:
> > > > > > On a similar theme, some devices want the clock lane in HS mode early
> > > > > > so they can use it in place of an external oscillator, but the data
> > > > > > lanes still in LP-11. There appears to be no way for the
> > > > > > display/bridge to signal this requirement or it be achieved.
> > > > >
> > > > > You're right. A loooong time ago, the omapdrm driver had an internal
> > > > > infrastructure that didn't use drm_bridge or drm_panel and instead
> > > > > required omapdrm-specific drivers for those components. It used to model
> > > > > the display pipeline in a different way than drm_bridge, with the sync
> > > > > explicitly setting the source state. A DSI sink could thus control its
> > > > > enable sequence, interleaving programming of the sink with control of
> > > > > the source.
> > > > >
> > > > > Migrating omapdrm to the drm_bridge model took a really large effort,
> > > > > which makes me believe that transitioning the whole subsystem to
> > > > > sink-controlled sources would be close to impossible. We could add
> > > > > DSI-specific operations, or add another enable bridge operation
> > > > > (post_pre_enable ? :-D). Neither would scale, but it may be enough.
> > > >
> > > > I haven't thought it through for all generic cases, but I suspect it's
> > > > more a pre_pre_enable that is needed to initialise the PHY etc,
> > > > probably from source to sink.

I believe it could be implemented as a pre-pre-enable indeed. It feels
like a bit of a hack, as the next time we need more fine-grained control
over the startup sequence, we'll have to add a pre-pre-pre-enable. Given
that the startup sequence requirements come from the sink device, it
would make sense to let it explicitly control the initialization,
instead of driving it from the source. I don't think we'll be able to
rework the bridge API in that direction though, so I'm fine with a hack.

> > > > If the panel/bridge can set a flag that can be checked at this point
> > > > for whether an early clock is required or not, I think that allows us
> > > > to comply with the requirements for a large number of panels/bridges
> > > > (LP-11 vs HS config for clock and or data lanes before pre_enable is
> > > > called).
> > > >
> > > > pre_enable retains the current behaviour (initialise the chain from
> > > > sink to source).
> > > > enable then actually starts sending video and enabling outputs.
> > >
> > > Flags indeed seem like a more contained option. Another one could be to
> > > have a mipi_dsi_host to (for example) power up the clock lane that would
> > > be called by default before the bridge's enable, or at the downstream
> > > bridge driver discretion before that.
> > 
> > Which driver will that call?
> 
> The parent DSI Host
> 
> > An extreme example perhaps, but Toshiba make the TC358860 eDP to DSI
> > bridge chip[1]. So the encoder will be eDP, but the DSI config needs
> > to go to that bridge. Does that happen automatically within the
> > framework? I guess so as the bridge will have called
> > mipi_dsi_host_register for the DSI sink to attach to.
> 
> In that case, whatever sink would be connected to the bridge would call
> the bridge clock enable hook if it needs it in its pre_enable, or it
> would be called automatically before enable if it doesn't
> 
> Would that help?

Sounds good to me, in theory at least (let's see what issues we'll run
into in practice :-)).

Has anyone given it a try, or is planning to ?

> > Perhaps a new mipi_dsi_host function to configure the PHY is the
> > easier solution. If it can allow the sink to request whatever
> > combination of states from clock and data lanes that it fancies, then
> > it can be as explicit as required for the initialisation sequence, and
> > the host driver does its best to comply with the requests.
> 
> I don't know, I'm not really fond in general of solutions that try to
> cover any single case if we don't need it and / or have today an issue
> with this. I'd rather have something that works for the particular
> bridge we were discussing, see if it applies to other bridges and modify
> it if it doesn't until it works for all our cases. Trying to reason in
> all possible cases tend to lead to solutions that are difficult to
> maintain and usually over-engineered.

A DSI host clock enable operation or a DSI host PHY configuration
operation both fit in the same place in the grand scheme of things, so I
don't mind either. We should be able to easily move from a more specific
operation to a more generic one if the need arises.

> > I'd have a slight query over when and how the host would drop to ULPS
> > or power off. It probably shouldn't be in post_disable as the sink
> > hasn't had a chance to finalise everything in its post_disable.
> >
> > Perhaps pm_runtime with autosuspend is the right call there?
> 
> pm_runtime semantics mean that once the device is suspended, its power
> domains, regulators, clocks, etc. are all shut down, so it doesn't
> really fit the low power state expected by DSI
> 
> > [1] https://toshiba.semicon-storage.com/ap-en/semiconductor/product/interface-bridge-ics-for-mobile-peripheral-devices/display-interface-bridge-ics/detail.TC358860XBG.html
> > 
> > > > When I discussed this briefly with Maxime there was a suggestion of
> > > > using pm_runtime to be able to power up the pipeline as a whole. If
> > > > the bridge driver can use pm_runtime to power up the PHY when
> > > > required, then that may solve the issue, however I know too little of
> > > > the details to say whether that is actually practical.
> > >
> > > I'm not sure it was about this topic in particular. If I remember well
> > > our discussion, this was about the vc4 driver that tries to circumvent
> > > the framework and call the pre_enable and enable hooks itself because it
> > > wasn't properly powered before and thus any DCS-related call by the
> > > downstream bridge or panel would end up creating a CPU stall.
> > >
> > > I suggested to use runtime_pm in the DCS related calls to make sure the
> > > device is powered because there's no relation between the state of the
> > > downstream bridge or panel and whether it can send DCS commands or not.
> > > For all we know, it could do it at probe time.
> > 
> > pm_runtime is all a bit of a magic black box to me.
> > 
> > We had discussed shifting to using pm_runtime from DCS (and enable)
> > calls to power up the PHY on demand, and that's what I implemented.
> > However Laurent flagged up that using
> > dsi->encoder->crtc->state->adjusted_mode to get the HS clock info
> > required to send a HS DCS command from that call is deprecated, so how
> > do we specify the clock rate to use at that point?
> 
> I guess the most sensible would be to have a custom bridge state, and
> add a pointer to the current bridge state in struct drm_bridge. Then, as
> long as you have a bridge pointer you have a way to get the current
> state associated to it, and since we already have atomic_duplicate_state
> / atomic_destroy_state we can create our own structure around it storing
> whatever we want.

That's a good point. It would only be needed if we use runtime PM to
work around the initialization sequence issue, not if we implement a DSI
host clock enable/disable operation, right ?

> > > > > > host_transfer calls can supposedly be made at any time, however unless
> > > > > > MIPI_DSI_MSG_USE_LPM is set in the message then we're meant to send it
> > > > > > in high speed mode. If this is before a mode has been set, what
> > > > > > defines the link frequency parameters at this point? Adopting a random
> > > > > > default sounds like a good way to get undefined behaviour.
> > > > > >
> > > > > > DSI burst mode needs to set the DSI link frequency independently of
> > > > > > the display mode. How is that meant to be configured? I would have
> > > > > > expected it to come from DT due to link frequency often being chosen
> > > > > > based on EMC restrictions, but I don't see such a thing in any
> > > > > > binding.
> > > > >
> > > > > Undefined too. DSI support was added to DRM without any design effort,
> > > > > it's more a hack than a real solution. The issue with devices that can
> > > > > be controlled over both DSI and I2C is completely unhandled. So far
> > > > > nobody has really cared about implementing DSI right as far as I can
> > > > > tell.
> > > >
> > > > :-(
> > > >
> > > > Thinking aloud, does having the option to set a burst link frequency
> > > > from DT (or ACPI) have any issue for other platforms?
> > > > Looking at the handling of MIPI_DSI_MODE_VIDEO_BURST in the various
> > > > drivers, all except stm/dw_mipi_dsi-stm.c appear to take it as a "use
> > > > all the defined timings, but drop to LP during blanking" option. The
> > > > link frequency has therefore remained a property of the
> > > > display/bridge.
> > > > dw_mipi_dsi-stm.c cranks the PLL up by 20%, but I haven't followed
> > > > through the full detail of the parameters it computes from there.
> > >
> > > I don't see anything wrong with using link-frequency from the DT to
> > > setup the burst frequency. It's what v4l2 has been using for a while
> > > without any known (to me) drawback, and we're using the same of-graph
> > > bindings, so it shouldn't be too controversial there.

How would that frequency we picked in practice ? Do panels typically
support a range of HS frequencies for DCS HS transfers ?

> > OK, that sounds like a vague plan.
> > 
> > > > DSI and I2C controlled devices is yet another issue that I haven't
> > > > even looked at.
> > > > I think it's more that vc4 wants to ignore DSI should the DSI host
> > > > node be enabled in DT, but there's no panel bound to it. One could say
> > > > that is a DT error and tough luck, but from a user's perspective that
> > > > is a bit harsh.
> > >
> > > I guess the larger "issue" is that the tree in the DT is done following
> > > the "control" bus, and Linux likes to tie the life cycle of a given
> > > device to its parent bus. Both these decisions make sense, but they
> > > interact in a weird way in some occurrences (like this one, or Allwinner
> > > has an Ethernet PHY controlled through MMIO which end up in the same
> > > case).
> > >
> > > I wonder if using device links here could help though.
> > 
> > I really don't know about that one.
> 
> It's a piece of infrastructure that was created at first (I think?) to
> model the power dependency between devices that don't have a parent /
> child relationship. For example, if you use DMA, you probably want to
> keep the IOMMU powered as long as you are, but it is in a completely
> separate branch of the "device tree" (not one from the DTB, the one that
> linux DM creates).
> 
> It was later expanded to also cover probe order and make sure a supplier
> would probe before its consumer, effectively making EPROBE_DEFER
> obsolete.
> 
> The second part is still fairly new, but I think we can solve this by
> adding a device link between the DSI host and whatever is at the end of
> the OF-Graph endpoint.
> 
> > > > > > As a follow on, bridge devices can support burst mode (eg TI's
> > > > > > SN65DSI83 that's just been merged), so it needs to know the desired
> > > > > > panel timings for the output side of the bridge, but the DSI link
> > > > > > timings to set up the bridge's PLL. What's the correct way for
> > > > > > signalling that? drm_crtc_state->adjusted_mode vs
> > > > > > drm_crtc_state->mode? Except mode is userspace's request, not what has
> > > > > > been validated/updated by the panel/bridge.
> > > > >
> > > > > adjusted_mode is also a bit of a hack, it solves very specific issues,
> > > > > and its design assumes a single encoder in the chain with no extra
> > > > > bridges. We should instead add modes to the bridge state, and negotiate
> > > > > modes along the pipeline the same way we negotiate formats.
> > > >
> > > > So as I understand it we already have format negotiation between
> > > > bridges via atomic_get_output_bus_fmts and atomic_get_input_bus_fmts,
> > > > so is it possible to extend that to modes?
> > > > Are you thinking bridge state that is owned by the framework, or by
> > > > the individual bridge drivers?
> > >
> > > atomic_check is made for that. I guess we could improve its call
> > > sequence to each time a mode is modified along the bridge list we
> > > restart the sequence until all components agree (or reject it entirely
> > > if they can't), but I don't really see why we would need yet another
> > > hook.

Isn't this what atomic_get_output_bus_fmts() and
atomic_get_input_bus_fmts() implement ?

> > Why do all nodes in the bridge list need to agree? Adjacent nodes need
> > to agree, but they then also need to retain that agreed timing
> > somewhere.
> 
> We might have mutually exclusive requirements though? Let's use the
> example of the VC4 HDMI driver that can't have odd horizontal timings,
> and assume it's a constraint of our DSI driver instead.
> 
> Then, we have a DSI->LVDS bridge, a LVDS->RGB bridge and a panel (which
> is a bit ridiculous, but whatever). If the LVDS->RGB bridge can't have
> even horizontal timings, then you just can't display it, even though
> they are not adjacent (unless the bridge in the middle can modify the
> timings between the input and output, but that's not always possible).
> 
> Similarly, if for the RGB panel we need to increase a bit some timings
> to accommodate for a larger pixel clock and end up above what the DSI
> host can provide, we're also done.

The hard part will be to figure out a good heuristics to perform the
negotiation without going back and forth (at least not in a way that
would require too many iterations, and certainly avoiding infinite
loops). That will be an interesting problem to solve, but maybe we'll be
lucky and a simple approach will work for the use cases we have to
support today.

> > Taking SN65DSI8[3|4|5] as an example, it supports burst mode, and the
> > DSI frequency and timings are permitted to be different from that
> > which it uses on the LVDS side. The LVDS panel and LVDS side of DSI83
> > need to agree over the format, and the DSI host and DSI side of DSI83
> > need to agree, but they may be two different timings.
> > Register 0x0B (DSI_CLK_DIVIDER & REFCLK_MULTIPLIER) allows you to
> > configure the LVDS rate compared to the DSI rate (the driver currently
> > goes for 1:1), and registers 0x20 to 0x34 allow you to set the number
> > of active pixel and blanking on the LVDS side (again currently just
> > copied across).
> >
> > The way I'm seeing burst mode as having been interpreted at present is
> > that it's largely just a flag to say "drop to LP mode between lines".
> > The timing that needs to be passed to the crtc is therefore going to
> > be based on the DSI link rate (converted to pixels) with increased
> > blanking periods.
> > 
> > I guess there are similarities with Media Controller and V4L2 here. A
> > typical chain there could be:
> >  sensor -> scaler -> crop -> CSI-2 receiver.
> > The format on each of those links may be different, but the chain as a
> > whole needs to be valid. Media Controller largely relies on userspace
> > to configure all links, but with a DRM chain that isn't really an
> > option as it's expected that the display chain configures itself.
> 
> Also, the userspace has no concept of media sub-devices in DRM, so it
> just sets the mode on the whole DRM/KMS device, unlike what v4l2 does.
> In v4l2, afaik, if you ended up with the above scenarios it would just
> be rejected when you set the format on the link, letting the userspace
> figure it out. We can't really do that here

I wonder how long we'll be able to keep userspace out of the picture to
configure the internals of the pipeline. I don't want to be the first
person who will have a use case that requires this.

-- 
Regards,

Laurent Pinchart