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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Rob Clark <robdclark@gmail.com>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: Re: [RFCv1 10/12] drm: convert crtc to properties/state
Date: Mon, 7 Oct 2013 17:19:38 +0300	[thread overview]
Message-ID: <20131007141938.GM9395@intel.com> (raw)
In-Reply-To: <CAF6AEGuG=BpgMne1eqaZm9CsH5qdc9R2b7hACcH=yoBP_JGQLQ@mail.gmail.com>

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().

> 
> > 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.

> 
> BR,
> -R
> 
> > So I don't think we can simply convert the current modeset codepaths to
> > call into the atomic code. We basically need another version of the
> > check function, or another step that happens before .check only in the
> > setcrtc case which eliminates the conflicts in a way that matches the
> > current setcrtc behaviour.
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC