[PATCH 1/3] drm/i915: Add missing '\n' to cdclk debug message

[PATCH 1/3] drm/i915: Add missing '\n' to cdclk debug message

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c51d950..d83a7f1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4621,7 +4621,7 @@ static void vlv_update_cdclk(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	dev_priv->vlv_cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
-	DRM_DEBUG_DRIVER("Current CD clock rate: %d kHz",
+	DRM_DEBUG_DRIVER("Current CD clock rate: %d kHz\n",
 			 dev_priv->vlv_cdclk_freq);
 
 	/*
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Grab rpm ref when changing cdclk on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We need to access the gunit mailbox when changing the cdclk frequency.
Currently we update the power wells only after chancing cdclk, so the
device might be suspended when we have to frob it. Grab a runtime PM
reference to make sure it's awake.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d83a7f1..149310b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4812,10 +4812,12 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
 	int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
 
 	if (req_cdclk != dev_priv->vlv_cdclk_freq) {
+		intel_runtime_pm_get(dev_priv);
 		if (IS_CHERRYVIEW(dev))
 			cherryview_set_cdclk(dev, req_cdclk);
 		else
 			valleyview_set_cdclk(dev, req_cdclk);
+		intel_runtime_pm_put(dev_priv);
 	}
 
 	modeset_update_crtc_power_domains(dev);
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Cache HPLL frequency on VLV/CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We need the HPLL frequency when calculating cdclk. Currently we read
that out from the hardware every single time, which isn't going to fly
very well if the device is runtime suspended. So cache the HPLL
frequency in dev_priv and use the cached value.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e476b5..68a3509 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1545,6 +1545,7 @@ struct drm_i915_private {
 
 	unsigned int fsb_freq, mem_freq, is_ddr3;
 	unsigned int vlv_cdclk_freq;
+	unsigned int hpll_freq;
 
 	/**
 	 * wq - Driver workqueue for GEM.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 149310b..6fbae00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4660,10 +4660,9 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	if (cdclk == 400000) {
-		u32 divider, vco;
+		u32 divider;
 
-		vco = valleyview_get_vco(dev_priv);
-		divider = DIV_ROUND_CLOSEST(vco << 1, cdclk) - 1;
+		divider = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
 
 		mutex_lock(&dev_priv->dpio_lock);
 		/* adjust cdclk divider */
@@ -4742,8 +4741,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
 				 int max_pixclk)
 {
-	int vco = valleyview_get_vco(dev_priv);
-	int freq_320 = (vco <<  1) % 320000 != 0 ? 333333 : 320000;
+	int freq_320 = (dev_priv->hpll_freq <<  1) % 320000 != 0 ? 333333 : 320000;
 
 	/* FIXME: Punit isn't quite ready yet */
 	if (IS_CHERRYVIEW(dev_priv->dev))
@@ -5452,7 +5450,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 static int valleyview_get_display_clock_speed(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int vco = valleyview_get_vco(dev_priv);
 	u32 val;
 	int divider;
 
@@ -5460,6 +5457,9 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
 	if (IS_CHERRYVIEW(dev))
 		return 400000;
 
+	if (dev_priv->hpll_freq == 0)
+		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
+
 	mutex_lock(&dev_priv->dpio_lock);
 	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
 	mutex_unlock(&dev_priv->dpio_lock);
@@ -5470,7 +5470,7 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
 	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
 	     "cdclk change in progress\n");
 
-	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
+	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
 }
 
 static int i945_get_display_clock_speed(struct drm_device *dev)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Grab rpm ref when changing cdclk on VLV/CHV

[Daniel Vetter]

On Tue, Oct 07, 2014 at 05:41:21PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We need to access the gunit mailbox when changing the cdclk frequency.
> Currently we update the power wells only after chancing cdclk, so the
> device might be suspended when we have to frob it. Grab a runtime PM
> reference to make sure it's awake.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Late response because mailman.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d83a7f1..149310b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4812,10 +4812,12 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
>  	int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
>  
>  	if (req_cdclk != dev_priv->vlv_cdclk_freq) {
> +		intel_runtime_pm_get(dev_priv);
>  		if (IS_CHERRYVIEW(dev))
>  			cherryview_set_cdclk(dev, req_cdclk);
>  		else
>  			valleyview_set_cdclk(dev, req_cdclk);
> +		intel_runtime_pm_put(dev_priv);

So shouldn't we move the cdclock within modeset_update_crtc_power_domains?
That one first grabs new domains then drops old ones to make sure we don't
ever touch the hw in the off state.

Also vlv noob question: Does the cdclock setting survive a runtime suspend
transition, i.e. when we'd routine suspend right here between the cdclock
change and grabbing power wells (e.g. when turning everything on).
-Daniel

>  	}
>  
>  	modeset_update_crtc_power_domains(dev);
> -- 
> 2.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Grab rpm ref when changing cdclk on VLV/CHV

[Ville Syrjälä]

On Mon, Oct 27, 2014 at 09:48:27AM +0100, Daniel Vetter wrote:
> On Tue, Oct 07, 2014 at 05:41:21PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We need to access the gunit mailbox when changing the cdclk frequency.
> > Currently we update the power wells only after chancing cdclk, so the
> > device might be suspended when we have to frob it. Grab a runtime PM
> > reference to make sure it's awake.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Late response because mailman.
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d83a7f1..149310b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4812,10 +4812,12 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
> >  	int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
> >  
> >  	if (req_cdclk != dev_priv->vlv_cdclk_freq) {
> > +		intel_runtime_pm_get(dev_priv);
> >  		if (IS_CHERRYVIEW(dev))
> >  			cherryview_set_cdclk(dev, req_cdclk);
> >  		else
> >  			valleyview_set_cdclk(dev, req_cdclk);
> > +		intel_runtime_pm_put(dev_priv);
> 
> So shouldn't we move the cdclock within modeset_update_crtc_power_domains?
> That one first grabs new domains then drops old ones to make sure we don't
> ever touch the hw in the off state.

Yeah that would be a nicer solution, or maybe split 
modeset_update_crtc_power_domains() into pre and post funcs?

There was also a patch somewhere in the list that frobbed with some
PFI credits or somesuch depending on the cdclk frequency, and I believe
that may require the disp2d power well since the register could live
there. So this rpm ref solution might not be enough in the end.

> 
> Also vlv noob question: Does the cdclock setting survive a runtime suspend
> transition, i.e. when we'd routine suspend right here between the cdclock
> change and grabbing power wells (e.g. when turning everything on).

I would expect it does survive since it's just a register value we write
into punit/cck. But I'm not entirely sure what would happen on VLV if
we choose 400MHz cdclk and then the entire soc enters s0ix. In that case
I could imagine that Punit would reissue the cdclk request to cck on
resume, which would then cause cdclk to come back as 320/333 MHz instead
of the 400MHz we were expecting. That too is just a theory at this
point.

> -Daniel
> 
> >  	}
> >  
> >  	modeset_update_crtc_power_domains(dev);
> > -- 
> > 2.0.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/3] drm/i915: Move the .global_resources() hook call into modeset_update_crtc_power_domains()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We may need to access various hardware bits in the .global_resources()
hook, so move the call to occur after enabling all the newly required
power wells, but before disabling all the now unneeded wells. This
should guarantee that we have all the sufficient hardware resources
available during the .global_resources() call. And if not, any additional
resources must be explicitly acquired by the .global_resorces() hook.

For instance on VLV/CHV we need to access the gunit mailbox so that we
can talk to punit/cck over sideband. In addition some PFI credit
reprogramming may need to be addes as well, which may require the disp2d
well.

This should also make the power domain refcounts consistent on platforms
which don't have a .global_resource() hook since now they too will
call modeset_update_crtc_power_domains() which will drop the init power.
Previously init power was just left enabled for such platforms.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9ec1ab7..234d54a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4681,6 +4681,9 @@ static void modeset_update_crtc_power_domains(struct drm_device *dev)
 			intel_display_power_get(dev_priv, domain);
 	}
 
+	if (dev_priv->display.modeset_global_resources)
+		dev_priv->display.modeset_global_resources(dev);
+
 	for_each_intel_crtc(dev, crtc) {
 		enum intel_display_power_domain domain;
 
@@ -4908,8 +4911,6 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
 		else
 			valleyview_set_cdclk(dev, req_cdclk);
 	}
-
-	modeset_update_crtc_power_domains(dev);
 }
 
 static void valleyview_crtc_enable(struct drm_crtc *crtc)
@@ -7948,16 +7949,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 	intel_prepare_ddi(dev);
 }
 
-static void snb_modeset_global_resources(struct drm_device *dev)
-{
-	modeset_update_crtc_power_domains(dev);
-}
-
-static void haswell_modeset_global_resources(struct drm_device *dev)
-{
-	modeset_update_crtc_power_domains(dev);
-}
-
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc)
 {
 	if (!intel_ddi_pll_select(crtc))
@@ -10915,8 +10906,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * update the the output configuration. */
 	intel_modeset_update_state(dev, prepare_pipes);
 
-	if (dev_priv->display.modeset_global_resources)
-		dev_priv->display.modeset_global_resources(dev);
+	modeset_update_crtc_power_domains(dev);
 
 	/* Set up the DPLL and any encoders state that needs to adjust or depend
 	 * on the DPLL.
@@ -12587,8 +12577,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.fdi_link_train = ironlake_fdi_link_train;
 	} else if (IS_GEN6(dev)) {
 		dev_priv->display.fdi_link_train = gen6_fdi_link_train;
-		dev_priv->display.modeset_global_resources =
-			snb_modeset_global_resources;
 	} else if (IS_IVYBRIDGE(dev)) {
 		/* FIXME: detect B0+ stepping and use auto training */
 		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
@@ -12596,14 +12584,9 @@ static void intel_init_display(struct drm_device *dev)
 			ivb_modeset_global_resources;
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
-		dev_priv->display.modeset_global_resources =
-			haswell_modeset_global_resources;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.modeset_global_resources =
 			valleyview_modeset_global_resources;
-	} else if (INTEL_INFO(dev)->gen >= 9) {
-		dev_priv->display.modeset_global_resources =
-			haswell_modeset_global_resources;
 	}
 
 	/* Default just returns -ENODEV to indicate unsupported */
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Move the .global_resources() hook call into modeset_update_crtc_power_domains()

[Daniel Vetter]

On Thu, Nov 06, 2014 at 02:49:12PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We may need to access various hardware bits in the .global_resources()
> hook, so move the call to occur after enabling all the newly required
> power wells, but before disabling all the now unneeded wells. This
> should guarantee that we have all the sufficient hardware resources
> available during the .global_resources() call. And if not, any additional
> resources must be explicitly acquired by the .global_resorces() hook.
> 
> For instance on VLV/CHV we need to access the gunit mailbox so that we
> can talk to punit/cck over sideband. In addition some PFI credit
> reprogramming may need to be addes as well, which may require the disp2d
> well.
> 
> This should also make the power domain refcounts consistent on platforms
> which don't have a .global_resource() hook since now they too will
> call modeset_update_crtc_power_domains() which will drop the init power.
> Previously init power was just left enabled for such platforms.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yeah I think that's a lot saner and hopefully allows us to unify the power
domain more. Thanks for the patch, queued for next.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Cache HPLL frequency on VLV/CHV

[Imre Deak]

On Tue, 2014-10-07 at 17:41 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We need the HPLL frequency when calculating cdclk. Currently we read
> that out from the hardware every single time, which isn't going to fly
> very well if the device is runtime suspended. So cache the HPLL
> frequency in dev_priv and use the cached value.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

You could add:
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=82939

where this fixes some subtest failures on VLV.
On 1/3 and 3/3 of this patchset:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e476b5..68a3509 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1545,6 +1545,7 @@ struct drm_i915_private {
>  
>  	unsigned int fsb_freq, mem_freq, is_ddr3;
>  	unsigned int vlv_cdclk_freq;
> +	unsigned int hpll_freq;
>  
>  	/**
>  	 * wq - Driver workqueue for GEM.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 149310b..6fbae00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4660,10 +4660,9 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	if (cdclk == 400000) {
> -		u32 divider, vco;
> +		u32 divider;
>  
> -		vco = valleyview_get_vco(dev_priv);
> -		divider = DIV_ROUND_CLOSEST(vco << 1, cdclk) - 1;
> +		divider = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>  
>  		mutex_lock(&dev_priv->dpio_lock);
>  		/* adjust cdclk divider */
> @@ -4742,8 +4741,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>  static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
>  				 int max_pixclk)
>  {
> -	int vco = valleyview_get_vco(dev_priv);
> -	int freq_320 = (vco <<  1) % 320000 != 0 ? 333333 : 320000;
> +	int freq_320 = (dev_priv->hpll_freq <<  1) % 320000 != 0 ? 333333 : 320000;
>  
>  	/* FIXME: Punit isn't quite ready yet */
>  	if (IS_CHERRYVIEW(dev_priv->dev))
> @@ -5452,7 +5450,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  static int valleyview_get_display_clock_speed(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int vco = valleyview_get_vco(dev_priv);
>  	u32 val;
>  	int divider;
>  
> @@ -5460,6 +5457,9 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
>  	if (IS_CHERRYVIEW(dev))
>  		return 400000;
>  
> +	if (dev_priv->hpll_freq == 0)
> +		dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> +
>  	mutex_lock(&dev_priv->dpio_lock);
>  	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
>  	mutex_unlock(&dev_priv->dpio_lock);
> @@ -5470,7 +5470,7 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
>  	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
>  	     "cdclk change in progress\n");
>  
> -	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
> +	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
>  }
>  
>  static int i945_get_display_clock_speed(struct drm_device *dev)


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Cache HPLL frequency on VLV/CHV

[Daniel Vetter]

On Thu, Nov 06, 2014 at 04:02:10PM +0200, Imre Deak wrote:
> On Tue, 2014-10-07 at 17:41 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We need the HPLL frequency when calculating cdclk. Currently we read
> > that out from the hardware every single time, which isn't going to fly
> > very well if the device is runtime suspended. So cache the HPLL
> > frequency in dev_priv and use the cached value.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> You could add:
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=82939
> 
> where this fixes some subtest failures on VLV.
> On 1/3 and 3/3 of this patchset:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Move the .global_resources() hook call into modeset_update_crtc_power_domains()

[Jesse Barnes]

On Thu, 6 Nov 2014 14:10:49 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Nov 06, 2014 at 02:49:12PM +0200,
> ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We may need to access various hardware bits in
> > the .global_resources() hook, so move the call to occur after
> > enabling all the newly required power wells, but before disabling
> > all the now unneeded wells. This should guarantee that we have all
> > the sufficient hardware resources available during
> > the .global_resources() call. And if not, any additional resources
> > must be explicitly acquired by the .global_resorces() hook.
> > 
> > For instance on VLV/CHV we need to access the gunit mailbox so that
> > we can talk to punit/cck over sideband. In addition some PFI credit
> > reprogramming may need to be addes as well, which may require the
> > disp2d well.
> > 
> > This should also make the power domain refcounts consistent on
> > platforms which don't have a .global_resource() hook since now they
> > too will call modeset_update_crtc_power_domains() which will drop
> > the init power. Previously init power was just left enabled for
> > such platforms.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Yeah I think that's a lot saner and hopefully allows us to unify the
> power domain more. Thanks for the patch, queued for next.

As a cleanup later it might be good to pull it out into a separate
function.  The global resources can be related to power domains, but
not everything we do there is (e.g. re-clocking cdclk), so it may be a
little confusing for readers in the future.

Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Move the .global_resources() hook call into modeset_update_crtc_power_domains()

[Ville Syrjälä]

On Mon, Nov 10, 2014 at 09:14:11AM -0800, Jesse Barnes wrote:
> On Thu, 6 Nov 2014 14:10:49 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Thu, Nov 06, 2014 at 02:49:12PM +0200,
> > ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We may need to access various hardware bits in
> > > the .global_resources() hook, so move the call to occur after
> > > enabling all the newly required power wells, but before disabling
> > > all the now unneeded wells. This should guarantee that we have all
> > > the sufficient hardware resources available during
> > > the .global_resources() call. And if not, any additional resources
> > > must be explicitly acquired by the .global_resorces() hook.
> > > 
> > > For instance on VLV/CHV we need to access the gunit mailbox so that
> > > we can talk to punit/cck over sideband. In addition some PFI credit
> > > reprogramming may need to be addes as well, which may require the
> > > disp2d well.
> > > 
> > > This should also make the power domain refcounts consistent on
> > > platforms which don't have a .global_resource() hook since now they
> > > too will call modeset_update_crtc_power_domains() which will drop
> > > the init power. Previously init power was just left enabled for
> > > such platforms.
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Yeah I think that's a lot saner and hopefully allows us to unify the
> > power domain more. Thanks for the patch, queued for next.
> 
> As a cleanup later it might be good to pull it out into a separate
> function.  The global resources can be related to power domains, but
> not everything we do there is (e.g. re-clocking cdclk), so it may be a
> little confusing for readers in the future.

Well, we can't pull it out. That's the whole point with the patch. Or
maybe I misunderstood what you want to pull and where?

But we can certainly rename modeset_update_crtc_power_domains() itself.
But I suck at names anyway so didn't even bother to try :P  Now that I
actually think about it, I guess I could have just called it
intel_global_resources() or something.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Move the .global_resources() hook call into modeset_update_crtc_power_domains()

[Jesse Barnes]

On Mon, 10 Nov 2014 19:24:37 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Nov 10, 2014 at 09:14:11AM -0800, Jesse Barnes wrote:
> > On Thu, 6 Nov 2014 14:10:49 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Thu, Nov 06, 2014 at 02:49:12PM +0200,
> > > ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > We may need to access various hardware bits in
> > > > the .global_resources() hook, so move the call to occur after
> > > > enabling all the newly required power wells, but before
> > > > disabling all the now unneeded wells. This should guarantee
> > > > that we have all the sufficient hardware resources available
> > > > during the .global_resources() call. And if not, any additional
> > > > resources must be explicitly acquired by the .global_resorces()
> > > > hook.
> > > > 
> > > > For instance on VLV/CHV we need to access the gunit mailbox so
> > > > that we can talk to punit/cck over sideband. In addition some
> > > > PFI credit reprogramming may need to be addes as well, which
> > > > may require the disp2d well.
> > > > 
> > > > This should also make the power domain refcounts consistent on
> > > > platforms which don't have a .global_resource() hook since now
> > > > they too will call modeset_update_crtc_power_domains() which
> > > > will drop the init power. Previously init power was just left
> > > > enabled for such platforms.
> > > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Yeah I think that's a lot saner and hopefully allows us to unify
> > > the power domain more. Thanks for the patch, queued for next.
> > 
> > As a cleanup later it might be good to pull it out into a separate
> > function.  The global resources can be related to power domains, but
> > not everything we do there is (e.g. re-clocking cdclk), so it may
> > be a little confusing for readers in the future.
> 
> Well, we can't pull it out. That's the whole point with the patch. Or
> maybe I misunderstood what you want to pull and where?
> 
> But we can certainly rename modeset_update_crtc_power_domains()
> itself. But I suck at names anyway so didn't even bother to try :P
> Now that I actually think about it, I guess I could have just called
> it intel_global_resources() or something.

Yeah that would do what I'd like, something that wraps both the power
domain stuff and the global resources call into something that's named
more accurately.

Thanks,
Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx