| Message ID | 20240625-topic-smem_speedbin-v4-1-f6f8493ab814@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SMEM-based speedbin matching | expand |
On Tue, Jun 25, 2024 at 08:28:06PM +0200, Konrad Dybcio wrote: > On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is > abstracted through SMEM, instead of being directly available in a fuse. > > Add support for SMEM-based speed binning, which includes getting > "feature code" and "product code" from said source and parsing them > to form something that lets us match OPPs against. > > Due to the product code being ignored in the context of Adreno on > production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++--- > drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++ > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++++--- > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 ++++- > 4 files changed, 50 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index c98cdb1e9326..8ace096bb68c 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -2124,13 +2124,15 @@ static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse) > return UINT_MAX; > } > > -static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *info) > +static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu, > + struct device *dev, > + const struct adreno_info *info) > { > u32 supp_hw; > u32 speedbin; > int ret; > > - ret = adreno_read_speedbin(dev, &speedbin); > + ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin); > /* > * -ENOENT means that the platform doesn't support speedbin which is > * fine > @@ -2290,7 +2292,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev) > > a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx); > > - ret = a6xx_set_supported_hw(&pdev->dev, config->info); > + ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info); > if (ret) { > a6xx_llc_slices_destroy(a6xx_gpu); > kfree(a6xx_gpu); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index 1e789ff6945e..e514346088f9 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -6,6 +6,8 @@ > * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved. > */ > > +#include <linux/soc/qcom/socinfo.h> > + > #include "adreno_gpu.h" > > bool hang_debug = false; > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index 1c6626747b98..6ffd02f38499 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -21,6 +21,9 @@ > #include "msm_gem.h" > #include "msm_mmu.h" > > +#include <linux/soc/qcom/smem.h> > +#include <linux/soc/qcom/socinfo.h> > + > static u64 address_space_size = 0; > MODULE_PARM_DESC(address_space_size, "Override for size of processes private GPU address space"); > module_param(address_space_size, ullong, 0600); > @@ -1061,9 +1064,39 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > adreno_ocmem->hdl); > } > > -int adreno_read_speedbin(struct device *dev, u32 *speedbin) > +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, > + struct device *dev, u32 *fuse) > { > - return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin); > + u32 fcode; > + int ret; > + > + /* > + * Try reading the speedbin via a nvmem cell first > + * -ENOENT means "no nvmem-cells" and essentially means "old DT" or > + * "nvmem fuse is irrelevant", simply assume it's fine. > + */ > + ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", fuse); > + if (!ret) > + return 0; > + else if (ret != -ENOENT) > + return dev_err_probe(dev, ret, "Couldn't read the speed bin fuse value\n"); > + > +#ifdef CONFIG_QCOM_SMEM > + /* > + * Only check the feature code - the product code only matters for > + * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin > + * matching is concerned. > + * > + * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM. > + */ > + ret = qcom_smem_get_feature_code(&fcode); > + if (!ret) > + *fuse = ADRENO_SKU_ID(fcode); > + else if (ret != -EOPNOTSUPP) > + return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n"); Probably want to update a6xx_set_supported_hw() error handling to ignore -EOPNOTSUPP or do: else /* ret == -EOPNOTSUPP */ return -ENOENT; > +#endif > + > + return 0; I noticed that if SMEM isn't enabled and nvmem returns -ENOENT, we still return 0. That could lead to uninitialized access of speedbin in both users of adreno_read_speedbin(). Maybe: return ret; > } > > int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > @@ -1102,9 +1135,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > devm_pm_opp_set_clkname(dev, "core"); > } > > - if (adreno_read_speedbin(dev, &speedbin) || !speedbin) > + if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin) > speedbin = 0xffff; > - adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin); > + adreno_gpu->speedbin = speedbin; > > gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT, > ADRENO_CHIPID_ARGS(config->chip_id)); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > index cff8ce541d2c..563c08b44624 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > @@ -79,6 +79,10 @@ struct adreno_reglist { > > struct adreno_speedbin { > uint16_t fuse; > +/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */ > +#define ADRENO_SKU_ID_FCODE GENMASK(15, 0) > +#define ADRENO_SKU_ID(fcode) (fcode) > + > uint16_t speedbin; > }; > > @@ -545,7 +549,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags, > struct adreno_smmu_fault_info *info, const char *block, > u32 scratch[4]); > > -int adreno_read_speedbin(struct device *dev, u32 *speedbin); > +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, > + struct device *dev, u32 *speedbin); > > /* > * For a5xx and a6xx targets load the zap shader that is used to pull the GPU > > -- > 2.45.2 > >
On Fri, Jun 28, 2024 at 10:24:52AM -0700, Elliot Berman wrote: > On Tue, Jun 25, 2024 at 08:28:06PM +0200, Konrad Dybcio wrote: > > On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is > > abstracted through SMEM, instead of being directly available in a fuse. > > > > Add support for SMEM-based speed binning, which includes getting > > "feature code" and "product code" from said source and parsing them > > to form something that lets us match OPPs against. > > > > Due to the product code being ignored in the context of Adreno on > > production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN. > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > --- > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++--- > > drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++ > > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++++--- > > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 ++++- > > 4 files changed, 50 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > index c98cdb1e9326..8ace096bb68c 100644 > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > @@ -2124,13 +2124,15 @@ static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse) > > return UINT_MAX; > > } > > > > -static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *info) > > +static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu, > > + struct device *dev, > > + const struct adreno_info *info) > > { > > u32 supp_hw; > > u32 speedbin; > > int ret; > > > > - ret = adreno_read_speedbin(dev, &speedbin); > > + ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin); > > /* > > * -ENOENT means that the platform doesn't support speedbin which is > > * fine > > @@ -2290,7 +2292,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev) > > > > a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx); > > > > - ret = a6xx_set_supported_hw(&pdev->dev, config->info); > > + ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info); > > if (ret) { > > a6xx_llc_slices_destroy(a6xx_gpu); > > kfree(a6xx_gpu); > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > index 1e789ff6945e..e514346088f9 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > @@ -6,6 +6,8 @@ > > * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved. > > */ > > > > +#include <linux/soc/qcom/socinfo.h> > > + > > #include "adreno_gpu.h" > > > > bool hang_debug = false; > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > index 1c6626747b98..6ffd02f38499 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > @@ -21,6 +21,9 @@ > > #include "msm_gem.h" > > #include "msm_mmu.h" > > > > +#include <linux/soc/qcom/smem.h> > > +#include <linux/soc/qcom/socinfo.h> > > + > > static u64 address_space_size = 0; > > MODULE_PARM_DESC(address_space_size, "Override for size of processes private GPU address space"); > > module_param(address_space_size, ullong, 0600); > > @@ -1061,9 +1064,39 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > > adreno_ocmem->hdl); > > } > > > > -int adreno_read_speedbin(struct device *dev, u32 *speedbin) > > +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, > > + struct device *dev, u32 *fuse) > > { > > - return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin); > > + u32 fcode; > > + int ret; > > + > > + /* > > + * Try reading the speedbin via a nvmem cell first > > + * -ENOENT means "no nvmem-cells" and essentially means "old DT" or > > + * "nvmem fuse is irrelevant", simply assume it's fine. > > + */ > > + ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", fuse); > > + if (!ret) > > + return 0; > > + else if (ret != -ENOENT) > > + return dev_err_probe(dev, ret, "Couldn't read the speed bin fuse value\n"); > > + > > +#ifdef CONFIG_QCOM_SMEM > > + /* > > + * Only check the feature code - the product code only matters for > > + * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin > > + * matching is concerned. > > + * > > + * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM. > > + */ > > + ret = qcom_smem_get_feature_code(&fcode); > > + if (!ret) > > + *fuse = ADRENO_SKU_ID(fcode); > > + else if (ret != -EOPNOTSUPP) > > + return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n"); > > Probably want to update a6xx_set_supported_hw() error handling to ignore > -EOPNOTSUPP or do: > > else /* ret == -EOPNOTSUPP */ > return -ENOENT; > > > > > +#endif > > + > > + return 0; > > I noticed that if SMEM isn't enabled and nvmem returns -ENOENT, we still > return 0. That could lead to uninitialized access of speedbin in both > users of adreno_read_speedbin(). Maybe: > > return ret; > Ah, I see patch 4 in the series now, but I wonder if we can do something better so that this patch works without relying on later patch in series? > > } > > > > int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > @@ -1102,9 +1135,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > devm_pm_opp_set_clkname(dev, "core"); > > } > > > > - if (adreno_read_speedbin(dev, &speedbin) || !speedbin) > > + if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin) > > speedbin = 0xffff; > > - adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin); > > + adreno_gpu->speedbin = speedbin; > > > > gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT, > > ADRENO_CHIPID_ARGS(config->chip_id)); > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > > index cff8ce541d2c..563c08b44624 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > > @@ -79,6 +79,10 @@ struct adreno_reglist { > > > > struct adreno_speedbin { > > uint16_t fuse; > > +/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */ > > +#define ADRENO_SKU_ID_FCODE GENMASK(15, 0) > > +#define ADRENO_SKU_ID(fcode) (fcode) > > + > > uint16_t speedbin; > > }; > > > > @@ -545,7 +549,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags, > > struct adreno_smmu_fault_info *info, const char *block, > > u32 scratch[4]); > > > > -int adreno_read_speedbin(struct device *dev, u32 *speedbin); > > +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, > > + struct device *dev, u32 *speedbin); > > > > /* > > * For a5xx and a6xx targets load the zap shader that is used to pull the GPU > > > > -- > > 2.45.2 > > > > >
On 28.06.2024 7:31 PM, Elliot Berman wrote: > On Fri, Jun 28, 2024 at 10:24:52AM -0700, Elliot Berman wrote: >> On Tue, Jun 25, 2024 at 08:28:06PM +0200, Konrad Dybcio wrote: >>> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is >>> abstracted through SMEM, instead of being directly available in a fuse. >>> >>> Add support for SMEM-based speed binning, which includes getting >>> "feature code" and "product code" from said source and parsing them >>> to form something that lets us match OPPs against. >>> >>> Due to the product code being ignored in the context of Adreno on >>> production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN. >>> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>> --- [...] >>> + ret = qcom_smem_get_feature_code(&fcode); >>> + if (!ret) >>> + *fuse = ADRENO_SKU_ID(fcode); >>> + else if (ret != -EOPNOTSUPP) >>> + return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n"); >> >> Probably want to update a6xx_set_supported_hw() error handling to ignore >> -EOPNOTSUPP or do: >> >> else /* ret == -EOPNOTSUPP */ >> return -ENOENT; >> >> >> >>> +#endif >>> + >>> + return 0; >> >> I noticed that if SMEM isn't enabled and nvmem returns -ENOENT, we still >> return 0. That could lead to uninitialized access of speedbin in both >> users of adreno_read_speedbin(). Maybe: >> >> return ret; >> > > Ah, I see patch 4 in the series now, but I wonder if we can do something > better so that this patch works without relying on later patch in > series? Looks like rebase mess on my side :/ Rob already picked this up for next.. Guess we could ask really nicely for a forcepush there? Konrad
On Tue, Jun 25, 2024 at 08:28:06PM +0200, Konrad Dybcio wrote: > On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is > abstracted through SMEM, instead of being directly available in a fuse. > > Add support for SMEM-based speed binning, which includes getting > "feature code" and "product code" from said source and parsing them > to form something that lets us match OPPs against. > > Due to the product code being ignored in the context of Adreno on > production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++--- > drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++ > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++++--- > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 ++++- > 4 files changed, 50 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index c98cdb1e9326..8ace096bb68c 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -2124,13 +2124,15 @@ static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse) > return UINT_MAX; > } > > -static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *info) > +static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu, > + struct device *dev, > + const struct adreno_info *info) > { > u32 supp_hw; > u32 speedbin; > int ret; > > - ret = adreno_read_speedbin(dev, &speedbin); > + ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin); > /* > * -ENOENT means that the platform doesn't support speedbin which is > * fine > @@ -2290,7 +2292,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev) > > a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx); > > - ret = a6xx_set_supported_hw(&pdev->dev, config->info); > + ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info); > if (ret) { > a6xx_llc_slices_destroy(a6xx_gpu); > kfree(a6xx_gpu); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index 1e789ff6945e..e514346088f9 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -6,6 +6,8 @@ > * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved. > */ > > +#include <linux/soc/qcom/socinfo.h> > + > #include "adreno_gpu.h" > > bool hang_debug = false; > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index 1c6626747b98..6ffd02f38499 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -21,6 +21,9 @@ > #include "msm_gem.h" > #include "msm_mmu.h" > > +#include <linux/soc/qcom/smem.h> > +#include <linux/soc/qcom/socinfo.h> > + > static u64 address_space_size = 0; > MODULE_PARM_DESC(address_space_size, "Override for size of processes private GPU address space"); > module_param(address_space_size, ullong, 0600); > @@ -1061,9 +1064,39 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > adreno_ocmem->hdl); > } > > -int adreno_read_speedbin(struct device *dev, u32 *speedbin) > +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, > + struct device *dev, u32 *fuse) > { > - return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin); > + u32 fcode; > + int ret; > + > + /* > + * Try reading the speedbin via a nvmem cell first > + * -ENOENT means "no nvmem-cells" and essentially means "old DT" or > + * "nvmem fuse is irrelevant", simply assume it's fine. > + */ > + ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", fuse); > + if (!ret) > + return 0; > + else if (ret != -ENOENT) > + return dev_err_probe(dev, ret, "Couldn't read the speed bin fuse value\n"); > + > +#ifdef CONFIG_QCOM_SMEM > + /* > + * Only check the feature code - the product code only matters for > + * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin > + * matching is concerned. > + * > + * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM. > + */ > + ret = qcom_smem_get_feature_code(&fcode); > + if (!ret) > + *fuse = ADRENO_SKU_ID(fcode); > + else if (ret != -EOPNOTSUPP) > + return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n"); > +#endif > + > + return 0; > } > > int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > @@ -1102,9 +1135,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > devm_pm_opp_set_clkname(dev, "core"); > } > > - if (adreno_read_speedbin(dev, &speedbin) || !speedbin) > + if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin) > speedbin = 0xffff; > - adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin); > + adreno_gpu->speedbin = speedbin; This value is exposed to userspace via MSM_PARAM_CHIP_ID. 16 bits are reserved for speedbin, so we should ensure somewhere that we don't accidently use more than that. Also, what is the the max value of fcode? I think we should leave some space for pcode too. We never know for sure if that won't be required in future. -Akhil > > gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT, > ADRENO_CHIPID_ARGS(config->chip_id)); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > index cff8ce541d2c..563c08b44624 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > @@ -79,6 +79,10 @@ struct adreno_reglist { > > struct adreno_speedbin { > uint16_t fuse; > +/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */ > +#define ADRENO_SKU_ID_FCODE GENMASK(15, 0) > +#define ADRENO_SKU_ID(fcode) (fcode) > + > uint16_t speedbin; > }; > > @@ -545,7 +549,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags, > struct adreno_smmu_fault_info *info, const char *block, > u32 scratch[4]); > > -int adreno_read_speedbin(struct device *dev, u32 *speedbin); > +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, > + struct device *dev, u32 *speedbin); > > /* > * For a5xx and a6xx targets load the zap shader that is used to pull the GPU > > -- > 2.45.2 >
On 30.06.2024 12:25 PM, Akhil P Oommen wrote: > On Tue, Jun 25, 2024 at 08:28:06PM +0200, Konrad Dybcio wrote: >> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is >> abstracted through SMEM, instead of being directly available in a fuse. >> >> Add support for SMEM-based speed binning, which includes getting >> "feature code" and "product code" from said source and parsing them >> to form something that lets us match OPPs against. >> >> Due to the product code being ignored in the context of Adreno on >> production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- [...] > > This value is exposed to userspace via MSM_PARAM_CHIP_ID. 16 bits are > reserved for speedbin, so we should ensure somewhere that we don't > accidently use more than that. The "real" chip id is 32b, leaving the other 32 for speedbin, so it's fine > > Also, what is the the max value of fcode? I think we should leave some > space for pcode too. We never know for sure if that won't be required in > future. As of today it seems to be 0xff. Worst case scenario we'll add a new param, but hopefully the people that are in charge won't randomly change things.. Konrad
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index c98cdb1e9326..8ace096bb68c 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -2124,13 +2124,15 @@ static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse) return UINT_MAX; } -static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *info) +static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu, + struct device *dev, + const struct adreno_info *info) { u32 supp_hw; u32 speedbin; int ret; - ret = adreno_read_speedbin(dev, &speedbin); + ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin); /* * -ENOENT means that the platform doesn't support speedbin which is * fine @@ -2290,7 +2292,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev) a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx); - ret = a6xx_set_supported_hw(&pdev->dev, config->info); + ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info); if (ret) { a6xx_llc_slices_destroy(a6xx_gpu); kfree(a6xx_gpu); diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 1e789ff6945e..e514346088f9 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -6,6 +6,8 @@ * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved. */ +#include <linux/soc/qcom/socinfo.h> + #include "adreno_gpu.h" bool hang_debug = false; diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 1c6626747b98..6ffd02f38499 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -21,6 +21,9 @@ #include "msm_gem.h" #include "msm_mmu.h" +#include <linux/soc/qcom/smem.h> +#include <linux/soc/qcom/socinfo.h> + static u64 address_space_size = 0; MODULE_PARM_DESC(address_space_size, "Override for size of processes private GPU address space"); module_param(address_space_size, ullong, 0600); @@ -1061,9 +1064,39 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) adreno_ocmem->hdl); } -int adreno_read_speedbin(struct device *dev, u32 *speedbin) +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, + struct device *dev, u32 *fuse) { - return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin); + u32 fcode; + int ret; + + /* + * Try reading the speedbin via a nvmem cell first + * -ENOENT means "no nvmem-cells" and essentially means "old DT" or + * "nvmem fuse is irrelevant", simply assume it's fine. + */ + ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", fuse); + if (!ret) + return 0; + else if (ret != -ENOENT) + return dev_err_probe(dev, ret, "Couldn't read the speed bin fuse value\n"); + +#ifdef CONFIG_QCOM_SMEM + /* + * Only check the feature code - the product code only matters for + * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin + * matching is concerned. + * + * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM. + */ + ret = qcom_smem_get_feature_code(&fcode); + if (!ret) + *fuse = ADRENO_SKU_ID(fcode); + else if (ret != -EOPNOTSUPP) + return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n"); +#endif + + return 0; } int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, @@ -1102,9 +1135,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, devm_pm_opp_set_clkname(dev, "core"); } - if (adreno_read_speedbin(dev, &speedbin) || !speedbin) + if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin) speedbin = 0xffff; - adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin); + adreno_gpu->speedbin = speedbin; gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config->chip_id)); diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h index cff8ce541d2c..563c08b44624 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h @@ -79,6 +79,10 @@ struct adreno_reglist { struct adreno_speedbin { uint16_t fuse; +/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */ +#define ADRENO_SKU_ID_FCODE GENMASK(15, 0) +#define ADRENO_SKU_ID(fcode) (fcode) + uint16_t speedbin; }; @@ -545,7 +549,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags, struct adreno_smmu_fault_info *info, const char *block, u32 scratch[4]); -int adreno_read_speedbin(struct device *dev, u32 *speedbin); +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu, + struct device *dev, u32 *speedbin); /* * For a5xx and a6xx targets load the zap shader that is used to pull the GPU
On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is abstracted through SMEM, instead of being directly available in a fuse. Add support for SMEM-based speed binning, which includes getting "feature code" and "product code" from said source and parsing them to form something that lets us match OPPs against. Due to the product code being ignored in the context of Adreno on production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++--- drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++ drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++++--- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 ++++- 4 files changed, 50 insertions(+), 8 deletions(-)