Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active
Date: Thu, 22 Jan 2015 17:15:26 +0100	[thread overview]
Message-ID: <20150122161526.GW10113@phenom.ffwll.local> (raw)
In-Reply-To: <20150122155654.GQ19354@intel.com>

On Thu, Jan 22, 2015 at 05:56:54PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 22, 2015 at 04:36:21PM +0100, Daniel Vetter wrote:
> > This is the infrastructure for DPMS ported to the atomic world.
> > Fundamental changes compare to legacy DPMS are:
> > 
> > - No more per-connector dpms state, instead there's just one per each
> >   display pipeline. So if you clone either you have to unclone first
> >   if you only want to switch off one screen, or you just switch of
> >   everything (like all desktops do). This massively reduces complexity
> >   for cloning since now there's no more half-enabled cloned configs to
> >   consider.
> > 
> > - Only on/off, dpms standby/suspend are as dead as real CRTs. Again
> >   reduces complexity a lot.
> > 
> > Now especially for backwards compat the really important part for dpms
> > support is that dpms on always succeeds (except for hw death and
> > unplugged cables ofc). Which means everything that could fail (like
> > configuration checking, resources assignments and buffer management)
> > must be done irrespective from ->active. ->active is really only a
> > toggle to change the hardware state. More precisely:
> > 
> > - Drivers MUST NOT look at ->active in their ->atomic_check callbacks.
> >   Changes to ->active MUST always suceed if nothing else changes.
> > 
> > - Drivers using the atomic helpers MUST NOT look at ->active anywhere,
> >   period. The helpers will take care of calling the respective
> >   enable/modeset/disable hooks as necessary. As before the helpers
> >   will carefully keep track of the state and not call any hooks
> >   unecessarily, so still no double-disables or enables like with crtc
> >   helpers.
> > 
> > - ->mode_set hooks are only called when the mode or output
> >   configuration changes, not for changes in ->active state.
> > 
> > - Drivers which reconstruct the state objects in their ->reset hooks
> >   or through some other hw state readout infrastructure must ensure
> >   that ->active reflects actual hw state.
> > 
> > This just implements the core bits and helper logic, a subsequent
> > patch will implement the helper code to implement legacy dpms with
> > this.
> 
> So we now have multiple conflicting properties for the same thing? I
> don't particularly relish that idea.
> 
> I suppose a rather easy way to way to deal with that would be to hide
> the dpms properties from non-atomic clients, and conversly hide the
> active property from legacy clients.

Which is pretty much what I do - you can only access the per-crtc ACTIVE
property from the atomic ioctl, the per-connector dpms property is _not_
exposed through atomic. Vice-versa legacy clients wont see atomic
properties (and hence this new one) either.

Is that good enough?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx