Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448644 > unrolled thread

[PATCH 0/4] GPU-DRM-Etnaviv: Fine-tuning for a few functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/4] GPU-DRM-Etnaviv: Fine-tuning for a few functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 17:50 +0200
    [PATCH 2/4] GPU-DRM-Etnaviv: Delete unnecessary if statement in  __etnaviv_gem_new() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 17:50 +0200
      Re: [PATCH 2/4] GPU-DRM-Etnaviv: Delete unnecessary if statement  in __etnaviv_gem_new() walter harms <wharms@bfs.de> - 2016-07-22 18:50 +0200
        [PATCH] drm/etnaviv: Improve readability in __etnaviv_gem_new Sean Paul <seanpaul@chromium.org> - 2016-07-22 19:50 +0200
    [PATCH 1/4] GPU-DRM-Etnaviv: Delete unnecessary checks before two  function calls SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 17:50 +0200
      Re: [PATCH 1/4] GPU-DRM-Etnaviv: Delete unnecessary checks before two  function calls Sean Paul <seanpaul@chromium.org> - 2016-07-22 20:00 +0200
    [PATCH 4/4] GPU-DRM-Etnaviv: Optimize error handling in  etnaviv_gem_new_userptr() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 18:00 +0200
      Re: [PATCH 4/4] GPU-DRM-Etnaviv: Optimize error handling in etnaviv_gem_new_userptr() Sean Paul <seanpaul@chromium.org> - 2016-07-22 20:00 +0200
    [PATCH 3/4] GPU-DRM-Etnaviv: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 18:00 +0200
      Re: [PATCH 3/4] GPU-DRM-Etnaviv: Rename jump labels Sean Paul <seanpaul@chromium.org> - 2016-07-22 19:50 +0200

#1448644 — [PATCH 0/4] GPU-DRM-Etnaviv: Fine-tuning for a few functions

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 22 Jul 2016 17:34:32 +0200

Further update suggestions were taken into account
after a patch was applied from static source code analysis.

Markus Elfring (4):
  Delete unnecessary checks before two function calls
  Delete unnecessary if statement in __etnaviv_gem_new()
  Rename jump labels
  Optimize error handling in etnaviv_gem_new_userptr()

 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 36 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

-- 
2.9.2

[toc] | [next] | [standalone]

#1448646 — [PATCH 2/4] GPU-DRM-Etnaviv: Delete unnecessary if statement in __etnaviv_gem_new()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 22 Jul 2016 16:45:22 +0200

Move a return statement into a block for successful function execution.
Omit a duplicate check for the local variable "ret" then at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 8eee742..851a8ba 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -661,13 +661,9 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 		 */
 		mapping = obj->filp->f_mapping;
 		mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
+		return obj;
 	}
 
-	if (ret)
-		goto fail;
-
-	return obj;
-
 fail:
 	drm_gem_object_unreference_unlocked(obj);
 	return ERR_PTR(ret);
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1448670 — Re: [PATCH 2/4] GPU-DRM-Etnaviv: Delete unnecessary if statement in __etnaviv_gem_new()

Am 22.07.2016 17:48, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 22 Jul 2016 16:45:22 +0200
> 
> Move a return statement into a block for successful function execution.
> Omit a duplicate check for the local variable "ret" then at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 8eee742..851a8ba 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -661,13 +661,9 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>  		 */
>  		mapping = obj->filp->f_mapping;
>  		mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
> +		return obj;
>  	}
>  
> -	if (ret)
> -		goto fail;
> -
> -	return obj;
> -
>  fail:
>  	drm_gem_object_unreference_unlocked(obj);
>  	return ERR_PTR(ret);


From the program flow an readability it would be more nice the branch on error

 ret = drm_gem_object_init(dev, obj, size);
 if (ret)
	goto fail;


just m 2 cents

re,
 wh

[toc] | [prev] | [next] | [standalone]

#1448687 — [PATCH] drm/etnaviv: Improve readability in __etnaviv_gem_new

Move things around a little in __etnaviv_gem_new() to make it
more readable.

Reported-by: Markus Elfring <elfring@users.sourceforge.net>
Reported-by: walter harms <wharms@bfs.de>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index df9bcba..7d13628 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -640,6 +640,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 		u32 size, u32 flags)
 {
 	struct drm_gem_object *obj = NULL;
+	struct address_space *mapping;
 	int ret;
 
 	size = PAGE_ALIGN(size);
@@ -650,23 +651,19 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 		goto fail;
 
 	ret = drm_gem_object_init(dev, obj, size);
-	if (ret == 0) {
-		struct address_space *mapping;
-
-		/*
-		 * Our buffers are kept pinned, so allocating them
-		 * from the MOVABLE zone is a really bad idea, and
-		 * conflicts with CMA.  See coments above new_inode()
-		 * why this is required _and_ expected if you're
-		 * going to pin these pages.
-		 */
-		mapping = file_inode(obj->filp)->i_mapping;
-		mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
-	}
-
 	if (ret)
 		goto fail;
 
+	/*
+	 * Our buffers are kept pinned, so allocating them
+	 * from the MOVABLE zone is a really bad idea, and
+	 * conflicts with CMA.  See coments above new_inode()
+	 * why this is required _and_ expected if you're
+	 * going to pin these pages.
+	 */
+	mapping = file_inode(obj->filp)->i_mapping;
+	mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
+
 	return obj;
 
 fail:
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1448647 — [PATCH 1/4] GPU-DRM-Etnaviv: Delete unnecessary checks before two function calls

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 22 Jul 2016 15:56:15 +0200

The functions drm_gem_object_unreference_unlocked() and vunmap() perform
also input parameter validation.
Thus the tests around their calls are not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 8c6f750..8eee742 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -535,8 +535,7 @@ void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv,
 
 static void etnaviv_gem_shmem_release(struct etnaviv_gem_object *etnaviv_obj)
 {
-	if (etnaviv_obj->vaddr)
-		vunmap(etnaviv_obj->vaddr);
+	vunmap(etnaviv_obj->vaddr);
 	put_pages(etnaviv_obj);
 }
 
@@ -670,9 +669,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 	return obj;
 
 fail:
-	if (obj)
-		drm_gem_object_unreference_unlocked(obj);
-
+	drm_gem_object_unreference_unlocked(obj);
 	return ERR_PTR(ret);
 }
 
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1448697 — Re: [PATCH 1/4] GPU-DRM-Etnaviv: Delete unnecessary checks before two function calls

On Fri, Jul 22, 2016 at 11:47 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 22 Jul 2016 15:56:15 +0200
>
> The functions drm_gem_object_unreference_unlocked() and vunmap() perform
> also input parameter validation.
> Thus the tests around their calls are not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to drm-misc. In the future, could you please format your
subjects starting with "drm/<driver>"? I've fixed this one and the
next, but it'd be nice not to have to do that going forward.

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 8c6f750..8eee742 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -535,8 +535,7 @@ void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv,
>
>  static void etnaviv_gem_shmem_release(struct etnaviv_gem_object *etnaviv_obj)
>  {
> -       if (etnaviv_obj->vaddr)
> -               vunmap(etnaviv_obj->vaddr);
> +       vunmap(etnaviv_obj->vaddr);
>         put_pages(etnaviv_obj);
>  }
>
> @@ -670,9 +669,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>         return obj;
>
>  fail:
> -       if (obj)
> -               drm_gem_object_unreference_unlocked(obj);
> -
> +       drm_gem_object_unreference_unlocked(obj);
>         return ERR_PTR(ret);
>  }
>
> --
> 2.9.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1448649 — [PATCH 4/4] GPU-DRM-Etnaviv: Optimize error handling in etnaviv_gem_new_userptr()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 22 Jul 2016 17:17:48 +0200

Refactor this function implementation so that the
drm_gem_object_unreference_unlocked() function will only be called once
in case of a failure according to the Linux coding style recommendation
for centralized exiting of functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 0a5c00c..007577c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -909,15 +909,12 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
 	get_task_struct(current);
 
 	ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
-		return ret;
-	}
+	if (ret)
+		goto unreference;
 
 	ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
-
+unreference:
 	/* drop reference from allocate - handle holds it now */
 	drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
-
 	return ret;
 }
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1448696 — Re: [PATCH 4/4] GPU-DRM-Etnaviv: Optimize error handling in etnaviv_gem_new_userptr()

On Fri, Jul 22, 2016 at 11:51 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 22 Jul 2016 17:17:48 +0200
>
> Refactor this function implementation so that the
> drm_gem_object_unreference_unlocked() function will only be called once
> in case of a failure according to the Linux coding style recommendation
> for centralized exiting of functions.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to drm-misc. In the future, could you please format your
subjects starting with "drm/<driver>"? I've fixed this one and the
first, but it'd be nice not to have to do that going forward.

Sean

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 0a5c00c..007577c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -909,15 +909,12 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
>         get_task_struct(current);
>
>         ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
> -       if (ret) {
> -               drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
> -               return ret;
> -       }
> +       if (ret)
> +               goto unreference;
>
>         ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
> -
> +unreference:
>         /* drop reference from allocate - handle holds it now */
>         drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
> -
>         return ret;
>  }
> --
> 2.9.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1448652 — [PATCH 3/4] GPU-DRM-Etnaviv: Rename jump labels

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 22 Jul 2016 16:51:00 +0200

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 851a8ba..0a5c00c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -308,17 +308,17 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
 				mapping = NULL;
 			mutex_unlock(&gpu->mmu->lock);
 			if (mapping)
-				goto out;
+				goto unlock;
 		} else {
 			mapping->use += 1;
-			goto out;
+			goto unlock;
 		}
 	}
 
 	pages = etnaviv_gem_get_pages(etnaviv_obj);
 	if (IS_ERR(pages)) {
 		ret = PTR_ERR(pages);
-		goto out;
+		goto unlock;
 	}
 
 	/*
@@ -330,7 +330,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
 		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
 		if (!mapping) {
 			ret = -ENOMEM;
-			goto out;
+			goto unlock;
 		}
 
 		INIT_LIST_HEAD(&mapping->scan_node);
@@ -349,7 +349,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
 	else
 		list_add_tail(&mapping->obj_node, &etnaviv_obj->vram_list);
 
-out:
+unlock:
 	mutex_unlock(&etnaviv_obj->lock);
 
 	if (ret)
@@ -646,7 +646,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 	ret = etnaviv_gem_new_impl(dev, size, flags, NULL,
 				   &etnaviv_gem_shmem_ops, &obj);
 	if (ret)
-		goto fail;
+		goto unreference;
 
 	ret = drm_gem_object_init(dev, obj, size);
 	if (ret == 0) {
@@ -664,7 +664,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 		return obj;
 	}
 
-fail:
+unreference:
 	drm_gem_object_unreference_unlocked(obj);
 	return ERR_PTR(ret);
 }
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1448691 — Re: [PATCH 3/4] GPU-DRM-Etnaviv: Rename jump labels

On Fri, Jul 22, 2016 at 11:50 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 22 Jul 2016 16:51:00 +0200
>
> Adjust jump targets according to the Linux coding style convention.
>

I'm not convinced the old labels are really that bad, tbh.

Sean

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 851a8ba..0a5c00c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -308,17 +308,17 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
>                                 mapping = NULL;
>                         mutex_unlock(&gpu->mmu->lock);
>                         if (mapping)
> -                               goto out;
> +                               goto unlock;
>                 } else {
>                         mapping->use += 1;
> -                       goto out;
> +                       goto unlock;
>                 }
>         }
>
>         pages = etnaviv_gem_get_pages(etnaviv_obj);
>         if (IS_ERR(pages)) {
>                 ret = PTR_ERR(pages);
> -               goto out;
> +               goto unlock;
>         }
>
>         /*
> @@ -330,7 +330,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
>                 mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>                 if (!mapping) {
>                         ret = -ENOMEM;
> -                       goto out;
> +                       goto unlock;
>                 }
>
>                 INIT_LIST_HEAD(&mapping->scan_node);
> @@ -349,7 +349,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get(
>         else
>                 list_add_tail(&mapping->obj_node, &etnaviv_obj->vram_list);
>
> -out:
> +unlock:
>         mutex_unlock(&etnaviv_obj->lock);
>
>         if (ret)
> @@ -646,7 +646,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>         ret = etnaviv_gem_new_impl(dev, size, flags, NULL,
>                                    &etnaviv_gem_shmem_ops, &obj);
>         if (ret)
> -               goto fail;
> +               goto unreference;
>
>         ret = drm_gem_object_init(dev, obj, size);
>         if (ret == 0) {
> @@ -664,7 +664,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>                 return obj;
>         }
>
> -fail:
> +unreference:
>         drm_gem_object_unreference_unlocked(obj);
>         return ERR_PTR(ret);
>  }
> --
> 2.9.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web