public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* Re: [PATCH] drm/i915: Clean the request structure on alloc
  2014-12-05 17:54 [PATCH] drm/i915: Clean the request structure on alloc Mika Kuoppala
@ 2014-12-05 17:54 ` John Harrison
  2014-12-05 18:02   ` John Harrison
  2014-12-05 18:09   ` Mika Kuoppala
  2014-12-05 21:41 ` shuang.he
  1 sibling, 2 replies; 6+ messages in thread
From: John Harrison @ 2014-12-05 17:54 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

This is already part of the seqno/request patch series and has been 
right from the start. See email 'drm/i915: Zero fill the request structure'.

On 05/12/2014 17:54, Mika Kuoppala wrote:
> Otherwise we might end up referencing uninitialized fields.
> This is apparent when we try to cleanup the preallocated request
> on ring reset, before any request has been submitted to the ring.
> The request->ctx is foobar and we end up freeing the foobarness.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=86959
> References: https://bugs.freedesktop.org/show_bug.cgi?id=86962
> References: https://bugs.freedesktop.org/show_bug.cgi?id=86992
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 79b4ca5..2c6c6f8 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
>   	if (ring->outstanding_lazy_request)
>   		return 0;
>   
> -	request = kmalloc(sizeof(*request), GFP_KERNEL);
> +	request = kzalloc(sizeof(*request), GFP_KERNEL);
>   	if (request == NULL)
>   		return -ENOMEM;
>   

_______________________________________________
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] drm/i915: Clean the request structure on alloc
@ 2014-12-05 17:54 Mika Kuoppala
  2014-12-05 17:54 ` John Harrison
  2014-12-05 21:41 ` shuang.he
  0 siblings, 2 replies; 6+ messages in thread
From: Mika Kuoppala @ 2014-12-05 17:54 UTC (permalink / raw)
  To: intel-gfx

Otherwise we might end up referencing uninitialized fields.
This is apparent when we try to cleanup the preallocated request
on ring reset, before any request has been submitted to the ring.
The request->ctx is foobar and we end up freeing the foobarness.

References: https://bugs.freedesktop.org/show_bug.cgi?id=86959
References: https://bugs.freedesktop.org/show_bug.cgi?id=86962
References: https://bugs.freedesktop.org/show_bug.cgi?id=86992
Cc: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 79b4ca5..2c6c6f8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
 	if (ring->outstanding_lazy_request)
 		return 0;
 
-	request = kmalloc(sizeof(*request), GFP_KERNEL);
+	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
 		return -ENOMEM;
 
-- 
1.9.1

_______________________________________________
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: Clean the request structure on alloc
  2014-12-05 17:54 ` John Harrison
@ 2014-12-05 18:02   ` John Harrison
  2014-12-05 20:44     ` Daniel Vetter
  2014-12-05 18:09   ` Mika Kuoppala
  1 sibling, 1 reply; 6+ messages in thread
From: John Harrison @ 2014-12-05 18:02 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

But yes, the reasoning why it crashes without the zero fill is correct. 
Dodgy context pointers that used to be ignored now get processed. Doing 
the zero fill keeps it all sane.

On 05/12/2014 17:54, John Harrison wrote:
> This is already part of the seqno/request patch series and has been 
> right from the start. See email 'drm/i915: Zero fill the request 
> structure'.
>
> On 05/12/2014 17:54, Mika Kuoppala wrote:
>> Otherwise we might end up referencing uninitialized fields.
>> This is apparent when we try to cleanup the preallocated request
>> on ring reset, before any request has been submitted to the ring.
>> The request->ctx is foobar and we end up freeing the foobarness.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=86959
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=86962
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=86992
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 79b4ca5..2c6c6f8 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs 
>> *ring)
>>       if (ring->outstanding_lazy_request)
>>           return 0;
>>   -    request = kmalloc(sizeof(*request), GFP_KERNEL);
>> +    request = kzalloc(sizeof(*request), GFP_KERNEL);
>>       if (request == NULL)
>>           return -ENOMEM;
>

_______________________________________________
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: Clean the request structure on alloc
  2014-12-05 17:54 ` John Harrison
  2014-12-05 18:02   ` John Harrison
@ 2014-12-05 18:09   ` Mika Kuoppala
  1 sibling, 0 replies; 6+ messages in thread
From: Mika Kuoppala @ 2014-12-05 18:09 UTC (permalink / raw)
  To: John Harrison, intel-gfx

John Harrison <John.C.Harrison@Intel.com> writes:

> This is already part of the seqno/request patch series and has been 
> right from the start. See email 'drm/i915: Zero fill the request structure'.

Indeed. Sorry for missing this one. My patch can be ignored.

But do you agree on failure path?

-Mika

> On 05/12/2014 17:54, Mika Kuoppala wrote:
>> Otherwise we might end up referencing uninitialized fields.
>> This is apparent when we try to cleanup the preallocated request
>> on ring reset, before any request has been submitted to the ring.
>> The request->ctx is foobar and we end up freeing the foobarness.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=86959
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=86962
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=86992
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 79b4ca5..2c6c6f8 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
>>   	if (ring->outstanding_lazy_request)
>>   		return 0;
>>   
>> -	request = kmalloc(sizeof(*request), GFP_KERNEL);
>> +	request = kzalloc(sizeof(*request), GFP_KERNEL);
>>   	if (request == NULL)
>>   		return -ENOMEM;
>>   
_______________________________________________
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: Clean the request structure on alloc
  2014-12-05 18:02   ` John Harrison
@ 2014-12-05 20:44     ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-12-05 20:44 UTC (permalink / raw)
  To: John Harrison; +Cc: intel-gfx

On Fri, Dec 05, 2014 at 06:02:35PM +0000, John Harrison wrote:
> But yes, the reasoning why it crashes without the zero fill is correct.
> Dodgy context pointers that used to be ignored now get processed. Doing the
> zero fill keeps it all sane.
> 
> On 05/12/2014 17:54, John Harrison wrote:
> >This is already part of the seqno/request patch series and has been right
> >from the start. See email 'drm/i915: Zero fill the request structure'.
> >
> >On 05/12/2014 17:54, Mika Kuoppala wrote:
> >>Otherwise we might end up referencing uninitialized fields.
> >>This is apparent when we try to cleanup the preallocated request
> >>on ring reset, before any request has been submitted to the ring.
> >>The request->ctx is foobar and we end up freeing the foobarness.
> >>
> >>References: https://bugs.freedesktop.org/show_bug.cgi?id=86959
> >>References: https://bugs.freedesktop.org/show_bug.cgi?id=86962
> >>References: https://bugs.freedesktop.org/show_bug.cgi?id=86992
> >>Cc: John Harrison <John.C.Harrison@Intel.com>
> >>Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

I've added this explanation plus the note about which commit introduced
the regression to John's patch and merged that one.

Thanks for tracking this down.
-Daniel

> >>---
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>index 79b4ca5..2c6c6f8 100644
> >>--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>@@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs
> >>*ring)
> >>      if (ring->outstanding_lazy_request)
> >>          return 0;
> >>  -    request = kmalloc(sizeof(*request), GFP_KERNEL);
> >>+    request = kzalloc(sizeof(*request), GFP_KERNEL);
> >>      if (request == NULL)
> >>          return -ENOMEM;
> >
> 
> _______________________________________________
> 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: Clean the request structure on alloc
  2014-12-05 17:54 [PATCH] drm/i915: Clean the request structure on alloc Mika Kuoppala
  2014-12-05 17:54 ` John Harrison
@ 2014-12-05 21:41 ` shuang.he
  1 sibling, 0 replies; 6+ messages in thread
From: shuang.he @ 2014-12-05 21:41 UTC (permalink / raw)
  To: shuang.he, intel-gfx, mika.kuoppala

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK                 -4              366/366              362/366
SNB                 -1              450/450              449/450
IVB              +17                 481/498              498/498
BYT                                  289/289              289/289
HSW                 -1              564/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible      DMESG_WARN(1, M26)PASS(4, M37M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_flip-vs-panning      PASS(2, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_plain-flip-ts-check-interruptible      DMESG_WARN(1, M26)PASS(1, M37)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      PASS(3, M37M26)      DMESG_WARN(1, M26)
 SNB  igt_kms_force_connector      NRUN(4, M35M22)PASS(1, M35)      NRUN(1, M35)
 IVB  igt_kms_3d      DMESG_WARN(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-128x128-onscreen      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-128x128-random      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-128x128-sliding      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-256x256-offscreen      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-256x256-onscreen      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-256x256-sliding      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-64x64-onscreen      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-64x64-random      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-64x64-sliding      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_cursor_crc_cursor-size-change      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_fence_pin_leak      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_rotation_crc_primary-rotation      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 IVB  igt_kms_rotation_crc_sprite-rotation      NSPT(1, M34)PASS(14, M4M34M21)      PASS(1, M34)
 HSW  igt_kms_force_connector      NRUN(4, M40M19M20)PASS(1, M40)      NRUN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
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-05 21:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-05 17:54 [PATCH] drm/i915: Clean the request structure on alloc Mika Kuoppala
2014-12-05 17:54 ` John Harrison
2014-12-05 18:02   ` John Harrison
2014-12-05 20:44     ` Daniel Vetter
2014-12-05 18:09   ` Mika Kuoppala
2014-12-05 21:41 ` shuang.he

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