[Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure

[Sean V Kelley]

On 02/09/2015 08:46 AM, Chris Wilson wrote:
> On Sun, Feb 08, 2015 at 03:27:13PM -0800, Sean V Kelley wrote:
>> 
>> 
>> On 01/16/2015 08:05 PM, Daniel Vetter wrote:
>>> On Thu, Jan 15, 2015 at 08:44:00PM +0000, Chris Wilson wrote:
>>>> On Thu, Jan 15, 2015 at 08:36:15PM +0100, Daniel Vetter
>>>> wrote:
>>>>> On Wed, Jan 14, 2015 at 9:34 PM, Chris Wilson 
>>>>> <chris at chris-wilson.co.uk> wrote:
>>>>>> This (partially) reverts
>>>>>> 
>>>>>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd Author:
>>>>>> Chris Wilson <chris at chris-wilson.co.uk> Date:   Tue Mar
>>>>>> 25 13:23:06 2014 +0000
>>>>>> 
>>>>>> drm/i915: Invalidate our pages under memory pressure
>>>>> 
>>>>> Shouldn't we also revert the hunk in
>>>>> i915_gem_free_objects? Without the truncate vs. invalidate
>>>>> disdinction it seems to have lost it's reason for existence
>>>>> ...
>>>> 
>>>> No, setting MADV_DONTNEED has other nice properties during 
>>>> put_pages() - I think it is useful in its own right, for
>>>> example that is where my page stealing code goes...
>>> 
>>> Well right now I can't make sense of this bit any more (tbh I 
>>> didn't with the other code either, but overlooked that while 
>>> reviewing). When it's just there for future work but atm dead
>>> code I prefer for it to get removed. -Daniel
>> 
>> 
>> So can we also revert the hunk in i915_gem_free_objects?  I would
>> like to get this patch merged, it looks like that is the primary
>> concern.
> 
> A problem I have is that the test written to hit the exact
> condition considered in the changelog does not ellict the bug.
> 
> Can you test whether
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index
> 39e032615b31..6269204ba16f 100644 ---
> a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1030,6 +1030,7 @@
> i915_gem_execbuffer_move_to_active(struct list_head *vmas, /*
> update for the implicit flush after a batch */ 
> obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS; } +
> obj->dirty = 1; if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) { 
> i915_gem_request_assign(&obj->last_fenced_req, req); if
> (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
> 
> makes the bug go away. If so, I think the bug is in the caller not 
> setting reloc domains correctly. -Chris

Sure,  I will take a look.

Thanks,

Sean

>