Re: [PATCH 10/17] drm: Atomic crtc/connector updates using crtc/plane helper interfaces

From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 10/17] drm: Atomic crtc/connector updates using crtc/plane helper interfaces
Date: Wed, 5 Nov 2014 22:44:13 +0100	[thread overview]
Message-ID: <20141105214413.GW26941@phenom.ffwll.local> (raw)
In-Reply-To: <CAOw6vbJ2XYktxCdWKm7EhizP7=GUHfj2qUWxCBgXKur0p2DG_Q@mail.gmail.com>

I've applied all the other nits, replies to the more interesting bits
below.

On Wed, Nov 05, 2014 at 01:53:48PM -0500, Sean Paul wrote:
> On Sun, Nov 02, 2014 at 02:19:23PM +0100, Daniel Vetter wrote:
> > + if (new_encoder != connector_state->best_encoder) {
> 
> nit: If you just returned early when the encoder doesn't change, you can save
> indentation and line breaks.

Hm, that would mean either a goto (I don't like that for non-exceptional
control flow) or duplicating the debug output. I'll go with the latter and
frob it a bit.

> > + return ret;
> > +
> > + has_connectors = drm_atomic_connectors_for_crtc(state,
> > + crtc);
> > +
> > + if (crtc_state->enable != has_connectors) {
> 
> This makes me a little nervous. Even though has_connectors is a bool,
> drm_atomic_connectors_for_crtc returns an int, and this seems like something
> that someone might "fix" in the future.
> 
> [PATCH] drm/atomic: Use proper type for drm_atomic_connectors_for_crtc

Hm, yeah. I'll rename to num_connectors and add a !!. The idea of
returning an int and not just a bool is that drivers might care if there's
more than one, i.e. for cloned setups. At least i915 has some special
considerations for that in some cases.

> > + ret = drm_crtc_vblank_get(crtc);
> > + if (ret != 0)
> > + continue;
> > +
> > + old_crtc_state->enable = true;
> > + old_crtc_state->last_vblank_count = drm_vblank_count(dev, i);
> 
> I think you should collect last_vblank_count before drm_crtc_vblank_get

vblank_get should block on anything, and I'm not sure whether the
drm_vblank_count can't just fall over if vblank_get fails (since
vblank_get before vblank_count is the pattern drm_irq.c uses iirc). So I'd
prefer it this way round just for paranoia. So I think I'll stick with
this scheme. The waiting is only done once we've grabbed all the vblank
count, and that's the important part to ensure we don't wait for too long.

This really is just all part of the "generic code has no idea about the
dpms state of a crtc". I'll plan to fix that with the missing dpms on
atomic pieces.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel