| Message ID | 20240405-topic-smem_speedbin-v1-0-ce2b864251b1@linaro.org |
|---|---|
| Headers | show |
| Series | Add SMEM-based speedbin matching | expand |
On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > In preparation for parsing the chip "feature code" (FC) and "product > code" (PC) (essentially the parameters that let us conclusively > characterize the sillicon we're running on, including various speed > bins), move the socinfo version defines to the public header and > include some more FC/PC defines. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/soc/qcom/socinfo.c | 8 -------- > include/linux/soc/qcom/socinfo.h | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c > index 277c07a6603d..cf4616a468f2 100644 > --- a/drivers/soc/qcom/socinfo.c > +++ b/drivers/soc/qcom/socinfo.c > @@ -21,14 +21,6 @@ > > #include <dt-bindings/arm/qcom,ids.h> > > -/* > - * SoC version type with major number in the upper 16 bits and minor > - * number in the lower 16 bits. > - */ > -#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff) > -#define SOCINFO_MINOR(ver) ((ver) & 0xffff) > -#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff)) > - > /* Helper macros to create soc_id table */ > #define qcom_board_id(id) QCOM_ID_ ## id, __stringify(id) > #define qcom_board_id_named(id, name) QCOM_ID_ ## id, (name) > diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h > index e78777bb0f4a..ba7f683bd32c 100644 > --- a/include/linux/soc/qcom/socinfo.h > +++ b/include/linux/soc/qcom/socinfo.h > @@ -3,6 +3,8 @@ > #ifndef __QCOM_SOCINFO_H__ > #define __QCOM_SOCINFO_H__ > > +#include <linux/types.h> > + > /* > * SMEM item id, used to acquire handles to respective > * SMEM region. > @@ -12,6 +14,14 @@ > #define SMEM_SOCINFO_BUILD_ID_LENGTH 32 > #define SMEM_SOCINFO_CHIP_ID_LENGTH 32 > > +/* > + * SoC version type with major number in the upper 16 bits and minor > + * number in the lower 16 bits. > + */ > +#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff) > +#define SOCINFO_MINOR(ver) ((ver) & 0xffff) > +#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff)) > + > /* Socinfo SMEM item structure */ > struct socinfo { > __le32 fmt; > @@ -74,4 +84,30 @@ struct socinfo { > __le32 boot_core; > }; > > +/* Internal feature codes */ > +enum feature_code { > + /* External feature codes */ > + SOCINFO_FC_UNKNOWN = 0x0, > + SOCINFO_FC_AA, > + SOCINFO_FC_AB, > + SOCINFO_FC_AC, > + SOCINFO_FC_AD, > + SOCINFO_FC_AE, > + SOCINFO_FC_AF, > + SOCINFO_FC_AG, > + SOCINFO_FC_AH, > + SOCINFO_FC_EXT_RESERVE, > +}; > + > +/* Internal feature codes */ > +/* Valid values: 0 <= n <= 0xf */ > +#define SOCINFO_FC_Yn(n) (0xf1 + n) > +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10) > + > +/* Product codes */ > +#define SOCINFO_PC_UNKNOWN 0 > +/* Valid values: 0 <= n <= 8, the rest is reserved */ > +#define SOCINFO_PCn(n) (n + 1) > +#define SOCINFO_PC_RESERVE (BIT(31) - 1) Please move these defines into the next patch. > + > #endif > > -- > 2.40.1 >
On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: > From: Neil Armstrong <neil.armstrong@linaro.org> > > Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock > the highest. Falling back to it when things go wrong is largely > suboptimal, as more often than not, the top frequencies are not > supposed to work on other bins. Isn't it better to just return an error here instead of trying to guess which speedbin to use? If that's not the case, I think the commit should be expanded with actually setting default_speedbin for the existing GPUs. > > Let the developer specify the intended "lowest common denominator" bin > in struct adreno_info. If not specified, partial struct initialization > will ensure it's set to zero, retaining previous behavior. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > [Konrad: clean up, add commit message] > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 0674aca0f8a3..4cbdfabbcee5 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -2915,7 +2915,7 @@ static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *i > DRM_DEV_ERROR(dev, > "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n", > speedbin); > - supp_hw = BIT(0); /* Default */ > + supp_hw = BIT(info->default_speedbin); /* Default */ > } > > ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1); > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > index 77526892eb8c..460b399be37b 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > @@ -110,6 +110,7 @@ struct adreno_info { > * {SHRT_MAX, 0} sentinal. > */ > struct adreno_speedbin *speedbins; > + unsigned int default_speedbin; > }; > > #define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 } > > -- > 2.40.1 >
On Fri, Apr 05, 2024 at 10:41:33AM +0200, Konrad Dybcio wrote: > Add speebin data for A740, as found on SM8550 and derivative SoCs. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index 901ef767e491..c976a485aef2 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = { > .zapfw = "a740_zap.mdt", > .hwcg = a740_hwcg, > .address_space_size = SZ_16G, > + .speedbins = ADRENO_SPEEDBINS( I think this deserves either a comment or some info in the commit message. > + { ADRENO_SKU_ID(SOCINFO_PC_UNKNOWN, SOCINFO_FC_AC), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PC_UNKNOWN, SOCINFO_FC_AF), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(1), SOCINFO_FC_UNKNOWN), 1 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(2), SOCINFO_FC_Yn(0x0)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(2), SOCINFO_FC_Yn(0x2)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(4), SOCINFO_FC_Yn(0x0)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(4), SOCINFO_FC_Yn(0x2)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0x0)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0x1)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0xd)), 0 }, > + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0xe)), 0 }, > + ), > + .default_speedbin = 1, > }, { > .chip_ids = ADRENO_CHIP_IDS(0x43051401), /* "C520v2" */ > .family = ADRENO_7XX_GEN3, > > -- > 2.40.1 >
On Fri, Apr 05, 2024 at 10:41:28AM +0200, Konrad Dybcio wrote: > Newer (SM8550+) SoCs don't seem to have a nice speedbin fuse anymore, > but instead rely on a set of combinations of "feature code" (FC) and > "product code" (PC) identifiers to match the bins. This series adds > support for that. > > I suppose a qcom/for-soc immutable branch would be in order if we want > to land this in the upcoming cycle. > > FWIW I preferred the fuses myself.. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Konrad Dybcio (5): > soc: qcom: Move some socinfo defines to the header, expand them > soc: qcom: smem: Add pcode/fcode getters > drm/msm/adreno: Implement SMEM-based speed bin > drm/msm/adreno: Add speedbin data for SM8550 / A740 > arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs > > Neil Armstrong (1): > drm/msm/adreno: Allow specifying default speedbin value Generic comment: as you are reworking speed bins implementaiton, could you please take a broader look. A5xx just reads nvmem manually. A6xx uses adreno_read_speedbin(). And then we call adreno_read_speedbin second time from from adreno_gpu_init(). Can we get to the point where the function is called only once for all the platforms which implements speed binning?
On 4/6/24 04:56, Dmitry Baryshkov wrote: > On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: >> From: Neil Armstrong <neil.armstrong@linaro.org> >> >> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock >> the highest. Falling back to it when things go wrong is largely >> suboptimal, as more often than not, the top frequencies are not >> supposed to work on other bins. > > Isn't it better to just return an error here instead of trying to guess > which speedbin to use? Not sure. I'd rather better compatibility for e.g. booting up a new laptop with just dt. > > If that's not the case, I think the commit should be expanded with > actually setting default_speedbin for the existing GPUs. I think that should be addressed, although separately. Konrad
On 4/6/24 05:25, Dmitry Baryshkov wrote: > On Fri, Apr 05, 2024 at 10:41:33AM +0200, Konrad Dybcio wrote: >> Add speebin data for A740, as found on SM8550 and derivative SoCs. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >> index 901ef767e491..c976a485aef2 100644 >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >> @@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = { >> .zapfw = "a740_zap.mdt", >> .hwcg = a740_hwcg, >> .address_space_size = SZ_16G, >> + .speedbins = ADRENO_SPEEDBINS( > > I think this deserves either a comment or some info in the commit > message. "this" = ? Konrad
On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: > > > On 4/6/24 04:56, Dmitry Baryshkov wrote: > > On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: > > > From: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock > > > the highest. Falling back to it when things go wrong is largely > > > suboptimal, as more often than not, the top frequencies are not > > > supposed to work on other bins. > > > > Isn't it better to just return an error here instead of trying to guess > > which speedbin to use? > > Not sure. I'd rather better compatibility for e.g. booting up a new > laptop with just dt. New speedbin can have lower max speed, so by attempting to run it at higher freq you might be breaking it. > > > > > If that's not the case, I think the commit should be expanded with > > actually setting default_speedbin for the existing GPUs. > > I think that should be addressed, although separately. I'd prefer to have it as a part of this patch, but I'd not NAK it just for this reason.
On Tue, Apr 09, 2024 at 05:13:15PM +0200, Konrad Dybcio wrote: > > > On 4/6/24 05:25, Dmitry Baryshkov wrote: > > On Fri, Apr 05, 2024 at 10:41:33AM +0200, Konrad Dybcio wrote: > > > Add speebin data for A740, as found on SM8550 and derivative SoCs. > > > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > --- > > > drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > > index 901ef767e491..c976a485aef2 100644 > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > > @@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = { > > > .zapfw = "a740_zap.mdt", > > > .hwcg = a740_hwcg, > > > .address_space_size = SZ_16G, > > > + .speedbins = ADRENO_SPEEDBINS( > > > > I think this deserves either a comment or some info in the commit > > message. > > "this" = ? I see two types of speedbins here, it would be nice to understand at least some reason or some defailts for that (if you know them).
On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: > > > > > > On 4/6/24 04:56, Dmitry Baryshkov wrote: > > > On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: > > > > From: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > > Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock > > > > the highest. Falling back to it when things go wrong is largely > > > > suboptimal, as more often than not, the top frequencies are not > > > > supposed to work on other bins. > > > > > > Isn't it better to just return an error here instead of trying to guess > > > which speedbin to use? > > > > Not sure. I'd rather better compatibility for e.g. booting up a new > > laptop with just dt. > > New speedbin can have lower max speed, so by attempting to run it at > higher freq you might be breaking it. Usually there are some OPPs in common to all speedbins, so picking a freq from that set would seem like the safe thing to do BR, -R > > > > > > > > > If that's not the case, I think the commit should be expanded with > > > actually setting default_speedbin for the existing GPUs. > > > > I think that should be addressed, although separately. > > I'd prefer to have it as a part of this patch, but I'd not NAK it just > for this reason. > > -- > With best wishes > Dmitry
On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote: > On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: > > > > > > > > > On 4/6/24 04:56, Dmitry Baryshkov wrote: > > > > On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: > > > > > From: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > > > > Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock > > > > > the highest. Falling back to it when things go wrong is largely > > > > > suboptimal, as more often than not, the top frequencies are not > > > > > supposed to work on other bins. > > > > > > > > Isn't it better to just return an error here instead of trying to guess > > > > which speedbin to use? > > > > > > Not sure. I'd rather better compatibility for e.g. booting up a new > > > laptop with just dt. > > > > New speedbin can have lower max speed, so by attempting to run it at > > higher freq you might be breaking it. > > Usually there are some OPPs in common to all speedbins, so picking a > freq from that set would seem like the safe thing to do Well, the issue is about an uknown speed bin. So in theory we know nothing about the set of speeds itsupports. My point is that we should simplfy fail in such case. > > BR, > -R > > > > > > > > > > > > > > If that's not the case, I think the commit should be expanded with > > > > actually setting default_speedbin for the existing GPUs. > > > > > > I think that should be addressed, although separately. > > > > I'd prefer to have it as a part of this patch, but I'd not NAK it just > > for this reason. > > > > -- > > With best wishes > > Dmitry
On 4/9/24 20:04, Dmitry Baryshkov wrote: > On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote: >> On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov >> <dmitry.baryshkov@linaro.org> wrote: >>> >>> On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: >>>> >>>> >>>> On 4/6/24 04:56, Dmitry Baryshkov wrote: >>>>> On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: >>>>>> From: Neil Armstrong <neil.armstrong@linaro.org> >>>>>> >>>>>> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock >>>>>> the highest. Falling back to it when things go wrong is largely >>>>>> suboptimal, as more often than not, the top frequencies are not >>>>>> supposed to work on other bins. >>>>> >>>>> Isn't it better to just return an error here instead of trying to guess >>>>> which speedbin to use? >>>> >>>> Not sure. I'd rather better compatibility for e.g. booting up a new >>>> laptop with just dt. >>> >>> New speedbin can have lower max speed, so by attempting to run it at >>> higher freq you might be breaking it. >> >> Usually there are some OPPs in common to all speedbins, so picking a >> freq from that set would seem like the safe thing to do > > Well, the issue is about an uknown speed bin. So in theory we know > nothing about the set of speeds itsupports. My point is that we should > simplfy fail in such case. Or we could allow e.g. the lowest frequency (or 2) which if often shared across the board to work, giving a compromise between OOBE and sanity Konrad
On 4/9/24 17:24, Dmitry Baryshkov wrote: > On Tue, Apr 09, 2024 at 05:13:15PM +0200, Konrad Dybcio wrote: >> >> >> On 4/6/24 05:25, Dmitry Baryshkov wrote: >>> On Fri, Apr 05, 2024 at 10:41:33AM +0200, Konrad Dybcio wrote: >>>> Add speebin data for A740, as found on SM8550 and derivative SoCs. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> index 901ef767e491..c976a485aef2 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> @@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = { >>>> .zapfw = "a740_zap.mdt", >>>> .hwcg = a740_hwcg, >>>> .address_space_size = SZ_16G, >>>> + .speedbins = ADRENO_SPEEDBINS( >>> >>> I think this deserves either a comment or some info in the commit >>> message. >> >> "this" = ? > > I see two types of speedbins here, it would be nice to understand at > least some reason or some defailts for that (if you know them). "one is slightly faster" sorry, qcom downstream has been getting increasingly cryptic lately.. Konrad
On Tue, Apr 09, 2024 at 08:07:56PM +0200, Konrad Dybcio wrote: > > > On 4/9/24 20:04, Dmitry Baryshkov wrote: > > On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote: > > > On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: > > > > > > > > > > > > > > > On 4/6/24 04:56, Dmitry Baryshkov wrote: > > > > > > On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: > > > > > > > From: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > > > > > > > > Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock > > > > > > > the highest. Falling back to it when things go wrong is largely > > > > > > > suboptimal, as more often than not, the top frequencies are not > > > > > > > supposed to work on other bins. > > > > > > > > > > > > Isn't it better to just return an error here instead of trying to guess > > > > > > which speedbin to use? > > > > > > > > > > Not sure. I'd rather better compatibility for e.g. booting up a new > > > > > laptop with just dt. > > > > > > > > New speedbin can have lower max speed, so by attempting to run it at > > > > higher freq you might be breaking it. > > > > > > Usually there are some OPPs in common to all speedbins, so picking a > > > freq from that set would seem like the safe thing to do > > > > Well, the issue is about an uknown speed bin. So in theory we know > > nothing about the set of speeds itsupports. My point is that we should > > simplfy fail in such case. > > Or we could allow e.g. the lowest frequency (or 2) which if often shared > across the board to work, giving a compromise between OOBE and sanity That's also an option. But we should not be using existing speed table for the unknown bin.
On 4/9/24 20:15, Dmitry Baryshkov wrote: > On Tue, Apr 09, 2024 at 08:07:56PM +0200, Konrad Dybcio wrote: >> >> >> On 4/9/24 20:04, Dmitry Baryshkov wrote: >>> On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote: >>>> On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov >>>> <dmitry.baryshkov@linaro.org> wrote: >>>>> >>>>> On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: >>>>>> >>>>>> >>>>>> On 4/6/24 04:56, Dmitry Baryshkov wrote: >>>>>>> On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: >>>>>>>> From: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>>> >>>>>>>> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock >>>>>>>> the highest. Falling back to it when things go wrong is largely >>>>>>>> suboptimal, as more often than not, the top frequencies are not >>>>>>>> supposed to work on other bins. >>>>>>> >>>>>>> Isn't it better to just return an error here instead of trying to guess >>>>>>> which speedbin to use? >>>>>> >>>>>> Not sure. I'd rather better compatibility for e.g. booting up a new >>>>>> laptop with just dt. >>>>> >>>>> New speedbin can have lower max speed, so by attempting to run it at >>>>> higher freq you might be breaking it. >>>> >>>> Usually there are some OPPs in common to all speedbins, so picking a >>>> freq from that set would seem like the safe thing to do >>> >>> Well, the issue is about an uknown speed bin. So in theory we know >>> nothing about the set of speeds itsupports. My point is that we should >>> simplfy fail in such case. >> >> Or we could allow e.g. the lowest frequency (or 2) which if often shared >> across the board to work, giving a compromise between OOBE and sanity > > That's also an option. But we should not be using existing speed table for > the unknown bin. I derived this logic from msm-5.15 where it's "intended behavior".. I suppose we can do better as you said though There have been cases in the past where the default speed bin ended up having a higher max freq than subsequent ones, and I don't think I can trust this product/feature code approach to guarantee this never happening again. So. I think sticking to a single lowest freq and printing a big red line in dmesg makes sense here Konrad
On Tue, 9 Apr 2024 at 21:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 4/9/24 20:15, Dmitry Baryshkov wrote: > > On Tue, Apr 09, 2024 at 08:07:56PM +0200, Konrad Dybcio wrote: > >> > >> > >> On 4/9/24 20:04, Dmitry Baryshkov wrote: > >>> On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote: > >>>> On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov > >>>> <dmitry.baryshkov@linaro.org> wrote: > >>>>> > >>>>> On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: > >>>>>> > >>>>>> > >>>>>> On 4/6/24 04:56, Dmitry Baryshkov wrote: > >>>>>>> On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: > >>>>>>>> From: Neil Armstrong <neil.armstrong@linaro.org> > >>>>>>>> > >>>>>>>> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock > >>>>>>>> the highest. Falling back to it when things go wrong is largely > >>>>>>>> suboptimal, as more often than not, the top frequencies are not > >>>>>>>> supposed to work on other bins. > >>>>>>> > >>>>>>> Isn't it better to just return an error here instead of trying to guess > >>>>>>> which speedbin to use? > >>>>>> > >>>>>> Not sure. I'd rather better compatibility for e.g. booting up a new > >>>>>> laptop with just dt. > >>>>> > >>>>> New speedbin can have lower max speed, so by attempting to run it at > >>>>> higher freq you might be breaking it. > >>>> > >>>> Usually there are some OPPs in common to all speedbins, so picking a > >>>> freq from that set would seem like the safe thing to do > >>> > >>> Well, the issue is about an uknown speed bin. So in theory we know > >>> nothing about the set of speeds itsupports. My point is that we should > >>> simplfy fail in such case. > >> > >> Or we could allow e.g. the lowest frequency (or 2) which if often shared > >> across the board to work, giving a compromise between OOBE and sanity > > > > That's also an option. But we should not be using existing speed table for > > the unknown bin. > > I derived this logic from msm-5.15 where it's "intended behavior".. I > suppose we can do better as you said though > > There have been cases in the past where the default speed bin ended up > having a higher max freq than subsequent ones, and I don't think I can > trust this product/feature code approach to guarantee this never > happening again. > > So. I think sticking to a single lowest freq and printing a big red line > in dmesg makes sense here Make 0x80 the default supported-hw, make sure that the lowest freq has 0xff. Plus big-red-line. And hope that we'll never see 16 speed bins for the hardware.
On 4/9/24 20:31, Dmitry Baryshkov wrote: > On Tue, 9 Apr 2024 at 21:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 4/9/24 20:15, Dmitry Baryshkov wrote: >>> On Tue, Apr 09, 2024 at 08:07:56PM +0200, Konrad Dybcio wrote: >>>> >>>> >>>> On 4/9/24 20:04, Dmitry Baryshkov wrote: >>>>> On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote: >>>>>> On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov >>>>>> <dmitry.baryshkov@linaro.org> wrote: >>>>>>> >>>>>>> On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 4/6/24 04:56, Dmitry Baryshkov wrote: >>>>>>>>> On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote: >>>>>>>>>> From: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>>>>> >>>>>>>>>> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock >>>>>>>>>> the highest. Falling back to it when things go wrong is largely >>>>>>>>>> suboptimal, as more often than not, the top frequencies are not >>>>>>>>>> supposed to work on other bins. >>>>>>>>> >>>>>>>>> Isn't it better to just return an error here instead of trying to guess >>>>>>>>> which speedbin to use? >>>>>>>> >>>>>>>> Not sure. I'd rather better compatibility for e.g. booting up a new >>>>>>>> laptop with just dt. >>>>>>> >>>>>>> New speedbin can have lower max speed, so by attempting to run it at >>>>>>> higher freq you might be breaking it. >>>>>> >>>>>> Usually there are some OPPs in common to all speedbins, so picking a >>>>>> freq from that set would seem like the safe thing to do >>>>> >>>>> Well, the issue is about an uknown speed bin. So in theory we know >>>>> nothing about the set of speeds itsupports. My point is that we should >>>>> simplfy fail in such case. >>>> >>>> Or we could allow e.g. the lowest frequency (or 2) which if often shared >>>> across the board to work, giving a compromise between OOBE and sanity >>> >>> That's also an option. But we should not be using existing speed table for >>> the unknown bin. >> >> I derived this logic from msm-5.15 where it's "intended behavior".. I >> suppose we can do better as you said though >> >> There have been cases in the past where the default speed bin ended up >> having a higher max freq than subsequent ones, and I don't think I can >> trust this product/feature code approach to guarantee this never >> happening again. >> >> So. I think sticking to a single lowest freq and printing a big red line >> in dmesg makes sense here > > Make 0x80 the default supported-hw, make sure that the lowest freq has > 0xff. Plus big-red-line. > And hope that we'll never see 16 speed bins for the hardware. opp-supported-hw is a u32 bitmask fwiw I was thinking, either 0xffffffff or some driver-side hackery (dev_pm_opp_enable). Perhaps I'd be more in favor of the latter, as putting meaningless gobblygoo in dt is not good Konrad
On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > In preparation for parsing the chip "feature code" (FC) and "product > code" (PC) (essentially the parameters that let us conclusively > characterize the sillicon we're running on, including various speed > bins), move the socinfo version defines to the public header and > include some more FC/PC defines. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/soc/qcom/socinfo.c | 8 -------- > include/linux/soc/qcom/socinfo.h | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+), 8 deletions(-) > ... > diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h ... > @@ -74,4 +84,30 @@ struct socinfo { > __le32 boot_core; > }; > > +/* Internal feature codes */ > +enum feature_code { > + /* External feature codes */ > + SOCINFO_FC_UNKNOWN = 0x0, > + SOCINFO_FC_AA, > + SOCINFO_FC_AB, > + SOCINFO_FC_AC, > + SOCINFO_FC_AD, > + SOCINFO_FC_AE, > + SOCINFO_FC_AF, > + SOCINFO_FC_AG, > + SOCINFO_FC_AH, > + SOCINFO_FC_EXT_RESERVE, > +}; SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8 feature codes so far. We should remove the EXT_RESERVE and test for the Y0-YF (internal feature code) values instead. > + > +/* Internal feature codes */ > +/* Valid values: 0 <= n <= 0xf */ > +#define SOCINFO_FC_Yn(n) (0xf1 + n) > +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10) We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies it's reserved for some future use, but it's really the max value it could be. > + > +/* Product codes */ > +#define SOCINFO_PC_UNKNOWN 0 > +/* Valid values: 0 <= n <= 8, the rest is reserved */ > +#define SOCINFO_PCn(n) (n + 1) > +#define SOCINFO_PC_RESERVE (BIT(31) - 1) Similar comments here as the SOCINFO_FC_EXT_*. It's more like known values are [0,8], but more values could come in future chipsets. > + > #endif >
On 4/11/24 20:55, Elliot Berman wrote: > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: >> In preparation for parsing the chip "feature code" (FC) and "product >> code" (PC) (essentially the parameters that let us conclusively >> characterize the sillicon we're running on, including various speed >> bins), move the socinfo version defines to the public header and >> include some more FC/PC defines. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- [...] >> + SOCINFO_FC_EXT_RESERVE, >> +}; > > SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped > SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8 > feature codes so far. > > We should remove the EXT_RESERVE and test for the Y0-YF (internal > feature code) values instead. OK > >> + >> +/* Internal feature codes */ >> +/* Valid values: 0 <= n <= 0xf */ >> +#define SOCINFO_FC_Yn(n) (0xf1 + n) >> +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10) > > We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies > it's reserved for some future use, but it's really the max value it > could be. So, should SOCINFO_FC_Yn(0x10) also be considered valid, or is (0xf) the last one? > >> + >> +/* Product codes */ >> +#define SOCINFO_PC_UNKNOWN 0 >> +/* Valid values: 0 <= n <= 8, the rest is reserved */ >> +#define SOCINFO_PCn(n) (n + 1) >> +#define SOCINFO_PC_RESERVE (BIT(31) - 1) > > Similar comments here as the SOCINFO_FC_EXT_*. It's more like known > values are [0,8], but more values could come in future chipsets. Ok, sounds good, I'll remove the comment then Konrad
On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: > > > On 4/11/24 20:55, Elliot Berman wrote: > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > > > In preparation for parsing the chip "feature code" (FC) and "product > > > code" (PC) (essentially the parameters that let us conclusively > > > characterize the sillicon we're running on, including various speed > > > bins), move the socinfo version defines to the public header and > > > include some more FC/PC defines. > > > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > --- > > [...] > > > > + SOCINFO_FC_EXT_RESERVE, > > > +}; > > > > SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped > > SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8 > > feature codes so far. > > > > We should remove the EXT_RESERVE and test for the Y0-YF (internal > > feature code) values instead. > > OK > > > > > > + > > > +/* Internal feature codes */ > > > +/* Valid values: 0 <= n <= 0xf */ > > > +#define SOCINFO_FC_Yn(n) (0xf1 + n) > > > +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10) > > > > We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies > > it's reserved for some future use, but it's really the max value it > > could be. > > So, should SOCINFO_FC_Yn(0x10) also be considered valid, or is (0xf) > the last one? > 0xf is the last one. Thanks, Elliot > > > > > + > > > +/* Product codes */ > > > +#define SOCINFO_PC_UNKNOWN 0 > > > +/* Valid values: 0 <= n <= 8, the rest is reserved */ > > > +#define SOCINFO_PCn(n) (n + 1) > > > +#define SOCINFO_PC_RESERVE (BIT(31) - 1) > > > > Similar comments here as the SOCINFO_FC_EXT_*. It's more like known > > values are [0,8], but more values could come in future chipsets. > > Ok, sounds good, I'll remove the comment then > > Konrad
On 4/11/24 22:09, Elliot Berman wrote: > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: >> >> >> On 4/11/24 20:55, Elliot Berman wrote: >>> On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: >>>> In preparation for parsing the chip "feature code" (FC) and "product >>>> code" (PC) (essentially the parameters that let us conclusively >>>> characterize the sillicon we're running on, including various speed >>>> bins), move the socinfo version defines to the public header and >>>> include some more FC/PC defines. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- [...] > > 0xf is the last one. One more question, are the "internal/external feature codes" referring to internality/externality of the chips (i.e. "are they QC-lab-only engineering samples), or what else does that represent? Konrad
On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote: > > > On 4/11/24 22:09, Elliot Berman wrote: > > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: > > > > > > > > > On 4/11/24 20:55, Elliot Berman wrote: > > > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > > > > > In preparation for parsing the chip "feature code" (FC) and "product > > > > > code" (PC) (essentially the parameters that let us conclusively > > > > > characterize the sillicon we're running on, including various speed > > > > > bins), move the socinfo version defines to the public header and > > > > > include some more FC/PC defines. > > > > > > > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > > > --- > > [...] > > > > > 0xf is the last one. > > One more question, are the "internal/external feature codes" referring to > internality/externality of the chips (i.e. "are they QC-lab-only engineering > samples), or what else does that represent? Yes, QC-lab-only engineering samples is the right interpretation of these feature codes. Thanks, Elliot
On 4/12/24 01:49, Elliot Berman wrote: > On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote: >> >> >> On 4/11/24 22:09, Elliot Berman wrote: >>> On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: >>>> >>>> >>>> On 4/11/24 20:55, Elliot Berman wrote: >>>>> On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: >>>>>> In preparation for parsing the chip "feature code" (FC) and "product >>>>>> code" (PC) (essentially the parameters that let us conclusively >>>>>> characterize the sillicon we're running on, including various speed >>>>>> bins), move the socinfo version defines to the public header and >>>>>> include some more FC/PC defines. >>>>>> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>> --- >> >> [...] >> >>> >>> 0xf is the last one. >> >> One more question, are the "internal/external feature codes" referring to >> internality/externality of the chips (i.e. "are they QC-lab-only engineering >> samples), or what else does that represent? > > Yes, QC-lab-only engineering samples is the right interpretation of > these feature codes. Do you think it would be beneficial to keep the logic for these ESes in the upstream GPU driver? Otherwise, I can yank out half of the added lines. Konrad
On Fri, Apr 12, 2024 at 02:10:30AM +0200, Konrad Dybcio wrote: > > > On 4/12/24 01:49, Elliot Berman wrote: > > On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote: > > > > > > > > > On 4/11/24 22:09, Elliot Berman wrote: > > > > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: > > > > > > > > > > > > > > > On 4/11/24 20:55, Elliot Berman wrote: > > > > > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > > > > > > > In preparation for parsing the chip "feature code" (FC) and "product > > > > > > > code" (PC) (essentially the parameters that let us conclusively > > > > > > > characterize the sillicon we're running on, including various speed > > > > > > > bins), move the socinfo version defines to the public header and > > > > > > > include some more FC/PC defines. > > > > > > > > > > > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > > > > > --- > > > > > > [...] > > > > > > > > > > > 0xf is the last one. > > > > > > One more question, are the "internal/external feature codes" referring to > > > internality/externality of the chips (i.e. "are they QC-lab-only engineering > > > samples), or what else does that represent? > > > > Yes, QC-lab-only engineering samples is the right interpretation of > > these feature codes. > > Do you think it would be beneficial to keep the logic for these ESes in > the upstream GPU driver? Otherwise, I can yank out half of the added lines. > Should be fine to yank, IMO.
Newer (SM8550+) SoCs don't seem to have a nice speedbin fuse anymore, but instead rely on a set of combinations of "feature code" (FC) and "product code" (PC) identifiers to match the bins. This series adds support for that. I suppose a qcom/for-soc immutable branch would be in order if we want to land this in the upcoming cycle. FWIW I preferred the fuses myself.. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Konrad Dybcio (5): soc: qcom: Move some socinfo defines to the header, expand them soc: qcom: smem: Add pcode/fcode getters drm/msm/adreno: Implement SMEM-based speed bin drm/msm/adreno: Add speedbin data for SM8550 / A740 arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs Neil Armstrong (1): drm/msm/adreno: Allow specifying default speedbin value arch/arm64/boot/dts/qcom/sm8550.dtsi | 21 +++++++++- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 +++-- drivers/gpu/drm/msm/adreno/adreno_device.c | 16 ++++++++ drivers/gpu/drm/msm/adreno/adreno_gpu.c | 39 ++++++++++++++++-- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 13 ++++-- drivers/soc/qcom/smem.c | 66 ++++++++++++++++++++++++++++++ drivers/soc/qcom/socinfo.c | 8 ---- include/linux/soc/qcom/smem.h | 2 + include/linux/soc/qcom/socinfo.h | 36 ++++++++++++++++ 9 files changed, 191 insertions(+), 20 deletions(-) --- base-commit: 2b3d5988ae2cb5cd945ddbc653f0a71706231fdd change-id: 20240404-topic-smem_speedbin-8deecd0bef0e Best regards,