public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl
@ 2014-11-12 14:40 Imre Deak
  2014-11-12 15:32 ` Daniel Vetter
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Imre Deak @ 2014-11-12 14:40 UTC (permalink / raw)
  To: intel-gfx

Without this RPM ref we can hit the device suspended WARN via:
i915_gem_object_pin()->ggtt_bind_vma->gen6_ggtt_insert_entries(). I
noticed this on my BYT while keeping the i915 device in runtime
suspended state for a while. I chose this place to take the ref to
avoid the possible deadlock via the mutex_lock taken both later in this
function and in the runtime suspend handler. This can happen if an RPM
suspend event is queued and need to be flushed before taking the RPM
ref.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 86cf428..ec2ee28 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1075,6 +1075,7 @@ int
 i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		      struct drm_file *file)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_pwrite *args = data;
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -1094,9 +1095,11 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 			return -EFAULT;
 	}
 
+	intel_runtime_pm_get(dev_priv);
+
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
-		return ret;
+		goto put_rpm;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
@@ -1148,6 +1151,9 @@ out:
 	drm_gem_object_unreference(&obj->base);
 unlock:
 	mutex_unlock(&dev->struct_mutex);
+put_rpm:
+	intel_runtime_pm_put(dev_priv);
+
 	return ret;
 }
 
-- 
1.8.4

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl
  2014-11-12 14:40 [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
@ 2014-11-12 15:32 ` Daniel Vetter
  2014-11-13 12:58   ` [PATCH] tests/pm_rpm: add gem-evict-pwrite subtest Imre Deak
  2014-11-13  2:10 ` [PATCH] drm/i915: add missing rpm ref to shuang.he
  2014-12-16 18:12 ` [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-11-12 15:32 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

On Wed, Nov 12, 2014 at 04:40:35PM +0200, Imre Deak wrote:
> Without this RPM ref we can hit the device suspended WARN via:
> i915_gem_object_pin()->ggtt_bind_vma->gen6_ggtt_insert_entries(). I
> noticed this on my BYT while keeping the i915 device in runtime
> suspended state for a while. I chose this place to take the ref to
> avoid the possible deadlock via the mutex_lock taken both later in this
> function and in the runtime suspend handler. This can happen if an RPM
> suspend event is queued and need to be flushed before taking the RPM
> ref.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

We should be able to hit this easily with an igt by doing a pwrite to an
object which is attached to a cursor. I.e.
1. Set up mode with cursor visible.
2. dpms off
-> ensure rpm is entered
3. pwrite to cursor -> boom.

Can you please add this as a new subtest to pm_rpm.c? I think it should
fit fairly easily there.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 86cf428..ec2ee28 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1075,6 +1075,7 @@ int
>  i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		      struct drm_file *file)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_pwrite *args = data;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
> @@ -1094,9 +1095,11 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  			return -EFAULT;
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
> -		return ret;
> +		goto put_rpm;
>  
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
> @@ -1148,6 +1151,9 @@ out:
>  	drm_gem_object_unreference(&obj->base);
>  unlock:
>  	mutex_unlock(&dev->struct_mutex);
> +put_rpm:
> +	intel_runtime_pm_put(dev_priv);
> +
>  	return ret;
>  }
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: add missing rpm ref to
  2014-11-12 14:40 [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
  2014-11-12 15:32 ` Daniel Vetter
@ 2014-11-13  2:10 ` shuang.he
  2014-12-16 18:12 ` [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
  2 siblings, 0 replies; 6+ messages in thread
From: shuang.he @ 2014-11-13  2:10 UTC (permalink / raw)
  To: shuang.he, intel-gfx, imre.deak

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=291/291->290/291
PNV: pass/total=356/356->356/356
ILK: pass/total=372/372->371/372
IVB: pass/total=545/546->543/546
SNB: pass/total=380/380->379/380
HSW: pass/total=579/579->579/579
BDW: pass/total=422/423->422/423
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(9, M36M31)PASS(1, M31) -> TIMEOUT(1, M31)PASS(3, M31)
ILK: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, FAIL(2, M26)DMESG_FAIL(1, M26)TIMEOUT(13, M37M6M26)PASS(1, M26) -> TIMEOUT(1, M37)PASS(3, M37)
IVB: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc, NSPT(5, M21M34)PASS(8, M34M21) -> NSPT(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-128x128-sliding, PASS(4, M21) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-64x64-offscreen, PASS(4, M21) -> DMESG_WARN(1, M21)PASS(3, M21)
IVB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(12, M34M21)PASS(1, M21) -> TIMEOUT(1, M21)PASS(3, M21)
SNB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(18, M35M22)PASS(1, M35) -> TIMEOUT(1, M22)PASS(3, M22)
BDW: Intel_gpu_tools, igt_gem_reset_stats_ban-bsd, DMESG_WARN(1, M28)PASS(15, M42M30M28) -> PASS(4, M42)
BDW: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(15, M42M30M28)PASS(1, M28) -> TIMEOUT(1, M42)PASS(3, M42)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] tests/pm_rpm: add gem-evict-pwrite subtest
  2014-11-12 15:32 ` Daniel Vetter
@ 2014-11-13 12:58   ` Imre Deak
  0 siblings, 0 replies; 6+ messages in thread
From: Imre Deak @ 2014-11-13 12:58 UTC (permalink / raw)
  To: intel-gfx; +Cc: Daniel Vetter

This triggers a device suspended WARN in the kernel in
gen6_ggtt_insert_entries() while calling the GEM pwrite ioctl.

The sequence is suggested by Daniel.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 lib/igt_aux.h  |  3 +++
 tests/pm_rpm.c | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index 9b42918..7bca11c 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -31,6 +31,9 @@
 #include <intel_bufmgr.h>
 #include <stdbool.h>
 
+extern drm_intel_bo **trash_bos;
+extern int num_trash_bos;
+
 /* auxialiary igt helpers from igt_aux.c */
 /* generally useful helpers */
 void igt_fork_signal_helper(void);
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 1e65c04..c120d75 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1319,6 +1319,29 @@ static void gem_idle_subtest(void)
 	gem_quiescent_gpu(drm_fd);
 }
 
+static void gem_evict_pwrite_subtest(void)
+{
+	static drm_intel_bufmgr *bufmgr;
+	uint32_t buf;
+	int i;
+
+	bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
+	igt_assert(bufmgr);
+	igt_init_aperture_trashers(bufmgr);
+
+	igt_trash_aperture();
+
+	disable_or_dpms_all_screens_and_wait(&ms_data, true);
+	igt_assert(wait_for_suspended());
+
+	buf = 0;
+	for (i = 0; i < num_trash_bos; i++)
+		gem_write(drm_fd, trash_bos[i]->handle, 0, &buf, sizeof(buf));
+
+	igt_cleanup_aperture_trashers();
+	drm_intel_bufmgr_destroy(bufmgr);
+}
+
 /* This also triggered WARNs on dmesg at some point. */
 static void reg_read_ioctl_subtest(void)
 {
@@ -1830,6 +1853,8 @@ int main(int argc, char *argv[])
 		gem_execbuf_subtest();
 	igt_subtest("gem-idle")
 		gem_idle_subtest();
+	igt_subtest("gem-evict-pwrite")
+		gem_evict_pwrite_subtest();
 
 	/* Planes and cursors */
 	igt_subtest("cursor")
-- 
1.8.4

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl
  2014-11-12 14:40 [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
  2014-11-12 15:32 ` Daniel Vetter
  2014-11-13  2:10 ` [PATCH] drm/i915: add missing rpm ref to shuang.he
@ 2014-12-16 18:12 ` Imre Deak
  2014-12-18 13:45   ` Jani Nikula
  2 siblings, 1 reply; 6+ messages in thread
From: Imre Deak @ 2014-12-16 18:12 UTC (permalink / raw)
  To: intel-gfx

On Wed, 2014-11-12 at 16:40 +0200, Imre Deak wrote:
> Without this RPM ref we can hit the device suspended WARN via:
> i915_gem_object_pin()->ggtt_bind_vma->gen6_ggtt_insert_entries(). I
> noticed this on my BYT while keeping the i915 device in runtime
> suspended state for a while. I chose this place to take the ref to
> avoid the possible deadlock via the mutex_lock taken both later in this
> function and in the runtime suspend handler. This can happen if an RPM
> suspend event is queued and need to be flushed before taking the RPM
> ref.

Testcase: igt/pm_rpm/gem-evict-pwrite
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87363

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 86cf428..ec2ee28 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1075,6 +1075,7 @@ int
>  i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		      struct drm_file *file)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_pwrite *args = data;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
> @@ -1094,9 +1095,11 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  			return -EFAULT;
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
> -		return ret;
> +		goto put_rpm;
>  
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
> @@ -1148,6 +1151,9 @@ out:
>  	drm_gem_object_unreference(&obj->base);
>  unlock:
>  	mutex_unlock(&dev->struct_mutex);
> +put_rpm:
> +	intel_runtime_pm_put(dev_priv);
> +
>  	return ret;
>  }
>  


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl
  2014-12-16 18:12 ` [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
@ 2014-12-18 13:45   ` Jani Nikula
  0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2014-12-18 13:45 UTC (permalink / raw)
  To: imre.deak, intel-gfx

On Tue, 16 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> On Wed, 2014-11-12 at 16:40 +0200, Imre Deak wrote:
>> Without this RPM ref we can hit the device suspended WARN via:
>> i915_gem_object_pin()->ggtt_bind_vma->gen6_ggtt_insert_entries(). I
>> noticed this on my BYT while keeping the i915 device in runtime
>> suspended state for a while. I chose this place to take the ref to
>> avoid the possible deadlock via the mutex_lock taken both later in this
>> function and in the runtime suspend handler. This can happen if an RPM
>> suspend event is queued and need to be flushed before taking the RPM
>> ref.
>
> Testcase: igt/pm_rpm/gem-evict-pwrite
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87363

Pushed to drm-intel-next-fixes, thanks for the patch.

BR,
Jani.

>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 86cf428..ec2ee28 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1075,6 +1075,7 @@ int
>>  i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>>  		      struct drm_file *file)
>>  {
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_i915_gem_pwrite *args = data;
>>  	struct drm_i915_gem_object *obj;
>>  	int ret;
>> @@ -1094,9 +1095,11 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>>  			return -EFAULT;
>>  	}
>>  
>> +	intel_runtime_pm_get(dev_priv);
>> +
>>  	ret = i915_mutex_lock_interruptible(dev);
>>  	if (ret)
>> -		return ret;
>> +		goto put_rpm;
>>  
>>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>>  	if (&obj->base == NULL) {
>> @@ -1148,6 +1151,9 @@ out:
>>  	drm_gem_object_unreference(&obj->base);
>>  unlock:
>>  	mutex_unlock(&dev->struct_mutex);
>> +put_rpm:
>> +	intel_runtime_pm_put(dev_priv);
>> +
>>  	return ret;
>>  }
>>  
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-18 13:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-12 14:40 [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
2014-11-12 15:32 ` Daniel Vetter
2014-11-13 12:58   ` [PATCH] tests/pm_rpm: add gem-evict-pwrite subtest Imre Deak
2014-11-13  2:10 ` [PATCH] drm/i915: add missing rpm ref to shuang.he
2014-12-16 18:12 ` [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl Imre Deak
2014-12-18 13:45   ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox