On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Double "From: ", one should be enough.
>
> 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()
>
> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
> 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, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b37ffea..6a6998c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -387,8 +387,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 */
> @@ -4527,14 +4527,16 @@ 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) {
> + ret = drm_gem_object_init(dev, &obj->base, size);
> + if (ret) {
> i915_gem_object_free(obj);
> - return NULL;
> + return ERR_PTR(ret);
While at it, I'd make this have a goto teardown path.
> }
>
> mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
> @@ -5390,7 +5392,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 fe580cb..a5adc66 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;
> + }
The teardown path in this function leaves so->obj != NULL in later
failure path. Also i915_gem_alloc_object has only
drm_gem_object_unreference as a counterpart so it'll leak too. Should
be fixed in a follow-up patch.
>
> 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 da86bdb..004f8f7 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -617,7 +617,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 551541b303..2247bdb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10283,8 +10283,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 0d6dc5e..c0543bb 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1483,9 +1483,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);
> @@ -2638,9 +2640,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 6694e92..77ee12f 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 41b604e..b20cf91 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;
> }
>
> @@ -2021,9 +2022,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);
> @@ -2156,8 +2157,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;
> @@ -2780,7 +2781,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 {
> @@ -2889,9 +2890,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);
Some comments above.
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
