RE: [PATCH] drm/amdkfd: Initialize kfd_gpu_cache_info for KFD topology

[Deucher, Alexander]

[AMD Official Use Only - General]

> -Original Message-
> From: Kuehling, Felix 
> Sent: Tuesday, February 6, 2024 4:15 PM
> To: Greathouse, Joseph ; amd-
> g...@lists.freedesktop.org; Deucher, Alexander
> 
> Subject: Re: [PATCH] drm/amdkfd: Initialize kfd_gpu_cache_info for KFD
> topology
>
>
> On 2024-02-06 15:55, Joseph Greathouse wrote:
> > The current kfd_gpu_cache_info structure is only partially filled in
> > for some architectures. This means that for devices where we do not
> > fill in some fields, we can returned uninitialized values through  the
> > KFD topology.
> > Zero out the kfd_gpu_cache_info before asking the remaining fields to
> > be filled in by lower-level functions.
> >
> > Signed-off-by: Joseph Greathouse 
>
> This fixes your previous patch "drm/amdkfd: Add cache line sizes to KFD
> topology". Alex, I think the previous patch hasn't gone upstream yet. Do you
> want a Fixes: tag or is is possible to squash this with Joe's previous patch
> before upstreaming?

Either way.  I can fix up the tag when we upstream or squash it.

Alex

>
> One nit-pick below.
>
>
> > ---
> >   drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> > index 3df2a8ad86fb..67c1e7f84750 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> > @@ -1707,6 +1707,7 @@ static void kfd_fill_cache_non_crat_info(struct
> > kfd_topology_device *dev, struct
> >
> > gpu_processor_id = dev->node_props.simd_id_base;
> >
> > +   memset(cache_info, 0, sizeof(struct kfd_gpu_cache_info) *
> > +KFD_MAX_CACHE_TYPES);
>
> Just use sizeof(cache_info). No need to calculate the size of the array and 
> risk
> getting it wrong.
>
> Regards,
>Felix
>
>
> > pcache_info = cache_info;
> > num_of_cache_types = kfd_get_gpu_cache_info(kdev, _info);
> > if (!num_of_cache_types) {

Re: [PATCH] drm/amdkfd: Initialize kfd_gpu_cache_info for KFD topology

[Felix Kuehling]

On 2024-02-06 15:55, Joseph Greathouse wrote:

The current kfd_gpu_cache_info structure is only partially
filled in for some architectures. This means that for devices
where we do not fill in some fields, we can returned
uninitialized values through  the KFD topology.
Zero out the kfd_gpu_cache_info before asking the remaining
fields to be filled in by lower-level functions.

Signed-off-by: Joseph Greathouse 


This fixes your previous patch "drm/amdkfd: Add cache line sizes to KFD 
topology". Alex, I think the previous patch hasn't gone upstream yet. Do 
you want a Fixes: tag or is is possible to squash this with Joe's 
previous patch before upstreaming?


One nit-pick below.



---
  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 3df2a8ad86fb..67c1e7f84750 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1707,6 +1707,7 @@ static void kfd_fill_cache_non_crat_info(struct 
kfd_topology_device *dev, struct
  
  	gpu_processor_id = dev->node_props.simd_id_base;
  
+	memset(cache_info, 0, sizeof(struct kfd_gpu_cache_info) * KFD_MAX_CACHE_TYPES);


Just use sizeof(cache_info). No need to calculate the size of the array 
and risk getting it wrong.


Regards,
  Felix



pcache_info = cache_info;
num_of_cache_types = kfd_get_gpu_cache_info(kdev, _info);
if (!num_of_cache_types) {