[Intel-gfx] [PATCH 04/42] drm/i915: use intel_crtc_control everywhere

[Daniel Vetter]

On Tue, May 12, 2015 at 02:06:39PM +0200, Maarten Lankhorst wrote:
> Op 11-05-15 om 19:11 schreef Daniel Vetter:
> > On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote:
> >> @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
> >>  	enum intel_display_power_domain domain;
> >>  	unsigned long domains;
> >>  
> >> +	if (enable == intel_crtc->active)
> >> +		return;
> >> +
> >> +	if (enable && !crtc->state->enable)
> >> +		return;
> > I think we only need to check for !state->enable here. Changing dpms while
> > the crtc is fully of is totally legit. And at least -modesetting loves to
> > do just that iirc.
> >
> > I'll will be caught by state->active implying state->enable, but that's
> > hard to read imo.
> > -Daniel
> As discussed on irc it's not. :-)

Hm there's actually a bug in drm_atomic_helper_connector_dpms I think ...
we seem to unconditionally update crtc_state->active.

Oh I'm missing that ->enable == false also implies that no connectors are
connected to the crtc, so we can't ever end up setting this to true. So
indeed changing active while enable == false is impossible.

With that is it really possible to see have that early return at all?
Should we wrap it in a WARN_ON as impossible?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch