Re: [PATCH 3/3] drm/i915: send PCI_D3hot adapter opregion message on BDW RPM suspend

[Damien Lespiau <damien.lespiau@intel.com>]

On Thu, Aug 21, 2014 at 05:09:38PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On BDW we're seeing a problem that after we runtime resume, the
> outputs connected to DDI C are not detected: they don't appear in the
> SDEISR register and GMBUS transactions don't work. They stop working
> at the moment we call intel_opregion_notify_adapter() during runtime
> suspend, but they don't go back to work when we call the same function
> during runtime resume. They only work after we do a modeset and call
> intel_opregion_notify_encoder(), but this point is already too late.
> 
> While debugging, I tried to pass PCI_D3hot which is the value that
> matches the spec, and it seems to have solved the problem. I couldn't
> find any explanation of why this solves the problem, but there's also
> no documented explanation - besides our code and git log - of why
> Haswell should use PCI_D1, so keep this for now in order to keep BDW
> runtime PM working.
> 
> Also add a comment to point the fact that there's no spec documenting
> all the weirdness involved here.
> 
> Cc: kristen.c.accardi@intel.com
> Testcase: igt/pm_rpm/drm-resources-equal
> Testcase: igt/pm_rpm/i2c
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

There's still investigation to understand what the firmware is doing
behind our backs, but the change looks likely to be correct and fixes
the problem at hand so:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2dcc0d8..6f0c95f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1455,13 +1455,29 @@ static int intel_runtime_suspend(struct device *device)
>  	dev_priv->pm.suspended = true;
>  
>  	/*
> -	 * current versions of firmware which depend on this opregion
> -	 * notification have repurposed the D1 definition to mean
> -	 * "runtime suspended" vs. what you would normally expect (D3)
> -	 * to distinguish it from notifications that might be sent
> -	 * via the suspend path.
> +	 * FIXME: We really should find a document that references the arguments
> +	 * used below!
>  	 */
> -	intel_opregion_notify_adapter(dev, PCI_D1);
> +	if (IS_HASWELL(dev)) {
> +		/*
> +		 * current versions of firmware which depend on this opregion
> +		 * notification have repurposed the D1 definition to mean
> +		 * "runtime suspended" vs. what you would normally expect (D3)
> +		 * to distinguish it from notifications that might be sent via
> +		 * the suspend path.
> +		 */
> +		intel_opregion_notify_adapter(dev, PCI_D1);
> +	} else {
> +		/*
> +		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
> +		 * being detected, and the call we do at intel_runtime_resume()
> +		 * won't be able to restore them. Since PCI_D3hot matches the
> +		 * actual specification and appears to be working, use it. Let's
> +		 * assume the other non-Haswell platforms will stay the same as
> +		 * Broadwell.
> +		 */
> +		intel_opregion_notify_adapter(dev, PCI_D3hot);
> +	}
>  
>  	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx