From: Konrad Dybcio <konrad.dybcio@linaro.org>
Add speebin data for A740, as found on SM8550 and derivative SoCs.
For non-development SoCs it seems that "everything except FC_AC, FC_AF
should be speedbin 1", but what the values are for said "everything" are
not known, so that's an exercise left to the user..
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
@@ -11,6 +11,9 @@
#include "a6xx.xml.h"
#include "a6xx_gmu.xml.h"
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/socinfo.h>
+
static const struct adreno_reglist a612_hwcg[] = {
{REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
{REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
@@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
},
.address_space_size = SZ_16G,
.preempt_record_size = 4192 * SZ_1K,
+ .speedbins = ADRENO_SPEEDBINS(
+ { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
+ { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
+ /* Other feature codes (on prod SoCs) should match to speedbin 1 */
+ ),
}, {
.chip_ids = ADRENO_CHIP_IDS(0x43050c01), /* "C512v2" */
.family = ADRENO_7XX_GEN2,
--
2.49.0Hi,
On 30/04/2025 13:34, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>
> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>
> For non-development SoCs it seems that "everything except FC_AC, FC_AF
> should be speedbin 1", but what the values are for said "everything" are
> not known, so that's an exercise left to the user..
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -11,6 +11,9 @@
> #include "a6xx.xml.h"
> #include "a6xx_gmu.xml.h"
>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/socinfo.h>
> +
> static const struct adreno_reglist a612_hwcg[] = {
> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
> },
> .address_space_size = SZ_16G,
> .preempt_record_size = 4192 * SZ_1K,
> + .speedbins = ADRENO_SPEEDBINS(
> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
I'm trying to understand this sentence. because reading patch 4, when there's no match
devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
Before this change the fallback was speedbin = BIT(0), but this disappeared.
Neil
> + ),
> }, {
> .chip_ids = ADRENO_CHIP_IDS(0x43050c01), /* "C512v2" */
> .family = ADRENO_7XX_GEN2,
>
On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
> Hi,
>
> On 30/04/2025 13:34, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>
>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>> should be speedbin 1", but what the values are for said "everything" are
>> not known, so that's an exercise left to the user..
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>> @@ -11,6 +11,9 @@
>> #include "a6xx.xml.h"
>> #include "a6xx_gmu.xml.h"
>> +#include <linux/soc/qcom/smem.h>
>> +#include <linux/soc/qcom/socinfo.h>
>> +
>> static const struct adreno_reglist a612_hwcg[] = {
>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>> },
>> .address_space_size = SZ_16G,
>> .preempt_record_size = 4192 * SZ_1K,
>> + .speedbins = ADRENO_SPEEDBINS(
>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>
> I'm trying to understand this sentence. because reading patch 4, when there's no match
> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
What I'm saying is that all other entries that happen to be possibly
added down the line are expected to be speedbin 1 (i.e. BIT(1))
> Before this change the fallback was speedbin = BIT(0), but this disappeared.
No, the default was to allow speedbin mask ~(0U)
Konrad
On 30/04/2025 14:35, Konrad Dybcio wrote:
> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>
>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>
>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>> should be speedbin 1", but what the values are for said "everything" are
>>> not known, so that's an exercise left to the user..
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>> @@ -11,6 +11,9 @@
>>> #include "a6xx.xml.h"
>>> #include "a6xx_gmu.xml.h"
>>> +#include <linux/soc/qcom/smem.h>
>>> +#include <linux/soc/qcom/socinfo.h>
>>> +
>>> static const struct adreno_reglist a612_hwcg[] = {
>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>> },
>>> .address_space_size = SZ_16G,
>>> .preempt_record_size = 4192 * SZ_1K,
>>> + .speedbins = ADRENO_SPEEDBINS(
>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>
>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>
> What I'm saying is that all other entries that happen to be possibly
> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>
>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>
> No, the default was to allow speedbin mask ~(0U)
Hmm no:
supp_hw = fuse_to_supp_hw(info, speedbin);
if (supp_hw == UINT_MAX) {
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 */
}
ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
if (ret)
return ret;
>
> Konrad
On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 14:35, Konrad Dybcio wrote:
>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>> Hi,
>>>
>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>
>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>
>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>> should be speedbin 1", but what the values are for said "everything" are
>>>> not known, so that's an exercise left to the user..
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>> @@ -11,6 +11,9 @@
>>>> #include "a6xx.xml.h"
>>>> #include "a6xx_gmu.xml.h"
>>>> +#include <linux/soc/qcom/smem.h>
>>>> +#include <linux/soc/qcom/socinfo.h>
>>>> +
>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>> },
>>>> .address_space_size = SZ_16G,
>>>> .preempt_record_size = 4192 * SZ_1K,
>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>
>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>
>> What I'm saying is that all other entries that happen to be possibly
>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>
>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>
>> No, the default was to allow speedbin mask ~(0U)
>
> Hmm no:
>
> supp_hw = fuse_to_supp_hw(info, speedbin);
>
> if (supp_hw == UINT_MAX) {
> 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 */
> }
>
> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
> if (ret)
> return ret;
Right, that's my own code even..
in any case, the kernel can't know about the speed bins that aren't
defined and here we only define bin0, which doesn't break things
the kernel isn't aware about hw with bin1 with or without this change
so it effectively doesn't matter
Konrad
On 30/04/2025 15:09, Konrad Dybcio wrote:
> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>> Hi,
>>>>
>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>
>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>
>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>> not known, so that's an exercise left to the user..
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>> 1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>> @@ -11,6 +11,9 @@
>>>>> #include "a6xx.xml.h"
>>>>> #include "a6xx_gmu.xml.h"
>>>>> +#include <linux/soc/qcom/smem.h>
>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>> +
>>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>> },
>>>>> .address_space_size = SZ_16G,
>>>>> .preempt_record_size = 4192 * SZ_1K,
>>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>
>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>
>>> What I'm saying is that all other entries that happen to be possibly
>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>
>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>
>>> No, the default was to allow speedbin mask ~(0U)
>>
>> Hmm no:
>>
>> supp_hw = fuse_to_supp_hw(info, speedbin);
>>
>> if (supp_hw == UINT_MAX) {
>> 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 */
>> }
>>
>> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>> if (ret)
>> return ret;
>
> Right, that's my own code even..
>
> in any case, the kernel can't know about the speed bins that aren't
> defined and here we only define bin0, which doesn't break things
>
> the kernel isn't aware about hw with bin1 with or without this change
> so it effectively doesn't matter
But it's regression for the other platforms, where before an unknown SKU
mapped to supp_hw=BIT(0)
Not calling devm_pm_opp_set_supported_hw() is a major regression,
if the opp-supported-hw is present, the OPP will be rejected:
https://elixir.bootlin.com/linux/v6.14.4/source/drivers/opp/of.c#L538
if (!opp_table->supported_hw) {
/*
* In the case that no supported_hw has been set by the
* platform but there is an opp-supported-hw value set for
* an OPP then the OPP should not be enabled as there is
* no way to see if the hardware supports it.
*/
if (of_property_present(np, "opp-supported-hw"))
return false;
else
return true;
}
Neil
>
> Konrad
On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 15:09, Konrad Dybcio wrote:
>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>> Hi,
>>>>>
>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>
>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>
>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>> not known, so that's an exercise left to the user..
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> ---
>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>> 1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>> @@ -11,6 +11,9 @@
>>>>>> #include "a6xx.xml.h"
>>>>>> #include "a6xx_gmu.xml.h"
>>>>>> +#include <linux/soc/qcom/smem.h>
>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>> +
>>>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>> },
>>>>>> .address_space_size = SZ_16G,
>>>>>> .preempt_record_size = 4192 * SZ_1K,
>>>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>
>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>
>>>> What I'm saying is that all other entries that happen to be possibly
>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>
>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>
>>>> No, the default was to allow speedbin mask ~(0U)
>>>
>>> Hmm no:
>>>
>>> supp_hw = fuse_to_supp_hw(info, speedbin);
>>>
>>> if (supp_hw == UINT_MAX) {
>>> 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 */
>>> }
>>>
>>> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>> if (ret)
>>> return ret;
>>
>> Right, that's my own code even..
>>
>> in any case, the kernel can't know about the speed bins that aren't
>> defined and here we only define bin0, which doesn't break things
>>
>> the kernel isn't aware about hw with bin1 with or without this change
>> so it effectively doesn't matter
>
> But it's regression for the other platforms, where before an unknown SKU
> mapped to supp_hw=BIT(0)
>
> Not calling devm_pm_opp_set_supported_hw() is a major regression,
> if the opp-supported-hw is present, the OPP will be rejected:
A comment in patch 4 explains that. We can either be forwards or backwards
compatible (i.e. accept a limited amount of
speedbin_in_driver x speedbin_in_dt combinations)
Konrad
On 30/04/2025 17:36, Konrad Dybcio wrote:
> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>
>>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>>
>>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>> 1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>> @@ -11,6 +11,9 @@
>>>>>>> #include "a6xx.xml.h"
>>>>>>> #include "a6xx_gmu.xml.h"
>>>>>>> +#include <linux/soc/qcom/smem.h>
>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>> +
>>>>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>> },
>>>>>>> .address_space_size = SZ_16G,
>>>>>>> .preempt_record_size = 4192 * SZ_1K,
>>>>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>>
>>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>>
>>>>> What I'm saying is that all other entries that happen to be possibly
>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>
>>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>>
>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>
>>>> Hmm no:
>>>>
>>>> supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>
>>>> if (supp_hw == UINT_MAX) {
>>>> 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 */
>>>> }
>>>>
>>>> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>> if (ret)
>>>> return ret;
>>>
>>> Right, that's my own code even..
>>>
>>> in any case, the kernel can't know about the speed bins that aren't
>>> defined and here we only define bin0, which doesn't break things
>>>
>>> the kernel isn't aware about hw with bin1 with or without this change
>>> so it effectively doesn't matter
>>
>> But it's regression for the other platforms, where before an unknown SKU
>> mapped to supp_hw=BIT(0)
>>
>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>> if the opp-supported-hw is present, the OPP will be rejected:
>
> A comment in patch 4 explains that. We can either be forwards or backwards
> compatible (i.e. accept a limited amount of
> speedbin_in_driver x speedbin_in_dt combinations)
I have a hard time understanding the change, please be much more verbose
in the cover letter and commit messages.
The fact that you do such a large change in the speedbin policy in patch 4
makes it hard to understand why it's needed in the first place.
Finally I'm very concerned that "old" SM8550 DT won't work on new kernels,
this is frankly unacceptable, and this should be addressed in the first
place.
The nvmem situation was much simple, where we considered we added the nvmem
property at the same time as opp-supported-hw in OPPs, but it's no more the
case.
So I think the OPP API should probably be extended to address this situation
first, since if we do not have the opp-supported-hw in OPPs, all OPPs are safe.
So this code:
count = of_property_count_u32_elems(np, "opp-supported-hw");
if (count <= 0 || count % levels) {
dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
__func__, count);
return false;
}
should return true in this specific case, like a supported_hw_failsafe mode.
Neil
>
> Konrad
On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 17:36, Konrad Dybcio wrote:
>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>
>>>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>>>
>>>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>>
>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>> 1 file changed, 8 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>> #include "a6xx.xml.h"
>>>>>>>> #include "a6xx_gmu.xml.h"
>>>>>>>> +#include <linux/soc/qcom/smem.h>
>>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>>> +
>>>>>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>>> },
>>>>>>>> .address_space_size = SZ_16G,
>>>>>>>> .preempt_record_size = 4192 * SZ_1K,
>>>>>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>>>
>>>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>>>
>>>>>> What I'm saying is that all other entries that happen to be possibly
>>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>>
>>>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>>>
>>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>>
>>>>> Hmm no:
>>>>>
>>>>> supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>>
>>>>> if (supp_hw == UINT_MAX) {
>>>>> 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 */
>>>>> }
>>>>>
>>>>> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>> if (ret)
>>>>> return ret;
>>>>
>>>> Right, that's my own code even..
>>>>
>>>> in any case, the kernel can't know about the speed bins that aren't
>>>> defined and here we only define bin0, which doesn't break things
>>>>
>>>> the kernel isn't aware about hw with bin1 with or without this change
>>>> so it effectively doesn't matter
>>>
>>> But it's regression for the other platforms, where before an unknown SKU
>>> mapped to supp_hw=BIT(0)
>>>
>>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>>> if the opp-supported-hw is present, the OPP will be rejected:
>>
>> A comment in patch 4 explains that. We can either be forwards or backwards
>> compatible (i.e. accept a limited amount of
>> speedbin_in_driver x speedbin_in_dt combinations)
>
> I have a hard time understanding the change, please be much more verbose
> in the cover letter and commit messages.
>
> The fact that you do such a large change in the speedbin policy in patch 4
> makes it hard to understand why it's needed in the first place.
>
> Finally I'm very concerned that "old" SM8550 DT won't work on new kernels,
> this is frankly unacceptable, and this should be addressed in the first
> place.
>
> The nvmem situation was much simple, where we considered we added the nvmem
> property at the same time as opp-supported-hw in OPPs, but it's no more the
> case.
>
> So I think the OPP API should probably be extended to address this situation
> first, since if we do not have the opp-supported-hw in OPPs, all OPPs are safe.
>
> So this code:
> count = of_property_count_u32_elems(np, "opp-supported-hw");
> if (count <= 0 || count % levels) {
> dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
> __func__, count);
> return false;
> }
> should return true in this specific case, like a supported_hw_failsafe mode.
Not really. opp-supported-hws = <BIT(0)> usually translates to the *fastest*
bin in our case, so perhaps that change I made previously to default to it
wasn't the wisest. In other words, all slower SKUs that weren't added to the
kernel catalog & dt are potentially getting overclocked, which is no bueno.
That is not always the case, but it most certainly has been for a number of
years.
Old DTs in this case would be DTs lacking opp-supported-hw with the kernel
having speedbin tables. The inverse ("too new DTs") case translates into
"someone put some unexpected stuff in dt and the kernel has no idea what
to do with it".
In this context, old DTs would continue to work after patch 4, as the first
early return in adreno_set_speedbin() takes care of that.
Konrad
On 30/04/2025 18:39, Konrad Dybcio wrote:
> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>
>>>>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>>>>
>>>>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>>> 1 file changed, 8 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>>> #include "a6xx.xml.h"
>>>>>>>>> #include "a6xx_gmu.xml.h"
>>>>>>>>> +#include <linux/soc/qcom/smem.h>
>>>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>>>> +
>>>>>>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>>>> },
>>>>>>>>> .address_space_size = SZ_16G,
>>>>>>>>> .preempt_record_size = 4192 * SZ_1K,
>>>>>>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>>>> + /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>>>>
>>>>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>>>>
>>>>>>> What I'm saying is that all other entries that happen to be possibly
>>>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>>>
>>>>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>>>>
>>>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>>>
>>>>>> Hmm no:
>>>>>>
>>>>>> supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>>>
>>>>>> if (supp_hw == UINT_MAX) {
>>>>>> 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 */
>>>>>> }
>>>>>>
>>>>>> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>>> if (ret)
>>>>>> return ret;
>>>>>
>>>>> Right, that's my own code even..
>>>>>
>>>>> in any case, the kernel can't know about the speed bins that aren't
>>>>> defined and here we only define bin0, which doesn't break things
>>>>>
>>>>> the kernel isn't aware about hw with bin1 with or without this change
>>>>> so it effectively doesn't matter
>>>>
>>>> But it's regression for the other platforms, where before an unknown SKU
>>>> mapped to supp_hw=BIT(0)
>>>>
>>>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>>>> if the opp-supported-hw is present, the OPP will be rejected:
>>>
>>> A comment in patch 4 explains that. We can either be forwards or backwards
>>> compatible (i.e. accept a limited amount of
>>> speedbin_in_driver x speedbin_in_dt combinations)
>>
>> I have a hard time understanding the change, please be much more verbose
>> in the cover letter and commit messages.
>>
>> The fact that you do such a large change in the speedbin policy in patch 4
>> makes it hard to understand why it's needed in the first place.
>>
>> Finally I'm very concerned that "old" SM8550 DT won't work on new kernels,
>> this is frankly unacceptable, and this should be addressed in the first
>> place.
>>
>> The nvmem situation was much simple, where we considered we added the nvmem
>> property at the same time as opp-supported-hw in OPPs, but it's no more the
>> case.
>>
>> So I think the OPP API should probably be extended to address this situation
>> first, since if we do not have the opp-supported-hw in OPPs, all OPPs are safe.
>>
>> So this code:
>> count = of_property_count_u32_elems(np, "opp-supported-hw");
>> if (count <= 0 || count % levels) {
>> dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
>> __func__, count);
>> return false;
>> }
>> should return true in this specific case, like a supported_hw_failsafe mode.
>
> Not really. opp-supported-hws = <BIT(0)> usually translates to the *fastest*
> bin in our case, so perhaps that change I made previously to default to it
> wasn't the wisest. In other words, all slower SKUs that weren't added to the
> kernel catalog & dt are potentially getting overclocked, which is no bueno.
> That is not always the case, but it most certainly has been for a number of
> years.
>
> Old DTs in this case would be DTs lacking opp-supported-hw with the kernel
> having speedbin tables. The inverse ("too new DTs") case translates into
> "someone put some unexpected stuff in dt and the kernel has no idea what
> to do with it".
> In this context, old DTs would continue to work after patch 4, as the first
> early return in adreno_set_speedbin() takes care of that.
No.
With only patches 1-4 applied (keep "old" DT) on today's -next:
SM8550-QRD:
[ 7.574569] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[ 7.586578] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[ 7.597886] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse value: 0x2
[ 7.605518] msm_dpu ae01000.display-controller: failed to load adreno gpu
[ 7.612599] msm_dpu ae01000.display-controller: failed to bind 3d00000.gpu (ops a3xx_ops [msm]): -22
SM8550-HDK:
[ 10.137558] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[ 10.151796] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[ 10.163358] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse value: 0x2
[ 10.171066] msm_dpu ae01000.display-controller: failed to load adreno gpu
[ 10.178118] msm_dpu ae01000.display-controller: failed to bind 3d00000.gpu (ops a3xx_ops [msm]): -22
With:
=================><==================
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
index 61daa3315679..7cac14a585a9 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
@@ -1435,6 +1435,7 @@ static const struct adreno_info a7xx_gpus[] = {
.address_space_size = SZ_16G,
.preempt_record_size = 4192 * SZ_1K,
.speedbins = ADRENO_SPEEDBINS(
+ { ADRENO_SKU_ID(SOCINFO_FC_AB), 1 },
{ ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
{ ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
/* Other feature codes (on prod SoCs) should match to speedbin 1 */
=================><==================
SM8550-QRD:
[ 7.681816] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[ 7.694479] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[ 7.705784] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.714322] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722851] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722853] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722855] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722856] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722858] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722860] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 7.722861] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
[ 7.722863] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR* Unable to set the OPP table
SM8550-HDK:
[ 10.119986] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[ 10.133872] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[ 10.147377] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.161640] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.171198] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.179756] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.188313] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.196868] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.205424] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.226025] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[ 10.234589] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
[ 10.247165] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR* Unable to set the OPP table
This behaves exactly as I said, so please fix it.
Neil
>
> Konrad
On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 18:39, Konrad Dybcio wrote:
>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>>
>>>>>>>>>> Add speebin data for A740, as found on SM8550 and derivative
>>>>>>>>>> SoCs.
>>>>>>>>>>
>>>>>>>>>> For non-development SoCs it seems that "everything except
>>>>>>>>>> FC_AC, FC_AF
>>>>>>>>>> should be speedbin 1", but what the values are for said
>>>>>>>>>> "everything" are
>>>>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>>>> 1 file changed, 8 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/
>>>>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>>> index
>>>>>>>>>> 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>>>> #include "a6xx.xml.h"
>>>>>>>>>> #include "a6xx_gmu.xml.h"
>>>>>>>>>> +#include <linux/soc/qcom/smem.h>
>>>>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>>>>> +
>>>>>>>>>> static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>>>> {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info
>>>>>>>>>> a7xx_gpus[] = {
>>>>>>>>>> },
>>>>>>>>>> .address_space_size = SZ_16G,
>>>>>>>>>> .preempt_record_size = 4192 * SZ_1K,
>>>>>>>>>> + .speedbins = ADRENO_SPEEDBINS(
>>>>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>>>>> + { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>>>>> + /* Other feature codes (on prod SoCs) should
>>>>>>>>>> match to speedbin 1 */
>>>>>>>>>
>>>>>>>>> I'm trying to understand this sentence. because reading patch
>>>>>>>>> 4, when there's no match
>>>>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how
>>>>>>>>> can it match speedbin 1 ?
>>>>>>>>
>>>>>>>> What I'm saying is that all other entries that happen to be
>>>>>>>> possibly
>>>>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>>>>
>>>>>>>>> Before this change the fallback was speedbin = BIT(0), but this
>>>>>>>>> disappeared.
>>>>>>>>
>>>>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>>>>
>>>>>>> Hmm no:
>>>>>>>
>>>>>>> supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>>>>
>>>>>>> if (supp_hw == UINT_MAX) {
>>>>>>> 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 */
>>>>>>> }
>>>>>>>
>>>>>>> ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>>>> if (ret)
>>>>>>> return ret;
>>>>>>
>>>>>> Right, that's my own code even..
>>>>>>
>>>>>> in any case, the kernel can't know about the speed bins that aren't
>>>>>> defined and here we only define bin0, which doesn't break things
>>>>>>
>>>>>> the kernel isn't aware about hw with bin1 with or without this change
>>>>>> so it effectively doesn't matter
>>>>>
>>>>> But it's regression for the other platforms, where before an
>>>>> unknown SKU
>>>>> mapped to supp_hw=BIT(0)
>>>>>
>>>>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>>>>> if the opp-supported-hw is present, the OPP will be rejected:
>>>>
>>>> A comment in patch 4 explains that. We can either be forwards or
>>>> backwards
>>>> compatible (i.e. accept a limited amount of
>>>> speedbin_in_driver x speedbin_in_dt combinations)
>>>
>>> I have a hard time understanding the change, please be much more verbose
>>> in the cover letter and commit messages.
>>>
>>> The fact that you do such a large change in the speedbin policy in
>>> patch 4
>>> makes it hard to understand why it's needed in the first place.
>>>
>>> Finally I'm very concerned that "old" SM8550 DT won't work on new
>>> kernels,
>>> this is frankly unacceptable, and this should be addressed in the first
>>> place.
>>>
>>> The nvmem situation was much simple, where we considered we added the
>>> nvmem
>>> property at the same time as opp-supported-hw in OPPs, but it's no
>>> more the
>>> case.
>>>
>>> So I think the OPP API should probably be extended to address this
>>> situation
>>> first, since if we do not have the opp-supported-hw in OPPs, all OPPs
>>> are safe.
>>>
>>> So this code:
>>> count = of_property_count_u32_elems(np, "opp-supported-hw");
>>> if (count <= 0 || count % levels) {
>>> dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
>>> __func__, count);
>>> return false;
>>> }
>>> should return true in this specific case, like a
>>> supported_hw_failsafe mode.
>>
>> Not really. opp-supported-hws = <BIT(0)> usually translates to the
>> *fastest*
>> bin in our case, so perhaps that change I made previously to default
>> to it
>> wasn't the wisest. In other words, all slower SKUs that weren't added
>> to the
>> kernel catalog & dt are potentially getting overclocked, which is no
>> bueno.
>> That is not always the case, but it most certainly has been for a
>> number of
>> years.
>>
>> Old DTs in this case would be DTs lacking opp-supported-hw with the
>> kernel
>> having speedbin tables. The inverse ("too new DTs") case translates into
>> "someone put some unexpected stuff in dt and the kernel has no idea what
>> to do with it".
>> In this context, old DTs would continue to work after patch 4, as the
>> first
>> early return in adreno_set_speedbin() takes care of that.
>
> No.
>
> With only patches 1-4 applied (keep "old" DT) on today's -next:
>
> SM8550-QRD:
> [ 7.574569] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [ 7.586578] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [ 7.597886] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse
> value: 0x2
> [ 7.605518] msm_dpu ae01000.display-controller: failed to load adreno
> gpu
> [ 7.612599] msm_dpu ae01000.display-controller: failed to bind
> 3d00000.gpu (ops a3xx_ops [msm]): -22
>
> SM8550-HDK:
> [ 10.137558] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [ 10.151796] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [ 10.163358] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse
> value: 0x2
> [ 10.171066] msm_dpu ae01000.display-controller: failed to load adreno
> gpu
> [ 10.178118] msm_dpu ae01000.display-controller: failed to bind
> 3d00000.gpu (ops a3xx_ops [msm]): -22
>
> With:
> =================><==================
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/
> drm/msm/adreno/a6xx_catalog.c
> index 61daa3315679..7cac14a585a9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -1435,6 +1435,7 @@ static const struct adreno_info a7xx_gpus[] = {
> .address_space_size = SZ_16G,
> .preempt_record_size = 4192 * SZ_1K,
> .speedbins = ADRENO_SPEEDBINS(
> + { ADRENO_SKU_ID(SOCINFO_FC_AB), 1 },
> { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
> { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
> /* Other feature codes (on prod SoCs) should
> match to speedbin 1 */
> =================><==================
>
> SM8550-QRD:
> [ 7.681816] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [ 7.694479] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [ 7.705784] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.714322] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722851] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722853] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722855] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722856] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722858] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722860] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 7.722861] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
> [ 7.722863] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR*
> Unable to set the OPP table
>
> SM8550-HDK:
> [ 10.119986] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [ 10.133872] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [ 10.147377] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.161640] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.171198] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.179756] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.188313] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.196868] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.205424] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.226025] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [ 10.234589] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
> [ 10.247165] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR*
> Unable to set the OPP table
>
> This behaves exactly as I said, so please fix it.
Konrad,
iirc, we discussed this in one of the earlier revision. There is a
circular dependency between the driver change for SKU support and the dt
change that adds supported_hw bitmask in opp-table. Only scenario it
works is when you add these to the initial patches series of a new GPU.
It will be very useful if we can break this circular dependency.
-Akhil.
>
> Neil
>
>>
>> Konrad
>
>
On 5/1/25 11:29 AM, Akhil P Oommen wrote: > On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote: >> On 30/04/2025 18:39, Konrad Dybcio wrote: >>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote: >>>> On 30/04/2025 17:36, Konrad Dybcio wrote: >>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote: >>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote: >>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote: >>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote: >>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote: [...] >> This behaves exactly as I said, so please fix it. Eh, I was so sure I tested things correctly.. > > Konrad, > > iirc, we discussed this in one of the earlier revision. There is a > circular dependency between the driver change for SKU support and the dt > change that adds supported_hw bitmask in opp-table. Only scenario it > works is when you add these to the initial patches series of a new GPU. > > It will be very useful if we can break this circular dependency. Right. Let's start with getting that in order Konrad
On 5/1/2025 9:23 PM, Konrad Dybcio wrote: > On 5/1/25 11:29 AM, Akhil P Oommen wrote: >> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote: >>> On 30/04/2025 18:39, Konrad Dybcio wrote: >>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote: >>>>> On 30/04/2025 17:36, Konrad Dybcio wrote: >>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote: >>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote: >>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote: >>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote: >>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote: > > [...] > >>> This behaves exactly as I said, so please fix it. > > Eh, I was so sure I tested things correctly.. > >> >> Konrad, >> >> iirc, we discussed this in one of the earlier revision. There is a >> circular dependency between the driver change for SKU support and the dt >> change that adds supported_hw bitmask in opp-table. Only scenario it >> works is when you add these to the initial patches series of a new GPU. >> >> It will be very useful if we can break this circular dependency. > > Right. Let's start with getting that in order Another complication with the socinfo is that the value is unique for a chipset, not for a GPU. So, it won't work if we keep this data in GPU list in the driver. Downstream solved this problem by keeping the PCODE/FCODE mappings in the devicetree. -Akhil. > > Konrad
On 5/11/25 11:51 AM, Akhil P Oommen wrote: > On 5/1/2025 9:23 PM, Konrad Dybcio wrote: >> On 5/1/25 11:29 AM, Akhil P Oommen wrote: >>> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote: >>>> On 30/04/2025 18:39, Konrad Dybcio wrote: >>>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote: >>>>>> On 30/04/2025 17:36, Konrad Dybcio wrote: >>>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote: >>>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote: >>>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote: >>>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote: >>>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote: >> >> [...] >> >>>> This behaves exactly as I said, so please fix it. >> >> Eh, I was so sure I tested things correctly.. >> >>> >>> Konrad, >>> >>> iirc, we discussed this in one of the earlier revision. There is a >>> circular dependency between the driver change for SKU support and the dt >>> change that adds supported_hw bitmask in opp-table. Only scenario it >>> works is when you add these to the initial patches series of a new GPU. >>> >>> It will be very useful if we can break this circular dependency. >> >> Right. Let's start with getting that in order > > Another complication with the socinfo is that the value is unique for a > chipset, not for a GPU. So, it won't work if we keep this data in GPU > list in the driver. > > Downstream solved this problem by keeping the PCODE/FCODE mappings in > the devicetree. Hmm.. that actually does not sound very bad.. it would allow for e.g. new bins to appear without having to replace the kernel.. great for backwards/forwards compat Rob, WDYT? Konrad
On Thu, May 22, 2025 at 9:03 AM Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> wrote: > > On 5/11/25 11:51 AM, Akhil P Oommen wrote: > > On 5/1/2025 9:23 PM, Konrad Dybcio wrote: > >> On 5/1/25 11:29 AM, Akhil P Oommen wrote: > >>> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote: > >>>> On 30/04/2025 18:39, Konrad Dybcio wrote: > >>>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote: > >>>>>> On 30/04/2025 17:36, Konrad Dybcio wrote: > >>>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote: > >>>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote: > >>>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote: > >>>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote: > >>>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote: > >> > >> [...] > >> > >>>> This behaves exactly as I said, so please fix it. > >> > >> Eh, I was so sure I tested things correctly.. > >> > >>> > >>> Konrad, > >>> > >>> iirc, we discussed this in one of the earlier revision. There is a > >>> circular dependency between the driver change for SKU support and the dt > >>> change that adds supported_hw bitmask in opp-table. Only scenario it > >>> works is when you add these to the initial patches series of a new GPU. > >>> > >>> It will be very useful if we can break this circular dependency. > >> > >> Right. Let's start with getting that in order > > > > Another complication with the socinfo is that the value is unique for a > > chipset, not for a GPU. So, it won't work if we keep this data in GPU > > list in the driver. > > > > Downstream solved this problem by keeping the PCODE/FCODE mappings in > > the devicetree. > > Hmm.. that actually does not sound very bad.. it would allow for e.g. > new bins to appear without having to replace the kernel.. great for > backwards/forwards compat > > Rob, WDYT? Not against having it in dt if the dt maintainers can be convinced.. Alternatively, there is the optional machine string in adreno_info. We've used that in a few other places where speedbin mappings are different for multiple SoCs using the same gpu. BR, -R
© 2016 - 2025 Red Hat, Inc.