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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active
Date: Thu, 22 Jan 2015 19:59:45 +0200	[thread overview]
Message-ID: <20150122175945.GS19354@intel.com> (raw)
In-Reply-To: <CAKMK7uHzMbAN+URBkTY+o4TjKQzB4=pi-FGEQePHAEN5wj5Awg@mail.gmail.com>

On Thu, Jan 22, 2015 at 06:38:32PM +0100, Daniel Vetter wrote:
> On Thu, Jan 22, 2015 at 5:42 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> 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.
> 
> On the atomic side the dpms prop is simple not decoded. The driver
> /could/ do that itself, but that would be really pointless. In
> getprops atomic ioctls are filtered out for non-atomic clients. Which
> means that an atomic client could do a dpms on the connector through
> the legacy setprop ioctl, but that would be rather stupid.

Well I'd rather it refused the entire thing. Less weird interactions to
worry about if we're strict and block all the silly stuff at the top.

> 
> >> 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.
> 
> fb pinning is done irrespective of the state of active. So if you
> update the fb while the display pipe is off the helpers will upin/pin
> correctly. Imo that's totally fine, since failing to pin when setting
> the display back to active really isn't a great thing. And we need to
> be able to tell userspace when something has gone wrong with the
> pinning (e.g. due to giant framebuffer or something).

Yeah that was pretty much my original idea too. I suppose now that's the
call comes from the core it's a bit less likely to be messed up again.

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