[Intel-gfx] [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 at intel.com> wrote:
> > On 29/06/15 12:39, Jani Nikula wrote:
> >> On Wed, 06 May 2015, Daniel Vetter <daniel at ffwll.ch> wrote:
> >>> On Thu, Apr 30, 2015 at 01:54:41PM +0100, Dave Gordon wrote:
> >>>> On 29/04/15 17:10, yu.dai at intel.com wrote:
> >>>>> From: Alex Dai <yu.dai at 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 at 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 at intel.com>
> >>>> with input also from John Harrison <john.c.harrison at 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