diff mbox series

[v5,4/7] drm/msm: adreno: find bandwidth index of OPP and set it along freq index

Message ID 20241211-topic-sm8x50-gpu-bw-vote-v5-4-6112f9f785ec@linaro.org (mailing list archive)
State Superseded
Headers show
Series drm/msm: adreno: add support for DDR bandwidth scaling via GMU | expand

Commit Message

Neil Armstrong Dec. 11, 2024, 8:29 a.m. UTC
The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
along the Frequency and Power Domain level, until now we left the OPP
core scale the OPP bandwidth via the interconnect path.

In order to enable bandwidth voting via the GPU Management
Unit (GMU), when an opp is set by devfreq we also look for
the corresponding bandwidth index in the previously generated
bw_table and pass this value along the frequency index to the GMU.

The GMU also takes another vote called AB which is a 16bit quantized
value of the floor bandwidth against the maximum supported bandwidth.

The AB is calculated with a default 25% of the bandwidth like the
downstream implementation too inform the GMU firmware the minimal
quantity of bandwidth we require for this OPP.

Since we now vote for all resources via the GMU, setting the OPP
is no more needed, so we can completely skip calling
dev_pm_opp_set_opp() in this situation.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
 drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
 drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
 4 files changed, 46 insertions(+), 6 deletions(-)

Comments

Konrad Dybcio Dec. 12, 2024, 8:21 p.m. UTC | #1
On 11.12.2024 9:29 AM, Neil Armstrong wrote:
> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
> along the Frequency and Power Domain level, until now we left the OPP
> core scale the OPP bandwidth via the interconnect path.
> 
> In order to enable bandwidth voting via the GPU Management
> Unit (GMU), when an opp is set by devfreq we also look for
> the corresponding bandwidth index in the previously generated
> bw_table and pass this value along the frequency index to the GMU.
> 
> The GMU also takes another vote called AB which is a 16bit quantized
> value of the floor bandwidth against the maximum supported bandwidth.
> 
> The AB is calculated with a default 25% of the bandwidth like the
> downstream implementation too inform the GMU firmware the minimal
> quantity of bandwidth we require for this OPP.
> 
> Since we now vote for all resources via the GMU, setting the OPP
> is no more needed, so we can completely skip calling
> dev_pm_opp_set_opp() in this situation.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>  drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>  drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>  4 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
>  		       bool suspended)
>  {
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +	const struct a6xx_info *info = adreno_gpu->info->a6xx;
>  	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>  	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>  	u32 perf_index;
> +	u32 bw_index = 0;
>  	unsigned long gpu_freq;
>  	int ret = 0;
>  
> @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
>  		if (gpu_freq == gmu->gpu_freqs[perf_index])
>  			break;
>  
> +	/* If enabled, find the corresponding DDR bandwidth index */
> +	if (info->bcms && gmu->nr_gpu_bws > 1) {

if (gmu->nr_gpu_bws)

> +		unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
> +
> +		for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index++) {
> +			if (bw == gmu->gpu_bw_table[bw_index])
> +				break;
> +		}
> +
> +		/* Vote AB as a fraction of the max bandwidth */
> +		if (bw) {

This seems to only be introduced with certain a7xx too.. you should
ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported

> +			u64 tmp;
> +
> +			/* For now, vote for 25% of the bandwidth */
> +			tmp = bw * 25;
> +			do_div(tmp, 100);
> +
> +			/*
> +			 * The AB vote consists of a 16 bit wide quantized level
> +			 * against the maximum supported bandwidth.
> +			 * Quantization can be calculated as below:
> +			 * vote = (bandwidth * 2^16) / max bandwidth
> +			 */
> +			tmp *= MAX_AB_VOTE;
> +			do_div(tmp, gmu->gpu_bw_table[gmu->nr_gpu_bws - 1]);
> +
> +			bw_index |= AB_VOTE(clamp(tmp, 1, MAX_AB_VOTE));
> +			bw_index |= AB_VOTE_ENABLE;
> +		}
> +	}

BTW, did you dump the values we send to the GMU here and in the RPMh
builder part & validate against downstream?

Konrad
Neil Armstrong Dec. 12, 2024, 9:37 p.m. UTC | #2
On 12/12/2024 21:21, Konrad Dybcio wrote:
> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
>> along the Frequency and Power Domain level, until now we left the OPP
>> core scale the OPP bandwidth via the interconnect path.
>>
>> In order to enable bandwidth voting via the GPU Management
>> Unit (GMU), when an opp is set by devfreq we also look for
>> the corresponding bandwidth index in the previously generated
>> bw_table and pass this value along the frequency index to the GMU.
>>
>> The GMU also takes another vote called AB which is a 16bit quantized
>> value of the floor bandwidth against the maximum supported bandwidth.
>>
>> The AB is calculated with a default 25% of the bandwidth like the
>> downstream implementation too inform the GMU firmware the minimal
>> quantity of bandwidth we require for this OPP.
>>
>> Since we now vote for all resources via the GMU, setting the OPP
>> is no more needed, so we can completely skip calling
>> dev_pm_opp_set_opp() in this situation.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>   drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>   drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>   4 files changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> index 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
>>   		       bool suspended)
>>   {
>>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +	const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>   	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>   	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>   	u32 perf_index;
>> +	u32 bw_index = 0;
>>   	unsigned long gpu_freq;
>>   	int ret = 0;
>>   
>> @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
>>   		if (gpu_freq == gmu->gpu_freqs[perf_index])
>>   			break;
>>   
>> +	/* If enabled, find the corresponding DDR bandwidth index */
>> +	if (info->bcms && gmu->nr_gpu_bws > 1) {
> 
> if (gmu->nr_gpu_bws)

gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the "off" state)

> 
>> +		unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>> +
>> +		for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index++) {
>> +			if (bw == gmu->gpu_bw_table[bw_index])
>> +				break;
>> +		}
>> +
>> +		/* Vote AB as a fraction of the max bandwidth */
>> +		if (bw) {
> 
> This seems to only be introduced with certain a7xx too.. you should
> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported

Good point

> 
>> +			u64 tmp;
>> +
>> +			/* For now, vote for 25% of the bandwidth */
>> +			tmp = bw * 25;
>> +			do_div(tmp, 100);
>> +
>> +			/*
>> +			 * The AB vote consists of a 16 bit wide quantized level
>> +			 * against the maximum supported bandwidth.
>> +			 * Quantization can be calculated as below:
>> +			 * vote = (bandwidth * 2^16) / max bandwidth
>> +			 */
>> +			tmp *= MAX_AB_VOTE;
>> +			do_div(tmp, gmu->gpu_bw_table[gmu->nr_gpu_bws - 1]);
>> +
>> +			bw_index |= AB_VOTE(clamp(tmp, 1, MAX_AB_VOTE));
>> +			bw_index |= AB_VOTE_ENABLE;
>> +		}
>> +	}
> 
> BTW, did you dump the values we send to the GMU here and in the RPMh
> builder part & validate against downstream?

It matches how downstream builds the Ab vote yes

> 
> Konrad
Akhil P Oommen Dec. 13, 2024, 1:12 p.m. UTC | #3
On 12/13/2024 3:07 AM, Neil Armstrong wrote:
> On 12/12/2024 21:21, Konrad Dybcio wrote:
>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
>>> along the Frequency and Power Domain level, until now we left the OPP
>>> core scale the OPP bandwidth via the interconnect path.
>>>
>>> In order to enable bandwidth voting via the GPU Management
>>> Unit (GMU), when an opp is set by devfreq we also look for
>>> the corresponding bandwidth index in the previously generated
>>> bw_table and pass this value along the frequency index to the GMU.
>>>
>>> The GMU also takes another vote called AB which is a 16bit quantized
>>> value of the floor bandwidth against the maximum supported bandwidth.
>>>
>>> The AB is calculated with a default 25% of the bandwidth like the
>>> downstream implementation too inform the GMU firmware the minimal
>>> quantity of bandwidth we require for this OPP.
>>>
>>> Since we now vote for all resources via the GMU, setting the OPP
>>> is no more needed, so we can completely skip calling
>>> dev_pm_opp_set_opp() in this situation.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 ++++++++++++++++++++++++
>>> +++++++++--
>>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>   drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>   drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>   4 files changed, 46 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/
>>> msm/adreno/a6xx_gmu.c
>>> index
>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>> struct dev_pm_opp *opp,
>>>                  bool suspended)
>>>   {
>>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>       u32 perf_index;
>>> +    u32 bw_index = 0;
>>>       unsigned long gpu_freq;
>>>       int ret = 0;
>>>   @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>> struct dev_pm_opp *opp,
>>>           if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>               break;
>>>   +    /* If enabled, find the corresponding DDR bandwidth index */
>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>
>> if (gmu->nr_gpu_bws)
> 
> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
> "off" state)
> 
>>
>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>> +
>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index+
>>> +) {
>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>> +                break;
>>> +        }
>>> +
>>> +        /* Vote AB as a fraction of the max bandwidth */
>>> +        if (bw) {
>>
>> This seems to only be introduced with certain a7xx too.. you should
>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
> 
> Good point

No no. Doing this will trigger some assert in pre-A750 gmu firmwares. We
learned it the hard way. No improvisation please. :)

-Akhil.

> 
>>
>>> +            u64 tmp;
>>> +
>>> +            /* For now, vote for 25% of the bandwidth */
>>> +            tmp = bw * 25;
>>> +            do_div(tmp, 100);
>>> +
>>> +            /*
>>> +             * The AB vote consists of a 16 bit wide quantized level
>>> +             * against the maximum supported bandwidth.
>>> +             * Quantization can be calculated as below:
>>> +             * vote = (bandwidth * 2^16) / max bandwidth
>>> +             */
>>> +            tmp *= MAX_AB_VOTE;
>>> +            do_div(tmp, gmu->gpu_bw_table[gmu->nr_gpu_bws - 1]);
>>> +
>>> +            bw_index |= AB_VOTE(clamp(tmp, 1, MAX_AB_VOTE));
>>> +            bw_index |= AB_VOTE_ENABLE;
>>> +        }
>>> +    }
>>
>> BTW, did you dump the values we send to the GMU here and in the RPMh
>> builder part & validate against downstream?
> 
> It matches how downstream builds the Ab vote yes
> 
>>
>> Konrad
>
Konrad Dybcio Dec. 13, 2024, 3:37 p.m. UTC | #4
On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
>>>> along the Frequency and Power Domain level, until now we left the OPP
>>>> core scale the OPP bandwidth via the interconnect path.
>>>>
>>>> In order to enable bandwidth voting via the GPU Management
>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>> the corresponding bandwidth index in the previously generated
>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>
>>>> The GMU also takes another vote called AB which is a 16bit quantized
>>>> value of the floor bandwidth against the maximum supported bandwidth.
>>>>
>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>> downstream implementation too inform the GMU firmware the minimal
>>>> quantity of bandwidth we require for this OPP.
>>>>
>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>> is no more needed, so we can completely skip calling
>>>> dev_pm_opp_set_opp() in this situation.
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 ++++++++++++++++++++++++
>>>> +++++++++--
>>>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>   drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>   drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>   4 files changed, 46 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/
>>>> msm/adreno/a6xx_gmu.c
>>>> index
>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>> struct dev_pm_opp *opp,
>>>>                  bool suspended)
>>>>   {
>>>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>       u32 perf_index;
>>>> +    u32 bw_index = 0;
>>>>       unsigned long gpu_freq;
>>>>       int ret = 0;
>>>>   @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>> struct dev_pm_opp *opp,
>>>>           if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>               break;
>>>>   +    /* If enabled, find the corresponding DDR bandwidth index */
>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>
>>> if (gmu->nr_gpu_bws)
>>
>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>> "off" state)
>>
>>>
>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>> +
>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index+
>>>> +) {
>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>> +                break;
>>>> +        }
>>>> +
>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>> +        if (bw) {
>>>
>>> This seems to only be introduced with certain a7xx too.. you should
>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>
>> Good point
> 
> No no. Doing this will trigger some assert in pre-A750 gmu firmwares. We
> learned it the hard way. No improvisation please. :)

We shouldn't be sending that AB data to firmware that doesn't expect
it either too, though..

Konrad
Neil Armstrong Dec. 13, 2024, 4:28 p.m. UTC | #5
On 13/12/2024 16:37, Konrad Dybcio wrote:
> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
>>>>> along the Frequency and Power Domain level, until now we left the OPP
>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>
>>>>> In order to enable bandwidth voting via the GPU Management
>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>> the corresponding bandwidth index in the previously generated
>>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>>
>>>>> The GMU also takes another vote called AB which is a 16bit quantized
>>>>> value of the floor bandwidth against the maximum supported bandwidth.
>>>>>
>>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>>> downstream implementation too inform the GMU firmware the minimal
>>>>> quantity of bandwidth we require for this OPP.
>>>>>
>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>> is no more needed, so we can completely skip calling
>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 ++++++++++++++++++++++++
>>>>> +++++++++--
>>>>>    drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>    drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>    drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>    4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/
>>>>> msm/adreno/a6xx_gmu.c
>>>>> index
>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>> struct dev_pm_opp *opp,
>>>>>                   bool suspended)
>>>>>    {
>>>>>        struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>        struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>        struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>        u32 perf_index;
>>>>> +    u32 bw_index = 0;
>>>>>        unsigned long gpu_freq;
>>>>>        int ret = 0;
>>>>>    @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>> struct dev_pm_opp *opp,
>>>>>            if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>                break;
>>>>>    +    /* If enabled, find the corresponding DDR bandwidth index */
>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>
>>>> if (gmu->nr_gpu_bws)
>>>
>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>>> "off" state)
>>>
>>>>
>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>> +
>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index+
>>>>> +) {
>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>> +                break;
>>>>> +        }
>>>>> +
>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>> +        if (bw) {
>>>>
>>>> This seems to only be introduced with certain a7xx too.. you should
>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>>
>>> Good point
>>
>> No no. Doing this will trigger some assert in pre-A750 gmu firmwares. We
>> learned it the hard way. No improvisation please. :)
> 
> We shouldn't be sending that AB data to firmware that doesn't expect
> it either too, though..

Well we don't !

> 
> Konrad
Konrad Dybcio Dec. 13, 2024, 4:31 p.m. UTC | #6
On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
> On 13/12/2024 16:37, Konrad Dybcio wrote:
>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
>>>>>> along the Frequency and Power Domain level, until now we left the OPP
>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>
>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>> the corresponding bandwidth index in the previously generated
>>>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>>>
>>>>>> The GMU also takes another vote called AB which is a 16bit quantized
>>>>>> value of the floor bandwidth against the maximum supported bandwidth.
>>>>>>
>>>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>>>> downstream implementation too inform the GMU firmware the minimal
>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>
>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>> is no more needed, so we can completely skip calling
>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 ++++++++++++++++++++++++
>>>>>> +++++++++--
>>>>>>    drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>    drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>    drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>    4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/
>>>>>> msm/adreno/a6xx_gmu.c
>>>>>> index
>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>> struct dev_pm_opp *opp,
>>>>>>                   bool suspended)
>>>>>>    {
>>>>>>        struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>        struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>        struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>        u32 perf_index;
>>>>>> +    u32 bw_index = 0;
>>>>>>        unsigned long gpu_freq;
>>>>>>        int ret = 0;
>>>>>>    @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>> struct dev_pm_opp *opp,
>>>>>>            if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>                break;
>>>>>>    +    /* If enabled, find the corresponding DDR bandwidth index */
>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>
>>>>> if (gmu->nr_gpu_bws)
>>>>
>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>>>> "off" state)
>>>>
>>>>>
>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>> +
>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index+
>>>>>> +) {
>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>> +                break;
>>>>>> +        }
>>>>>> +
>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>> +        if (bw) {
>>>>>
>>>>> This seems to only be introduced with certain a7xx too.. you should
>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>>>
>>>> Good point
>>>
>>> No no. Doing this will trigger some assert in pre-A750 gmu firmwares. We
>>> learned it the hard way. No improvisation please. :)
>>
>> We shouldn't be sending that AB data to firmware that doesn't expect
>> it either too, though..
> 
> Well we don't !

The code in the scope that I quoted above does that

see the full explanation here

https://git.codelinaro.org/clo/le/platform/vendor/qcom/opensource/graphics-kernel/-/commit/6026c31a54444b712f7ea36ac1aafaaeef07fa4e

Konrad
Neil Armstrong Dec. 13, 2024, 4:40 p.m. UTC | #7
On 13/12/2024 17:31, Konrad Dybcio wrote:
> On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
>> On 13/12/2024 16:37, Konrad Dybcio wrote:
>>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR Bandwidth
>>>>>>> along the Frequency and Power Domain level, until now we left the OPP
>>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>>
>>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>>> the corresponding bandwidth index in the previously generated
>>>>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>>>>
>>>>>>> The GMU also takes another vote called AB which is a 16bit quantized
>>>>>>> value of the floor bandwidth against the maximum supported bandwidth.
>>>>>>>
>>>>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>>>>> downstream implementation too inform the GMU firmware the minimal
>>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>>
>>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>>> is no more needed, so we can completely skip calling
>>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 ++++++++++++++++++++++++
>>>>>>> +++++++++--
>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>>     4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/
>>>>>>> msm/adreno/a6xx_gmu.c
>>>>>>> index
>>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>> struct dev_pm_opp *opp,
>>>>>>>                    bool suspended)
>>>>>>>     {
>>>>>>>         struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>>         struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>>         struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>>         u32 perf_index;
>>>>>>> +    u32 bw_index = 0;
>>>>>>>         unsigned long gpu_freq;
>>>>>>>         int ret = 0;
>>>>>>>     @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>> struct dev_pm_opp *opp,
>>>>>>>             if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>>                 break;
>>>>>>>     +    /* If enabled, find the corresponding DDR bandwidth index */
>>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>>
>>>>>> if (gmu->nr_gpu_bws)
>>>>>
>>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>>>>> "off" state)
>>>>>
>>>>>>
>>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>>> +
>>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index+
>>>>>>> +) {
>>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>>> +                break;
>>>>>>> +        }
>>>>>>> +
>>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>>> +        if (bw) {
>>>>>>
>>>>>> This seems to only be introduced with certain a7xx too.. you should
>>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>>>>
>>>>> Good point
>>>>
>>>> No no. Doing this will trigger some assert in pre-A750 gmu firmwares. We
>>>> learned it the hard way. No improvisation please. :)
>>>
>>> We shouldn't be sending that AB data to firmware that doesn't expect
>>> it either too, though..
>>
>> Well we don't !
> 
> The code in the scope that I quoted above does that

No it doesn't, if the proper bcms are not declared in the gpu_info, it won't

Neil

> 
> see the full explanation here
> 
> https://git.codelinaro.org/clo/le/platform/vendor/qcom/opensource/graphics-kernel/-/commit/6026c31a54444b712f7ea36ac1aafaaeef07fa4e
> 
> Konrad
Akhil P Oommen Dec. 13, 2024, 4:55 p.m. UTC | #8
On 12/13/2024 10:10 PM, neil.armstrong@linaro.org wrote:
> On 13/12/2024 17:31, Konrad Dybcio wrote:
>> On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
>>> On 13/12/2024 16:37, Konrad Dybcio wrote:
>>>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR
>>>>>>>> Bandwidth
>>>>>>>> along the Frequency and Power Domain level, until now we left
>>>>>>>> the OPP
>>>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>>>
>>>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>>>> the corresponding bandwidth index in the previously generated
>>>>>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>>>>>
>>>>>>>> The GMU also takes another vote called AB which is a 16bit
>>>>>>>> quantized
>>>>>>>> value of the floor bandwidth against the maximum supported
>>>>>>>> bandwidth.
>>>>>>>>
>>>>>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>>>>>> downstream implementation too inform the GMU firmware the minimal
>>>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>>>
>>>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>>>> is no more needed, so we can completely skip calling
>>>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>>>
>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++++++
>>>>>>>> +++++++
>>>>>>>> +++++++++--
>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>>>     4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/
>>>>>>>> gpu/drm/
>>>>>>>> msm/adreno/a6xx_gmu.c
>>>>>>>> index
>>>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>                    bool suspended)
>>>>>>>>     {
>>>>>>>>         struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>>>         struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>>>         struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>>>         u32 perf_index;
>>>>>>>> +    u32 bw_index = 0;
>>>>>>>>         unsigned long gpu_freq;
>>>>>>>>         int ret = 0;
>>>>>>>>     @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu
>>>>>>>> *gpu,
>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>             if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>>>                 break;
>>>>>>>>     +    /* If enabled, find the corresponding DDR bandwidth
>>>>>>>> index */
>>>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>>>
>>>>>>> if (gmu->nr_gpu_bws)
>>>>>>
>>>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>>>>>> "off" state)
>>>>>>
>>>>>>>
>>>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>>>> +
>>>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1;
>>>>>>>> bw_index+
>>>>>>>> +) {
>>>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>>>> +                break;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>>>> +        if (bw) {
>>>>>>>
>>>>>>> This seems to only be introduced with certain a7xx too.. you should
>>>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>>>>>
>>>>>> Good point
>>>>>
>>>>> No no. Doing this will trigger some assert in pre-A750 gmu
>>>>> firmwares. We
>>>>> learned it the hard way. No improvisation please. :)
>>>>
>>>> We shouldn't be sending that AB data to firmware that doesn't expect
>>>> it either too, though..
>>>
>>> Well we don't !
>>
>> The code in the scope that I quoted above does that
> 
> No it doesn't, if the proper bcms are not declared in the gpu_info, it
> won't

I think what Konrad meant was that IB voting is supported from a650+,
but AB voting is support only from a750+. So we can add bcm nodes to
enable IB voting, but how do we ensure AB voting via GMU is done only on
a750+.

-Akhil

> 
> Neil
> 
>>
>> see the full explanation here
>>
>> https://git.codelinaro.org/clo/le/platform/vendor/qcom/opensource/
>> graphics-kernel/-/commit/6026c31a54444b712f7ea36ac1aafaaeef07fa4e
>>
>> Konrad
>
Konrad Dybcio Dec. 13, 2024, 11:46 p.m. UTC | #9
On 13.12.2024 5:55 PM, Akhil P Oommen wrote:
> On 12/13/2024 10:10 PM, neil.armstrong@linaro.org wrote:
>> On 13/12/2024 17:31, Konrad Dybcio wrote:
>>> On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
>>>> On 13/12/2024 16:37, Konrad Dybcio wrote:
>>>>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>>>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR
>>>>>>>>> Bandwidth
>>>>>>>>> along the Frequency and Power Domain level, until now we left
>>>>>>>>> the OPP
>>>>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>>>>
>>>>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>>>>> the corresponding bandwidth index in the previously generated
>>>>>>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>>>>>>
>>>>>>>>> The GMU also takes another vote called AB which is a 16bit
>>>>>>>>> quantized
>>>>>>>>> value of the floor bandwidth against the maximum supported
>>>>>>>>> bandwidth.
>>>>>>>>>
>>>>>>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>>>>>>> downstream implementation too inform the GMU firmware the minimal
>>>>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>>>>
>>>>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>>>>> is no more needed, so we can completely skip calling
>>>>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++++++
>>>>>>>>> +++++++
>>>>>>>>> +++++++++--
>>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>>>>     4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/
>>>>>>>>> gpu/drm/
>>>>>>>>> msm/adreno/a6xx_gmu.c
>>>>>>>>> index
>>>>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>                    bool suspended)
>>>>>>>>>     {
>>>>>>>>>         struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>>>>         struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>>>>         struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>>>>         u32 perf_index;
>>>>>>>>> +    u32 bw_index = 0;
>>>>>>>>>         unsigned long gpu_freq;
>>>>>>>>>         int ret = 0;
>>>>>>>>>     @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu
>>>>>>>>> *gpu,
>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>             if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>>>>                 break;
>>>>>>>>>     +    /* If enabled, find the corresponding DDR bandwidth
>>>>>>>>> index */
>>>>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>>>>
>>>>>>>> if (gmu->nr_gpu_bws)
>>>>>>>
>>>>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>>>>>>> "off" state)
>>>>>>>
>>>>>>>>
>>>>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>>>>> +
>>>>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1;
>>>>>>>>> bw_index+
>>>>>>>>> +) {
>>>>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>>>>> +                break;
>>>>>>>>> +        }
>>>>>>>>> +
>>>>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>>>>> +        if (bw) {
>>>>>>>>
>>>>>>>> This seems to only be introduced with certain a7xx too.. you should
>>>>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>>>>>>
>>>>>>> Good point
>>>>>>
>>>>>> No no. Doing this will trigger some assert in pre-A750 gmu
>>>>>> firmwares. We
>>>>>> learned it the hard way. No improvisation please. :)
>>>>>
>>>>> We shouldn't be sending that AB data to firmware that doesn't expect
>>>>> it either too, though..
>>>>
>>>> Well we don't !
>>>
>>> The code in the scope that I quoted above does that
>>
>> No it doesn't, if the proper bcms are not declared in the gpu_info, it
>> won't
> 
> I think what Konrad meant was that IB voting is supported from a650+,
> but AB voting is support only from a750+. So we can add bcm nodes to
> enable IB voting, but how do we ensure AB voting via GMU is done only on
> a750+.

Yep, relying on incomplete data in the catalog is not a great way
to ensure that

Konrad
Neil Armstrong Dec. 16, 2024, 9:43 a.m. UTC | #10
On 14/12/2024 00:46, Konrad Dybcio wrote:
> On 13.12.2024 5:55 PM, Akhil P Oommen wrote:
>> On 12/13/2024 10:10 PM, neil.armstrong@linaro.org wrote:
>>> On 13/12/2024 17:31, Konrad Dybcio wrote:
>>>> On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
>>>>> On 13/12/2024 16:37, Konrad Dybcio wrote:
>>>>>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>>>>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>>>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR
>>>>>>>>>> Bandwidth
>>>>>>>>>> along the Frequency and Power Domain level, until now we left
>>>>>>>>>> the OPP
>>>>>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>>>>>
>>>>>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>>>>>> the corresponding bandwidth index in the previously generated
>>>>>>>>>> bw_table and pass this value along the frequency index to the GMU.
>>>>>>>>>>
>>>>>>>>>> The GMU also takes another vote called AB which is a 16bit
>>>>>>>>>> quantized
>>>>>>>>>> value of the floor bandwidth against the maximum supported
>>>>>>>>>> bandwidth.
>>>>>>>>>>
>>>>>>>>>> The AB is calculated with a default 25% of the bandwidth like the
>>>>>>>>>> downstream implementation too inform the GMU firmware the minimal
>>>>>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>>>>>
>>>>>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>>>>>> is no more needed, so we can completely skip calling
>>>>>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++++++
>>>>>>>>>> +++++++
>>>>>>>>>> +++++++++--
>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>>>>>      4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/
>>>>>>>>>> gpu/drm/
>>>>>>>>>> msm/adreno/a6xx_gmu.c
>>>>>>>>>> index
>>>>>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>>                     bool suspended)
>>>>>>>>>>      {
>>>>>>>>>>          struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>>>>>          struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>>>>>          struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>>>>>          u32 perf_index;
>>>>>>>>>> +    u32 bw_index = 0;
>>>>>>>>>>          unsigned long gpu_freq;
>>>>>>>>>>          int ret = 0;
>>>>>>>>>>      @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu
>>>>>>>>>> *gpu,
>>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>>              if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>>>>>                  break;
>>>>>>>>>>      +    /* If enabled, find the corresponding DDR bandwidth
>>>>>>>>>> index */
>>>>>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>>>>>
>>>>>>>>> if (gmu->nr_gpu_bws)
>>>>>>>>
>>>>>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0 is the
>>>>>>>> "off" state)
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>>>>>> +
>>>>>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1;
>>>>>>>>>> bw_index+
>>>>>>>>>> +) {
>>>>>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>>>>>> +                break;
>>>>>>>>>> +        }
>>>>>>>>>> +
>>>>>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>>>>>> +        if (bw) {
>>>>>>>>>
>>>>>>>>> This seems to only be introduced with certain a7xx too.. you should
>>>>>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's supported
>>>>>>>>
>>>>>>>> Good point
>>>>>>>
>>>>>>> No no. Doing this will trigger some assert in pre-A750 gmu
>>>>>>> firmwares. We
>>>>>>> learned it the hard way. No improvisation please. :)
>>>>>>
>>>>>> We shouldn't be sending that AB data to firmware that doesn't expect
>>>>>> it either too, though..
>>>>>
>>>>> Well we don't !
>>>>
>>>> The code in the scope that I quoted above does that
>>>
>>> No it doesn't, if the proper bcms are not declared in the gpu_info, it
>>> won't
>>
>> I think what Konrad meant was that IB voting is supported from a650+,
>> but AB voting is support only from a750+. So we can add bcm nodes to
>> enable IB voting, but how do we ensure AB voting via GMU is done only on
>> a750+.
> 
> Yep, relying on incomplete data in the catalog is not a great way
> to ensure that

I understood correctly, so I'll add a bool to enable AB voting, but please
don't ask me to remove it because it's dead code and useless if only
enabled on a750+...

Neil

> 
> Konrad
Akhil P Oommen Dec. 16, 2024, 10:40 a.m. UTC | #11
On 12/16/2024 3:13 PM, neil.armstrong@linaro.org wrote:
> On 14/12/2024 00:46, Konrad Dybcio wrote:
>> On 13.12.2024 5:55 PM, Akhil P Oommen wrote:
>>> On 12/13/2024 10:10 PM, neil.armstrong@linaro.org wrote:
>>>> On 13/12/2024 17:31, Konrad Dybcio wrote:
>>>>> On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
>>>>>> On 13/12/2024 16:37, Konrad Dybcio wrote:
>>>>>>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>>>>>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>>>>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>>>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR
>>>>>>>>>>> Bandwidth
>>>>>>>>>>> along the Frequency and Power Domain level, until now we left
>>>>>>>>>>> the OPP
>>>>>>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>>>>>>
>>>>>>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>>>>>>> the corresponding bandwidth index in the previously generated
>>>>>>>>>>> bw_table and pass this value along the frequency index to the
>>>>>>>>>>> GMU.
>>>>>>>>>>>
>>>>>>>>>>> The GMU also takes another vote called AB which is a 16bit
>>>>>>>>>>> quantized
>>>>>>>>>>> value of the floor bandwidth against the maximum supported
>>>>>>>>>>> bandwidth.
>>>>>>>>>>>
>>>>>>>>>>> The AB is calculated with a default 25% of the bandwidth like
>>>>>>>>>>> the
>>>>>>>>>>> downstream implementation too inform the GMU firmware the
>>>>>>>>>>> minimal
>>>>>>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>>>>>>
>>>>>>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>>>>>>> is no more needed, so we can completely skip calling
>>>>>>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>>>>>>
>>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++
>>>>>>>>>>> ++++
>>>>>>>>>>> +++++++
>>>>>>>>>>> +++++++++--
>>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>>>>>>      4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/
>>>>>>>>>>> gpu/drm/
>>>>>>>>>>> msm/adreno/a6xx_gmu.c
>>>>>>>>>>> index
>>>>>>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>>>                     bool suspended)
>>>>>>>>>>>      {
>>>>>>>>>>>          struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>>>>>>          struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>>>>>>          struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>>>>>>          u32 perf_index;
>>>>>>>>>>> +    u32 bw_index = 0;
>>>>>>>>>>>          unsigned long gpu_freq;
>>>>>>>>>>>          int ret = 0;
>>>>>>>>>>>      @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu
>>>>>>>>>>> *gpu,
>>>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>>>              if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>>>>>>                  break;
>>>>>>>>>>>      +    /* If enabled, find the corresponding DDR bandwidth
>>>>>>>>>>> index */
>>>>>>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>>>>>>
>>>>>>>>>> if (gmu->nr_gpu_bws)
>>>>>>>>>
>>>>>>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0
>>>>>>>>> is the
>>>>>>>>> "off" state)
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>>>>>>> +
>>>>>>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1;
>>>>>>>>>>> bw_index+
>>>>>>>>>>> +) {
>>>>>>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>>>>>>> +                break;
>>>>>>>>>>> +        }
>>>>>>>>>>> +
>>>>>>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>>>>>>> +        if (bw) {
>>>>>>>>>>
>>>>>>>>>> This seems to only be introduced with certain a7xx too.. you
>>>>>>>>>> should
>>>>>>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's
>>>>>>>>>> supported
>>>>>>>>>
>>>>>>>>> Good point
>>>>>>>>
>>>>>>>> No no. Doing this will trigger some assert in pre-A750 gmu
>>>>>>>> firmwares. We
>>>>>>>> learned it the hard way. No improvisation please. :)
>>>>>>>
>>>>>>> We shouldn't be sending that AB data to firmware that doesn't expect
>>>>>>> it either too, though..
>>>>>>
>>>>>> Well we don't !
>>>>>
>>>>> The code in the scope that I quoted above does that
>>>>
>>>> No it doesn't, if the proper bcms are not declared in the gpu_info, it
>>>> won't
>>>
>>> I think what Konrad meant was that IB voting is supported from a650+,
>>> but AB voting is support only from a750+. So we can add bcm nodes to
>>> enable IB voting, but how do we ensure AB voting via GMU is done only on
>>> a750+.
>>
>> Yep, relying on incomplete data in the catalog is not a great way
>> to ensure that
> 
> I understood correctly, so I'll add a bool to enable AB voting, but please
> don't ask me to remove it because it's dead code and useless if only
> enabled on a750+...

Can't we just add ">= A7XX_GEN3" check here to decide on GMU AB VOTE?
For older generation, AB vote should be via icc driver. And that I guess
is out of the scope of this series.

-Akhil.

> 
> Neil
> 
>>
>> Konrad
>
Neil Armstrong Dec. 16, 2024, 11:03 a.m. UTC | #12
On 16/12/2024 11:40, Akhil P Oommen wrote:
> On 12/16/2024 3:13 PM, neil.armstrong@linaro.org wrote:
>> On 14/12/2024 00:46, Konrad Dybcio wrote:
>>> On 13.12.2024 5:55 PM, Akhil P Oommen wrote:
>>>> On 12/13/2024 10:10 PM, neil.armstrong@linaro.org wrote:
>>>>> On 13/12/2024 17:31, Konrad Dybcio wrote:
>>>>>> On 13.12.2024 5:28 PM, neil.armstrong@linaro.org wrote:
>>>>>>> On 13/12/2024 16:37, Konrad Dybcio wrote:
>>>>>>>> On 13.12.2024 2:12 PM, Akhil P Oommen wrote:
>>>>>>>>> On 12/13/2024 3:07 AM, Neil Armstrong wrote:
>>>>>>>>>> On 12/12/2024 21:21, Konrad Dybcio wrote:
>>>>>>>>>>> On 11.12.2024 9:29 AM, Neil Armstrong wrote:
>>>>>>>>>>>> The Adreno GPU Management Unit (GMU) can also scale the DDR
>>>>>>>>>>>> Bandwidth
>>>>>>>>>>>> along the Frequency and Power Domain level, until now we left
>>>>>>>>>>>> the OPP
>>>>>>>>>>>> core scale the OPP bandwidth via the interconnect path.
>>>>>>>>>>>>
>>>>>>>>>>>> In order to enable bandwidth voting via the GPU Management
>>>>>>>>>>>> Unit (GMU), when an opp is set by devfreq we also look for
>>>>>>>>>>>> the corresponding bandwidth index in the previously generated
>>>>>>>>>>>> bw_table and pass this value along the frequency index to the
>>>>>>>>>>>> GMU.
>>>>>>>>>>>>
>>>>>>>>>>>> The GMU also takes another vote called AB which is a 16bit
>>>>>>>>>>>> quantized
>>>>>>>>>>>> value of the floor bandwidth against the maximum supported
>>>>>>>>>>>> bandwidth.
>>>>>>>>>>>>
>>>>>>>>>>>> The AB is calculated with a default 25% of the bandwidth like
>>>>>>>>>>>> the
>>>>>>>>>>>> downstream implementation too inform the GMU firmware the
>>>>>>>>>>>> minimal
>>>>>>>>>>>> quantity of bandwidth we require for this OPP.
>>>>>>>>>>>>
>>>>>>>>>>>> Since we now vote for all resources via the GMU, setting the OPP
>>>>>>>>>>>> is no more needed, so we can completely skip calling
>>>>>>>>>>>> dev_pm_opp_set_opp() in this situation.
>>>>>>>>>>>>
>>>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>>       drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 39 +++++++++++++
>>>>>>>>>>>> ++++
>>>>>>>>>>>> +++++++
>>>>>>>>>>>> +++++++++--
>>>>>>>>>>>>       drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 +-
>>>>>>>>>>>>       drivers/gpu/drm/msm/adreno/a6xx_hfi.c |  6 +++---
>>>>>>>>>>>>       drivers/gpu/drm/msm/adreno/a6xx_hfi.h |  5 +++++
>>>>>>>>>>>>       4 files changed, 46 insertions(+), 6 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/
>>>>>>>>>>>> gpu/drm/
>>>>>>>>>>>> msm/adreno/a6xx_gmu.c
>>>>>>>>>>>> index
>>>>>>>>>>>> 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>>>>>>>>>>> @@ -110,9 +110,11 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu,
>>>>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>>>>                      bool suspended)
>>>>>>>>>>>>       {
>>>>>>>>>>>>           struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>>>>>>>> +    const struct a6xx_info *info = adreno_gpu->info->a6xx;
>>>>>>>>>>>>           struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>>>>>>>>>>>           struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>>>>>>>>>>>           u32 perf_index;
>>>>>>>>>>>> +    u32 bw_index = 0;
>>>>>>>>>>>>           unsigned long gpu_freq;
>>>>>>>>>>>>           int ret = 0;
>>>>>>>>>>>>       @@ -125,6 +127,37 @@ void a6xx_gmu_set_freq(struct msm_gpu
>>>>>>>>>>>> *gpu,
>>>>>>>>>>>> struct dev_pm_opp *opp,
>>>>>>>>>>>>               if (gpu_freq == gmu->gpu_freqs[perf_index])
>>>>>>>>>>>>                   break;
>>>>>>>>>>>>       +    /* If enabled, find the corresponding DDR bandwidth
>>>>>>>>>>>> index */
>>>>>>>>>>>> +    if (info->bcms && gmu->nr_gpu_bws > 1) {
>>>>>>>>>>>
>>>>>>>>>>> if (gmu->nr_gpu_bws)
>>>>>>>>>>
>>>>>>>>>> gmu->nr_gpu_bws == 1 means there's not BW in the OPPs (index 0
>>>>>>>>>> is the
>>>>>>>>>> "off" state)
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +        unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
>>>>>>>>>>>> +
>>>>>>>>>>>> +        for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1;
>>>>>>>>>>>> bw_index+
>>>>>>>>>>>> +) {
>>>>>>>>>>>> +            if (bw == gmu->gpu_bw_table[bw_index])
>>>>>>>>>>>> +                break;
>>>>>>>>>>>> +        }
>>>>>>>>>>>> +
>>>>>>>>>>>> +        /* Vote AB as a fraction of the max bandwidth */
>>>>>>>>>>>> +        if (bw) {
>>>>>>>>>>>
>>>>>>>>>>> This seems to only be introduced with certain a7xx too.. you
>>>>>>>>>>> should
>>>>>>>>>>> ping the GMU with HFI_VALUE_GMU_AB_VOTE to check if it's
>>>>>>>>>>> supported
>>>>>>>>>>
>>>>>>>>>> Good point
>>>>>>>>>
>>>>>>>>> No no. Doing this will trigger some assert in pre-A750 gmu
>>>>>>>>> firmwares. We
>>>>>>>>> learned it the hard way. No improvisation please. :)
>>>>>>>>
>>>>>>>> We shouldn't be sending that AB data to firmware that doesn't expect
>>>>>>>> it either too, though..
>>>>>>>
>>>>>>> Well we don't !
>>>>>>
>>>>>> The code in the scope that I quoted above does that
>>>>>
>>>>> No it doesn't, if the proper bcms are not declared in the gpu_info, it
>>>>> won't
>>>>
>>>> I think what Konrad meant was that IB voting is supported from a650+,
>>>> but AB voting is support only from a750+. So we can add bcm nodes to
>>>> enable IB voting, but how do we ensure AB voting via GMU is done only on
>>>> a750+.
>>>
>>> Yep, relying on incomplete data in the catalog is not a great way
>>> to ensure that
>>
>> I understood correctly, so I'll add a bool to enable AB voting, but please
>> don't ask me to remove it because it's dead code and useless if only
>> enabled on a750+...
> 
> Can't we just add ">= A7XX_GEN3" check here to decide on GMU AB VOTE?
> For older generation, AB vote should be via icc driver. And that I guess
> is out of the scope of this series.

Sure, I'll do that.

Thanks,
Neil

> 
> -Akhil.
> 
>>
>> Neil
>>
>>>
>>> Konrad
>>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 36696d372a42a27b26a018b19e73bc6d8a4a5235..46ae0ec7a16a41d55755ce04fb32404cdba087be 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -110,9 +110,11 @@  void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
 		       bool suspended)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	const struct a6xx_info *info = adreno_gpu->info->a6xx;
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
 	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
 	u32 perf_index;
+	u32 bw_index = 0;
 	unsigned long gpu_freq;
 	int ret = 0;
 
@@ -125,6 +127,37 @@  void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
 		if (gpu_freq == gmu->gpu_freqs[perf_index])
 			break;
 
+	/* If enabled, find the corresponding DDR bandwidth index */
+	if (info->bcms && gmu->nr_gpu_bws > 1) {
+		unsigned int bw = dev_pm_opp_get_bw(opp, true, 0);
+
+		for (bw_index = 0; bw_index < gmu->nr_gpu_bws - 1; bw_index++) {
+			if (bw == gmu->gpu_bw_table[bw_index])
+				break;
+		}
+
+		/* Vote AB as a fraction of the max bandwidth */
+		if (bw) {
+			u64 tmp;
+
+			/* For now, vote for 25% of the bandwidth */
+			tmp = bw * 25;
+			do_div(tmp, 100);
+
+			/*
+			 * The AB vote consists of a 16 bit wide quantized level
+			 * against the maximum supported bandwidth.
+			 * Quantization can be calculated as below:
+			 * vote = (bandwidth * 2^16) / max bandwidth
+			 */
+			tmp *= MAX_AB_VOTE;
+			do_div(tmp, gmu->gpu_bw_table[gmu->nr_gpu_bws - 1]);
+
+			bw_index |= AB_VOTE(clamp(tmp, 1, MAX_AB_VOTE));
+			bw_index |= AB_VOTE_ENABLE;
+		}
+	}
+
 	gmu->current_perf_index = perf_index;
 	gmu->freq = gmu->gpu_freqs[perf_index];
 
@@ -140,8 +173,10 @@  void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
 		return;
 
 	if (!gmu->legacy) {
-		a6xx_hfi_set_freq(gmu, perf_index);
-		dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
+		a6xx_hfi_set_freq(gmu, perf_index, bw_index);
+		/* With Bandwidth voting, we now vote for all resources, so skip OPP set */
+		if (!bw_index)
+			dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 2062a2be224768c1937d7768f7b8439920e9e127..0c888b326cfb485400118f3601fa5f1949b03374 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -209,7 +209,7 @@  void a6xx_hfi_init(struct a6xx_gmu *gmu);
 int a6xx_hfi_start(struct a6xx_gmu *gmu, int boot_state);
 void a6xx_hfi_stop(struct a6xx_gmu *gmu);
 int a6xx_hfi_send_prep_slumber(struct a6xx_gmu *gmu);
-int a6xx_hfi_set_freq(struct a6xx_gmu *gmu, int index);
+int a6xx_hfi_set_freq(struct a6xx_gmu *gmu, u32 perf_index, u32 bw_index);
 
 bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu);
 bool a6xx_gmu_sptprac_is_on(struct a6xx_gmu *gmu);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
index 995526620d678cd05020315f771213e4a6943bec..0989aee3dd2cf9bc3405c3b25a595c22e6f06387 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
@@ -772,13 +772,13 @@  static int a6xx_hfi_send_core_fw_start(struct a6xx_gmu *gmu)
 		sizeof(msg), NULL, 0);
 }
 
-int a6xx_hfi_set_freq(struct a6xx_gmu *gmu, int index)
+int a6xx_hfi_set_freq(struct a6xx_gmu *gmu, u32 freq_index, u32 bw_index)
 {
 	struct a6xx_hfi_gx_bw_perf_vote_cmd msg = { 0 };
 
 	msg.ack_type = 1; /* blocking */
-	msg.freq = index;
-	msg.bw = 0; /* TODO: bus scaling */
+	msg.freq = freq_index;
+	msg.bw = bw_index;
 
 	return a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_GX_BW_PERF_VOTE, &msg,
 		sizeof(msg), NULL, 0);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h
index 528110169398f69f16443a29a1594d19c36fb595..52ba4a07d7b9a709289acd244a751ace9bdaab5d 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h
@@ -173,6 +173,11 @@  struct a6xx_hfi_gx_bw_perf_vote_cmd {
 	u32 bw;
 };
 
+#define AB_VOTE_MASK		GENMASK(31, 16)
+#define MAX_AB_VOTE		(FIELD_MAX(AB_VOTE_MASK) - 1)
+#define AB_VOTE(vote)		FIELD_PREP(AB_VOTE_MASK, (vote))
+#define AB_VOTE_ENABLE		BIT(8)
+
 #define HFI_H2F_MSG_PREPARE_SLUMBER 33
 
 struct a6xx_hfi_prep_slumber_cmd {