Re: [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 08, 2015 at 11:54:26AM +0530, ankitprasad.r.sharma@xxxxxxxxx wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> 
> Propagating correct error codes to userspace by using ERR_PTR and
> PTR_ERR macros for stolen memory based object allocation. We generally
> return -ENOMEM to the user whenever there is a failure in object
> allocation. This patch helps user to identify the correct reason for the
> failure and not just -ENOMEM each time.
> 
> v2: Moved the patch up in the series, added error propagation for
> i915_gem_alloc_object too (Chris)
> 
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_gem.c              | 15 +++++-----
>  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 |  4 +--
>  drivers/gpu/drm/i915/i915_gem_stolen.c       | 43 ++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_guc_submission.c   |  4 +--
>  drivers/gpu/drm/i915/intel_display.c         |  2 +-
>  drivers/gpu/drm/i915/intel_fbdev.c           |  6 ++--
>  drivers/gpu/drm/i915/intel_lrc.c             |  8 +++---
>  drivers/gpu/drm/i915/intel_overlay.c         |  4 +--
>  drivers/gpu/drm/i915/intel_pm.c              |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c      | 20 ++++++-------
>  12 files changed, 61 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d3f4321..54f7df1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4273,14 +4273,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 = 0;

Pointless initialisation.

>  
>  	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) {
> +	if ((ret = drm_gem_object_init(dev, &obj->base, size)) != 0) {
>  		i915_gem_object_free(obj);
> -		return NULL;
> +		return ERR_PTR(ret);
>  	}
>  
>  	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
> 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 74aa0c9..603600c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -157,8 +157,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 5026a62..50d010e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -58,8 +58,8 @@ 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))
> +		return PTR_ERR(so->obj);
>  
>  	ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 5ec2190..d3e6813 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -406,6 +406,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct sg_table *st;
>  	struct scatterlist *sg;
> +	int ret;
>  
>  	DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
>  	BUG_ON(offset > dev_priv->gtt.stolen_size - size);
> @@ -417,11 +418,12 @@ i915_pages_create_for_stolen(struct drm_device *dev,
>  
>  	st = kmalloc(sizeof(*st), GFP_KERNEL);
>  	if (st == NULL)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
> -	if (sg_alloc_table(st, 1, GFP_KERNEL)) {
> +	ret = sg_alloc_table(st, 1, GFP_KERNEL);
> +	if (ret) {
>  		kfree(st);
> -		return NULL;
> +		return ERR_PTR(ret);
>  	}
>  
>  	sg = st->sgl;
> @@ -470,18 +472,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>  			       struct drm_mm_node *stolen)
>  {
>  	struct drm_i915_gem_object *obj;
> +	int ret = 0;
>  
>  	obj = i915_gem_object_alloc(dev);
>  	if (obj == NULL)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	drm_gem_private_object_init(dev, &obj->base, stolen->size);
>  	i915_gem_object_init(obj, &i915_gem_object_stolen_ops);
>  
>  	obj->pages = i915_pages_create_for_stolen(dev,
>  						  stolen->start, stolen->size);
> -	if (obj->pages == NULL)
> +	if (IS_ERR(obj->pages)) {
> +		ret = PTR_ERR(obj->pages);
>  		goto cleanup;
> +	}
>  
>  	i915_gem_object_pin_pages(obj);
>  	obj->stolen = stolen;
> @@ -493,7 +498,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>  
>  cleanup:
>  	i915_gem_object_free(obj);
> -	return NULL;
> +	return ERR_PTR(ret);
>  }
>  
>  struct drm_i915_gem_object *
> @@ -505,29 +510,29 @@ i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
>  	int ret;
>  
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> -		return NULL;
> +		return ERR_PTR(-ENODEV);
>  
>  	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
>  	if (size == 0)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
>  	if (!stolen)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
>  	if (ret) {
>  		kfree(stolen);
> -		return NULL;
> +		return ERR_PTR(ret);
>  	}
>  
>  	obj = _i915_gem_object_create_stolen(dev, stolen);
> -	if (obj)
> +	if (!IS_ERR(obj))
>  		return obj;
>  
>  	i915_gem_stolen_remove_node(dev_priv, stolen);
>  	kfree(stolen);
> -	return NULL;
> +	return obj;
>  }
>  
>  struct drm_i915_gem_object *
> @@ -544,7 +549,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	int ret;
>  
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> -		return NULL;
> +		return ERR_PTR(-ENODEV);
>  
>  	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
>  			stolen_offset, gtt_offset, size);
> @@ -552,11 +557,11 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	/* KISS and expect everything to be page-aligned */
>  	if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
>  	    WARN_ON(stolen_offset & 4095))
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
>  	if (!stolen)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	stolen->start = stolen_offset;
>  	stolen->size = size;
> @@ -566,15 +571,15 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	if (ret) {
>  		DRM_DEBUG_KMS("failed to allocate stolen space\n");
>  		kfree(stolen);
> -		return NULL;
> +		return ERR_PTR(ret);
>  	}
>  
>  	obj = _i915_gem_object_create_stolen(dev, stolen);
> -	if (obj == NULL) {
> +	if (IS_ERR(obj)) {
>  		DRM_DEBUG_KMS("failed to allocate stolen object\n");
>  		i915_gem_stolen_remove_node(dev_priv, stolen);
>  		kfree(stolen);
> -		return NULL;
> +		return obj;
>  	}
>  
>  	/* Some objects just need physical mem from stolen space */
> @@ -612,5 +617,5 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  
>  err:
>  	drm_gem_object_unreference(&obj->base);
> -	return NULL;
> +	return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 792d0b9..3901698 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -646,8 +646,8 @@ 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)
> -		return NULL;
> +	if (IS_ERR(obj))
> +		return obj;

Did you check the caller?
>  
>  	if (i915_gem_object_get_pages(obj)) {
>  		drm_gem_object_unreference(&obj->base);

New error code return here.

The call to get_pages() is redundant anyway.

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1847257..d08989a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2514,7 +2514,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
>  							     base_aligned,
>  							     base_aligned,
>  							     size_aligned);
> -	if (!obj)
> +	if (IS_ERR(obj))
>  		return false;
>  
>  	obj->tiling_mode = plane_config->tiling;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..67de958 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -140,11 +140,11 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = PAGE_ALIGN(size);
>  	obj = i915_gem_object_create_stolen(dev, size);
> -	if (obj == NULL)
> +	if (IS_ERR(obj))
>  		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 256167b..d9bd9a1 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1377,9 +1377,9 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *ring, u32 size)
>  	int ret;
>  
>  	ring->wa_ctx.obj = i915_gem_alloc_object(ring->dev, PAGE_ALIGN(size));
> -	if (!ring->wa_ctx.obj) {
> +	if (IS_ERR(ring->wa_ctx.obj)) {
>  		DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
> -		return -ENOMEM;
> +		return PTR_ERR(ring->wa_ctx.obj);

Safer not to store error objects inside structs - we have a habit of
using drm_gem_object_unreference() which will then explode.

>  	}
>  
>  	ret = i915_gem_obj_ggtt_pin(ring->wa_ctx.obj, PAGE_SIZE, 0);
> @@ -2459,9 +2459,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(ring, 4 * PAGE_SIZE);
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 4445426..bfb95a2 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1392,9 +1392,9 @@ void intel_setup_overlay(struct drm_device *dev)
>  	reg_bo = NULL;
>  	if (!OVERLAY_NEEDS_PHYSICAL(dev))
>  		reg_bo = i915_gem_object_create_stolen(dev, PAGE_SIZE);
> -	if (reg_bo == NULL)
> +	if (IS_ERR_OR_NULL(reg_bo))
>  		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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ab5ac5e..0d35d7f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5387,7 +5387,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  	 * memory, or any other relevant ranges.
>  	 */
>  	pctx = i915_gem_object_create_stolen(dev, pctx_size);
> -	if (!pctx) {
> +	if (IS_ERR(pctx)) {
>  		DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 16a4ead..a928602 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -676,9 +676,9 @@ intel_init_pipe_control(struct intel_engine_cs *ring)
>  	WARN_ON(ring->scratch.obj);
>  
>  	ring->scratch.obj = i915_gem_alloc_object(ring->dev, 4096);
> -	if (ring->scratch.obj == NULL) {
> +	if (IS_ERR(ring->scratch.obj)) {
>  		DRM_ERROR("Failed to allocate seqno page\n");
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(ring->scratch.obj);

Beware the caller subsequently calling drm_gem_object_unreference on the
error pointer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux