From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 12/17] drm: convert crtc to properties/state Date: Mon, 26 May 2014 16:56:31 +0200 Message-ID: <20140526145631.GV14357@phenom.ffwll.local> References: <1400956226-28053-1-git-send-email-robdclark@gmail.com> <1400956226-28053-13-git-send-email-robdclark@gmail.com> <20140526093110.GC14357@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f44.google.com (mail-wg0-f44.google.com [74.125.82.44]) by gabe.freedesktop.org (Postfix) with ESMTP id B0BA56E5C7 for ; Mon, 26 May 2014 07:56:35 -0700 (PDT) Received: by mail-wg0-f44.google.com with SMTP id a1so7988078wgh.15 for ; Mon, 26 May 2014 07:56:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On Mon, May 26, 2014 at 07:35:45AM -0400, Rob Clark wrote: > On Mon, May 26, 2014 at 5:31 AM, Daniel Vetter wrote: > >> +struct drm_crtc_state * > >> +drm_atomic_get_crtc_state(struct drm_crtc *crtc, struct drm_atomic_state *a) > >> +{ > >> + struct drm_crtc_state *cstate; > >> + int ret; > >> + > >> + cstate = a->cstates[crtc->id]; > >> + > >> + if (!cstate) { > >> + ret = drm_modeset_lock(&crtc->mutex, &a->acquire_ctx); > >> + if (ret) > >> + return ERR_PTR(ret); > >> + > >> + cstate = drm_crtc_create_state(crtc); > >> + if (!cstate) > >> + return ERR_PTR(-ENOMEM); > >> + init_crtc_state(crtc, cstate, a); > >> + a->crtcs[crtc->id] = crtc; > >> + a->cstates[crtc->id] = cstate; > >> + > >> + /* we'll need it later, so make sure we have state > >> + * for primary plane too: > >> + */ > >> + drm_atomic_get_plane_state(crtc->primary, a); > > > > I haven't figured out why. With primary planes I don't really see a need > > for this. If we need it to implement the legacy setcrtc interface, then > > that should be done there, not here. > > > well, if you sort out how to disable primary helper plane, then yes, > you are right :-) > > see commit_crtc_state() Imo bail when we have a crtc with NULL primary plane in crtc_commit (and wont disable the entire crtc ofc). Also I think your current commit_crtc should be pushed down into the crtc helpers - it's not going to do any good for i915. But I guess until we have a real user of the atomic interface (i.e. updates more than 1 crtc) like the fb helper code we don't need this yet. But as soon as we update more than one crtc we _must_ push it down into the crtc helpers or the i915 machinery - if you try to enable crtcs which need resources from crtcs which aren't yet off this is simply going to fail. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch