On Thu, Jul 6, 2023 at 4:36 PM Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> 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);
