[PATCH 00/19] atomic, remixed

[Rob Clark]

On Sun, Jul 27, 2014 at 5:41 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> Hi all,
>
> So I've figured instead of just talking I should draw up my ideas for atomic
> helpers and related stuff in some draft patches. Major differences compared to
> Rob's series:
>
> - The software side state update is done synchronously, at least when using the
>   helper code. Drivers are free to do whatever they want, as usual. This has a
>   bunch of nice upsides:
>
>   + All the tricky locking dances can be abolished, and if the ordering and
>     synchronization is done correctly async worker threads don't need any
>     modeset state locking at all. I'm fairly sure that Rob's scheme won't blow
>     up, but I much prefer we don't need it at all.
>
>   + In Rob's patches any atomic operation must wait for outstanding updates to
>     complete. Otherwise the software state isn't committed yet. With the
>     synchronous updates we can immediately proceed with the state construction
>     and verification and will only block right before applying the state update.
>     And even for that drivers can implement cancellation of the previous update.

fwiw, the rough plan here for my version was to (since state was
refcnt'd) eventually allow things to chain up (ie. copying N+1'th
state from N'th state, rather than mode objects themselves) which
could be done without locks for check-only..

That said, I'm not super sold on the idea that userspace needs to be
able to queue up multiple frames ahead.. but it was something that
people ask for from time to time..

>     This has the nice benefit that check-only operations (which compositors need
>     to do right after having completed the current frame to know how to render
>     the next one) can be done without blocking for completion of the previous
>     update.
>
> - Internal interfaces for atomic updates don't need to go through properties,
>   but can directly set the state. Drivers can always compare updates in their
>   ->check hook later on, so we don't lose expressiveness in the interface. But
>   the resulting code in the callers looks much better.

hmm, in cases where drivers have to fwd one property to another object
or other sort of side effect, I think my way was nicer, imho ;-)

Sure, we can figure it out in check_hook later, but it is a bit
awkward.  And means we need to be careful about sequence of checks
(potentially having to re-check) of different objects.  Looks worse to
me ;-)

>   In general I've ditched a lot of interfaces where drivers could overwrite
>   default behaviour and boiled down the interface two state handling functions
>   (duplicate+destroy), setting driver-private properties and check/commit.
>
> - There is a clear helper separation between core code which should be used to
>   use the atomic interface to drivers, and helper functions. Core never uses
>   helper functions so that we can keep the clear separation we have in all other
>   places in the kms api.
>
>   There's a set of helpers sitting in-between to implement legacy interfaces on
>   top of atomic. Those _only_ use the core atomic interfaces.
>
> - I've added a new set of plane helpers. Mostly this was motivated to make the
>   atomic helpers work on less stellar hardware where you can't just stream the
>   state and then atomically commit with some GO bits. But those helpers should
>   also be useful for more gradual transitions of drivers to the atomic
>   interface. Since a requirement for atomic is to have universal plane support
>   they can be used bare-bones just for that, without all the other atomic
>   baggage (i.e. all the state tracking for crtcs/connectors).
>
> - State tracking for connectors. i915 has piles of relevant connector
>   properties, which we want to save/restore in atomic ops (e.g. hdmi metadata
>   and stuff).
>
> - fbdev panic locking is implemented with trylock instead of nolock.
>
> - I've thought a bit about resume. See the proposed state handling for the
>   default reset functions and the addition of a plane reset callback.
>
> There's also a metric pile of stuff missing:
>
> - It's completely untested (except for the parts touching currently used code)
>   since I lack suitable hw. Converting i915 is just a bit too much of a pain for
>   a quick w/e code draft ;-)
>
> - I didn't add all the properties for things currently exposed through ioctl
>   structures. Since the interface here doesn't force internal code to use
>   properties for everything (which really makes things look much better) we
>   don't have a need for that until we merge the actual atomic ioctl. So I left
>   that out.
>
> - I didn't write code for the fbdev helper to use atomic. The w/e ran out of
>   time for that ...
>
> - Kerneldoc is occasionally not updated or still missing.

Unless I'm missing something you also completely lost all the core
crtc and plane check code (except for what you left in legacy
codepaths.. tbh, this was one thing that I liked about my approach is
it unified all the core error checking for legacy and atomic paths..

Anyways, I'm still looking through it.. so may have some more comments later.

BR,
-R


> Anyway I think this draft code is good enough to explain how I'd like to address
> some of the concerns I have with the current atomic code. Comments, flames and
> ideas highly welcome. And if anyone is insane enought to try this on real
> hardware, that would certainly be interesting and I'm very much willing to help
> out as much as possible ... i915 just really isn't suitable since it won't be
> using the helpers, and all the other hw I have here doesn't support planes.
>
> Cheers, Daniel
>
> Daniel Vetter (18):
>   drm: Add atomic driver interface definitions for objects
>   drm: Add drm_plane/connector_index
>   drm: Move modeset_lock_all helpers to drm_modeset_lock.[hc]
>   drm: Handle legacy per-crtc locking with full acquire ctx
>   drm: Global atomic state handling
>   drm: Add atomic/plane helpers
>   drm/irq: Implement a generic vblank_wait function
>   drm/i915: Use generic vblank wait
>   drm/plane-helper: transitional atomic plane helpers
>   drm/crtc-helper: Transitional functions using atomic plane helpers
>   drm: Atomic crtc/connector updates using crtc helper interfaces
>   drm: Move ->old_fb from crtc to plane
>   drm/atomic-helper: implementatations for legacy interfaces
>   drm/plane-helper: Add async mode to prepare_fb
>   drm/atomic-helpers: document how to implement async commit
>   drm/atomic-helper: implement ->page_flip
>   drm: trylock modest locking for fbdev panics
>   drm/atomic-helpers: functions for state duplicate/destroy/reset
>
> Ville Syrjälä (1):
>   drm: Add drm_crtc_vblank_waitqueue()
>
>  Documentation/DocBook/drm.tmpl       |    1 +
>  drivers/gpu/drm/Makefile             |    4 +-
>  drivers/gpu/drm/drm_atomic.c         |  529 ++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c  | 1570 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c           |  194 ++---
>  drivers/gpu/drm/drm_crtc_helper.c    |  139 +++
>  drivers/gpu/drm/drm_fb_helper.c      |   10 +-
>  drivers/gpu/drm/drm_irq.c            |   45 +
>  drivers/gpu/drm/drm_modeset_lock.c   |  204 ++++-
>  drivers/gpu/drm/drm_plane_helper.c   |  181 +++-
>  drivers/gpu/drm/i915/intel_display.c |   41 +-
>  include/drm/drmP.h                   |   13 +
>  include/drm/drm_atomic.h             |   63 ++
>  include/drm/drm_atomic_helper.h      |   98 +++
>  include/drm/drm_crtc.h               |  208 ++++-
>  include/drm/drm_crtc_helper.h        |   13 +
>  include/drm/drm_modeset_lock.h       |   16 +
>  include/drm/drm_plane_helper.h       |   31 +
>  18 files changed, 3188 insertions(+), 172 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_atomic.c
>  create mode 100644 drivers/gpu/drm/drm_atomic_helper.c
>  create mode 100644 include/drm/drm_atomic.h
>  create mode 100644 include/drm/drm_atomic_helper.h
>
> --
> 2.0.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel