Re: [RFCv1 10/12] drm: convert crtc to properties/state

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 07, 2013 at 10:29:06AM -0400, Rob Clark wrote:
> On Mon, Oct 7, 2013 at 10:19 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Oct 07, 2013 at 10:03:01AM -0400, Rob Clark wrote:
> >> On Mon, Oct 7, 2013 at 9:39 AM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Sat, Oct 05, 2013 at 08:45:48PM -0400, Rob Clark wrote:
> >> >> Break the mutable state of a crtc out into a separate structure
> >> >> and use atomic properties mechanism to set crtc attributes.  This
> >> >> makes it easier to have some helpers for crtc->set_property()
> >> >> and for checking for invalid params.  The idea is that individual
> >> >> drivers can wrap the state struct in their own struct which adds
> >> >> driver specific parameters, for easy build-up of state across
> >> >> multiple set_property() calls and for easy atomic commit or roll-
> >> >> back.
> >> >
> >> > I'm not sure how we're going to handle the mismatch in the behaviour of
> >> > the atomic modeset vs. the current setcrtc.
> >> >
> >> > The problem is that setcrtc ignore all kinds of conflicting
> >> > crtc->connector assignments, and just overwrites whatever was there
> >> > with the latest configuration. For the atomic case we want to return an
> >> > error if there's a conflict.
> >>
> >> Hmm, well currently we preserve the setcrtc behavior because it ends
> >> up going through crtc helpers (or whatever the driver uses).  So
> >> should be fine for setcrtc, but probably not what we want for atomic
> >> ioctl.
> >>
> >> I suppose we could solve some of this via internal flags, ie
> >> .atomic_begin(dev, LEGACY_SETCRTC_CHECK_MODE)
> >>
> >> it is a bit ugly, but it keeps the ugly in core and drivers don't have
> >> to care as much about it (which is my main concern)
> >
> > Well, it could be an entirely separate .legacy_crap() hook or something
> > that happens just before .check().
> 
> yeah, that could work too.. I'll think about it a bit and see what I
> can come up with

Why can't the legacy setcrtc ioctl implemenation just fiddle the current
state out of the connector/encoder pointers and then make a relevant
atomic call? So if it notices that some connectors would be disabled it
just adds an explicit clearing of the connector->crtc link  to the atomic
request.

> >> > And another thing is the DPMS handling. The
> >> > current API forces DPMS on when you do a modeset, but for the atomic
> >> > case I want to keep things nice and clean and avoid doing such silly
> >> > things.
> >>
> >> I guess the easy thing is to set DPMS property in setcrtc too ;-)
> >
> > That's what we do, but I don't want it for atomic.
> 
> yeah, I need think about how that could work if we are still using
> .set_config() from the commit, but I guess I should be able to sort
> out something..

Similarly here the legacy setcrtc simply needs to supply a dpms=ON request
for all connectors that are in the setcrtc request.

With those changes drivers can get rid of these hacks and legacy ioctl
quirks internally and we consolidate them into one single place ...

The real crux here is getting legacy pageflip semantics out of the new
interface. In any case I think if we're left with some non-helper
set_cursor, set_plane, setfoo/bar from yonder the new atomic interface is
probably not good enough. After all anything that current userspace does
with these ioctls it probably wants to keep on doing in the new world. So
imo making sure that all the old ioctls work in terms of the new driver
interface is a nice real-world test of their suitability.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch