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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jan 22, 2015 at 05:15:26PM +0100, Daniel Vetter wrote:
> 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.

Oh, OK. I didn't see anything obvious that would filter out the dpms
prop for non-atomic, nor do I see code to reject stuff in setprop/getprop
ioctls etc. But I suppose such stuff may be in flight and I've just not
paid attention.

> Is that good enough?

I suppose.

Another thing that came to mind wrt. the 'this can't fail rule' was
fb pinning. So is the rule now going to be that we need to pin even
when ACTIVE==false, or otherwise make sure all the FBs can be pinned
simultaneosly? If we don't want to everything pinned all the time when
ACTIVE==false, then we would need to prevent pinning of anything else
in the meantime to make sure we don't run out of address space.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx