Re: [PATCH] drm/i915: Fix an incorrect free rather than derefence issue.

[Nick Hoath <nicholas.hoath@intel.com>]

On 13/02/2015 09:32, Daniel Vetter wrote:
> On Thu, Feb 12, 2015 at 12:29:21PM +0000, Nick Hoath wrote:
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652
>>
>> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
>
> Commit message is missing the absolutely crucial detail about which patch
> introduced this regression:
>
> commit 6d3d8274bc45de4babb62d64562d92af984dd238
> Author:     Nick Hoath <nicholas.hoath@intel.com>
> AuthorDate: Thu Jan 15 13:10:39 2015 +0000
>
>      drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request
>
> Another thing I've noticed is that we explicitly drop the context
> reference for the request before dropping the request reference. Without
> clearing the req->ctx pointer. That has a very high chance to leading to
> tears, imo the context unreferenceing should be pushed into
> i915_gem_request_free.
>
> Except that it's there already, which means we have a double unref now?

Looking at the code, it looks like that's the case.

>
> Also this patch is for the legacy ringbuffer code, but the referenced bug
> is for gen8+ execlists. We're definitely not running this code here I
> think.

i915_gem_reset_ring_cleanup is used in execlists in the hang recovery case.

>
> Imo step one is to drop all the explicit ctx refcounting for req->ctx and
> always rely on the implicit reference. Then see what happens.

I agree that the refcounting needs re-evaluating after the merge of 
execlist queue entries & requests, however I think the cleanup of the 
double unref/removing the refcounting should be done in another 
patchset. This patch is purely to fix the issue raised in 88652. Depends 
on the relative priorities.

>
> Cheers, Daniel
>
>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 2 +-
>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 1765989..dc10d86 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2661,7 +2661,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
>>   			intel_lr_context_unpin(ring, submit_req->ctx);
>>
>>   		i915_gem_context_unreference(submit_req->ctx);
>> -		kfree(submit_req);
>> +		i915_gem_request_unreference(submit_req);
>>   	}
>>
>>   	/*
>> --
>> 2.1.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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