Re: [PATCH 3/4] drm/i915: Move pll power state to crtc power domains.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2016-03-01 at 17:03 +0000, R, Durgadoss wrote:
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Maarten Lankhorst
> > Sent: Monday, February 29, 2016 6:22 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 3/4] drm/i915: Move pll power state to crtc
> > power domains.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 6e3b8a1f7dd3..6f2dd3192bac 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1912,8 +1912,6 @@ static void intel_enable_shared_dpll(struct intel_crtc
> > *crtc)
> > 	}
> > 	WARN_ON(pll->on);
> > 
> > -	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > -
> 
> Looking from upfront link train perspective, my initial thought was that
> direct users of this _enable/disable_shared_dpll may have issues..
> But then realized, before we get to _dpll functions, we do a get on
> Port power domains, so, hopefully, that should keep us going.

Seems like that should work on BXT, but that is not how the power domain API is
supposed to be used, as far as I know. You should actually get/put the power
domain when you need it, so that the code is not dependent on side effects and
is tolerant to a new platform having that specific power domain in a different
power well.

With that being said, I'm not exactly sure what is POWER_DOMAIN_PLLS is supposed
to represent. It seems commit bd2bb1b9a1c8 ("drm/i915: add POWER_DOMAIN_PLLS")
added it to fix a shared dpll state mismatch in SandyBridge, but with the
current code the check for the power domain in ibx_pch_dpll_get_hw_state() will
always return true. Later patches show some connection of that power domain with
CDCLK, but I'm failing to grasp it.

Thanks,
Ander

> 
> So,
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
> 
> Thanks,
> Durga
> 
> > 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
> > 	pll->enable(dev_priv, pll);
> > 	pll->on = true;
> > @@ -1952,8 +1950,6 @@ static void intel_disable_shared_dpll(struct
> > intel_crtc *crtc)
> > 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
> > 	pll->disable(dev_priv, pll);
> > 	pll->on = false;
> > -
> > -	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > }
> > 
> > static void ironlake_enable_pch_transcoder(struct drm_i915_private
> > *dev_priv,
> > @@ -5347,6 +5343,9 @@ static unsigned long get_crtc_power_domains(struct
> > drm_crtc *crtc,
> > 		mask |= BIT(intel_display_port_power_domain(intel_encoder));
> > 	}
> > 
> > +	if (crtc_state->shared_dpll != DPLL_ID_PRIVATE)
> > +		mask |= BIT(POWER_DOMAIN_PLLS);
> > +
> > 	return mask;
> > }
> > 
> > @@ -15775,9 +15774,6 @@ static void intel_modeset_readout_hw_state(struct
> > drm_device *dev)
> > 
> > 		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
> > 			      pll->name, pll->config.crtc_mask, pll->on);
> > -
> > -		if (pll->config.crtc_mask)
> > -			intel_display_power_get(dev_priv,
> > POWER_DOMAIN_PLLS);
> > 	}
> > 
> > 	for_each_intel_encoder(dev, encoder) {
> > --
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx