Re: [PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed
Date: Tue, 7 Jul 2015 14:03:39 +0200	[thread overview]
Message-ID: <20150707120338.GN7568@phenom.ffwll.local> (raw)
In-Reply-To: <559BA474.6040805@linux.intel.com>

On Tue, Jul 07, 2015 at 12:05:40PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 10:59 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 09:08:12AM +0200, Maarten Lankhorst wrote:
> >> @@ -373,7 +375,17 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >>  		if (crtc->state->enable != crtc_state->enable) {
> >>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
> >>  					 crtc->base.id);
> >> +
> >> +			/*
> >> +			 * For clarity this assignment is done here, but
> >> +			 * enable == 0 is only true when there are no
> >> +			 * connectors and a NULL mode.
> >> +			 *
> >> +			 * The other way around is true as well. enable != 0
> >> +			 * iff connectors are attached and a mode is set.
> >> +			 */
> >>  			crtc_state->mode_changed = true;
> > I'd drop this one so that connectors_changed and mode_changed are truly
> > orthogonal. Also ->enable implies connectors changed since we do check
> > that there's only connected connectors if the crtc is on. Needs kerneldoc
> > update too ofc.
> 
> They are orthogonal, think of this case:
> 
> 1. crtc previously enabled, connector removed, mode stays same ->
> 	connector_changed = true, mode_changed = false
> 
> 2. crtc previously enabled, connectors stay the same, different mode -> 
> 	connectors_changed = false, mode_changed = true
> 
> The following is enforced by the checks:
> crtc disabled, implies 0 connectors, no mode.
> crtc enabled implies > 0 connectors, and a mode.
> 
> Hence the connectors_changed and mode_changed here are for documentation purposes only. :)
> 
> So if someone wonders what happens when enable is changed they don't have to dig through
> the entire drm_atomic_helper_check_modeset function and still not be sure if it's coincidence
> or not. 
> 
> You're right about the kerneldoc, I'll fix it.

Right, I guess I should have read your comment properly and disregarded
the kerneldoc ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel