diff mbox series

[10/12] drm/msm/adreno: Add helper for formating chip-id

Message ID 20230706211045.204925-11-robdclark@gmail.com (mailing list archive)
State Superseded
Headers show
Series drm/msm/adreno: Move away from legacy revision matching | expand

Commit Message

Rob Clark July 6, 2023, 9:10 p.m. UTC 
From: Rob Clark <robdclark@chromium.org>

This is used in a few places, including one that is parsed by userspace
tools.  So let's standardize it a bit better.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c |  8 +++-----
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 19 ++++++++-----------
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  6 ++++++
 3 files changed, 17 insertions(+), 16 deletions(-)

Comments

Konrad Dybcio July 6, 2023, 11:36 p.m. UTC | #1 
On 6.07.2023 23:10, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> This is used in a few places, including one that is parsed by userspace
> tools.  So let's standardize it a bit better.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
Userspace parsed this weird string instead of the hex-based chipid?

weird^2

Konrad
>  drivers/gpu/drm/msm/adreno/adreno_device.c |  8 +++-----
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 19 ++++++++-----------
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  6 ++++++
>  3 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index dcd6363ac7b0..fd2e183bce60 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -661,14 +661,12 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>  	info = adreno_info(config.rev);
>  
>  	if (!info) {
> -		dev_warn(drm->dev, "Unknown GPU revision: %u.%u.%u.%u\n",
> -			config.rev.core, config.rev.major,
> -			config.rev.minor, config.rev.patchid);
> +		dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> +			ADRENO_CHIPID_ARGS(config.rev));
>  		return -ENXIO;
>  	}
>  
> -	DBG("Found GPU: %u.%u.%u.%u", config.rev.core, config.rev.major,
> -		config.rev.minor, config.rev.patchid);
> +	DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
>  
>  	priv->is_a2xx = info->family < ADRENO_3XX;
>  	priv->has_cached_coherent =
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 75ff7fb46099..1a982a926f21 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -847,10 +847,9 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
>  	if (IS_ERR_OR_NULL(state))
>  		return;
>  
> -	drm_printf(p, "revision: %d (%d.%d.%d.%d)\n",
> -			adreno_gpu->info->revn, adreno_gpu->rev.core,
> -			adreno_gpu->rev.major, adreno_gpu->rev.minor,
> -			adreno_gpu->rev.patchid);
> +	drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> +			adreno_gpu->info->revn,
> +			ADRENO_CHIPID_ARGS(adreno_gpu->rev));
>  	/*
>  	 * If this is state collected due to iova fault, so fault related info
>  	 *
> @@ -921,10 +920,9 @@ void adreno_dump_info(struct msm_gpu *gpu)
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>  	int i;
>  
> -	printk("revision: %d (%d.%d.%d.%d)\n",
> -			adreno_gpu->info->revn, adreno_gpu->rev.core,
> -			adreno_gpu->rev.major, adreno_gpu->rev.minor,
> -			adreno_gpu->rev.patchid);
> +	printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> +			adreno_gpu->info->revn,
> +			ADRENO_CHIPID_ARGS(adreno_gpu->rev));
>  
>  	for (i = 0; i < gpu->nr_rings; i++) {
>  		struct msm_ringbuffer *ring = gpu->rb[i];
> @@ -1105,9 +1103,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		speedbin = 0xffff;
>  	adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
>  
> -	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%d.%d.%d.%d",
> -			rev->core, rev->major, rev->minor,
> -			rev->patchid);
> +	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> +			ADRENO_CHIPID_ARGS(config->rev));
>  	if (!gpu_name)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 2fa14dcd4e40..73e7155f164c 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -66,6 +66,12 @@ struct adreno_rev {
>  #define ADRENO_REV(core, major, minor, patchid) \
>  	((struct adreno_rev){ core, major, minor, patchid })
>  
> +/* Helper for formating the chip_id in the way that userspace tools like
> + * crashdec expect.
> + */
> +#define ADRENO_CHIPID_FMT "u.%u.%u.%u"
> +#define ADRENO_CHIPID_ARGS(_r) (_r).core, (_r).major, (_r).minor, (_r).patchid
> +
>  struct adreno_gpu_funcs {
>  	struct msm_gpu_funcs base;
>  	int (*get_timestamp)(struct msm_gpu *gpu, uint64_t *value);
Dmitry Baryshkov July 7, 2023, 2:50 a.m. UTC | #2 
On 07/07/2023 00:10, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> This is used in a few places, including one that is parsed by userspace
> tools.  So let's standardize it a bit better.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_device.c |  8 +++-----
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 19 ++++++++-----------
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  6 ++++++
>   3 files changed, 17 insertions(+), 16 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Rob Clark July 10, 2023, 8:21 p.m. UTC | #3 
On Thu, Jul 6, 2023 at 4:36 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > This is used in a few places, including one that is parsed by userspace
> > tools.  So let's standardize it a bit better.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> Userspace parsed this weird string instead of the hex-based chipid?
>
> weird^2

AFAICT it is just crashdec (the creatively named tool for parsing gpu
devcore dumps) which parses using "%u.%u.%u.%u"..  I suppose one
_could_ make the argument that, since userspace doesn't yet support
any device where "%x.%x.%x.%x" parsing would be different, we could
get away with switching to hex without it being an ABI break..

BR,
-R

> Konrad
> >  drivers/gpu/drm/msm/adreno/adreno_device.c |  8 +++-----
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 19 ++++++++-----------
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  6 ++++++
> >  3 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index dcd6363ac7b0..fd2e183bce60 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -661,14 +661,12 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >       info = adreno_info(config.rev);
> >
> >       if (!info) {
> > -             dev_warn(drm->dev, "Unknown GPU revision: %u.%u.%u.%u\n",
> > -                     config.rev.core, config.rev.major,
> > -                     config.rev.minor, config.rev.patchid);
> > +             dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> > +                     ADRENO_CHIPID_ARGS(config.rev));
> >               return -ENXIO;
> >       }
> >
> > -     DBG("Found GPU: %u.%u.%u.%u", config.rev.core, config.rev.major,
> > -             config.rev.minor, config.rev.patchid);
> > +     DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> >
> >       priv->is_a2xx = info->family < ADRENO_3XX;
> >       priv->has_cached_coherent =
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 75ff7fb46099..1a982a926f21 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -847,10 +847,9 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> >       if (IS_ERR_OR_NULL(state))
> >               return;
> >
> > -     drm_printf(p, "revision: %d (%d.%d.%d.%d)\n",
> > -                     adreno_gpu->info->revn, adreno_gpu->rev.core,
> > -                     adreno_gpu->rev.major, adreno_gpu->rev.minor,
> > -                     adreno_gpu->rev.patchid);
> > +     drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > +                     adreno_gpu->info->revn,
> > +                     ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> >       /*
> >        * If this is state collected due to iova fault, so fault related info
> >        *
> > @@ -921,10 +920,9 @@ void adreno_dump_info(struct msm_gpu *gpu)
> >       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >       int i;
> >
> > -     printk("revision: %d (%d.%d.%d.%d)\n",
> > -                     adreno_gpu->info->revn, adreno_gpu->rev.core,
> > -                     adreno_gpu->rev.major, adreno_gpu->rev.minor,
> > -                     adreno_gpu->rev.patchid);
> > +     printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > +                     adreno_gpu->info->revn,
> > +                     ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> >
> >       for (i = 0; i < gpu->nr_rings; i++) {
> >               struct msm_ringbuffer *ring = gpu->rb[i];
> > @@ -1105,9 +1103,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> >               speedbin = 0xffff;
> >       adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> >
> > -     gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%d.%d.%d.%d",
> > -                     rev->core, rev->major, rev->minor,
> > -                     rev->patchid);
> > +     gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > +                     ADRENO_CHIPID_ARGS(config->rev));
> >       if (!gpu_name)
> >               return -ENOMEM;
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 2fa14dcd4e40..73e7155f164c 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -66,6 +66,12 @@ struct adreno_rev {
> >  #define ADRENO_REV(core, major, minor, patchid) \
> >       ((struct adreno_rev){ core, major, minor, patchid })
> >
> > +/* Helper for formating the chip_id in the way that userspace tools like
> > + * crashdec expect.
> > + */
> > +#define ADRENO_CHIPID_FMT "u.%u.%u.%u"
> > +#define ADRENO_CHIPID_ARGS(_r) (_r).core, (_r).major, (_r).minor, (_r).patchid
> > +
> >  struct adreno_gpu_funcs {
> >       struct msm_gpu_funcs base;
> >       int (*get_timestamp)(struct msm_gpu *gpu, uint64_t *value);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index dcd6363ac7b0..fd2e183bce60 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -661,14 +661,12 @@  static int adreno_bind(struct device *dev, struct device *master, void *data)
 	info = adreno_info(config.rev);
 
 	if (!info) {
-		dev_warn(drm->dev, "Unknown GPU revision: %u.%u.%u.%u\n",
-			config.rev.core, config.rev.major,
-			config.rev.minor, config.rev.patchid);
+		dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
+			ADRENO_CHIPID_ARGS(config.rev));
 		return -ENXIO;
 	}
 
-	DBG("Found GPU: %u.%u.%u.%u", config.rev.core, config.rev.major,
-		config.rev.minor, config.rev.patchid);
+	DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
 
 	priv->is_a2xx = info->family < ADRENO_3XX;
 	priv->has_cached_coherent =
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 75ff7fb46099..1a982a926f21 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -847,10 +847,9 @@  void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
 	if (IS_ERR_OR_NULL(state))
 		return;
 
-	drm_printf(p, "revision: %d (%d.%d.%d.%d)\n",
-			adreno_gpu->info->revn, adreno_gpu->rev.core,
-			adreno_gpu->rev.major, adreno_gpu->rev.minor,
-			adreno_gpu->rev.patchid);
+	drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
+			adreno_gpu->info->revn,
+			ADRENO_CHIPID_ARGS(adreno_gpu->rev));
 	/*
 	 * If this is state collected due to iova fault, so fault related info
 	 *
@@ -921,10 +920,9 @@  void adreno_dump_info(struct msm_gpu *gpu)
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	int i;
 
-	printk("revision: %d (%d.%d.%d.%d)\n",
-			adreno_gpu->info->revn, adreno_gpu->rev.core,
-			adreno_gpu->rev.major, adreno_gpu->rev.minor,
-			adreno_gpu->rev.patchid);
+	printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
+			adreno_gpu->info->revn,
+			ADRENO_CHIPID_ARGS(adreno_gpu->rev));
 
 	for (i = 0; i < gpu->nr_rings; i++) {
 		struct msm_ringbuffer *ring = gpu->rb[i];
@@ -1105,9 +1103,8 @@  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		speedbin = 0xffff;
 	adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
 
-	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%d.%d.%d.%d",
-			rev->core, rev->major, rev->minor,
-			rev->patchid);
+	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
+			ADRENO_CHIPID_ARGS(config->rev));
 	if (!gpu_name)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 2fa14dcd4e40..73e7155f164c 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -66,6 +66,12 @@  struct adreno_rev {
 #define ADRENO_REV(core, major, minor, patchid) \
 	((struct adreno_rev){ core, major, minor, patchid })
 
+/* Helper for formating the chip_id in the way that userspace tools like
+ * crashdec expect.
+ */
+#define ADRENO_CHIPID_FMT "u.%u.%u.%u"
+#define ADRENO_CHIPID_ARGS(_r) (_r).core, (_r).major, (_r).minor, (_r).patchid
+
 struct adreno_gpu_funcs {
 	struct msm_gpu_funcs base;
 	int (*get_timestamp)(struct msm_gpu *gpu, uint64_t *value);