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

[John Harrison]

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.