Re: [PATCH 5/5] drm/i915: Propagate error from drm_gem_object_init()

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2016-04-12 at 16:57 +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> From: Chris Wilson <chris@chris-wilson.co.uk>

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@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  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@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx