Re: [Freedreno] [PATCH 02/12] drm/msm/adreno: Remove redundant gmem size param

2023-07-13 Thread Akhil P Oommen 
On Fri, Jul 07, 2023 at 01:22:56AM +0200, Konrad Dybcio wrote:
> 
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark 
> > 
> > Even in the ocmem case, the allocated ocmem buffer size should match the
> > requested size.
> > 
> > Signed-off-by: Rob Clark 
> > ---
> [...]
> 
> > +
> > +   WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);
> I believe this should be an error condition. If the sizes are mismatched,
> best case scenario you get suboptimal perf and worst case scenario your
> system explodes.

No, the worst case scenarios are subtle bugs like random corruptions,
pagefaults etc which you debug for months. ;)

-Akhil.

> 
> Very nice cleanup though!
> 
> Konrad
> >  
> > return 0;
> >  }
> > @@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct 
> > platform_device *pdev,
> >  
> > adreno_gpu->funcs = funcs;
> > adreno_gpu->info = adreno_info(config->rev);
> > -   adreno_gpu->gmem = adreno_gpu->info->gmem;
> > adreno_gpu->revn = adreno_gpu->info->revn;
> > adreno_gpu->rev = *rev;
> >  
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 6830c3776c2d..aaf09c642dc6 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -77,7 +77,6 @@ struct adreno_gpu {
> > struct msm_gpu base;
> > struct adreno_rev rev;
> > const struct adreno_info *info;
> > -   uint32_t gmem;  /* actual gmem size */
> > uint32_t revn;  /* numeric revision name */
> > uint16_t speedbin;
> > const struct adreno_gpu_funcs *funcs;

Re: [Freedreno] [PATCH 02/12] drm/msm/adreno: Remove redundant gmem size param

2023-07-06 Thread Dmitry Baryshkov 

On 07/07/2023 00:10, Rob Clark wrote:

From: Rob Clark 

Even in the ocmem case, the allocated ocmem buffer size should match the
requested size.

Signed-off-by: Rob Clark 
---
  drivers/gpu/drm/msm/adreno/a2xx_gpu.c  | 2 +-
  drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 2 +-
  drivers/gpu/drm/msm/adreno/a6xx_gpu.c  | 2 +-
  drivers/gpu/drm/msm/adreno/adreno_device.c | 1 -
  drivers/gpu/drm/msm/adreno/adreno_gpu.c| 8 
  drivers/gpu/drm/msm/adreno/adreno_gpu.h| 1 -
  6 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
index c67089a7ebc1..50ee03bc94b4 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
@@ -205,7 +205,7 @@ static int a2xx_hw_init(struct msm_gpu *gpu)
A2XX_MH_INTERRUPT_MASK_MMU_PAGE_FAULT);
  
  	for (i = 3; i <= 5; i++)

-   if ((SZ_16K << i) == adreno_gpu->gmem)
+   if ((SZ_16K << i) == adreno_gpu->info->gmem)
break;
gpu_write(gpu, REG_A2XX_RB_EDRAM_INFO, i);
  
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c

index a99310b68793..f0803e94ebe5 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -749,7 +749,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MIN_LO, 0x0010);
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MIN_HI, 0x);
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_LO,
-   0x0010 + adreno_gpu->gmem - 1);
+   0x0010 + adreno_gpu->info->gmem - 1);
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_HI, 0x);
  
  	if (adreno_is_a506(adreno_gpu) || adreno_is_a508(adreno_gpu) ||

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index b3ada1e7b598..edbade75020f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1270,7 +1270,7 @@ static int hw_init(struct msm_gpu *gpu)
gpu_write64(gpu, REG_A6XX_UCHE_GMEM_RANGE_MIN, 0x0010);
  
  		gpu_write64(gpu, REG_A6XX_UCHE_GMEM_RANGE_MAX,

-   0x0010 + adreno_gpu->gmem - 1);
+   0x0010 + adreno_gpu->info->gmem - 1);
}
  
  	gpu_write(gpu, REG_A6XX_UCHE_FILTER_CNTL, 0x804);

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 5eba0ae5c9a7..326912284a95 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -239,7 +239,6 @@ static const struct adreno_info gpulist[] = {
}, {
.rev = ADRENO_REV(6, 1, 0, ANY_ID),
.revn = 610,
-   .name = "A610",
.fw = {
[ADRENO_FW_SQE] = "a630_sqe.fw",
},


This one should go to the previous patch.



diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index e3cd9ff6ff1d..4f59682f585e 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -320,7 +320,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct 
msm_file_private *ctx,
*value = adreno_gpu->info->revn;
return 0;
case MSM_PARAM_GMEM_SIZE:
-   *value = adreno_gpu->gmem;
+   *value = adreno_gpu->info->gmem;
return 0;
case MSM_PARAM_GMEM_BASE:
*value = !adreno_is_a650_family(adreno_gpu) ? 0x10 : 0;
@@ -1041,14 +1041,15 @@ int adreno_gpu_ocmem_init(struct device *dev, struct 
adreno_gpu *adreno_gpu,
return PTR_ERR(ocmem);
}
  
-	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);

+   ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, 
adreno_gpu->info->gmem);
if (IS_ERR(ocmem_hdl))
return PTR_ERR(ocmem_hdl);
  
  	adreno_ocmem->ocmem = ocmem;

adreno_ocmem->base = ocmem_hdl->addr;
adreno_ocmem->hdl = ocmem_hdl;
-   adreno_gpu->gmem = ocmem_hdl->len;
+
+   WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);
  
  	return 0;

  }
@@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct 
platform_device *pdev,
  
  	adreno_gpu->funcs = funcs;

adreno_gpu->info = adreno_info(config->rev);
-   adreno_gpu->gmem = adreno_gpu->info->gmem;
adreno_gpu->revn = adreno_gpu->info->revn;
adreno_gpu->rev = *rev;
  
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h

index 6830c3776c2d..aaf09c642dc6 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -77,7 +77,6 @@ struct adreno_gpu {
struct msm_gpu base;
struct adreno_rev rev;
const struct

Re: [Freedreno] [PATCH 02/12] drm/msm/adreno: Remove redundant gmem size param

2023-07-06 Thread Konrad Dybcio 
On 6.07.2023 23:10, Rob Clark wrote:
> From: Rob Clark 
> 
> Even in the ocmem case, the allocated ocmem buffer size should match the
> requested size.
> 
> Signed-off-by: Rob Clark 
> ---
[...]

> +
> + WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);
I believe this should be an error condition. If the sizes are mismatched,
best case scenario you get suboptimal perf and worst case scenario your
system explodes.

Very nice cleanup though!

Konrad
>  
>   return 0;
>  }
> @@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct 
> platform_device *pdev,
>  
>   adreno_gpu->funcs = funcs;
>   adreno_gpu->info = adreno_info(config->rev);
> - adreno_gpu->gmem = adreno_gpu->info->gmem;
>   adreno_gpu->revn = adreno_gpu->info->revn;
>   adreno_gpu->rev = *rev;
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 6830c3776c2d..aaf09c642dc6 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -77,7 +77,6 @@ struct adreno_gpu {
>   struct msm_gpu base;
>   struct adreno_rev rev;
>   const struct adreno_info *info;
> - uint32_t gmem;  /* actual gmem size */
>   uint32_t revn;  /* numeric revision name */
>   uint16_t speedbin;
>   const struct adreno_gpu_funcs *funcs;

[Freedreno] [PATCH 02/12] drm/msm/adreno: Remove redundant gmem size param

2023-07-06 Thread Rob Clark 
From: Rob Clark 

Even in the ocmem case, the allocated ocmem buffer size should match the
requested size.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/adreno/a2xx_gpu.c  | 2 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 2 +-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c  | 2 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c | 1 -
 drivers/gpu/drm/msm/adreno/adreno_gpu.c| 8 
 drivers/gpu/drm/msm/adreno/adreno_gpu.h| 1 -
 6 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
index c67089a7ebc1..50ee03bc94b4 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
@@ -205,7 +205,7 @@ static int a2xx_hw_init(struct msm_gpu *gpu)
A2XX_MH_INTERRUPT_MASK_MMU_PAGE_FAULT);
 
for (i = 3; i <= 5; i++)
-   if ((SZ_16K << i) == adreno_gpu->gmem)
+   if ((SZ_16K << i) == adreno_gpu->info->gmem)
break;
gpu_write(gpu, REG_A2XX_RB_EDRAM_INFO, i);
 
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a99310b68793..f0803e94ebe5 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -749,7 +749,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MIN_LO, 0x0010);
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MIN_HI, 0x);
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_LO,
-   0x0010 + adreno_gpu->gmem - 1);
+   0x0010 + adreno_gpu->info->gmem - 1);
gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_HI, 0x);
 
if (adreno_is_a506(adreno_gpu) || adreno_is_a508(adreno_gpu) ||
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index b3ada1e7b598..edbade75020f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1270,7 +1270,7 @@ static int hw_init(struct msm_gpu *gpu)
gpu_write64(gpu, REG_A6XX_UCHE_GMEM_RANGE_MIN, 0x0010);
 
gpu_write64(gpu, REG_A6XX_UCHE_GMEM_RANGE_MAX,
-   0x0010 + adreno_gpu->gmem - 1);
+   0x0010 + adreno_gpu->info->gmem - 1);
}
 
gpu_write(gpu, REG_A6XX_UCHE_FILTER_CNTL, 0x804);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 5eba0ae5c9a7..326912284a95 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -239,7 +239,6 @@ static const struct adreno_info gpulist[] = {
}, {
.rev = ADRENO_REV(6, 1, 0, ANY_ID),
.revn = 610,
-   .name = "A610",
.fw = {
[ADRENO_FW_SQE] = "a630_sqe.fw",
},
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index e3cd9ff6ff1d..4f59682f585e 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -320,7 +320,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct 
msm_file_private *ctx,
*value = adreno_gpu->info->revn;
return 0;
case MSM_PARAM_GMEM_SIZE:
-   *value = adreno_gpu->gmem;
+   *value = adreno_gpu->info->gmem;
return 0;
case MSM_PARAM_GMEM_BASE:
*value = !adreno_is_a650_family(adreno_gpu) ? 0x10 : 0;
@@ -1041,14 +1041,15 @@ int adreno_gpu_ocmem_init(struct device *dev, struct 
adreno_gpu *adreno_gpu,
return PTR_ERR(ocmem);
}
 
-   ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
+   ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, 
adreno_gpu->info->gmem);
if (IS_ERR(ocmem_hdl))
return PTR_ERR(ocmem_hdl);
 
adreno_ocmem->ocmem = ocmem;
adreno_ocmem->base = ocmem_hdl->addr;
adreno_ocmem->hdl = ocmem_hdl;
-   adreno_gpu->gmem = ocmem_hdl->len;
+
+   WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);
 
return 0;
 }
@@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct 
platform_device *pdev,
 
adreno_gpu->funcs = funcs;
adreno_gpu->info = adreno_info(config->rev);
-   adreno_gpu->gmem = adreno_gpu->info->gmem;
adreno_gpu->revn = adreno_gpu->info->revn;
adreno_gpu->rev = *rev;
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 6830c3776c2d..aaf09c642dc6 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -77,7 +77,6 @@ struct adreno_gpu {
struct msm_gpu base;
struct adreno_rev rev;
const struct adreno_info *info;
-   uint32_t gmem;  /* actual gmem size */