Re: [PATCH] drm/etnaviv: remove register logging

[Christian Gmeiner]

2018-04-20 19:14 GMT+02:00 Lucas Stach :
> I'm not aware of any case where tracing GPU register manipulation at the
> kernel level would have been useful. It only adds more indirections and
> adds to the code size.

It was a nice to have during initial driver development and I am okay
to remove it.

> Signed-off-by: Lucas Stach 

Reviewed-by: Christian Gmeiner 

> ---
>  drivers/gpu/drm/etnaviv/Kconfig   |  8 --
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 51 
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h |  5 
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  4 ++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  4 +--
>  5 files changed, 5 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
> index e5bfeca361bd..041a77e400d4 100644
> --- a/drivers/gpu/drm/etnaviv/Kconfig
> +++ b/drivers/gpu/drm/etnaviv/Kconfig
> @@ -22,11 +22,3 @@ config DRM_ETNAVIV_THERMAL
> help
>   Compile in support for thermal throttling.
>   Say Y unless you want to risk burning your SoC.
> -
> -config DRM_ETNAVIV_REGISTER_LOGGING
> -   bool "enable ETNAVIV register logging"
> -   depends on DRM_ETNAVIV
> -   help
> - Compile in support for logging register reads/writes in a format
> - that can be parsed by envytools demsm tool.  If enabled, register
> - logging can be switched on via etnaviv.reglog=y module param.
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index ab50090d066c..0aa543d75953 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -25,57 +25,6 @@
>  #include "etnaviv_mmu.h"
>  #include "etnaviv_perfmon.h"
>
> -#ifdef CONFIG_DRM_ETNAVIV_REGISTER_LOGGING
> -static bool reglog;
> -MODULE_PARM_DESC(reglog, "Enable register read/write logging");
> -module_param(reglog, bool, 0600);
> -#else
> -#define reglog 0
> -#endif
> -
> -void __iomem *etnaviv_ioremap(struct platform_device *pdev, const char *name,
> -   const char *dbgname)
> -{
> -   struct resource *res;
> -   void __iomem *ptr;
> -
> -   if (name)
> -   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
> name);
> -   else
> -   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -   ptr = devm_ioremap_resource(>dev, res);
> -   if (IS_ERR(ptr)) {
> -   dev_err(>dev, "failed to ioremap %s: %ld\n", name,
> -   PTR_ERR(ptr));
> -   return ptr;
> -   }
> -
> -   if (reglog)
> -   dev_printk(KERN_DEBUG, >dev, "IO:region %s 0x%p 
> %08zx\n",
> -  dbgname, ptr, (size_t)resource_size(res));
> -
> -   return ptr;
> -}
> -
> -void etnaviv_writel(u32 data, void __iomem *addr)
> -{
> -   if (reglog)
> -   printk(KERN_DEBUG "IO:W %p %08x\n", addr, data);
> -
> -   writel(data, addr);
> -}
> -
> -u32 etnaviv_readl(const void __iomem *addr)
> -{
> -   u32 val = readl(addr);
> -
> -   if (reglog)
> -   printk(KERN_DEBUG "IO:R %p %08x\n", addr, val);
> -
> -   return val;
> -}
> -
>  /*
>   * DRM operations:
>   */
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h 
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index ddb17ee565e9..56be51c13c49 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -101,11 +101,6 @@ void etnaviv_gem_describe_objects(struct 
> etnaviv_drm_private *priv,
> struct seq_file *m);
>  #endif
>
> -void __iomem *etnaviv_ioremap(struct platform_device *pdev, const char *name,
> -   const char *dbgname);
> -void etnaviv_writel(u32 data, void __iomem *addr);
> -u32 etnaviv_readl(const void __iomem *addr);
> -
>  #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
>  #define VERB(fmt, ...) if (0) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 8a88799bf79b..08c587547f19 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1735,6 +1735,7 @@ static int etnaviv_gpu_platform_probe(struct 
> platform_device *pdev)
>  {
> struct device *dev = >dev;
> struct etnaviv_gpu *gpu;
> +   struct resource *res;
> int err;
>
> gpu = devm_kzalloc(dev, sizeof(*gpu), GFP_KERNEL);
> @@ -1746,7 +1747,8 @@ static int etnaviv_gpu_platform_probe(struct 
> platform_device *pdev)
> mutex_init(>fence_idr_lock);
>
> /* Map registers: */
> -   gpu->mmio = etnaviv_ioremap(pdev, NULL, dev_name(gpu->dev));
> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +   gpu->mmio = devm_ioremap_resource(>dev, res);
> if (IS_ERR(gpu->mmio))
> return 

[PATCH] drm/etnaviv: remove register logging

[Lucas Stach]

I'm not aware of any case where tracing GPU register manipulation at the
kernel level would have been useful. It only adds more indirections and
adds to the code size.

Signed-off-by: Lucas Stach 
---
 drivers/gpu/drm/etnaviv/Kconfig   |  8 --
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 51 ---
 drivers/gpu/drm/etnaviv/etnaviv_drv.h |  5 
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  4 ++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  4 +--
 5 files changed, 5 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
index e5bfeca361bd..041a77e400d4 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -22,11 +22,3 @@ config DRM_ETNAVIV_THERMAL
help
  Compile in support for thermal throttling.
  Say Y unless you want to risk burning your SoC.
-
-config DRM_ETNAVIV_REGISTER_LOGGING
-   bool "enable ETNAVIV register logging"
-   depends on DRM_ETNAVIV
-   help
- Compile in support for logging register reads/writes in a format
- that can be parsed by envytools demsm tool.  If enabled, register
- logging can be switched on via etnaviv.reglog=y module param.
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c 
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index ab50090d066c..0aa543d75953 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -25,57 +25,6 @@
 #include "etnaviv_mmu.h"
 #include "etnaviv_perfmon.h"
 
-#ifdef CONFIG_DRM_ETNAVIV_REGISTER_LOGGING
-static bool reglog;
-MODULE_PARM_DESC(reglog, "Enable register read/write logging");
-module_param(reglog, bool, 0600);
-#else
-#define reglog 0
-#endif
-
-void __iomem *etnaviv_ioremap(struct platform_device *pdev, const char *name,
-   const char *dbgname)
-{
-   struct resource *res;
-   void __iomem *ptr;
-
-   if (name)
-   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
-   else
-   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-   ptr = devm_ioremap_resource(>dev, res);
-   if (IS_ERR(ptr)) {
-   dev_err(>dev, "failed to ioremap %s: %ld\n", name,
-   PTR_ERR(ptr));
-   return ptr;
-   }
-
-   if (reglog)
-   dev_printk(KERN_DEBUG, >dev, "IO:region %s 0x%p %08zx\n",
-  dbgname, ptr, (size_t)resource_size(res));
-
-   return ptr;
-}
-
-void etnaviv_writel(u32 data, void __iomem *addr)
-{
-   if (reglog)
-   printk(KERN_DEBUG "IO:W %p %08x\n", addr, data);
-
-   writel(data, addr);
-}
-
-u32 etnaviv_readl(const void __iomem *addr)
-{
-   u32 val = readl(addr);
-
-   if (reglog)
-   printk(KERN_DEBUG "IO:R %p %08x\n", addr, val);
-
-   return val;
-}
-
 /*
  * DRM operations:
  */
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h 
b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index ddb17ee565e9..56be51c13c49 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -101,11 +101,6 @@ void etnaviv_gem_describe_objects(struct 
etnaviv_drm_private *priv,
struct seq_file *m);
 #endif
 
-void __iomem *etnaviv_ioremap(struct platform_device *pdev, const char *name,
-   const char *dbgname);
-void etnaviv_writel(u32 data, void __iomem *addr);
-u32 etnaviv_readl(const void __iomem *addr);
-
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 #define VERB(fmt, ...) if (0) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c 
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 8a88799bf79b..08c587547f19 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1735,6 +1735,7 @@ static int etnaviv_gpu_platform_probe(struct 
platform_device *pdev)
 {
struct device *dev = >dev;
struct etnaviv_gpu *gpu;
+   struct resource *res;
int err;
 
gpu = devm_kzalloc(dev, sizeof(*gpu), GFP_KERNEL);
@@ -1746,7 +1747,8 @@ static int etnaviv_gpu_platform_probe(struct 
platform_device *pdev)
mutex_init(>fence_idr_lock);
 
/* Map registers: */
-   gpu->mmio = etnaviv_ioremap(pdev, NULL, dev_name(gpu->dev));
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   gpu->mmio = devm_ioremap_resource(>dev, res);
if (IS_ERR(gpu->mmio))
return PTR_ERR(gpu->mmio);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h 
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 3c3005501846..6052093d00b2 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -161,12 +161,12 @@ struct etnaviv_gpu {
 
 static inline void gpu_write(struct etnaviv_gpu *gpu, u32 reg, u32 data)
 {
-   etnaviv_writel(data, gpu->mmio + reg);
+   writel(data, gpu->mmio + reg);
 }
 
 static inline u32