| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e72188ac-d680-42c2-81be-eb17bea8e94a@oss.qualcomm.com>
Date: Mon, 8 Sep 2025 22:16:14 +0530
From: Akhil P Oommen <akhilpo@....qualcomm.com>
To: Connor Abbott <cwabbott0@...il.com>
Cc: Rob Clark <robin.clark@....qualcomm.com>, Sean Paul <sean@...rly.run>,
Konrad Dybcio <konradybcio@...nel.org>,
Dmitry Baryshkov <lumag@...nel.org>,
Abhinav Kumar <abhinav.kumar@...ux.dev>,
Jessica Zhang <jessica.zhang@....qualcomm.com>,
Marijn Suijten <marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Antonino Maniscalco <antomani103@...il.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/16] drm/msm/adreno: Add fenced regwrite support
On 9/8/2025 9:24 PM, Connor Abbott wrote:
> On Mon, Sep 8, 2025 at 4:27 AM Akhil P Oommen <akhilpo@....qualcomm.com> wrote:
>>
>> There are some special registers which are accessible even when GX power
>> domain is collapsed during an IFPC sleep. Accessing these registers
>> wakes up GPU from power collapse and allow programming these registers
>> without additional handshake with GMU. This patch adds support for this
>> special register write sequence.
>>
>> Signed-off-by: Akhil P Oommen <akhilpo@....qualcomm.com>
>> ---
>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 80 ++++++++++++++++++++++++++++++-
>> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 1 +
>> drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 20 ++++----
>> 3 files changed, 90 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index 45dd5fd1c2bfcb0a01b71a326c7d95b0f9496d99..a63dad80ef461da45d5c41a042ed4f19d8282ef5 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -16,6 +16,84 @@
>>
>> #define GPU_PAS_ID 13
>>
>> +static bool fence_status_check(struct msm_gpu *gpu, u32 offset, u32 value, u32 status, u32 mask)
>> +{
>> + /* Success if !writedropped0/1 */
>> + if (!(status & mask))
>> + return true;
>> +
>> + udelay(10);
>> +
>> + /* Try to update fenced register again */
>> + gpu_write(gpu, offset, value);
>> +
>> + /* We can't do a posted write here because the power domain could be
>> + * in collapse state. So use the heaviest barrier instead
>> + */
>> + mb();
>> + return false;
>> +}
>> +
>> +static int fenced_write(struct a6xx_gpu *a6xx_gpu, u32 offset, u32 value, u32 mask)
>> +{
>> + struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>> + struct msm_gpu *gpu = &adreno_gpu->base;
>> + struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>> + u32 status;
>> +
>> + gpu_write(gpu, offset, value);
>> +
>> + /* Nothing else to be done in the case of no-GMU */
>> + if (adreno_has_gmu_wrapper(adreno_gpu))
>> + return 0;
>> +
>> + /* We can't do a posted write here because the power domain could be
>> + * in collapse state. So use the heaviest barrier instead
>> + */
>> + mb();
>> +
>> + if (!gmu_poll_timeout(gmu, REG_A6XX_GMU_AHB_FENCE_STATUS, status,
>> + fence_status_check(gpu, offset, value, status, mask), 0, 1000))
>> + return 0;
>> +
>> + /* Try again for another 1ms before failing */
>> + gpu_write(gpu, offset, value);
>> + mb();
>> +
>> + if (!gmu_poll_timeout(gmu, REG_A6XX_GMU_AHB_FENCE_STATUS, status,
>> + fence_status_check(gpu, offset, value, status, mask), 0, 1000)) {
>> + /*
>> + * The 'delay' warning is here because the pause to print this
>> + * warning will allow gpu to move to power collapse which
>> + * defeats the purpose of continuous polling for 2 ms
>> + */
>> + dev_err_ratelimited(gmu->dev, "delay in fenced register write (0x%x)\n",
>> + offset);
>> + return 0;
>> + }
>> +
>> + dev_err_ratelimited(gmu->dev, "fenced register write (0x%x) fail\n",
>> + offset);
>> +
>> + return -ETIMEDOUT;
>> +}
>> +
>> +int a6xx_fenced_write(struct a6xx_gpu *a6xx_gpu, u32 offset, u64 value, u32 mask, bool is_64b)
>> +{
>> + int ret;
>> +
>> + ret = fenced_write(a6xx_gpu, offset, lower_32_bits(value), mask);
>> + if (ret)
>> + return ret;
>> +
>> + if (!is_64b)
>> + return 0;
>> +
>> + ret = fenced_write(a6xx_gpu, offset + 1, upper_32_bits(value), mask);
>> +
>> + return ret;
>> +}
>> +
>> static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
>> {
>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> @@ -86,7 +164,7 @@ static void a6xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
>> /* Update HW if this is the current ring and we are not in preempt*/
>> if (!a6xx_in_preempt(a6xx_gpu)) {
>> if (a6xx_gpu->cur_ring == ring)
>> - gpu_write(gpu, REG_A6XX_CP_RB_WPTR, wptr);
>> + a6xx_fenced_write(a6xx_gpu, REG_A6XX_CP_RB_WPTR, wptr, BIT(0), false);
>> else
>> ring->restore_wptr = true;
>> } else {
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> index 6e71f617fc3d0d564e51650dfed63a18f31042ac..e736c59d566b3fcf8c62a212494e3b110c09caa9 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> @@ -295,5 +295,6 @@ int a6xx_gpu_state_put(struct msm_gpu_state *state);
>>
>> void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
>> void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
>> +int a6xx_fenced_write(struct a6xx_gpu *gpu, u32 offset, u64 value, u32 mask, bool is_64b);
>
> "mask" makes it sound like it's the mask for a masked write, which it
> isn't. At least in the public API I'd name it something more explicit
> like "fence_status_mask". Also it would be nice to add defines like
> GMU_FENCE_STATUS_WPTR/CONTEXT_SWITCH to make the parameter values in
> callsites less magical. Finally, this might be personal preference,
> but it's not immediately obvious what the "true"/"false" in callsites
> mean, so it would make users clearer to add a separate
> "a6xx_fenced_write64" and make 64-bit reg writes use that instead of
> is_64b.
I agree about the BIT definition. Will update if I send another
revision. Same for the 'mask'. I can see the confusion due to write and
mask in the same line.
64B fenced write is used only at a single place (in the preempt trigger
call). So I feel it is an overkill to create another function for that.
I did weigh that option earlier though.
-Akhil
>
> Connor
>
>>
Powered by blists - more mailing lists