Re: [PATCH] drm/v3d: Use kvcalloc

[Harshit Mogalapalli]

On 28/03/22 5:55 pm, Melissa Wen wrote:

On 03/12, Harshit Mogalapalli wrote:

kvcalloc is same as kvmalloc_array + __GFP_ZERO.

Signed-off-by: Harshit Mogalapalli 
---
  drivers/gpu/drm/v3d/v3d_gem.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index c7ed2e1cbab6..f7d37228461e 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
return -EINVAL;
}
  
-	job->bo = kvmalloc_array(job->bo_count,

-sizeof(struct drm_gem_cma_object *),
-GFP_KERNEL | __GFP_ZERO);
+   job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
+  GFP_KERNEL);


Hi Harshit,
Hi Melissa,



This change seems valid to me, but I believe, in this point, v3d should
move to use the DRM function `drm_gem_objects_lookup()`, and then your
change goes there, since drm_get_objects_lookup() has the same issue
you're pointing. What do you think?


Thanks for looking at the patch.

Yes, you are right, the issue is same there as well.

Few other similar instances in drm/ subsystem.

drivers/gpu/drm/drm_gem.c:700 drm_gem_objects_lookup() warn: Please 
consider using kvcalloc instead
drivers/gpu/drm/ttm/ttm_tt.c:99 ttm_tt_alloc_page_directory() warn: 
Please consider using kvcalloc instead
drivers/gpu/drm/ttm/ttm_tt.c:108 ttm_dma_tt_alloc_page_directory() warn: 
Please consider using kvcalloc instead
drivers/gpu/drm/ttm/ttm_tt.c:121 ttm_sg_tt_alloc_page_directory() warn: 
Please consider using kvcalloc instead
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:541 amdgpu_cs_parser_bos() warn: 
Please consider using kvcalloc instead
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_svm.c:152 
svm_range_dma_map_dev() warn: Please consider using kvcalloc instead
drivers/gpu/drm/v3d/v3d_gem.c:311 v3d_lookup_bos() warn: Please consider 
using kvcalloc instead
drivers/gpu/drm/vc4/vc4_gem.c:746 vc4_cl_lookup_bos() warn: Please 
consider using kvcalloc instead
drivers/gpu/drm/lima/lima_gem.c:42 lima_heap_alloc() warn: Please 
consider using kvcalloc instead
drivers/gpu/drm/panfrost/panfrost_drv.c:147 panfrost_lookup_bos() warn: 
Please consider using kvcalloc instead
drivers/gpu/drm/panfrost/panfrost_mmu.c:452 
panfrost_mmu_map_fault_addr() warn: Please consider using kvcalloc instead
drivers/gpu/drm/panfrost/panfrost_mmu.c:460 
panfrost_mmu_map_fault_addr() warn: Please consider using kvcalloc instead


Tool Used: Smatch.


I already sent a patchset to replace steps in v3d_lookup_bos() by
drm_gem_objects_lookup(), as I mentioned. The patchset is here:
https://patchwork.freedesktop.org/series/101610/
Willing to review it? ^



Sorry Melissa, I am still a beginner, Can't review it.

Regards,
Harshit


Thanks,

Melissa


if (!job->bo) {
DRM_DEBUG("Failed to allocate validated BO pointers\n");
return -ENOMEM;
--
2.31.1

Re: [PATCH] drm/v3d: Use kvcalloc

[Melissa Wen]

On 03/12, Harshit Mogalapalli wrote:
> kvcalloc is same as kvmalloc_array + __GFP_ZERO.
> 
> Signed-off-by: Harshit Mogalapalli 
> ---
>  drivers/gpu/drm/v3d/v3d_gem.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index c7ed2e1cbab6..f7d37228461e 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> - job->bo = kvmalloc_array(job->bo_count,
> -  sizeof(struct drm_gem_cma_object *),
> -  GFP_KERNEL | __GFP_ZERO);
> + job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
> +GFP_KERNEL);

Hi Harshit,

This change seems valid to me, but I believe, in this point, v3d should
move to use the DRM function `drm_gem_objects_lookup()`, and then your
change goes there, since drm_get_objects_lookup() has the same issue
you're pointing. What do you think?

I already sent a patchset to replace steps in v3d_lookup_bos() by
drm_gem_objects_lookup(), as I mentioned. The patchset is here:
https://patchwork.freedesktop.org/series/101610/
Willing to review it? ^

Thanks,

Melissa

>   if (!job->bo) {
>   DRM_DEBUG("Failed to allocate validated BO pointers\n");
>   return -ENOMEM;
> -- 
> 2.31.1
> 


signature.asc
Description: PGP signature

Re: [PATCH] drm/v3d: Use kvcalloc

[Harshit Mogalapalli]

Hi Joe,

On 13/03/22 3:57 am, Joe Perches wrote:

On Sat, 2022-03-12 at 07:26 -0800, Harshit Mogalapalli wrote:

kvcalloc is same as kvmalloc_array + __GFP_ZERO.

[]

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c

[]

@@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
return -EINVAL;
}
  
-	job->bo = kvmalloc_array(job->bo_count,

-sizeof(struct drm_gem_cma_object *),
-GFP_KERNEL | __GFP_ZERO);
+   job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
+  GFP_KERNEL);
if (!job->bo) {
DRM_DEBUG("Failed to allocate validated BO pointers\n");
return -ENOMEM;


trivia:

The DRM_DEBUG could also be removed as the the alloc will do a
a dump_stack on failure.



Thanks for sharing your comments.

DRM_DEBUG statements are present in other parts of code as well. So 
didnot remove it.


Example below:
drivers/gpu/drm/v3d/v3d_gem.c at 322.

 311 job->bo = kvmalloc_array(job->bo_count,
 312  sizeof(struct drm_gem_cma_object *),
 313  GFP_KERNEL | __GFP_ZERO);
 314 if (!job->bo) {
 315 DRM_DEBUG("Failed to allocate validated BO 
pointers\n");

 316 return -ENOMEM;
 317 }
 318
 319 handles = kvmalloc_array(job->bo_count, sizeof(u32), 
GFP_KERNEL);

 320 if (!handles) {
 321 ret = -ENOMEM;
 322 DRM_DEBUG("Failed to allocate incoming GEM 
handles\n");

 323 goto fail;
 324 }


Regards,
Harshit

[PATCH] drm/v3d: Use kvcalloc

[Harshit Mogalapalli]

kvcalloc is same as kvmalloc_array + __GFP_ZERO.

Signed-off-by: Harshit Mogalapalli 
---
 drivers/gpu/drm/v3d/v3d_gem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index c7ed2e1cbab6..f7d37228461e 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
return -EINVAL;
}
 
-   job->bo = kvmalloc_array(job->bo_count,
-sizeof(struct drm_gem_cma_object *),
-GFP_KERNEL | __GFP_ZERO);
+   job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
+  GFP_KERNEL);
if (!job->bo) {
DRM_DEBUG("Failed to allocate validated BO pointers\n");
return -ENOMEM;
-- 
2.31.1

Re: [PATCH] drm/v3d: Use kvcalloc

[Joe Perches]

On Sat, 2022-03-12 at 07:26 -0800, Harshit Mogalapalli wrote:
> kvcalloc is same as kvmalloc_array + __GFP_ZERO.
[]
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
[]
> @@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> - job->bo = kvmalloc_array(job->bo_count,
> -  sizeof(struct drm_gem_cma_object *),
> -  GFP_KERNEL | __GFP_ZERO);
> + job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
> +GFP_KERNEL);
>   if (!job->bo) {
>   DRM_DEBUG("Failed to allocate validated BO pointers\n");
>   return -ENOMEM;

trivia:

The DRM_DEBUG could also be removed as the the alloc will do a
a dump_stack on failure.