[PATCH v1 0/8] drm/etnaviv: Various cleanup

[PATCH v1 0/8] drm/etnaviv: Various cleanup

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

No functional change.

Sui Jingfeng (8):
  drm/etnaviv: Using the size_t variable to store the number of pages
  drm/etnaviv: Using the unsigned int type to count the number of pages
  drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()
  drm/etnaviv: Remove surplus else after return
  drm/etnaviv: Keep the curly brace aligned
  drm/etnaviv: No indentation by double tabs
  drm/etnaviv: Add dedicated functions to create and destroy platform
    device
  drm/etnaviv: Add a helper to get a pointer to the first available node

 drivers/gpu/drm/etnaviv/etnaviv_drv.c       | 100 +++++++++++++-------
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       |  14 +--
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |   7 +-
 3 files changed, 77 insertions(+), 44 deletions(-)

-- 
2.25.1

[PATCH v1 1/8] drm/etnaviv: Using the size_t variable to store the number of pages

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Because the etnaviv_gem_new_private() function receives the size_t argument
for the number of pages. And the number of pages should be unsigned.

Note that Most 32-bit architectures use "unsigned int" size_t,
and all 64-bit architectures use "unsigned long" size_t.
So, let's keep the argument and parameter consistent.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 3524b5811682..b003481adc2b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -114,7 +114,8 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
 {
 	struct etnaviv_gem_object *etnaviv_obj;
 	size_t size = PAGE_ALIGN(attach->dmabuf->size);
-	int ret, npages;
+	size_t npages = size / PAGE_SIZE;
+	int ret;
 
 	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
 				      &etnaviv_gem_prime_ops, &etnaviv_obj);
@@ -123,8 +124,6 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
 
 	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_prime_lock_class);
 
-	npages = size / PAGE_SIZE;
-
 	etnaviv_obj->sgt = sgt;
 	etnaviv_obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
 	if (!etnaviv_obj->pages) {
-- 
2.25.1

[PATCH v1 2/8] drm/etnaviv: Using the unsigned int type to count the number of pages

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Instead of the 'int' type in the etnaviv_gem_prime_get_sg_table(),
because the drm_prime_pages_to_sg() function takes an unsigned int type
as its third argument.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index b003481adc2b..6a2129a92cb6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -17,7 +17,7 @@ static struct lock_class_key etnaviv_prime_lock_class;
 struct sg_table *etnaviv_gem_prime_get_sg_table(struct drm_gem_object *obj)
 {
 	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
-	int npages = obj->size >> PAGE_SHIFT;
+	unsigned int npages = obj->size >> PAGE_SHIFT;
 
 	if (WARN_ON(!etnaviv_obj->pages))  /* should have already pinned! */
 		return ERR_PTR(-EINVAL);
-- 
2.25.1

[PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Because it is not used by the etnaviv_gem_new_impl() function,
no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index b5f73502e3dd..be2f459c66b5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
 	.vm_ops = &vm_ops,
 };
 
-static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
+static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
 	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
 {
 	struct etnaviv_gem_object *etnaviv_obj;
@@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 
 	size = PAGE_ALIGN(size);
 
-	ret = etnaviv_gem_new_impl(dev, size, flags,
-				   &etnaviv_gem_shmem_ops, &obj);
+	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
 	if (ret)
 		goto fail;
 
@@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
 	struct drm_gem_object *obj;
 	int ret;
 
-	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
+	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v1 4/8] drm/etnaviv: Remove surplus else after return

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Because the 'else' is not generally useful after the 'return'.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index be2f459c66b5..271470723d5e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -358,10 +358,11 @@ static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
 {
 	if (op & ETNA_PREP_READ)
 		return DMA_FROM_DEVICE;
-	else if (op & ETNA_PREP_WRITE)
+
+	if (op & ETNA_PREP_WRITE)
 		return DMA_TO_DEVICE;
-	else
-		return DMA_BIDIRECTIONAL;
+
+	return DMA_BIDIRECTIONAL;
 }
 
 int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op,
-- 
2.25.1

[PATCH v1 5/8] drm/etnaviv: Keep the curly brace aligned

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

No functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 00223a874909..cef97bb9c99f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -77,7 +77,7 @@ static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
 			drm_sched_entity_init(&ctx->sched_entity[i],
 					      DRM_SCHED_PRIORITY_NORMAL, &sched,
 					      1, NULL);
-			}
+		}
 	}
 
 	file->driver_priv = ctx;
-- 
2.25.1

[PATCH v1 6/8] drm/etnaviv: No indentation by double tabs

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Single tab should be enough.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index cef97bb9c99f..14c2e9690ce1 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -231,11 +231,11 @@ static int show_each_gpu(struct seq_file *m, void *arg)
 }
 
 static struct drm_info_list etnaviv_debugfs_list[] = {
-		{"gpu", show_each_gpu, 0, etnaviv_gpu_debugfs},
-		{"gem", show_unlocked, 0, etnaviv_gem_show},
-		{ "mm", show_unlocked, 0, etnaviv_mm_show },
-		{"mmu", show_each_gpu, 0, etnaviv_mmu_show},
-		{"ring", show_each_gpu, 0, etnaviv_ring_show},
+	{"gpu", show_each_gpu, 0, etnaviv_gpu_debugfs},
+	{"gem", show_unlocked, 0, etnaviv_gem_show},
+	{ "mm", show_unlocked, 0, etnaviv_mm_show },
+	{"mmu", show_each_gpu, 0, etnaviv_mmu_show},
+	{"ring", show_each_gpu, 0, etnaviv_ring_show},
 };
 
 static void etnaviv_debugfs_init(struct drm_minor *minor)
-- 
2.25.1

[PATCH v1 7/8] drm/etnaviv: Add dedicated functions to create and destroy platform device

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Also rename the virtual master device as etnaviv_platform_device,
for better reflection that it is a platform device, not a DRM device.
Another benefit is that we no longer need to call of_node_put() for three
different cases, Instead, we only need to call it once.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 14c2e9690ce1..7d0eeab3e8b7 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -655,12 +655,44 @@ static struct platform_driver etnaviv_platform_driver = {
 	},
 };
 
-static struct platform_device *etnaviv_drm;
+static struct platform_device *etnaviv_platform_device;
 
-static int __init etnaviv_init(void)
+static int etnaviv_create_platform_device(const char *name,
+					  struct platform_device **ppdev)
 {
 	struct platform_device *pdev;
 	int ret;
+
+	pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
+	if (!pdev)
+		return -ENOMEM;
+
+	ret = platform_device_add(pdev);
+	if (ret) {
+		platform_device_put(pdev);
+		return ret;
+	}
+
+	*ppdev = pdev;
+
+	return 0;
+}
+
+static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
+{
+	struct platform_device *pdev = *ppdev;
+
+	if (!pdev)
+		return;
+
+	platform_device_unregister(pdev);
+
+	*ppdev = NULL;
+}
+
+static int __init etnaviv_init(void)
+{
+	int ret;
 	struct device_node *np;
 
 	etnaviv_validate_init();
@@ -680,23 +712,13 @@ static int __init etnaviv_init(void)
 	for_each_compatible_node(np, NULL, "vivante,gc") {
 		if (!of_device_is_available(np))
 			continue;
+		of_node_put(np);
 
-		pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
-		if (!pdev) {
-			ret = -ENOMEM;
-			of_node_put(np);
-			goto unregister_platform_driver;
-		}
-
-		ret = platform_device_add(pdev);
-		if (ret) {
-			platform_device_put(pdev);
-			of_node_put(np);
+		ret = etnaviv_create_platform_device("etnaviv",
+						     &etnaviv_platform_device);
+		if (ret)
 			goto unregister_platform_driver;
-		}
 
-		etnaviv_drm = pdev;
-		of_node_put(np);
 		break;
 	}
 
@@ -712,7 +734,7 @@ module_init(etnaviv_init);
 
 static void __exit etnaviv_exit(void)
 {
-	platform_device_unregister(etnaviv_drm);
+	etnaviv_destroy_platform_device(&etnaviv_platform_device);
 	platform_driver_unregister(&etnaviv_platform_driver);
 	platform_driver_unregister(&etnaviv_gpu_driver);
 }
-- 
2.25.1

[PATCH v1 8/8] drm/etnaviv: Add a helper to get a pointer to the first available node

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

This make the code in etnaviv_pdev_probe() less twisted, drop the reference
to device node after finished. Also kill a double blank line.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 7d0eeab3e8b7..3446f8eabf59 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -27,6 +27,19 @@
  * DRM operations:
  */
 
+/* If the DT contains at least one available GPU, return a pointer to it */
+
+static struct device_node *etnaviv_of_first_node(void)
+{
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "vivante,gc") {
+		if (of_device_is_available(np))
+			return np;
+	}
+
+	return NULL;
+}
 
 static void load_gpu(struct drm_device *dev)
 {
@@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
 static int etnaviv_pdev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *first_node = NULL;
+	struct device_node *first_node;
 	struct component_match *match = NULL;
 
 	if (!dev->platform_data) {
@@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
 			if (!of_device_is_available(core_node))
 				continue;
 
-			if (!first_node)
-				first_node = core_node;
-
 			drm_of_component_match_add(&pdev->dev, &match,
 						   component_compare_of, core_node);
+
+			of_node_put(core_node);
 		}
 	} else {
 		char **names = dev->platform_data;
@@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
 	 * device as the GPU we found. This assumes that all Vivante
 	 * GPUs in the system share the same DMA constraints.
 	 */
-	if (first_node)
+	first_node = etnaviv_of_first_node();
+	if (first_node) {
 		of_dma_configure(&pdev->dev, first_node, true);
+		of_node_put(first_node);
+	}
 
 	return component_master_add_with_match(dev, &etnaviv_master_ops, match);
 }
@@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
 	 * If the DT contains at least one available GPU device, instantiate
 	 * the DRM platform device.
 	 */
-	for_each_compatible_node(np, NULL, "vivante,gc") {
-		if (!of_device_is_available(np))
-			continue;
+	np = etnaviv_of_first_node();
+	if (np) {
 		of_node_put(np);
 
 		ret = etnaviv_create_platform_device("etnaviv",
 						     &etnaviv_platform_device);
 		if (ret)
 			goto unregister_platform_driver;
-
-		break;
 	}
 
 	return 0;
-- 
2.25.1

Re: [PATCH v1 0/8] drm/etnaviv: Various cleanup

[suijingfeng]

Hi, Dear etnaviv folks


Would you like to review this cleanup patch set ?

I am asking because I'm wondering that

if I should re-spin my other patch from the code base

which *with* this series applied or from the code base

*without* this series applied.


I think this series looks fine, is it acceptable?


On 2023/6/23 18:08, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> No functional change.
>
> Sui Jingfeng (8):
>    drm/etnaviv: Using the size_t variable to store the number of pages
>    drm/etnaviv: Using the unsigned int type to count the number of pages
>    drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()
>    drm/etnaviv: Remove surplus else after return
>    drm/etnaviv: Keep the curly brace aligned
>    drm/etnaviv: No indentation by double tabs
>    drm/etnaviv: Add dedicated functions to create and destroy platform
>      device
>    drm/etnaviv: Add a helper to get a pointer to the first available node
>
>   drivers/gpu/drm/etnaviv/etnaviv_drv.c       | 100 +++++++++++++-------
>   drivers/gpu/drm/etnaviv/etnaviv_gem.c       |  14 +--
>   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |   7 +-
>   3 files changed, 77 insertions(+), 44 deletions(-)
>

Re: [PATCH v1 1/8] drm/etnaviv: Using the size_t variable to store the number of pages

[Lucas Stach]

Hi Jingfeng,

Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Because the etnaviv_gem_new_private() function receives the size_t argument
> for the number of pages. And the number of pages should be unsigned.
> 
> Note that Most 32-bit architectures use "unsigned int" size_t,
> and all 64-bit architectures use "unsigned long" size_t.
> So, let's keep the argument and parameter consistent.
> 
This explanation doesn't add up. npages is just that: a number of
pages. Why would it make sense to use size_t here?

If you want to be consistent I would have expected this change to
switch things to unsigned int, as you did in the second patch of this
series.

Regards,
Lucas

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index 3524b5811682..b003481adc2b 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -114,7 +114,8 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>  {
>  	struct etnaviv_gem_object *etnaviv_obj;
>  	size_t size = PAGE_ALIGN(attach->dmabuf->size);
> -	int ret, npages;
> +	size_t npages = size / PAGE_SIZE;
> +	int ret;
>  
>  	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
>  				      &etnaviv_gem_prime_ops, &etnaviv_obj);
> @@ -123,8 +124,6 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>  
>  	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_prime_lock_class);
>  
> -	npages = size / PAGE_SIZE;
> -
>  	etnaviv_obj->sgt = sgt;
>  	etnaviv_obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
>  	if (!etnaviv_obj->pages) {

Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[Lucas Stach]

Hi Jingfeng,

Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Because it is not used by the etnaviv_gem_new_impl() function,
> no functional change.
> 
I think it would make sense to move into the opposite direction: in
both callsites of etnaviv_gem_new_impl we immediately call
drm_gem_object_init with the size. A better cleanup would be to make
use of the size parameter and move this object init call into
etnaviv_gem_new_impl.

Regards,
Lucas

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index b5f73502e3dd..be2f459c66b5 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>  	.vm_ops = &vm_ops,
>  };
>  
> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>  	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>  {
>  	struct etnaviv_gem_object *etnaviv_obj;
> @@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  
>  	size = PAGE_ALIGN(size);
>  
> -	ret = etnaviv_gem_new_impl(dev, size, flags,
> -				   &etnaviv_gem_shmem_ops, &obj);
> +	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
>  	if (ret)
>  		goto fail;
>  
> @@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>  	struct drm_gem_object *obj;
>  	int ret;
>  
> -	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
> +	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH v1 4/8] drm/etnaviv: Remove surplus else after return

[Lucas Stach]

Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Because the 'else' is not generally useful after the 'return'.

While your cleanup is a correct rewrite of the function, the current
code in this function is bogus, as we need to check for the
bidirectional (READ | WRITE) case first. Currently we just pick the
DMA_FROM_DEVICE direction when both flags are set, which is clearly not
right.

Regards,
Lucas

> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index be2f459c66b5..271470723d5e 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -358,10 +358,11 @@ static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
>  {
>  	if (op & ETNA_PREP_READ)
>  		return DMA_FROM_DEVICE;
> -	else if (op & ETNA_PREP_WRITE)
> +
> +	if (op & ETNA_PREP_WRITE)
>  		return DMA_TO_DEVICE;
> -	else
> -		return DMA_BIDIRECTIONAL;
> +
> +	return DMA_BIDIRECTIONAL;
>  }
>  
>  int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op,

Re: [PATCH v1 8/8] drm/etnaviv: Add a helper to get a pointer to the first available node

[Lucas Stach]

Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> This make the code in etnaviv_pdev_probe() less twisted, drop the reference
> to device node after finished. Also kill a double blank line.
> 
I can't spot the double blank line you claim to remove.

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 7d0eeab3e8b7..3446f8eabf59 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -27,6 +27,19 @@
>   * DRM operations:
>   */
>  
> +/* If the DT contains at least one available GPU, return a pointer to it */
> +
I think the code in the function is simple enough that we don't need a
comment explaining what it does.

Regards,
Lucas

> +static struct device_node *etnaviv_of_first_node(void)
> +{
> +	struct device_node *np;
> +
> +	for_each_compatible_node(np, NULL, "vivante,gc") {
> +		if (of_device_is_available(np))
> +			return np;
> +	}
> +
> +	return NULL;
> +}
>  
>  static void load_gpu(struct drm_device *dev)
>  {
> @@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
>  static int etnaviv_pdev_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *first_node = NULL;
> +	struct device_node *first_node;
>  	struct component_match *match = NULL;
>  
>  	if (!dev->platform_data) {
> @@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>  			if (!of_device_is_available(core_node))
>  				continue;
>  
> -			if (!first_node)
> -				first_node = core_node;
> -
>  			drm_of_component_match_add(&pdev->dev, &match,
>  						   component_compare_of, core_node);
> +
> +			of_node_put(core_node);
>  		}
>  	} else {
>  		char **names = dev->platform_data;
> @@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>  	 * device as the GPU we found. This assumes that all Vivante
>  	 * GPUs in the system share the same DMA constraints.
>  	 */
> -	if (first_node)
> +	first_node = etnaviv_of_first_node();
> +	if (first_node) {
>  		of_dma_configure(&pdev->dev, first_node, true);
> +		of_node_put(first_node);
> +	}
>  
>  	return component_master_add_with_match(dev, &etnaviv_master_ops, match);
>  }
> @@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
>  	 * If the DT contains at least one available GPU device, instantiate
>  	 * the DRM platform device.
>  	 */
> -	for_each_compatible_node(np, NULL, "vivante,gc") {
> -		if (!of_device_is_available(np))
> -			continue;
> +	np = etnaviv_of_first_node();
> +	if (np) {
>  		of_node_put(np);
>  
>  		ret = etnaviv_create_platform_device("etnaviv",
>  						     &etnaviv_platform_device);
>  		if (ret)
>  			goto unregister_platform_driver;
> -
> -		break;
>  	}
>  
>  	return 0;

Re: [PATCH v1 0/8] drm/etnaviv: Various cleanup

[Lucas Stach]

Hi Jingfeng,

Am Montag, dem 17.07.2023 um 16:36 +0800 schrieb suijingfeng:
> Hi, Dear etnaviv folks
> 
> 
> Would you like to review this cleanup patch set ?
> 
> I am asking because I'm wondering that
> 
> if I should re-spin my other patch from the code base
> 
> which *with* this series applied or from the code base
> 
> *without* this series applied.
> 
> 
> I think this series looks fine, is it acceptable?
> 
I've gone through the series and left some comments. All patches that I
didn't comment on look fine to me. I'm generally in favor of taking
this series.

Regards,
Lucas

> 
> On 2023/6/23 18:08, Sui Jingfeng wrote:
> > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > 
> > No functional change.
> > 
> > Sui Jingfeng (8):
> >    drm/etnaviv: Using the size_t variable to store the number of pages
> >    drm/etnaviv: Using the unsigned int type to count the number of pages
> >    drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()
> >    drm/etnaviv: Remove surplus else after return
> >    drm/etnaviv: Keep the curly brace aligned
> >    drm/etnaviv: No indentation by double tabs
> >    drm/etnaviv: Add dedicated functions to create and destroy platform
> >      device
> >    drm/etnaviv: Add a helper to get a pointer to the first available node
> > 
> >   drivers/gpu/drm/etnaviv/etnaviv_drv.c       | 100 +++++++++++++-------
> >   drivers/gpu/drm/etnaviv/etnaviv_gem.c       |  14 +--
> >   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |   7 +-
> >   3 files changed, 77 insertions(+), 44 deletions(-)
> > 
> 

Re: [PATCH v1 1/8] drm/etnaviv: Using the size_t variable to store the number of pages

[Sui Jingfeng]

Hi

On 2023/7/17 17:43, Lucas Stach wrote:
> Hi Jingfeng,
>
> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Because the etnaviv_gem_new_private() function receives the size_t argument
>> for the number of pages. And the number of pages should be unsigned.
>>
>> Note that Most 32-bit architectures use "unsigned int" size_t,
>> and all 64-bit architectures use "unsigned long" size_t.
>> So, let's keep the argument and parameter consistent.
>>
> This explanation doesn't add up. npages is just that: a number of
> pages. Why would it make sense to use size_t here?

Because the 'size' variable in the etnaviv_gem_prime_import_sg_table() 
function is declared

as size_t type. On 64-bit machine, size_t is actually is 64-bit wide and 
it is *unsigned*.

While 'int' is actually 32-bit wide(at both 32-bit system and 64-bit 
system) and it is *signed*,

So, my point (argument) is that


1) This patch help to avoid the unnecessary 64 bit to 32 bit conversion.

2) The kvmalloc_array() function also accept  size_t type (see the 
prototype of  kvmalloc_array function include/linux/slab.h)


So my patch do helps to keep the code style consistent.


> If you want to be consistent I would have expected this change to
> switch things to unsigned int,

This may introduce a truncate operation (from a 64-bit to 32-bit), which 
is necessary.

And when you pass the 'npages' parameter to kvmalloc_array() function,

The compiler probably has to do the integer promotion (from a 32-bit to 
64-bit) for you.


> as you did in the second patch of this
> series.
>
> Regards,
> Lucas
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>> index 3524b5811682..b003481adc2b 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>> @@ -114,7 +114,8 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>   {
>>   	struct etnaviv_gem_object *etnaviv_obj;
>>   	size_t size = PAGE_ALIGN(attach->dmabuf->size);
>> -	int ret, npages;
>> +	size_t npages = size / PAGE_SIZE;
>> +	int ret;
>>   
>>   	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
>>   				      &etnaviv_gem_prime_ops, &etnaviv_obj);
>> @@ -123,8 +124,6 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>   
>>   	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_prime_lock_class);
>>   
>> -	npages = size / PAGE_SIZE;
>> -
>>   	etnaviv_obj->sgt = sgt;
>>   	etnaviv_obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
>>   	if (!etnaviv_obj->pages) {

Re: [PATCH v1 8/8] drm/etnaviv: Add a helper to get a pointer to the first available node

[suijingfeng]

Hi,

On 2023/7/17 18:07, Lucas Stach wrote:
> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> This make the code in etnaviv_pdev_probe() less twisted, drop the reference
>> to device node after finished. Also kill a double blank line.
>>
> I can't spot the double blank line you claim to remove.
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
>>   1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 7d0eeab3e8b7..3446f8eabf59 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -27,6 +27,19 @@
>>    * DRM operations:
>>    */
>>   
>> +/* If the DT contains at least one available GPU, return a pointer to it */
>> +

Here is the double blank line my patch remove, it (a blank line) is occupied by
the comment of "/* If the DT contains at least one available GPU, return a pointer to it */"


> I think the code in the function is simple enough that we don't need a
> comment explaining what it does.

Ok, then I'll remove the comment at the next version. Thanks


> Regards,
> Lucas
>
>> +static struct device_node *etnaviv_of_first_node(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	for_each_compatible_node(np, NULL, "vivante,gc") {
>> +		if (of_device_is_available(np))
>> +			return np;
>> +	}
>> +
>> +	return NULL;
>> +}
>>   
>>   static void load_gpu(struct drm_device *dev)
>>   {
>> @@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
>>   static int etnaviv_pdev_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> -	struct device_node *first_node = NULL;
>> +	struct device_node *first_node;
>>   	struct component_match *match = NULL;
>>   
>>   	if (!dev->platform_data) {
>> @@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>>   			if (!of_device_is_available(core_node))
>>   				continue;
>>   
>> -			if (!first_node)
>> -				first_node = core_node;
>> -
>>   			drm_of_component_match_add(&pdev->dev, &match,
>>   						   component_compare_of, core_node);
>> +
>> +			of_node_put(core_node);
>>   		}
>>   	} else {
>>   		char **names = dev->platform_data;
>> @@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>>   	 * device as the GPU we found. This assumes that all Vivante
>>   	 * GPUs in the system share the same DMA constraints.
>>   	 */
>> -	if (first_node)
>> +	first_node = etnaviv_of_first_node();
>> +	if (first_node) {
>>   		of_dma_configure(&pdev->dev, first_node, true);
>> +		of_node_put(first_node);
>> +	}
>>   
>>   	return component_master_add_with_match(dev, &etnaviv_master_ops, match);
>>   }
>> @@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
>>   	 * If the DT contains at least one available GPU device, instantiate
>>   	 * the DRM platform device.
>>   	 */
>> -	for_each_compatible_node(np, NULL, "vivante,gc") {
>> -		if (!of_device_is_available(np))
>> -			continue;
>> +	np = etnaviv_of_first_node();
>> +	if (np) {
>>   		of_node_put(np);
>>   
>>   		ret = etnaviv_create_platform_device("etnaviv",
>>   						     &etnaviv_platform_device);
>>   		if (ret)
>>   			goto unregister_platform_driver;
>> -
>> -		break;
>>   	}
>>   
>>   	return 0;

Re: [PATCH v1 8/8] drm/etnaviv: Add a helper to get a pointer to the first available node

[Sui Jingfeng]

Hi,

On 2023/7/17 18:07, Lucas Stach wrote:
> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> This make the code in etnaviv_pdev_probe() less twisted, drop the reference
>> to device node after finished. Also kill a double blank line.
>>
> I can't spot the double blank line you claim to remove.
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
>>   1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 7d0eeab3e8b7..3446f8eabf59 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -27,6 +27,19 @@
>>    * DRM operations:
>>    */
>>   
>> +/* If the DT contains at least one available GPU, return a pointer to it */
>> +
> I think the code in the function is simple enough that we don't need a
> comment explaining what it does.

Because the DT could disable GPU cores by add "status=disabled" property.

So, only the word *available* in this comments is deserved.

But I'm fine to delete the comment for this function, will be fixed at 
the next version.

Thanks for reviewing.

> Regards,
> Lucas
>
>> +static struct device_node *etnaviv_of_first_node(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	for_each_compatible_node(np, NULL, "vivante,gc") {
>> +		if (of_device_is_available(np))
>> +			return np;
>> +	}
>> +
>> +	return NULL;
>> +}
>>   
>>   static void load_gpu(struct drm_device *dev)
>>   {
>> @@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
>>   static int etnaviv_pdev_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> -	struct device_node *first_node = NULL;
>> +	struct device_node *first_node;
>>   	struct component_match *match = NULL;
>>   
>>   	if (!dev->platform_data) {
>> @@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>>   			if (!of_device_is_available(core_node))
>>   				continue;
>>   
>> -			if (!first_node)
>> -				first_node = core_node;
>> -
>>   			drm_of_component_match_add(&pdev->dev, &match,
>>   						   component_compare_of, core_node);
>> +
>> +			of_node_put(core_node);
>>   		}
>>   	} else {
>>   		char **names = dev->platform_data;
>> @@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>>   	 * device as the GPU we found. This assumes that all Vivante
>>   	 * GPUs in the system share the same DMA constraints.
>>   	 */
>> -	if (first_node)
>> +	first_node = etnaviv_of_first_node();
>> +	if (first_node) {
>>   		of_dma_configure(&pdev->dev, first_node, true);
>> +		of_node_put(first_node);
>> +	}
>>   
>>   	return component_master_add_with_match(dev, &etnaviv_master_ops, match);
>>   }
>> @@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
>>   	 * If the DT contains at least one available GPU device, instantiate
>>   	 * the DRM platform device.
>>   	 */
>> -	for_each_compatible_node(np, NULL, "vivante,gc") {
>> -		if (!of_device_is_available(np))
>> -			continue;
>> +	np = etnaviv_of_first_node();
>> +	if (np) {
>>   		of_node_put(np);
>>   
>>   		ret = etnaviv_create_platform_device("etnaviv",
>>   						     &etnaviv_platform_device);
>>   		if (ret)
>>   			goto unregister_platform_driver;
>> -
>> -		break;
>>   	}
>>   
>>   	return 0;

Re: [PATCH v1 1/8] drm/etnaviv: Using the size_t variable to store the number of pages

[Lucas Stach]

Am Montag, dem 17.07.2023 um 18:12 +0800 schrieb Sui Jingfeng:
> Hi
> 
> On 2023/7/17 17:43, Lucas Stach wrote:
> > Hi Jingfeng,
> > 
> > Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > 
> > > Because the etnaviv_gem_new_private() function receives the size_t argument
> > > for the number of pages. And the number of pages should be unsigned.
> > > 
> > > Note that Most 32-bit architectures use "unsigned int" size_t,
> > > and all 64-bit architectures use "unsigned long" size_t.
> > > So, let's keep the argument and parameter consistent.
> > > 
> > This explanation doesn't add up. npages is just that: a number of
> > pages. Why would it make sense to use size_t here?
> 
> Because the 'size' variable in the etnaviv_gem_prime_import_sg_table() 
> function is declared
> 
> as size_t type. On 64-bit machine, size_t is actually is 64-bit wide and 
> it is *unsigned*.
> 
> While 'int' is actually 32-bit wide(at both 32-bit system and 64-bit 
> system) and it is *signed*,
> 
> So, my point (argument) is that
> 
> 
> 1) This patch help to avoid the unnecessary 64 bit to 32 bit conversion.
> 
> 2) The kvmalloc_array() function also accept  size_t type (see the 
> prototype of  kvmalloc_array function include/linux/slab.h)
> 
> 
> So my patch do helps to keep the code style consistent.
> 
But then we go on to call drm_prime_sq_to_page_array(), which takes a
integer as the number of pages parameter, so the parameter types are
inconsistent before and after your patch, it just switches which
function call has to do some conversion.

> 
> > If you want to be consistent I would have expected this change to
> > switch things to unsigned int,
> 
> This may introduce a truncate operation (from a 64-bit to 32-bit), which 
> is necessary.
> 
If this truncation happens in the real world then something else is
already badly broken. All Vivante GPUs to date can only deal with 32bit
virtual addresses, so a buffer exhausting 31 bits of pages is way
larger than we could ever fit into the GPU VM.

Regards,
Lucas

> And when you pass the 'npages' parameter to kvmalloc_array() function,
> 
> The compiler probably has to do the integer promotion (from a 32-bit to 
> 64-bit) for you.
> 
> 
> > as you did in the second patch of this
> > series.
> > 
> > Regards,
> > Lucas
> > 
> > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > ---
> > >   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> > > index 3524b5811682..b003481adc2b 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> > > @@ -114,7 +114,8 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
> > >   {
> > >   	struct etnaviv_gem_object *etnaviv_obj;
> > >   	size_t size = PAGE_ALIGN(attach->dmabuf->size);
> > > -	int ret, npages;
> > > +	size_t npages = size / PAGE_SIZE;
> > > +	int ret;
> > >   
> > >   	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
> > >   				      &etnaviv_gem_prime_ops, &etnaviv_obj);
> > > @@ -123,8 +124,6 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
> > >   
> > >   	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_prime_lock_class);
> > >   
> > > -	npages = size / PAGE_SIZE;
> > > -
> > >   	etnaviv_obj->sgt = sgt;
> > >   	etnaviv_obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> > >   	if (!etnaviv_obj->pages) {

Re: [PATCH v1 1/8] drm/etnaviv: Using the size_t variable to store the number of pages

[Sui Jingfeng]

[-- Attachment #1: Type: text/plain, Size: 1825 bytes --]

Hi,

On 2023/7/17 18:38, Lucas Stach wrote:
> Am Montag, dem 17.07.2023 um 18:12 +0800 schrieb Sui Jingfeng:
>> Hi
>>
>> On 2023/7/17 17:43, Lucas Stach wrote:
>>> Hi Jingfeng,
>>>
>>> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>>>> From: Sui Jingfeng<suijingfeng@loongson.cn>
>>>>
>>>> Because the etnaviv_gem_new_private() function receives the size_t argument
>>>> for the number of pages. And the number of pages should be unsigned.
>>>>
>>>> Note that Most 32-bit architectures use "unsigned int" size_t,
>>>> and all 64-bit architectures use "unsigned long" size_t.
>>>> So, let's keep the argument and parameter consistent.
>>>>
>>> This explanation doesn't add up. npages is just that: a number of
>>> pages. Why would it make sense to use size_t here?
>> Because the 'size' variable in the etnaviv_gem_prime_import_sg_table()
>> function is declared
>>
>> as size_t type. On 64-bit machine, size_t is actually is 64-bit wide and
>> it is*unsigned*.
>>
>> While 'int' is actually 32-bit wide(at both 32-bit system and 64-bit
>> system) and it is*signed*,
>>
>> So, my point (argument) is that
>>
>>
>> 1) This patch help to avoid the unnecessary 64 bit to 32 bit conversion.
>>
>> 2) The kvmalloc_array() function also accept  size_t type (see the
>> prototype of  kvmalloc_array function include/linux/slab.h)
>>
>>
>> So my patch do helps to keep the code style consistent.
>>
> But then we go on to call drm_prime_sq_to_page_array(), which takes a
> integer as the number of pages parameter, so the parameter types are
> inconsistent before and after your patch, it just switches which
> function call has to do some conversion.
>
But the drm_prime_sg_to_page_array() function is going to be depreciated,

We probably could modified it also to unified it, that is to take size_t 
arguments.

[-- Attachment #2: Type: text/html, Size: 2953 bytes --]

Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[suijingfeng]

Hi,  Lucas


Thanks for you guidance!


On 2023/7/17 17:51, Lucas Stach wrote:
> Hi Jingfeng,
>
> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Because it is not used by the etnaviv_gem_new_impl() function,
>> no functional change.
>>
> I think it would make sense to move into the opposite direction: in
> both callsites of etnaviv_gem_new_impl we immediately call
> drm_gem_object_init with the size.

Really?

But there are multiple call path to the etnaviv_gem_new_impl() function.


Code path 1 (PRIME):

|- etnaviv_gem_prime_import_sg_table()

|--  etnaviv_gem_new_private()

|--- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)

|--- drm_gem_private_object_init(dev, obj, size)


Code path 2 (USERPTR):

|- etnaviv_gem_new_userptr()

|--  etnaviv_gem_new_private()

|--- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)

|--- drm_gem_private_object_init(dev, obj, size)


Code path 3 (construct a GEM buffer object for the user-space):

|- etnaviv_ioctl_gem_new()

|-- etnaviv_gem_new_handle()

|--- etnaviv_gem_new_impl(dev, size, flags, &etnaviv_gem_shmem_ops, &obj);

|---  drm_gem_object_init(dev, obj, size);


If I understand this correctly:


Code path 1 is for cross device (and cross driver) buffer-sharing,

Code path 2 is going to share the buffer the userspace,


*Only* the code path 3 is to construct a GEM buffer object for the 
user-space the userspace,

that is say, *only* the code path 3 need to do the backing memory 
allocation work for the userspace.

thus it need to call drm_gem_object_init() function, which really the 
shmem do the backing memory

allocation.


The code path 1 and the code path 2 do not need the kernel space 
allocate the backing memory.

Because they are going to share the buffer already allocated by others.

thus, code path 2 and code path 3 should call drm_gem_private_object_init(),

*not* the drm_gem_object_init() function.


When import buffer from the a specific KMS driver,

then etnaviv_gem_prime_import_sg_table() will be called.


I guess you means that drm_gem_private_object_init() (not the 
drm_gem_object_init() function)here ?


> A better cleanup would be to make
> use of the size parameter and move this object init call into
> etnaviv_gem_new_impl.

If I following you guidance, how do I differentiate the cases

when to call drm_gem_private_object_init() not drm_gem_object_init() ?

and when call drm_gem_object_init() not drm_gem_private_object_init()?


I don't think you are right here.

>
> Regards,
> Lucas
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index b5f73502e3dd..be2f459c66b5 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>   	.vm_ops = &vm_ops,
>>   };
>>   
>> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>>   	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>>   {
>>   	struct etnaviv_gem_object *etnaviv_obj;
>> @@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>>   
>>   	size = PAGE_ALIGN(size);
>>   
>> -	ret = etnaviv_gem_new_impl(dev, size, flags,
>> -				   &etnaviv_gem_shmem_ops, &obj);
>> +	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
>>   	if (ret)
>>   		goto fail;
>>   
>> @@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>>   	struct drm_gem_object *obj;
>>   	int ret;
>>   
>> -	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
>> +	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
>>   	if (ret)
>>   		return ret;
>>   

Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[Lucas Stach]

Hi Jingfeng,

Am Dienstag, dem 18.07.2023 um 02:34 +0800 schrieb suijingfeng:
> Hi,  Lucas
> 
> 
> Thanks for you guidance!
> 
> 
> On 2023/7/17 17:51, Lucas Stach wrote:
> > Hi Jingfeng,
> > 
> > Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > 
> > > Because it is not used by the etnaviv_gem_new_impl() function,
> > > no functional change.
> > > 
> > I think it would make sense to move into the opposite direction: in
> > both callsites of etnaviv_gem_new_impl we immediately call
> > drm_gem_object_init with the size.
> 
> Really?
> 
> But there are multiple call path to the etnaviv_gem_new_impl() function.
> 
> 
> Code path 1 (PRIME):
> 
> > - etnaviv_gem_prime_import_sg_table()
> 
> > --  etnaviv_gem_new_private()
> 
> > --- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)
> 
> > --- drm_gem_private_object_init(dev, obj, size)
> 
> 
> Code path 2 (USERPTR):
> 
> > - etnaviv_gem_new_userptr()
> 
> > --  etnaviv_gem_new_private()
> 
> > --- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)
> 
> > --- drm_gem_private_object_init(dev, obj, size)
> 
> 
> Code path 3 (construct a GEM buffer object for the user-space):
> 
> > - etnaviv_ioctl_gem_new()
> 
> > -- etnaviv_gem_new_handle()
> 
> > --- etnaviv_gem_new_impl(dev, size, flags, &etnaviv_gem_shmem_ops, &obj);
> 
> > ---  drm_gem_object_init(dev, obj, size);
> 
> 
> If I understand this correctly:
> 
> 
> Code path 1 is for cross device (and cross driver) buffer-sharing,
> 
> Code path 2 is going to share the buffer the userspace,
> 
> 
> *Only* the code path 3 is to construct a GEM buffer object for the 
> user-space the userspace,
> 
> that is say, *only* the code path 3 need to do the backing memory 
> allocation work for the userspace.
> 
> thus it need to call drm_gem_object_init() function, which really the 
> shmem do the backing memory
> 
> allocation.
> 
> 
> The code path 1 and the code path 2 do not need the kernel space 
> allocate the backing memory.
> 
> Because they are going to share the buffer already allocated by others.
> 
> thus, code path 2 and code path 3 should call drm_gem_private_object_init(),
> 
> *not* the drm_gem_object_init() function.
> 
> 
> When import buffer from the a specific KMS driver,
> 
> then etnaviv_gem_prime_import_sg_table() will be called.
> 
> 
> I guess you means that drm_gem_private_object_init() (not the 
> drm_gem_object_init() function)here ?
> 
> 
> > A better cleanup would be to make
> > use of the size parameter and move this object init call into
> > etnaviv_gem_new_impl.
> 
> If I following you guidance, how do I differentiate the cases
> 
> when to call drm_gem_private_object_init() not drm_gem_object_init() ?
> 
> and when call drm_gem_object_init() not drm_gem_private_object_init()?
> 
> 
> I don't think you are right here.
> 
Yes, clearly I was not taking into account the differences between
drm_gem_private_object_init and drm_gem_object_init properly. Please
disregard my comment, this patch is good as-is.

Regards,
Lucas

> > 
> > Regards,
> > Lucas
> > 
> > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > ---
> > >   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > index b5f73502e3dd..be2f459c66b5 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > @@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
> > >   	.vm_ops = &vm_ops,
> > >   };
> > >   
> > > -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
> > > +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
> > >   	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
> > >   {
> > >   	struct etnaviv_gem_object *etnaviv_obj;
> > > @@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> > >   
> > >   	size = PAGE_ALIGN(size);
> > >   
> > > -	ret = etnaviv_gem_new_impl(dev, size, flags,
> > > -				   &etnaviv_gem_shmem_ops, &obj);
> > > +	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
> > >   	if (ret)
> > >   		goto fail;
> > >   
> > > @@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
> > >   	struct drm_gem_object *obj;
> > >   	int ret;
> > >   
> > > -	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
> > > +	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
> > >   	if (ret)
> > >   		return ret;
> > >   
> 

Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[suijingfeng]

Hi,

On 2023/7/18 16:12, Lucas Stach wrote:
> Hi Jingfeng,
>
> Am Dienstag, dem 18.07.2023 um 02:34 +0800 schrieb suijingfeng:
>> Hi,  Lucas
>>
>>
>> Thanks for you guidance!
>>
>>
>> On 2023/7/17 17:51, Lucas Stach wrote:
>>> Hi Jingfeng,
>>>
>>> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>>> Because it is not used by the etnaviv_gem_new_impl() function,
>>>> no functional change.
>>>>
>>> I think it would make sense to move into the opposite direction: in
>>> both callsites of etnaviv_gem_new_impl we immediately call
>>> drm_gem_object_init with the size.
>> Really?
>>
>> But there are multiple call path to the etnaviv_gem_new_impl() function.
>>
>>
>> Code path 1 (PRIME):
>>
>>> - etnaviv_gem_prime_import_sg_table()
>>> --  etnaviv_gem_new_private()
>>> --- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)
>>> --- drm_gem_private_object_init(dev, obj, size)
>>
>> Code path 2 (USERPTR):
>>
>>> - etnaviv_gem_new_userptr()
>>> --  etnaviv_gem_new_private()
>>> --- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)
>>> --- drm_gem_private_object_init(dev, obj, size)
>>
>> Code path 3 (construct a GEM buffer object for the user-space):
>>
>>> - etnaviv_ioctl_gem_new()
>>> -- etnaviv_gem_new_handle()
>>> --- etnaviv_gem_new_impl(dev, size, flags, &etnaviv_gem_shmem_ops, &obj);
>>> ---  drm_gem_object_init(dev, obj, size);
>>
>> If I understand this correctly:
>>
>>
>> Code path 1 is for cross device (and cross driver) buffer-sharing,
>>
>> Code path 2 is going to share the buffer the userspace,
>>
>>
>> *Only* the code path 3 is to construct a GEM buffer object for the
>> user-space the userspace,
>>
>> that is say, *only* the code path 3 need to do the backing memory
>> allocation work for the userspace.
>>
>> thus it need to call drm_gem_object_init() function, which really the
>> shmem do the backing memory
>>
>> allocation.
>>
>>
>> The code path 1 and the code path 2 do not need the kernel space
>> allocate the backing memory.
>>
>> Because they are going to share the buffer already allocated by others.
>>
>> thus, code path 2 and code path 3 should call drm_gem_private_object_init(),
>>
>> *not* the drm_gem_object_init() function.
>>
>>
>> When import buffer from the a specific KMS driver,
>>
>> then etnaviv_gem_prime_import_sg_table() will be called.
>>
>>
>> I guess you means that drm_gem_private_object_init() (not the
>> drm_gem_object_init() function)here ?
>>
>>
>>> A better cleanup would be to make
>>> use of the size parameter and move this object init call into
>>> etnaviv_gem_new_impl.
>> If I following you guidance, how do I differentiate the cases
>>
>> when to call drm_gem_private_object_init() not drm_gem_object_init() ?
>>
>> and when call drm_gem_object_init() not drm_gem_private_object_init()?
>>
>>
>> I don't think you are right here.
>>
> Yes, clearly I was not taking into account the differences between
> drm_gem_private_object_init and drm_gem_object_init properly. Please
> disregard my comment, this patch is good as-is.

I have study your patch in the past frequently.

As you could solve very complex(and difficulty) bugs.

So I still believe that you know everything about etnaviv.

I'm just wondering that you are designing the traps. But I'm not sure.

Okay, still acceptable.

Because communicate will you is interesting.

Thank you.

> Regards,
> Lucas
>
>>> Regards,
>>> Lucas
>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> index b5f73502e3dd..be2f459c66b5 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> @@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>>>    	.vm_ops = &vm_ops,
>>>>    };
>>>>    
>>>> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>>> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>>>>    	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>>>>    {
>>>>    	struct etnaviv_gem_object *etnaviv_obj;
>>>> @@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>>>>    
>>>>    	size = PAGE_ALIGN(size);
>>>>    
>>>> -	ret = etnaviv_gem_new_impl(dev, size, flags,
>>>> -				   &etnaviv_gem_shmem_ops, &obj);
>>>> +	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
>>>>    	if (ret)
>>>>    		goto fail;
>>>>    
>>>> @@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>>>>    	struct drm_gem_object *obj;
>>>>    	int ret;
>>>>    
>>>> -	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
>>>> +	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
>>>>    	if (ret)
>>>>    		return ret;
>>>>    

Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[Sui Jingfeng]

On 2023/7/19 00:16, suijingfeng wrote:
> Because communicate will you


'will' -> 'with'

Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[Lucas Stach]

Hi Jingfeng,

Am Mittwoch, dem 19.07.2023 um 00:16 +0800 schrieb suijingfeng:
> Hi,
> > > 
[...]
> > > I don't think you are right here.
> > > 
> > Yes, clearly I was not taking into account the differences between
> > drm_gem_private_object_init and drm_gem_object_init properly. Please
> > disregard my comment, this patch is good as-is.
> 
> I have study your patch in the past frequently.
> 
> As you could solve very complex(and difficulty) bugs.
> 
> So I still believe that you know everything about etnaviv.
> 
While flattering, even I myself am not thinking I know everything about
etnaviv. The different HW generations and the complex subsystem the
driver is living in doesn't make it easy for anyone to keep in mind
everything.

> I'm just wondering that you are designing the traps. But I'm not sure.
> 
Certainly not. I'm just human and do make mistakes as everyone. During
the quick scrolling though the code when reviewing this patch my mind
clearly just dropped the _private_ part of one of the function names. I
appreciate being told when I am wrong and I do believe that the
exchange during the review is helpful for everyone to get on the same
page.

Regards,
Lucas

> Okay, still acceptable.
> 
> Because communicate will you is interesting.
> 
> Thank you.
> 
> > Regards,
> > Lucas
> > 
> > > > Regards,
> > > > Lucas
> > > > 
> > > > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > > > ---
> > > > >    drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
> > > > >    1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > > > index b5f73502e3dd..be2f459c66b5 100644
> > > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > > > @@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
> > > > >    	.vm_ops = &vm_ops,
> > > > >    };
> > > > >    
> > > > > -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
> > > > > +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
> > > > >    	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
> > > > >    {
> > > > >    	struct etnaviv_gem_object *etnaviv_obj;
> > > > > @@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> > > > >    
> > > > >    	size = PAGE_ALIGN(size);
> > > > >    
> > > > > -	ret = etnaviv_gem_new_impl(dev, size, flags,
> > > > > -				   &etnaviv_gem_shmem_ops, &obj);
> > > > > +	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
> > > > >    	if (ret)
> > > > >    		goto fail;
> > > > >    
> > > > > @@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
> > > > >    	struct drm_gem_object *obj;
> > > > >    	int ret;
> > > > >    
> > > > > -	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
> > > > > +	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
> > > > >    	if (ret)
> > > > >    		return ret;
> > > > >    
>