[PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl

[PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl

[Imre Deak]

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

Re: [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl

[Daniel Vetter]

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

[PATCH] tests/pm_rpm: add gem-evict-pwrite subtest

[Imre Deak]

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

Re: [PATCH] drm/i915: add missing rpm ref to

[shuang.he]

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

Re: [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl

[Imre Deak]

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

Re: [PATCH] drm/i915: add missing rpm ref to i915_gem_pwrite_ioctl

[Jani Nikula]

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