[Intel-gfx] [PATCH v2 07/17] drm/i915: Use crtc_state->active instead of crtc_state->enable

[Daniel Vetter]

On Mon, May 18, 2015 at 06:35:59PM +0200, Maarten Lankhorst wrote:
> Op 18-05-15 om 17:30 schreef Daniel Vetter:
> > On Wed, May 13, 2015 at 10:23:37PM +0200, Maarten Lankhorst wrote:
> >> crtc_state->enable means a crtc is configured, but it may be turned
> >> off for dpms. Until the previous commit crtc_state->active was not
> >> updated on crtc off, but now that we do we should use that for tracking
> >> whether a crtc is scanning out or not.
> >>
> >> At this point crtc->active should mirror crtc_state->active,
> >> so some paranoia from the crtc_disable functions can be removed.
> >>
> >> Note that intel_set_mode_setup_plls still checks for ->enable,
> >> because all resources that are needed have to be calculated, so
> >> dpms changes will still succeed.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > A few detail comments below.
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
> >>  drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
> >>  2 files changed, 23 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 557af8877a2e..ca457317a8ac 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	if (!crtc->state->enable) {
> >> +	if (!crtc->state->active) {
> > This change looks unjustified I think.
> Why? Can you get a vblank on crtc that is dpms off? I think not..
> 
> Either way later on I use hwmode->crtc_clock which renders this moot.
> >> <snip>
> >> @@ -5742,7 +5738,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
> >>  
> >>  	/* add all active pipes to the state */
> >>  	for_each_crtc(state->dev, crtc) {
> >> -		if (!crtc->state->enable)
> >> +		if (!crtc->state->active)
> > This is a functional change to the cdclk code and might break it and/or
> > conflict with the ongoing cdclk work from Ville/Mika. Definitely needs to
> > be split out.
> No this function just sets mode_changed on all active crtc's.
> 
> This is done to turn them off while changing the clock. In case they're already turned off you don't have to turn them off.
> 
> >>  			continue;
> >>  
> >>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >> @@ -5752,7 +5748,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
> >>  
> >>  	/* disable/enable all currently active pipes while we change cdclk */
> >>  	for_each_crtc_in_state(state, crtc, crtc_state, i)
> >> -		if (crtc_state->enable)
> >> +		if (crtc_state->active)
> >>  			crtc_state->mode_changed = true;
> > Same here.
> >
> > Hm, aside of all that maybe we should drop vlv_modeset_global_pipes and
> > instead just look at crtc_state->mode_changed? That way we don't need to
> > duplicate the same checks twice, once to set ->mode_changed and once to
> > for the prepare_pipes mask. Or is that duplication already getting
> > removed?
> I think we could get rid of a lot of those extra loops if we want to later,
> but for now readability is more important.

Hm makes sense with your replies - there's a few cases indeed where we
need to switch from checking ->enable to ->active if we start using the
mode_set machinery for dpms. I think it'd be good to explain that a bit in
the commit message for all the different cases, so that reviewers can go
hunting for more and make sure there aren't ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch