Re: [PATCH 14/17] drm/msm: add atomic support

From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 14/17] drm/msm: add atomic support
Date: Tue, 27 May 2014 19:50:17 +0200	[thread overview]
Message-ID: <20140527175017.GJ14841@phenom.ffwll.local> (raw)
In-Reply-To: <CAF6AEGvTEobkJfLSpCP_+v1-vaGRp_k+6piEn8+BgiSLsfwkqA@mail.gmail.com>

On Tue, May 27, 2014 at 11:58:33AM -0400, Rob Clark wrote:
> On Mon, May 26, 2014 at 1:54 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > Ok, I think I should have read your msm implementation a _lot_ earlier.
> > Explains your desing choices neatly.
> >
> > Two observations:
> >
> > - A GO bit makes nuclear pageflips ridiculously easy to implement,
> >   presuming the hardware actually works. And it's the sane model, so imo a
> >   good one to wrap the atomic helpers around.
> >
> >   But reality is often a lot more ugly, especially if you're employed by
> >   Intel. Which is why I'm harping so much on this helpers-vs-core
> >   interface issues ... We really need the full state transition in one
> >   piece to do anything useful.
> >
> > - msm doesn't have any resource sharing going on for modeset operations
> >   (which I mean lighting up crtcs to feed pixel streams to connectors).
> >   Which means the simplistic "loop over all crtcs and call the old
> >   ->setcrtc" approach actually works.
> 
> we do actually have some shared resources on mdp5 generation (the
> "smp" blocks, basically a shared buffer which we need to allocate fifo
> space from for each plane)..
> 
> I'm mostly ignoring this at the moment, because we don't support
> enough crtc's yet to run out of smp blocks.  But hooking in at the
> current ->atomic_commit() would be enough for me, I think.  Tbh, it is
> something that should be handled at the ->atomic_check() stage so I
> hadn't given much though to ->atomic_commit() stage.
> 
> That all said, I've no problem with adding one more hook, after
> ->atomic_commit() and lock magic, before loop over planes/crtcs, so
> drivers that need to can replace the loops and do their own thing.
> Current state should be reasonably good for sane hw.  I'm just waiting
> for patches for i915 to add whatever it needs.

I don't think that will be enough for you. Example, slightly hypothetical:

- Configuration A: crtc 0 displays a small screen, crtc 1 a giant screen.
  Together they just barely fit into your fifo space.

- Configuration B: crtc 0 drives a giant screen, crtc 1 a tiny one.
  Presume different outputs here so that no implicit output stealing
  happens upon mode switching.

Atomic switch should work, but don't since if you just loop over crtcs you
have the intermediate stage where you try to drive 2 giant screens, which
will run out of fifo resources. And I think it's really common to have
such implicit resource sharing, maybe except for the most beefy desktop
gpu which simply can't run out of memory bw with today's screen.

So afaics you really need to push a bit of smarts into the crtc helpers to
make atomic possible, which then leaves you with interaction issues
between the atomic stuff for GO bit capable hw for plane updates and the
modeset ordering hell.

> >   The problem I see here is that if you have hardware with more elaborate
> >   needs (e.g. shared dplls), but otherwise sanity for plane updates (i.e.
> >   a GO bit) then your current atomic helpers will make it rather hard to
> >   mix this. So I think we should pimp the crtc helpers a bit to be atomic
> >   compliant (i.e. kill all outputs first before enabling anything new) and
> >   try to integrate this with the atomic helpers used for GO bit style
> >   updates.
> 
> Not really, I don't think.  You can still do whatever shared resource
> setup in ->atomic_commit().  For drivers which want to defer commit
> (ie. doing commit after fb's ready from cpu, rather than from gpu), it
> would probably be more convenient to split up atomic_commit() so
> drivers have a post lock hook.  I think it is just a few line patch,
> and I think that it probably isn't really needed until i915 is ready.
> I'm pretty sure we can change this to do what i915 needs, while at the
> same time keeping it simple for 'GO bit' drivers.
>
> The bit about crtc helpers, I'll have to think about.  I'm not sure
> that we want to require that 'atomic' (modeset or pageflip) completely
> *replaces* current state.  So disabling unrelated crtcs doesn't seem
> like the right thing.  But we have some time to decide about the
> semantics of an atomic modeset before we expose to userspace.

I'm not talking about replacing unrelated crtcs. It's also not about
underspecified state or about enabling/changing global resources.

It is _only_ about ordering of the various operations: If both the
current and the desired new configuration are at the limit of what the hw
can do you can't switch to the new configuration by looping over all
crtcs. The fact that this doesn't work is the entire point of atomic
modesets. And if we have helpers which aren't cut out for the task at hand
for any hw where we need to have atomic modesets to make such changes
possible without userspace going nuts, then the helpers are imo simply not
useful

> >   i915 has dpll sharing on ivb/hsw, but doesn't use the the crtc helpers
> >   anymore. But the radone eyefinity (or whatever the damn thing is called)
> >   I have lying around here fits the bill: It has 5 DP+ outputs but only 2
> >   dplls. So you can drive e.g. 3 DP screens and then switch to 2 HDMI
> >   screens atomically and it should all pan out.
> >
> >   But since your current helpers just loop over all crtcs without any
> >   regard to ordering constraints this will fall over if the 2 HDMI outputs
> >   get enabled before the 3 DP outputs get disabled all disabled.
> 
> the driver should have rejected the config before it gets to commit
> stage, if it were an impossible config.

The configuration _is_ possible. We simply have to be somewhat careful
with transitioning to it, since not _all_ intermediate states are
possible. Your current helpers presume that's the case, which afaik isn't
the case on any hw where we have global limits. For modesets.

Nuclear pageflips are a completely different thing, as long as your hw has
a GO bit.

> > So with all that in mind I have 3 sanity checks for the atomic interfaces
> > and helpers:
> >
> > 1. The atomic helpers should make enabling sane hw with a GO bit easy for
> > nuclear pageflips. Benchmark would be sane hw like msm or omap.
> >
> > 2. It should cooperate with the crtc helpers such that hw with some shared
> > resources (like dplls) works for atomic modesets. That pretty much means
> > "disable everything (including crtc/connetor pipelines that only changed
> > some parts) before enabling anything". Benchmark would be a platform with
> > shared dplls.
> 
> btw, not something I'd thought about before, but shared dplls seem
> common enough, that it is something core could help with.  (Assuming
> they are all related to pixel clk and not some derived thing that core
> wouldn't know about.)

Yup, that's what I'm trying to say ;-) But it was just an example, I
believe that atm your helpers don't help for any shared modeset resources
at all.

> I think we do need to decide what partial state updates me in the
> context of modeset or pageflip.  I kinda think the right thing is
> different for modeset vs pageflip.  Maybe we want to define an atomic
> flag to mean "disable/discard everything else"..  at any rate, we only
> need to sort that before exposing the API to userspace.

Yeah, I still strongly support this split in the api itself. For i915 my
plan is to have separate configuration structures for modeset state and
pageflip/plane config state. When we do an atomic modeset we compute both
(maybe with some shortcut if we know that the pipe config didn't change at
all). Then comes the big switch:

- If we have the same pipe config, we simply to a vblank synce nuclear
  flip to the new config.

- If pipe configs change it will be much more elaborate:

  1. Do a vblank synced nuclear flip to black on all pipes that need to go
  off (whether they get disabled or reconfigured doesn't matter for now).

  2. Disable pipes.

  3. Commit new state on the sw side.

  4. Enable all pipes with the new config, but only scanning out black.

  5. Do a vblank-synced nuclear flip to the new config. This would also be
  the one that would signale the drm events that the atomic update
  completed.

  For fastboot we might need some hacks to fuse this all together, e.g for
  some panel fitter changes we don't need to disable the pipe completely.
  But that's the advanced stuff really.

I think modelling the crtc helpers after this model could work. But that
means that the crtc helpers and the nuclear flip atomic helpers for GO
bit capable hw need to be rather tightly integrated, while still allowing
drivers to override the nuclear flip parts.

> > 3. The core->driver interface should be powerful enough to support
> > insanity like i915, but no more. Which means all the code that's share
> > (i.e. the set_prop code I've been harping all over the place about) should
> > be done in the core, without any means for drivers to override. Currently
> > the drm core also takes care of a bunch of things like basic locking and
> > refcounting, and that's kinda the spirit for this. i915 is the obvious
> > benchmark here.
> 
> The more I think about it, the more I think we should leave set_prop
> as it is (although possibly with drm_atomic_state ->
> drm_{plane,crtc,etc}_state).
> 
> In the past, before primary plane, I really needed this.  And I expect
> having a convenient way to 'sniff' changing properties as they go by
> will be useful in some cases.

I actually really like the addition of the state object to ->set_prop. At
least for i915 (which already has a fair pile of additional properties)
that looks like the perfect way to prep the stage.

But at least for the transition we should keep the impact minimal. So no
manadatory callbacks and don't enforce the usage of the state object until
the drm core is fully converted to always follow a set_prop with a
->commit. Since currently we have internal mode_set calls in all i915
set_prop functions and we need to convert them all. But we can't do that
until all the core stuff uses the atomic interface for all legacy ioctl
ops.

> > I think we can roll this out piecemeal (and it's probably better to do it
> > that way), but I also think that until we've resolved the requirements of
> > 2&3 we should try to minimize subsystem wide changes as much as possible
> > by making them opt-in and the vfuncs optional.
> 
> the new mandatory vfuncs don't amount to too much churn.. not as much
> as the extra param for crtc lock/unlock fxns.  Maybe if I was planning
> on keeping this alive on a branch for so long in the beginning I would
> have arranged a few things slightly different, but meh.
> 
> A very early iteration of atomic preserved the old pageflip, setcrtc,
> setplane, etc paths for drivers not implementing atomic fxns.  But
> that was at least a bit ugly.  I like the current approach since the
> code paths are very much the same for atomic and non-atomic.

I'm not advertising for keeping the old driver interfaces, that would be
much worse. I'm only saying that we'll likely change this a few times, and
we will transition fairly slowly anyway. Reducing the overall churn and
especially reducing the amount of code we might need to fix up if we
change our minds seems like a good approach to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch