| Message ID | 20240709-topic-smem_speedbin-v5-1-e2146be0c96f@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SMEM-based speedbin matching | expand |
On Tue, Jul 09, 2024 at 12:45:29PM +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 | 14 +++++----- > drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++ > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 42 +++++++++++++++++++++++++++--- > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 ++++- > 4 files changed, 54 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index bcaec86ac67a..0d8682c28ba4 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -2117,18 +2117,20 @@ 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 > + * -ENOENT/EOPNOTSUPP means that the platform doesn't support speedbin > + * which is fine > */ > - if (ret == -ENOENT) { > + if (ret == -ENOENT || ret == -EOPNOTSUPP) { > return 0; > } else if (ret) { > dev_err_probe(dev, ret, > @@ -2283,7 +2285,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 cfc74a9e2646..0842ea76e616 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..cf6652c4439d 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,40 @@ 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); > + 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 > + u32 fcode; > + > + /* > + * 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 ret; > } > > int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > @@ -1102,9 +1136,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; There are some chipsets which uses both Speedbin and Socinfo data for SKU detection [1]. We don't need to worry about that logic for now. But I am worried about mixing Speedbin and SKU_ID in the UABI with this patch. It will be difficult when we have to expose both to userspace. I think we can use a separate bitfield to expose FCODE/PCODE. Currently, the lower 32 bit is reserved for chipid and 33-48 is reserved for speedbin, so I think we can use the rest of the 16 bits for SKU_ID. And within that 16bits, 12 bits should be sufficient for FCODE and the rest 8 bits reserved for future PCODE. [1] https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/graphics-kernel/-/commit/ab8015dec341d85cd1c97aa7eb5a903527496102 -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 1ab523a163a0..0d629343ebb4 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; > }; > > @@ -555,7 +559,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 15.07.2024 10:04 PM, Akhil P Oommen wrote: > On Tue, Jul 09, 2024 at 12:45:29PM +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> >> --- [...] >> >> - 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; > > There are some chipsets which uses both Speedbin and Socinfo data for > SKU detection [1]. 0_0 > We don't need to worry about that logic for now. But > I am worried about mixing Speedbin and SKU_ID in the UABI with this patch. > It will be difficult when we have to expose both to userspace. > > I think we can use a separate bitfield to expose FCODE/PCODE. Currently, > the lower 32 bit is reserved for chipid and 33-48 is reserved for speedbin, > so I think we can use the rest of the 16 bits for SKU_ID. And within that > 16bits, 12 bits should be sufficient for FCODE and the rest 8 bits > reserved for future PCODE. Right, sounds reasonable. Hopefully nothing overflows.. Konrad
On 16.07.2024 1:56 PM, Konrad Dybcio wrote: > On 15.07.2024 10:04 PM, Akhil P Oommen wrote: >> On Tue, Jul 09, 2024 at 12:45:29PM +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> >>> --- > > [...] > >>> >>> - 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; >> >> There are some chipsets which uses both Speedbin and Socinfo data for >> SKU detection [1]. > > 0_0 > > >> We don't need to worry about that logic for now. But >> I am worried about mixing Speedbin and SKU_ID in the UABI with this patch. >> It will be difficult when we have to expose both to userspace. >> >> I think we can use a separate bitfield to expose FCODE/PCODE. Currently, >> the lower 32 bit is reserved for chipid and 33-48 is reserved for speedbin, >> so I think we can use the rest of the 16 bits for SKU_ID. And within that >> 16bits, 12 bits should be sufficient for FCODE and the rest 8 bits >> reserved for future PCODE. > > Right, sounds reasonable. Hopefully nothing overflows.. +CC Elliot Would you know whether these sizes ^ are going to be sufficient for the foreseeable future? Konrad
On 29.07.2024 2:13 PM, Konrad Dybcio wrote: > On 16.07.2024 1:56 PM, Konrad Dybcio wrote: >> On 15.07.2024 10:04 PM, Akhil P Oommen wrote: >>> On Tue, Jul 09, 2024 at 12:45:29PM +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> >>>> --- >> [...] >> >>>> >>>> - 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; >>> There are some chipsets which uses both Speedbin and Socinfo data for >>> SKU detection [1]. >> 0_0 >> >> >>> We don't need to worry about that logic for now. But >>> I am worried about mixing Speedbin and SKU_ID in the UABI with this patch. >>> It will be difficult when we have to expose both to userspace. >>> >>> I think we can use a separate bitfield to expose FCODE/PCODE. Currently, >>> the lower 32 bit is reserved for chipid and 33-48 is reserved for speedbin, >>> so I think we can use the rest of the 16 bits for SKU_ID. And within that >>> 16bits, 12 bits should be sufficient for FCODE and the rest 8 bits >>> reserved for future PCODE. >> Right, sounds reasonable. Hopefully nothing overflows.. > +CC Elliot > > Would you know whether these sizes ^ are going to be sufficient for > the foreseeable future? Also Akhil, 12 + 8 > 16.. did you mean 8 bits for both P and FCODE? Or 12 for FCODE and 4 for PCODE? Konrad
On Mon, Jul 29, 2024 at 02:40:30PM +0200, Konrad Dybcio wrote: > > > On 29.07.2024 2:13 PM, Konrad Dybcio wrote: > > On 16.07.2024 1:56 PM, Konrad Dybcio wrote: > >> On 15.07.2024 10:04 PM, Akhil P Oommen wrote: > >>> On Tue, Jul 09, 2024 at 12:45:29PM +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> > >>>> --- > >> [...] > >> > >>>> > >>>> - 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; > >>> There are some chipsets which uses both Speedbin and Socinfo data for > >>> SKU detection [1]. > >> 0_0 > >> > >> > >>> We don't need to worry about that logic for now. But > >>> I am worried about mixing Speedbin and SKU_ID in the UABI with this patch. > >>> It will be difficult when we have to expose both to userspace. > >>> > >>> I think we can use a separate bitfield to expose FCODE/PCODE. Currently, > >>> the lower 32 bit is reserved for chipid and 33-48 is reserved for speedbin, > >>> so I think we can use the rest of the 16 bits for SKU_ID. And within that > >>> 16bits, 12 bits should be sufficient for FCODE and the rest 8 bits > >>> reserved for future PCODE. > >> Right, sounds reasonable. Hopefully nothing overflows.. > > +CC Elliot > > > > Would you know whether these sizes ^ are going to be sufficient for > > the foreseeable future? > > Also Akhil, 12 + 8 > 16.. did you mean 8 bits for both P and FCODE? Or > 12 for FCODE and 4 for PCODE? Sorry, "8 bits" was a typo. You are right, 12 bits for Fcode and 4 bits for PCODE. -Akhil > > Konrad
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index bcaec86ac67a..0d8682c28ba4 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -2117,18 +2117,20 @@ 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 + * -ENOENT/EOPNOTSUPP means that the platform doesn't support speedbin + * which is fine */ - if (ret == -ENOENT) { + if (ret == -ENOENT || ret == -EOPNOTSUPP) { return 0; } else if (ret) { dev_err_probe(dev, ret, @@ -2283,7 +2285,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 cfc74a9e2646..0842ea76e616 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..cf6652c4439d 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,40 @@ 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); + 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 + u32 fcode; + + /* + * 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 ret; } int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, @@ -1102,9 +1136,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 1ab523a163a0..0d629343ebb4 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; }; @@ -555,7 +559,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 | 14 +++++----- drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++ drivers/gpu/drm/msm/adreno/adreno_gpu.c | 42 +++++++++++++++++++++++++++--- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 ++++- 4 files changed, 54 insertions(+), 11 deletions(-)