| Message ID | 1461322677-20728-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/04/16 11:57, Matthew Auld wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Propagate the real error from drm_gem_object_init(). Note this also > fixes some confusion in the error return from i915_gem_alloc_object... > > v2: > (Matthew Auld) > - updated new users of gem_alloc_object from latest drm-nightly > - replaced occurrences of IS_ERR_OR_NULL() with IS_ERR() > v3: > (Joonas Lahtinen) > - fix double "From:" in commit message > - add goto teardown path > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 21 +++++++++++++-------- > drivers/gpu/drm/i915/i915_gem_batch_pool.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_context.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_render_state.c | 7 +++++-- > drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > drivers/gpu/drm/i915/intel_fbdev.c | 4 ++-- > drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++---- > drivers/gpu/drm/i915/intel_overlay.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 19 ++++++++++--------- > 10 files changed, 44 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 261a3ef..c6c17dd 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -382,8 +382,8 @@ i915_gem_create(struct drm_file *file, > > /* Allocate the new object */ > obj = i915_gem_alloc_object(dev, size); > - if (obj == NULL) > - return -ENOMEM; > + if (IS_ERR(obj)) > + return PTR_ERR(obj); > > ret = drm_gem_handle_create(file, &obj->base, &handle); > /* drop reference from allocate - handle holds it now */ > @@ -4498,15 +4498,15 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > struct drm_i915_gem_object *obj; > struct address_space *mapping; > gfp_t mask; > + int ret; > > obj = i915_gem_object_alloc(dev); > if (obj == NULL) > - return NULL; > + return ERR_PTR(-ENOMEM); The two changes above looked really really confusing, where one tests the returned pointer and returns it if it's an ERR_PTR, and the other tests for NULL and returns ERR_PTR(-ENOMEM). Then I realised one was i915_gem_alloc_object() and the other was i915_gem_object_alloc()! Can we please get rid of one or the other? Since we generally use subsystem_class_action naming, I'd suggest keeping (the low-level memory-allocator) i915_gem_object_alloc(), and renaming the high-level i915_gem_alloc_object() to i915_gem_object_create() or similar. > - if (drm_gem_object_init(dev, &obj->base, size) != 0) { > - i915_gem_object_free(obj); > - return NULL; > - } > + ret = drm_gem_object_init(dev, &obj->base, size); > + if (ret) > + goto fail; > > mask = GFP_HIGHUSER | __GFP_RECLAIMABLE; > if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) { > @@ -4543,6 +4543,11 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > trace_i915_gem_object_create(obj); Oh and BTW i915_gem_alloc_object() already calls itself i915_gem_object_create() in trace messages! .Dave.
On pe, 2016-04-22 at 12:59 +0100, Dave Gordon wrote: > On 22/04/16 11:57, Matthew Auld wrote: > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Propagate the real error from drm_gem_object_init(). Note this also > > fixes some confusion in the error return from i915_gem_alloc_object... > > > > v2: > > (Matthew Auld) > > - updated new users of gem_alloc_object from latest drm-nightly > > - replaced occurrences of IS_ERR_OR_NULL() with IS_ERR() > > v3: > > (Joonas Lahtinen) > > - fix double "From:" in commit message > > - add goto teardown path > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 21 +++++++++++++-------- > > drivers/gpu/drm/i915/i915_gem_batch_pool.c | 4 ++-- > > drivers/gpu/drm/i915/i915_gem_context.c | 4 ++-- > > drivers/gpu/drm/i915/i915_gem_render_state.c | 7 +++++-- > > drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > drivers/gpu/drm/i915/intel_fbdev.c | 4 ++-- > > drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++---- > > drivers/gpu/drm/i915/intel_overlay.c | 2 +- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 19 ++++++++++--------- > > 10 files changed, 44 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 261a3ef..c6c17dd 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -382,8 +382,8 @@ i915_gem_create(struct drm_file *file, > > > > /* Allocate the new object */ > > obj = i915_gem_alloc_object(dev, size); > > - if (obj == NULL) > > - return -ENOMEM; > > + if (IS_ERR(obj)) > > + return PTR_ERR(obj); > > > > ret = drm_gem_handle_create(file, &obj->base, &handle); > > /* drop reference from allocate - handle holds it now */ > > @@ -4498,15 +4498,15 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > struct drm_i915_gem_object *obj; > > struct address_space *mapping; > > gfp_t mask; > > + int ret; > > > > obj = i915_gem_object_alloc(dev); > > if (obj == NULL) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > The two changes above looked really really confusing, where one tests > the returned pointer and returns it if it's an ERR_PTR, and the other > tests for NULL and returns ERR_PTR(-ENOMEM). > > Then I realised one was i915_gem_alloc_object() and the other was > i915_gem_object_alloc()! Yep, noticed that a few days ago too, +1 on correcting it. Regards, Joonas > > Can we please get rid of one or the other? Since we generally use > subsystem_class_action naming, I'd suggest keeping (the low-level > memory-allocator) i915_gem_object_alloc(), and renaming the high-level > i915_gem_alloc_object() to i915_gem_object_create() or similar. > > > > > - if (drm_gem_object_init(dev, &obj->base, size) != 0) { > > - i915_gem_object_free(obj); > > - return NULL; > > - } > > + ret = drm_gem_object_init(dev, &obj->base, size); > > + if (ret) > > + goto fail; > > > > mask = GFP_HIGHUSER | __GFP_RECLAIMABLE; > > if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) { > > @@ -4543,6 +4543,11 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > trace_i915_gem_object_create(obj); > Oh and BTW i915_gem_alloc_object() already calls itself > i915_gem_object_create() in trace messages! > > .Dave. > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 261a3ef..c6c17dd 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -382,8 +382,8 @@ i915_gem_create(struct drm_file *file, /* Allocate the new object */ obj = i915_gem_alloc_object(dev, size); - if (obj == NULL) - return -ENOMEM; + if (IS_ERR(obj)) + return PTR_ERR(obj); ret = drm_gem_handle_create(file, &obj->base, &handle); /* drop reference from allocate - handle holds it now */ @@ -4498,15 +4498,15 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, struct drm_i915_gem_object *obj; struct address_space *mapping; gfp_t mask; + int ret; obj = i915_gem_object_alloc(dev); if (obj == NULL) - return NULL; + return ERR_PTR(-ENOMEM); - if (drm_gem_object_init(dev, &obj->base, size) != 0) { - i915_gem_object_free(obj); - return NULL; - } + ret = drm_gem_object_init(dev, &obj->base, size); + if (ret) + goto fail; mask = GFP_HIGHUSER | __GFP_RECLAIMABLE; if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) { @@ -4543,6 +4543,11 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, trace_i915_gem_object_create(obj); return obj; + +fail: + i915_gem_object_free(obj); + + return ERR_PTR(ret); } static bool discard_backing_storage(struct drm_i915_gem_object *obj) @@ -5362,7 +5367,7 @@ i915_gem_object_create_from_data(struct drm_device *dev, int ret; obj = i915_gem_alloc_object(dev, round_up(size, PAGE_SIZE)); - if (IS_ERR_OR_NULL(obj)) + if (IS_ERR(obj)) return obj; ret = i915_gem_object_set_to_cpu_domain(obj, true); diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c index 7bf2f3f..d79caa2 100644 --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c @@ -135,8 +135,8 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool, int ret; obj = i915_gem_alloc_object(pool->dev, size); - if (obj == NULL) - return ERR_PTR(-ENOMEM); + if (IS_ERR(obj)) + return obj; ret = i915_gem_object_get_pages(obj); if (ret) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index e5acc39..34f4930 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -179,8 +179,8 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size) int ret; obj = i915_gem_alloc_object(dev, size); - if (obj == NULL) - return ERR_PTR(-ENOMEM); + if (IS_ERR(obj)) + return obj; /* * Try to make the context utilize L3 as well as LLC. diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c index 71611bf..071c11e 100644 --- a/drivers/gpu/drm/i915/i915_gem_render_state.c +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c @@ -58,8 +58,11 @@ static int render_state_init(struct render_state *so, struct drm_device *dev) return -EINVAL; so->obj = i915_gem_alloc_object(dev, 4096); - if (so->obj == NULL) - return -ENOMEM; + if (IS_ERR(so->obj)) { + ret = PTR_ERR(so->obj); + so->obj = NULL; + return ret; + } ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0); if (ret) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index d40c13f..85122d9 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -588,7 +588,7 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev, struct drm_i915_gem_object *obj; obj = i915_gem_alloc_object(dev, size); - if (!obj) + if (IS_ERR(obj)) return NULL; if (i915_gem_object_get_pages(obj)) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ff60241..5aff4ca 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10320,8 +10320,8 @@ intel_framebuffer_create_for_mode(struct drm_device *dev, obj = i915_gem_alloc_object(dev, intel_framebuffer_size_for_mode(mode, bpp)); - if (obj == NULL) - return ERR_PTR(-ENOMEM); + if (IS_ERR(obj)) + return ERR_CAST(obj); mode_cmd.width = mode->hdisplay; mode_cmd.height = mode->vdisplay; diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 79ac202..7047b38 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -151,9 +151,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper, obj = i915_gem_object_create_stolen(dev, size); if (obj == NULL) obj = i915_gem_alloc_object(dev, size); - if (!obj) { + if (IS_ERR(obj)) { DRM_ERROR("failed to allocate framebuffer\n"); - ret = -ENOMEM; + ret = PTR_ERR(obj); goto out; } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 6179b59..1983198 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1474,9 +1474,11 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *engine, u32 size) engine->wa_ctx.obj = i915_gem_alloc_object(engine->dev, PAGE_ALIGN(size)); - if (!engine->wa_ctx.obj) { + if (IS_ERR(engine->wa_ctx.obj)) { DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n"); - return -ENOMEM; + ret = PTR_ERR(engine->wa_ctx.obj); + engine->wa_ctx.obj = NULL; + return ret; } ret = i915_gem_obj_ggtt_pin(engine->wa_ctx.obj, PAGE_SIZE, 0); @@ -2666,9 +2668,9 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx, context_size += PAGE_SIZE * LRC_PPHWSP_PN; ctx_obj = i915_gem_alloc_object(dev, context_size); - if (!ctx_obj) { + if (IS_ERR(ctx_obj)) { DRM_DEBUG_DRIVER("Alloc LRC backing obj failed.\n"); - return -ENOMEM; + return PTR_ERR(ctx_obj); } ringbuf = intel_engine_create_ringbuffer(engine, 4 * PAGE_SIZE); diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c index bcc3b6a..0d4bcbe 100644 --- a/drivers/gpu/drm/i915/intel_overlay.c +++ b/drivers/gpu/drm/i915/intel_overlay.c @@ -1397,7 +1397,7 @@ void intel_setup_overlay(struct drm_device *dev) reg_bo = i915_gem_object_create_stolen(dev, PAGE_SIZE); if (reg_bo == NULL) reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE); - if (reg_bo == NULL) + if (IS_ERR(reg_bo)) goto out_free; overlay->reg_bo = reg_bo; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 245386e..29094ef 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -672,9 +672,10 @@ intel_init_pipe_control(struct intel_engine_cs *engine) WARN_ON(engine->scratch.obj); engine->scratch.obj = i915_gem_alloc_object(engine->dev, 4096); - if (engine->scratch.obj == NULL) { + if (IS_ERR(engine->scratch.obj)) { DRM_ERROR("Failed to allocate seqno page\n"); - ret = -ENOMEM; + ret = PTR_ERR(engine->scratch.obj); + engine->scratch.obj = NULL; goto err; } @@ -2023,9 +2024,9 @@ static int init_status_page(struct intel_engine_cs *engine) int ret; obj = i915_gem_alloc_object(engine->dev, 4096); - if (obj == NULL) { + if (IS_ERR(obj)) { DRM_ERROR("Failed to allocate status page\n"); - return -ENOMEM; + return PTR_ERR(obj); } ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC); @@ -2164,8 +2165,8 @@ static int intel_alloc_ringbuffer_obj(struct drm_device *dev, obj = i915_gem_object_create_stolen(dev, ringbuf->size); if (obj == NULL) obj = i915_gem_alloc_object(dev, ringbuf->size); - if (obj == NULL) - return -ENOMEM; + if (IS_ERR(obj)) + return PTR_ERR(obj); /* mark ring buffers as read-only from GPU side by default */ obj->gt_ro = 1; @@ -2777,7 +2778,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev) if (INTEL_INFO(dev)->gen >= 8) { if (i915_semaphore_is_enabled(dev)) { obj = i915_gem_alloc_object(dev, 4096); - if (obj == NULL) { + if (IS_ERR(obj)) { DRM_ERROR("Failed to allocate semaphore bo. Disabling semaphores\n"); i915.semaphores = 0; } else { @@ -2886,9 +2887,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev) /* Workaround batchbuffer to combat CS tlb bug. */ if (HAS_BROKEN_CS_TLB(dev)) { obj = i915_gem_alloc_object(dev, I830_WA_SIZE); - if (obj == NULL) { + if (IS_ERR(obj)) { DRM_ERROR("Failed to allocate batch bo\n"); - return -ENOMEM; + return PTR_ERR(obj); } ret = i915_gem_obj_ggtt_pin(obj, 0, 0);