On 7/22/2025 10:56 PM, Rob Clark wrote:
> On Tue, Jul 22, 2025 at 6:33 AM Dmitry Baryshkov
> <dmitry.barysh...@oss.qualcomm.com> wrote:
>>
>> On Sun, Jul 20, 2025 at 05:46:06PM +0530, Akhil P Oommen wrote:
>>> Since the PDC resides out of the GPU subsystem and cannot be reset in
>>> case it enters bad state, utmost care must be taken to trigger the PDC
>>> wake/sleep routines in the correct order.
>>>
>>> The PDC wake sequence can be exercised only after a PDC sleep sequence.
>>> Additionally, GMU firmware should initialize a few registers before the
>>> KMD can trigger a PDC sleep sequence. So PDC sleep can't be done if the
>>
>> s/KMD/the driver/
>
> IMHO for gpu things "KMD" makes sense, to differentiate between kernel
> and user mode (UMD).. this is perhaps different from other areas where
> there isn't a userspace component to the driver stack
Ack
>
> BR,
> -R
>
>>> GMU firmware has not initialized. Track these dependencies using a new
>>> status variable and trigger PDC sleep/wake sequences appropriately.
>>
>> Again, it looks like there should be a Fixes tag here.
Ack. I guess it is not a bad idea to backport this one too.
>>
>>>
>>> Signed-off-by: Akhil P Oommen <akhi...@oss.qualcomm.com>
>>> ---
>>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 30 +++++++++++++++++++-----------
>>> drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 6 ++++++
>>> 2 files changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> index
>>> 3bebb6dd7059782ceca29f2efd2acee24d3fc930..4d6c70735e0892ed87d6a68d64f24bda844e5e16
>>> 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> @@ -279,6 +279,8 @@ static int a6xx_gmu_start(struct a6xx_gmu *gmu)
>>> if (ret)
>>> DRM_DEV_ERROR(gmu->dev, "GMU firmware initialization timed
>>> out\n");
>>>
>>> + set_bit(GMU_STATUS_FW_START, &gmu->status);
>>> +
>>> return ret;
>>> }
>>>
>>> @@ -528,6 +530,9 @@ static int a6xx_rpmh_start(struct a6xx_gmu *gmu)
>>> int ret;
>>> u32 val;
>>>
>>> + if (!test_and_clear_bit(GMU_STATUS_PDC_SLEEP, &gmu->status))
>>> + return 0;
>>> +
>>> gmu_write(gmu, REG_A6XX_GMU_RSCC_CONTROL_REQ, BIT(1));
>>>
>>> ret = gmu_poll_timeout(gmu, REG_A6XX_GMU_RSCC_CONTROL_ACK, val,
>>> @@ -555,6 +560,11 @@ static void a6xx_rpmh_stop(struct a6xx_gmu *gmu)
>>> int ret;
>>> u32 val;
>>>
>>> + if (test_and_clear_bit(GMU_STATUS_FW_START, &gmu->status))
>>> + return;
>>> +
>>> + /* TODO: should we skip if IFPC is not enabled */
>>
>> Is this a question or a statement?
It was a reminder to myself which I forgot to revisit later. Will
addresss this in the next revision.
-Akhil.
>>
>>> +
>>> gmu_write(gmu, REG_A6XX_GMU_RSCC_CONTROL_REQ, 1);
>>>
>>> ret = gmu_poll_timeout_rscc(gmu, REG_A6XX_GPU_RSCC_RSC_STATUS0_DRV0,
>>> @@ -563,6 +573,8 @@ static void a6xx_rpmh_stop(struct a6xx_gmu *gmu)
>>> DRM_DEV_ERROR(gmu->dev, "Unable to power off the GPU RSC\n");
>>>
>>> gmu_write(gmu, REG_A6XX_GMU_RSCC_CONTROL_REQ, 0);
>>> +
>>> + set_bit(GMU_STATUS_PDC_SLEEP, &gmu->status);
>>> }
>>>
>>> static inline void pdc_write(void __iomem *ptr, u32 offset, u32 value)
>>> @@ -691,8 +703,6 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
>>> /* ensure no writes happen before the uCode is fully written */
>>> wmb();
>>>
>>> - a6xx_rpmh_stop(gmu);
>>> -
>>> err:
>>> if (!IS_ERR_OR_NULL(pdcptr))
>>> iounmap(pdcptr);
>>> @@ -852,19 +862,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
>>> unsigned int state)
>>> else
>>> gmu_write(gmu, REG_A6XX_GMU_GENERAL_7, 1);
>>>
>>> - if (state == GMU_WARM_BOOT) {
>>> - ret = a6xx_rpmh_start(gmu);
>>> - if (ret)
>>> - return ret;
>>> - } else {
>>> + ret = a6xx_rpmh_start(gmu);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (state == GMU_COLD_BOOT) {
>>> if (WARN(!adreno_gpu->fw[ADRENO_FW_GMU],
>>> "GMU firmware is not loaded\n"))
>>> return -ENOENT;
>>>
>>> - ret = a6xx_rpmh_start(gmu);
>>> - if (ret)
>>> - return ret;
>>> -
>>> ret = a6xx_gmu_fw_load(gmu);
>>> if (ret)
>>> return ret;
>>> @@ -1046,6 +1052,8 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
>>>
>>> /* Reset GPU core blocks */
>>> a6xx_gpu_sw_reset(gpu, true);
>>> +
>>> + a6xx_rpmh_stop(gmu);
>>> }
>>>
>>> static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu
>>> *gmu)
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
>>> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
>>> index
>>> b2d4489b40249b1916ab4a42c89e3f4bdc5c4af9..034f1b4e5a3fb9cd601bfbe6d06d64e5ace3b6e7
>>> 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
>>> @@ -117,6 +117,12 @@ struct a6xx_gmu {
>>>
>>> struct qmp *qmp;
>>> struct a6xx_hfi_msg_bw_table *bw_table;
>>> +
>>> +/* To check if we can trigger sleep seq at PDC. Cleared in
>>> a6xx_rpmh_stop() */
>>> +#define GMU_STATUS_FW_START 0
>>> +/* To track if PDC sleep seq was done */
>>> +#define GMU_STATUS_PDC_SLEEP 1
>>> + unsigned long status;
>>> };
>>>
>>> static inline u32 gmu_read(struct a6xx_gmu *gmu, u32 offset)
>>>
>>> --
>>> 2.50.1
>>>
>>
>> --
>> With best wishes
>> Dmitry
