[Intel-gfx] [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Oct 8 03:22:45 PDT 2015
Hi,
On 08/10/15 07:24, ankitprasad.r.sharma at intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
>
> 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 at intel.com>
> ---
> 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
> @@ -393,9 +393,9 @@ i915_gem_create(struct drm_file *file,
> if (flags & I915_CREATE_PLACEMENT_STOLEN) {
> mutex_lock(&dev->struct_mutex);
> obj = i915_gem_object_create_stolen(dev, size);
> - if (!obj) {
> + if (IS_ERR(obj)) {
> mutex_unlock(&dev->struct_mutex);
> - return -ENOMEM;
> + return PTR_ERR(obj);
> }
>
> /* Always clear fresh buffers before handing to userspace */
> @@ -411,8 +411,8 @@ i915_gem_create(struct drm_file *file,
> 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 */
> @@ -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;
>
> 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;
You have to change kerneldoc for this function now.
And even more importantly update error handling in the callers!
The rest looks fine to me.
One possible class of improvements could be, now that error codes are
not swallowed, to print them out in all the existing DRM_DEBUG_ logging
after the relevant call sites. Up to you.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list