[PATCH] drm/i915: Preallocate request before access of the ring

[PATCH] drm/i915: Preallocate request before access of the ring

[yu.dai]

From: Alex Dai <yu.dai@intel.com>

This is to avoid bad IO access caused by writing NOOP to wrap the
ring buffer whilst ring is unpinned.

Signed-off-by: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 732fd63..3e8fcfd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
 	if (ret)
 		return ret;
 
-	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
+	/* Preallocate the olr before touching the ring */
+	ret = i915_gem_request_alloc(ring, ctx);
 	if (ret)
 		return ret;
 
-	/* Preallocate the olr before touching the ring */
-	ret = i915_gem_request_alloc(ring, ctx);
+	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
 	if (ret)
 		return ret;
 
-- 
1.9.1

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

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[Dave Gordon]

On 29/04/15 17:10, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> This is to avoid bad IO access caused by writing NOOP to wrap the
> ring buffer whilst ring is unpinned.
> 
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 732fd63..3e8fcfd 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
>  	if (ret)
>  		return ret;
>  
> -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> +	/* Preallocate the olr before touching the ring */
> +	ret = i915_gem_request_alloc(ring, ctx);
>  	if (ret)
>  		return ret;
>  
> -	/* Preallocate the olr before touching the ring */
> -	ret = i915_gem_request_alloc(ring, ctx);
> +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>  	if (ret)
>  		return ret;

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

with input also from John Harrison <john.c.harrison@intel.com>, who
would like to point out that this will be superceded by the Anti-OLR
patches already posted. (In that model, the request will be allocated
much earlier, and passed around explicitly rather than dangling from the
context).

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

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6292
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -3              227/227              224/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_dummy_reloc_loop@render      FAIL(1)PASS(13)      TIMEOUT(1)PASS(1)
*BYT  igt@gem_exec_parse@bitmasks      FAIL(1)PASS(6)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(7)PASS(7)      TIMEOUT(1)PASS(1)
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

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[Daniel Vetter]

On Thu, Apr 30, 2015 at 01:54:41PM +0100, Dave Gordon wrote:
> On 29/04/15 17:10, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> > 
> > This is to avoid bad IO access caused by writing NOOP to wrap the
> > ring buffer whilst ring is unpinned.
> > 
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 732fd63..3e8fcfd 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> > +	/* Preallocate the olr before touching the ring */
> > +	ret = i915_gem_request_alloc(ring, ctx);
> >  	if (ret)
> >  		return ret;
> >  
> > -	/* Preallocate the olr before touching the ring */
> > -	ret = i915_gem_request_alloc(ring, ctx);
> > +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> >  	if (ret)
> >  		return ret;
> 
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> 
> with input also from John Harrison <john.c.harrison@intel.com>, who
> would like to point out that this will be superceded by the Anti-OLR
> patches already posted. (In that model, the request will be allocated
> much earlier, and passed around explicitly rather than dangling from the
> context).

Do we need this for execlist in general, i.e. cc: stable? Where's the bug
report/igt testcase?

For serious-looking bugs please add more details like that to the commit
message, otherwise maintainers have no idea where to apply a patch.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[Jani Nikula]

On Wed, 06 May 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 30, 2015 at 01:54:41PM +0100, Dave Gordon wrote:
>> On 29/04/15 17:10, yu.dai@intel.com wrote:
>> > From: Alex Dai <yu.dai@intel.com>
>> > 
>> > This is to avoid bad IO access caused by writing NOOP to wrap the
>> > ring buffer whilst ring is unpinned.
>> > 
>> > Signed-off-by: Alex Dai <yu.dai@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> > index 732fd63..3e8fcfd 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> > @@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
>> >  	if (ret)
>> >  		return ret;
>> >  
>> > -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>> > +	/* Preallocate the olr before touching the ring */
>> > +	ret = i915_gem_request_alloc(ring, ctx);
>> >  	if (ret)
>> >  		return ret;
>> >  
>> > -	/* Preallocate the olr before touching the ring */
>> > -	ret = i915_gem_request_alloc(ring, ctx);
>> > +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>> >  	if (ret)
>> >  		return ret;
>> 
>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>> 
>> with input also from John Harrison <john.c.harrison@intel.com>, who
>> would like to point out that this will be superceded by the Anti-OLR
>> patches already posted. (In that model, the request will be allocated
>> much earlier, and passed around explicitly rather than dangling from the
>> context).
>
> Do we need this for execlist in general, i.e. cc: stable? Where's the bug
> report/igt testcase?
>
> For serious-looking bugs please add more details like that to the commit
> message, otherwise maintainers have no idea where to apply a patch.

And due to no reply, the maintainers have forgotten about patches like
this. Dropping from my fixes queue.

BR,
Jani.






>
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[Dave Gordon]

On 29/06/15 12:39, Jani Nikula wrote:
> On Wed, 06 May 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Thu, Apr 30, 2015 at 01:54:41PM +0100, Dave Gordon wrote:
>>> On 29/04/15 17:10, yu.dai@intel.com wrote:
>>>> From: Alex Dai <yu.dai@intel.com>
>>>>
>>>> This is to avoid bad IO access caused by writing NOOP to wrap the
>>>> ring buffer whilst ring is unpinned.
>>>>
>>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index 732fd63..3e8fcfd 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>>>> +	/* Preallocate the olr before touching the ring */
>>>> +	ret = i915_gem_request_alloc(ring, ctx);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	/* Preallocate the olr before touching the ring */
>>>> -	ret = i915_gem_request_alloc(ring, ctx);
>>>> +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>>>>  	if (ret)
>>>>  		return ret;
>>>
>>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>>> with input also from John Harrison <john.c.harrison@intel.com>, who
>>> would like to point out that this will be superceded by the Anti-OLR
>>> patches already posted. (In that model, the request will be allocated
>>> much earlier, and passed around explicitly rather than dangling from the
>>> context).
>>
>> Do we need this for execlist in general, i.e. cc: stable? Where's the bug
>> report/igt testcase?
>>
>> For serious-looking bugs please add more details like that to the commit
>> message, otherwise maintainers have no idea where to apply a patch.
> 
> And due to no reply, the maintainers have forgotten about patches like
> this. Dropping from my fixes queue.
> 
> BR,
> Jani.

This became obsolete with the merge of John Harrison's patchset
	"Remove the outstanding_lazy_request"

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

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[Jani Nikula]

On Mon, 29 Jun 2015, Dave Gordon <david.s.gordon@intel.com> wrote:
> On 29/06/15 12:39, Jani Nikula wrote:
>> On Wed, 06 May 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Thu, Apr 30, 2015 at 01:54:41PM +0100, Dave Gordon wrote:
>>>> On 29/04/15 17:10, yu.dai@intel.com wrote:
>>>>> From: Alex Dai <yu.dai@intel.com>
>>>>>
>>>>> This is to avoid bad IO access caused by writing NOOP to wrap the
>>>>> ring buffer whilst ring is unpinned.
>>>>>
>>>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> index 732fd63..3e8fcfd 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> @@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>  
>>>>> -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>>>>> +	/* Preallocate the olr before touching the ring */
>>>>> +	ret = i915_gem_request_alloc(ring, ctx);
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>  
>>>>> -	/* Preallocate the olr before touching the ring */
>>>>> -	ret = i915_gem_request_alloc(ring, ctx);
>>>>> +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>
>>>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>>>> with input also from John Harrison <john.c.harrison@intel.com>, who
>>>> would like to point out that this will be superceded by the Anti-OLR
>>>> patches already posted. (In that model, the request will be allocated
>>>> much earlier, and passed around explicitly rather than dangling from the
>>>> context).
>>>
>>> Do we need this for execlist in general, i.e. cc: stable? Where's the bug
>>> report/igt testcase?
>>>
>>> For serious-looking bugs please add more details like that to the commit
>>> message, otherwise maintainers have no idea where to apply a patch.
>> 
>> And due to no reply, the maintainers have forgotten about patches like
>> this. Dropping from my fixes queue.
>> 
>> BR,
>> Jani.
>
> This became obsolete with the merge of John Harrison's patchset
> 	"Remove the outstanding_lazy_request"

Thanks.

BR,
Jani.


>
> .Dave.

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

Re: [PATCH] drm/i915: Preallocate request before access of the ring

[Daniel Vetter]

On Mon, Jun 29, 2015 at 05:44:40PM +0300, Jani Nikula wrote:
> On Mon, 29 Jun 2015, Dave Gordon <david.s.gordon@intel.com> wrote:
> > On 29/06/15 12:39, Jani Nikula wrote:
> >> On Wed, 06 May 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>> On Thu, Apr 30, 2015 at 01:54:41PM +0100, Dave Gordon wrote:
> >>>> On 29/04/15 17:10, yu.dai@intel.com wrote:
> >>>>> From: Alex Dai <yu.dai@intel.com>
> >>>>>
> >>>>> This is to avoid bad IO access caused by writing NOOP to wrap the
> >>>>> ring buffer whilst ring is unpinned.
> >>>>>
> >>>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
> >>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>> index 732fd63..3e8fcfd 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>> @@ -803,12 +803,12 @@ static int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
> >>>>>  	if (ret)
> >>>>>  		return ret;
> >>>>>  
> >>>>> -	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> >>>>> +	/* Preallocate the olr before touching the ring */
> >>>>> +	ret = i915_gem_request_alloc(ring, ctx);
> >>>>>  	if (ret)
> >>>>>  		return ret;
> >>>>>  
> >>>>> -	/* Preallocate the olr before touching the ring */
> >>>>> -	ret = i915_gem_request_alloc(ring, ctx);
> >>>>> +	ret = logical_ring_prepare(ringbuf, ctx, num_dwords * sizeof(uint32_t));
> >>>>>  	if (ret)
> >>>>>  		return ret;
> >>>>
> >>>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> >>>> with input also from John Harrison <john.c.harrison@intel.com>, who
> >>>> would like to point out that this will be superceded by the Anti-OLR
> >>>> patches already posted. (In that model, the request will be allocated
> >>>> much earlier, and passed around explicitly rather than dangling from the
> >>>> context).
> >>>
> >>> Do we need this for execlist in general, i.e. cc: stable? Where's the bug
> >>> report/igt testcase?
> >>>
> >>> For serious-looking bugs please add more details like that to the commit
> >>> message, otherwise maintainers have no idea where to apply a patch.
> >> 
> >> And due to no reply, the maintainers have forgotten about patches like
> >> this. Dropping from my fixes queue.
> >> 
> >> BR,
> >> Jani.
> >
> > This became obsolete with the merge of John Harrison's patchset
> > 	"Remove the outstanding_lazy_request"
> 
> Thanks.

It only became obsolete for dinq/4.3, I think we still need this for any
other kernels shipping with execlist. Dave, can you please confirm?

I tried to digg out when exactly we enabled execlist, but there's a module
options loop and so I got lost in the recursion ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx