[RFC 0/6] enabling runtime on BYT

[RFC 0/6] enabling runtime on BYT

[naresh.kumar.kachhi]

From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

This is the first set of patches to enable runtime pm for BYT.
These patches are created keeping in mind that PC8 feature will
be replaced with runtime pm. So all the patches are on assumption
that HAS_RUNTIME_PM is true while HAS_PC8 is false.
This set is mainly to cover the accesses with runtime_[get/put].
while the second set (WIP) will be to introduce BYT specific
runtime_[suspend/resume].

Naresh Kumar Kachhi (6):
  drm/i915: cover ioctls with runtime_get/put
  drm/i915: cover ring access with rpm get/put
  drm/i915: introduce runtime get/put based on display activity
  drm/i915/vlv: call runtime get/put based on disp activity
  drm/i915: device specific runtime suspend/resume
  FOR_UPSTREAM [VPG]: drm/i915: call init_runtime_pm before gem_init

 drivers/gpu/drm/i915/i915_dma.c            |   9 +-
 drivers/gpu/drm/i915/i915_drv.c            |  44 ++++++----
 drivers/gpu/drm/i915/i915_drv.h            |  18 +++-
 drivers/gpu/drm/i915/i915_gem.c            |  27 ++++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   4 +-
 drivers/gpu/drm/i915/i915_ioc32.c          |  11 ++-
 drivers/gpu/drm/i915/intel_display.c       |   3 +
 drivers/gpu/drm/i915/intel_drv.h           |   4 +
 drivers/gpu/drm/i915/intel_pm.c            | 135 +++++++++++++++++++++++++++++
 9 files changed, 222 insertions(+), 33 deletions(-)

-- 
1.8.1.2

[RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[naresh.kumar.kachhi]

From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

With runtime PM enabled, we need to make sure that all HW access
are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
HW hangs. Ex. A hang is seen if display register is accessed on
BYT while display power island is power gated.

This patch is covering all the IOCTLs with get/put.
TODO: limit runtime_get/put to IOCTLs that accesses HW

Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c   | 21 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_ioc32.c | 11 +++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 82c4605..80965be 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -845,6 +845,25 @@ i915_pci_remove(struct pci_dev *pdev)
 	drm_put_dev(dev);
 }
 
+static long i915_unlocked_ioctl(struct file *filp,
+	      unsigned int cmd, unsigned long arg)
+{
+	struct drm_file *file_priv = filp->private_data;
+	struct drm_device *dev = file_priv->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	/* Don't do anything if device is not ready */
+	if (drm_device_is_unplugged(dev))
+		return -ENODEV;
+
+	intel_runtime_pm_get(dev_priv);
+	ret = drm_ioctl(filp, cmd, arg);
+	intel_runtime_pm_put(dev_priv);
+
+	return ret;
+}
+
 static int i915_pm_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -970,7 +989,7 @@ static const struct file_operations i915_driver_fops = {
 	.owner = THIS_MODULE,
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = drm_ioctl,
+	.unlocked_ioctl = i915_unlocked_ioctl,
 	.mmap = drm_gem_mmap,
 	.poll = drm_poll,
 	.read = drm_read,
diff --git a/drivers/gpu/drm/i915/i915_ioc32.c b/drivers/gpu/drm/i915/i915_ioc32.c
index 3c59584..bfc773e 100644
--- a/drivers/gpu/drm/i915/i915_ioc32.c
+++ b/drivers/gpu/drm/i915/i915_ioc32.c
@@ -201,12 +201,17 @@ static drm_ioctl_compat_t *i915_compat_ioctls[] = {
  */
 long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	struct drm_file *file_priv = filp->private_data;
+	struct drm_device *dev = file_priv->minor->dev;
 	unsigned int nr = DRM_IOCTL_NR(cmd);
 	drm_ioctl_compat_t *fn = NULL;
 	int ret;
 
-	if (nr < DRM_COMMAND_BASE)
-		return drm_compat_ioctl(filp, cmd, arg);
+	intel_runtime_pm_get(dev->dev_private);
+	if (nr < DRM_COMMAND_BASE) {
+		ret = drm_compat_ioctl(filp, cmd, arg);
+		goto out;
+	}
 
 	if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(i915_compat_ioctls))
 		fn = i915_compat_ioctls[nr - DRM_COMMAND_BASE];
@@ -216,6 +221,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	else
 		ret = drm_ioctl(filp, cmd, arg);
 
+out:
+	intel_runtime_pm_put(dev->dev_private);
 	return ret;
 }
 #endif
-- 
1.8.1.2

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Daniel Vetter]

On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> 
> With runtime PM enabled, we need to make sure that all HW access
> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> HW hangs. Ex. A hang is seen if display register is accessed on
> BYT while display power island is power gated.
> 
> This patch is covering all the IOCTLs with get/put.
> TODO: limit runtime_get/put to IOCTLs that accesses HW
> 
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

Nack on the concept. We need to have get/put calls for the individual
functional blocks of the hw, which then in turn (if it's not the top-level
power domain) need to grab references to the next up power domain.
Splattering unconditional get/puts over all driver entry points is bad
style and imo also too fragile.

Also, with Paulos final runtime pm/pc8 unification patches and Imre's
display power well patches for byt we should have full coverage already.
Have you looked at the work of these too?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c   | 21 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_ioc32.c | 11 +++++++++--
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 82c4605..80965be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -845,6 +845,25 @@ i915_pci_remove(struct pci_dev *pdev)
>  	drm_put_dev(dev);
>  }
>  
> +static long i915_unlocked_ioctl(struct file *filp,
> +	      unsigned int cmd, unsigned long arg)
> +{
> +	struct drm_file *file_priv = filp->private_data;
> +	struct drm_device *dev = file_priv->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	/* Don't do anything if device is not ready */
> +	if (drm_device_is_unplugged(dev))
> +		return -ENODEV;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	ret = drm_ioctl(filp, cmd, arg);
> +	intel_runtime_pm_put(dev_priv);
> +
> +	return ret;
> +}
> +
>  static int i915_pm_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -970,7 +989,7 @@ static const struct file_operations i915_driver_fops = {
>  	.owner = THIS_MODULE,
>  	.open = drm_open,
>  	.release = drm_release,
> -	.unlocked_ioctl = drm_ioctl,
> +	.unlocked_ioctl = i915_unlocked_ioctl,
>  	.mmap = drm_gem_mmap,
>  	.poll = drm_poll,
>  	.read = drm_read,
> diff --git a/drivers/gpu/drm/i915/i915_ioc32.c b/drivers/gpu/drm/i915/i915_ioc32.c
> index 3c59584..bfc773e 100644
> --- a/drivers/gpu/drm/i915/i915_ioc32.c
> +++ b/drivers/gpu/drm/i915/i915_ioc32.c
> @@ -201,12 +201,17 @@ static drm_ioctl_compat_t *i915_compat_ioctls[] = {
>   */
>  long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +	struct drm_file *file_priv = filp->private_data;
> +	struct drm_device *dev = file_priv->minor->dev;
>  	unsigned int nr = DRM_IOCTL_NR(cmd);
>  	drm_ioctl_compat_t *fn = NULL;
>  	int ret;
>  
> -	if (nr < DRM_COMMAND_BASE)
> -		return drm_compat_ioctl(filp, cmd, arg);
> +	intel_runtime_pm_get(dev->dev_private);
> +	if (nr < DRM_COMMAND_BASE) {
> +		ret = drm_compat_ioctl(filp, cmd, arg);
> +		goto out;
> +	}
>  
>  	if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(i915_compat_ioctls))
>  		fn = i915_compat_ioctls[nr - DRM_COMMAND_BASE];
> @@ -216,6 +221,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	else
>  		ret = drm_ioctl(filp, cmd, arg);
>  
> +out:
> +	intel_runtime_pm_put(dev->dev_private);
>  	return ret;
>  }
>  #endif
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1844 bytes --]

On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
> On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com wrote:
> > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > 
> > With runtime PM enabled, we need to make sure that all HW access
> > are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> > HW hangs. Ex. A hang is seen if display register is accessed on
> > BYT while display power island is power gated.
> > 
> > This patch is covering all the IOCTLs with get/put.
> > TODO: limit runtime_get/put to IOCTLs that accesses HW
> > 
> > Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> 
> Nack on the concept. We need to have get/put calls for the individual
> functional blocks of the hw, which then in turn (if it's not the top-level
> power domain) need to grab references to the next up power domain.
> Splattering unconditional get/puts over all driver entry points is bad
> style and imo also too fragile.
> 
> Also, with Paulos final runtime pm/pc8 unification patches and Imre's
> display power well patches for byt we should have full coverage already.
> Have you looked at the work of these too?

I'm still in debt with the BYT specific power domain patches, I want to
post them (this week) after I sorted out spurious pipe stat IRQs we'd
receive atm with the power well off. Until then there is only the WIP
version at: https://github.com/ideak/linux/commits/powerwells

But in practice, as you point out the plan was to only call
modeset_update_power_wells() during modeset time and that will end up
doing the proper RPM get/put as necessary. Similarly on some other code
paths like connector detect and HW state read-out code, we'd ask only
for the needed power domains which would end up doing the RPM get/put.

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Naresh Kumar Kachhi]

[-- Attachment #1.1: Type: text/plain, Size: 4078 bytes --]

On 01/22/2014 06:53 PM, Imre Deak wrote:
> On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
>> On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com wrote:
>>> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>>
>>> With runtime PM enabled, we need to make sure that all HW access
>>> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
>>> HW hangs. Ex. A hang is seen if display register is accessed on
>>> BYT while display power island is power gated.
>>>
>>> This patch is covering all the IOCTLs with get/put.
>>> TODO: limit runtime_get/put to IOCTLs that accesses HW
>>>
>>> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>> Nack on the concept. We need to have get/put calls for the individual
>> functional blocks of the hw, which then in turn (if it's not the top-level
>> power domain) need to grab references to the next up power domain.
>> Splattering unconditional get/puts over all driver entry points is bad
>> style and imo also too fragile.
>>
>> Also, with Paulos final runtime pm/pc8 unification patches and Imre's
>> display power well patches for byt we should have full coverage already.
>> Have you looked at the work of these too?
> I'm still in debt with the BYT specific power domain patches, I want to
> post them (this week) after I sorted out spurious pipe stat IRQs we'd
> receive atm with the power well off. Until then there is only the WIP
> version at: https://github.com/ideak/linux/commits/powerwells
>
> But in practice, as you point out the plan was to only call
> modeset_update_power_wells() during modeset time and that will end up
> doing the proper RPM get/put as necessary. Similarly on some other code
> paths like connector detect and HW state read-out code, we'd ask only
> for the needed power domains which would end up doing the RPM get/put.
>
> --Imre
>
Hi Imre,
I tried to go through your and Paulo's patches (http://patchwork.freedesktop.org/patch/16952/).
As per my understanding we are doing disp power well gate/ungate independent of we are in
runtime_suspend/resume state we might face problem here as on BYT interrupts are routed through
display power well, so we might have a situation where display power island is power gated,
but render workload is still active. As we won't be receiving any interrupt __wait_seq_no will
stall and we might end up in a TDR. We will not see the issue until display power gating is
enabled. I will try to include a test to cover this test

This can be avoided by adding display_get/put in runtime_resume/suspend.
I am not sure if this scenario will be applicable for HSW.

Need for covering ioclts:
However there is one more problem for LP platforms. Once display/render/media power
islands are power gated, system will enter into deeper sleep state causing Gunit to power
gate. We will loose the state once Gunit resumes, so Gunit restore and ring initialization is
required in runtime_resume. Even after adding these to runtime_suspend/resume, we will have
to make sure all the ring activities, Gunit register accesses are covered with runtime_get/put.
There are few places with current implementation where we might hit this problem.
Example:
i915_batchbuffer(ioctl)->i915_batchbuffer (gpu commands scheduled)
i915_irq_emit(ioctl) -> i915_emit_irq (gpu commands scheduled)
i915_cmdbuffer (ioctl) -> i915_dispatch_cmdbuffer-> i915_emit_cmds(gpu commands scheduled)
There are few more.
These ioclts not being covered with runtime_get/put will might cause invalid executions
if we are resuming for deeper power state (gunit was powergated).

solutions:
 From design perspective, we can cover all accesses at low level, but this means a lot of get/put
and also will have to make sure that we cover future accesses as well. As an other solution we
can cover these ioctls individually or can have a wrapper function (as purposed in this patch)
to cover the accesses (TODO: make them conditional to do get/put on few ioclts only).
I will try to upload refined patch with conditional get/put.

Thanks,
Naresh












[-- Attachment #1.2: Type: text/html, Size: 5080 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Paulo Zanoni]

2014/1/24 Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>:
> On 01/22/2014 06:53 PM, Imre Deak wrote:
>
> On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
>
> On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com
> wrote:
>
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> With runtime PM enabled, we need to make sure that all HW access
> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> HW hangs. Ex. A hang is seen if display register is accessed on
> BYT while display power island is power gated.
>
> This patch is covering all the IOCTLs with get/put.
> TODO: limit runtime_get/put to IOCTLs that accesses HW
>
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> Nack on the concept. We need to have get/put calls for the individual
> functional blocks of the hw, which then in turn (if it's not the top-level
> power domain) need to grab references to the next up power domain.
> Splattering unconditional get/puts over all driver entry points is bad
> style and imo also too fragile.
>
> Also, with Paulos final runtime pm/pc8 unification patches and Imre's
> display power well patches for byt we should have full coverage already.
> Have you looked at the work of these too?
>
> I'm still in debt with the BYT specific power domain patches, I want to
> post them (this week) after I sorted out spurious pipe stat IRQs we'd
> receive atm with the power well off. Until then there is only the WIP
> version at: https://github.com/ideak/linux/commits/powerwells
>
> But in practice, as you point out the plan was to only call
> modeset_update_power_wells() during modeset time and that will end up
> doing the proper RPM get/put as necessary. Similarly on some other code
> paths like connector detect and HW state read-out code, we'd ask only
> for the needed power domains which would end up doing the RPM get/put.
>
> --Imre
>
> Hi Imre,
> I tried to go through your and Paulo's patches
> (http://patchwork.freedesktop.org/patch/16952/).
> As per my understanding we are doing disp power well gate/ungate independent
> of we are in
> runtime_suspend/resume state we might face problem here as on BYT interrupts
> are routed through
> display power well, so we might have a situation where display power island
> is power gated,
> but render workload is still active. As we won't be receiving any interrupt
> __wait_seq_no will
> stall and we might end up in a TDR. We will not see the issue until display
> power gating is
> enabled. I will try to include a test to cover this test
>
> This can be avoided by adding display_get/put in runtime_resume/suspend.
> I am not sure if this scenario will be applicable for HSW.
>
> Need for covering ioclts:
> However there is one more problem for LP platforms. Once
> display/render/media power
> islands are power gated, system will enter into deeper sleep state causing
> Gunit to power
> gate. We will loose the state once Gunit resumes, so Gunit restore and ring
> initialization is
> required in runtime_resume. Even after adding these to
> runtime_suspend/resume, we will have
> to make sure all the ring activities, Gunit register accesses are covered
> with runtime_get/put.
> There are few places with current implementation where we might hit this
> problem.
> Example:
> i915_batchbuffer(ioctl)->i915_batchbuffer (gpu commands scheduled)
> i915_irq_emit(ioctl) -> i915_emit_irq (gpu commands scheduled)
> i915_cmdbuffer (ioctl) -> i915_dispatch_cmdbuffer-> i915_emit_cmds(gpu
> commands scheduled)
> There are few more.
> These ioclts not being covered with runtime_get/put will might cause invalid
> executions
> if we are resuming for deeper power state (gunit was powergated).

If possible, please write pm_pc8.c subtests that reproduce all these
problems you found :)


>
> solutions:
> From design perspective, we can cover all accesses at low level, but this
> means a lot of get/put
> and also will have to make sure that we cover future accesses as well. As an
> other solution we
> can cover these ioctls individually or can have a wrapper function (as
> purposed in this patch)
> to cover the accesses (TODO: make them conditional to do get/put on few
> ioclts only).
> I will try to upload refined patch with conditional get/put.
>
> Thanks,
> Naresh
>
>
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Paulo Zanoni

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Naresh Kumar Kachhi]

On 01/24/2014 09:26 PM, Paulo Zanoni wrote:
> 2014/1/24 Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>:
>> On 01/22/2014 06:53 PM, Imre Deak wrote:
>>
>> On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
>>
>> On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com
>> wrote:
>>
>> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>
>> With runtime PM enabled, we need to make sure that all HW access
>> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
>> HW hangs. Ex. A hang is seen if display register is accessed on
>> BYT while display power island is power gated.
>>
>> This patch is covering all the IOCTLs with get/put.
>> TODO: limit runtime_get/put to IOCTLs that accesses HW
>>
>> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>
>> Nack on the concept. We need to have get/put calls for the individual
>> functional blocks of the hw, which then in turn (if it's not the top-level
>> power domain) need to grab references to the next up power domain.
>> Splattering unconditional get/puts over all driver entry points is bad
>> style and imo also too fragile.
>>
>> Also, with Paulos final runtime pm/pc8 unification patches and Imre's
>> display power well patches for byt we should have full coverage already.
>> Have you looked at the work of these too?
>>
>> I'm still in debt with the BYT specific power domain patches, I want to
>> post them (this week) after I sorted out spurious pipe stat IRQs we'd
>> receive atm with the power well off. Until then there is only the WIP
>> version at: https://github.com/ideak/linux/commits/powerwells
>>
>> But in practice, as you point out the plan was to only call
>> modeset_update_power_wells() during modeset time and that will end up
>> doing the proper RPM get/put as necessary. Similarly on some other code
>> paths like connector detect and HW state read-out code, we'd ask only
>> for the needed power domains which would end up doing the RPM get/put.
>>
>> --Imre
>>
>> Hi Imre,
>> I tried to go through your and Paulo's patches
>> (http://patchwork.freedesktop.org/patch/16952/).
>> As per my understanding we are doing disp power well gate/ungate independent
>> of we are in
>> runtime_suspend/resume state we might face problem here as on BYT interrupts
>> are routed through
>> display power well, so we might have a situation where display power island
>> is power gated,
>> but render workload is still active. As we won't be receiving any interrupt
>> __wait_seq_no will
>> stall and we might end up in a TDR. We will not see the issue until display
>> power gating is
>> enabled. I will try to include a test to cover this test
>>
>> This can be avoided by adding display_get/put in runtime_resume/suspend.
>> I am not sure if this scenario will be applicable for HSW.
>>
>> Need for covering ioclts:
>> However there is one more problem for LP platforms. Once
>> display/render/media power
>> islands are power gated, system will enter into deeper sleep state causing
>> Gunit to power
>> gate. We will loose the state once Gunit resumes, so Gunit restore and ring
>> initialization is
>> required in runtime_resume. Even after adding these to
>> runtime_suspend/resume, we will have
>> to make sure all the ring activities, Gunit register accesses are covered
>> with runtime_get/put.
>> There are few places with current implementation where we might hit this
>> problem.
>> Example:
>> i915_batchbuffer(ioctl)->i915_batchbuffer (gpu commands scheduled)
>> i915_irq_emit(ioctl) -> i915_emit_irq (gpu commands scheduled)
>> i915_cmdbuffer (ioctl) -> i915_dispatch_cmdbuffer-> i915_emit_cmds(gpu
>> commands scheduled)
>> There are few more.
>> These ioclts not being covered with runtime_get/put will might cause invalid
>> executions
>> if we are resuming for deeper power state (gunit was powergated).
> If possible, please write pm_pc8.c subtests that reproduce all these
> problems you found :)
>
Above scenarios are only valid for non-KMS mode. I missed it, so looks like
we should be fine with the current implementation and your outstanding patches

>> solutions:
>>  From design perspective, we can cover all accesses at low level, but this
>> means a lot of get/put
>> and also will have to make sure that we cover future accesses as well. As an
>> other solution we
>> can cover these ioctls individually or can have a wrapper function (as
>> purposed in this patch)
>> to cover the accesses (TODO: make them conditional to do get/put on few
>> ioclts only).
>> I will try to upload refined patch with conditional get/put.
>>
>> Thanks,
>> Naresh
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 4949 bytes --]

On Fri, 2014-01-24 at 20:35 +0530, Naresh Kumar Kachhi wrote:
> On 01/22/2014 06:53 PM, Imre Deak wrote:
> 
> > On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
> > > On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com wrote:
> > > > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > > > 
> > > > With runtime PM enabled, we need to make sure that all HW access
> > > > are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> > > > HW hangs. Ex. A hang is seen if display register is accessed on
> > > > BYT while display power island is power gated.
> > > > 
> > > > This patch is covering all the IOCTLs with get/put.
> > > > TODO: limit runtime_get/put to IOCTLs that accesses HW
> > > > 
> > > > Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > > Nack on the concept. We need to have get/put calls for the individual
> > > functional blocks of the hw, which then in turn (if it's not the top-level
> > > power domain) need to grab references to the next up power domain.
> > > Splattering unconditional get/puts over all driver entry points is bad
> > > style and imo also too fragile.
> > > 
> > > Also, with Paulos final runtime pm/pc8 unification patches and Imre's
> > > display power well patches for byt we should have full coverage already.
> > > Have you looked at the work of these too?
> > I'm still in debt with the BYT specific power domain patches, I want to
> > post them (this week) after I sorted out spurious pipe stat IRQs we'd
> > receive atm with the power well off. Until then there is only the WIP
> > version at: https://github.com/ideak/linux/commits/powerwells
> > 
> > But in practice, as you point out the plan was to only call
> > modeset_update_power_wells() during modeset time and that will end up
> > doing the proper RPM get/put as necessary. Similarly on some other code
> > paths like connector detect and HW state read-out code, we'd ask only
> > for the needed power domains which would end up doing the RPM get/put.
> > 
> > --Imre
> > 
> Hi Imre,
> I tried to go through your and Paulo's patches (http://patchwork.freedesktop.org/patch/16952/).
> As per my understanding we are doing disp power well gate/ungate independent of we are in 
> runtime_suspend/resume state we might face problem here as on BYT interrupts are routed through
> display power well, so we might have a situation where display power island is power gated,
> but render workload is still active. As we won't be receiving any interrupt __wait_seq_no will
> stall and we might end up in a TDR. We will not see the issue until display power gating is 
> enabled. I will try to include a test to cover this test

Hm, which exact interrupt routing are you referring to? At least on my
BYT I manage to power off only the display power well and keep the
render well on, while still being able to run for example
igt/tests/gem_render_copy, gem_render_linear_blits. I can see that
through /proc/interrupts that GT interrupts are properly generated. The
display side interrupt routing is of course off, for example the
PIPESTAT registers will read all 0xffffffff, but DEIIR, DEIMR, DEISR are
all seem to work ok.

--Imre

> This can be avoided by adding display_get/put in runtime_resume/suspend.
> I am not sure if this scenario will be applicable for HSW.
> 
> Need for covering ioclts:
> However there is one more problem for LP platforms. Once display/render/media power
> islands are power gated, system will enter into deeper sleep state causing Gunit to power
> gate. We will loose the state once Gunit resumes, so Gunit restore and ring initialization is
> required in runtime_resume. Even after adding these to runtime_suspend/resume, we will have
> to make sure all the ring activities, Gunit register accesses are covered with runtime_get/put.
> There are few places with current implementation where we might hit this problem.
> Example: 
> i915_batchbuffer(ioctl)->i915_batchbuffer (gpu commands scheduled)
> i915_irq_emit(ioctl) -> i915_emit_irq (gpu commands scheduled)
> i915_cmdbuffer (ioctl) -> i915_dispatch_cmdbuffer-> i915_emit_cmds(gpu commands scheduled)
> There are few more.
> These ioclts not being covered with runtime_get/put will might cause invalid executions
> if we are resuming for deeper power state (gunit was powergated).
> 
> solutions:
> From design perspective, we can cover all accesses at low level, but this means a lot of get/put
> and also will have to make sure that we cover future accesses as well. As an other solution we
> can cover these ioctls individually or can have a wrapper function (as purposed in this patch)
> to cover the accesses (TODO: make them conditional to do get/put on few ioclts only).
> I will try to upload refined patch with conditional get/put.
> 
> Thanks,
> Naresh
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Jesse Barnes]

On Fri, 24 Jan 2014 19:23:54 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Fri, 2014-01-24 at 20:35 +0530, Naresh Kumar Kachhi wrote:
> > On 01/22/2014 06:53 PM, Imre Deak wrote:
> > 
> > > On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
> > > > On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com wrote:
> > > > > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > > > > 
> > > > > With runtime PM enabled, we need to make sure that all HW access
> > > > > are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> > > > > HW hangs. Ex. A hang is seen if display register is accessed on
> > > > > BYT while display power island is power gated.
> > > > > 
> > > > > This patch is covering all the IOCTLs with get/put.
> > > > > TODO: limit runtime_get/put to IOCTLs that accesses HW
> > > > > 
> > > > > Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > > > Nack on the concept. We need to have get/put calls for the individual
> > > > functional blocks of the hw, which then in turn (if it's not the top-level
> > > > power domain) need to grab references to the next up power domain.
> > > > Splattering unconditional get/puts over all driver entry points is bad
> > > > style and imo also too fragile.
> > > > 
> > > > Also, with Paulos final runtime pm/pc8 unification patches and Imre's
> > > > display power well patches for byt we should have full coverage already.
> > > > Have you looked at the work of these too?
> > > I'm still in debt with the BYT specific power domain patches, I want to
> > > post them (this week) after I sorted out spurious pipe stat IRQs we'd
> > > receive atm with the power well off. Until then there is only the WIP
> > > version at: https://github.com/ideak/linux/commits/powerwells
> > > 
> > > But in practice, as you point out the plan was to only call
> > > modeset_update_power_wells() during modeset time and that will end up
> > > doing the proper RPM get/put as necessary. Similarly on some other code
> > > paths like connector detect and HW state read-out code, we'd ask only
> > > for the needed power domains which would end up doing the RPM get/put.
> > > 
> > > --Imre
> > > 
> > Hi Imre,
> > I tried to go through your and Paulo's patches (http://patchwork.freedesktop.org/patch/16952/).
> > As per my understanding we are doing disp power well gate/ungate independent of we are in 
> > runtime_suspend/resume state we might face problem here as on BYT interrupts are routed through
> > display power well, so we might have a situation where display power island is power gated,
> > but render workload is still active. As we won't be receiving any interrupt __wait_seq_no will
> > stall and we might end up in a TDR. We will not see the issue until display power gating is 
> > enabled. I will try to include a test to cover this test
> 
> Hm, which exact interrupt routing are you referring to? At least on my
> BYT I manage to power off only the display power well and keep the
> render well on, while still being able to run for example
> igt/tests/gem_render_copy, gem_render_linear_blits. I can see that
> through /proc/interrupts that GT interrupts are properly generated. The
> display side interrupt routing is of course off, for example the
> PIPESTAT registers will read all 0xffffffff, but DEIIR, DEIMR, DEISR are
> all seem to work ok.

I think Naresh is referring to full D3 state shutdown, where the Gunit
goes away too.  But in that case the GT will be shut down as well, so I
don't think the display vs GT thing by itself is a problem.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Naresh Kumar Kachhi]

On 02/22/2014 01:37 AM, Jesse Barnes wrote:
> On Fri, 24 Jan 2014 19:23:54 +0200
> Imre Deak <imre.deak@intel.com> wrote:
>
>> On Fri, 2014-01-24 at 20:35 +0530, Naresh Kumar Kachhi wrote:
>>> On 01/22/2014 06:53 PM, Imre Deak wrote:
>>>
>>>> On Wed, 2014-01-22 at 13:51 +0100, Daniel Vetter wrote:
>>>>> On Wed, Jan 22, 2014 at 05:34:17PM +0530, naresh.kumar.kachhi@intel.com wrote:
>>>>>> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>>>>>
>>>>>> With runtime PM enabled, we need to make sure that all HW access
>>>>>> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
>>>>>> HW hangs. Ex. A hang is seen if display register is accessed on
>>>>>> BYT while display power island is power gated.
>>>>>>
>>>>>> This patch is covering all the IOCTLs with get/put.
>>>>>> TODO: limit runtime_get/put to IOCTLs that accesses HW
>>>>>>
>>>>>> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>>>> Nack on the concept. We need to have get/put calls for the individual
>>>>> functional blocks of the hw, which then in turn (if it's not the top-level
>>>>> power domain) need to grab references to the next up power domain.
>>>>> Splattering unconditional get/puts over all driver entry points is bad
>>>>> style and imo also too fragile.
>>>>>
>>>>> Also, with Paulos final runtime pm/pc8 unification patches and Imre's
>>>>> display power well patches for byt we should have full coverage already.
>>>>> Have you looked at the work of these too?
>>>> I'm still in debt with the BYT specific power domain patches, I want to
>>>> post them (this week) after I sorted out spurious pipe stat IRQs we'd
>>>> receive atm with the power well off. Until then there is only the WIP
>>>> version at: https://github.com/ideak/linux/commits/powerwells
>>>>
>>>> But in practice, as you point out the plan was to only call
>>>> modeset_update_power_wells() during modeset time and that will end up
>>>> doing the proper RPM get/put as necessary. Similarly on some other code
>>>> paths like connector detect and HW state read-out code, we'd ask only
>>>> for the needed power domains which would end up doing the RPM get/put.
>>>>
>>>> --Imre
>>>>
>>> Hi Imre,
>>> I tried to go through your and Paulo's patches (http://patchwork.freedesktop.org/patch/16952/).
>>> As per my understanding we are doing disp power well gate/ungate independent of we are in
>>> runtime_suspend/resume state we might face problem here as on BYT interrupts are routed through
>>> display power well, so we might have a situation where display power island is power gated,
>>> but render workload is still active. As we won't be receiving any interrupt __wait_seq_no will
>>> stall and we might end up in a TDR. We will not see the issue until display power gating is
>>> enabled. I will try to include a test to cover this test
>> Hm, which exact interrupt routing are you referring to? At least on my
>> BYT I manage to power off only the display power well and keep the
>> render well on, while still being able to run for example
>> igt/tests/gem_render_copy, gem_render_linear_blits. I can see that
>> through /proc/interrupts that GT interrupts are properly generated. The
>> display side interrupt routing is of course off, for example the
>> PIPESTAT registers will read all 0xffffffff, but DEIIR, DEIMR, DEISR are
>> all seem to work ok.
> I think Naresh is referring to full D3 state shutdown, where the Gunit
> goes away too.  But in that case the GT will be shut down as well, so I
> don't think the display vs GT thing by itself is a problem.
>
Please ignore my previous comment as I confused interrupts being routed through display unit on VLV.
But this comment will be valid on some platforms like HSW, where render interrupts are routed
through display unit. So, display power gating in such platforms has to be done by taking
render/media also into consideration.
-Naresh

Re: [RFC 1/6] drm/i915: cover ioctls with runtime_get/put

[Paulo Zanoni]

2014/1/22  <naresh.kumar.kachhi@intel.com>:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> With runtime PM enabled, we need to make sure that all HW access
> are valid (i.e.  Gfx is in D0). Invalid accesses might end up in
> HW hangs. Ex. A hang is seen if display register is accessed on
> BYT while display power island is power gated.
>
> This patch is covering all the IOCTLs with get/put.
> TODO: limit runtime_get/put to IOCTLs that accesses HW
>
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

We believe the current upstream code is enough to get runtime PM
references whenever they are needed. Can you please point the cases
where we fail to do this?

Also, if you find a spot we don't cover, please also patch
intel-gpu-tools, adding new test cases to pm_pc8.c.

Thanks,
Paulo

> ---
>  drivers/gpu/drm/i915/i915_drv.c   | 21 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_ioc32.c | 11 +++++++++--
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 82c4605..80965be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -845,6 +845,25 @@ i915_pci_remove(struct pci_dev *pdev)
>         drm_put_dev(dev);
>  }
>
> +static long i915_unlocked_ioctl(struct file *filp,
> +             unsigned int cmd, unsigned long arg)
> +{
> +       struct drm_file *file_priv = filp->private_data;
> +       struct drm_device *dev = file_priv->minor->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       int ret;
> +
> +       /* Don't do anything if device is not ready */
> +       if (drm_device_is_unplugged(dev))
> +               return -ENODEV;
> +
> +       intel_runtime_pm_get(dev_priv);
> +       ret = drm_ioctl(filp, cmd, arg);
> +       intel_runtime_pm_put(dev_priv);
> +
> +       return ret;
> +}
> +
>  static int i915_pm_suspend(struct device *dev)
>  {
>         struct pci_dev *pdev = to_pci_dev(dev);
> @@ -970,7 +989,7 @@ static const struct file_operations i915_driver_fops = {
>         .owner = THIS_MODULE,
>         .open = drm_open,
>         .release = drm_release,
> -       .unlocked_ioctl = drm_ioctl,
> +       .unlocked_ioctl = i915_unlocked_ioctl,
>         .mmap = drm_gem_mmap,
>         .poll = drm_poll,
>         .read = drm_read,
> diff --git a/drivers/gpu/drm/i915/i915_ioc32.c b/drivers/gpu/drm/i915/i915_ioc32.c
> index 3c59584..bfc773e 100644
> --- a/drivers/gpu/drm/i915/i915_ioc32.c
> +++ b/drivers/gpu/drm/i915/i915_ioc32.c
> @@ -201,12 +201,17 @@ static drm_ioctl_compat_t *i915_compat_ioctls[] = {
>   */
>  long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +       struct drm_file *file_priv = filp->private_data;
> +       struct drm_device *dev = file_priv->minor->dev;
>         unsigned int nr = DRM_IOCTL_NR(cmd);
>         drm_ioctl_compat_t *fn = NULL;
>         int ret;
>
> -       if (nr < DRM_COMMAND_BASE)
> -               return drm_compat_ioctl(filp, cmd, arg);
> +       intel_runtime_pm_get(dev->dev_private);
> +       if (nr < DRM_COMMAND_BASE) {
> +               ret = drm_compat_ioctl(filp, cmd, arg);
> +               goto out;
> +       }
>
>         if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(i915_compat_ioctls))
>                 fn = i915_compat_ioctls[nr - DRM_COMMAND_BASE];
> @@ -216,6 +221,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>         else
>                 ret = drm_ioctl(filp, cmd, arg);
>
> +out:
> +       intel_runtime_pm_put(dev->dev_private);
>         return ret;
>  }
>  #endif
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[RFC 2/6] drm/i915: cover ring access with rpm get/put

[naresh.kumar.kachhi]

From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

GPU idleness is tracked by checking the request queue. Whenever
request queue is empty we assume that GPU is idle. When a new
set of commands sheduled on ring we call i915_add_request to
make sure these commands are tracked properly. However there are
few places which are not being treacked currently.
This patch introduces a new function add_request_wo_flush to track
such requests. add_request_wo_flush is same as add_request, only
difference is that it will not cause a flush. This is to avoid
any extra overhead while adding new request.

To make sure Gfx is in D0 while there are still commands pending
on ring following is done.
- All the ioctls are already covered with get/put this makes sure
  at the time of scheduling commands on GPU, Gfx is in D0
- Once command scheduling is done, we call add_request to track
  ring activity.
- We call get_noresume if this is first request (ioctl is already
  covered with get_sync).
- put is called only when request_list becomes empty. i.e GPU is
  idle and there are no pending commands on the rings

Note: Make sure we don't do multiple add_request in same
ioctl/callback only one in the end is enough

Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c            |  5 ++++
 drivers/gpu/drm/i915/i915_drv.h            | 10 +++++--
 drivers/gpu/drm/i915/i915_gem.c            | 27 ++++++++++++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +--
 drivers/gpu/drm/i915/intel_display.c       |  1 +
 drivers/gpu/drm/i915/intel_drv.h           |  3 ++
 drivers/gpu/drm/i915/intel_pm.c            | 47 ++++++++++++++++++++++++++++++
 7 files changed, 84 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ee9502b..b5af745 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -454,6 +454,7 @@ static int i915_dispatch_cmdbuffer(struct drm_device * dev,
 				   struct drm_clip_rect *cliprects,
 				   void *cmdbuf)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int nbox = cmd->num_cliprects;
 	int i = 0, count, ret;
 
@@ -480,6 +481,7 @@ static int i915_dispatch_cmdbuffer(struct drm_device * dev,
 	}
 
 	i915_emit_breadcrumb(dev);
+	i915_add_request_wo_flush(LP_RING(dev_priv));
 	return 0;
 }
 
@@ -542,6 +544,7 @@ static int i915_dispatch_batchbuffer(struct drm_device * dev,
 	}
 
 	i915_emit_breadcrumb(dev);
+	i915_add_request_wo_flush(LP_RING(dev_priv));
 	return 0;
 }
 
@@ -595,6 +598,7 @@ static int i915_dispatch_flip(struct drm_device * dev)
 		ADVANCE_LP_RING();
 	}
 
+	i915_add_request_wo_flush(LP_RING(dev_priv));
 	master_priv->sarea_priv->pf_current_page = dev_priv->dri1.current_page;
 	return 0;
 }
@@ -768,6 +772,7 @@ static int i915_emit_irq(struct drm_device * dev)
 		OUT_RING(dev_priv->dri1.counter);
 		OUT_RING(MI_USER_INTERRUPT);
 		ADVANCE_LP_RING();
+		i915_add_request_wo_flush(LP_RING(dev_priv));
 	}
 
 	return dev_priv->dri1.counter;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56c720b..d1399f9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1324,6 +1324,7 @@ struct i915_package_c8 {
 
 struct i915_runtime_pm {
 	bool suspended;
+	bool gpu_idle;
 };
 
 enum intel_pipe_crc_source {
@@ -2063,7 +2064,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 
 int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
 int i915_gem_object_sync(struct drm_i915_gem_object *obj,
-			 struct intel_ring_buffer *to);
+			 struct intel_ring_buffer *to, bool add_request);
 void i915_vma_move_to_active(struct i915_vma *vma,
 			     struct intel_ring_buffer *ring);
 int i915_gem_dumb_create(struct drm_file *file_priv,
@@ -2139,9 +2140,12 @@ int __must_check i915_gem_suspend(struct drm_device *dev);
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
 		       struct drm_i915_gem_object *batch_obj,
-		       u32 *seqno);
+		       u32 *seqno,
+		       bool flush_caches);
 #define i915_add_request(ring, seqno) \
-	__i915_add_request(ring, NULL, NULL, seqno)
+	__i915_add_request(ring, NULL, NULL, seqno, true)
+#define i915_add_request_wo_flush(ring) \
+	__i915_add_request(ring, NULL, NULL, NULL, false)
 int __must_check i915_wait_seqno(struct intel_ring_buffer *ring,
 				 uint32_t seqno);
 int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 024e454..3e8202e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2136,7 +2136,8 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 int __i915_add_request(struct intel_ring_buffer *ring,
 		       struct drm_file *file,
 		       struct drm_i915_gem_object *obj,
-		       u32 *out_seqno)
+		       u32 *out_seqno,
+		       bool flush_caches)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
@@ -2152,9 +2153,11 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	 * is that the flush _must_ happen before the next request, no matter
 	 * what.
 	 */
-	ret = intel_ring_flush_all_caches(ring);
-	if (ret)
-		return ret;
+	if (flush_caches) {
+		ret = intel_ring_flush_all_caches(ring);
+		if (ret)
+			return ret;
+	}
 
 	request = ring->preallocated_lazy_request;
 	if (WARN_ON(request == NULL))
@@ -2219,6 +2222,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 					   &dev_priv->mm.retire_work,
 					   round_jiffies_up_relative(HZ));
 			intel_mark_busy(dev_priv->dev);
+			intel_runtime_pm_gpu_busy(dev_priv);
 		}
 	}
 
@@ -2544,10 +2548,12 @@ i915_gem_retire_requests(struct drm_device *dev)
 		idle &= list_empty(&ring->request_list);
 	}
 
-	if (idle)
+	if (idle) {
 		mod_delayed_work(dev_priv->wq,
 				   &dev_priv->mm.idle_work,
 				   msecs_to_jiffies(100));
+		intel_runtime_pm_gpu_idle(dev_priv);
+	}
 
 	return idle;
 }
@@ -2691,6 +2697,8 @@ out:
  *
  * @obj: object which may be in use on another ring.
  * @to: ring we wish to use the object on. May be NULL.
+ * @add_request: do we need to add a request to track operations
+ *    submitted on ring with sync_to function
  *
  * This code is meant to abstract object synchronization with the GPU.
  * Calling with NULL implies synchronizing the object with the CPU
@@ -2700,7 +2708,7 @@ out:
  */
 int
 i915_gem_object_sync(struct drm_i915_gem_object *obj,
-		     struct intel_ring_buffer *to)
+		     struct intel_ring_buffer *to, bool add_request)
 {
 	struct intel_ring_buffer *from = obj->ring;
 	u32 seqno;
@@ -2724,12 +2732,15 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 
 	trace_i915_gem_ring_sync_to(from, to, seqno);
 	ret = to->sync_to(to, from, seqno);
-	if (!ret)
+	if (!ret) {
 		/* We use last_read_seqno because sync_to()
 		 * might have just caused seqno wrap under
 		 * the radar.
 		 */
 		from->sync_seqno[idx] = obj->last_read_seqno;
+		if (add_request)
+			i915_add_request_wo_flush(to);
+	}
 
 	return ret;
 }
@@ -3707,7 +3718,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	int ret;
 
 	if (pipelined != obj->ring) {
-		ret = i915_gem_object_sync(obj, pipelined);
+		ret = i915_gem_object_sync(obj, pipelined, true);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0c6bcff..bda7a06 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -832,7 +832,7 @@ i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring,
 
 	list_for_each_entry(vma, vmas, exec_list) {
 		struct drm_i915_gem_object *obj = vma->obj;
-		ret = i915_gem_object_sync(obj, ring);
+		ret = i915_gem_object_sync(obj, ring, false);
 		if (ret)
 			return ret;
 
@@ -969,7 +969,7 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 	ring->gpu_caches_dirty = true;
 
 	/* Add a breadcrumb for the completion of the batch buffer */
-	(void)__i915_add_request(ring, file, obj, NULL);
+	(void)__i915_add_request(ring, file, obj, NULL, true);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ec96002..25eae03 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8624,6 +8624,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 
 	intel_mark_page_flip_active(intel_crtc);
 	__intel_ring_advance(ring);
+	i915_add_request_wo_flush(ring);
 	return 0;
 
 err_unpin:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b3c209..9061aa7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -881,9 +881,12 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv);
 void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
 void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
+void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
 void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
+void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv);
+void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv);
 void ilk_wm_get_hw_state(struct drm_device *dev);
 
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b9b4fe4..991ff62 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5470,6 +5470,37 @@ void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
 	hsw_enable_package_c8(dev_priv);
 }
 
+void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_RUNTIME_PM(dev_priv->dev))
+		return;
+
+	/* don't need a seperate mutex here as callers are
+	 * already under struct_mutex
+	 */
+	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+	if (!dev_priv->pm.gpu_idle) {
+		dev_priv->pm.gpu_idle = true;
+		/* match with get in gpu_busy */
+		intel_runtime_pm_put(dev_priv);
+	}
+}
+
+void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_RUNTIME_PM(dev_priv->dev))
+		return;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+	if (dev_priv->pm.gpu_idle) {
+		dev_priv->pm.gpu_idle = false;
+		/* make sure that we keep the GPU on until request list
+		 * is empty
+		 */
+		intel_runtime_pm_get_noresume(dev_priv);
+	}
+}
+
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -5482,6 +5513,21 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
 }
 
+void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct device *device = &dev->pdev->dev;
+
+	if (!HAS_RUNTIME_PM(dev))
+		return;
+
+	/* driver calls no resume when it is sure that device is
+	 * already active and just want to increment the ref count
+	 */
+	WARN(dev_priv->pm.suspended, "Device suspended. call get_sync?\n");
+	pm_runtime_get_noresume(device);
+}
+
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -5500,6 +5546,7 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
 	struct device *device = &dev->pdev->dev;
 
 	dev_priv->pm.suspended = false;
+	dev_priv->pm.gpu_idle = true;
 
 	if (!HAS_RUNTIME_PM(dev))
 		return;
-- 
1.8.1.2

Re: [RFC 2/6] drm/i915: cover ring access with rpm get/put

[Paulo Zanoni]

2014/1/22  <naresh.kumar.kachhi@intel.com>:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> GPU idleness is tracked by checking the request queue. Whenever
> request queue is empty we assume that GPU is idle. When a new
> set of commands sheduled on ring we call i915_add_request to
> make sure these commands are tracked properly. However there are
> few places which are not being treacked currently.
> This patch introduces a new function add_request_wo_flush to track
> such requests. add_request_wo_flush is same as add_request, only
> difference is that it will not cause a flush. This is to avoid
> any extra overhead while adding new request.
>
> To make sure Gfx is in D0 while there are still commands pending
> on ring following is done.
> - All the ioctls are already covered with get/put this makes sure
>   at the time of scheduling commands on GPU, Gfx is in D0
> - Once command scheduling is done, we call add_request to track
>   ring activity.
> - We call get_noresume if this is first request (ioctl is already
>   covered with get_sync).
> - put is called only when request_list becomes empty. i.e GPU is
>   idle and there are no pending commands on the rings
>
> Note: Make sure we don't do multiple add_request in same
> ioctl/callback only one in the end is enough
>

The PC8 code already has an infrastructure to track GPU idleness, and
I already submitted a patch to move that code to
dev_priv->pm.gpu_idle. Please see this patch:
http://patchwork.freedesktop.org/patch/16952/ . I suggest you should
try to reuse this infrastructure, and if you find any problems, please
add testcases to pm_pc8.c reproducing these problems.


> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c            |  5 ++++
>  drivers/gpu/drm/i915/i915_drv.h            | 10 +++++--
>  drivers/gpu/drm/i915/i915_gem.c            | 27 ++++++++++++-----
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +--
>  drivers/gpu/drm/i915/intel_display.c       |  1 +
>  drivers/gpu/drm/i915/intel_drv.h           |  3 ++
>  drivers/gpu/drm/i915/intel_pm.c            | 47 ++++++++++++++++++++++++++++++
>  7 files changed, 84 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index ee9502b..b5af745 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -454,6 +454,7 @@ static int i915_dispatch_cmdbuffer(struct drm_device * dev,
>                                    struct drm_clip_rect *cliprects,
>                                    void *cmdbuf)
>  {
> +       struct drm_i915_private *dev_priv = dev->dev_private;
>         int nbox = cmd->num_cliprects;
>         int i = 0, count, ret;
>
> @@ -480,6 +481,7 @@ static int i915_dispatch_cmdbuffer(struct drm_device * dev,
>         }
>
>         i915_emit_breadcrumb(dev);
> +       i915_add_request_wo_flush(LP_RING(dev_priv));
>         return 0;
>  }
>
> @@ -542,6 +544,7 @@ static int i915_dispatch_batchbuffer(struct drm_device * dev,
>         }
>
>         i915_emit_breadcrumb(dev);
> +       i915_add_request_wo_flush(LP_RING(dev_priv));
>         return 0;
>  }
>
> @@ -595,6 +598,7 @@ static int i915_dispatch_flip(struct drm_device * dev)
>                 ADVANCE_LP_RING();
>         }
>
> +       i915_add_request_wo_flush(LP_RING(dev_priv));
>         master_priv->sarea_priv->pf_current_page = dev_priv->dri1.current_page;
>         return 0;
>  }
> @@ -768,6 +772,7 @@ static int i915_emit_irq(struct drm_device * dev)
>                 OUT_RING(dev_priv->dri1.counter);
>                 OUT_RING(MI_USER_INTERRUPT);
>                 ADVANCE_LP_RING();
> +               i915_add_request_wo_flush(LP_RING(dev_priv));
>         }
>
>         return dev_priv->dri1.counter;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 56c720b..d1399f9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1324,6 +1324,7 @@ struct i915_package_c8 {
>
>  struct i915_runtime_pm {
>         bool suspended;
> +       bool gpu_idle;
>  };
>
>  enum intel_pipe_crc_source {
> @@ -2063,7 +2064,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>
>  int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
>  int i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -                        struct intel_ring_buffer *to);
> +                        struct intel_ring_buffer *to, bool add_request);
>  void i915_vma_move_to_active(struct i915_vma *vma,
>                              struct intel_ring_buffer *ring);
>  int i915_gem_dumb_create(struct drm_file *file_priv,
> @@ -2139,9 +2140,12 @@ int __must_check i915_gem_suspend(struct drm_device *dev);
>  int __i915_add_request(struct intel_ring_buffer *ring,
>                        struct drm_file *file,
>                        struct drm_i915_gem_object *batch_obj,
> -                      u32 *seqno);
> +                      u32 *seqno,
> +                      bool flush_caches);
>  #define i915_add_request(ring, seqno) \
> -       __i915_add_request(ring, NULL, NULL, seqno)
> +       __i915_add_request(ring, NULL, NULL, seqno, true)
> +#define i915_add_request_wo_flush(ring) \
> +       __i915_add_request(ring, NULL, NULL, NULL, false)
>  int __must_check i915_wait_seqno(struct intel_ring_buffer *ring,
>                                  uint32_t seqno);
>  int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 024e454..3e8202e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2136,7 +2136,8 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
>  int __i915_add_request(struct intel_ring_buffer *ring,
>                        struct drm_file *file,
>                        struct drm_i915_gem_object *obj,
> -                      u32 *out_seqno)
> +                      u32 *out_seqno,
> +                      bool flush_caches)
>  {
>         drm_i915_private_t *dev_priv = ring->dev->dev_private;
>         struct drm_i915_gem_request *request;
> @@ -2152,9 +2153,11 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>          * is that the flush _must_ happen before the next request, no matter
>          * what.
>          */
> -       ret = intel_ring_flush_all_caches(ring);
> -       if (ret)
> -               return ret;
> +       if (flush_caches) {
> +               ret = intel_ring_flush_all_caches(ring);
> +               if (ret)
> +                       return ret;
> +       }
>
>         request = ring->preallocated_lazy_request;
>         if (WARN_ON(request == NULL))
> @@ -2219,6 +2222,7 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>                                            &dev_priv->mm.retire_work,
>                                            round_jiffies_up_relative(HZ));
>                         intel_mark_busy(dev_priv->dev);
> +                       intel_runtime_pm_gpu_busy(dev_priv);
>                 }
>         }
>
> @@ -2544,10 +2548,12 @@ i915_gem_retire_requests(struct drm_device *dev)
>                 idle &= list_empty(&ring->request_list);
>         }
>
> -       if (idle)
> +       if (idle) {
>                 mod_delayed_work(dev_priv->wq,
>                                    &dev_priv->mm.idle_work,
>                                    msecs_to_jiffies(100));
> +               intel_runtime_pm_gpu_idle(dev_priv);
> +       }
>
>         return idle;
>  }
> @@ -2691,6 +2697,8 @@ out:
>   *
>   * @obj: object which may be in use on another ring.
>   * @to: ring we wish to use the object on. May be NULL.
> + * @add_request: do we need to add a request to track operations
> + *    submitted on ring with sync_to function
>   *
>   * This code is meant to abstract object synchronization with the GPU.
>   * Calling with NULL implies synchronizing the object with the CPU
> @@ -2700,7 +2708,7 @@ out:
>   */
>  int
>  i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -                    struct intel_ring_buffer *to)
> +                    struct intel_ring_buffer *to, bool add_request)
>  {
>         struct intel_ring_buffer *from = obj->ring;
>         u32 seqno;
> @@ -2724,12 +2732,15 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>
>         trace_i915_gem_ring_sync_to(from, to, seqno);
>         ret = to->sync_to(to, from, seqno);
> -       if (!ret)
> +       if (!ret) {
>                 /* We use last_read_seqno because sync_to()
>                  * might have just caused seqno wrap under
>                  * the radar.
>                  */
>                 from->sync_seqno[idx] = obj->last_read_seqno;
> +               if (add_request)
> +                       i915_add_request_wo_flush(to);
> +       }
>
>         return ret;
>  }
> @@ -3707,7 +3718,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>         int ret;
>
>         if (pipelined != obj->ring) {
> -               ret = i915_gem_object_sync(obj, pipelined);
> +               ret = i915_gem_object_sync(obj, pipelined, true);
>                 if (ret)
>                         return ret;
>         }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0c6bcff..bda7a06 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -832,7 +832,7 @@ i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring,
>
>         list_for_each_entry(vma, vmas, exec_list) {
>                 struct drm_i915_gem_object *obj = vma->obj;
> -               ret = i915_gem_object_sync(obj, ring);
> +               ret = i915_gem_object_sync(obj, ring, false);
>                 if (ret)
>                         return ret;
>
> @@ -969,7 +969,7 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
>         ring->gpu_caches_dirty = true;
>
>         /* Add a breadcrumb for the completion of the batch buffer */
> -       (void)__i915_add_request(ring, file, obj, NULL);
> +       (void)__i915_add_request(ring, file, obj, NULL, true);
>  }
>
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ec96002..25eae03 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8624,6 +8624,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>
>         intel_mark_page_flip_active(intel_crtc);
>         __intel_ring_advance(ring);
> +       i915_add_request_wo_flush(ring);
>         return 0;
>
>  err_unpin:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b3c209..9061aa7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -881,9 +881,12 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv);
>  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
>  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> +void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
> +void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv);
> +void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
>
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b9b4fe4..991ff62 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5470,6 +5470,37 @@ void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
>         hsw_enable_package_c8(dev_priv);
>  }
>
> +void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv)
> +{
> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
> +               return;
> +
> +       /* don't need a seperate mutex here as callers are
> +        * already under struct_mutex
> +        */
> +       WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> +       if (!dev_priv->pm.gpu_idle) {
> +               dev_priv->pm.gpu_idle = true;
> +               /* match with get in gpu_busy */
> +               intel_runtime_pm_put(dev_priv);
> +       }
> +}
> +
> +void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv)
> +{
> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
> +               return;
> +
> +       WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> +       if (dev_priv->pm.gpu_idle) {
> +               dev_priv->pm.gpu_idle = false;
> +               /* make sure that we keep the GPU on until request list
> +                * is empty
> +                */
> +               intel_runtime_pm_get_noresume(dev_priv);
> +       }
> +}
> +
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
> @@ -5482,6 +5513,21 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>         WARN(dev_priv->pm.suspended, "Device still suspended.\n");
>  }
>
> +void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       struct device *device = &dev->pdev->dev;
> +
> +       if (!HAS_RUNTIME_PM(dev))
> +               return;
> +
> +       /* driver calls no resume when it is sure that device is
> +        * already active and just want to increment the ref count
> +        */
> +       WARN(dev_priv->pm.suspended, "Device suspended. call get_sync?\n");
> +       pm_runtime_get_noresume(device);
> +}
> +
>  void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
> @@ -5500,6 +5546,7 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>         struct device *device = &dev->pdev->dev;
>
>         dev_priv->pm.suspended = false;
> +       dev_priv->pm.gpu_idle = true;
>
>         if (!HAS_RUNTIME_PM(dev))
>                 return;
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[RFC 3/6] drm/i915: introduce runtime get/put based on display activity

[naresh.kumar.kachhi]

From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

Once the display is disabled, we need to call runtime_put to
make sure Runtime framework triggers runtime_suspend based on
idleness. Similarly when display gets enabled, runtime_get should
be called. We have similiar function for pc8 feature, but some
platform(BYT) might not have pc8 feature, so creating a generic
function for runtime_pm

Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 50 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d1399f9..6a6f046 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1325,6 +1325,8 @@ struct i915_package_c8 {
 struct i915_runtime_pm {
 	bool suspended;
 	bool gpu_idle;
+	bool disp_idle;
+	struct mutex lock;
 };
 
 enum intel_pipe_crc_source {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9061aa7..94a6127 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -887,6 +887,7 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
 void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv);
+void intel_runtime_update_disp_state(struct drm_i915_private *dev_priv);
 void ilk_wm_get_hw_state(struct drm_device *dev);
 
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 991ff62..9d6d0e1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5501,6 +5501,54 @@ void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv)
 	}
 }
 
+static void intel_runtime_pm_disp_idle(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_RUNTIME_PM(dev_priv->dev))
+		return;
+
+	mutex_lock(&dev_priv->pm.lock);
+	if (!dev_priv->pm.disp_idle) {
+		dev_priv->pm.disp_idle = true;
+		intel_runtime_pm_put(dev_priv);
+	}
+	mutex_unlock(&dev_priv->pm.lock);
+}
+
+static void intel_runtime_pm_disp_busy(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_RUNTIME_PM(dev_priv->dev))
+		return;
+
+	mutex_lock(&dev_priv->pm.lock);
+	if (dev_priv->pm.disp_idle) {
+		dev_priv->pm.disp_idle = false;
+		/* This call is coming from an IOCTL so we have already done a
+		 * get_sync. get_noresume should suffice here
+		 */
+		intel_runtime_pm_get_noresume(dev_priv);
+	}
+	mutex_unlock(&dev_priv->pm.lock);
+}
+
+void intel_runtime_update_disp_state(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc;
+	bool enabled = false;
+
+	if (!HAS_RUNTIME_PM(dev_priv->dev))
+		return;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+		enabled |= crtc->base.enabled;
+
+	if (enabled)
+		intel_runtime_pm_disp_busy(dev_priv);
+	else
+		intel_runtime_pm_disp_idle(dev_priv);
+
+}
+
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -5547,6 +5595,8 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
 
 	dev_priv->pm.suspended = false;
 	dev_priv->pm.gpu_idle = true;
+	dev_priv->pm.disp_idle = true;
+	mutex_init(&dev_priv->pm.lock);
 
 	if (!HAS_RUNTIME_PM(dev))
 		return;
-- 
1.8.1.2

Re: [RFC 3/6] drm/i915: introduce runtime get/put based on display activity

[Paulo Zanoni]

Hi

2014/1/22  <naresh.kumar.kachhi@intel.com>:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> Once the display is disabled, we need to call runtime_put to
> make sure Runtime framework triggers runtime_suspend based on
> idleness. Similarly when display gets enabled, runtime_get should
> be called. We have similiar function for pc8 feature, but some
> platform(BYT) might not have pc8 feature, so creating a generic
> function for runtime_pm

Does this patch series help you somehow?
http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html

>
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 50 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d1399f9..6a6f046 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1325,6 +1325,8 @@ struct i915_package_c8 {
>  struct i915_runtime_pm {
>         bool suspended;
>         bool gpu_idle;
> +       bool disp_idle;
> +       struct mutex lock;
>  };
>
>  enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9061aa7..94a6127 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -887,6 +887,7 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv);
> +void intel_runtime_update_disp_state(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
>
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 991ff62..9d6d0e1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5501,6 +5501,54 @@ void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv)
>         }
>  }
>
> +static void intel_runtime_pm_disp_idle(struct drm_i915_private *dev_priv)
> +{
> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
> +               return;
> +
> +       mutex_lock(&dev_priv->pm.lock);
> +       if (!dev_priv->pm.disp_idle) {
> +               dev_priv->pm.disp_idle = true;
> +               intel_runtime_pm_put(dev_priv);
> +       }
> +       mutex_unlock(&dev_priv->pm.lock);
> +}
> +
> +static void intel_runtime_pm_disp_busy(struct drm_i915_private *dev_priv)
> +{
> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
> +               return;
> +
> +       mutex_lock(&dev_priv->pm.lock);
> +       if (dev_priv->pm.disp_idle) {
> +               dev_priv->pm.disp_idle = false;
> +               /* This call is coming from an IOCTL so we have already done a
> +                * get_sync. get_noresume should suffice here
> +                */
> +               intel_runtime_pm_get_noresume(dev_priv);
> +       }
> +       mutex_unlock(&dev_priv->pm.lock);
> +}
> +
> +void intel_runtime_update_disp_state(struct drm_i915_private *dev_priv)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       struct intel_crtc *crtc;
> +       bool enabled = false;
> +
> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
> +               return;
> +
> +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
> +               enabled |= crtc->base.enabled;
> +
> +       if (enabled)
> +               intel_runtime_pm_disp_busy(dev_priv);
> +       else
> +               intel_runtime_pm_disp_idle(dev_priv);
> +
> +}
> +
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
> @@ -5547,6 +5595,8 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>
>         dev_priv->pm.suspended = false;
>         dev_priv->pm.gpu_idle = true;
> +       dev_priv->pm.disp_idle = true;
> +       mutex_init(&dev_priv->pm.lock);
>
>         if (!HAS_RUNTIME_PM(dev))
>                 return;
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [RFC 3/6] drm/i915: introduce runtime get/put based on display activity

[Naresh Kumar Kachhi]

On 01/22/2014 07:10 PM, Paulo Zanoni wrote:
> Hi
>
> 2014/1/22  <naresh.kumar.kachhi@intel.com>:
>> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>>
>> Once the display is disabled, we need to call runtime_put to
>> make sure Runtime framework triggers runtime_suspend based on
>> idleness. Similarly when display gets enabled, runtime_get should
>> be called. We have similiar function for pc8 feature, but some
>> platform(BYT) might not have pc8 feature, so creating a generic
>> function for runtime_pm
> Does this patch series help you somehow?
> http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html

Yes, this patch series helps here. Do we know when these patches will
bemerged to nightly build?

>> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c  | 50 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index d1399f9..6a6f046 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1325,6 +1325,8 @@ struct i915_package_c8 {
>>   struct i915_runtime_pm {
>>          bool suspended;
>>          bool gpu_idle;
>> +       bool disp_idle;
>> +       struct mutex lock;
>>   };
>>
>>   enum intel_pipe_crc_source {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9061aa7..94a6127 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -887,6 +887,7 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>>   void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>>   void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv);
>>   void intel_runtime_pm_gpu_idle(struct drm_i915_private *dev_priv);
>> +void intel_runtime_update_disp_state(struct drm_i915_private *dev_priv);
>>   void ilk_wm_get_hw_state(struct drm_device *dev);
>>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 991ff62..9d6d0e1 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5501,6 +5501,54 @@ void intel_runtime_pm_gpu_busy(struct drm_i915_private *dev_priv)
>>          }
>>   }
>>
>> +static void intel_runtime_pm_disp_idle(struct drm_i915_private *dev_priv)
>> +{
>> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
>> +               return;
>> +
>> +       mutex_lock(&dev_priv->pm.lock);
>> +       if (!dev_priv->pm.disp_idle) {
>> +               dev_priv->pm.disp_idle = true;
>> +               intel_runtime_pm_put(dev_priv);
>> +       }
>> +       mutex_unlock(&dev_priv->pm.lock);
>> +}
>> +
>> +static void intel_runtime_pm_disp_busy(struct drm_i915_private *dev_priv)
>> +{
>> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
>> +               return;
>> +
>> +       mutex_lock(&dev_priv->pm.lock);
>> +       if (dev_priv->pm.disp_idle) {
>> +               dev_priv->pm.disp_idle = false;
>> +               /* This call is coming from an IOCTL so we have already done a
>> +                * get_sync. get_noresume should suffice here
>> +                */
>> +               intel_runtime_pm_get_noresume(dev_priv);
>> +       }
>> +       mutex_unlock(&dev_priv->pm.lock);
>> +}
>> +
>> +void intel_runtime_update_disp_state(struct drm_i915_private *dev_priv)
>> +{
>> +       struct drm_device *dev = dev_priv->dev;
>> +       struct intel_crtc *crtc;
>> +       bool enabled = false;
>> +
>> +       if (!HAS_RUNTIME_PM(dev_priv->dev))
>> +               return;
>> +
>> +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
>> +               enabled |= crtc->base.enabled;
>> +
>> +       if (enabled)
>> +               intel_runtime_pm_disp_busy(dev_priv);
>> +       else
>> +               intel_runtime_pm_disp_idle(dev_priv);
>> +
>> +}
>> +
>>   void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>>   {
>>          struct drm_device *dev = dev_priv->dev;
>> @@ -5547,6 +5595,8 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>>
>>          dev_priv->pm.suspended = false;
>>          dev_priv->pm.gpu_idle = true;
>> +       dev_priv->pm.disp_idle = true;
>> +       mutex_init(&dev_priv->pm.lock);
>>
>>          if (!HAS_RUNTIME_PM(dev))
>>                  return;
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

[RFC 4/6] drm/i915/vlv: call runtime get/put based on disp activity

[naresh.kumar.kachhi]

From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

do a runtime_get/put based on the display activity for valleyview
platrform

Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@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 25eae03..52aa5f3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4132,6 +4132,8 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
 
 	if (req_cdclk != cur_cdclk)
 		valleyview_set_cdclk(dev, req_cdclk);
+
+	intel_runtime_update_disp_state(dev_priv);
 }
 
 static void valleyview_crtc_enable(struct drm_crtc *crtc)
-- 
1.8.1.2

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

[naresh.kumar.kachhi]

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>
---
 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

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

[Daniel Vetter]

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
> ---
>  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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Naresh Kumar Kachhi]

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

[RFC 6/6] FOR_UPSTREAM [VPG]: drm/i915: call init_runtime_pm before gem_init

[naresh.kumar.kachhi]

From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>

with current code intel_runtime_pm_gpu_idle is getting called
even before runtime_pm is initialized. Moving runtime_pm_init
before i915_gem_init

Following is the call stack, note: by this time
runtime_pm was not initialized

intel_runtime_pm_gpu_idle+0x37/0x90
i915_gem_retire_requests+0x8d/0xa0
i915_gem_init_seqno+0x48/0x90
i915_gem_set_seqno+0x2a/0x70
i915_gem_init_hw+0x19c/0x300
?
i915_gem_context_init+0x123/0x220
i915_gem_init+0x57/0x1a0
i915_driver_load+0xbf4/0xd50

Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b5af745..85162da 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1511,6 +1511,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->dpio_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 
+	intel_init_runtime_pm(dev_priv);
+
 	intel_pm_setup(dev);
 
 	intel_display_crc_init(dev);
@@ -1674,8 +1676,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (IS_GEN5(dev))
 		intel_gpu_ips_init(dev_priv);
 
-	intel_init_runtime_pm(dev_priv);
-
 	return 0;
 
 out_power_well:
-- 
1.8.1.2

Re: [RFC 6/6] FOR_UPSTREAM [VPG]: drm/i915: call init_runtime_pm before gem_init

[Paulo Zanoni]

2014/1/22  <naresh.kumar.kachhi@intel.com>:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> with current code intel_runtime_pm_gpu_idle is getting called
> even before runtime_pm is initialized. Moving runtime_pm_init
> before i915_gem_init
>
> Following is the call stack, note: by this time
> runtime_pm was not initialized
>
> intel_runtime_pm_gpu_idle+0x37/0x90
> i915_gem_retire_requests+0x8d/0xa0
> i915_gem_init_seqno+0x48/0x90
> i915_gem_set_seqno+0x2a/0x70
> i915_gem_init_hw+0x19c/0x300
> ?
> i915_gem_context_init+0x123/0x220
> i915_gem_init+0x57/0x1a0
> i915_driver_load+0xbf4/0xd50

And why exactly is this a problem? Delaying intel_init_runtime_pm is a
nice thing because we don't want our driver trying to runtime suspend
while it's still being loaded.

>
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b5af745..85162da 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1511,6 +1511,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>         mutex_init(&dev_priv->dpio_lock);
>         mutex_init(&dev_priv->modeset_restore_lock);
>
> +       intel_init_runtime_pm(dev_priv);
> +
>         intel_pm_setup(dev);
>
>         intel_display_crc_init(dev);
> @@ -1674,8 +1676,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>         if (IS_GEN5(dev))
>                 intel_gpu_ips_init(dev_priv);
>
> -       intel_init_runtime_pm(dev_priv);
> -
>         return 0;
>
>  out_power_well:
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [RFC 6/6] FOR_UPSTREAM [VPG]: drm/i915: call init_runtime_pm before gem_init

[Naresh Kumar Kachhi]

[-- Attachment #1.1: Type: text/plain, Size: 2680 bytes --]

On 01/22/2014 07:14 PM, Paulo Zanoni wrote:
> 2014/1/22<naresh.kumar.kachhi@intel.com>:
>> From: Naresh Kumar Kachhi<naresh.kumar.kachhi@intel.com>
>>
>> with current code intel_runtime_pm_gpu_idle is getting called
>> even before runtime_pm is initialized. Moving runtime_pm_init
>> before i915_gem_init
>>
>> Following is the call stack, note: by this time
>> runtime_pm was not initialized
>>
>> intel_runtime_pm_gpu_idle+0x37/0x90
>> i915_gem_retire_requests+0x8d/0xa0
>> i915_gem_init_seqno+0x48/0x90
>> i915_gem_set_seqno+0x2a/0x70
>> i915_gem_init_hw+0x19c/0x300
>> ?
>> i915_gem_context_init+0x123/0x220
>> i915_gem_init+0x57/0x1a0
>> i915_driver_load+0xbf4/0xd50
> And why exactly is this a problem? Delaying intel_init_runtime_pm is a
> nice thing because we don't want our driver trying to runtime suspend
> while it's still being loaded.

pm.gpu_idle will be false (dev_priv is zeroed) until the init_runtime_pm is called.
In the call stack mentioned above we might see a call to intel_runtime_pm_gpu_idle.
If runtime pm is not initialized by now, pm.gpu_idle will be false and we will do a
runtime_put, without a matching runtime_get.
However in you patch
http://lists.freedesktop.org/archives/intel-gfx/2013-December/037729.html, i saw
you have moved the initializing gpu_idle in intel_pm_setup, which will fix this problem.
We will not need this patch with your new patches.
only concern in this patch is that we are scattering the pm initialization across two
function (runtime_pm_init and pm_setup).


>> Signed-off-by: Naresh Kumar Kachhi<naresh.kumar.kachhi@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_dma.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index b5af745..85162da 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -1511,6 +1511,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>          mutex_init(&dev_priv->dpio_lock);
>>          mutex_init(&dev_priv->modeset_restore_lock);
>>
>> +       intel_init_runtime_pm(dev_priv);
>> +
>>          intel_pm_setup(dev);
>>
>>          intel_display_crc_init(dev);
>> @@ -1674,8 +1676,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>          if (IS_GEN5(dev))
>>                  intel_gpu_ips_init(dev_priv);
>>
>> -       intel_init_runtime_pm(dev_priv);
>> -
>>          return 0;
>>
>>   out_power_well:
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>


[-- Attachment #1.2: Type: text/html, Size: 3894 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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