Re: Atomic mode-setting drivers

From: Daniel Vetter <daniel@ffwll.ch>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: "Zach Reizner" <zachr@google.com>,
	"Stéphane Marchesin" <marcheu@google.com>,
	"Emil Velikov" <emil.l.velikov@gmail.com>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: Atomic mode-setting drivers
Date: Thu, 24 Mar 2016 08:00:49 +0100	[thread overview]
Message-ID: <20160324070049.GE2510@phenom.ffwll.local> (raw)
In-Reply-To: <CADnq5_MRRLejGa7RwFR636_rSr+HeD5bRwwDnGTLuOtz-ga8ww@mail.gmail.com>

On Fri, Mar 04, 2016 at 05:20:00PM -0500, Alex Deucher wrote:
> Code complexities and internal abstractions aside, we've implemented
> the API and tested it and it appears to work.  What sort of additional
> semantics does atomic imply that you don't think DAL would handle?

Michel asked me to follow up here. I thought I've followed up on irc.
Anyway the big problem and reason why I didnt' fully dig into things are
that reading 100kloc for an atomic review when it's clear the code overall
needs some serious love is something I didn't want to do. I'm still up to
reading things once they look a bit more reasonable, and it's not
massively more work than all the other atomic drivers/conversions I looked
at.

So without looking again, with a few months of brain memory degradation:
- The glue looked fairly incomplete, e.g. it wasn't using all the
  legacy2atomic helpers we have. Which likely means it wasn't really
  tested much.
- Not universal planes, hence atomic plane updates not possible.
- It did roll it's own commit (which is how this is meant to be really in
  the end), but I did not see the a clear reason, and it didn't seem to
  have been closely modelled after atomic helpers. Not necessarily bad,
  just increases chances that some of the semantics are wrong.

And the more fundamental thing, but the one I couldn't check because
abstraction:
- Doesn't seem to stage derived state needed to check limits (e.g. clocks,
  memory bandwidth) in something subclassing drm_*_state structures.
  Either that means you're rolling your own atomic machinery (no-go) or
  you get it wrong (no-go). But since DAL completely forgoes drm
  structures I didn't have a map to read the code and got lost.
- It looked liked DAL works with a try_commit()/rollback() model, but
  atomic requires a check()/commit() model. And the check phase is not
  allowed to change _any_ persistent state (whether sw or hw). That's why
  atomic needs to stage everything that might change, and which might need
  to be checked up-front in these free-standing state structures.

I think for me to be able to actually give you a clear answer here we'd
need 2 things:
- Handle all the list of cleanup tasks to get rid of reinvented code.
- Embed drm_* structs in corresponding DAL structs to really link concepts
  together, and have some kind of map to be able to read the code. I think
  only with that it's possible to have a clear idea whether DAL needs to
  be redesigned to be able to implement atomic, or not.

Again: I din't look at the code, this is purely from memory.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel