| Message ID | 1605196144-23516-1-git-send-email-akhilpo@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm: adreno: Make speed-bin support generic | expand |
On Thu, Nov 12, 2020 at 09:19:04PM +0530, Akhil P Oommen wrote: > So far a530v2 gpu has support for detecting its supported opps > based on a fuse value called speed-bin. This patch makes this > support generic across gpu families. This is in preparation to > extend speed-bin support to a6x family. > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > --- > This patch is rebased on top of msm-next-staging branch in rob's tree. > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- > drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ > 4 files changed, 80 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index 8fa5c91..7d42321 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { > .get_timestamp = a5xx_get_timestamp, > }; > > -static void check_speed_bin(struct device *dev) > -{ > - struct nvmem_cell *cell; > - u32 val; > - > - /* > - * If the OPP table specifies a opp-supported-hw property then we have > - * to set something with dev_pm_opp_set_supported_hw() or the table > - * doesn't get populated so pick an arbitrary value that should > - * ensure the default frequencies are selected but not conflict with any > - * actual bins > - */ > - val = 0x80; > - > - cell = nvmem_cell_get(dev, "speed_bin"); > - > - if (!IS_ERR(cell)) { > - void *buf = nvmem_cell_read(cell, NULL); > - > - if (!IS_ERR(buf)) { > - u8 bin = *((u8 *) buf); > - > - val = (1 << bin); > - kfree(buf); > - } > - > - nvmem_cell_put(cell); > - } > - > - dev_pm_opp_set_supported_hw(dev, &val, 1); > -} > - > struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > { > struct msm_drm_private *priv = dev->dev_private; > @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > > a5xx_gpu->lm_leakage = 0x4E001A; > > - check_speed_bin(&pdev->dev); > - > ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); > if (ret) { > a5xx_destroy(&(a5xx_gpu->base.base)); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index 87c8b03..e0ff16c 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; > MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); > module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); > > +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; > + > static const struct adreno_info gpulist[] = { > { > .rev = ADRENO_REV(2, 0, 0, 0), > @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { > ADRENO_QUIRK_FAULT_DETECT_MASK, > .init = a5xx_gpu_init, > .zapfw = "a530_zap.mdt", > + .speedbins = a530v2_speedbins, > + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), > }, { > .rev = ADRENO_REV(5, 4, 0, 2), > .revn = 540, > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index f21561d..cdd0c11 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -14,6 +14,7 @@ > #include <linux/pm_opp.h> > #include <linux/slab.h> > #include <linux/soc/qcom/mdt_loader.h> > +#include <linux/nvmem-consumer.h> > #include <soc/qcom/ocmem.h> > #include "adreno_gpu.h" > #include "msm_gem.h" > @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > adreno_ocmem->hdl); > } > > +static int adreno_set_supported_hw(struct device *dev, > + struct adreno_gpu *adreno_gpu) > +{ > + u8 speedbins_count = adreno_gpu->info->speedbins_count; > + const u32 *speedbins = adreno_gpu->info->speedbins; We don't need to make this generic and put it in the table. Just call the function from the target specific code and pass the speedbin array and size from there. > + struct nvmem_cell *cell; > + u32 bin, i; > + u32 val = 0; > + void *buf, *opp_table; > + > + cell = nvmem_cell_get(dev, "speed_bin"); > + /* > + * -ENOENT means that the platform doesn't support speedbin which is > + * fine > + */ > + if (PTR_ERR(cell) == -ENOENT) > + return 0; > + else if (IS_ERR(cell)) > + return PTR_ERR(cell); > + > + /* A speedbin table is must if the platform supports speedbin */ > + if (!speedbins) { > + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); > + return -ENOENT; > + } > + > + buf = nvmem_cell_read(cell, NULL); > + if (IS_ERR(buf)) { > + nvmem_cell_put(cell); > + return PTR_ERR(buf); > + } > + > + bin = *((u32 *) buf); > + > + for (i = 0; i < speedbins_count; i++) { > + if (bin == speedbins[i]) { > + val = (1 << i); > + break; > + } > + } > + > + kfree(buf); > + nvmem_cell_put(cell); > + > + if (!val) { > + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); > + return -ENOENT; > + } > + > + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); > + if (IS_ERR(opp_table)) > + return PTR_ERR(opp_table); > + > + adreno_gpu->opp_table = opp_table; > + return 0; > +} > + > +static void adreno_put_supported_hw(struct opp_table *opp_table) > +{ > + if (opp_table) > + dev_pm_opp_put_supported_hw(opp_table); > +} > + > int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > struct adreno_gpu *adreno_gpu, > const struct adreno_gpu_funcs *funcs, int nr_rings) > @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > struct adreno_platform_config *config = dev->platform_data; > struct msm_gpu_config adreno_gpu_config = { 0 }; > struct msm_gpu *gpu = &adreno_gpu->base; > + int ret; > > adreno_gpu->funcs = funcs; > adreno_gpu->info = adreno_info(config->rev); > @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > adreno_gpu_config.nr_rings = nr_rings; > > + ret = adreno_set_supported_hw(dev, adreno_gpu); > + if (ret) > + return ret; This bit should be in the target specific code > + > adreno_get_pwrlevels(dev, gpu); > > pm_runtime_set_autosuspend_delay(dev, > @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) > > icc_put(gpu->icc_path); > icc_put(gpu->ocmem_icc_path); > + > + adreno_put_supported_hw(adreno_gpu->opp_table); And this bit too, though it would be easier to just call the put function directly without having a intermediate function. Also the OPP function should be NULL aware but thats a different story. Jordan > } > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > index c3775f7..a756ad7 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > @@ -55,6 +55,7 @@ struct adreno_reglist { > }; > > extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; > +extern const u32 a618_speedbins[]; > > struct adreno_info { > struct adreno_rev rev; > @@ -67,6 +68,8 @@ struct adreno_info { > const char *zapfw; > u32 inactive_period; > const struct adreno_reglist *hwcg; > + const u32 *speedbins; > + const u8 speedbins_count; > }; > > const struct adreno_info *adreno_info(struct adreno_rev rev); > @@ -112,6 +115,8 @@ struct adreno_gpu { > * code (a3xx_gpu.c) and stored in this common location. > */ > const unsigned int *reg_offsets; > + > + struct opp_table *opp_table; > }; > #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) > > -- > 2.7.4 >
On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote: > > So far a530v2 gpu has support for detecting its supported opps > based on a fuse value called speed-bin. This patch makes this > support generic across gpu families. This is in preparation to > extend speed-bin support to a6x family. > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > --- > This patch is rebased on top of msm-next-staging branch in rob's tree. > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- > drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ > 4 files changed, 80 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index 8fa5c91..7d42321 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { > .get_timestamp = a5xx_get_timestamp, > }; > > -static void check_speed_bin(struct device *dev) > -{ > - struct nvmem_cell *cell; > - u32 val; > - > - /* > - * If the OPP table specifies a opp-supported-hw property then we have > - * to set something with dev_pm_opp_set_supported_hw() or the table > - * doesn't get populated so pick an arbitrary value that should > - * ensure the default frequencies are selected but not conflict with any > - * actual bins > - */ > - val = 0x80; > - > - cell = nvmem_cell_get(dev, "speed_bin"); > - > - if (!IS_ERR(cell)) { > - void *buf = nvmem_cell_read(cell, NULL); > - > - if (!IS_ERR(buf)) { > - u8 bin = *((u8 *) buf); > - > - val = (1 << bin); > - kfree(buf); > - } > - > - nvmem_cell_put(cell); > - } > - > - dev_pm_opp_set_supported_hw(dev, &val, 1); > -} > - > struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > { > struct msm_drm_private *priv = dev->dev_private; > @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > > a5xx_gpu->lm_leakage = 0x4E001A; > > - check_speed_bin(&pdev->dev); > - > ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); > if (ret) { > a5xx_destroy(&(a5xx_gpu->base.base)); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index 87c8b03..e0ff16c 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; > MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); > module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); > > +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; > + > static const struct adreno_info gpulist[] = { > { > .rev = ADRENO_REV(2, 0, 0, 0), > @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { > ADRENO_QUIRK_FAULT_DETECT_MASK, > .init = a5xx_gpu_init, > .zapfw = "a530_zap.mdt", > + .speedbins = a530v2_speedbins, > + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), > }, { > .rev = ADRENO_REV(5, 4, 0, 2), > .revn = 540, > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index f21561d..cdd0c11 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -14,6 +14,7 @@ > #include <linux/pm_opp.h> > #include <linux/slab.h> > #include <linux/soc/qcom/mdt_loader.h> > +#include <linux/nvmem-consumer.h> > #include <soc/qcom/ocmem.h> > #include "adreno_gpu.h" > #include "msm_gem.h" > @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > adreno_ocmem->hdl); > } > > +static int adreno_set_supported_hw(struct device *dev, > + struct adreno_gpu *adreno_gpu) > +{ > + u8 speedbins_count = adreno_gpu->info->speedbins_count; > + const u32 *speedbins = adreno_gpu->info->speedbins; > + struct nvmem_cell *cell; > + u32 bin, i; > + u32 val = 0; > + void *buf, *opp_table; > + > + cell = nvmem_cell_get(dev, "speed_bin"); > + /* > + * -ENOENT means that the platform doesn't support speedbin which is > + * fine > + */ > + if (PTR_ERR(cell) == -ENOENT) > + return 0; > + else if (IS_ERR(cell)) > + return PTR_ERR(cell); > + > + /* A speedbin table is must if the platform supports speedbin */ > + if (!speedbins) { > + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); > + return -ENOENT; Hmm, this means that hw which supports speed-bin, but for which we haven't yet added a speedbin table, will start failing. Which seems not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so people realize something is missing), but return 0? Or do you think we could add the speed-bin tables for all supported hw immediately? BR, -R > + } > + > + buf = nvmem_cell_read(cell, NULL); > + if (IS_ERR(buf)) { > + nvmem_cell_put(cell); > + return PTR_ERR(buf); > + } > + > + bin = *((u32 *) buf); > + > + for (i = 0; i < speedbins_count; i++) { > + if (bin == speedbins[i]) { > + val = (1 << i); > + break; > + } > + } > + > + kfree(buf); > + nvmem_cell_put(cell); > + > + if (!val) { > + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); > + return -ENOENT; > + } > + > + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); > + if (IS_ERR(opp_table)) > + return PTR_ERR(opp_table); > + > + adreno_gpu->opp_table = opp_table; > + return 0; > +} > + > +static void adreno_put_supported_hw(struct opp_table *opp_table) > +{ > + if (opp_table) > + dev_pm_opp_put_supported_hw(opp_table); > +} > + > int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > struct adreno_gpu *adreno_gpu, > const struct adreno_gpu_funcs *funcs, int nr_rings) > @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > struct adreno_platform_config *config = dev->platform_data; > struct msm_gpu_config adreno_gpu_config = { 0 }; > struct msm_gpu *gpu = &adreno_gpu->base; > + int ret; > > adreno_gpu->funcs = funcs; > adreno_gpu->info = adreno_info(config->rev); > @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > adreno_gpu_config.nr_rings = nr_rings; > > + ret = adreno_set_supported_hw(dev, adreno_gpu); > + if (ret) > + return ret; > + > adreno_get_pwrlevels(dev, gpu); > > pm_runtime_set_autosuspend_delay(dev, > @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) > > icc_put(gpu->icc_path); > icc_put(gpu->ocmem_icc_path); > + > + adreno_put_supported_hw(adreno_gpu->opp_table); > } > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > index c3775f7..a756ad7 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > @@ -55,6 +55,7 @@ struct adreno_reglist { > }; > > extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; > +extern const u32 a618_speedbins[]; > > struct adreno_info { > struct adreno_rev rev; > @@ -67,6 +68,8 @@ struct adreno_info { > const char *zapfw; > u32 inactive_period; > const struct adreno_reglist *hwcg; > + const u32 *speedbins; > + const u8 speedbins_count; > }; > > const struct adreno_info *adreno_info(struct adreno_rev rev); > @@ -112,6 +115,8 @@ struct adreno_gpu { > * code (a3xx_gpu.c) and stored in this common location. > */ > const unsigned int *reg_offsets; > + > + struct opp_table *opp_table; > }; > #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) > > -- > 2.7.4 >
On 11/12/2020 10:05 PM, Jordan Crouse wrote: > On Thu, Nov 12, 2020 at 09:19:04PM +0530, Akhil P Oommen wrote: >> So far a530v2 gpu has support for detecting its supported opps >> based on a fuse value called speed-bin. This patch makes this >> support generic across gpu families. This is in preparation to >> extend speed-bin support to a6x family. >> >> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >> --- >> This patch is rebased on top of msm-next-staging branch in rob's tree. >> >> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- >> drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ >> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ >> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ >> 4 files changed, 80 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> index 8fa5c91..7d42321 100644 >> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { >> .get_timestamp = a5xx_get_timestamp, >> }; >> >> -static void check_speed_bin(struct device *dev) >> -{ >> - struct nvmem_cell *cell; >> - u32 val; >> - >> - /* >> - * If the OPP table specifies a opp-supported-hw property then we have >> - * to set something with dev_pm_opp_set_supported_hw() or the table >> - * doesn't get populated so pick an arbitrary value that should >> - * ensure the default frequencies are selected but not conflict with any >> - * actual bins >> - */ >> - val = 0x80; >> - >> - cell = nvmem_cell_get(dev, "speed_bin"); >> - >> - if (!IS_ERR(cell)) { >> - void *buf = nvmem_cell_read(cell, NULL); >> - >> - if (!IS_ERR(buf)) { >> - u8 bin = *((u8 *) buf); >> - >> - val = (1 << bin); >> - kfree(buf); >> - } >> - >> - nvmem_cell_put(cell); >> - } >> - >> - dev_pm_opp_set_supported_hw(dev, &val, 1); >> -} >> - >> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >> { >> struct msm_drm_private *priv = dev->dev_private; >> @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >> >> a5xx_gpu->lm_leakage = 0x4E001A; >> >> - check_speed_bin(&pdev->dev); >> - >> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); >> if (ret) { >> a5xx_destroy(&(a5xx_gpu->base.base)); >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >> index 87c8b03..e0ff16c 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >> @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; >> MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); >> module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); >> >> +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; >> + >> static const struct adreno_info gpulist[] = { >> { >> .rev = ADRENO_REV(2, 0, 0, 0), >> @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { >> ADRENO_QUIRK_FAULT_DETECT_MASK, >> .init = a5xx_gpu_init, >> .zapfw = "a530_zap.mdt", >> + .speedbins = a530v2_speedbins, >> + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), >> }, { >> .rev = ADRENO_REV(5, 4, 0, 2), >> .revn = 540, >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> index f21561d..cdd0c11 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> @@ -14,6 +14,7 @@ >> #include <linux/pm_opp.h> >> #include <linux/slab.h> >> #include <linux/soc/qcom/mdt_loader.h> >> +#include <linux/nvmem-consumer.h> >> #include <soc/qcom/ocmem.h> >> #include "adreno_gpu.h" >> #include "msm_gem.h" >> @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) >> adreno_ocmem->hdl);jjjj >> } >> >> +static int adreno_set_supported_hw(struct device *dev, >> + struct adreno_gpu *adreno_gpu) >> +{ >> + u8 speedbins_count = adreno_gpu->info->speedbins_count; >> + const u32 *speedbins = adreno_gpu->info->speedbins; > > We don't need to make this generic and put it in the table. Just call the > function from the target specific code and pass the speedbin array and size from > there. > I didn't get you entirely. Do you mean we should avoid keeping speedbin array in the adreno_gpu->info table? -Akhil. >> + struct nvmem_cell *cell; >> + u32 bin, i; >> + u32 val = 0; >> + void *buf, *opp_table; >> + >> + cell = nvmem_cell_get(dev, "speed_bin"); >> + /* >> + * -ENOENT means that the platform doesn't support speedbin which is >> + * fine >> + */ >> + if (PTR_ERR(cell) == -ENOENT) >> + return 0; >> + else if (IS_ERR(cell)) >> + return PTR_ERR(cell); >> + >> + /* A speedbin table is must if the platform supports speedbin */ >> + if (!speedbins) { >> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); >> + return -ENOENT; >> + } >> + >> + buf = nvmem_cell_read(cell, NULL); >> + if (IS_ERR(buf)) { >> + nvmem_cell_put(cell); >> + return PTR_ERR(buf); >> + } >> + >> + bin = *((u32 *) buf); >> + >> + for (i = 0; i < speedbins_count; i++) { >> + if (bin == speedbins[i]) { >> + val = (1 << i); >> + break; >> + } >> + } >> + >> + kfree(buf); >> + nvmem_cell_put(cell); >> + >> + if (!val) { >> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); >> + return -ENOENT; >> + } >> + >> + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); >> + if (IS_ERR(opp_table)) >> + return PTR_ERR(opp_table); >> + >> + adreno_gpu->opp_table = opp_table; >> + return 0; >> +} >> + >> +static void adreno_put_supported_hw(struct opp_table *opp_table) >> +{ >> + if (opp_table) >> + dev_pm_opp_put_supported_hw(opp_table); >> +} >> + >> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >> struct adreno_gpu *adreno_gpu, >> const struct adreno_gpu_funcs *funcs, int nr_rings) >> @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >> struct adreno_platform_config *config = dev->platform_data; >> struct msm_gpu_config adreno_gpu_config = { 0 }; >> struct msm_gpu *gpu = &adreno_gpu->base; >> + int ret; >> >> adreno_gpu->funcs = funcs; >> adreno_gpu->info = adreno_info(config->rev); >> @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >> >> adreno_gpu_config.nr_rings = nr_rings; >> >> + ret = adreno_set_supported_hw(dev, adreno_gpu); >> + if (ret) >> + return ret; > > This bit should be in the target specific code >> + >> adreno_get_pwrlevels(dev, gpu); >> >> pm_runtime_set_autosuspend_delay(dev, >> @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) >> >> icc_put(gpu->icc_path); >> icc_put(gpu->ocmem_icc_path); >> + >> + adreno_put_supported_hw(adreno_gpu->opp_table); > > And this bit too, though it would be easier to just call the put function > directly without having a intermediate function. Also the OPP function should > be NULL aware but thats a different story. > > Jordan >> } >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >> index c3775f7..a756ad7 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >> @@ -55,6 +55,7 @@ struct adreno_reglist { >> }; >> >> extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; >> +extern const u32 a618_speedbins[]; >> >> struct adreno_info { >> struct adreno_rev rev; >> @@ -67,6 +68,8 @@ struct adreno_info { >> const char *zapfw; >> u32 inactive_period; >> const struct adreno_reglist *hwcg; >> + const u32 *speedbins; >> + const u8 speedbins_count; >> }; >> >> const struct adreno_info *adreno_info(struct adreno_rev rev); >> @@ -112,6 +115,8 @@ struct adreno_gpu { >> * code (a3xx_gpu.c) and stored in this common location. >> */ >> const unsigned int *reg_offsets; >> + >> + struct opp_table *opp_table; >> }; >> #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) >> >> -- >> 2.7.4 >> >
On 11/12/2020 10:07 PM, Rob Clark wrote: > On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote: >> >> So far a530v2 gpu has support for detecting its supported opps >> based on a fuse value called speed-bin. This patch makes this >> support generic across gpu families. This is in preparation to >> extend speed-bin support to a6x family. >> >> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >> --- >> This patch is rebased on top of msm-next-staging branch in rob's tree. >> >> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- >> drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ >> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ >> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ >> 4 files changed, 80 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> index 8fa5c91..7d42321 100644 >> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { >> .get_timestamp = a5xx_get_timestamp, >> }; >> >> -static void check_speed_bin(struct device *dev) >> -{ >> - struct nvmem_cell *cell; >> - u32 val; >> - >> - /* >> - * If the OPP table specifies a opp-supported-hw property then we have >> - * to set something with dev_pm_opp_set_supported_hw() or the table >> - * doesn't get populated so pick an arbitrary value that should >> - * ensure the default frequencies are selected but not conflict with any >> - * actual bins >> - */ >> - val = 0x80; >> - >> - cell = nvmem_cell_get(dev, "speed_bin"); >> - >> - if (!IS_ERR(cell)) { >> - void *buf = nvmem_cell_read(cell, NULL); >> - >> - if (!IS_ERR(buf)) { >> - u8 bin = *((u8 *) buf); >> - >> - val = (1 << bin); >> - kfree(buf); >> - } >> - >> - nvmem_cell_put(cell); >> - } >> - >> - dev_pm_opp_set_supported_hw(dev, &val, 1); >> -} >> - >> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >> { >> struct msm_drm_private *priv = dev->dev_private; >> @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >> >> a5xx_gpu->lm_leakage = 0x4E001A; >> >> - check_speed_bin(&pdev->dev); >> - >> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); >> if (ret) { >> a5xx_destroy(&(a5xx_gpu->base.base)); >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >> index 87c8b03..e0ff16c 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >> @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; >> MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); >> module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); >> >> +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; >> + >> static const struct adreno_info gpulist[] = { >> { >> .rev = ADRENO_REV(2, 0, 0, 0), >> @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { >> ADRENO_QUIRK_FAULT_DETECT_MASK, >> .init = a5xx_gpu_init, >> .zapfw = "a530_zap.mdt", >> + .speedbins = a530v2_speedbins, >> + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), >> }, { >> .rev = ADRENO_REV(5, 4, 0, 2), >> .revn = 540, >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> index f21561d..cdd0c11 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >> @@ -14,6 +14,7 @@ >> #include <linux/pm_opp.h> >> #include <linux/slab.h> >> #include <linux/soc/qcom/mdt_loader.h> >> +#include <linux/nvmem-consumer.h> >> #include <soc/qcom/ocmem.h> >> #include "adreno_gpu.h" >> #include "msm_gem.h" >> @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) >> adreno_ocmem->hdl); >> } >> >> +static int adreno_set_supported_hw(struct device *dev, >> + struct adreno_gpu *adreno_gpu) >> +{ >> + u8 speedbins_count = adreno_gpu->info->speedbins_count; >> + const u32 *speedbins = adreno_gpu->info->speedbins; >> + struct nvmem_cell *cell; >> + u32 bin, i; >> + u32 val = 0; >> + void *buf, *opp_table; >> + >> + cell = nvmem_cell_get(dev, "speed_bin"); >> + /* >> + * -ENOENT means that the platform doesn't support speedbin which is >> + * fine >> + */ >> + if (PTR_ERR(cell) == -ENOENT) >> + return 0; >> + else if (IS_ERR(cell)) >> + return PTR_ERR(cell); >> + >> + /* A speedbin table is must if the platform supports speedbin */ >> + if (!speedbins) { >> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); >> + return -ENOENT; > > Hmm, this means that hw which supports speed-bin, but for which we > haven't yet added a speedbin table, will start failing. Which seems > not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so > people realize something is missing), but return 0? We can't because if the gpu opp table has "opp-supported-hw" property, opp driver expects us to call dev_pm_opp_set_supported_hw() to select the supported hardware. I think we can just pick a default one and also print a detailed warning, will that work for you? -Akhil. > > Or do you think we could add the speed-bin tables for all supported hw > immediately? > > BR, > -R > >> + } >> + >> + buf = nvmem_cell_read(cell, NULL); >> + if (IS_ERR(buf)) { >> + nvmem_cell_put(cell); >> + return PTR_ERR(buf); >> + } >> + >> + bin = *((u32 *) buf); >> + >> + for (i = 0; i < speedbins_count; i++) { >> + if (bin == speedbins[i]) { >> + val = (1 << i); >> + break; >> + } >> + } >> + >> + kfree(buf); >> + nvmem_cell_put(cell); >> + >> + if (!val) { >> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); >> + return -ENOENT; >> + } >> + >> + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); >> + if (IS_ERR(opp_table)) >> + return PTR_ERR(opp_table); >> + >> + adreno_gpu->opp_table = opp_table; >> + return 0; >> +} >> + >> +static void adreno_put_supported_hw(struct opp_table *opp_table) >> +{ >> + if (opp_table) >> + dev_pm_opp_put_supported_hw(opp_table); >> +} >> + >> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >> struct adreno_gpu *adreno_gpu, >> const struct adreno_gpu_funcs *funcs, int nr_rings) >> @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >> struct adreno_platform_config *config = dev->platform_data; >> struct msm_gpu_config adreno_gpu_config = { 0 }; >> struct msm_gpu *gpu = &adreno_gpu->base; >> + int ret; >> >> adreno_gpu->funcs = funcs; >> adreno_gpu->info = adreno_info(config->rev); >> @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >> >> adreno_gpu_config.nr_rings = nr_rings; >> >> + ret = adreno_set_supported_hw(dev, adreno_gpu); >> + if (ret) >> + return ret; >> + >> adreno_get_pwrlevels(dev, gpu); >> >> pm_runtime_set_autosuspend_delay(dev, >> @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) >> >> icc_put(gpu->icc_path); >> icc_put(gpu->ocmem_icc_path); >> + >> + adreno_put_supported_hw(adreno_gpu->opp_table); >> } >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >> index c3775f7..a756ad7 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >> @@ -55,6 +55,7 @@ struct adreno_reglist { >> }; >> >> extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; >> +extern const u32 a618_speedbins[]; >> >> struct adreno_info { >> struct adreno_rev rev; >> @@ -67,6 +68,8 @@ struct adreno_info { >> const char *zapfw; >> u32 inactive_period; >> const struct adreno_reglist *hwcg; >> + const u32 *speedbins; >> + const u8 speedbins_count; >> }; >> >> const struct adreno_info *adreno_info(struct adreno_rev rev); >> @@ -112,6 +115,8 @@ struct adreno_gpu { >> * code (a3xx_gpu.c) and stored in this common location. >> */ >> const unsigned int *reg_offsets; >> + >> + struct opp_table *opp_table; >> }; >> #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) >> >> -- >> 2.7.4 >>
On Mon, Nov 16, 2020 at 6:34 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote: > > On 11/12/2020 10:07 PM, Rob Clark wrote: > > On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote: > >> > >> So far a530v2 gpu has support for detecting its supported opps > >> based on a fuse value called speed-bin. This patch makes this > >> support generic across gpu families. This is in preparation to > >> extend speed-bin support to a6x family. > >> > >> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > >> --- > >> This patch is rebased on top of msm-next-staging branch in rob's tree. > >> > >> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- > >> drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ > >> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ > >> 4 files changed, 80 insertions(+), 34 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > >> index 8fa5c91..7d42321 100644 > >> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > >> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > >> @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { > >> .get_timestamp = a5xx_get_timestamp, > >> }; > >> > >> -static void check_speed_bin(struct device *dev) > >> -{ > >> - struct nvmem_cell *cell; > >> - u32 val; > >> - > >> - /* > >> - * If the OPP table specifies a opp-supported-hw property then we have > >> - * to set something with dev_pm_opp_set_supported_hw() or the table > >> - * doesn't get populated so pick an arbitrary value that should > >> - * ensure the default frequencies are selected but not conflict with any > >> - * actual bins > >> - */ > >> - val = 0x80; > >> - > >> - cell = nvmem_cell_get(dev, "speed_bin"); > >> - > >> - if (!IS_ERR(cell)) { > >> - void *buf = nvmem_cell_read(cell, NULL); > >> - > >> - if (!IS_ERR(buf)) { > >> - u8 bin = *((u8 *) buf); > >> - > >> - val = (1 << bin); > >> - kfree(buf); > >> - } > >> - > >> - nvmem_cell_put(cell); > >> - } > >> - > >> - dev_pm_opp_set_supported_hw(dev, &val, 1); > >> -} > >> - > >> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > >> { > >> struct msm_drm_private *priv = dev->dev_private; > >> @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > >> > >> a5xx_gpu->lm_leakage = 0x4E001A; > >> > >> - check_speed_bin(&pdev->dev); > >> - > >> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); > >> if (ret) { > >> a5xx_destroy(&(a5xx_gpu->base.base)); > >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > >> index 87c8b03..e0ff16c 100644 > >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > >> @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; > >> MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); > >> module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); > >> > >> +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; > >> + > >> static const struct adreno_info gpulist[] = { > >> { > >> .rev = ADRENO_REV(2, 0, 0, 0), > >> @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { > >> ADRENO_QUIRK_FAULT_DETECT_MASK, > >> .init = a5xx_gpu_init, > >> .zapfw = "a530_zap.mdt", > >> + .speedbins = a530v2_speedbins, > >> + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), > >> }, { > >> .rev = ADRENO_REV(5, 4, 0, 2), > >> .revn = 540, > >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > >> index f21561d..cdd0c11 100644 > >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > >> @@ -14,6 +14,7 @@ > >> #include <linux/pm_opp.h> > >> #include <linux/slab.h> > >> #include <linux/soc/qcom/mdt_loader.h> > >> +#include <linux/nvmem-consumer.h> > >> #include <soc/qcom/ocmem.h> > >> #include "adreno_gpu.h" > >> #include "msm_gem.h" > >> @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > >> adreno_ocmem->hdl); > >> } > >> > >> +static int adreno_set_supported_hw(struct device *dev, > >> + struct adreno_gpu *adreno_gpu) > >> +{ > >> + u8 speedbins_count = adreno_gpu->info->speedbins_count; > >> + const u32 *speedbins = adreno_gpu->info->speedbins; > >> + struct nvmem_cell *cell; > >> + u32 bin, i; > >> + u32 val = 0; > >> + void *buf, *opp_table; > >> + > >> + cell = nvmem_cell_get(dev, "speed_bin"); > >> + /* > >> + * -ENOENT means that the platform doesn't support speedbin which is > >> + * fine > >> + */ > >> + if (PTR_ERR(cell) == -ENOENT) > >> + return 0; > >> + else if (IS_ERR(cell)) > >> + return PTR_ERR(cell); > >> + > >> + /* A speedbin table is must if the platform supports speedbin */ > >> + if (!speedbins) { > >> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); > >> + return -ENOENT; > > > > Hmm, this means that hw which supports speed-bin, but for which we > > haven't yet added a speedbin table, will start failing. Which seems > > not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so > > people realize something is missing), but return 0? > We can't because if the gpu opp table has "opp-supported-hw" property, > opp driver expects us to call dev_pm_opp_set_supported_hw() to select > the supported hardware. I think we can just pick a default one and also > print a detailed warning, will that work for you? That seems like it could work.. or maybe just skip all this if there is no opp table? BR, -R > -Akhil. > > > > Or do you think we could add the speed-bin tables for all supported hw > > immediately? > > > > BR, > > -R > > > >> + } > >> + > >> + buf = nvmem_cell_read(cell, NULL); > >> + if (IS_ERR(buf)) { > >> + nvmem_cell_put(cell); > >> + return PTR_ERR(buf); > >> + } > >> + > >> + bin = *((u32 *) buf); > >> + > >> + for (i = 0; i < speedbins_count; i++) { > >> + if (bin == speedbins[i]) { > >> + val = (1 << i); > >> + break; > >> + } > >> + } > >> + > >> + kfree(buf); > >> + nvmem_cell_put(cell); > >> + > >> + if (!val) { > >> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); > >> + return -ENOENT; > >> + } > >> + > >> + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); > >> + if (IS_ERR(opp_table)) > >> + return PTR_ERR(opp_table); > >> + > >> + adreno_gpu->opp_table = opp_table; > >> + return 0; > >> +} > >> + > >> +static void adreno_put_supported_hw(struct opp_table *opp_table) > >> +{ > >> + if (opp_table) > >> + dev_pm_opp_put_supported_hw(opp_table); > >> +} > >> + > >> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > >> struct adreno_gpu *adreno_gpu, > >> const struct adreno_gpu_funcs *funcs, int nr_rings) > >> @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > >> struct adreno_platform_config *config = dev->platform_data; > >> struct msm_gpu_config adreno_gpu_config = { 0 }; > >> struct msm_gpu *gpu = &adreno_gpu->base; > >> + int ret; > >> > >> adreno_gpu->funcs = funcs; > >> adreno_gpu->info = adreno_info(config->rev); > >> @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > >> > >> adreno_gpu_config.nr_rings = nr_rings; > >> > >> + ret = adreno_set_supported_hw(dev, adreno_gpu); > >> + if (ret) > >> + return ret; > >> + > >> adreno_get_pwrlevels(dev, gpu); > >> > >> pm_runtime_set_autosuspend_delay(dev, > >> @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) > >> > >> icc_put(gpu->icc_path); > >> icc_put(gpu->ocmem_icc_path); > >> + > >> + adreno_put_supported_hw(adreno_gpu->opp_table); > >> } > >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > >> index c3775f7..a756ad7 100644 > >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > >> @@ -55,6 +55,7 @@ struct adreno_reglist { > >> }; > >> > >> extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; > >> +extern const u32 a618_speedbins[]; > >> > >> struct adreno_info { > >> struct adreno_rev rev; > >> @@ -67,6 +68,8 @@ struct adreno_info { > >> const char *zapfw; > >> u32 inactive_period; > >> const struct adreno_reglist *hwcg; > >> + const u32 *speedbins; > >> + const u8 speedbins_count; > >> }; > >> > >> const struct adreno_info *adreno_info(struct adreno_rev rev); > >> @@ -112,6 +115,8 @@ struct adreno_gpu { > >> * code (a3xx_gpu.c) and stored in this common location. > >> */ > >> const unsigned int *reg_offsets; > >> + > >> + struct opp_table *opp_table; > >> }; > >> #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) > >> > >> -- > >> 2.7.4 > >> >
On Mon, Nov 16, 2020 at 07:40:03PM +0530, Akhil P Oommen wrote: > On 11/12/2020 10:05 PM, Jordan Crouse wrote: > >On Thu, Nov 12, 2020 at 09:19:04PM +0530, Akhil P Oommen wrote: > >>So far a530v2 gpu has support for detecting its supported opps > >>based on a fuse value called speed-bin. This patch makes this > >>support generic across gpu families. This is in preparation to > >>extend speed-bin support to a6x family. > >> > >>Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > >>--- > >>This patch is rebased on top of msm-next-staging branch in rob's tree. > >> > >> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- > >> drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ > >> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ > >> 4 files changed, 80 insertions(+), 34 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > >>index 8fa5c91..7d42321 100644 > >>--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > >>+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > >>@@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { > >> .get_timestamp = a5xx_get_timestamp, > >> }; > >>-static void check_speed_bin(struct device *dev) > >>-{ > >>- struct nvmem_cell *cell; > >>- u32 val; > >>- > >>- /* > >>- * If the OPP table specifies a opp-supported-hw property then we have > >>- * to set something with dev_pm_opp_set_supported_hw() or the table > >>- * doesn't get populated so pick an arbitrary value that should > >>- * ensure the default frequencies are selected but not conflict with any > >>- * actual bins > >>- */ > >>- val = 0x80; > >>- > >>- cell = nvmem_cell_get(dev, "speed_bin"); > >>- > >>- if (!IS_ERR(cell)) { > >>- void *buf = nvmem_cell_read(cell, NULL); > >>- > >>- if (!IS_ERR(buf)) { > >>- u8 bin = *((u8 *) buf); > >>- > >>- val = (1 << bin); > >>- kfree(buf); > >>- } > >>- > >>- nvmem_cell_put(cell); > >>- } > >>- > >>- dev_pm_opp_set_supported_hw(dev, &val, 1); > >>-} > >>- > >> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > >> { > >> struct msm_drm_private *priv = dev->dev_private; > >>@@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > >> a5xx_gpu->lm_leakage = 0x4E001A; > >>- check_speed_bin(&pdev->dev); > >>- > >> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); > >> if (ret) { > >> a5xx_destroy(&(a5xx_gpu->base.base)); > >>diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > >>index 87c8b03..e0ff16c 100644 > >>--- a/drivers/gpu/drm/msm/adreno/adreno_device.c > >>+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > >>@@ -18,6 +18,8 @@ bool snapshot_debugbus = false; > >> MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); > >> module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); > >>+const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; > >>+ > >> static const struct adreno_info gpulist[] = { > >> { > >> .rev = ADRENO_REV(2, 0, 0, 0), > >>@@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { > >> ADRENO_QUIRK_FAULT_DETECT_MASK, > >> .init = a5xx_gpu_init, > >> .zapfw = "a530_zap.mdt", > >>+ .speedbins = a530v2_speedbins, > >>+ .speedbins_count = ARRAY_SIZE(a530v2_speedbins), > >> }, { > >> .rev = ADRENO_REV(5, 4, 0, 2), > >> .revn = 540, > >>diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > >>index f21561d..cdd0c11 100644 > >>--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > >>+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > >>@@ -14,6 +14,7 @@ > >> #include <linux/pm_opp.h> > >> #include <linux/slab.h> > >> #include <linux/soc/qcom/mdt_loader.h> > >>+#include <linux/nvmem-consumer.h> > >> #include <soc/qcom/ocmem.h> > >> #include "adreno_gpu.h" > >> #include "msm_gem.h" > >>@@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) > >> adreno_ocmem->hdl);jjjj > >> } > >>+static int adreno_set_supported_hw(struct device *dev, > >>+ struct adreno_gpu *adreno_gpu) > >>+{ > >>+ u8 speedbins_count = adreno_gpu->info->speedbins_count; > >>+ const u32 *speedbins = adreno_gpu->info->speedbins; > > > >We don't need to make this generic and put it in the table. Just call the > >function from the target specific code and pass the speedbin array and size from > >there. > > > I didn't get you entirely. Do you mean we should avoid keeping speedbin > array in the adreno_gpu->info table? Exactly. Jordan > -Akhil. > >>+ struct nvmem_cell *cell; > >>+ u32 bin, i; > >>+ u32 val = 0; > >>+ void *buf, *opp_table; > >>+ > >>+ cell = nvmem_cell_get(dev, "speed_bin"); > >>+ /* > >>+ * -ENOENT means that the platform doesn't support speedbin which is > >>+ * fine > >>+ */ > >>+ if (PTR_ERR(cell) == -ENOENT) > >>+ return 0; > >>+ else if (IS_ERR(cell)) > >>+ return PTR_ERR(cell); > >>+ > >>+ /* A speedbin table is must if the platform supports speedbin */ > >>+ if (!speedbins) { > >>+ DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); > >>+ return -ENOENT; > >>+ } > >>+ > >>+ buf = nvmem_cell_read(cell, NULL); > >>+ if (IS_ERR(buf)) { > >>+ nvmem_cell_put(cell); > >>+ return PTR_ERR(buf); > >>+ } > >>+ > >>+ bin = *((u32 *) buf); > >>+ > >>+ for (i = 0; i < speedbins_count; i++) { > >>+ if (bin == speedbins[i]) { > >>+ val = (1 << i); > >>+ break; > >>+ } > >>+ } > >>+ > >>+ kfree(buf); > >>+ nvmem_cell_put(cell); > >>+ > >>+ if (!val) { > >>+ DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); > >>+ return -ENOENT; > >>+ } > >>+ > >>+ opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); > >>+ if (IS_ERR(opp_table)) > >>+ return PTR_ERR(opp_table); > >>+ > >>+ adreno_gpu->opp_table = opp_table; > >>+ return 0; > >>+} > >>+ > >>+static void adreno_put_supported_hw(struct opp_table *opp_table) > >>+{ > >>+ if (opp_table) > >>+ dev_pm_opp_put_supported_hw(opp_table); > >>+} > >>+ > >> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > >> struct adreno_gpu *adreno_gpu, > >> const struct adreno_gpu_funcs *funcs, int nr_rings) > >>@@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > >> struct adreno_platform_config *config = dev->platform_data; > >> struct msm_gpu_config adreno_gpu_config = { 0 }; > >> struct msm_gpu *gpu = &adreno_gpu->base; > >>+ int ret; > >> adreno_gpu->funcs = funcs; > >> adreno_gpu->info = adreno_info(config->rev); > >>@@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > >> adreno_gpu_config.nr_rings = nr_rings; > >>+ ret = adreno_set_supported_hw(dev, adreno_gpu); > >>+ if (ret) > >>+ return ret; > > > >This bit should be in the target specific code > >>+ > >> adreno_get_pwrlevels(dev, gpu); > >> pm_runtime_set_autosuspend_delay(dev, > >>@@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) > >> icc_put(gpu->icc_path); > >> icc_put(gpu->ocmem_icc_path); > >>+ > >>+ adreno_put_supported_hw(adreno_gpu->opp_table); > > > >And this bit too, though it would be easier to just call the put function > >directly without having a intermediate function. Also the OPP function should > >be NULL aware but thats a different story. > > > >Jordan > >> } > >>diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > >>index c3775f7..a756ad7 100644 > >>--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > >>+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > >>@@ -55,6 +55,7 @@ struct adreno_reglist { > >> }; > >> extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; > >>+extern const u32 a618_speedbins[]; > >> struct adreno_info { > >> struct adreno_rev rev; > >>@@ -67,6 +68,8 @@ struct adreno_info { > >> const char *zapfw; > >> u32 inactive_period; > >> const struct adreno_reglist *hwcg; > >>+ const u32 *speedbins; > >>+ const u8 speedbins_count; > >> }; > >> const struct adreno_info *adreno_info(struct adreno_rev rev); > >>@@ -112,6 +115,8 @@ struct adreno_gpu { > >> * code (a3xx_gpu.c) and stored in this common location. > >> */ > >> const unsigned int *reg_offsets; > >>+ > >>+ struct opp_table *opp_table; > >> }; > >> #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) > >>-- > >>2.7.4 > >> > > > > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
On 11/16/2020 9:52 PM, Rob Clark wrote: > On Mon, Nov 16, 2020 at 6:34 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote: >> >> On 11/12/2020 10:07 PM, Rob Clark wrote: >>> On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote: >>>> >>>> So far a530v2 gpu has support for detecting its supported opps >>>> based on a fuse value called speed-bin. This patch makes this >>>> support generic across gpu families. This is in preparation to >>>> extend speed-bin support to a6x family. >>>> >>>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >>>> --- >>>> This patch is rebased on top of msm-next-staging branch in rob's tree. >>>> >>>> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- >>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ >>>> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ >>>> 4 files changed, 80 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >>>> index 8fa5c91..7d42321 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >>>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >>>> @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { >>>> .get_timestamp = a5xx_get_timestamp, >>>> }; >>>> >>>> -static void check_speed_bin(struct device *dev) >>>> -{ >>>> - struct nvmem_cell *cell; >>>> - u32 val; >>>> - >>>> - /* >>>> - * If the OPP table specifies a opp-supported-hw property then we have >>>> - * to set something with dev_pm_opp_set_supported_hw() or the table >>>> - * doesn't get populated so pick an arbitrary value that should >>>> - * ensure the default frequencies are selected but not conflict with any >>>> - * actual bins >>>> - */ >>>> - val = 0x80; >>>> - >>>> - cell = nvmem_cell_get(dev, "speed_bin"); >>>> - >>>> - if (!IS_ERR(cell)) { >>>> - void *buf = nvmem_cell_read(cell, NULL); >>>> - >>>> - if (!IS_ERR(buf)) { >>>> - u8 bin = *((u8 *) buf); >>>> - >>>> - val = (1 << bin); >>>> - kfree(buf); >>>> - } >>>> - >>>> - nvmem_cell_put(cell); >>>> - } >>>> - >>>> - dev_pm_opp_set_supported_hw(dev, &val, 1); >>>> -} >>>> - >>>> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >>>> { >>>> struct msm_drm_private *priv = dev->dev_private; >>>> @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >>>> >>>> a5xx_gpu->lm_leakage = 0x4E001A; >>>> >>>> - check_speed_bin(&pdev->dev); >>>> - >>>> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); >>>> if (ret) { >>>> a5xx_destroy(&(a5xx_gpu->base.base)); >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> index 87c8b03..e0ff16c 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; >>>> MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); >>>> module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); >>>> >>>> +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; >>>> + >>>> static const struct adreno_info gpulist[] = { >>>> { >>>> .rev = ADRENO_REV(2, 0, 0, 0), >>>> @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { >>>> ADRENO_QUIRK_FAULT_DETECT_MASK, >>>> .init = a5xx_gpu_init, >>>> .zapfw = "a530_zap.mdt", >>>> + .speedbins = a530v2_speedbins, >>>> + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), >>>> }, { >>>> .rev = ADRENO_REV(5, 4, 0, 2), >>>> .revn = 540, >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>>> index f21561d..cdd0c11 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>>> @@ -14,6 +14,7 @@ >>>> #include <linux/pm_opp.h> >>>> #include <linux/slab.h> >>>> #include <linux/soc/qcom/mdt_loader.h> >>>> +#include <linux/nvmem-consumer.h> >>>> #include <soc/qcom/ocmem.h> >>>> #include "adreno_gpu.h" >>>> #include "msm_gem.h" >>>> @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) >>>> adreno_ocmem->hdl); >>>> } >>>> >>>> +static int adreno_set_supported_hw(struct device *dev, >>>> + struct adreno_gpu *adreno_gpu) >>>> +{ >>>> + u8 speedbins_count = adreno_gpu->info->speedbins_count; >>>> + const u32 *speedbins = adreno_gpu->info->speedbins; >>>> + struct nvmem_cell *cell; >>>> + u32 bin, i; >>>> + u32 val = 0; >>>> + void *buf, *opp_table; >>>> + >>>> + cell = nvmem_cell_get(dev, "speed_bin"); >>>> + /* >>>> + * -ENOENT means that the platform doesn't support speedbin which is >>>> + * fine >>>> + */ >>>> + if (PTR_ERR(cell) == -ENOENT) >>>> + return 0; >>>> + else if (IS_ERR(cell)) >>>> + return PTR_ERR(cell); >>>> + >>>> + /* A speedbin table is must if the platform supports speedbin */ >>>> + if (!speedbins) { >>>> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); >>>> + return -ENOENT; >>> >>> Hmm, this means that hw which supports speed-bin, but for which we >>> haven't yet added a speedbin table, will start failing. Which seems >>> not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so >>> people realize something is missing), but return 0? >> We can't because if the gpu opp table has "opp-supported-hw" property, >> opp driver expects us to call dev_pm_opp_set_supported_hw() to select >> the supported hardware. I think we can just pick a default one and also >> print a detailed warning, will that work for you? > > That seems like it could work.. or maybe just skip all this if there > is no opp table? > > BR, > -R Rob, I will share a new patchset shortly. -Akhil. > >> -Akhil. >>> >>> Or do you think we could add the speed-bin tables for all supported hw >>> immediately? >>> >>> BR, >>> -R >>> >>>> + } >>>> + >>>> + buf = nvmem_cell_read(cell, NULL); >>>> + if (IS_ERR(buf)) { >>>> + nvmem_cell_put(cell); >>>> + return PTR_ERR(buf); >>>> + } >>>> + >>>> + bin = *((u32 *) buf); >>>> + >>>> + for (i = 0; i < speedbins_count; i++) { >>>> + if (bin == speedbins[i]) { >>>> + val = (1 << i); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + kfree(buf); >>>> + nvmem_cell_put(cell); >>>> + >>>> + if (!val) { >>>> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); >>>> + return -ENOENT; >>>> + } >>>> + >>>> + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); >>>> + if (IS_ERR(opp_table)) >>>> + return PTR_ERR(opp_table); >>>> + >>>> + adreno_gpu->opp_table = opp_table; >>>> + return 0; >>>> +} >>>> + >>>> +static void adreno_put_supported_hw(struct opp_table *opp_table) >>>> +{ >>>> + if (opp_table) >>>> + dev_pm_opp_put_supported_hw(opp_table); >>>> +} >>>> + >>>> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >>>> struct adreno_gpu *adreno_gpu, >>>> const struct adreno_gpu_funcs *funcs, int nr_rings) >>>> @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >>>> struct adreno_platform_config *config = dev->platform_data; >>>> struct msm_gpu_config adreno_gpu_config = { 0 }; >>>> struct msm_gpu *gpu = &adreno_gpu->base; >>>> + int ret; >>>> >>>> adreno_gpu->funcs = funcs; >>>> adreno_gpu->info = adreno_info(config->rev); >>>> @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >>>> >>>> adreno_gpu_config.nr_rings = nr_rings; >>>> >>>> + ret = adreno_set_supported_hw(dev, adreno_gpu); >>>> + if (ret) >>>> + return ret; >>>> + >>>> adreno_get_pwrlevels(dev, gpu); >>>> >>>> pm_runtime_set_autosuspend_delay(dev, >>>> @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) >>>> >>>> icc_put(gpu->icc_path); >>>> icc_put(gpu->ocmem_icc_path); >>>> + >>>> + adreno_put_supported_hw(adreno_gpu->opp_table); >>>> } >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >>>> index c3775f7..a756ad7 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >>>> @@ -55,6 +55,7 @@ struct adreno_reglist { >>>> }; >>>> >>>> extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; >>>> +extern const u32 a618_speedbins[]; >>>> >>>> struct adreno_info { >>>> struct adreno_rev rev; >>>> @@ -67,6 +68,8 @@ struct adreno_info { >>>> const char *zapfw; >>>> u32 inactive_period; >>>> const struct adreno_reglist *hwcg; >>>> + const u32 *speedbins; >>>> + const u8 speedbins_count; >>>> }; >>>> >>>> const struct adreno_info *adreno_info(struct adreno_rev rev); >>>> @@ -112,6 +115,8 @@ struct adreno_gpu { >>>> * code (a3xx_gpu.c) and stored in this common location. >>>> */ >>>> const unsigned int *reg_offsets; >>>> + >>>> + struct opp_table *opp_table; >>>> }; >>>> #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) >>>> >>>> -- >>>> 2.7.4 >>>> >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 11/16/2020 10:44 PM, Jordan Crouse wrote: > On Mon, Nov 16, 2020 at 07:40:03PM +0530, Akhil P Oommen wrote: >> On 11/12/2020 10:05 PM, Jordan Crouse wrote: >>> On Thu, Nov 12, 2020 at 09:19:04PM +0530, Akhil P Oommen wrote: >>>> So far a530v2 gpu has support for detecting its supported opps >>>> based on a fuse value called speed-bin. This patch makes this >>>> support generic across gpu families. This is in preparation to >>>> extend speed-bin support to a6x family. >>>> >>>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >>>> --- >>>> This patch is rebased on top of msm-next-staging branch in rob's tree. >>>> >>>> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- >>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ >>>> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ >>>> 4 files changed, 80 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >>>> index 8fa5c91..7d42321 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >>>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >>>> @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { >>>> .get_timestamp = a5xx_get_timestamp, >>>> }; >>>> -static void check_speed_bin(struct device *dev) >>>> -{ >>>> - struct nvmem_cell *cell; >>>> - u32 val; >>>> - >>>> - /* >>>> - * If the OPP table specifies a opp-supported-hw property then we have >>>> - * to set something with dev_pm_opp_set_supported_hw() or the table >>>> - * doesn't get populated so pick an arbitrary value that should >>>> - * ensure the default frequencies are selected but not conflict with any >>>> - * actual bins >>>> - */ >>>> - val = 0x80; >>>> - >>>> - cell = nvmem_cell_get(dev, "speed_bin"); >>>> - >>>> - if (!IS_ERR(cell)) { >>>> - void *buf = nvmem_cell_read(cell, NULL); >>>> - >>>> - if (!IS_ERR(buf)) { >>>> - u8 bin = *((u8 *) buf); >>>> - >>>> - val = (1 << bin); >>>> - kfree(buf); >>>> - } >>>> - >>>> - nvmem_cell_put(cell); >>>> - } >>>> - >>>> - dev_pm_opp_set_supported_hw(dev, &val, 1); >>>> -} >>>> - >>>> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >>>> { >>>> struct msm_drm_private *priv = dev->dev_private; >>>> @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) >>>> a5xx_gpu->lm_leakage = 0x4E001A; >>>> - check_speed_bin(&pdev->dev); >>>> - >>>> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); >>>> if (ret) { >>>> a5xx_destroy(&(a5xx_gpu->base.base)); >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> index 87c8b03..e0ff16c 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; >>>> MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); >>>> module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); >>>> +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; >>>> + >>>> static const struct adreno_info gpulist[] = { >>>> { >>>> .rev = ADRENO_REV(2, 0, 0, 0), >>>> @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { >>>> ADRENO_QUIRK_FAULT_DETECT_MASK, >>>> .init = a5xx_gpu_init, >>>> .zapfw = "a530_zap.mdt", >>>> + .speedbins = a530v2_speedbins, >>>> + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), >>>> }, { >>>> .rev = ADRENO_REV(5, 4, 0, 2), >>>> .revn = 540, >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>>> index f21561d..cdd0c11 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c >>>> @@ -14,6 +14,7 @@ >>>> #include <linux/pm_opp.h> >>>> #include <linux/slab.h> >>>> #include <linux/soc/qcom/mdt_loader.h> >>>> +#include <linux/nvmem-consumer.h> >>>> #include <soc/qcom/ocmem.h> >>>> #include "adreno_gpu.h" >>>> #include "msm_gem.h" >>>> @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) >>>> adreno_ocmem->hdl);jjjj >>>> } >>>> +static int adreno_set_supported_hw(struct device *dev, >>>> + struct adreno_gpu *adreno_gpu) >>>> +{ >>>> + u8 speedbins_count = adreno_gpu->info->speedbins_count; >>>> + const u32 *speedbins = adreno_gpu->info->speedbins; >>> >>> We don't need to make this generic and put it in the table. Just call the >>> function from the target specific code and pass the speedbin array and size from >>> there. >>> >> I didn't get you entirely. Do you mean we should avoid keeping speedbin >> array in the adreno_gpu->info table? > > Exactly. > > Jordan But why duplicate this code if it can be made generic? Could you please check the v2 version? -Akhil. > >> -Akhil. >>>> + struct nvmem_cell *cell; >>>> + u32 bin, i; >>>> + u32 val = 0; >>>> + void *buf, *opp_table; >>>> + >>>> + cell = nvmem_cell_get(dev, "speed_bin"); >>>> + /* >>>> + * -ENOENT means that the platform doesn't support speedbin which is >>>> + * fine >>>> + */ >>>> + if (PTR_ERR(cell) == -ENOENT) >>>> + return 0; >>>> + else if (IS_ERR(cell)) >>>> + return PTR_ERR(cell); >>>> + >>>> + /* A speedbin table is must if the platform supports speedbin */ >>>> + if (!speedbins) { >>>> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); >>>> + return -ENOENT; >>>> + } >>>> + >>>> + buf = nvmem_cell_read(cell, NULL); >>>> + if (IS_ERR(buf)) { >>>> + nvmem_cell_put(cell); >>>> + return PTR_ERR(buf); >>>> + } >>>> + >>>> + bin = *((u32 *) buf); >>>> + >>>> + for (i = 0; i < speedbins_count; i++) { >>>> + if (bin == speedbins[i]) { >>>> + val = (1 << i); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + kfree(buf); >>>> + nvmem_cell_put(cell); >>>> + >>>> + if (!val) { >>>> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); >>>> + return -ENOENT; >>>> + } >>>> + >>>> + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); >>>> + if (IS_ERR(opp_table)) >>>> + return PTR_ERR(opp_table); >>>> + >>>> + adreno_gpu->opp_table = opp_table; >>>> + return 0; >>>> +} >>>> + >>>> +static void adreno_put_supported_hw(struct opp_table *opp_table) >>>> +{ >>>> + if (opp_table) >>>> + dev_pm_opp_put_supported_hw(opp_table); >>>> +} >>>> + >>>> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >>>> struct adreno_gpu *adreno_gpu, >>>> const struct adreno_gpu_funcs *funcs, int nr_rings) >>>> @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >>>> struct adreno_platform_config *config = dev->platform_data; >>>> struct msm_gpu_config adreno_gpu_config = { 0 }; >>>> struct msm_gpu *gpu = &adreno_gpu->base; >>>> + int ret; >>>> adreno_gpu->funcs = funcs; >>>> adreno_gpu->info = adreno_info(config->rev); >>>> @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, >>>> adreno_gpu_config.nr_rings = nr_rings; >>>> + ret = adreno_set_supported_hw(dev, adreno_gpu); >>>> + if (ret) >>>> + return ret; >>> >>> This bit should be in the target specific code >>>> + >>>> adreno_get_pwrlevels(dev, gpu); >>>> pm_runtime_set_autosuspend_delay(dev, >>>> @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) >>>> icc_put(gpu->icc_path); >>>> icc_put(gpu->ocmem_icc_path); >>>> + >>>> + adreno_put_supported_hw(adreno_gpu->opp_table); >>> >>> And this bit too, though it would be easier to just call the put function >>> directly without having a intermediate function. Also the OPP function should >>> be NULL aware but thats a different story. >>> >>> Jordan >>>> } >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >>>> index c3775f7..a756ad7 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h >>>> @@ -55,6 +55,7 @@ struct adreno_reglist { >>>> }; >>>> extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; >>>> +extern const u32 a618_speedbins[]; >>>> struct adreno_info { >>>> struct adreno_rev rev; >>>> @@ -67,6 +68,8 @@ struct adreno_info { >>>> const char *zapfw; >>>> u32 inactive_period; >>>> const struct adreno_reglist *hwcg; >>>> + const u32 *speedbins; >>>> + const u8 speedbins_count; >>>> }; >>>> const struct adreno_info *adreno_info(struct adreno_rev rev); >>>> @@ -112,6 +115,8 @@ struct adreno_gpu { >>>> * code (a3xx_gpu.c) and stored in this common location. >>>> */ >>>> const unsigned int *reg_offsets; >>>> + >>>> + struct opp_table *opp_table; >>>> }; >>>> #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base) >>>> -- >>>> 2.7.4 >>>> >>> >> >> _______________________________________________ >> Freedreno mailing list >> Freedreno@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/freedreno >
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 8fa5c91..7d42321 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = { .get_timestamp = a5xx_get_timestamp, }; -static void check_speed_bin(struct device *dev) -{ - struct nvmem_cell *cell; - u32 val; - - /* - * If the OPP table specifies a opp-supported-hw property then we have - * to set something with dev_pm_opp_set_supported_hw() or the table - * doesn't get populated so pick an arbitrary value that should - * ensure the default frequencies are selected but not conflict with any - * actual bins - */ - val = 0x80; - - cell = nvmem_cell_get(dev, "speed_bin"); - - if (!IS_ERR(cell)) { - void *buf = nvmem_cell_read(cell, NULL); - - if (!IS_ERR(buf)) { - u8 bin = *((u8 *) buf); - - val = (1 << bin); - kfree(buf); - } - - nvmem_cell_put(cell); - } - - dev_pm_opp_set_supported_hw(dev, &val, 1); -} - struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) { struct msm_drm_private *priv = dev->dev_private; @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) a5xx_gpu->lm_leakage = 0x4E001A; - check_speed_bin(&pdev->dev); - ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4); if (ret) { a5xx_destroy(&(a5xx_gpu->base.base)); diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 87c8b03..e0ff16c 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -18,6 +18,8 @@ bool snapshot_debugbus = false; MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)"); module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600); +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7}; + static const struct adreno_info gpulist[] = { { .rev = ADRENO_REV(2, 0, 0, 0), @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = { ADRENO_QUIRK_FAULT_DETECT_MASK, .init = a5xx_gpu_init, .zapfw = "a530_zap.mdt", + .speedbins = a530v2_speedbins, + .speedbins_count = ARRAY_SIZE(a530v2_speedbins), }, { .rev = ADRENO_REV(5, 4, 0, 2), .revn = 540, diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index f21561d..cdd0c11 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -14,6 +14,7 @@ #include <linux/pm_opp.h> #include <linux/slab.h> #include <linux/soc/qcom/mdt_loader.h> +#include <linux/nvmem-consumer.h> #include <soc/qcom/ocmem.h> #include "adreno_gpu.h" #include "msm_gem.h" @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem) adreno_ocmem->hdl); } +static int adreno_set_supported_hw(struct device *dev, + struct adreno_gpu *adreno_gpu) +{ + u8 speedbins_count = adreno_gpu->info->speedbins_count; + const u32 *speedbins = adreno_gpu->info->speedbins; + struct nvmem_cell *cell; + u32 bin, i; + u32 val = 0; + void *buf, *opp_table; + + cell = nvmem_cell_get(dev, "speed_bin"); + /* + * -ENOENT means that the platform doesn't support speedbin which is + * fine + */ + if (PTR_ERR(cell) == -ENOENT) + return 0; + else if (IS_ERR(cell)) + return PTR_ERR(cell); + + /* A speedbin table is must if the platform supports speedbin */ + if (!speedbins) { + DRM_DEV_ERROR(dev, "speed-bin table is missing\n"); + return -ENOENT; + } + + buf = nvmem_cell_read(cell, NULL); + if (IS_ERR(buf)) { + nvmem_cell_put(cell); + return PTR_ERR(buf); + } + + bin = *((u32 *) buf); + + for (i = 0; i < speedbins_count; i++) { + if (bin == speedbins[i]) { + val = (1 << i); + break; + } + } + + kfree(buf); + nvmem_cell_put(cell); + + if (!val) { + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin); + return -ENOENT; + } + + opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1); + if (IS_ERR(opp_table)) + return PTR_ERR(opp_table); + + adreno_gpu->opp_table = opp_table; + return 0; +} + +static void adreno_put_supported_hw(struct opp_table *opp_table) +{ + if (opp_table) + dev_pm_opp_put_supported_hw(opp_table); +} + int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, struct adreno_gpu *adreno_gpu, const struct adreno_gpu_funcs *funcs, int nr_rings) @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, struct adreno_platform_config *config = dev->platform_data; struct msm_gpu_config adreno_gpu_config = { 0 }; struct msm_gpu *gpu = &adreno_gpu->base; + int ret; adreno_gpu->funcs = funcs; adreno_gpu->info = adreno_info(config->rev); @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, adreno_gpu_config.nr_rings = nr_rings; + ret = adreno_set_supported_hw(dev, adreno_gpu); + if (ret) + return ret; + adreno_get_pwrlevels(dev, gpu); pm_runtime_set_autosuspend_delay(dev, @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu) icc_put(gpu->icc_path); icc_put(gpu->ocmem_icc_path); + + adreno_put_supported_hw(adreno_gpu->opp_table); } diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h index c3775f7..a756ad7 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h @@ -55,6 +55,7 @@ struct adreno_reglist { }; extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[]; +extern const u32 a618_speedbins[]; struct adreno_info { struct adreno_rev rev; @@ -67,6 +68,8 @@ struct adreno_info { const char *zapfw; u32 inactive_period; const struct adreno_reglist *hwcg; + const u32 *speedbins; + const u8 speedbins_count; }; const struct adreno_info *adreno_info(struct adreno_rev rev); @@ -112,6 +115,8 @@ struct adreno_gpu { * code (a3xx_gpu.c) and stored in this common location. */ const unsigned int *reg_offsets; + + struct opp_table *opp_table; }; #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
So far a530v2 gpu has support for detecting its supported opps based on a fuse value called speed-bin. This patch makes this support generic across gpu families. This is in preparation to extend speed-bin support to a6x family. Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> --- This patch is rebased on top of msm-next-staging branch in rob's tree. drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 -------------- drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++ drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++ drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++ 4 files changed, 80 insertions(+), 34 deletions(-)