Re: [PATCH 1/3] drm/i915/kbl: Adding missing IS_KABYLAKE checks.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

Hi Michel,

On Thu, 2016-01-07 at 16:14 +0000, Michel Thierry wrote:
> On 1/7/2016 1:15 AM, Rodrigo Vivi wrote:
> > When adding IS_KABYLAKE definition I didn't included the
> > DC states related because I was planing to include them
> > with the patch that fixes DMC firmware loading, but I
> > forgot them.
> > 
> > Meanwhile this runtime pm code changed a lot for
> > Skylake.
> > 
> > Well, I didn't expect that this would crash the machine
> > and I just noticed now that Sarah warned me our driver
> > wasn't working. Thanks Sarah.
> > 
> > Cc: Sarah Sharp <sarah.a.sharp@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_csr.c        |  5 +++--
> >   drivers/gpu/drm/i915/intel_runtime_pm.c | 19 ++++++++++++-------
> >   2 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_csr.c 
> > b/drivers/gpu/drm/i915/intel_csr.c
> > index 9bb63a8..3f69829 100644
> > --- a/drivers/gpu/drm/i915/intel_csr.c
> > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > @@ -278,7 +278,8 @@ static uint32_t *parse_csr_fw(struct 
> > drm_i915_private *dev_priv,
> > 
> >   	csr->version = css_header->version;
> > 
> > -	if (IS_SKYLAKE(dev) && csr->version < 
> > SKL_CSR_VERSION_REQUIRED) {
> > +	if ((IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) &&
> > +	     csr->version < SKL_CSR_VERSION_REQUIRED) {
> >   		DRM_INFO("Refusing to load old Skylake DMC 
> > firmware v%u.%u,"
> >   			 " please upgrade to v%u.%u or later"
> >   			 " [
> > https://01.org/linuxgraphics/intel-linux-graphics-firmwares].\n",
> > @@ -421,7 +422,7 @@ void intel_csr_ucode_init(struct 
> > drm_i915_private *dev_priv)
> >   	if (!HAS_CSR(dev_priv))
> >   		return;
> > 
> > -	if (IS_SKYLAKE(dev_priv))
> > +	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> >   		csr->fw_path = I915_CSR_SKL;
> >   	else if (IS_BROXTON(dev_priv))
> >   		csr->fw_path = I915_CSR_BXT;
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index ddbdbff..f5bf54c 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -532,7 +532,8 @@ static void assert_can_enable_dc5(struct 
> > drm_i915_private *dev_priv)
> >   	bool pg2_enabled = 
> > intel_display_power_well_is_enabled(dev_priv,
> >   					SKL_DISP_PW_2);
> > 
> > -	WARN_ONCE(!IS_SKYLAKE(dev), "Platform doesn't support 
> > DC5.\n");
> > +	WARN_ONCE(!IS_SKYLAKE(dev) && !IS_KABYLAKE(dev),
> > +		  "Platform doesn't support DC5.\n");
> >   	WARN_ONCE(!HAS_RUNTIME_PM(dev), "Runtime PM not 
> > enabled.\n");
> >   	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable 
> > DC5.\n");
> > 
> > @@ -568,7 +569,8 @@ static void assert_can_enable_dc6(struct 
> > drm_i915_private *dev_priv)
> >   {
> >   	struct drm_device *dev = dev_priv->dev;
> > 
> > -	WARN_ONCE(!IS_SKYLAKE(dev), "Platform doesn't support 
> > DC6.\n");
> > +	WARN_ONCE(!IS_SKYLAKE(dev) && !IS_KABYLAKE(dev),
> > +		  "Platform doesn't support DC6.\n");
> >   	WARN_ONCE(!HAS_RUNTIME_PM(dev), "Runtime PM not 
> > enabled.\n");
> >   	WARN_ONCE(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
> >   		  "Backlight is not disabled.\n");
> > @@ -595,7 +597,8 @@ static void gen9_disable_dc5_dc6(struct 
> > drm_i915_private *dev_priv)
> >   {
> >   	assert_can_disable_dc5(dev_priv);
> > 
> > -	if (IS_SKYLAKE(dev_priv) && i915.enable_dc != 0 && 
> > i915.enable_dc != 1)
> > +	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > +	    i915.enable_dc != 0 && i915.enable_dc != 1)
> >   		assert_can_disable_dc6(dev_priv);
> > 
> >   	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > @@ -783,7 +786,8 @@ static void 
> > gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> >   static void gen9_dc_off_power_well_disable(struct 
> > drm_i915_private *dev_priv,
> >   					   struct i915_power_well 
> > *power_well)
> >   {
> > -	if (IS_SKYLAKE(dev_priv) && i915.enable_dc != 0 && 
> > i915.enable_dc != 1)
> > +	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > +	    i915.enable_dc != 0 && i915.enable_dc != 1)
> >   		skl_enable_dc6(dev_priv);
> >   	else
> >   		gen9_enable_dc5(dev_priv);
> > @@ -795,7 +799,8 @@ static void 
> > gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
> >   	if (power_well->count > 0) {
> >   		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> >   	} else {
> > -		if (IS_SKYLAKE(dev_priv) && i915.enable_dc != 0 &&
> > +		if ((IS_SKYLAKE(dev_priv) || 
> > IS_KABYLAKE(dev_priv)) &&
> > +		    i915.enable_dc != 0 &&
> >   		    i915.enable_dc != 1)
> >   			gen9_set_dc_state(dev_priv, 
> > DC_STATE_EN_UPTO_DC6);
> >   		else
> > @@ -1851,7 +1856,7 @@ void skl_pw1_misc_io_init(struct 
> > drm_i915_private *dev_priv)
> >   {
> >   	struct i915_power_well *well;
> > 
> > -	if (!IS_SKYLAKE(dev_priv))
> > +	if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
> >   		return;
> > 
> >   	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> > @@ -1865,7 +1870,7 @@ void skl_pw1_misc_io_fini(struct 
> > drm_i915_private *dev_priv)
> >   {
> >   	struct i915_power_well *well;
> > 
> > -	if (!IS_SKYLAKE(dev_priv))
> > +	if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
> >   		return;
> > 
> >   	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> > 
> 
> Hi Rodrigo,
> 
> I had the same issue with the power wells and sent the same fix 
> yesterday 
> (http://cgit.freedesktop.org/drm-intel/commit/?id=16fbc291cb87c7defcd
> 13ad715d3e4af0d523e43).

I hadn't seen it. I'm sorry.

> 
> I didn't try to use DMC, so I didn't catch the other problem.

I will merge a v2 of my patch rebased on top of yours.


> 
> Without the now redundant changes in skl_pw1_misc_io_init/fini,
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Thanks for the review.

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