[06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

Commit Message

Rob Clark July 6, 2023, 9:10 p.m. UTC

From: Rob Clark <[e-mail]>

There are cases where there are differences due to SoC integration.
Such as cache-coherency support, and (in the next patch) e-fuse to
speedbin mappings.

Signed-off-by: Rob Clark <[e-mail]>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

Comments

Konrad Dybcio July 7, 2023, 12:40 a.m. UTC | #1

On 6.07.2023 23:10, Rob Clark wrote:
> From: Rob Clark <[e-mail]>
> 
> There are cases where there are differences due to SoC integration.
> Such as cache-coherency support, and (in the next patch) e-fuse to
> speedbin mappings.
> 
> Signed-off-by: Rob Clark <[e-mail]>
> ---
of_machine_is_compatible is rather used in extremely desperate
situations :/ I'm not sure this is the correct way to do this..

Especially since there's a direct correlation between GMU presence
and ability to do cached coherent.

The GMU mandates presence of RPMh (as most of what the GMU does is
talk to AOSS through its RSC).

To achieve I/O coherency, there must be some memory that both the
CPU and GPU (and possibly others) can access through some sort of
a negotiator/manager.

In our case, I believe that's LLC. And guess what that implies.
MEMNOC instead of BIMC. And guess what that implies. RPMh!

Now, we know GMU => RPMh, but does it work the other way around?

Yes. GMU wrapper was a hack because probably nobody in the Adreno team
would have imagined that somebody would be crazy enough to fork
multiple year old designs multiple times and release them as new
SoCs with updated arm cores and 5G..

(Except for A612 which has a "Reduced GMU" but that zombie still talks
to RPMh. And A612 is IO-coherent. So I guess it works anyway.)

Konrad

>  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 3c531da417b9..e62bc895a31f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
>  		.init = a6xx_gpu_init,
> +	}, {
> +		.machine = "qcom,sm4350",
> +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> +		.revn = 619,
> +		.fw = {
> +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> +		},
> +		.gmem = SZ_512K,
> +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> +		.init = a6xx_gpu_init,
> +		.zapfw = "a615_zap.mdt",
> +		.hwcg = a615_hwcg,
> +	}, {
> +		.machine = "qcom,sm6375",
> +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> +		.revn = 619,
> +		.fw = {
> +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> +		},
> +		.gmem = SZ_512K,
> +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> +		.init = a6xx_gpu_init,
> +		.zapfw = "a615_zap.mdt",
> +		.hwcg = a615_hwcg,
>  	}, {
>  		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
>  		.revn = 619,
> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
>  	/* identify gpu: */
>  	for (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;
>  	}
> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>  		config.rev.minor, config.rev.patchid);
>  
>  	priv->is_a2xx = config.rev.core == 2;
> +	priv->has_cached_coherent =
> +		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
>  
>  	gpu = info->init(drm);
>  	if (IS_ERR(gpu)) {
> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	priv->has_cached_coherent =
> -		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> -		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index e08d41337169..d5335b99c64c 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
>  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
>  
>  struct adreno_info {
> +	const char *machine;
>  	struct adreno_rev rev;
>  	uint32_t revn;
>  	const char *fw[ADRENO_FW_MAX];

Dmitry Baryshkov July 7, 2023, 2:34 a.m. UTC | #2

On 07/07/2023 00:10, Rob Clark wrote:
> From: Rob Clark <[e-mail]>
> 
> There are cases where there are differences due to SoC integration.
> Such as cache-coherency support, and (in the next patch) e-fuse to
> speedbin mappings.

I have the feeling that we are trying to circumvent the way DT works. 
I'd suggest adding explicit SoC-compatible strings to Adreno bindings 
and then using of_device_id::data and then of_device_get_match_data().

> 
> Signed-off-by: Rob Clark <[e-mail]>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
>   2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 3c531da417b9..e62bc895a31f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
>   		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>   		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
>   		.init = a6xx_gpu_init,
> +	}, {
> +		.machine = "qcom,sm4350",
> +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> +		.revn = 619,
> +		.fw = {
> +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> +		},
> +		.gmem = SZ_512K,
> +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> +		.init = a6xx_gpu_init,
> +		.zapfw = "a615_zap.mdt",
> +		.hwcg = a615_hwcg,
> +	}, {
> +		.machine = "qcom,sm6375",
> +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> +		.revn = 619,
> +		.fw = {
> +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> +		},
> +		.gmem = SZ_512K,
> +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> +		.init = a6xx_gpu_init,
> +		.zapfw = "a615_zap.mdt",
> +		.hwcg = a615_hwcg,
>   	}, {
>   		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
>   		.revn = 619,
> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
>   	/* identify gpu: */
>   	for (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;
>   	}
> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>   		config.rev.minor, config.rev.patchid);
>   
>   	priv->is_a2xx = config.rev.core == 2;
> +	priv->has_cached_coherent =
> +		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
>   
>   	gpu = info->init(drm);
>   	if (IS_ERR(gpu)) {
> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>   	if (ret)
>   		return ret;
>   
> -	priv->has_cached_coherent =
> -		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> -		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index e08d41337169..d5335b99c64c 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
>   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
>   
>   struct adreno_info {
> +	const char *machine;
>   	struct adreno_rev rev;
>   	uint32_t revn;
>   	const char *fw[ADRENO_FW_MAX];

Akhil P Oommen July 13, 2023, 8:26 p.m. UTC | #3

On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> 
> On 07/07/2023 00:10, Rob Clark wrote:
> > From: Rob Clark <[e-mail]>
> > 
> > There are cases where there are differences due to SoC integration.
> > Such as cache-coherency support, and (in the next patch) e-fuse to
> > speedbin mappings.
> 
> I have the feeling that we are trying to circumvent the way DT works. I'd
> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> using of_device_id::data and then of_device_get_match_data().
> 
Just thinking, then how about a unique compatible string which we match
to identify gpu->info and drop chip-id check completely here?

-Akhil

> > 
> > Signed-off-by: Rob Clark <[e-mail]>
> > ---
> >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> >   2 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index 3c531da417b9..e62bc895a31f 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> >   		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >   		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> >   		.init = a6xx_gpu_init,
> > +	}, {
> > +		.machine = "qcom,sm4350",
> > +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > +		.revn = 619,
> > +		.fw = {
> > +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> > +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> > +		},
> > +		.gmem = SZ_512K,
> > +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > +		.init = a6xx_gpu_init,
> > +		.zapfw = "a615_zap.mdt",
> > +		.hwcg = a615_hwcg,
> > +	}, {
> > +		.machine = "qcom,sm6375",
> > +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > +		.revn = 619,
> > +		.fw = {
> > +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> > +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> > +		},
> > +		.gmem = SZ_512K,
> > +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > +		.init = a6xx_gpu_init,
> > +		.zapfw = "a615_zap.mdt",
> > +		.hwcg = a615_hwcg,
> >   	}, {
> >   		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >   		.revn = 619,
> > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> >   	/* identify gpu: */
> >   	for (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;
> >   	}
> > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >   		config.rev.minor, config.rev.patchid);
> >   	priv->is_a2xx = config.rev.core == 2;
> > +	priv->has_cached_coherent =
> > +		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> >   	gpu = info->init(drm);
> >   	if (IS_ERR(gpu)) {
> > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >   	if (ret)
> >   		return ret;
> > -	priv->has_cached_coherent =
> > -		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > -		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > -
> >   	return 0;
> >   }
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index e08d41337169..d5335b99c64c 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> >   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> >   struct adreno_info {
> > +	const char *machine;
> >   	struct adreno_rev rev;
> >   	uint32_t revn;
> >   	const char *fw[ADRENO_FW_MAX];
> 
> -- 
> With best wishes
> Dmitry
>

Akhil P Oommen July 13, 2023, 10:15 p.m. UTC | #4

On Fri, Jul 07, 2023 at 02:40:47AM +0200, Konrad Dybcio wrote:
> 
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark <[e-mail]>
> > 
> > There are cases where there are differences due to SoC integration.
> > Such as cache-coherency support, and (in the next patch) e-fuse to
> > speedbin mappings.
> > 
> > Signed-off-by: Rob Clark <[e-mail]>
> > ---
> of_machine_is_compatible is rather used in extremely desperate
> situations :/ I'm not sure this is the correct way to do this..
> 
> Especially since there's a direct correlation between GMU presence
> and ability to do cached coherent.
> 
> The GMU mandates presence of RPMh (as most of what the GMU does is
> talk to AOSS through its RSC).
> 
> To achieve I/O coherency, there must be some memory that both the
> CPU and GPU (and possibly others) can access through some sort of
> a negotiator/manager.
> 
> In our case, I believe that's LLC. And guess what that implies.
> MEMNOC instead of BIMC. And guess what that implies. RPMh!
> 
> Now, we know GMU => RPMh, but does it work the other way around?

I don't think we should tie gpu io-coherency with rpmh or llc. These
features are more dependent on SoC architecture than GPU arch.

-Akhil

> 
> Yes. GMU wrapper was a hack because probably nobody in the Adreno team
> would have imagined that somebody would be crazy enough to fork
> multiple year old designs multiple times and release them as new
> SoCs with updated arm cores and 5G..
> 
> (Except for A612 which has a "Reduced GMU" but that zombie still talks
> to RPMh. And A612 is IO-coherent. So I guess it works anyway.)
> 
> Konrad
> 
> >  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> >  2 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index 3c531da417b9..e62bc895a31f 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> >  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >  		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> >  		.init = a6xx_gpu_init,
> > +	}, {
> > +		.machine = "qcom,sm4350",
> > +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > +		.revn = 619,
> > +		.fw = {
> > +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> > +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> > +		},
> > +		.gmem = SZ_512K,
> > +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > +		.init = a6xx_gpu_init,
> > +		.zapfw = "a615_zap.mdt",
> > +		.hwcg = a615_hwcg,
> > +	}, {
> > +		.machine = "qcom,sm6375",
> > +		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > +		.revn = 619,
> > +		.fw = {
> > +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> > +			[ADRENO_FW_GMU] = "a619_gmu.bin",
> > +		},
> > +		.gmem = SZ_512K,
> > +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > +		.init = a6xx_gpu_init,
> > +		.zapfw = "a615_zap.mdt",
> > +		.hwcg = a615_hwcg,
> >  	}, {
> >  		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >  		.revn = 619,
> > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> >  	/* identify gpu: */
> >  	for (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;
> >  	}
> > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >  		config.rev.minor, config.rev.patchid);
> >  
> >  	priv->is_a2xx = config.rev.core == 2;
> > +	priv->has_cached_coherent =
> > +		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> >  
> >  	gpu = info->init(drm);
> >  	if (IS_ERR(gpu)) {
> > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >  	if (ret)
> >  		return ret;
> >  
> > -	priv->has_cached_coherent =
> > -		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > -		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > -
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index e08d41337169..d5335b99c64c 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> >  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> >  
> >  struct adreno_info {
> > +	const char *machine;
> >  	struct adreno_rev rev;
> >  	uint32_t revn;
> >  	const char *fw[ADRENO_FW_MAX];

Rob Clark July 26, 2023, 6:28 p.m. UTC | #5

On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
>
> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> >
> > On 07/07/2023 00:10, Rob Clark wrote:
> > > From: Rob Clark <[e-mail]>
> > >
> > > There are cases where there are differences due to SoC integration.
> > > Such as cache-coherency support, and (in the next patch) e-fuse to
> > > speedbin mappings.
> >
> > I have the feeling that we are trying to circumvent the way DT works. I'd
> > suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > using of_device_id::data and then of_device_get_match_data().
> >
> Just thinking, then how about a unique compatible string which we match
> to identify gpu->info and drop chip-id check completely here?

Ok, I think we could do this, so something like:

  compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350
or sm6375, so I suppose we could get away with this change

BR,
-R

> -Akhil
>
> > >
> > > Signed-off-by: Rob Clark <[e-mail]>
> > > ---
> > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > >   2 files changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > index 3c531da417b9..e62bc895a31f 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > >             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > >             .init = a6xx_gpu_init,
> > > +   }, {
> > > +           .machine = "qcom,sm4350",
> > > +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > +           .revn = 619,
> > > +           .fw = {
> > > +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > +           },
> > > +           .gmem = SZ_512K,
> > > +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > +           .init = a6xx_gpu_init,
> > > +           .zapfw = "a615_zap.mdt",
> > > +           .hwcg = a615_hwcg,
> > > +   }, {
> > > +           .machine = "qcom,sm6375",
> > > +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > +           .revn = 619,
> > > +           .fw = {
> > > +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > +           },
> > > +           .gmem = SZ_512K,
> > > +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > +           .init = a6xx_gpu_init,
> > > +           .zapfw = "a615_zap.mdt",
> > > +           .hwcg = a615_hwcg,
> > >     }, {
> > >             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > >             .revn = 619,
> > > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > >     /* identify gpu: */
> > >     for (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;
> > >     }
> > > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > >             config.rev.minor, config.rev.patchid);
> > >     priv->is_a2xx = config.rev.core == 2;
> > > +   priv->has_cached_coherent =
> > > +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > >     gpu = info->init(drm);
> > >     if (IS_ERR(gpu)) {
> > > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > >     if (ret)
> > >             return ret;
> > > -   priv->has_cached_coherent =
> > > -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > -
> > >     return 0;
> > >   }
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > index e08d41337169..d5335b99c64c 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > >   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > >   struct adreno_info {
> > > +   const char *machine;
> > >     struct adreno_rev rev;
> > >     uint32_t revn;
> > >     const char *fw[ADRENO_FW_MAX];
> >
> > --
> > With best wishes
> > Dmitry
> >

Dmitry Baryshkov July 26, 2023, 8 p.m. UTC | #6

On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
>
> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> >
> > On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > >
> > > On 07/07/2023 00:10, Rob Clark wrote:
> > > > From: Rob Clark <[e-mail]>
> > > >
> > > > There are cases where there are differences due to SoC integration.
> > > > Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > speedbin mappings.
> > >
> > > I have the feeling that we are trying to circumvent the way DT works. I'd
> > > suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > using of_device_id::data and then of_device_get_match_data().
> > >
> > Just thinking, then how about a unique compatible string which we match
> > to identify gpu->info and drop chip-id check completely here?
>
> Ok, I think we could do this, so something like:
>
>   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
>
> ?
>
> It looks like we don't have gpu dt bits upstream yet for either sm4350
> or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string.
So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a
part of SoC data:

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

>
> BR,
> -R
>
> > -Akhil
> >
> > > >
> > > > Signed-off-by: Rob Clark <[e-mail]>
> > > > ---
> > > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > >   2 files changed, 31 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > index 3c531da417b9..e62bc895a31f 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > >             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > >             .init = a6xx_gpu_init,
> > > > +   }, {
> > > > +           .machine = "qcom,sm4350",
> > > > +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > +           .revn = 619,
> > > > +           .fw = {
> > > > +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > +           },
> > > > +           .gmem = SZ_512K,
> > > > +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > +           .init = a6xx_gpu_init,
> > > > +           .zapfw = "a615_zap.mdt",
> > > > +           .hwcg = a615_hwcg,
> > > > +   }, {
> > > > +           .machine = "qcom,sm6375",
> > > > +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > +           .revn = 619,
> > > > +           .fw = {
> > > > +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > +           },
> > > > +           .gmem = SZ_512K,
> > > > +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > +           .init = a6xx_gpu_init,
> > > > +           .zapfw = "a615_zap.mdt",
> > > > +           .hwcg = a615_hwcg,
> > > >     }, {
> > > >             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >             .revn = 619,
> > > > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > >     /* identify gpu: */
> > > >     for (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;
> > > >     }
> > > > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >             config.rev.minor, config.rev.patchid);
> > > >     priv->is_a2xx = config.rev.core == 2;
> > > > +   priv->has_cached_coherent =
> > > > +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > >     gpu = info->init(drm);
> > > >     if (IS_ERR(gpu)) {
> > > > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >     if (ret)
> > > >             return ret;
> > > > -   priv->has_cached_coherent =
> > > > -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > > -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > > -
> > > >     return 0;
> > > >   }
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > index e08d41337169..d5335b99c64c 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > >   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > >   struct adreno_info {
> > > > +   const char *machine;
> > > >     struct adreno_rev rev;
> > > >     uint32_t revn;
> > > >     const char *fw[ADRENO_FW_MAX];
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >

Rob Clark July 26, 2023, 8:11 p.m. UTC | #7

On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> >
> > On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > >
> > > On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > >
> > > > On 07/07/2023 00:10, Rob Clark wrote:
> > > > > From: Rob Clark <[e-mail]>
> > > > >
> > > > > There are cases where there are differences due to SoC integration.
> > > > > Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > > speedbin mappings.
> > > >
> > > > I have the feeling that we are trying to circumvent the way DT works. I'd
> > > > suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > > using of_device_id::data and then of_device_get_match_data().
> > > >
> > > Just thinking, then how about a unique compatible string which we match
> > > to identify gpu->info and drop chip-id check completely here?
> >
> > Ok, I think we could do this, so something like:
> >
> >   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> >
> > ?
> >
> > It looks like we don't have gpu dt bits upstream yet for either sm4350
> > or sm6375, so I suppose we could get away with this change
>
> I think we can even skip the 619.0 part in the SoC compat string.
> So it will be:
>
> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
>
> In future we can drop the chipid part completely and handle that as a
> part of SoC data:
>
> compatible = "qcom,sm4350-adreno", "qcom,adreno";
>
> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
>

I don't think we can do that, there are cases where the same SoC had
multiple revisions of adreno.  We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a
tiny bit more complex is something like:

   compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

> >
> > BR,
> > -R
> >
> > > -Akhil
> > >
> > > > >
> > > > > Signed-off-by: Rob Clark <[e-mail]>
> > > > > ---
> > > > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > > >   2 files changed, 31 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > index 3c531da417b9..e62bc895a31f 100644
> > > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > > >             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > >             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > > >             .init = a6xx_gpu_init,
> > > > > +   }, {
> > > > > +           .machine = "qcom,sm4350",
> > > > > +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > +           .revn = 619,
> > > > > +           .fw = {
> > > > > +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > +           },
> > > > > +           .gmem = SZ_512K,
> > > > > +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > +           .init = a6xx_gpu_init,
> > > > > +           .zapfw = "a615_zap.mdt",
> > > > > +           .hwcg = a615_hwcg,
> > > > > +   }, {
> > > > > +           .machine = "qcom,sm6375",
> > > > > +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > +           .revn = 619,
> > > > > +           .fw = {
> > > > > +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > +           },
> > > > > +           .gmem = SZ_512K,
> > > > > +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > +           .init = a6xx_gpu_init,
> > > > > +           .zapfw = "a615_zap.mdt",
> > > > > +           .hwcg = a615_hwcg,
> > > > >     }, {
> > > > >             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > >             .revn = 619,
> > > > > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > > >     /* identify gpu: */
> > > > >     for (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;
> > > > >     }
> > > > > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > >             config.rev.minor, config.rev.patchid);
> > > > >     priv->is_a2xx = config.rev.core == 2;
> > > > > +   priv->has_cached_coherent =
> > > > > +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > > >     gpu = info->init(drm);
> > > > >     if (IS_ERR(gpu)) {
> > > > > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > >     if (ret)
> > > > >             return ret;
> > > > > -   priv->has_cached_coherent =
> > > > > -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > > > -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > > > -
> > > > >     return 0;
> > > > >   }
> > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > index e08d41337169..d5335b99c64c 100644
> > > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > > >   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > > >   struct adreno_info {
> > > > > +   const char *machine;
> > > > >     struct adreno_rev rev;
> > > > >     uint32_t revn;
> > > > >     const char *fw[ADRENO_FW_MAX];
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> > > >
>
>
>
> --
> With best wishes
> Dmitry

Dmitry Baryshkov July 26, 2023, 9:43 p.m. UTC | #8

On 26/07/2023 23:11, Rob Clark wrote:
> On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> <[e-mail]> wrote:
>>
>> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
>>>
>>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
>>>>
>>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
>>>>>
>>>>> On 07/07/2023 00:10, Rob Clark wrote:
>>>>>> From: Rob Clark <[e-mail]>
>>>>>>
>>>>>> There are cases where there are differences due to SoC integration.
>>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
>>>>>> speedbin mappings.
>>>>>
>>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
>>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
>>>>> using of_device_id::data and then of_device_get_match_data().
>>>>>
>>>> Just thinking, then how about a unique compatible string which we match
>>>> to identify gpu->info and drop chip-id check completely here?
>>>
>>> Ok, I think we could do this, so something like:
>>>
>>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
>>>
>>> ?
>>>
>>> It looks like we don't have gpu dt bits upstream yet for either sm4350
>>> or sm6375, so I suppose we could get away with this change
>>
>> I think we can even skip the 619.0 part in the SoC compat string.
>> So it will be:
>>
>> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
>>
>> In future we can drop the chipid part completely and handle that as a
>> part of SoC data:
>>
>> compatible = "qcom,sm4350-adreno", "qcom,adreno";
>>
>> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
>>
> 
> I don't think we can do that, there are cases where the same SoC had
> multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other 
subsystems what we usually do is that we add support only for the latest 
SoC revision (which would probably mean the latest GPU patch revision). 
Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial 
sample
qcom,sm4350-v1-adreno -> 6,1,9,0

>  We could possibly do that with future
> things where we can read the chip-id from fw.
> 
> What we _could_ do at the expense of making the compatible parsing a
> tiny bit more complex is something like:
> 
>     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> 
> BR,
> -R
> 
>>>
>>> BR,
>>> -R
>>>
>>>> -Akhil
>>>>
>>>>>>
>>>>>> Signed-off-by: Rob Clark <[e-mail]>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
>>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
>>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
>>>>>> index 3c531da417b9..e62bc895a31f 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
>>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
>>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
>>>>>>              .init = a6xx_gpu_init,
>>>>>> +   }, {
>>>>>> +           .machine = "qcom,sm4350",
>>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
>>>>>> +           .revn = 619,
>>>>>> +           .fw = {
>>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
>>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
>>>>>> +           },
>>>>>> +           .gmem = SZ_512K,
>>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>>>>> +           .init = a6xx_gpu_init,
>>>>>> +           .zapfw = "a615_zap.mdt",
>>>>>> +           .hwcg = a615_hwcg,
>>>>>> +   }, {
>>>>>> +           .machine = "qcom,sm6375",
>>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
>>>>>> +           .revn = 619,
>>>>>> +           .fw = {
>>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
>>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
>>>>>> +           },
>>>>>> +           .gmem = SZ_512K,
>>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>>>>> +           .init = a6xx_gpu_init,
>>>>>> +           .zapfw = "a615_zap.mdt",
>>>>>> +           .hwcg = a615_hwcg,
>>>>>>      }, {
>>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
>>>>>>              .revn = 619,
>>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
>>>>>>      /* identify gpu: */
>>>>>>      for (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;
>>>>>>      }
>>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>>>>>>              config.rev.minor, config.rev.patchid);
>>>>>>      priv->is_a2xx = config.rev.core == 2;
>>>>>> +   priv->has_cached_coherent =
>>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
>>>>>>      gpu = info->init(drm);
>>>>>>      if (IS_ERR(gpu)) {
>>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>>>>>>      if (ret)
>>>>>>              return ret;
>>>>>> -   priv->has_cached_coherent =
>>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
>>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
>>>>>> -
>>>>>>      return 0;
>>>>>>    }
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>>>>> index e08d41337169..d5335b99c64c 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
>>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
>>>>>>    struct adreno_info {
>>>>>> +   const char *machine;
>>>>>>      struct adreno_rev rev;
>>>>>>      uint32_t revn;
>>>>>>      const char *fw[ADRENO_FW_MAX];
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>
>>
>>
>> --
>> With best wishes
>> Dmitry

Rob Clark July 26, 2023, 10:03 p.m. UTC | #9

On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On 26/07/2023 23:11, Rob Clark wrote:
> > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> >>
> >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> >>>
> >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> >>>>
> >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> >>>>>
> >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> >>>>>> From: Rob Clark <[e-mail]>
> >>>>>>
> >>>>>> There are cases where there are differences due to SoC integration.
> >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> >>>>>> speedbin mappings.
> >>>>>
> >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> >>>>> using of_device_id::data and then of_device_get_match_data().
> >>>>>
> >>>> Just thinking, then how about a unique compatible string which we match
> >>>> to identify gpu->info and drop chip-id check completely here?
> >>>
> >>> Ok, I think we could do this, so something like:
> >>>
> >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> >>>
> >>> ?
> >>>
> >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> >>> or sm6375, so I suppose we could get away with this change
> >>
> >> I think we can even skip the 619.0 part in the SoC compat string.
> >> So it will be:
> >>
> >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> >>
> >> In future we can drop the chipid part completely and handle that as a
> >> part of SoC data:
> >>
> >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> >>
> >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> >>
> >
> > I don't think we can do that, there are cases where the same SoC had
> > multiple revisions of adreno.
>
> Is that the case for the production versions of the SoC? In other
> subsystems what we usually do is that we add support only for the latest
> SoC revision (which would probably mean the latest GPU patch revision).
> Previous GPU revisions can be added in the following way (pure example):
>
> qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> sample
> qcom,sm4350-v1-adreno -> 6,1,9,0
>

My recollection was that nexus4 shipped with an early version of 8064
which needed userspace workarounds that later 8064 did not.  Not sure
if that is the only such example, but it is one that userspace needed
to be aware of.

Anyways, future things, it sounds like we'll be able to read the id
from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
so I don't want to change any of the existing compat strings.

BR,
-R

> >  We could possibly do that with future
> > things where we can read the chip-id from fw.
> >
> > What we _could_ do at the expense of making the compatible parsing a
> > tiny bit more complex is something like:
> >
> >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> >
> > BR,
> > -R
> >
> >>>
> >>> BR,
> >>> -R
> >>>
> >>>> -Akhil
> >>>>
> >>>>>>
> >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> >>>>>> ---
> >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >>>>>> index 3c531da417b9..e62bc895a31f 100644
> >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> >>>>>>              .init = a6xx_gpu_init,
> >>>>>> +   }, {
> >>>>>> +           .machine = "qcom,sm4350",
> >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >>>>>> +           .revn = 619,
> >>>>>> +           .fw = {
> >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> >>>>>> +           },
> >>>>>> +           .gmem = SZ_512K,
> >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >>>>>> +           .init = a6xx_gpu_init,
> >>>>>> +           .zapfw = "a615_zap.mdt",
> >>>>>> +           .hwcg = a615_hwcg,
> >>>>>> +   }, {
> >>>>>> +           .machine = "qcom,sm6375",
> >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >>>>>> +           .revn = 619,
> >>>>>> +           .fw = {
> >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> >>>>>> +           },
> >>>>>> +           .gmem = SZ_512K,
> >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >>>>>> +           .init = a6xx_gpu_init,
> >>>>>> +           .zapfw = "a615_zap.mdt",
> >>>>>> +           .hwcg = a615_hwcg,
> >>>>>>      }, {
> >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >>>>>>              .revn = 619,
> >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> >>>>>>      /* identify gpu: */
> >>>>>>      for (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;
> >>>>>>      }
> >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >>>>>>              config.rev.minor, config.rev.patchid);
> >>>>>>      priv->is_a2xx = config.rev.core == 2;
> >>>>>> +   priv->has_cached_coherent =
> >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> >>>>>>      gpu = info->init(drm);
> >>>>>>      if (IS_ERR(gpu)) {
> >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> >>>>>>      if (ret)
> >>>>>>              return ret;
> >>>>>> -   priv->has_cached_coherent =
> >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> >>>>>> -
> >>>>>>      return 0;
> >>>>>>    }
> >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> >>>>>> index e08d41337169..d5335b99c64c 100644
> >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> >>>>>>    struct adreno_info {
> >>>>>> +   const char *machine;
> >>>>>>      struct adreno_rev rev;
> >>>>>>      uint32_t revn;
> >>>>>>      const char *fw[ADRENO_FW_MAX];
> >>>>>
> >>>>> --
> >>>>> With best wishes
> >>>>> Dmitry
> >>>>>
> >>
> >>
> >>
> >> --
> >> With best wishes
> >> Dmitry
>
> --
> With best wishes
> Dmitry
>

Dmitry Baryshkov July 26, 2023, 10:33 p.m. UTC | #10

On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
>
> On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> <[e-mail]> wrote:
> >
> > On 26/07/2023 23:11, Rob Clark wrote:
> > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > <[e-mail]> wrote:
> > >>
> > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> > >>>
> > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > >>>>
> > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > >>>>>
> > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > >>>>>> From: Rob Clark <[e-mail]>
> > >>>>>>
> > >>>>>> There are cases where there are differences due to SoC integration.
> > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > >>>>>> speedbin mappings.
> > >>>>>
> > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > >>>>> using of_device_id::data and then of_device_get_match_data().
> > >>>>>
> > >>>> Just thinking, then how about a unique compatible string which we match
> > >>>> to identify gpu->info and drop chip-id check completely here?
> > >>>
> > >>> Ok, I think we could do this, so something like:
> > >>>
> > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > >>>
> > >>> ?
> > >>>
> > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > >>> or sm6375, so I suppose we could get away with this change
> > >>
> > >> I think we can even skip the 619.0 part in the SoC compat string.
> > >> So it will be:
> > >>
> > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > >>
> > >> In future we can drop the chipid part completely and handle that as a
> > >> part of SoC data:
> > >>
> > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > >>
> > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > >>
> > >
> > > I don't think we can do that, there are cases where the same SoC had
> > > multiple revisions of adreno.
> >
> > Is that the case for the production versions of the SoC? In other
> > subsystems what we usually do is that we add support only for the latest
> > SoC revision (which would probably mean the latest GPU patch revision).
> > Previous GPU revisions can be added in the following way (pure example):
> >
> > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > sample
> > qcom,sm4350-v1-adreno -> 6,1,9,0
> >
>
> My recollection was that nexus4 shipped with an early version of 8064
> which needed userspace workarounds that later 8064 did not.  Not sure
> if that is the only such example, but it is one that userspace needed
> to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

>
> Anyways, future things, it sounds like we'll be able to read the id
> from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

>
> BR,
> -R
>
> > >  We could possibly do that with future
> > > things where we can read the chip-id from fw.
> > >
> > > What we _could_ do at the expense of making the compatible parsing a
> > > tiny bit more complex is something like:
> > >
> > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > >
> > > BR,
> > > -R
> > >
> > >>>
> > >>> BR,
> > >>> -R
> > >>>
> > >>>> -Akhil
> > >>>>
> > >>>>>>
> > >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> > >>>>>> ---
> > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > >>>>>>              .init = a6xx_gpu_init,
> > >>>>>> +   }, {
> > >>>>>> +           .machine = "qcom,sm4350",
> > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > >>>>>> +           .revn = 619,
> > >>>>>> +           .fw = {
> > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > >>>>>> +           },
> > >>>>>> +           .gmem = SZ_512K,
> > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >>>>>> +           .init = a6xx_gpu_init,
> > >>>>>> +           .zapfw = "a615_zap.mdt",
> > >>>>>> +           .hwcg = a615_hwcg,
> > >>>>>> +   }, {
> > >>>>>> +           .machine = "qcom,sm6375",
> > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > >>>>>> +           .revn = 619,
> > >>>>>> +           .fw = {
> > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > >>>>>> +           },
> > >>>>>> +           .gmem = SZ_512K,
> > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >>>>>> +           .init = a6xx_gpu_init,
> > >>>>>> +           .zapfw = "a615_zap.mdt",
> > >>>>>> +           .hwcg = a615_hwcg,
> > >>>>>>      }, {
> > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > >>>>>>              .revn = 619,
> > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > >>>>>>      /* identify gpu: */
> > >>>>>>      for (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;
> > >>>>>>      }
> > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > >>>>>>              config.rev.minor, config.rev.patchid);
> > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > >>>>>> +   priv->has_cached_coherent =
> > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > >>>>>>      gpu = info->init(drm);
> > >>>>>>      if (IS_ERR(gpu)) {
> > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > >>>>>>      if (ret)
> > >>>>>>              return ret;
> > >>>>>> -   priv->has_cached_coherent =
> > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > >>>>>> -
> > >>>>>>      return 0;
> > >>>>>>    }
> > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > >>>>>> index e08d41337169..d5335b99c64c 100644
> > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > >>>>>>    struct adreno_info {
> > >>>>>> +   const char *machine;
> > >>>>>>      struct adreno_rev rev;
> > >>>>>>      uint32_t revn;
> > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > >>>>>
> > >>>>> --
> > >>>>> With best wishes
> > >>>>> Dmitry
> > >>>>>
> > >>
> > >>
> > >>
> > >> --
> > >> With best wishes
> > >> Dmitry
> >
> > --
> > With best wishes
> > Dmitry
> >

Rob Clark July 26, 2023, 10:53 p.m. UTC | #11

On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
> >
> > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> > >
> > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > > <[e-mail]> wrote:
> > > >>
> > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> > > >>>
> > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > > >>>>
> > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > >>>>>
> > > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > > >>>>>> From: Rob Clark <[e-mail]>
> > > >>>>>>
> > > >>>>>> There are cases where there are differences due to SoC integration.
> > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > >>>>>> speedbin mappings.
> > > >>>>>
> > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > >>>>> using of_device_id::data and then of_device_get_match_data().
> > > >>>>>
> > > >>>> Just thinking, then how about a unique compatible string which we match
> > > >>>> to identify gpu->info and drop chip-id check completely here?
> > > >>>
> > > >>> Ok, I think we could do this, so something like:
> > > >>>
> > > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > > >>>
> > > >>> ?
> > > >>>
> > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > >>> or sm6375, so I suppose we could get away with this change
> > > >>
> > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > >> So it will be:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > >>
> > > >> In future we can drop the chipid part completely and handle that as a
> > > >> part of SoC data:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > >>
> > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > >>
> > > >
> > > > I don't think we can do that, there are cases where the same SoC had
> > > > multiple revisions of adreno.
> > >
> > > Is that the case for the production versions of the SoC? In other
> > > subsystems what we usually do is that we add support only for the latest
> > > SoC revision (which would probably mean the latest GPU patch revision).
> > > Previous GPU revisions can be added in the following way (pure example):
> > >
> > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > sample
> > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > >
> >
> > My recollection was that nexus4 shipped with an early version of 8064
> > which needed userspace workarounds that later 8064 did not.  Not sure
> > if that is the only such example, but it is one that userspace needed
> > to be aware of.
>
> Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
>
> And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

I no longer have a n4 that boots.. but if I did both it and the later
ones should work properly if they expose the appropriate chip id

I do still prefer parsing the chip-id out of the compatible.  It
avoids needing separate table entries just to have a different
chip-id.  Maybe the scheme that is used elsewhere makes sense when it
is only the kernel that needs to be aware of the device-id.  And maybe
we could just done matching based on compat-id in userspace as well,
but (a) msm and freedreno pre-date dt, and (b) that ship has already
sailed.

BR,
-R

> >
> > Anyways, future things, it sounds like we'll be able to read the id
> > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > so I don't want to change any of the existing compat strings.
>
> I think so too. Current compat strings should stay.
>
> >
> > BR,
> > -R
> >
> > > >  We could possibly do that with future
> > > > things where we can read the chip-id from fw.
> > > >
> > > > What we _could_ do at the expense of making the compatible parsing a
> > > > tiny bit more complex is something like:
> > > >
> > > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > >
> > > > BR,
> > > > -R
> > > >
> > > >>>
> > > >>> BR,
> > > >>> -R
> > > >>>
> > > >>>> -Akhil
> > > >>>>
> > > >>>>>>
> > > >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> > > >>>>>> ---
> > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > >>>>>>              .init = a6xx_gpu_init,
> > > >>>>>> +   }, {
> > > >>>>>> +           .machine = "qcom,sm4350",
> > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>> +           .revn = 619,
> > > >>>>>> +           .fw = {
> > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > >>>>>> +           },
> > > >>>>>> +           .gmem = SZ_512K,
> > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>> +           .init = a6xx_gpu_init,
> > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > >>>>>> +           .hwcg = a615_hwcg,
> > > >>>>>> +   }, {
> > > >>>>>> +           .machine = "qcom,sm6375",
> > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>> +           .revn = 619,
> > > >>>>>> +           .fw = {
> > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > >>>>>> +           },
> > > >>>>>> +           .gmem = SZ_512K,
> > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>> +           .init = a6xx_gpu_init,
> > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > >>>>>> +           .hwcg = a615_hwcg,
> > > >>>>>>      }, {
> > > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>>              .revn = 619,
> > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > >>>>>>      /* identify gpu: */
> > > >>>>>>      for (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;
> > > >>>>>>      }
> > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >>>>>>              config.rev.minor, config.rev.patchid);
> > > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > > >>>>>> +   priv->has_cached_coherent =
> > > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > >>>>>>      gpu = info->init(drm);
> > > >>>>>>      if (IS_ERR(gpu)) {
> > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >>>>>>      if (ret)
> > > >>>>>>              return ret;
> > > >>>>>> -   priv->has_cached_coherent =
> > > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > >>>>>> -
> > > >>>>>>      return 0;
> > > >>>>>>    }
> > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> index e08d41337169..d5335b99c64c 100644
> > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > >>>>>>    struct adreno_info {
> > > >>>>>> +   const char *machine;
> > > >>>>>>      struct adreno_rev rev;
> > > >>>>>>      uint32_t revn;
> > > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > > >>>>>
> > > >>>>> --
> > > >>>>> With best wishes
> > > >>>>> Dmitry
> > > >>>>>
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >
>
>
>
> --
> With best wishes
> Dmitry

Konrad Dybcio July 27, 2023, 7:51 a.m. UTC | #12

On 27.07.2023 00:53, Rob Clark wrote:
> On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
> <[e-mail]> wrote:
>>
>> On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
>>>
>>> On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
>>> <[e-mail]> wrote:
>>>>
>>>> On 26/07/2023 23:11, Rob Clark wrote:
>>>>> On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
>>>>>>>
>>>>>>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
>>>>>>>>>
>>>>>>>>> On 07/07/2023 00:10, Rob Clark wrote:
>>>>>>>>>> From: Rob Clark <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> There are cases where there are differences due to SoC integration.
>>>>>>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
>>>>>>>>>> speedbin mappings.
>>>>>>>>>
>>>>>>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
>>>>>>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
>>>>>>>>> using of_device_id::data and then of_device_get_match_data().
>>>>>>>>>
>>>>>>>> Just thinking, then how about a unique compatible string which we match
>>>>>>>> to identify gpu->info and drop chip-id check completely here?
>>>>>>>
>>>>>>> Ok, I think we could do this, so something like:
>>>>>>>
>>>>>>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
>>>>>>>
>>>>>>> ?
>>>>>>>
>>>>>>> It looks like we don't have gpu dt bits upstream yet for either sm4350
>>>>>>> or sm6375, so I suppose we could get away with this change
>>>>>>
>>>>>> I think we can even skip the 619.0 part in the SoC compat string.
>>>>>> So it will be:
>>>>>>
>>>>>> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
>>>>>>
>>>>>> In future we can drop the chipid part completely and handle that as a
>>>>>> part of SoC data:
>>>>>>
>>>>>> compatible = "qcom,sm4350-adreno", "qcom,adreno";
>>>>>>
>>>>>> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
>>>>>>
>>>>>
>>>>> I don't think we can do that, there are cases where the same SoC had
>>>>> multiple revisions of adreno.
>>>>
>>>> Is that the case for the production versions of the SoC? In other
>>>> subsystems what we usually do is that we add support only for the latest
>>>> SoC revision (which would probably mean the latest GPU patch revision).
>>>> Previous GPU revisions can be added in the following way (pure example):
>>>>
>>>> qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
>>>> sample
>>>> qcom,sm4350-v1-adreno -> 6,1,9,0
>>>>
>>>
>>> My recollection was that nexus4 shipped with an early version of 8064
>>> which needed userspace workarounds that later 8064 did not.  Not sure
>>> if that is the only such example, but it is one that userspace needed
>>> to be aware of.
>>
>> Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
>>
>> And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.
> 
> I no longer have a n4 that boots.. but if I did both it and the later
> ones should work properly if they expose the appropriate chip id
> 
> I do still prefer parsing the chip-id out of the compatible.  It
> avoids needing separate table entries just to have a different
> chip-id.  Maybe the scheme that is used elsewhere makes sense when it
> is only the kernel that needs to be aware of the device-id.  And maybe
> we could just done matching based on compat-id in userspace as well,
> but (a) msm and freedreno pre-date dt, and (b) that ship has already
> sailed.
I think a per-soc dt would be the better approach..

We could probably solve the revision issue with a socid readout of
the silicon revision and override based on that?

Konrad

Rob Clark July 27, 2023, 2:52 p.m. UTC | #13

On Thu, Jul 27, 2023 at 12:51 AM Konrad Dybcio <[e-mail]> wrote:
>
> On 27.07.2023 00:53, Rob Clark wrote:
> > On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> >>
> >> On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
> >>>
> >>> On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On 26/07/2023 23:11, Rob Clark wrote:
> >>>>> On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> >>>>> <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> >>>>>>>>>
> >>>>>>>>> On 07/07/2023 00:10, Rob Clark wrote:
> >>>>>>>>>> From: Rob Clark <[e-mail]>
> >>>>>>>>>>
> >>>>>>>>>> There are cases where there are differences due to SoC integration.
> >>>>>>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> >>>>>>>>>> speedbin mappings.
> >>>>>>>>>
> >>>>>>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> >>>>>>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> >>>>>>>>> using of_device_id::data and then of_device_get_match_data().
> >>>>>>>>>
> >>>>>>>> Just thinking, then how about a unique compatible string which we match
> >>>>>>>> to identify gpu->info and drop chip-id check completely here?
> >>>>>>>
> >>>>>>> Ok, I think we could do this, so something like:
> >>>>>>>
> >>>>>>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> >>>>>>>
> >>>>>>> ?
> >>>>>>>
> >>>>>>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> >>>>>>> or sm6375, so I suppose we could get away with this change
> >>>>>>
> >>>>>> I think we can even skip the 619.0 part in the SoC compat string.
> >>>>>> So it will be:
> >>>>>>
> >>>>>> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> >>>>>>
> >>>>>> In future we can drop the chipid part completely and handle that as a
> >>>>>> part of SoC data:
> >>>>>>
> >>>>>> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> >>>>>>
> >>>>>> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> >>>>>>
> >>>>>
> >>>>> I don't think we can do that, there are cases where the same SoC had
> >>>>> multiple revisions of adreno.
> >>>>
> >>>> Is that the case for the production versions of the SoC? In other
> >>>> subsystems what we usually do is that we add support only for the latest
> >>>> SoC revision (which would probably mean the latest GPU patch revision).
> >>>> Previous GPU revisions can be added in the following way (pure example):
> >>>>
> >>>> qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> >>>> sample
> >>>> qcom,sm4350-v1-adreno -> 6,1,9,0
> >>>>
> >>>
> >>> My recollection was that nexus4 shipped with an early version of 8064
> >>> which needed userspace workarounds that later 8064 did not.  Not sure
> >>> if that is the only such example, but it is one that userspace needed
> >>> to be aware of.
> >>
> >> Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
> >>
> >> And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.
> >
> > I no longer have a n4 that boots.. but if I did both it and the later
> > ones should work properly if they expose the appropriate chip id
> >
> > I do still prefer parsing the chip-id out of the compatible.  It
> > avoids needing separate table entries just to have a different
> > chip-id.  Maybe the scheme that is used elsewhere makes sense when it
> > is only the kernel that needs to be aware of the device-id.  And maybe
> > we could just done matching based on compat-id in userspace as well,
> > but (a) msm and freedreno pre-date dt, and (b) that ship has already
> > sailed.
> I think a per-soc dt would be the better approach..

The newer rev of apq8064 would need its own dt w/ compatible override
in either case.  But if we continue with the current scheme parsing
the compatible, we don't need an extra gpu table entry for it.

BR,
-R

> We could probably solve the revision issue with a socid readout of
> the silicon revision and override based on that?
>
> Konrad
>

Rob Clark July 27, 2023, 9:13 p.m. UTC | #14

On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
> >
> > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> > >
> > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > > <[e-mail]> wrote:
> > > >>
> > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> > > >>>
> > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > > >>>>
> > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > >>>>>
> > > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > > >>>>>> From: Rob Clark <[e-mail]>
> > > >>>>>>
> > > >>>>>> There are cases where there are differences due to SoC integration.
> > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > >>>>>> speedbin mappings.
> > > >>>>>
> > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > >>>>> using of_device_id::data and then of_device_get_match_data().
> > > >>>>>
> > > >>>> Just thinking, then how about a unique compatible string which we match
> > > >>>> to identify gpu->info and drop chip-id check completely here?
> > > >>>
> > > >>> Ok, I think we could do this, so something like:
> > > >>>
> > > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > > >>>
> > > >>> ?
> > > >>>
> > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > >>> or sm6375, so I suppose we could get away with this change
> > > >>
> > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > >> So it will be:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > >>
> > > >> In future we can drop the chipid part completely and handle that as a
> > > >> part of SoC data:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > >>
> > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > >>
> > > >
> > > > I don't think we can do that, there are cases where the same SoC had
> > > > multiple revisions of adreno.
> > >
> > > Is that the case for the production versions of the SoC? In other
> > > subsystems what we usually do is that we add support only for the latest
> > > SoC revision (which would probably mean the latest GPU patch revision).
> > > Previous GPU revisions can be added in the following way (pure example):
> > >
> > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > sample
> > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > >
> >
> > My recollection was that nexus4 shipped with an early version of 8064
> > which needed userspace workarounds that later 8064 did not.  Not sure
> > if that is the only such example, but it is one that userspace needed
> > to be aware of.
>
> Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
>
> And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

At this point, I'm failing to see why my original solution of just
checking of_machine_is_compatible() is worse ;-)

I mean what is the difference between checking
"qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1".  I wouldn't really
want to use of_match_node() in either case.

BR,
-R

> >
> > Anyways, future things, it sounds like we'll be able to read the id
> > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > so I don't want to change any of the existing compat strings.
>
> I think so too. Current compat strings should stay.
>
> >
> > BR,
> > -R
> >
> > > >  We could possibly do that with future
> > > > things where we can read the chip-id from fw.
> > > >
> > > > What we _could_ do at the expense of making the compatible parsing a
> > > > tiny bit more complex is something like:
> > > >
> > > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > >
> > > > BR,
> > > > -R
> > > >
> > > >>>
> > > >>> BR,
> > > >>> -R
> > > >>>
> > > >>>> -Akhil
> > > >>>>
> > > >>>>>>
> > > >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> > > >>>>>> ---
> > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > >>>>>>              .init = a6xx_gpu_init,
> > > >>>>>> +   }, {
> > > >>>>>> +           .machine = "qcom,sm4350",
> > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>> +           .revn = 619,
> > > >>>>>> +           .fw = {
> > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > >>>>>> +           },
> > > >>>>>> +           .gmem = SZ_512K,
> > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>> +           .init = a6xx_gpu_init,
> > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > >>>>>> +           .hwcg = a615_hwcg,
> > > >>>>>> +   }, {
> > > >>>>>> +           .machine = "qcom,sm6375",
> > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>> +           .revn = 619,
> > > >>>>>> +           .fw = {
> > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > >>>>>> +           },
> > > >>>>>> +           .gmem = SZ_512K,
> > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>> +           .init = a6xx_gpu_init,
> > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > >>>>>> +           .hwcg = a615_hwcg,
> > > >>>>>>      }, {
> > > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>>              .revn = 619,
> > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > >>>>>>      /* identify gpu: */
> > > >>>>>>      for (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;
> > > >>>>>>      }
> > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >>>>>>              config.rev.minor, config.rev.patchid);
> > > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > > >>>>>> +   priv->has_cached_coherent =
> > > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > >>>>>>      gpu = info->init(drm);
> > > >>>>>>      if (IS_ERR(gpu)) {
> > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >>>>>>      if (ret)
> > > >>>>>>              return ret;
> > > >>>>>> -   priv->has_cached_coherent =
> > > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > >>>>>> -
> > > >>>>>>      return 0;
> > > >>>>>>    }
> > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> index e08d41337169..d5335b99c64c 100644
> > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > >>>>>>    struct adreno_info {
> > > >>>>>> +   const char *machine;
> > > >>>>>>      struct adreno_rev rev;
> > > >>>>>>      uint32_t revn;
> > > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > > >>>>>
> > > >>>>> --
> > > >>>>> With best wishes
> > > >>>>> Dmitry
> > > >>>>>
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >
>
>
>
> --
> With best wishes
> Dmitry

Dmitry Baryshkov July 27, 2023, 10:02 p.m. UTC | #15

On Fri, 28 Jul 2023 at 00:13, Rob Clark <[e-mail]> wrote:
>
> On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
> <[e-mail]> wrote:
> >
> > On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
> > >
> > > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> > > <[e-mail]> wrote:
> > > >
> > > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > > > <[e-mail]> wrote:
> > > > >>
> > > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> > > > >>>
> > > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > > > >>>>
> > > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > > >>>>>
> > > > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > > > >>>>>> From: Rob Clark <[e-mail]>
> > > > >>>>>>
> > > > >>>>>> There are cases where there are differences due to SoC integration.
> > > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > >>>>>> speedbin mappings.
> > > > >>>>>
> > > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > > >>>>> using of_device_id::data and then of_device_get_match_data().
> > > > >>>>>
> > > > >>>> Just thinking, then how about a unique compatible string which we match
> > > > >>>> to identify gpu->info and drop chip-id check completely here?
> > > > >>>
> > > > >>> Ok, I think we could do this, so something like:
> > > > >>>
> > > > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > > > >>>
> > > > >>> ?
> > > > >>>
> > > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > > >>> or sm6375, so I suppose we could get away with this change
> > > > >>
> > > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > > >> So it will be:
> > > > >>
> > > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > > >>
> > > > >> In future we can drop the chipid part completely and handle that as a
> > > > >> part of SoC data:
> > > > >>
> > > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > > >>
> > > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > > >>
> > > > >
> > > > > I don't think we can do that, there are cases where the same SoC had
> > > > > multiple revisions of adreno.
> > > >
> > > > Is that the case for the production versions of the SoC? In other
> > > > subsystems what we usually do is that we add support only for the latest
> > > > SoC revision (which would probably mean the latest GPU patch revision).
> > > > Previous GPU revisions can be added in the following way (pure example):
> > > >
> > > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > > sample
> > > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > > >
> > >
> > > My recollection was that nexus4 shipped with an early version of 8064
> > > which needed userspace workarounds that later 8064 did not.  Not sure
> > > if that is the only such example, but it is one that userspace needed
> > > to be aware of.
> >
> > Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
> >
> > And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.
>
> At this point, I'm failing to see why my original solution of just
> checking of_machine_is_compatible() is worse ;-)
>
> I mean what is the difference between checking
> "qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1".  I wouldn't really
> want to use of_match_node() in either case.

I have been proposing to use of_device_get_match_data().

>
> BR,
> -R
>
> > >
> > > Anyways, future things, it sounds like we'll be able to read the id
> > > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > > so I don't want to change any of the existing compat strings.
> >
> > I think so too. Current compat strings should stay.
> >
> > >
> > > BR,
> > > -R
> > >
> > > > >  We could possibly do that with future
> > > > > things where we can read the chip-id from fw.
> > > > >
> > > > > What we _could_ do at the expense of making the compatible parsing a
> > > > > tiny bit more complex is something like:
> > > > >
> > > > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > > >
> > > > > BR,
> > > > > -R
> > > > >
> > > > >>>
> > > > >>> BR,
> > > > >>> -R
> > > > >>>
> > > > >>>> -Akhil
> > > > >>>>
> > > > >>>>>>
> > > > >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> > > > >>>>>> ---
> > > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > > > >>>>>>
> > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > > >>>>>>              .init = a6xx_gpu_init,
> > > > >>>>>> +   }, {
> > > > >>>>>> +           .machine = "qcom,sm4350",
> > > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > >>>>>> +           .revn = 619,
> > > > >>>>>> +           .fw = {
> > > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > >>>>>> +           },
> > > > >>>>>> +           .gmem = SZ_512K,
> > > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > >>>>>> +           .init = a6xx_gpu_init,
> > > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > > >>>>>> +           .hwcg = a615_hwcg,
> > > > >>>>>> +   }, {
> > > > >>>>>> +           .machine = "qcom,sm6375",
> > > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > >>>>>> +           .revn = 619,
> > > > >>>>>> +           .fw = {
> > > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > >>>>>> +           },
> > > > >>>>>> +           .gmem = SZ_512K,
> > > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > >>>>>> +           .init = a6xx_gpu_init,
> > > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > > >>>>>> +           .hwcg = a615_hwcg,
> > > > >>>>>>      }, {
> > > > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > >>>>>>              .revn = 619,
> > > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > > >>>>>>      /* identify gpu: */
> > > > >>>>>>      for (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;
> > > > >>>>>>      }
> > > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > >>>>>>              config.rev.minor, config.rev.patchid);
> > > > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > > > >>>>>> +   priv->has_cached_coherent =
> > > > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > > >>>>>>      gpu = info->init(drm);
> > > > >>>>>>      if (IS_ERR(gpu)) {
> > > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > >>>>>>      if (ret)
> > > > >>>>>>              return ret;
> > > > >>>>>> -   priv->has_cached_coherent =
> > > > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > > >>>>>> -
> > > > >>>>>>      return 0;
> > > > >>>>>>    }
> > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > >>>>>> index e08d41337169..d5335b99c64c 100644
> > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > > >>>>>>    struct adreno_info {
> > > > >>>>>> +   const char *machine;
> > > > >>>>>>      struct adreno_rev rev;
> > > > >>>>>>      uint32_t revn;
> > > > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > > > >>>>>
> > > > >>>>> --
> > > > >>>>> With best wishes
> > > > >>>>> Dmitry
> > > > >>>>>
> > > > >>
> > > > >>
> > > > >>
> > > > >> --
> > > > >> With best wishes
> > > > >> Dmitry
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> > > >
> >
> >
> >
> > --
> > With best wishes
> > Dmitry

Rob Clark July 28, 2023, 2:43 p.m. UTC | #16

On Thu, Jul 27, 2023 at 3:02 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Fri, 28 Jul 2023 at 00:13, Rob Clark <[e-mail]> wrote:
> >
> > On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
> > <[e-mail]> wrote:
> > >
> > > On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
> > > >
> > > > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > > > > <[e-mail]> wrote:
> > > > > >>
> > > > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> > > > > >>>
> > > > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > > > > >>>>
> > > > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > > > >>>>>
> > > > > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > > > > >>>>>> From: Rob Clark <[e-mail]>
> > > > > >>>>>>
> > > > > >>>>>> There are cases where there are differences due to SoC integration.
> > > > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > > >>>>>> speedbin mappings.
> > > > > >>>>>
> > > > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > > > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > > > >>>>> using of_device_id::data and then of_device_get_match_data().
> > > > > >>>>>
> > > > > >>>> Just thinking, then how about a unique compatible string which we match
> > > > > >>>> to identify gpu->info and drop chip-id check completely here?
> > > > > >>>
> > > > > >>> Ok, I think we could do this, so something like:
> > > > > >>>
> > > > > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > > > > >>>
> > > > > >>> ?
> > > > > >>>
> > > > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > > > >>> or sm6375, so I suppose we could get away with this change
> > > > > >>
> > > > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > > > >> So it will be:
> > > > > >>
> > > > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > > > >>
> > > > > >> In future we can drop the chipid part completely and handle that as a
> > > > > >> part of SoC data:
> > > > > >>
> > > > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > > > >>
> > > > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > > > >>
> > > > > >
> > > > > > I don't think we can do that, there are cases where the same SoC had
> > > > > > multiple revisions of adreno.
> > > > >
> > > > > Is that the case for the production versions of the SoC? In other
> > > > > subsystems what we usually do is that we add support only for the latest
> > > > > SoC revision (which would probably mean the latest GPU patch revision).
> > > > > Previous GPU revisions can be added in the following way (pure example):
> > > > >
> > > > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > > > sample
> > > > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > > > >
> > > >
> > > > My recollection was that nexus4 shipped with an early version of 8064
> > > > which needed userspace workarounds that later 8064 did not.  Not sure
> > > > if that is the only such example, but it is one that userspace needed
> > > > to be aware of.
> > >
> > > Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
> > >
> > > And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.
> >
> > At this point, I'm failing to see why my original solution of just
> > checking of_machine_is_compatible() is worse ;-)
> >
> > I mean what is the difference between checking
> > "qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1".  I wouldn't really
> > want to use of_match_node() in either case.
>
> I have been proposing to use of_device_get_match_data().

That has the same limitation when it comes to our needs.. our current
setup lets us have a single table entry that matches multiple
chip-id's.  I don't really see the point of using of_match_node() or
of_device_get_match_data() just for the sake of using them.

BR,
-R

> >
> > BR,
> > -R
> >
> > > >
> > > > Anyways, future things, it sounds like we'll be able to read the id
> > > > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > > > so I don't want to change any of the existing compat strings.
> > >
> > > I think so too. Current compat strings should stay.
> > >
> > > >
> > > > BR,
> > > > -R
> > > >
> > > > > >  We could possibly do that with future
> > > > > > things where we can read the chip-id from fw.
> > > > > >
> > > > > > What we _could_ do at the expense of making the compatible parsing a
> > > > > > tiny bit more complex is something like:
> > > > > >
> > > > > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > > > >
> > > > > > BR,
> > > > > > -R
> > > > > >
> > > > > >>>
> > > > > >>> BR,
> > > > > >>> -R
> > > > > >>>
> > > > > >>>> -Akhil
> > > > > >>>>
> > > > > >>>>>>
> > > > > >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> > > > > >>>>>> ---
> > > > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > > > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > > > > >>>>>>
> > > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > > > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > > > >>>>>>              .init = a6xx_gpu_init,
> > > > > >>>>>> +   }, {
> > > > > >>>>>> +           .machine = "qcom,sm4350",
> > > > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > >>>>>> +           .revn = 619,
> > > > > >>>>>> +           .fw = {
> > > > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > >>>>>> +           },
> > > > > >>>>>> +           .gmem = SZ_512K,
> > > > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > >>>>>> +           .init = a6xx_gpu_init,
> > > > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > > > >>>>>> +           .hwcg = a615_hwcg,
> > > > > >>>>>> +   }, {
> > > > > >>>>>> +           .machine = "qcom,sm6375",
> > > > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > >>>>>> +           .revn = 619,
> > > > > >>>>>> +           .fw = {
> > > > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > >>>>>> +           },
> > > > > >>>>>> +           .gmem = SZ_512K,
> > > > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > >>>>>> +           .init = a6xx_gpu_init,
> > > > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > > > >>>>>> +           .hwcg = a615_hwcg,
> > > > > >>>>>>      }, {
> > > > > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > >>>>>>              .revn = 619,
> > > > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > > > >>>>>>      /* identify gpu: */
> > > > > >>>>>>      for (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;
> > > > > >>>>>>      }
> > > > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > > >>>>>>              config.rev.minor, config.rev.patchid);
> > > > > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > > > > >>>>>> +   priv->has_cached_coherent =
> > > > > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > > > >>>>>>      gpu = info->init(drm);
> > > > > >>>>>>      if (IS_ERR(gpu)) {
> > > > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > > >>>>>>      if (ret)
> > > > > >>>>>>              return ret;
> > > > > >>>>>> -   priv->has_cached_coherent =
> > > > > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > > > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > > > >>>>>> -
> > > > > >>>>>>      return 0;
> > > > > >>>>>>    }
> > > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > >>>>>> index e08d41337169..d5335b99c64c 100644
> > > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > > > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > > > >>>>>>    struct adreno_info {
> > > > > >>>>>> +   const char *machine;
> > > > > >>>>>>      struct adreno_rev rev;
> > > > > >>>>>>      uint32_t revn;
> > > > > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > > > > >>>>>
> > > > > >>>>> --
> > > > > >>>>> With best wishes
> > > > > >>>>> Dmitry
> > > > > >>>>>
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> --
> > > > > >> With best wishes
> > > > > >> Dmitry
> > > > >
> > > > > --
> > > > > With best wishes
> > > > > Dmitry
> > > > >
> > >
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
>
>
> --
> With best wishes
> Dmitry

Dmitry Baryshkov July 28, 2023, 2:51 p.m. UTC | #17

On Fri, 28 Jul 2023 at 17:43, Rob Clark <[e-mail]> wrote:
>
> On Thu, Jul 27, 2023 at 3:02 PM Dmitry Baryshkov
> <[e-mail]> wrote:
> >
> > On Fri, 28 Jul 2023 at 00:13, Rob Clark <[e-mail]> wrote:
> > >
> > > On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
> > > <[e-mail]> wrote:
> > > >
> > > > On Thu, 27 Jul 2023 at 01:04, Rob Clark <[e-mail]> wrote:
> > > > >
> > > > > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > > > > > <[e-mail]> wrote:
> > > > > > >>
> > > > > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <[e-mail]> wrote:
> > > > > > >>>
> > > > > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <[e-mail]> wrote:
> > > > > > >>>>
> > > > > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > > > > >>>>>
> > > > > > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > > > > > >>>>>> From: Rob Clark <[e-mail]>
> > > > > > >>>>>>
> > > > > > >>>>>> There are cases where there are differences due to SoC integration.
> > > > > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > > > >>>>>> speedbin mappings.
> > > > > > >>>>>
> > > > > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > > > > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > > > > >>>>> using of_device_id::data and then of_device_get_match_data().
> > > > > > >>>>>
> > > > > > >>>> Just thinking, then how about a unique compatible string which we match
> > > > > > >>>> to identify gpu->info and drop chip-id check completely here?
> > > > > > >>>
> > > > > > >>> Ok, I think we could do this, so something like:
> > > > > > >>>
> > > > > > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > > > > > >>>
> > > > > > >>> ?
> > > > > > >>>
> > > > > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > > > > >>> or sm6375, so I suppose we could get away with this change
> > > > > > >>
> > > > > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > > > > >> So it will be:
> > > > > > >>
> > > > > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > > > > >>
> > > > > > >> In future we can drop the chipid part completely and handle that as a
> > > > > > >> part of SoC data:
> > > > > > >>
> > > > > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > > > > >>
> > > > > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > > > > >>
> > > > > > >
> > > > > > > I don't think we can do that, there are cases where the same SoC had
> > > > > > > multiple revisions of adreno.
> > > > > >
> > > > > > Is that the case for the production versions of the SoC? In other
> > > > > > subsystems what we usually do is that we add support only for the latest
> > > > > > SoC revision (which would probably mean the latest GPU patch revision).
> > > > > > Previous GPU revisions can be added in the following way (pure example):
> > > > > >
> > > > > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > > > > sample
> > > > > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > > > > >
> > > > >
> > > > > My recollection was that nexus4 shipped with an early version of 8064
> > > > > which needed userspace workarounds that later 8064 did not.  Not sure
> > > > > if that is the only such example, but it is one that userspace needed
> > > > > to be aware of.
> > > >
> > > > Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
> > > >
> > > > And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.
> > >
> > > At this point, I'm failing to see why my original solution of just
> > > checking of_machine_is_compatible() is worse ;-)
> > >
> > > I mean what is the difference between checking
> > > "qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1".  I wouldn't really
> > > want to use of_match_node() in either case.
> >
> > I have been proposing to use of_device_get_match_data().
>
> That has the same limitation when it comes to our needs.. our current
> setup lets us have a single table entry that matches multiple
> chip-id's.  I don't really see the point of using of_match_node() or
> of_device_get_match_data() just for the sake of using them.

My point was to be able to use SoC compat strings as the rest of the
kernel does.
But if you feel that the current approach fits our needs, let it be so.

>
> BR,
> -R
>
> > >
> > > BR,
> > > -R
> > >
> > > > >
> > > > > Anyways, future things, it sounds like we'll be able to read the id
> > > > > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > > > > so I don't want to change any of the existing compat strings.
> > > >
> > > > I think so too. Current compat strings should stay.
> > > >
> > > > >
> > > > > BR,
> > > > > -R
> > > > >
> > > > > > >  We could possibly do that with future
> > > > > > > things where we can read the chip-id from fw.
> > > > > > >
> > > > > > > What we _could_ do at the expense of making the compatible parsing a
> > > > > > > tiny bit more complex is something like:
> > > > > > >
> > > > > > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > > > > >
> > > > > > > BR,
> > > > > > > -R
> > > > > > >
> > > > > > >>>
> > > > > > >>> BR,
> > > > > > >>> -R
> > > > > > >>>
> > > > > > >>>> -Akhil
> > > > > > >>>>
> > > > > > >>>>>>
> > > > > > >>>>>> Signed-off-by: Rob Clark <[e-mail]>
> > > > > > >>>>>> ---
> > > > > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > > > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > > > > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > > > > > >>>>>>
> > > > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > > > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > > > > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > > > > >>>>>>              .init = a6xx_gpu_init,
> > > > > > >>>>>> +   }, {
> > > > > > >>>>>> +           .machine = "qcom,sm4350",
> > > > > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > > >>>>>> +           .revn = 619,
> > > > > > >>>>>> +           .fw = {
> > > > > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > > >>>>>> +           },
> > > > > > >>>>>> +           .gmem = SZ_512K,
> > > > > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > > >>>>>> +           .init = a6xx_gpu_init,
> > > > > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > > > > >>>>>> +           .hwcg = a615_hwcg,
> > > > > > >>>>>> +   }, {
> > > > > > >>>>>> +           .machine = "qcom,sm6375",
> > > > > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > > >>>>>> +           .revn = 619,
> > > > > > >>>>>> +           .fw = {
> > > > > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > > >>>>>> +           },
> > > > > > >>>>>> +           .gmem = SZ_512K,
> > > > > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > > >>>>>> +           .init = a6xx_gpu_init,
> > > > > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > > > > >>>>>> +           .hwcg = a615_hwcg,
> > > > > > >>>>>>      }, {
> > > > > > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > > >>>>>>              .revn = 619,
> > > > > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > > > > >>>>>>      /* identify gpu: */
> > > > > > >>>>>>      for (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;
> > > > > > >>>>>>      }
> > > > > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > > > >>>>>>              config.rev.minor, config.rev.patchid);
> > > > > > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > > > > > >>>>>> +   priv->has_cached_coherent =
> > > > > > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > > > > >>>>>>      gpu = info->init(drm);
> > > > > > >>>>>>      if (IS_ERR(gpu)) {
> > > > > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > > > > >>>>>>      if (ret)
> > > > > > >>>>>>              return ret;
> > > > > > >>>>>> -   priv->has_cached_coherent =
> > > > > > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > > > > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > > > > >>>>>> -
> > > > > > >>>>>>      return 0;
> > > > > > >>>>>>    }
> > > > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > > >>>>>> index e08d41337169..d5335b99c64c 100644
> > > > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > > > > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > > > > >>>>>>    struct adreno_info {
> > > > > > >>>>>> +   const char *machine;
> > > > > > >>>>>>      struct adreno_rev rev;
> > > > > > >>>>>>      uint32_t revn;
> > > > > > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > > > > > >>>>>
> > > > > > >>>>> --
> > > > > > >>>>> With best wishes
> > > > > > >>>>> Dmitry
> > > > > > >>>>>
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >> --
> > > > > > >> With best wishes
> > > > > > >> Dmitry
> > > > > >
> > > > > > --
> > > > > > With best wishes
> > > > > > Dmitry
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> >
> >
> >
> > --
> > With best wishes
> > Dmitry

Patch

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 3c531da417b9..e62bc895a31f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -258,6 +258,32 @@  static const struct adreno_info gpulist[] = {
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
 		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
+	}, {
+		.machine = "qcom,sm4350",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
+	}, {
+		.machine = "qcom,sm6375",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
 	}, {
 		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
 		.revn = 619,
@@ -409,6 +435,8 @@  const struct adreno_info *adreno_info(struct adreno_rev rev)
 	/* identify gpu: */
 	for (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;
 	}
@@ -563,6 +591,8 @@  static int adreno_bind(struct device *dev, struct device *master, void *data)
 		config.rev.minor, config.rev.patchid);
 
 	priv->is_a2xx = config.rev.core == 2;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
 
 	gpu = info->init(drm);
 	if (IS_ERR(gpu)) {
@@ -574,10 +604,6 @@  static int adreno_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	priv->has_cached_coherent =
-		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index e08d41337169..d5335b99c64c 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -61,6 +61,7 @@  extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
 extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
 
 struct adreno_info {
+	const char *machine;
 	struct adreno_rev rev;
 	uint32_t revn;
 	const char *fw[ADRENO_FW_MAX];