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@xxxxxxxxx wrote:
> >>>From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >>>
> >>>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@xxxxxxxxx>
> >>>---
> >>> 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 = ¶ms_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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
