Re: [RFC 5/6] drm/i915: device specific runtime suspend/resume

[Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>]

On 01/22/2014 06:28 PM, Daniel Vetter wrote:
> On Wed, Jan 22, 2014 at 05:34:21PM +0530, naresh.kumar.kachhi@intel.com wrote:
>> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>
>> we might need special handling for different platforms.
>> for ex. baytrail. To handle this this patch creates function
>> pointers for runtime suspend/resume which are assigned
>> during driver load time
>>
>> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> Again NAK on principles: vtables always have a cost, so before we add a
> new one we need to demonstrate the need. Which means 2-3 different
> implementations for the vfunc must exist. Sometimes some of these are
> still embargo'ed in the -internal tree (like with the ppgtt refactor a
> year ago) and that's ok.
>
> The other reason I don't like this is that thus far the driver
> load/teardown and suspend/resume sequences are generic. There are tricky
> (sometimes really tricky) dependcies, but that's the design thus far. So
> if we want to switch the design to per-platform functions we need good
> reasons for that switch in general. Which means likely the same issues
> will crop up in the normal suspend/resume and driver load/teardown paths.
> Which leaves me wondering why we don't have this here.
>
> Finally we've been bitten countless times through superflous differences
> and duplicated codepaths between normal operation, suspend/resume, driver
> load/teardown and now runtime pm. So again deviating from a shared code
> pattern need excellent justification. Yes I know that atm pc8 has its own
> stuff, and I expect this to hurt us eventually ;-)
>
> Looking at this patch I don't see any of this.
> -Daniel

Hi Daniel,
I agree with you here, we should try to have a generic solutions. But we have
different scenario in case of LP platform and mainstream. In case of LP we
might go to deeper sleep state (causing Gunit power gate), and might need
some extra work during runtime_suspend/resume. Also as on BYT interrupts are
routed through display power island, making display power gating dependent on
runtime_suspend/resume. which might not be the case for other platforms.
Yes, this patch is not conclusive to prove the need of a separate function
I am working on BYT runtime runtime_suspend/resume. Once done, we can evaluate if we
need a separate functions or not.


>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++---------------
>>   drivers/gpu/drm/i915/i915_drv.h |  6 ++++++
>>   drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 52 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 80965be..dc1cfab 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -935,19 +935,10 @@ static int i915_runtime_suspend(struct device *device)
>>   
>>   	DRM_DEBUG_KMS("Suspending device\n");
>>   
>> -	i915_gem_release_all_mmaps(dev_priv);
>> -
>> -	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
>> -	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.
>> -	 */
>> -	intel_opregion_notify_adapter(dev, PCI_D1);
>> +	if (dev_priv->pm.funcs.runtime_suspend)
>> +		dev_priv->pm.funcs.runtime_suspend(dev_priv);
>> +	else
>> +		DRM_ERROR("Device does not have runtime functions");
>>   
>>   	return 0;
>>   }
>> @@ -962,8 +953,10 @@ static int i915_runtime_resume(struct device *device)
>>   
>>   	DRM_DEBUG_KMS("Resuming device\n");
>>   
>> -	intel_opregion_notify_adapter(dev, PCI_D0);
>> -	dev_priv->pm.suspended = false;
>> +	if (dev_priv->pm.funcs.runtime_resume)
>> +		dev_priv->pm.funcs.runtime_resume(dev_priv);
>> +	else
>> +		DRM_ERROR("Device does not have runtime functions");
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6a6f046..54aa31d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1322,11 +1322,17 @@ struct i915_package_c8 {
>>   	} regsave;
>>   };
>>   
>> +struct i915_runtime_pm_funcs {
>> +	int (*runtime_suspend)(struct drm_i915_private *dev_priv);
>> +	int (*runtime_resume)(struct drm_i915_private *dev_priv);
>> +};
>> +
>>   struct i915_runtime_pm {
>>   	bool suspended;
>>   	bool gpu_idle;
>>   	bool disp_idle;
>>   	struct mutex lock;
>> +	struct i915_runtime_pm_funcs funcs;
>>   };
>>   
>>   enum intel_pipe_crc_source {
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 9d6d0e1..6713995 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5588,6 +5588,41 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
>>   	pm_runtime_put_autosuspend(device);
>>   }
>>   
>> +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>> +{
>> +	struct drm_device *dev = dev_priv->dev;
>> +
>> +	WARN_ON(!HAS_RUNTIME_PM(dev));
>> +
>> +	i915_gem_release_all_mmaps(dev_priv);
>> +
>> +	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
>> +	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.
>> +	 */
>> +	intel_opregion_notify_adapter(dev, PCI_D1);
>> +
>> +
>> +	return 0;
>> +}
>> +
>> +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
>> +{
>> +	struct drm_device *dev = dev_priv->dev;
>> +	WARN_ON(!HAS_RUNTIME_PM(dev));
>> +
>> +	intel_opregion_notify_adapter(dev_priv->dev, PCI_D0);
>> +	dev_priv->pm.suspended = false;
>> +
>> +	return 0;
>> +}
>> +
>>   void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>>   {
>>   	struct drm_device *dev = dev_priv->dev;
>> @@ -5606,6 +5641,9 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>>   	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>>   	pm_runtime_mark_last_busy(device);
>>   	pm_runtime_use_autosuspend(device);
>> +
>> +	dev_priv->pm.funcs.runtime_suspend = hsw_runtime_suspend;
>> +	dev_priv->pm.funcs.runtime_resume = hsw_runtime_resume;
>>   }
>>   
>>   void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
>> -- 
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx