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

[Tomas Elf]

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 at Intel.com wrote:
>>>>>> From: John Harrison <John.C.Harrison at 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 at 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 at intel.com>

Thanks,
Tomas