Re: [PATCH 08/53] drm/i915: Update alloc_request to return the allocated request

Tomas Elf
2015-03-06T20:17:54Z

On 06/03/2015 17:36, John Harrison wrote:
> On 06/03/2015 16:18, Daniel Vetter wrote:
>> On Thu, Mar 05, 2015 at 08:13:07PM +0000, Tomas Elf wrote:
>>> On 05/03/2015 15:46, John Harrison wrote:
>>>> On 05/03/2015 15:27, Tomas Elf wrote:
>>>>> On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
>>>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>>>
>>>>>> The alloc_request() function does not actually return the newly
>>>>>> allocated
>>>>>> request. Instead, it must be pulled from
>>>>>> ring->outstanding_lazy_request. This
>>>>>> patch fixes this so that code can create a request and start using it
>>>>>> knowing
>>>>>> exactly which request it actually owns.
>>>>>>
>>>>>> For: VIZ-5115
>>>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/i915_drv.h | 3 ++-
>>>>>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 ++-
>>>>>> drivers/gpu/drm/i915/intel_lrc.c | 13 +++++++++----
>>>>>> drivers/gpu/drm/i915/intel_lrc.h | 3 ++-
>>>>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++----
>>>>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++-
>>>>>> 6 files changed, 27 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> index 87a4a2e..90223f208 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> @@ -1909,7 +1909,8 @@ struct drm_i915_private {
>>>>>> /* Abstract the submission mechanism (legacy ringbuffer or
>>>>>> execlists) away */
>>>>>> struct {
>>>>>> int (*alloc_request)(struct intel_engine_cs *ring,
>>>>>> - struct intel_context *ctx);
>>>>>> + struct intel_context *ctx,
>>>>>> + struct drm_i915_gem_request **req_out);
>>>>>> int (*do_execbuf)(struct i915_execbuffer_params *params,
>>>>>> struct drm_i915_gem_execbuffer2 *args,
>>>>>> struct list_head *vmas);
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> index 61471e9..37dcc6f 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> @@ -1353,6 +1353,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>>>>>> void *data,
>>>>>> struct i915_address_space *vm;
>>>>>> struct i915_execbuffer_params params_master; /* XXX: will be
>>>>>> removed later */
>>>>>> struct i915_execbuffer_params *params = &params_master;
>>>>>> + struct drm_i915_gem_request *request;
>>>>> Please initialize request to NULL. If you accidentally dereference it
>>>>> before it is allocated (seeing as the allocation is several pages down
>>>> >from here) you get a null pointer exception, which is a clear
>>>>> indication that you did something stupid. Otherwise it's not clear
>>>>> what will happen (sure, page fault, but null pointer exception is a
>>>>> better failure indication).
>>>> That should generate a 'use before assignment' compiler warning.
>>> That assumes that the developer in question isn't too busy hacking to
>>> check
>>> for warnings (in all honesty that developer probably would've been me
>>> ;)).
>>> Sure, you should always check for warnings but if we can save this
>>> developer
>>> some time by giving them a clear run-time indication aside from the
>>> compile-time warning then that would not be a bad thing. I've been there
>>> myself a few times and I know times in the past where this would've
>>> saved me
>>> the time it takes to rebuild and redeploy the kernel once.
>> kbuild is _very_ good at catching and reporting these. And the linux
>> kernel generally has a 0 warnings rule, which means even when it slips
>> through a lot of developers will get mad at me for not spotting it.
>>
>> I think relying upon gcc to do its job here is safe enough. We only
>> initialize stack variables if it's really needed, or if gcc is too dense
>> to figure it out itself.
>> -Daniel
>
> I agree with Daniel. If you ignore warnings in your own test builds, you
> get what you deserve. If you check warnings into the tree then you
> should be shot. Or worse.
>
>

Reviewed-by: Tomas Elf <tomas.elf@intel.com>

Thanks,
Tomas