[12/12] drm/msm/adreno: Switch to chip-id for identifying GPU

From: Rob Clark <robdclark@chromium.org>

Since the revision becomes an opaque identifier with future GPUs, move
away from treating different ranges of bits as having a given meaning.
This means that we need to explicitly list different patch revisions in
the device table.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
 drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
 8 files changed, 122 insertions(+), 110 deletions(-)

On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote:
> 
> On 07/07/2023 00:10, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> > 
> > Since the revision becomes an opaque identifier with future GPUs, move
> > away from treating different ranges of bits as having a given meaning.
> > This means that we need to explicitly list different patch revisions in
> > the device table.
> > 
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
> >   drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
> >   drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
> >   drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
> >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
> >   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
> >   8 files changed, 122 insertions(+), 110 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > index 715436cb3996..8b4cdf95f445 100644
> > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
> >   	gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x00220000);
> >   	/* Early A430's have a timing issue with SP/TP power collapse;
> >   	   disabling HW clock gating prevents it. */
> > -	if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
> > +	if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
> >   		gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
> >   	else
> >   		gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0xAAAAAAAA);
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > index f0803e94ebe5..70d2b5342cd9 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> >   	struct msm_drm_private *priv = dev->dev_private;
> >   	struct platform_device *pdev = priv->gpu_pdev;
> >   	struct adreno_platform_config *config = pdev->dev.platform_data;
> > +	const struct adreno_info *info;
> >   	struct a5xx_gpu *a5xx_gpu = NULL;
> >   	struct adreno_gpu *adreno_gpu;
> >   	struct msm_gpu *gpu;
> > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> >   	nr_rings = 4;
> > -	if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> > +	/*
> > +	 * Note that we wouldn't have been able to get this far if there is not
> > +	 * a device table entry for this chip_id
> > +	 */
> > +	info = adreno_find_info(config->chip_id);
> > +	if (WARN_ON(!info))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (info->revn == 510)
> >   		nr_rings = 1;
> >   	ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > index 0e63a1429189..7705f8010484 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
> >   	/* The battery current limiter isn't enabled for A540 */
> >   	config = AGC_LM_CONFIG_BCL_DISABLED;
> > -	config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > +	config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> >   	/* For now disable GPMU side throttling */
> >   	config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index f1bb20574018..a9ba547a120c 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
> >   	gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
> >   		(1 << 31) | (0xa << 18) | (0xa0));
> > -	chipid = adreno_gpu->rev.core << 24;
> > -	chipid |= adreno_gpu->rev.major << 16;
> > -	chipid |= adreno_gpu->rev.minor << 12;
> > -	chipid |= adreno_gpu->rev.patchid << 8;
> > +	/* Note that the GMU has a slightly different layout for
> > +	 * chip_id, for whatever reason, so a bit of massaging
> > +	 * is needed.  The upper 16b are the same, but minor and
> > +	 * patchid are packed in four bits each with the lower
> > +	 * 8b unused:
> > +	 */
> > +	chipid  = adreno_gpu->chip_id & 0xffff0000;
> > +	chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
> > +	chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
> 
> I'd beg for explicit FIELD_GET and FIELD_PREP here.
> 
> >   	gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index 77b23c004b94..ed075729ca09 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> >   	/*
> >   	 * We need to know the platform type before calling into adreno_gpu_init
> >   	 * so that the hw_apriv flag can be correctly set. Snoop into the info
> > -	 * and grab the revision number
> > +	 * and grab the revision number.
> > +	 *
> > +	 * Note that we wouldn't have been able to get this far if there is not
> > +	 * a device table entry for this chip_id
> 
> Having seen this note twice, shouldn't we explicitly pass adreno_info to our
> aNxx_gpu_init() functions and then further to adreno_gpu_init()?
> 
> >   	 */
> > -	info = adreno_info(config->rev);
> > -	if (!info)
> > +	info = adreno_find_info(config->chip_id);
> > +	if (WARN_ON(!info))
> >   		return ERR_PTR(-EINVAL);
> >   	adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index fd2e183bce60..4a718ff33635 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -22,7 +22,7 @@ module_param_named(allow_vram_carveout, allow_vram_carveout, bool, 0600);
> >   static const struct adreno_info gpulist[] = {
> >   	{
> > -		.rev   = ADRENO_REV(2, 0, 0, 0),
> > +		.chip_ids = ADRENO_CHIP_IDS(0x02000000),
> 
> This begs to use bit masks, but see below
> 
> >   		.family = ADRENO_2XX_GEN1,
> >   		.revn  = 200,
> >   		.fw = {
> 
> [skipped]
> 
> > @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
> >   		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >   		.init  = a3xx_gpu_init,
> >   	}, {
> > -		.rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
> > +		.chip_ids = ADRENO_CHIP_IDS(
> > +			0x03020000,
> > +			0x03020001,
> > +			0x03020002
> 
> definitely a bitmask would help
> 
> > +		),
> >   		.family = ADRENO_3XX,
> >   		.revn  = 320,
> >   		.fw = {
> 
> [skipped the rest]
> 
> > @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
> >   MODULE_FIRMWARE("qcom/a630_gmu.bin");
> >   MODULE_FIRMWARE("qcom/a630_zap.mbn");
> > -static inline bool _rev_match(uint8_t entry, uint8_t id)
> > -{
> > -	return (entry == ANY_ID) || (entry == id);
> > -}
> > -
> > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2)
> > -{
> > -
> > -	return _rev_match(rev1.core, rev2.core) &&
> > -		_rev_match(rev1.major, rev2.major) &&
> > -		_rev_match(rev1.minor, rev2.minor) &&
> > -		_rev_match(rev1.patchid, rev2.patchid);
> > -}
> > -
> > -const struct adreno_info *adreno_info(struct adreno_rev rev)
> > +const struct adreno_info *adreno_find_info(uint32_t chip_id)
> >   {
> > -	int i;
> > -
> >   	/* identify gpu: */
> > -	for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > +	for (int i = 0; i < ARRAY_SIZE(gpulist); i++) {
> >   		const struct adreno_info *info = &gpulist[i];
> >   		if (info->machine && !of_machine_is_compatible(info->machine))
> >   			continue;
> > -		if (adreno_cmp_rev(info->rev, rev))
> > -			return info;
> > +		for (int j = 0; info->chip_ids[j]; j++)
> > +			if (info->chip_ids[j] == chip_id)
> > +				return info;
> >   	}
> >   	return NULL;
> > @@ -598,12 +591,11 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
> >   	return NULL;
> >   }
> > -static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > +static int find_chipid(struct device *dev, uint32_t *chipid)
> >   {
> >   	struct device_node *node = dev->of_node;
> >   	const char *compat;
> >   	int ret;
> > -	u32 chipid;
> >   	/* first search the compat strings for qcom,adreno-XYZ.W: */
> >   	ret = of_property_read_string_index(node, "compatible", 0, &compat);
> > @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
> >   		if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
> >   		    sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
> > -			rev->core = r / 100;
> > +			uint32_t core, major, minor;
> > +
> > +			core = r / 100;
> >   			r %= 100;
> > -			rev->major = r / 10;
> > +			major = r / 10;
> >   			r %= 10;
> > -			rev->minor = r;
> > -			rev->patchid = patch;
> > +			minor = r;
> > +
> > +			*chipid = (core << 24) |
> > +				(major << 16) |
> > +				(minor << 8) |
> > +				patch;
> 
> This starts to look realy crazy. I'd repeat my suggestion of moving towards
> of_device match data. This would result in some duplication, we'd have to
> explicitly list all supported compatibles in the of_match_table. But then we
> can drop all the CHIPID lists from device table and/or manual parsing of the
> chipid from the compat field.
> 
> This way we can have per-SoC overrides, etc.
> 
> >   			return 0;
> >   		}
> > +
> > +		if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
> > +			return 0;
> >   	}
> >   	/* and if that fails, fall back to legacy "qcom,chipid" property: */
> > -	ret = of_property_read_u32(node, "qcom,chipid", &chipid);
> > +	ret = of_property_read_u32(node, "qcom,chipid", chipid);
> >   	if (ret) {
> >   		DRM_DEV_ERROR(dev, "could not parse qcom,chipid: %d\n", ret);
> >   		return ret;
> >   	}
> > -	rev->core = (chipid >> 24) & 0xff;
> > -	rev->major = (chipid >> 16) & 0xff;
> > -	rev->minor = (chipid >> 8) & 0xff;
> > -	rev->patchid = (chipid & 0xff);
> > -
> >   	dev_warn(dev, "Using legacy qcom,chipid binding!\n");
> > -	dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
> > -		rev->core, rev->major, rev->minor, rev->patchid);
> >   	return 0;
> >   }
> > @@ -651,22 +645,22 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >   	struct msm_gpu *gpu;
> >   	int ret;
> > -	ret = find_chipid(dev, &config.rev);
> > +	ret = find_chipid(dev, &config.chip_id);
> >   	if (ret)
> >   		return ret;
> >   	dev->platform_data = &config;
> >   	priv->gpu_pdev = to_platform_device(dev);
> > -	info = adreno_info(config.rev);
> > +	info = adreno_find_info(config.chip_id);
> >   	if (!info) {
> >   		dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> > -			ADRENO_CHIPID_ARGS(config.rev));
> > +			ADRENO_CHIPID_ARGS(config.chip_id));
> >   		return -ENXIO;
> >   	}
> > -	DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> > +	DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.chip_id));
> >   	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 1a982a926f21..1274609a74b1 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -326,10 +326,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> >   		*value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;
> >   		return 0;
> >   	case MSM_PARAM_CHIP_ID:
> > -		*value =  (uint64_t)adreno_gpu->rev.patchid |
> > -			 ((uint64_t)adreno_gpu->rev.minor << 8) |
> > -			 ((uint64_t)adreno_gpu->rev.major << 16) |
> > -			 ((uint64_t)adreno_gpu->rev.core  << 24);
> > +		*value = adreno_gpu->chip_id;
> >   		if (!adreno_gpu->info->revn)
> >   			*value |= ((uint64_t) adreno_gpu->speedbin) << 32;
> >   		return 0;
> > @@ -849,7 +846,7 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> >   	drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> >   			adreno_gpu->info->revn,
> > -			ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > +			ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> >   	/*
> >   	 * If this is state collected due to iova fault, so fault related info
> >   	 *
> > @@ -922,7 +919,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
> >   	printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> >   			adreno_gpu->info->revn,
> > -			ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > +			ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> >   	for (i = 0; i < gpu->nr_rings; i++) {
> >   		struct msm_ringbuffer *ring = gpu->rb[i];
> > @@ -1072,14 +1069,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> >   	struct adreno_platform_config *config = dev->platform_data;
> >   	struct msm_gpu_config adreno_gpu_config  = { 0 };
> >   	struct msm_gpu *gpu = &adreno_gpu->base;
> > -	struct adreno_rev *rev = &config->rev;
> >   	const char *gpu_name;
> >   	u32 speedbin;
> >   	int ret;
> >   	adreno_gpu->funcs = funcs;
> > -	adreno_gpu->info = adreno_info(config->rev);
> > -	adreno_gpu->rev = *rev;
> > +	adreno_gpu->info = adreno_find_info(config->chip_id);
> > +	adreno_gpu->chip_id = config->chip_id;
> >   	/* Only handle the core clock when GMU is not in use (or is absent). */
> >   	if (adreno_has_gmu_wrapper(adreno_gpu) ||
> > @@ -1104,7 +1100,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> >   	adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> >   	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > -			ADRENO_CHIPID_ARGS(config->rev));
> > +			ADRENO_CHIPID_ARGS(config->chip_id));
> >   	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 73e7155f164c..18f53c7ab589 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -54,23 +54,15 @@ enum adreno_family {
> >   #define ADRENO_QUIRK_HAS_HW_APRIV		BIT(3)
> >   #define ADRENO_QUIRK_HAS_CACHED_COHERENT	BIT(4)
> > -struct adreno_rev {
> > -	uint8_t  core;
> > -	uint8_t  major;
> > -	uint8_t  minor;
> > -	uint8_t  patchid;
> > -};
> > -
> > -#define ANY_ID 0xff
> > -
> > -#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
> > +#define ADRENO_CHIPID_ARGS(_c) \
> > +	(((_c) >> 24) & 0xff), \
> > +	(((_c) >> 16) & 0xff), \
> > +	(((_c) >> 8)  & 0xff), \
> > +	((_c) & 0xff)
> 
> So, we still have some meaning for chipid?
> 
> >   struct adreno_gpu_funcs {
> >   	struct msm_gpu_funcs base;
> > @@ -87,7 +79,12 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> >   struct adreno_info {
> >   	const char *machine;
> > -	struct adreno_rev rev;
> > +	/**
> > +	 * @chipids: Table of matching chip-ids
> > +	 *
> > +	 * Terminated with 0 sentinal
> > +	 */
> > +	uint32_t *chip_ids;
> >   	enum adreno_family family;
> >   	uint32_t revn;
> >   	const char *fw[ADRENO_FW_MAX];
> > @@ -107,6 +104,8 @@ struct adreno_info {
> >   	uint32_t *speedbins;
> >   };
> > +#define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
> > +
> >   /*
> >    * Helper to build a speedbin table, ie. the table:
> >    *      fuse | speedbin
> > @@ -125,12 +124,12 @@ struct adreno_info {
> >    */
> >   #define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
> > -const struct adreno_info *adreno_info(struct adreno_rev rev);
> > +const struct adreno_info *adreno_find_info(uint32_t chip_id);
> >   struct adreno_gpu {
> >   	struct msm_gpu base;
> > -	struct adreno_rev rev;
> >   	const struct adreno_info *info;
> > +	uint32_t chip_id;
> >   	uint16_t speedbin;
> >   	const struct adreno_gpu_funcs *funcs;
> > @@ -179,7 +178,7 @@ struct adreno_ocmem {
> >   /* platform config data (ie. from DT, or pdata) */
> >   struct adreno_platform_config {
> > -	struct adreno_rev rev;
> > +	uint32_t chip_id;
> >   };
> >   #define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
> > @@ -196,7 +195,15 @@ struct adreno_platform_config {
> >   	__ret;                                             \
> >   })
> > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
> > +static inline uint8_t adreno_patchid(const struct adreno_gpu *gpu)
> > +{
> > +	/* It is probably ok to assume legacy "adreno_rev" format
> > +	 * for all a6xx devices, but probably best to limit this
> > +	 * to older things.
> > +	 */
> > +	WARN_ON_ONCE(gpu->info->family >= ADRENO_6XX_GEN1);
> > +	return gpu->chip_id & 0xff;
> > +}
> >   static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
> >   {
> > @@ -252,7 +259,7 @@ static inline bool adreno_is_a330(const struct adreno_gpu *gpu)
> >   static inline bool adreno_is_a330v2(const struct adreno_gpu *gpu)
> >   {
> > -	return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
> > +	return adreno_is_a330(gpu) && (adreno_patchid(gpu) > 0);
> >   }
> >   static inline int adreno_is_a405(const struct adreno_gpu *gpu)
> > @@ -342,8 +349,7 @@ static inline int adreno_is_a650(const struct adreno_gpu *gpu)
> >   static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
> >   {
> > -	/* The order of args is important here to handle ANY_ID correctly */
> > -	return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> > +	return gpu->info->chip_ids[0] == 0x06030500;

Why not use adreno_gpu->chip_id here? Or do we need this check before
adreno_gpu_init()? I thought we got rid of those.

> 
> Ugh. The thing that bugs me isn't even the manual comparison of chip_ids[0],
> but the fact that these two platforms stand aside. I think at the end we
> should have a single mechanism for checking GPU's SKUs.
> 
> Or maybe we should get rid of that completely.
> 
> If we get rid of a single gpulist table and move individual GPU info
> definitions to aNxx_gpu.c, we can subclass a generic info structure with
> generation-specific data, for example generation-specific quirks, pointers
> to hcwg or VBIF registers, etc. And then instead of having adreno_is_foo all
> over the code we can migrate most of these checks to data in the gpu info
> data.

I think the downstream driver does what you described here. It helps to
move more generation-specific data to the table without inflating the
gpu table size.

-Akhil

> 
> >   }
> >   static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> > @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
> >   static inline int adreno_is_a690(const struct adreno_gpu *gpu)
> >   {
> > -	/* The order of args is important here to handle ANY_ID correctly */
> > -	return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> > +	return gpu->info->chip_ids[0] == 0x06090000;
> >   };
> >   /* check for a615, a616, a618, a619 or any a630 derivatives */
> >   static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)
> 
> -- 
> With best wishes
> Dmitry
>

On Thu, Jul 13, 2023 at 2:39 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote:
> >
> > On 07/07/2023 00:10, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Since the revision becomes an opaque identifier with future GPUs, move
> > > away from treating different ranges of bits as having a given meaning.
> > > This means that we need to explicitly list different patch revisions in
> > > the device table.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >   drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
> > >   drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
> > >   drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
> > >   drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
> > >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
> > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
> > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
> > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
> > >   8 files changed, 122 insertions(+), 110 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > index 715436cb3996..8b4cdf95f445 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
> > >     gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x00220000);
> > >     /* Early A430's have a timing issue with SP/TP power collapse;
> > >        disabling HW clock gating prevents it. */
> > > -   if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
> > > +   if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
> > >             gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
> > >     else
> > >             gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0xAAAAAAAA);
> > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > index f0803e94ebe5..70d2b5342cd9 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> > >     struct msm_drm_private *priv = dev->dev_private;
> > >     struct platform_device *pdev = priv->gpu_pdev;
> > >     struct adreno_platform_config *config = pdev->dev.platform_data;
> > > +   const struct adreno_info *info;
> > >     struct a5xx_gpu *a5xx_gpu = NULL;
> > >     struct adreno_gpu *adreno_gpu;
> > >     struct msm_gpu *gpu;
> > > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> > >     nr_rings = 4;
> > > -   if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> > > +   /*
> > > +    * Note that we wouldn't have been able to get this far if there is not
> > > +    * a device table entry for this chip_id
> > > +    */
> > > +   info = adreno_find_info(config->chip_id);
> > > +   if (WARN_ON(!info))
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   if (info->revn == 510)
> > >             nr_rings = 1;
> > >     ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
> > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > index 0e63a1429189..7705f8010484 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
> > >     /* The battery current limiter isn't enabled for A540 */
> > >     config = AGC_LM_CONFIG_BCL_DISABLED;
> > > -   config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > > +   config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > >     /* For now disable GPMU side throttling */
> > >     config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
> > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > index f1bb20574018..a9ba547a120c 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
> > >     gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
> > >             (1 << 31) | (0xa << 18) | (0xa0));
> > > -   chipid = adreno_gpu->rev.core << 24;
> > > -   chipid |= adreno_gpu->rev.major << 16;
> > > -   chipid |= adreno_gpu->rev.minor << 12;
> > > -   chipid |= adreno_gpu->rev.patchid << 8;
> > > +   /* Note that the GMU has a slightly different layout for
> > > +    * chip_id, for whatever reason, so a bit of massaging
> > > +    * is needed.  The upper 16b are the same, but minor and
> > > +    * patchid are packed in four bits each with the lower
> > > +    * 8b unused:
> > > +    */
> > > +   chipid  = adreno_gpu->chip_id & 0xffff0000;
> > > +   chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
> > > +   chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
> >
> > I'd beg for explicit FIELD_GET and FIELD_PREP here.
> >
> > >     gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);
> > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > index 77b23c004b94..ed075729ca09 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > @@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> > >     /*
> > >      * We need to know the platform type before calling into adreno_gpu_init
> > >      * so that the hw_apriv flag can be correctly set. Snoop into the info
> > > -    * and grab the revision number
> > > +    * and grab the revision number.
> > > +    *
> > > +    * Note that we wouldn't have been able to get this far if there is not
> > > +    * a device table entry for this chip_id
> >
> > Having seen this note twice, shouldn't we explicitly pass adreno_info to our
> > aNxx_gpu_init() functions and then further to adreno_gpu_init()?
> >
> > >      */
> > > -   info = adreno_info(config->rev);
> > > -   if (!info)
> > > +   info = adreno_find_info(config->chip_id);
> > > +   if (WARN_ON(!info))
> > >             return ERR_PTR(-EINVAL);
> > >     adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > index fd2e183bce60..4a718ff33635 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > @@ -22,7 +22,7 @@ module_param_named(allow_vram_carveout, allow_vram_carveout, bool, 0600);
> > >   static const struct adreno_info gpulist[] = {
> > >     {
> > > -           .rev   = ADRENO_REV(2, 0, 0, 0),
> > > +           .chip_ids = ADRENO_CHIP_IDS(0x02000000),
> >
> > This begs to use bit masks, but see below
> >
> > >             .family = ADRENO_2XX_GEN1,
> > >             .revn  = 200,
> > >             .fw = {
> >
> > [skipped]
> >
> > > @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
> > >             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >             .init  = a3xx_gpu_init,
> > >     }, {
> > > -           .rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
> > > +           .chip_ids = ADRENO_CHIP_IDS(
> > > +                   0x03020000,
> > > +                   0x03020001,
> > > +                   0x03020002
> >
> > definitely a bitmask would help
> >
> > > +           ),
> > >             .family = ADRENO_3XX,
> > >             .revn  = 320,
> > >             .fw = {
> >
> > [skipped the rest]
> >
> > > @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
> > >   MODULE_FIRMWARE("qcom/a630_gmu.bin");
> > >   MODULE_FIRMWARE("qcom/a630_zap.mbn");
> > > -static inline bool _rev_match(uint8_t entry, uint8_t id)
> > > -{
> > > -   return (entry == ANY_ID) || (entry == id);
> > > -}
> > > -
> > > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2)
> > > -{
> > > -
> > > -   return _rev_match(rev1.core, rev2.core) &&
> > > -           _rev_match(rev1.major, rev2.major) &&
> > > -           _rev_match(rev1.minor, rev2.minor) &&
> > > -           _rev_match(rev1.patchid, rev2.patchid);
> > > -}
> > > -
> > > -const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > +const struct adreno_info *adreno_find_info(uint32_t chip_id)
> > >   {
> > > -   int i;
> > > -
> > >     /* identify gpu: */
> > > -   for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > +   for (int i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > >             const struct adreno_info *info = &gpulist[i];
> > >             if (info->machine && !of_machine_is_compatible(info->machine))
> > >                     continue;
> > > -           if (adreno_cmp_rev(info->rev, rev))
> > > -                   return info;
> > > +           for (int j = 0; info->chip_ids[j]; j++)
> > > +                   if (info->chip_ids[j] == chip_id)
> > > +                           return info;
> > >     }
> > >     return NULL;
> > > @@ -598,12 +591,11 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
> > >     return NULL;
> > >   }
> > > -static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > > +static int find_chipid(struct device *dev, uint32_t *chipid)
> > >   {
> > >     struct device_node *node = dev->of_node;
> > >     const char *compat;
> > >     int ret;
> > > -   u32 chipid;
> > >     /* first search the compat strings for qcom,adreno-XYZ.W: */
> > >     ret = of_property_read_string_index(node, "compatible", 0, &compat);
> > > @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > >             if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
> > >                 sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
> > > -                   rev->core = r / 100;
> > > +                   uint32_t core, major, minor;
> > > +
> > > +                   core = r / 100;
> > >                     r %= 100;
> > > -                   rev->major = r / 10;
> > > +                   major = r / 10;
> > >                     r %= 10;
> > > -                   rev->minor = r;
> > > -                   rev->patchid = patch;
> > > +                   minor = r;
> > > +
> > > +                   *chipid = (core << 24) |
> > > +                           (major << 16) |
> > > +                           (minor << 8) |
> > > +                           patch;
> >
> > This starts to look realy crazy. I'd repeat my suggestion of moving towards
> > of_device match data. This would result in some duplication, we'd have to
> > explicitly list all supported compatibles in the of_match_table. But then we
> > can drop all the CHIPID lists from device table and/or manual parsing of the
> > chipid from the compat field.
> >
> > This way we can have per-SoC overrides, etc.
> >
> > >                     return 0;
> > >             }
> > > +
> > > +           if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
> > > +                   return 0;
> > >     }
> > >     /* and if that fails, fall back to legacy "qcom,chipid" property: */
> > > -   ret = of_property_read_u32(node, "qcom,chipid", &chipid);
> > > +   ret = of_property_read_u32(node, "qcom,chipid", chipid);
> > >     if (ret) {
> > >             DRM_DEV_ERROR(dev, "could not parse qcom,chipid: %d\n", ret);
> > >             return ret;
> > >     }
> > > -   rev->core = (chipid >> 24) & 0xff;
> > > -   rev->major = (chipid >> 16) & 0xff;
> > > -   rev->minor = (chipid >> 8) & 0xff;
> > > -   rev->patchid = (chipid & 0xff);
> > > -
> > >     dev_warn(dev, "Using legacy qcom,chipid binding!\n");
> > > -   dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
> > > -           rev->core, rev->major, rev->minor, rev->patchid);
> > >     return 0;
> > >   }
> > > @@ -651,22 +645,22 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > >     struct msm_gpu *gpu;
> > >     int ret;
> > > -   ret = find_chipid(dev, &config.rev);
> > > +   ret = find_chipid(dev, &config.chip_id);
> > >     if (ret)
> > >             return ret;
> > >     dev->platform_data = &config;
> > >     priv->gpu_pdev = to_platform_device(dev);
> > > -   info = adreno_info(config.rev);
> > > +   info = adreno_find_info(config.chip_id);
> > >     if (!info) {
> > >             dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> > > -                   ADRENO_CHIPID_ARGS(config.rev));
> > > +                   ADRENO_CHIPID_ARGS(config.chip_id));
> > >             return -ENXIO;
> > >     }
> > > -   DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> > > +   DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.chip_id));
> > >     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 1a982a926f21..1274609a74b1 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > @@ -326,10 +326,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > >             *value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;
> > >             return 0;
> > >     case MSM_PARAM_CHIP_ID:
> > > -           *value =  (uint64_t)adreno_gpu->rev.patchid |
> > > -                    ((uint64_t)adreno_gpu->rev.minor << 8) |
> > > -                    ((uint64_t)adreno_gpu->rev.major << 16) |
> > > -                    ((uint64_t)adreno_gpu->rev.core  << 24);
> > > +           *value = adreno_gpu->chip_id;
> > >             if (!adreno_gpu->info->revn)
> > >                     *value |= ((uint64_t) adreno_gpu->speedbin) << 32;
> > >             return 0;
> > > @@ -849,7 +846,7 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> > >     drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > >                     adreno_gpu->info->revn,
> > > -                   ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > > +                   ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> > >     /*
> > >      * If this is state collected due to iova fault, so fault related info
> > >      *
> > > @@ -922,7 +919,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
> > >     printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > >                     adreno_gpu->info->revn,
> > > -                   ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > > +                   ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> > >     for (i = 0; i < gpu->nr_rings; i++) {
> > >             struct msm_ringbuffer *ring = gpu->rb[i];
> > > @@ -1072,14 +1069,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> > >     struct adreno_platform_config *config = dev->platform_data;
> > >     struct msm_gpu_config adreno_gpu_config  = { 0 };
> > >     struct msm_gpu *gpu = &adreno_gpu->base;
> > > -   struct adreno_rev *rev = &config->rev;
> > >     const char *gpu_name;
> > >     u32 speedbin;
> > >     int ret;
> > >     adreno_gpu->funcs = funcs;
> > > -   adreno_gpu->info = adreno_info(config->rev);
> > > -   adreno_gpu->rev = *rev;
> > > +   adreno_gpu->info = adreno_find_info(config->chip_id);
> > > +   adreno_gpu->chip_id = config->chip_id;
> > >     /* Only handle the core clock when GMU is not in use (or is absent). */
> > >     if (adreno_has_gmu_wrapper(adreno_gpu) ||
> > > @@ -1104,7 +1100,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> > >     adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> > >     gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > > -                   ADRENO_CHIPID_ARGS(config->rev));
> > > +                   ADRENO_CHIPID_ARGS(config->chip_id));
> > >     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 73e7155f164c..18f53c7ab589 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > @@ -54,23 +54,15 @@ enum adreno_family {
> > >   #define ADRENO_QUIRK_HAS_HW_APRIV         BIT(3)
> > >   #define ADRENO_QUIRK_HAS_CACHED_COHERENT  BIT(4)
> > > -struct adreno_rev {
> > > -   uint8_t  core;
> > > -   uint8_t  major;
> > > -   uint8_t  minor;
> > > -   uint8_t  patchid;
> > > -};
> > > -
> > > -#define ANY_ID 0xff
> > > -
> > > -#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
> > > +#define ADRENO_CHIPID_ARGS(_c) \
> > > +   (((_c) >> 24) & 0xff), \
> > > +   (((_c) >> 16) & 0xff), \
> > > +   (((_c) >> 8)  & 0xff), \
> > > +   ((_c) & 0xff)
> >
> > So, we still have some meaning for chipid?
> >
> > >   struct adreno_gpu_funcs {
> > >     struct msm_gpu_funcs base;
> > > @@ -87,7 +79,12 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > >   struct adreno_info {
> > >     const char *machine;
> > > -   struct adreno_rev rev;
> > > +   /**
> > > +    * @chipids: Table of matching chip-ids
> > > +    *
> > > +    * Terminated with 0 sentinal
> > > +    */
> > > +   uint32_t *chip_ids;
> > >     enum adreno_family family;
> > >     uint32_t revn;
> > >     const char *fw[ADRENO_FW_MAX];
> > > @@ -107,6 +104,8 @@ struct adreno_info {
> > >     uint32_t *speedbins;
> > >   };
> > > +#define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
> > > +
> > >   /*
> > >    * Helper to build a speedbin table, ie. the table:
> > >    *      fuse | speedbin
> > > @@ -125,12 +124,12 @@ struct adreno_info {
> > >    */
> > >   #define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
> > > -const struct adreno_info *adreno_info(struct adreno_rev rev);
> > > +const struct adreno_info *adreno_find_info(uint32_t chip_id);
> > >   struct adreno_gpu {
> > >     struct msm_gpu base;
> > > -   struct adreno_rev rev;
> > >     const struct adreno_info *info;
> > > +   uint32_t chip_id;
> > >     uint16_t speedbin;
> > >     const struct adreno_gpu_funcs *funcs;
> > > @@ -179,7 +178,7 @@ struct adreno_ocmem {
> > >   /* platform config data (ie. from DT, or pdata) */
> > >   struct adreno_platform_config {
> > > -   struct adreno_rev rev;
> > > +   uint32_t chip_id;
> > >   };
> > >   #define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
> > > @@ -196,7 +195,15 @@ struct adreno_platform_config {
> > >     __ret;                                             \
> > >   })
> > > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
> > > +static inline uint8_t adreno_patchid(const struct adreno_gpu *gpu)
> > > +{
> > > +   /* It is probably ok to assume legacy "adreno_rev" format
> > > +    * for all a6xx devices, but probably best to limit this
> > > +    * to older things.
> > > +    */
> > > +   WARN_ON_ONCE(gpu->info->family >= ADRENO_6XX_GEN1);
> > > +   return gpu->chip_id & 0xff;
> > > +}
> > >   static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
> > >   {
> > > @@ -252,7 +259,7 @@ static inline bool adreno_is_a330(const struct adreno_gpu *gpu)
> > >   static inline bool adreno_is_a330v2(const struct adreno_gpu *gpu)
> > >   {
> > > -   return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
> > > +   return adreno_is_a330(gpu) && (adreno_patchid(gpu) > 0);
> > >   }
> > >   static inline int adreno_is_a405(const struct adreno_gpu *gpu)
> > > @@ -342,8 +349,7 @@ static inline int adreno_is_a650(const struct adreno_gpu *gpu)
> > >   static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
> > >   {
> > > -   /* The order of args is important here to handle ANY_ID correctly */
> > > -   return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> > > +   return gpu->info->chip_ids[0] == 0x06030500;
>
> Why not use adreno_gpu->chip_id here? Or do we need this check before
> adreno_gpu_init()? I thought we got rid of those.

This is intentional, because there can be multiple chip-id's for a
single table entry, in theory.  But the first chip_id in the gpu table
entry should suffice, and is simpler than comparing
adreno_gpu->chip_id against each possible match.

BR,
-R

> >
> > Ugh. The thing that bugs me isn't even the manual comparison of chip_ids[0],
> > but the fact that these two platforms stand aside. I think at the end we
> > should have a single mechanism for checking GPU's SKUs.
> >
> > Or maybe we should get rid of that completely.
> >
> > If we get rid of a single gpulist table and move individual GPU info
> > definitions to aNxx_gpu.c, we can subclass a generic info structure with
> > generation-specific data, for example generation-specific quirks, pointers
> > to hcwg or VBIF registers, etc. And then instead of having adreno_is_foo all
> > over the code we can migrate most of these checks to data in the gpu info
> > data.
>
> I think the downstream driver does what you described here. It helps to
> move more generation-specific data to the table without inflating the
> gpu table size.
>
> -Akhil
>
> >
> > >   }
> > >   static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> > > @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
> > >   static inline int adreno_is_a690(const struct adreno_gpu *gpu)
> > >   {
> > > -   /* The order of args is important here to handle ANY_ID correctly */
> > > -   return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> > > +   return gpu->info->chip_ids[0] == 0x06090000;
> > >   };
> > >   /* check for a615, a616, a618, a619 or any a630 derivatives */
> > >   static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)
> >
> > --
> > With best wishes
> > Dmitry
> >

On Fri, 14 Jul 2023 at 01:06, Rob Clark <robdclark@gmail.com> wrote:
>
> On Thu, Jul 13, 2023 at 2:39 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >
> > On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote:
> > >
> > > On 07/07/2023 00:10, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > Since the revision becomes an opaque identifier with future GPUs, move
> > > > away from treating different ranges of bits as having a given meaning.
> > > > This means that we need to explicitly list different patch revisions in
> > > > the device table.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >   drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
> > > >   drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
> > > >   drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
> > > >   drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
> > > >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
> > > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
> > > >   8 files changed, 122 insertions(+), 110 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > > index 715436cb3996..8b4cdf95f445 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
> > > >     gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x00220000);
> > > >     /* Early A430's have a timing issue with SP/TP power collapse;
> > > >        disabling HW clock gating prevents it. */
> > > > -   if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
> > > > +   if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
> > > >             gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
> > > >     else
> > > >             gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0xAAAAAAAA);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > > index f0803e94ebe5..70d2b5342cd9 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> > > >     struct msm_drm_private *priv = dev->dev_private;
> > > >     struct platform_device *pdev = priv->gpu_pdev;
> > > >     struct adreno_platform_config *config = pdev->dev.platform_data;
> > > > +   const struct adreno_info *info;
> > > >     struct a5xx_gpu *a5xx_gpu = NULL;
> > > >     struct adreno_gpu *adreno_gpu;
> > > >     struct msm_gpu *gpu;
> > > > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> > > >     nr_rings = 4;
> > > > -   if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> > > > +   /*
> > > > +    * Note that we wouldn't have been able to get this far if there is not
> > > > +    * a device table entry for this chip_id
> > > > +    */
> > > > +   info = adreno_find_info(config->chip_id);
> > > > +   if (WARN_ON(!info))
> > > > +           return ERR_PTR(-EINVAL);
> > > > +
> > > > +   if (info->revn == 510)
> > > >             nr_rings = 1;
> > > >     ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > > index 0e63a1429189..7705f8010484 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
> > > >     /* The battery current limiter isn't enabled for A540 */
> > > >     config = AGC_LM_CONFIG_BCL_DISABLED;
> > > > -   config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > > > +   config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > > >     /* For now disable GPMU side throttling */
> > > >     config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > > index f1bb20574018..a9ba547a120c 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
> > > >     gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
> > > >             (1 << 31) | (0xa << 18) | (0xa0));
> > > > -   chipid = adreno_gpu->rev.core << 24;
> > > > -   chipid |= adreno_gpu->rev.major << 16;
> > > > -   chipid |= adreno_gpu->rev.minor << 12;
> > > > -   chipid |= adreno_gpu->rev.patchid << 8;
> > > > +   /* Note that the GMU has a slightly different layout for
> > > > +    * chip_id, for whatever reason, so a bit of massaging
> > > > +    * is needed.  The upper 16b are the same, but minor and
> > > > +    * patchid are packed in four bits each with the lower
> > > > +    * 8b unused:
> > > > +    */
> > > > +   chipid  = adreno_gpu->chip_id & 0xffff0000;
> > > > +   chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
> > > > +   chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
> > >
> > > I'd beg for explicit FIELD_GET and FIELD_PREP here.
> > >
> > > >     gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > > index 77b23c004b94..ed075729ca09 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > > @@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> > > >     /*
> > > >      * We need to know the platform type before calling into adreno_gpu_init
> > > >      * so that the hw_apriv flag can be correctly set. Snoop into the info
> > > > -    * and grab the revision number
> > > > +    * and grab the revision number.
> > > > +    *
> > > > +    * Note that we wouldn't have been able to get this far if there is not
> > > > +    * a device table entry for this chip_id
> > >
> > > Having seen this note twice, shouldn't we explicitly pass adreno_info to our
> > > aNxx_gpu_init() functions and then further to adreno_gpu_init()?
> > >
> > > >      */
> > > > -   info = adreno_info(config->rev);
> > > > -   if (!info)
> > > > +   info = adreno_find_info(config->chip_id);
> > > > +   if (WARN_ON(!info))
> > > >             return ERR_PTR(-EINVAL);
> > > >     adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > index fd2e183bce60..4a718ff33635 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > @@ -22,7 +22,7 @@ module_param_named(allow_vram_carveout, allow_vram_carveout, bool, 0600);
> > > >   static const struct adreno_info gpulist[] = {
> > > >     {
> > > > -           .rev   = ADRENO_REV(2, 0, 0, 0),
> > > > +           .chip_ids = ADRENO_CHIP_IDS(0x02000000),
> > >
> > > This begs to use bit masks, but see below
> > >
> > > >             .family = ADRENO_2XX_GEN1,
> > > >             .revn  = 200,
> > > >             .fw = {
> > >
> > > [skipped]
> > >
> > > > @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
> > > >             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >             .init  = a3xx_gpu_init,
> > > >     }, {
> > > > -           .rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
> > > > +           .chip_ids = ADRENO_CHIP_IDS(
> > > > +                   0x03020000,
> > > > +                   0x03020001,
> > > > +                   0x03020002
> > >
> > > definitely a bitmask would help
> > >
> > > > +           ),
> > > >             .family = ADRENO_3XX,
> > > >             .revn  = 320,
> > > >             .fw = {
> > >
> > > [skipped the rest]
> > >
> > > > @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
> > > >   MODULE_FIRMWARE("qcom/a630_gmu.bin");
> > > >   MODULE_FIRMWARE("qcom/a630_zap.mbn");
> > > > -static inline bool _rev_match(uint8_t entry, uint8_t id)
> > > > -{
> > > > -   return (entry == ANY_ID) || (entry == id);
> > > > -}
> > > > -
> > > > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2)
> > > > -{
> > > > -
> > > > -   return _rev_match(rev1.core, rev2.core) &&
> > > > -           _rev_match(rev1.major, rev2.major) &&
> > > > -           _rev_match(rev1.minor, rev2.minor) &&
> > > > -           _rev_match(rev1.patchid, rev2.patchid);
> > > > -}
> > > > -
> > > > -const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > > +const struct adreno_info *adreno_find_info(uint32_t chip_id)
> > > >   {
> > > > -   int i;
> > > > -
> > > >     /* identify gpu: */
> > > > -   for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > > +   for (int i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > >             const struct adreno_info *info = &gpulist[i];
> > > >             if (info->machine && !of_machine_is_compatible(info->machine))
> > > >                     continue;
> > > > -           if (adreno_cmp_rev(info->rev, rev))
> > > > -                   return info;
> > > > +           for (int j = 0; info->chip_ids[j]; j++)
> > > > +                   if (info->chip_ids[j] == chip_id)
> > > > +                           return info;
> > > >     }
> > > >     return NULL;
> > > > @@ -598,12 +591,11 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
> > > >     return NULL;
> > > >   }
> > > > -static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > > > +static int find_chipid(struct device *dev, uint32_t *chipid)
> > > >   {
> > > >     struct device_node *node = dev->of_node;
> > > >     const char *compat;
> > > >     int ret;
> > > > -   u32 chipid;
> > > >     /* first search the compat strings for qcom,adreno-XYZ.W: */
> > > >     ret = of_property_read_string_index(node, "compatible", 0, &compat);
> > > > @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > > >             if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
> > > >                 sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
> > > > -                   rev->core = r / 100;
> > > > +                   uint32_t core, major, minor;
> > > > +
> > > > +                   core = r / 100;
> > > >                     r %= 100;
> > > > -                   rev->major = r / 10;
> > > > +                   major = r / 10;
> > > >                     r %= 10;
> > > > -                   rev->minor = r;
> > > > -                   rev->patchid = patch;
> > > > +                   minor = r;
> > > > +
> > > > +                   *chipid = (core << 24) |
> > > > +                           (major << 16) |
> > > > +                           (minor << 8) |
> > > > +                           patch;
> > >
> > > This starts to look realy crazy. I'd repeat my suggestion of moving towards
> > > of_device match data. This would result in some duplication, we'd have to
> > > explicitly list all supported compatibles in the of_match_table. But then we
> > > can drop all the CHIPID lists from device table and/or manual parsing of the
> > > chipid from the compat field.
> > >
> > > This way we can have per-SoC overrides, etc.
> > >
> > > >                     return 0;
> > > >             }
> > > > +
> > > > +           if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
> > > > +                   return 0;
> > > >     }
> > > >     /* and if that fails, fall back to legacy "qcom,chipid" property: */
> > > > -   ret = of_property_read_u32(node, "qcom,chipid", &chipid);
> > > > +   ret = of_property_read_u32(node, "qcom,chipid", chipid);
> > > >     if (ret) {
> > > >             DRM_DEV_ERROR(dev, "could not parse qcom,chipid: %d\n", ret);
> > > >             return ret;
> > > >     }
> > > > -   rev->core = (chipid >> 24) & 0xff;
> > > > -   rev->major = (chipid >> 16) & 0xff;
> > > > -   rev->minor = (chipid >> 8) & 0xff;
> > > > -   rev->patchid = (chipid & 0xff);
> > > > -
> > > >     dev_warn(dev, "Using legacy qcom,chipid binding!\n");
> > > > -   dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
> > > > -           rev->core, rev->major, rev->minor, rev->patchid);
> > > >     return 0;
> > > >   }
> > > > @@ -651,22 +645,22 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >     struct msm_gpu *gpu;
> > > >     int ret;
> > > > -   ret = find_chipid(dev, &config.rev);
> > > > +   ret = find_chipid(dev, &config.chip_id);
> > > >     if (ret)
> > > >             return ret;
> > > >     dev->platform_data = &config;
> > > >     priv->gpu_pdev = to_platform_device(dev);
> > > > -   info = adreno_info(config.rev);
> > > > +   info = adreno_find_info(config.chip_id);
> > > >     if (!info) {
> > > >             dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> > > > -                   ADRENO_CHIPID_ARGS(config.rev));
> > > > +                   ADRENO_CHIPID_ARGS(config.chip_id));
> > > >             return -ENXIO;
> > > >     }
> > > > -   DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> > > > +   DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.chip_id));
> > > >     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 1a982a926f21..1274609a74b1 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > @@ -326,10 +326,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > >             *value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;
> > > >             return 0;
> > > >     case MSM_PARAM_CHIP_ID:
> > > > -           *value =  (uint64_t)adreno_gpu->rev.patchid |
> > > > -                    ((uint64_t)adreno_gpu->rev.minor << 8) |
> > > > -                    ((uint64_t)adreno_gpu->rev.major << 16) |
> > > > -                    ((uint64_t)adreno_gpu->rev.core  << 24);
> > > > +           *value = adreno_gpu->chip_id;
> > > >             if (!adreno_gpu->info->revn)
> > > >                     *value |= ((uint64_t) adreno_gpu->speedbin) << 32;
> > > >             return 0;
> > > > @@ -849,7 +846,7 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> > > >     drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > > >                     adreno_gpu->info->revn,
> > > > -                   ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > > > +                   ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> > > >     /*
> > > >      * If this is state collected due to iova fault, so fault related info
> > > >      *
> > > > @@ -922,7 +919,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
> > > >     printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > > >                     adreno_gpu->info->revn,
> > > > -                   ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > > > +                   ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> > > >     for (i = 0; i < gpu->nr_rings; i++) {
> > > >             struct msm_ringbuffer *ring = gpu->rb[i];
> > > > @@ -1072,14 +1069,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> > > >     struct adreno_platform_config *config = dev->platform_data;
> > > >     struct msm_gpu_config adreno_gpu_config  = { 0 };
> > > >     struct msm_gpu *gpu = &adreno_gpu->base;
> > > > -   struct adreno_rev *rev = &config->rev;
> > > >     const char *gpu_name;
> > > >     u32 speedbin;
> > > >     int ret;
> > > >     adreno_gpu->funcs = funcs;
> > > > -   adreno_gpu->info = adreno_info(config->rev);
> > > > -   adreno_gpu->rev = *rev;
> > > > +   adreno_gpu->info = adreno_find_info(config->chip_id);
> > > > +   adreno_gpu->chip_id = config->chip_id;
> > > >     /* Only handle the core clock when GMU is not in use (or is absent). */
> > > >     if (adreno_has_gmu_wrapper(adreno_gpu) ||
> > > > @@ -1104,7 +1100,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> > > >     adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> > > >     gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > > > -                   ADRENO_CHIPID_ARGS(config->rev));
> > > > +                   ADRENO_CHIPID_ARGS(config->chip_id));
> > > >     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 73e7155f164c..18f53c7ab589 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > @@ -54,23 +54,15 @@ enum adreno_family {
> > > >   #define ADRENO_QUIRK_HAS_HW_APRIV         BIT(3)
> > > >   #define ADRENO_QUIRK_HAS_CACHED_COHERENT  BIT(4)
> > > > -struct adreno_rev {
> > > > -   uint8_t  core;
> > > > -   uint8_t  major;
> > > > -   uint8_t  minor;
> > > > -   uint8_t  patchid;
> > > > -};
> > > > -
> > > > -#define ANY_ID 0xff
> > > > -
> > > > -#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
> > > > +#define ADRENO_CHIPID_ARGS(_c) \
> > > > +   (((_c) >> 24) & 0xff), \
> > > > +   (((_c) >> 16) & 0xff), \
> > > > +   (((_c) >> 8)  & 0xff), \
> > > > +   ((_c) & 0xff)
> > >
> > > So, we still have some meaning for chipid?
> > >
> > > >   struct adreno_gpu_funcs {
> > > >     struct msm_gpu_funcs base;
> > > > @@ -87,7 +79,12 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > >   struct adreno_info {
> > > >     const char *machine;
> > > > -   struct adreno_rev rev;
> > > > +   /**
> > > > +    * @chipids: Table of matching chip-ids
> > > > +    *
> > > > +    * Terminated with 0 sentinal
> > > > +    */
> > > > +   uint32_t *chip_ids;
> > > >     enum adreno_family family;
> > > >     uint32_t revn;
> > > >     const char *fw[ADRENO_FW_MAX];
> > > > @@ -107,6 +104,8 @@ struct adreno_info {
> > > >     uint32_t *speedbins;
> > > >   };
> > > > +#define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
> > > > +
> > > >   /*
> > > >    * Helper to build a speedbin table, ie. the table:
> > > >    *      fuse | speedbin
> > > > @@ -125,12 +124,12 @@ struct adreno_info {
> > > >    */
> > > >   #define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
> > > > -const struct adreno_info *adreno_info(struct adreno_rev rev);
> > > > +const struct adreno_info *adreno_find_info(uint32_t chip_id);
> > > >   struct adreno_gpu {
> > > >     struct msm_gpu base;
> > > > -   struct adreno_rev rev;
> > > >     const struct adreno_info *info;
> > > > +   uint32_t chip_id;
> > > >     uint16_t speedbin;
> > > >     const struct adreno_gpu_funcs *funcs;
> > > > @@ -179,7 +178,7 @@ struct adreno_ocmem {
> > > >   /* platform config data (ie. from DT, or pdata) */
> > > >   struct adreno_platform_config {
> > > > -   struct adreno_rev rev;
> > > > +   uint32_t chip_id;
> > > >   };
> > > >   #define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
> > > > @@ -196,7 +195,15 @@ struct adreno_platform_config {
> > > >     __ret;                                             \
> > > >   })
> > > > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
> > > > +static inline uint8_t adreno_patchid(const struct adreno_gpu *gpu)
> > > > +{
> > > > +   /* It is probably ok to assume legacy "adreno_rev" format
> > > > +    * for all a6xx devices, but probably best to limit this
> > > > +    * to older things.
> > > > +    */
> > > > +   WARN_ON_ONCE(gpu->info->family >= ADRENO_6XX_GEN1);
> > > > +   return gpu->chip_id & 0xff;
> > > > +}
> > > >   static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
> > > >   {
> > > > @@ -252,7 +259,7 @@ static inline bool adreno_is_a330(const struct adreno_gpu *gpu)
> > > >   static inline bool adreno_is_a330v2(const struct adreno_gpu *gpu)
> > > >   {
> > > > -   return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
> > > > +   return adreno_is_a330(gpu) && (adreno_patchid(gpu) > 0);
> > > >   }
> > > >   static inline int adreno_is_a405(const struct adreno_gpu *gpu)
> > > > @@ -342,8 +349,7 @@ static inline int adreno_is_a650(const struct adreno_gpu *gpu)
> > > >   static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
> > > >   {
> > > > -   /* The order of args is important here to handle ANY_ID correctly */
> > > > -   return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> > > > +   return gpu->info->chip_ids[0] == 0x06030500;
> >
> > Why not use adreno_gpu->chip_id here? Or do we need this check before
> > adreno_gpu_init()? I thought we got rid of those.
>
> This is intentional, because there can be multiple chip-id's for a
> single table entry, in theory.  But the first chip_id in the gpu table
> entry should suffice, and is simpler than comparing
> adreno_gpu->chip_id against each possible match.

But why can't we use of_device_id::data ?

>
> BR,
> -R
>
> > >
> > > Ugh. The thing that bugs me isn't even the manual comparison of chip_ids[0],
> > > but the fact that these two platforms stand aside. I think at the end we
> > > should have a single mechanism for checking GPU's SKUs.
> > >
> > > Or maybe we should get rid of that completely.
> > >
> > > If we get rid of a single gpulist table and move individual GPU info
> > > definitions to aNxx_gpu.c, we can subclass a generic info structure with
> > > generation-specific data, for example generation-specific quirks, pointers
> > > to hcwg or VBIF registers, etc. And then instead of having adreno_is_foo all
> > > over the code we can migrate most of these checks to data in the gpu info
> > > data.
> >
> > I think the downstream driver does what you described here. It helps to
> > move more generation-specific data to the table without inflating the
> > gpu table size.
> >
> > -Akhil
> >
> > >
> > > >   }
> > > >   static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> > > > @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
> > > >   static inline int adreno_is_a690(const struct adreno_gpu *gpu)
> > > >   {
> > > > -   /* The order of args is important here to handle ANY_ID correctly */
> > > > -   return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> > > > +   return gpu->info->chip_ids[0] == 0x06090000;
> > > >   };
> > > >   /* check for a615, a616, a618, a619 or any a630 derivatives */
> > > >   static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >

On 7.07.2023 18:08, Rob Clark wrote:
> On Thu, Jul 6, 2023 at 5:25 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 6.07.2023 23:10, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Since the revision becomes an opaque identifier with future GPUs, move
>>> away from treating different ranges of bits as having a given meaning.
>>> This means that we need to explicitly list different patch revisions in
>>> the device table.
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
[...]

>>>  static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
>>>  {
>>> -     /* The order of args is important here to handle ANY_ID correctly */
>>> -     return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
>>> +     return gpu->info->chip_ids[0] == 0x06030500;
>>>  }
>> I'm sorry, but this screams trouble.. and doesn't sound very maintainable :/
>>
> 
> why? It is intentionally checking the first chip-id so that nothing
> breaks if later chip-ids are added
checking for magic numbers and magic array indices at the same time is
very prone to breaking, as breaking this check would not at all be easy
to spot in code review.

> 
>> Apart from all these comments, I don't really see the point of this patch,
>> other than trying to tie together Qualcomm's almost-meaningless chipids on
>> a7xx into the picture..
>>
>> Since they can't even be read back from the hardware, I don't think trying
>> to force them into the upstream kernel makes any sense.
> 
> Sure, we _could_ pick our own arbitrary identifiers, we don't have to
> align with kgsl.  But that would be a super huge PITA for mesa, which
> has support for both kernels.
Perhaps I'm biased towards keeping this kind of stuff out of the kernel,
but I'd say that Linux should decide on one logical path.

In between us starting this discussion, Qualcomm managed to introduce
yet another way of deciding what GPU is present on the system in their
downstream driver[1]. We're at like 5 now. Do we wanna keep up each time?
New ID rework for A8xx when that comes out one day?

> 
>> On a different note, I think we could try to blockify Adreno definitions a
>> bit by splitting things into:
>>
>> - Core GPU propeties (revision, fw name, GMEM size)
>>
>> - G(P)MU properties
>>
>> - Family data (quirks, reg presets in some config struct which could be a
>>   union of config structs per generation, hwcg, maybe protected regs ptr
>>   should also be moved there)
> 
> We do something like this on the mesa side.  But we also get to use
> python tricks to generate code as part of the build process which
> makes things a bit more elegant.
> 
> Fwiw, I was already thinking about splitting the gpu "hw catalog" from
> a flat table, to probably something more like a table of tables, so
> that we can split a2xx/a3xx/a4xx/a5xx/a6xx tables into separate files.
> And then we could move hwcg/protect/etc tables into the same file.
> But I thought that might be a bit too conflicty for the a7xx series so
> was thinking to wait until after that landed.. unless you don't think
> it will be a problem.
Yeah I'd like to get a7xx landed this season..

Konrad

[1] they now read parts of socinfo from smem and decide the CHIPID and
    speedbin based on that, but it's not available on most existing SoCs,
    that was thrown in with SOCID v17
> 
> BR,
> -R
> 
>> - Generation data (init function, a2xx and a6xx specifics)
>>
>> - Speedbin LUTs matched against socid
>>
>>
>> or something like that.. there's a whole lot of duplicated data atm
>>
>> Konrad
>>>
>>>  static inline int adreno_is_a660(const struct adreno_gpu *gpu)
>>> @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
>>>
>>>  static inline int adreno_is_a690(const struct adreno_gpu *gpu)
>>>  {
>>> -     /* The order of args is important here to handle ANY_ID correctly */
>>> -     return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
>>> +     return gpu->info->chip_ids[0] == 0x06090000;
>>>  };
>>>  /* check for a615, a616, a618, a619 or any a630 derivatives */
>>>  static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)

On Sat, Jul 15, 2023 at 6:38 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 7.07.2023 18:08, Rob Clark wrote:
> > On Thu, Jul 6, 2023 at 5:25 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 6.07.2023 23:10, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>> Since the revision becomes an opaque identifier with future GPUs, move
> >>> away from treating different ranges of bits as having a given meaning.
> >>> This means that we need to explicitly list different patch revisions in
> >>> the device table.
> >>>
> >>> Signed-off-by: Rob Clark <robdclark@chromium.org>
> >>> ---
> [...]
>
> >>>  static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
> >>>  {
> >>> -     /* The order of args is important here to handle ANY_ID correctly */
> >>> -     return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> >>> +     return gpu->info->chip_ids[0] == 0x06030500;
> >>>  }
> >> I'm sorry, but this screams trouble.. and doesn't sound very maintainable :/
> >>
> >
> > why? It is intentionally checking the first chip-id so that nothing
> > breaks if later chip-ids are added
> checking for magic numbers and magic array indices at the same time is
> very prone to breaking, as breaking this check would not at all be easy
> to spot in code review.

It isn't like we can arbitrarily change the chip-id's.. they are abi
between dt/kernel/userspace.  If they aren't changing, there isn't
anything to break.

> >
> >> Apart from all these comments, I don't really see the point of this patch,
> >> other than trying to tie together Qualcomm's almost-meaningless chipids on
> >> a7xx into the picture..
> >>
> >> Since they can't even be read back from the hardware, I don't think trying
> >> to force them into the upstream kernel makes any sense.
> >
> > Sure, we _could_ pick our own arbitrary identifiers, we don't have to
> > align with kgsl.  But that would be a super huge PITA for mesa, which
> > has support for both kernels.
> Perhaps I'm biased towards keeping this kind of stuff out of the kernel,
> but I'd say that Linux should decide on one logical path.

The kernel is the small part of the driver stack, and we have two
drivers above in userspace (vk and gallium) which can work on top of
three different kernels (msm, kgsl, and virtgpu).  So what sounds like
an easy/clean answer to you, is not necessarily easy/clean in the big
picture.

So wherever it comes from (and dt is fine, or socid or whatever, and
it's fine for it to just be an opaque 32b value as long as we don't
get conflicting values, I don't mind explicitly listing all of the
possible patch-id's for the legacy scheme) we need to stick with
CHIP_ID.

> In between us starting this discussion, Qualcomm managed to introduce
> yet another way of deciding what GPU is present on the system in their
> downstream driver[1]. We're at like 5 now. Do we wanna keep up each time?
> New ID rework for A8xx when that comes out one day?

What matters is how they expose it to userspace, not where they get
the value from.

BR,
-R

> >
> >> On a different note, I think we could try to blockify Adreno definitions a
> >> bit by splitting things into:
> >>
> >> - Core GPU propeties (revision, fw name, GMEM size)
> >>
> >> - G(P)MU properties
> >>
> >> - Family data (quirks, reg presets in some config struct which could be a
> >>   union of config structs per generation, hwcg, maybe protected regs ptr
> >>   should also be moved there)
> >
> > We do something like this on the mesa side.  But we also get to use
> > python tricks to generate code as part of the build process which
> > makes things a bit more elegant.
> >
> > Fwiw, I was already thinking about splitting the gpu "hw catalog" from
> > a flat table, to probably something more like a table of tables, so
> > that we can split a2xx/a3xx/a4xx/a5xx/a6xx tables into separate files.
> > And then we could move hwcg/protect/etc tables into the same file.
> > But I thought that might be a bit too conflicty for the a7xx series so
> > was thinking to wait until after that landed.. unless you don't think
> > it will be a problem.
> Yeah I'd like to get a7xx landed this season..
>
> Konrad
>
> [1] they now read parts of socinfo from smem and decide the CHIPID and
>     speedbin based on that, but it's not available on most existing SoCs,
>     that was thrown in with SOCID v17
> >
> > BR,
> > -R
> >
> >> - Generation data (init function, a2xx and a6xx specifics)
> >>
> >> - Speedbin LUTs matched against socid
> >>
> >>
> >> or something like that.. there's a whole lot of duplicated data atm
> >>
> >> Konrad
> >>>
> >>>  static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> >>> @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
> >>>
> >>>  static inline int adreno_is_a690(const struct adreno_gpu *gpu)
> >>>  {
> >>> -     /* The order of args is important here to handle ANY_ID correctly */
> >>> -     return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> >>> +     return gpu->info->chip_ids[0] == 0x06090000;
> >>>  };
> >>>  /* check for a615, a616, a618, a619 or any a630 derivatives */
> >>>  static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)

On Thu, Jul 13, 2023 at 03:06:36PM -0700, Rob Clark wrote:
> 
> On Thu, Jul 13, 2023 at 2:39 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >
> > On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote:
> > >
> > > On 07/07/2023 00:10, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > Since the revision becomes an opaque identifier with future GPUs, move
> > > > away from treating different ranges of bits as having a given meaning.
> > > > This means that we need to explicitly list different patch revisions in
> > > > the device table.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >   drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
> > > >   drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
> > > >   drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
> > > >   drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
> > > >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
> > > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
> > > >   8 files changed, 122 insertions(+), 110 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > > index 715436cb3996..8b4cdf95f445 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > > > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
> > > >     gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x00220000);
> > > >     /* Early A430's have a timing issue with SP/TP power collapse;
> > > >        disabling HW clock gating prevents it. */
> > > > -   if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
> > > > +   if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
> > > >             gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
> > > >     else
> > > >             gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0xAAAAAAAA);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > > index f0803e94ebe5..70d2b5342cd9 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> > > >     struct msm_drm_private *priv = dev->dev_private;
> > > >     struct platform_device *pdev = priv->gpu_pdev;
> > > >     struct adreno_platform_config *config = pdev->dev.platform_data;
> > > > +   const struct adreno_info *info;
> > > >     struct a5xx_gpu *a5xx_gpu = NULL;
> > > >     struct adreno_gpu *adreno_gpu;
> > > >     struct msm_gpu *gpu;
> > > > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> > > >     nr_rings = 4;
> > > > -   if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> > > > +   /*
> > > > +    * Note that we wouldn't have been able to get this far if there is not
> > > > +    * a device table entry for this chip_id
> > > > +    */
> > > > +   info = adreno_find_info(config->chip_id);
> > > > +   if (WARN_ON(!info))
> > > > +           return ERR_PTR(-EINVAL);
> > > > +
> > > > +   if (info->revn == 510)
> > > >             nr_rings = 1;
> > > >     ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > > index 0e63a1429189..7705f8010484 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > > > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
> > > >     /* The battery current limiter isn't enabled for A540 */
> > > >     config = AGC_LM_CONFIG_BCL_DISABLED;
> > > > -   config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > > > +   config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > > >     /* For now disable GPMU side throttling */
> > > >     config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > > index f1bb20574018..a9ba547a120c 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > > > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
> > > >     gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
> > > >             (1 << 31) | (0xa << 18) | (0xa0));
> > > > -   chipid = adreno_gpu->rev.core << 24;
> > > > -   chipid |= adreno_gpu->rev.major << 16;
> > > > -   chipid |= adreno_gpu->rev.minor << 12;
> > > > -   chipid |= adreno_gpu->rev.patchid << 8;
> > > > +   /* Note that the GMU has a slightly different layout for
> > > > +    * chip_id, for whatever reason, so a bit of massaging
> > > > +    * is needed.  The upper 16b are the same, but minor and
> > > > +    * patchid are packed in four bits each with the lower
> > > > +    * 8b unused:
> > > > +    */
> > > > +   chipid  = adreno_gpu->chip_id & 0xffff0000;
> > > > +   chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
> > > > +   chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
> > >
> > > I'd beg for explicit FIELD_GET and FIELD_PREP here.
> > >
> > > >     gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > > index 77b23c004b94..ed075729ca09 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > > @@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> > > >     /*
> > > >      * We need to know the platform type before calling into adreno_gpu_init
> > > >      * so that the hw_apriv flag can be correctly set. Snoop into the info
> > > > -    * and grab the revision number
> > > > +    * and grab the revision number.
> > > > +    *
> > > > +    * Note that we wouldn't have been able to get this far if there is not
> > > > +    * a device table entry for this chip_id
> > >
> > > Having seen this note twice, shouldn't we explicitly pass adreno_info to our
> > > aNxx_gpu_init() functions and then further to adreno_gpu_init()?
> > >
> > > >      */
> > > > -   info = adreno_info(config->rev);
> > > > -   if (!info)
> > > > +   info = adreno_find_info(config->chip_id);
> > > > +   if (WARN_ON(!info))
> > > >             return ERR_PTR(-EINVAL);
> > > >     adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > index fd2e183bce60..4a718ff33635 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > @@ -22,7 +22,7 @@ module_param_named(allow_vram_carveout, allow_vram_carveout, bool, 0600);
> > > >   static const struct adreno_info gpulist[] = {
> > > >     {
> > > > -           .rev   = ADRENO_REV(2, 0, 0, 0),
> > > > +           .chip_ids = ADRENO_CHIP_IDS(0x02000000),
> > >
> > > This begs to use bit masks, but see below
> > >
> > > >             .family = ADRENO_2XX_GEN1,
> > > >             .revn  = 200,
> > > >             .fw = {
> > >
> > > [skipped]
> > >
> > > > @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
> > > >             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >             .init  = a3xx_gpu_init,
> > > >     }, {
> > > > -           .rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
> > > > +           .chip_ids = ADRENO_CHIP_IDS(
> > > > +                   0x03020000,
> > > > +                   0x03020001,
> > > > +                   0x03020002
> > >
> > > definitely a bitmask would help
> > >
> > > > +           ),
> > > >             .family = ADRENO_3XX,
> > > >             .revn  = 320,
> > > >             .fw = {
> > >
> > > [skipped the rest]
> > >
> > > > @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
> > > >   MODULE_FIRMWARE("qcom/a630_gmu.bin");
> > > >   MODULE_FIRMWARE("qcom/a630_zap.mbn");
> > > > -static inline bool _rev_match(uint8_t entry, uint8_t id)
> > > > -{
> > > > -   return (entry == ANY_ID) || (entry == id);
> > > > -}
> > > > -
> > > > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2)
> > > > -{
> > > > -
> > > > -   return _rev_match(rev1.core, rev2.core) &&
> > > > -           _rev_match(rev1.major, rev2.major) &&
> > > > -           _rev_match(rev1.minor, rev2.minor) &&
> > > > -           _rev_match(rev1.patchid, rev2.patchid);
> > > > -}
> > > > -
> > > > -const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > > +const struct adreno_info *adreno_find_info(uint32_t chip_id)
> > > >   {
> > > > -   int i;
> > > > -
> > > >     /* identify gpu: */
> > > > -   for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > > +   for (int i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > >             const struct adreno_info *info = &gpulist[i];
> > > >             if (info->machine && !of_machine_is_compatible(info->machine))
> > > >                     continue;
> > > > -           if (adreno_cmp_rev(info->rev, rev))
> > > > -                   return info;
> > > > +           for (int j = 0; info->chip_ids[j]; j++)
> > > > +                   if (info->chip_ids[j] == chip_id)
> > > > +                           return info;
> > > >     }
> > > >     return NULL;
> > > > @@ -598,12 +591,11 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
> > > >     return NULL;
> > > >   }
> > > > -static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > > > +static int find_chipid(struct device *dev, uint32_t *chipid)
> > > >   {
> > > >     struct device_node *node = dev->of_node;
> > > >     const char *compat;
> > > >     int ret;
> > > > -   u32 chipid;
> > > >     /* first search the compat strings for qcom,adreno-XYZ.W: */
> > > >     ret = of_property_read_string_index(node, "compatible", 0, &compat);
> > > > @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > > >             if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
> > > >                 sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
> > > > -                   rev->core = r / 100;
> > > > +                   uint32_t core, major, minor;
> > > > +
> > > > +                   core = r / 100;
> > > >                     r %= 100;
> > > > -                   rev->major = r / 10;
> > > > +                   major = r / 10;
> > > >                     r %= 10;
> > > > -                   rev->minor = r;
> > > > -                   rev->patchid = patch;
> > > > +                   minor = r;
> > > > +
> > > > +                   *chipid = (core << 24) |
> > > > +                           (major << 16) |
> > > > +                           (minor << 8) |
> > > > +                           patch;
> > >
> > > This starts to look realy crazy. I'd repeat my suggestion of moving towards
> > > of_device match data. This would result in some duplication, we'd have to
> > > explicitly list all supported compatibles in the of_match_table. But then we
> > > can drop all the CHIPID lists from device table and/or manual parsing of the
> > > chipid from the compat field.
> > >
> > > This way we can have per-SoC overrides, etc.
> > >
> > > >                     return 0;
> > > >             }
> > > > +
> > > > +           if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
> > > > +                   return 0;
> > > >     }
> > > >     /* and if that fails, fall back to legacy "qcom,chipid" property: */
> > > > -   ret = of_property_read_u32(node, "qcom,chipid", &chipid);
> > > > +   ret = of_property_read_u32(node, "qcom,chipid", chipid);
> > > >     if (ret) {
> > > >             DRM_DEV_ERROR(dev, "could not parse qcom,chipid: %d\n", ret);
> > > >             return ret;
> > > >     }
> > > > -   rev->core = (chipid >> 24) & 0xff;
> > > > -   rev->major = (chipid >> 16) & 0xff;
> > > > -   rev->minor = (chipid >> 8) & 0xff;
> > > > -   rev->patchid = (chipid & 0xff);
> > > > -
> > > >     dev_warn(dev, "Using legacy qcom,chipid binding!\n");
> > > > -   dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
> > > > -           rev->core, rev->major, rev->minor, rev->patchid);
> > > >     return 0;
> > > >   }
> > > > @@ -651,22 +645,22 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >     struct msm_gpu *gpu;
> > > >     int ret;
> > > > -   ret = find_chipid(dev, &config.rev);
> > > > +   ret = find_chipid(dev, &config.chip_id);
> > > >     if (ret)
> > > >             return ret;
> > > >     dev->platform_data = &config;
> > > >     priv->gpu_pdev = to_platform_device(dev);
> > > > -   info = adreno_info(config.rev);
> > > > +   info = adreno_find_info(config.chip_id);
> > > >     if (!info) {
> > > >             dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> > > > -                   ADRENO_CHIPID_ARGS(config.rev));
> > > > +                   ADRENO_CHIPID_ARGS(config.chip_id));
> > > >             return -ENXIO;
> > > >     }
> > > > -   DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> > > > +   DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.chip_id));
> > > >     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 1a982a926f21..1274609a74b1 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > @@ -326,10 +326,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > > >             *value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;
> > > >             return 0;
> > > >     case MSM_PARAM_CHIP_ID:
> > > > -           *value =  (uint64_t)adreno_gpu->rev.patchid |
> > > > -                    ((uint64_t)adreno_gpu->rev.minor << 8) |
> > > > -                    ((uint64_t)adreno_gpu->rev.major << 16) |
> > > > -                    ((uint64_t)adreno_gpu->rev.core  << 24);
> > > > +           *value = adreno_gpu->chip_id;
> > > >             if (!adreno_gpu->info->revn)
> > > >                     *value |= ((uint64_t) adreno_gpu->speedbin) << 32;
> > > >             return 0;
> > > > @@ -849,7 +846,7 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> > > >     drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > > >                     adreno_gpu->info->revn,
> > > > -                   ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > > > +                   ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> > > >     /*
> > > >      * If this is state collected due to iova fault, so fault related info
> > > >      *
> > > > @@ -922,7 +919,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
> > > >     printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > > >                     adreno_gpu->info->revn,
> > > > -                   ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > > > +                   ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> > > >     for (i = 0; i < gpu->nr_rings; i++) {
> > > >             struct msm_ringbuffer *ring = gpu->rb[i];
> > > > @@ -1072,14 +1069,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> > > >     struct adreno_platform_config *config = dev->platform_data;
> > > >     struct msm_gpu_config adreno_gpu_config  = { 0 };
> > > >     struct msm_gpu *gpu = &adreno_gpu->base;
> > > > -   struct adreno_rev *rev = &config->rev;
> > > >     const char *gpu_name;
> > > >     u32 speedbin;
> > > >     int ret;
> > > >     adreno_gpu->funcs = funcs;
> > > > -   adreno_gpu->info = adreno_info(config->rev);
> > > > -   adreno_gpu->rev = *rev;
> > > > +   adreno_gpu->info = adreno_find_info(config->chip_id);
> > > > +   adreno_gpu->chip_id = config->chip_id;
> > > >     /* Only handle the core clock when GMU is not in use (or is absent). */
> > > >     if (adreno_has_gmu_wrapper(adreno_gpu) ||
> > > > @@ -1104,7 +1100,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> > > >     adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> > > >     gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > > > -                   ADRENO_CHIPID_ARGS(config->rev));
> > > > +                   ADRENO_CHIPID_ARGS(config->chip_id));
> > > >     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 73e7155f164c..18f53c7ab589 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > @@ -54,23 +54,15 @@ enum adreno_family {
> > > >   #define ADRENO_QUIRK_HAS_HW_APRIV         BIT(3)
> > > >   #define ADRENO_QUIRK_HAS_CACHED_COHERENT  BIT(4)
> > > > -struct adreno_rev {
> > > > -   uint8_t  core;
> > > > -   uint8_t  major;
> > > > -   uint8_t  minor;
> > > > -   uint8_t  patchid;
> > > > -};
> > > > -
> > > > -#define ANY_ID 0xff
> > > > -
> > > > -#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
> > > > +#define ADRENO_CHIPID_ARGS(_c) \
> > > > +   (((_c) >> 24) & 0xff), \
> > > > +   (((_c) >> 16) & 0xff), \
> > > > +   (((_c) >> 8)  & 0xff), \
> > > > +   ((_c) & 0xff)
> > >
> > > So, we still have some meaning for chipid?
> > >
> > > >   struct adreno_gpu_funcs {
> > > >     struct msm_gpu_funcs base;
> > > > @@ -87,7 +79,12 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > >   struct adreno_info {
> > > >     const char *machine;
> > > > -   struct adreno_rev rev;
> > > > +   /**
> > > > +    * @chipids: Table of matching chip-ids
> > > > +    *
> > > > +    * Terminated with 0 sentinal
> > > > +    */
> > > > +   uint32_t *chip_ids;
> > > >     enum adreno_family family;
> > > >     uint32_t revn;
> > > >     const char *fw[ADRENO_FW_MAX];
> > > > @@ -107,6 +104,8 @@ struct adreno_info {
> > > >     uint32_t *speedbins;
> > > >   };
> > > > +#define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
> > > > +
> > > >   /*
> > > >    * Helper to build a speedbin table, ie. the table:
> > > >    *      fuse | speedbin
> > > > @@ -125,12 +124,12 @@ struct adreno_info {
> > > >    */
> > > >   #define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
> > > > -const struct adreno_info *adreno_info(struct adreno_rev rev);
> > > > +const struct adreno_info *adreno_find_info(uint32_t chip_id);
> > > >   struct adreno_gpu {
> > > >     struct msm_gpu base;
> > > > -   struct adreno_rev rev;
> > > >     const struct adreno_info *info;
> > > > +   uint32_t chip_id;
> > > >     uint16_t speedbin;
> > > >     const struct adreno_gpu_funcs *funcs;
> > > > @@ -179,7 +178,7 @@ struct adreno_ocmem {
> > > >   /* platform config data (ie. from DT, or pdata) */
> > > >   struct adreno_platform_config {
> > > > -   struct adreno_rev rev;
> > > > +   uint32_t chip_id;
> > > >   };
> > > >   #define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
> > > > @@ -196,7 +195,15 @@ struct adreno_platform_config {
> > > >     __ret;                                             \
> > > >   })
> > > > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
> > > > +static inline uint8_t adreno_patchid(const struct adreno_gpu *gpu)
> > > > +{
> > > > +   /* It is probably ok to assume legacy "adreno_rev" format
> > > > +    * for all a6xx devices, but probably best to limit this
> > > > +    * to older things.
> > > > +    */
> > > > +   WARN_ON_ONCE(gpu->info->family >= ADRENO_6XX_GEN1);
> > > > +   return gpu->chip_id & 0xff;
> > > > +}
> > > >   static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
> > > >   {
> > > > @@ -252,7 +259,7 @@ static inline bool adreno_is_a330(const struct adreno_gpu *gpu)
> > > >   static inline bool adreno_is_a330v2(const struct adreno_gpu *gpu)
> > > >   {
> > > > -   return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
> > > > +   return adreno_is_a330(gpu) && (adreno_patchid(gpu) > 0);
> > > >   }
> > > >   static inline int adreno_is_a405(const struct adreno_gpu *gpu)
> > > > @@ -342,8 +349,7 @@ static inline int adreno_is_a650(const struct adreno_gpu *gpu)
> > > >   static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
> > > >   {
> > > > -   /* The order of args is important here to handle ANY_ID correctly */
> > > > -   return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> > > > +   return gpu->info->chip_ids[0] == 0x06030500;
> >
> > Why not use adreno_gpu->chip_id here? Or do we need this check before
> > adreno_gpu_init()? I thought we got rid of those.
> 
> This is intentional, because there can be multiple chip-id's for a
> single table entry, in theory.  But the first chip_id in the gpu table
> entry should suffice, and is simpler than comparing
> adreno_gpu->chip_id against each possible match.

Sounds that would work, but please mention somewhere that a new chipid
should always be appended to the bottom of the array. Otherwise, it
would introduce be a silly subtle bug.

-Akhil

> 
> BR,
> -R
> 
> > >
> > > Ugh. The thing that bugs me isn't even the manual comparison of chip_ids[0],
> > > but the fact that these two platforms stand aside. I think at the end we
> > > should have a single mechanism for checking GPU's SKUs.
> > >
> > > Or maybe we should get rid of that completely.
> > >
> > > If we get rid of a single gpulist table and move individual GPU info
> > > definitions to aNxx_gpu.c, we can subclass a generic info structure with
> > > generation-specific data, for example generation-specific quirks, pointers
> > > to hcwg or VBIF registers, etc. And then instead of having adreno_is_foo all
> > > over the code we can migrate most of these checks to data in the gpu info
> > > data.
> >
> > I think the downstream driver does what you described here. It helps to
> > move more generation-specific data to the table without inflating the
> > gpu table size.
> >
> > -Akhil
> >
> > >
> > > >   }
> > > >   static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> > > > @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
> > > >   static inline int adreno_is_a690(const struct adreno_gpu *gpu)
> > > >   {
> > > > -   /* The order of args is important here to handle ANY_ID correctly */
> > > > -   return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> > > > +   return gpu->info->chip_ids[0] == 0x06090000;
> > > >   };
> > > >   /* check for a615, a616, a618, a619 or any a630 derivatives */
> > > >   static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >

On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 07/07/2023 00:10, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Since the revision becomes an opaque identifier with future GPUs, move
> > away from treating different ranges of bits as having a given meaning.
> > This means that we need to explicitly list different patch revisions in
> > the device table.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
> >   drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
> >   drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
> >   drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
> >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
> >   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
> >   8 files changed, 122 insertions(+), 110 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > index 715436cb3996..8b4cdf95f445 100644
> > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
> >       gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x00220000);
> >       /* Early A430's have a timing issue with SP/TP power collapse;
> >          disabling HW clock gating prevents it. */
> > -     if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
> > +     if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
> >               gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
> >       else
> >               gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0xAAAAAAAA);
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > index f0803e94ebe5..70d2b5342cd9 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> >       struct msm_drm_private *priv = dev->dev_private;
> >       struct platform_device *pdev = priv->gpu_pdev;
> >       struct adreno_platform_config *config = pdev->dev.platform_data;
> > +     const struct adreno_info *info;
> >       struct a5xx_gpu *a5xx_gpu = NULL;
> >       struct adreno_gpu *adreno_gpu;
> >       struct msm_gpu *gpu;
> > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> >
> >       nr_rings = 4;
> >
> > -     if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> > +     /*
> > +      * Note that we wouldn't have been able to get this far if there is not
> > +      * a device table entry for this chip_id
> > +      */
> > +     info = adreno_find_info(config->chip_id);
> > +     if (WARN_ON(!info))
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     if (info->revn == 510)
> >               nr_rings = 1;
> >
> >       ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > index 0e63a1429189..7705f8010484 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
> >
> >       /* The battery current limiter isn't enabled for A540 */
> >       config = AGC_LM_CONFIG_BCL_DISABLED;
> > -     config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > +     config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> >
> >       /* For now disable GPMU side throttling */
> >       config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index f1bb20574018..a9ba547a120c 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
> >       gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
> >               (1 << 31) | (0xa << 18) | (0xa0));
> >
> > -     chipid = adreno_gpu->rev.core << 24;
> > -     chipid |= adreno_gpu->rev.major << 16;
> > -     chipid |= adreno_gpu->rev.minor << 12;
> > -     chipid |= adreno_gpu->rev.patchid << 8;
> > +     /* Note that the GMU has a slightly different layout for
> > +      * chip_id, for whatever reason, so a bit of massaging
> > +      * is needed.  The upper 16b are the same, but minor and
> > +      * patchid are packed in four bits each with the lower
> > +      * 8b unused:
> > +      */
> > +     chipid  = adreno_gpu->chip_id & 0xffff0000;
> > +     chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
> > +     chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
>
> I'd beg for explicit FIELD_GET and FIELD_PREP here.
>
> >
> >       gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index 77b23c004b94..ed075729ca09 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> >       /*
> >        * We need to know the platform type before calling into adreno_gpu_init
> >        * so that the hw_apriv flag can be correctly set. Snoop into the info
> > -      * and grab the revision number
> > +      * and grab the revision number.
> > +      *
> > +      * Note that we wouldn't have been able to get this far if there is not
> > +      * a device table entry for this chip_id
>
> Having seen this note twice, shouldn't we explicitly pass adreno_info to
> our aNxx_gpu_init() functions and then further to adreno_gpu_init()?
>
> >        */
> > -     info = adreno_info(config->rev);
> > -     if (!info)
> > +     info = adreno_find_info(config->chip_id);
> > +     if (WARN_ON(!info))
> >               return ERR_PTR(-EINVAL);
> >
> >       adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index fd2e183bce60..4a718ff33635 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -22,7 +22,7 @@ module_param_named(allow_vram_carveout, allow_vram_carveout, bool, 0600);
> >
> >   static const struct adreno_info gpulist[] = {
> >       {
> > -             .rev   = ADRENO_REV(2, 0, 0, 0),
> > +             .chip_ids = ADRENO_CHIP_IDS(0x02000000),
>
> This begs to use bit masks, but see below
>
> >               .family = ADRENO_2XX_GEN1,
> >               .revn  = 200,
> >               .fw = {
>
> [skipped]
>
> > @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
> >               .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >               .init  = a3xx_gpu_init,
> >       }, {
> > -             .rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
> > +             .chip_ids = ADRENO_CHIP_IDS(
> > +                     0x03020000,
> > +                     0x03020001,
> > +                     0x03020002
>
> definitely a bitmask would help
>
> > +             ),
> >               .family = ADRENO_3XX,
> >               .revn  = 320,
> >               .fw = {
>
> [skipped the rest]
>
> > @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
> >   MODULE_FIRMWARE("qcom/a630_gmu.bin");
> >   MODULE_FIRMWARE("qcom/a630_zap.mbn");
> >
> > -static inline bool _rev_match(uint8_t entry, uint8_t id)
> > -{
> > -     return (entry == ANY_ID) || (entry == id);
> > -}
> > -
> > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2)
> > -{
> > -
> > -     return _rev_match(rev1.core, rev2.core) &&
> > -             _rev_match(rev1.major, rev2.major) &&
> > -             _rev_match(rev1.minor, rev2.minor) &&
> > -             _rev_match(rev1.patchid, rev2.patchid);
> > -}
> > -
> > -const struct adreno_info *adreno_info(struct adreno_rev rev)
> > +const struct adreno_info *adreno_find_info(uint32_t chip_id)
> >   {
> > -     int i;
> > -
> >       /* identify gpu: */
> > -     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > +     for (int i = 0; i < ARRAY_SIZE(gpulist); i++) {
> >               const struct adreno_info *info = &gpulist[i];
> >               if (info->machine && !of_machine_is_compatible(info->machine))
> >                       continue;
> > -             if (adreno_cmp_rev(info->rev, rev))
> > -                     return info;
> > +             for (int j = 0; info->chip_ids[j]; j++)
> > +                     if (info->chip_ids[j] == chip_id)
> > +                             return info;
> >       }
> >
> >       return NULL;
> > @@ -598,12 +591,11 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
> >       return NULL;
> >   }
> >
> > -static int find_chipid(struct device *dev, struct adreno_rev *rev)
> > +static int find_chipid(struct device *dev, uint32_t *chipid)
> >   {
> >       struct device_node *node = dev->of_node;
> >       const char *compat;
> >       int ret;
> > -     u32 chipid;
> >
> >       /* first search the compat strings for qcom,adreno-XYZ.W: */
> >       ret = of_property_read_string_index(node, "compatible", 0, &compat);
> > @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
> >
> >               if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
> >                   sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
> > -                     rev->core = r / 100;
> > +                     uint32_t core, major, minor;
> > +
> > +                     core = r / 100;
> >                       r %= 100;
> > -                     rev->major = r / 10;
> > +                     major = r / 10;
> >                       r %= 10;
> > -                     rev->minor = r;
> > -                     rev->patchid = patch;
> > +                     minor = r;
> > +
> > +                     *chipid = (core << 24) |
> > +                             (major << 16) |
> > +                             (minor << 8) |
> > +                             patch;
>
> This starts to look realy crazy. I'd repeat my suggestion of moving
> towards of_device match data. This would result in some duplication,
> we'd have to explicitly list all supported compatibles in the
> of_match_table. But then we can drop all the CHIPID lists from device
> table and/or manual parsing of the chipid from the compat field.
>
> This way we can have per-SoC overrides, etc.
>
> >
> >                       return 0;
> >               }
> > +
> > +             if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
> > +                     return 0;
> >       }
> >
> >       /* and if that fails, fall back to legacy "qcom,chipid" property: */
> > -     ret = of_property_read_u32(node, "qcom,chipid", &chipid);
> > +     ret = of_property_read_u32(node, "qcom,chipid", chipid);
> >       if (ret) {
> >               DRM_DEV_ERROR(dev, "could not parse qcom,chipid: %d\n", ret);
> >               return ret;
> >       }
> >
> > -     rev->core = (chipid >> 24) & 0xff;
> > -     rev->major = (chipid >> 16) & 0xff;
> > -     rev->minor = (chipid >> 8) & 0xff;
> > -     rev->patchid = (chipid & 0xff);
> > -
> >       dev_warn(dev, "Using legacy qcom,chipid binding!\n");
> > -     dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
> > -             rev->core, rev->major, rev->minor, rev->patchid);
> >
> >       return 0;
> >   }
> > @@ -651,22 +645,22 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >       struct msm_gpu *gpu;
> >       int ret;
> >
> > -     ret = find_chipid(dev, &config.rev);
> > +     ret = find_chipid(dev, &config.chip_id);
> >       if (ret)
> >               return ret;
> >
> >       dev->platform_data = &config;
> >       priv->gpu_pdev = to_platform_device(dev);
> >
> > -     info = adreno_info(config.rev);
> > +     info = adreno_find_info(config.chip_id);
> >
> >       if (!info) {
> >               dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
> > -                     ADRENO_CHIPID_ARGS(config.rev));
> > +                     ADRENO_CHIPID_ARGS(config.chip_id));
> >               return -ENXIO;
> >       }
> >
> > -     DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> > +     DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.chip_id));
> >
> >       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 1a982a926f21..1274609a74b1 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -326,10 +326,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> >               *value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;
> >               return 0;
> >       case MSM_PARAM_CHIP_ID:
> > -             *value =  (uint64_t)adreno_gpu->rev.patchid |
> > -                      ((uint64_t)adreno_gpu->rev.minor << 8) |
> > -                      ((uint64_t)adreno_gpu->rev.major << 16) |
> > -                      ((uint64_t)adreno_gpu->rev.core  << 24);
> > +             *value = adreno_gpu->chip_id;
> >               if (!adreno_gpu->info->revn)
> >                       *value |= ((uint64_t) adreno_gpu->speedbin) << 32;
> >               return 0;
> > @@ -849,7 +846,7 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> >
> >       drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> >                       adreno_gpu->info->revn,
> > -                     ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > +                     ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> >       /*
> >        * If this is state collected due to iova fault, so fault related info
> >        *
> > @@ -922,7 +919,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
> >
> >       printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> >                       adreno_gpu->info->revn,
> > -                     ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> > +                     ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
> >
> >       for (i = 0; i < gpu->nr_rings; i++) {
> >               struct msm_ringbuffer *ring = gpu->rb[i];
> > @@ -1072,14 +1069,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> >       struct adreno_platform_config *config = dev->platform_data;
> >       struct msm_gpu_config adreno_gpu_config  = { 0 };
> >       struct msm_gpu *gpu = &adreno_gpu->base;
> > -     struct adreno_rev *rev = &config->rev;
> >       const char *gpu_name;
> >       u32 speedbin;
> >       int ret;
> >
> >       adreno_gpu->funcs = funcs;
> > -     adreno_gpu->info = adreno_info(config->rev);
> > -     adreno_gpu->rev = *rev;
> > +     adreno_gpu->info = adreno_find_info(config->chip_id);
> > +     adreno_gpu->chip_id = config->chip_id;
> >
> >       /* Only handle the core clock when GMU is not in use (or is absent). */
> >       if (adreno_has_gmu_wrapper(adreno_gpu) ||
> > @@ -1104,7 +1100,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> >       adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> >
> >       gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > -                     ADRENO_CHIPID_ARGS(config->rev));
> > +                     ADRENO_CHIPID_ARGS(config->chip_id));
> >       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 73e7155f164c..18f53c7ab589 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -54,23 +54,15 @@ enum adreno_family {
> >   #define ADRENO_QUIRK_HAS_HW_APRIV           BIT(3)
> >   #define ADRENO_QUIRK_HAS_CACHED_COHERENT    BIT(4)
> >
> > -struct adreno_rev {
> > -     uint8_t  core;
> > -     uint8_t  major;
> > -     uint8_t  minor;
> > -     uint8_t  patchid;
> > -};
> > -
> > -#define ANY_ID 0xff
> > -
> > -#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
> > +#define ADRENO_CHIPID_ARGS(_c) \
> > +     (((_c) >> 24) & 0xff), \
> > +     (((_c) >> 16) & 0xff), \
> > +     (((_c) >> 8)  & 0xff), \
> > +     ((_c) & 0xff)
>
> So, we still have some meaning for chipid?

Only enough to do the inverse of what userspace does when parsing
devcoredump to construct chip-id.  Basically it is just a different
way to represent a 32b #

BR,
-R

> >
> >   struct adreno_gpu_funcs {
> >       struct msm_gpu_funcs base;
> > @@ -87,7 +79,12 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> >
> >   struct adreno_info {
> >       const char *machine;
> > -     struct adreno_rev rev;
> > +     /**
> > +      * @chipids: Table of matching chip-ids
> > +      *
> > +      * Terminated with 0 sentinal
> > +      */
> > +     uint32_t *chip_ids;
> >       enum adreno_family family;
> >       uint32_t revn;
> >       const char *fw[ADRENO_FW_MAX];
> > @@ -107,6 +104,8 @@ struct adreno_info {
> >       uint32_t *speedbins;
> >   };
> >
> > +#define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
> > +
> >   /*
> >    * Helper to build a speedbin table, ie. the table:
> >    *      fuse | speedbin
> > @@ -125,12 +124,12 @@ struct adreno_info {
> >    */
> >   #define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
> >
> > -const struct adreno_info *adreno_info(struct adreno_rev rev);
> > +const struct adreno_info *adreno_find_info(uint32_t chip_id);
> >
> >   struct adreno_gpu {
> >       struct msm_gpu base;
> > -     struct adreno_rev rev;
> >       const struct adreno_info *info;
> > +     uint32_t chip_id;
> >       uint16_t speedbin;
> >       const struct adreno_gpu_funcs *funcs;
> >
> > @@ -179,7 +178,7 @@ struct adreno_ocmem {
> >
> >   /* platform config data (ie. from DT, or pdata) */
> >   struct adreno_platform_config {
> > -     struct adreno_rev rev;
> > +     uint32_t chip_id;
> >   };
> >
> >   #define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
> > @@ -196,7 +195,15 @@ struct adreno_platform_config {
> >       __ret;                                             \
> >   })
> >
> > -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
> > +static inline uint8_t adreno_patchid(const struct adreno_gpu *gpu)
> > +{
> > +     /* It is probably ok to assume legacy "adreno_rev" format
> > +      * for all a6xx devices, but probably best to limit this
> > +      * to older things.
> > +      */
> > +     WARN_ON_ONCE(gpu->info->family >= ADRENO_6XX_GEN1);
> > +     return gpu->chip_id & 0xff;
> > +}
> >
> >   static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
> >   {
> > @@ -252,7 +259,7 @@ static inline bool adreno_is_a330(const struct adreno_gpu *gpu)
> >
> >   static inline bool adreno_is_a330v2(const struct adreno_gpu *gpu)
> >   {
> > -     return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
> > +     return adreno_is_a330(gpu) && (adreno_patchid(gpu) > 0);
> >   }
> >
> >   static inline int adreno_is_a405(const struct adreno_gpu *gpu)
> > @@ -342,8 +349,7 @@ static inline int adreno_is_a650(const struct adreno_gpu *gpu)
> >
> >   static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
> >   {
> > -     /* The order of args is important here to handle ANY_ID correctly */
> > -     return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> > +     return gpu->info->chip_ids[0] == 0x06030500;
>
> Ugh. The thing that bugs me isn't even the manual comparison of
> chip_ids[0], but the fact that these two platforms stand aside. I think
> at the end we should have a single mechanism for checking GPU's SKUs.
>
> Or maybe we should get rid of that completely.
>
> If we get rid of a single gpulist table and move individual GPU info
> definitions to aNxx_gpu.c, we can subclass a generic info structure with
> generation-specific data, for example generation-specific quirks,
> pointers to hcwg or VBIF registers, etc. And then instead of having
> adreno_is_foo all over the code we can migrate most of these checks to
> data in the gpu info data.
>
> >   }
> >
> >   static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> > @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
> >
> >   static inline int adreno_is_a690(const struct adreno_gpu *gpu)
> >   {
> > -     /* The order of args is important here to handle ANY_ID correctly */
> > -     return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> > +     return gpu->info->chip_ids[0] == 0x06090000;
> >   };
> >   /* check for a615, a616, a618, a619 or any a630 derivatives */
> >   static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)
>
> --
> With best wishes
> Dmitry
>

On 27/07/2023 00:37, Rob Clark wrote:
> On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 07/07/2023 00:10, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Since the revision becomes an opaque identifier with future GPUs, move
>>> away from treating different ranges of bits as having a given meaning.
>>> This means that we need to explicitly list different patch revisions in
>>> the device table.
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/a4xx_gpu.c      |   2 +-
>>>    drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  11 +-
>>>    drivers/gpu/drm/msm/adreno/a5xx_power.c    |   2 +-
>>>    drivers/gpu/drm/msm/adreno/a6xx_gmu.c      |  13 ++-
>>>    drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |   9 +-
>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++++++++++-----------
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  16 +--
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  51 ++++----
>>>    8 files changed, 122 insertions(+), 110 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
>>> index 715436cb3996..8b4cdf95f445 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
>>> @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
>>>        gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x00220000);
>>>        /* Early A430's have a timing issue with SP/TP power collapse;
>>>           disabling HW clock gating prevents it. */
>>> -     if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
>>> +     if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
>>>                gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
>>>        else
>>>                gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0xAAAAAAAA);
>>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> index f0803e94ebe5..70d2b5342cd9 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>>>        struct msm_drm_private *priv = dev->dev_private;
>>>        struct platform_device *pdev = priv->gpu_pdev;
>>>        struct adreno_platform_config *config = pdev->dev.platform_data;
>>> +     const struct adreno_info *info;
>>>        struct a5xx_gpu *a5xx_gpu = NULL;
>>>        struct adreno_gpu *adreno_gpu;
>>>        struct msm_gpu *gpu;
>>> @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>>>
>>>        nr_rings = 4;
>>>
>>> -     if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
>>> +     /*
>>> +      * Note that we wouldn't have been able to get this far if there is not
>>> +      * a device table entry for this chip_id
>>> +      */
>>> +     info = adreno_find_info(config->chip_id);
>>> +     if (WARN_ON(!info))
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     if (info->revn == 510)
>>>                nr_rings = 1;
>>>
>>>        ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
>>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
>>> index 0e63a1429189..7705f8010484 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
>>> @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
>>>
>>>        /* The battery current limiter isn't enabled for A540 */
>>>        config = AGC_LM_CONFIG_BCL_DISABLED;
>>> -     config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
>>> +     config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
>>>
>>>        /* For now disable GPMU side throttling */
>>>        config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> index f1bb20574018..a9ba547a120c 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
>>>        gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
>>>                (1 << 31) | (0xa << 18) | (0xa0));
>>>
>>> -     chipid = adreno_gpu->rev.core << 24;
>>> -     chipid |= adreno_gpu->rev.major << 16;
>>> -     chipid |= adreno_gpu->rev.minor << 12;
>>> -     chipid |= adreno_gpu->rev.patchid << 8;
>>> +     /* Note that the GMU has a slightly different layout for
>>> +      * chip_id, for whatever reason, so a bit of massaging
>>> +      * is needed.  The upper 16b are the same, but minor and
>>> +      * patchid are packed in four bits each with the lower
>>> +      * 8b unused:
>>> +      */
>>> +     chipid  = adreno_gpu->chip_id & 0xffff0000;
>>> +     chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
>>> +     chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
>>
>> I'd beg for explicit FIELD_GET and FIELD_PREP here.
>>
>>>
>>>        gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index 77b23c004b94..ed075729ca09 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>>>        /*
>>>         * We need to know the platform type before calling into adreno_gpu_init
>>>         * so that the hw_apriv flag can be correctly set. Snoop into the info
>>> -      * and grab the revision number
>>> +      * and grab the revision number.
>>> +      *
>>> +      * Note that we wouldn't have been able to get this far if there is not
>>> +      * a device table entry for this chip_id
>>
>> Having seen this note twice, shouldn't we explicitly pass adreno_info to
>> our aNxx_gpu_init() functions and then further to adreno_gpu_init()?
>>
>>>         */
>>> -     info = adreno_info(config->rev);
>>> -     if (!info)
>>> +     info = adreno_find_info(config->chip_id);
>>> +     if (WARN_ON(!info))
>>>                return ERR_PTR(-EINVAL);
>>>
>>>        adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
>>> index fd2e183bce60..4a718ff33635 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
>>> @@ -22,7 +22,7 @@ module_param_named(allow_vram_carveout, allow_vram_carveout, bool, 0600);
>>>
>>>    static const struct adreno_info gpulist[] = {
>>>        {
>>> -             .rev   = ADRENO_REV(2, 0, 0, 0),
>>> +             .chip_ids = ADRENO_CHIP_IDS(0x02000000),
>>
>> This begs to use bit masks, but see below
>>
>>>                .family = ADRENO_2XX_GEN1,
>>>                .revn  = 200,
>>>                .fw = {
>>
>> [skipped]
>>
>>> @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
>>>                .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>>                .init  = a3xx_gpu_init,
>>>        }, {
>>> -             .rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
>>> +             .chip_ids = ADRENO_CHIP_IDS(
>>> +                     0x03020000,
>>> +                     0x03020001,
>>> +                     0x03020002
>>
>> definitely a bitmask would help
>>
>>> +             ),
>>>                .family = ADRENO_3XX,
>>>                .revn  = 320,
>>>                .fw = {
>>
>> [skipped the rest]
>>
>>> @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
>>>    MODULE_FIRMWARE("qcom/a630_gmu.bin");
>>>    MODULE_FIRMWARE("qcom/a630_zap.mbn");
>>>
>>> -static inline bool _rev_match(uint8_t entry, uint8_t id)
>>> -{
>>> -     return (entry == ANY_ID) || (entry == id);
>>> -}
>>> -
>>> -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2)
>>> -{
>>> -
>>> -     return _rev_match(rev1.core, rev2.core) &&
>>> -             _rev_match(rev1.major, rev2.major) &&
>>> -             _rev_match(rev1.minor, rev2.minor) &&
>>> -             _rev_match(rev1.patchid, rev2.patchid);
>>> -}
>>> -
>>> -const struct adreno_info *adreno_info(struct adreno_rev rev)
>>> +const struct adreno_info *adreno_find_info(uint32_t chip_id)
>>>    {
>>> -     int i;
>>> -
>>>        /* identify gpu: */
>>> -     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
>>> +     for (int i = 0; i < ARRAY_SIZE(gpulist); i++) {
>>>                const struct adreno_info *info = &gpulist[i];
>>>                if (info->machine && !of_machine_is_compatible(info->machine))
>>>                        continue;
>>> -             if (adreno_cmp_rev(info->rev, rev))
>>> -                     return info;
>>> +             for (int j = 0; info->chip_ids[j]; j++)
>>> +                     if (info->chip_ids[j] == chip_id)
>>> +                             return info;
>>>        }
>>>
>>>        return NULL;
>>> @@ -598,12 +591,11 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
>>>        return NULL;
>>>    }
>>>
>>> -static int find_chipid(struct device *dev, struct adreno_rev *rev)
>>> +static int find_chipid(struct device *dev, uint32_t *chipid)
>>>    {
>>>        struct device_node *node = dev->of_node;
>>>        const char *compat;
>>>        int ret;
>>> -     u32 chipid;
>>>
>>>        /* first search the compat strings for qcom,adreno-XYZ.W: */
>>>        ret = of_property_read_string_index(node, "compatible", 0, &compat);
>>> @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
>>>
>>>                if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
>>>                    sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
>>> -                     rev->core = r / 100;
>>> +                     uint32_t core, major, minor;
>>> +
>>> +                     core = r / 100;
>>>                        r %= 100;
>>> -                     rev->major = r / 10;
>>> +                     major = r / 10;
>>>                        r %= 10;
>>> -                     rev->minor = r;
>>> -                     rev->patchid = patch;
>>> +                     minor = r;
>>> +
>>> +                     *chipid = (core << 24) |
>>> +                             (major << 16) |
>>> +                             (minor << 8) |
>>> +                             patch;
>>
>> This starts to look realy crazy. I'd repeat my suggestion of moving
>> towards of_device match data. This would result in some duplication,
>> we'd have to explicitly list all supported compatibles in the
>> of_match_table. But then we can drop all the CHIPID lists from device
>> table and/or manual parsing of the chipid from the compat field.
>>
>> This way we can have per-SoC overrides, etc.
>>
>>>
>>>                        return 0;
>>>                }
>>> +
>>> +             if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
>>> +                     return 0;
>>>        }
>>>
>>>        /* and if that fails, fall back to legacy "qcom,chipid" property: */
>>> -     ret = of_property_read_u32(node, "qcom,chipid", &chipid);
>>> +     ret = of_property_read_u32(node, "qcom,chipid", chipid);
>>>        if (ret) {
>>>                DRM_DEV_ERROR(dev, "could not parse qcom,chipid: %d\n", ret);
>>>                return ret;
>>>        }
>>>
>>> -     rev->core = (chipid >> 24) & 0xff;
>>> -     rev->major = (chipid >> 16) & 0xff;
>>> -     rev->minor = (chipid >> 8) & 0xff;
>>> -     rev->patchid = (chipid & 0xff);
>>> -
>>>        dev_warn(dev, "Using legacy qcom,chipid binding!\n");
>>> -     dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
>>> -             rev->core, rev->major, rev->minor, rev->patchid);
>>>
>>>        return 0;
>>>    }
>>> @@ -651,22 +645,22 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>>>        struct msm_gpu *gpu;
>>>        int ret;
>>>
>>> -     ret = find_chipid(dev, &config.rev);
>>> +     ret = find_chipid(dev, &config.chip_id);
>>>        if (ret)
>>>                return ret;
>>>
>>>        dev->platform_data = &config;
>>>        priv->gpu_pdev = to_platform_device(dev);
>>>
>>> -     info = adreno_info(config.rev);
>>> +     info = adreno_find_info(config.chip_id);
>>>
>>>        if (!info) {
>>>                dev_warn(drm->dev, "Unknown GPU revision: %"ADRENO_CHIPID_FMT"\n",
>>> -                     ADRENO_CHIPID_ARGS(config.rev));
>>> +                     ADRENO_CHIPID_ARGS(config.chip_id));
>>>                return -ENXIO;
>>>        }
>>>
>>> -     DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
>>> +     DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.chip_id));
>>>
>>>        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 1a982a926f21..1274609a74b1 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -326,10 +326,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
>>>                *value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;
>>>                return 0;
>>>        case MSM_PARAM_CHIP_ID:
>>> -             *value =  (uint64_t)adreno_gpu->rev.patchid |
>>> -                      ((uint64_t)adreno_gpu->rev.minor << 8) |
>>> -                      ((uint64_t)adreno_gpu->rev.major << 16) |
>>> -                      ((uint64_t)adreno_gpu->rev.core  << 24);
>>> +             *value = adreno_gpu->chip_id;
>>>                if (!adreno_gpu->info->revn)
>>>                        *value |= ((uint64_t) adreno_gpu->speedbin) << 32;
>>>                return 0;
>>> @@ -849,7 +846,7 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
>>>
>>>        drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
>>>                        adreno_gpu->info->revn,
>>> -                     ADRENO_CHIPID_ARGS(adreno_gpu->rev));
>>> +                     ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
>>>        /*
>>>         * If this is state collected due to iova fault, so fault related info
>>>         *
>>> @@ -922,7 +919,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
>>>
>>>        printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
>>>                        adreno_gpu->info->revn,
>>> -                     ADRENO_CHIPID_ARGS(adreno_gpu->rev));
>>> +                     ADRENO_CHIPID_ARGS(adreno_gpu->chip_id));
>>>
>>>        for (i = 0; i < gpu->nr_rings; i++) {
>>>                struct msm_ringbuffer *ring = gpu->rb[i];
>>> @@ -1072,14 +1069,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>        struct adreno_platform_config *config = dev->platform_data;
>>>        struct msm_gpu_config adreno_gpu_config  = { 0 };
>>>        struct msm_gpu *gpu = &adreno_gpu->base;
>>> -     struct adreno_rev *rev = &config->rev;
>>>        const char *gpu_name;
>>>        u32 speedbin;
>>>        int ret;
>>>
>>>        adreno_gpu->funcs = funcs;
>>> -     adreno_gpu->info = adreno_info(config->rev);
>>> -     adreno_gpu->rev = *rev;
>>> +     adreno_gpu->info = adreno_find_info(config->chip_id);
>>> +     adreno_gpu->chip_id = config->chip_id;
>>>
>>>        /* Only handle the core clock when GMU is not in use (or is absent). */
>>>        if (adreno_has_gmu_wrapper(adreno_gpu) ||
>>> @@ -1104,7 +1100,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>        adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
>>>
>>>        gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
>>> -                     ADRENO_CHIPID_ARGS(config->rev));
>>> +                     ADRENO_CHIPID_ARGS(config->chip_id));
>>>        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 73e7155f164c..18f53c7ab589 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> @@ -54,23 +54,15 @@ enum adreno_family {
>>>    #define ADRENO_QUIRK_HAS_HW_APRIV           BIT(3)
>>>    #define ADRENO_QUIRK_HAS_CACHED_COHERENT    BIT(4)
>>>
>>> -struct adreno_rev {
>>> -     uint8_t  core;
>>> -     uint8_t  major;
>>> -     uint8_t  minor;
>>> -     uint8_t  patchid;
>>> -};
>>> -
>>> -#define ANY_ID 0xff
>>> -
>>> -#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
>>> +#define ADRENO_CHIPID_ARGS(_c) \
>>> +     (((_c) >> 24) & 0xff), \
>>> +     (((_c) >> 16) & 0xff), \
>>> +     (((_c) >> 8)  & 0xff), \
>>> +     ((_c) & 0xff)
>>
>> So, we still have some meaning for chipid?
> 
> Only enough to do the inverse of what userspace does when parsing
> devcoredump to construct chip-id.  Basically it is just a different
> way to represent a 32b #

What about passing it in the direct form? The macro adds assumptions.

> 
> BR,
> -R
> 
>>>
>>>    struct adreno_gpu_funcs {
>>>        struct msm_gpu_funcs base;
>>> @@ -87,7 +79,12 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
>>>
>>>    struct adreno_info {
>>>        const char *machine;
>>> -     struct adreno_rev rev;
>>> +     /**
>>> +      * @chipids: Table of matching chip-ids
>>> +      *
>>> +      * Terminated with 0 sentinal
>>> +      */
>>> +     uint32_t *chip_ids;
>>>        enum adreno_family family;
>>>        uint32_t revn;
>>>        const char *fw[ADRENO_FW_MAX];
>>> @@ -107,6 +104,8 @@ struct adreno_info {
>>>        uint32_t *speedbins;
>>>    };
>>>
>>> +#define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
>>> +
>>>    /*
>>>     * Helper to build a speedbin table, ie. the table:
>>>     *      fuse | speedbin
>>> @@ -125,12 +124,12 @@ struct adreno_info {
>>>     */
>>>    #define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
>>>
>>> -const struct adreno_info *adreno_info(struct adreno_rev rev);
>>> +const struct adreno_info *adreno_find_info(uint32_t chip_id);
>>>
>>>    struct adreno_gpu {
>>>        struct msm_gpu base;
>>> -     struct adreno_rev rev;
>>>        const struct adreno_info *info;
>>> +     uint32_t chip_id;
>>>        uint16_t speedbin;
>>>        const struct adreno_gpu_funcs *funcs;
>>>
>>> @@ -179,7 +178,7 @@ struct adreno_ocmem {
>>>
>>>    /* platform config data (ie. from DT, or pdata) */
>>>    struct adreno_platform_config {
>>> -     struct adreno_rev rev;
>>> +     uint32_t chip_id;
>>>    };
>>>
>>>    #define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
>>> @@ -196,7 +195,15 @@ struct adreno_platform_config {
>>>        __ret;                                             \
>>>    })
>>>
>>> -bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>>> +static inline uint8_t adreno_patchid(const struct adreno_gpu *gpu)
>>> +{
>>> +     /* It is probably ok to assume legacy "adreno_rev" format
>>> +      * for all a6xx devices, but probably best to limit this
>>> +      * to older things.
>>> +      */
>>> +     WARN_ON_ONCE(gpu->info->family >= ADRENO_6XX_GEN1);
>>> +     return gpu->chip_id & 0xff;
>>> +}
>>>
>>>    static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
>>>    {
>>> @@ -252,7 +259,7 @@ static inline bool adreno_is_a330(const struct adreno_gpu *gpu)
>>>
>>>    static inline bool adreno_is_a330v2(const struct adreno_gpu *gpu)
>>>    {
>>> -     return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
>>> +     return adreno_is_a330(gpu) && (adreno_patchid(gpu) > 0);
>>>    }
>>>
>>>    static inline int adreno_is_a405(const struct adreno_gpu *gpu)
>>> @@ -342,8 +349,7 @@ static inline int adreno_is_a650(const struct adreno_gpu *gpu)
>>>
>>>    static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
>>>    {
>>> -     /* The order of args is important here to handle ANY_ID correctly */
>>> -     return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
>>> +     return gpu->info->chip_ids[0] == 0x06030500;
>>
>> Ugh. The thing that bugs me isn't even the manual comparison of
>> chip_ids[0], but the fact that these two platforms stand aside. I think
>> at the end we should have a single mechanism for checking GPU's SKUs.
>>
>> Or maybe we should get rid of that completely.
>>
>> If we get rid of a single gpulist table and move individual GPU info
>> definitions to aNxx_gpu.c, we can subclass a generic info structure with
>> generation-specific data, for example generation-specific quirks,
>> pointers to hcwg or VBIF registers, etc. And then instead of having
>> adreno_is_foo all over the code we can migrate most of these checks to
>> data in the gpu info data.
>>
>>>    }
>>>
>>>    static inline int adreno_is_a660(const struct adreno_gpu *gpu)
>>> @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
>>>
>>>    static inline int adreno_is_a690(const struct adreno_gpu *gpu)
>>>    {
>>> -     /* The order of args is important here to handle ANY_ID correctly */
>>> -     return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
>>> +     return gpu->info->chip_ids[0] == 0x06090000;
>>>    };
>>>    /* check for a615, a616, a618, a619 or any a630 derivatives */
>>>    static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)
>>
>> --
>> With best wishes
>> Dmitry
>>

On Wed, Jul 26, 2023 at 2:38 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 27/07/2023 00:37, Rob Clark wrote:
> > On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>
> >> On 07/07/2023 00:10, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>>    /* 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
> >>> +#define ADRENO_CHIPID_ARGS(_c) \
> >>> +     (((_c) >> 24) & 0xff), \
> >>> +     (((_c) >> 16) & 0xff), \
> >>> +     (((_c) >> 8)  & 0xff), \
> >>> +     ((_c) & 0xff)
> >>
> >> So, we still have some meaning for chipid?
> >
> > Only enough to do the inverse of what userspace does when parsing
> > devcoredump to construct chip-id.  Basically it is just a different
> > way to represent a 32b #
>
> What about passing it in the direct form? The macro adds assumptions.

It is uabi

I wouldn't call it adding assumptions as much as just a funny way to
format a number.  In some cases it might work out to something that
vaguely resembles a marketing name (6.3.0.2), in other cases it won't
(12.34.56.78).. it's just formatting a 32b # to match the way existing
userspace parses it

BR,
-R

On 27/07/2023 00:44, Rob Clark wrote:
> On Wed, Jul 26, 2023 at 2:38 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 27/07/2023 00:37, Rob Clark wrote:
>>> On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On 07/07/2023 00:10, Rob Clark wrote:
>>>>> From: Rob Clark <robdclark@chromium.org>
>>>>>
>>>>>     /* 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
>>>>> +#define ADRENO_CHIPID_ARGS(_c) \
>>>>> +     (((_c) >> 24) & 0xff), \
>>>>> +     (((_c) >> 16) & 0xff), \
>>>>> +     (((_c) >> 8)  & 0xff), \
>>>>> +     ((_c) & 0xff)
>>>>
>>>> So, we still have some meaning for chipid?
>>>
>>> Only enough to do the inverse of what userspace does when parsing
>>> devcoredump to construct chip-id.  Basically it is just a different
>>> way to represent a 32b #
>>
>> What about passing it in the direct form? The macro adds assumptions.
> 
> It is uabi
> 
> I wouldn't call it adding assumptions as much as just a funny way to
> format a number.  In some cases it might work out to something that
> vaguely resembles a marketing name (6.3.0.2), in other cases it won't
> (12.34.56.78).. it's just formatting a 32b # to match the way existing
> userspace parses it

I see. Sounds fine then.

On Sat, Jul 15, 2023 at 6:38 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 7.07.2023 18:08, Rob Clark wrote:
> > On Thu, Jul 6, 2023 at 5:25 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> Apart from all these comments, I don't really see the point of this patch,
> >> other than trying to tie together Qualcomm's almost-meaningless chipids on
> >> a7xx into the picture..
> >>
> >> Since they can't even be read back from the hardware, I don't think trying
> >> to force them into the upstream kernel makes any sense.
> >
> > Sure, we _could_ pick our own arbitrary identifiers, we don't have to
> > align with kgsl.  But that would be a super huge PITA for mesa, which
> > has support for both kernels.
> Perhaps I'm biased towards keeping this kind of stuff out of the kernel,
> but I'd say that Linux should decide on one logical path.
>
> In between us starting this discussion, Qualcomm managed to introduce
> yet another way of deciding what GPU is present on the system in their
> downstream driver[1]. We're at like 5 now. Do we wanna keep up each time?
> New ID rework for A8xx when that comes out one day?

[snip]

> [1] they now read parts of socinfo from smem and decide the CHIPID and
>     speedbin based on that, but it's not available on most existing SoCs,
>     that was thrown in with SOCID v17

This is actually exactly what we want.. something that we can read
from hw/fw and that blob userspace also uses (so we don't have to
worry about qc forgetting to change the id, etc)

BR,
-R