| 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: <4df30c9e-8604-42aa-bf48-c8bb508429ad@gmail.com>
Date: Thu, 12 Sep 2024 13:15:34 +0200
From: Antonino Maniscalco <antomani103@...il.com>
To: Akhil P Oommen <quic_akhilpo@...cinc.com>
Cc: Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Marijn Suijten <marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
Jonathan Corbet <corbet@....net>, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 06/10] drm/msm/A6xx: Use posamble to reset counters on
preemption
On 9/12/24 9:12 AM, Akhil P Oommen wrote:
> On Wed, Sep 11, 2024 at 12:35:08AM +0200, Antonino Maniscalco wrote:
>> On 9/10/24 11:34 PM, Akhil P Oommen wrote:
>>> On Mon, Sep 09, 2024 at 05:07:42PM +0200, Antonino Maniscalco wrote:
>>>> On 9/6/24 10:08 PM, Akhil P Oommen wrote:
>>>>> On Thu, Sep 05, 2024 at 04:51:24PM +0200, Antonino Maniscalco wrote:
>>>>>> Use the postamble to reset perf counters when switching between rings,
>>>>>> except when sysprof is enabled, analogously to how they are reset
>>>>>> between submissions when switching pagetables.
>>>>>>
>>>>>> Signed-off-by: Antonino Maniscalco <antomani103@...il.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 20 ++++++++++++++++++-
>>>>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 5 +++++
>>>>>> drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 32 +++++++++++++++++++++++++++++++
>>>>>> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 7 +++++--
>>>>>> 4 files changed, 61 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>>> index ed0b138a2d66..710ec3ce2923 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>>> @@ -366,7 +366,8 @@ static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>>>>> static void a6xx_emit_set_pseudo_reg(struct msm_ringbuffer *ring,
>>>>>> struct a6xx_gpu *a6xx_gpu, struct msm_gpu_submitqueue *queue)
>>>>>> {
>>>>>> - u64 preempt_offset_priv_secure;
>>>>>> + bool sysprof = refcount_read(&a6xx_gpu->base.base.sysprof_active) > 1;
>>>>>> + u64 preempt_offset_priv_secure, preempt_postamble;
>>>>>> OUT_PKT7(ring, CP_SET_PSEUDO_REG, 15);
>>>>>> @@ -398,6 +399,23 @@ static void a6xx_emit_set_pseudo_reg(struct msm_ringbuffer *ring,
>>>>>> /* seems OK to set to 0 to disable it */
>>>>>> OUT_RING(ring, 0);
>>>>>> OUT_RING(ring, 0);
>>>>>> +
>>>>>> + /* if not profiling set postamble to clear perfcounters, else clear it */
>>>>>> + if (!sysprof && a6xx_gpu->preempt_postamble_len) {
>>>
>>> Setting len = 0 is enough to skip processing postamble packets. So how
>>> about a simpler:
>>>
>>> len = a6xx_gpu->preempt_postamble_len;
>>> if (sysprof)
>>> len = 0;
>>>
>>> OUT_PKT7(ring, CP_SET_AMBLE, 3);
>>> OUT_RING(ring, lower_32_bits(preempt_postamble));
>>> OUT_RING(ring, upper_32_bits(preempt_postamble));
>>> OUT_RING(ring, CP_SET_AMBLE_2_DWORDS(len) |
>>> CP_SET_AMBLE_2_TYPE(KMD_AMBLE_TYPE));
>>>
>>>>>> + preempt_postamble = a6xx_gpu->preempt_postamble_iova;
>>>>>> +
>>>>>> + OUT_PKT7(ring, CP_SET_AMBLE, 3);
>>>>>> + OUT_RING(ring, lower_32_bits(preempt_postamble));
>>>>>> + OUT_RING(ring, upper_32_bits(preempt_postamble));
>>>>>> + OUT_RING(ring, CP_SET_AMBLE_2_DWORDS(
>>>>>> + a6xx_gpu->preempt_postamble_len) |
>>>>>> + CP_SET_AMBLE_2_TYPE(KMD_AMBLE_TYPE));
>>>>>> + } else {
>>>>>
>>>>> Why do we need this else part?
>>>>
>>>> Wouldn't the postmable remain set if we don't explicitly set it to 0?
>>>
>>> Aah, that is a genuine concern. I am not sure! Lets keep it.
>>>
>>>>
>>>>>
>>>>>> + OUT_PKT7(ring, CP_SET_AMBLE, 3);
>>>>>> + OUT_RING(ring, 0);
>>>>>> + OUT_RING(ring, 0);
>>>>>> + OUT_RING(ring, CP_SET_AMBLE_2_TYPE(KMD_AMBLE_TYPE));
>>>>>> + }
>>>>>> }
>>>>>> static void a7xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>>>>>> index da10060e38dc..b009732c08c5 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>>>>>> @@ -71,6 +71,11 @@ struct a6xx_gpu {
>>>>>> bool uses_gmem;
>>>>>> bool skip_save_restore;
>>>>>> + struct drm_gem_object *preempt_postamble_bo;
>>>>>> + void *preempt_postamble_ptr;
>>>>>> + uint64_t preempt_postamble_iova;
>>>>>> + uint64_t preempt_postamble_len;
>>>>>> +
>>>>>> struct a6xx_gmu gmu;
>>>>>> struct drm_gem_object *shadow_bo;
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c
>>>>>> index 1caff76aca6e..ec44f44d925f 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c
>>>>>> @@ -346,6 +346,28 @@ static int preempt_init_ring(struct a6xx_gpu *a6xx_gpu,
>>>>>> return 0;
>>>>>> }
>>>>>> +static void preempt_prepare_postamble(struct a6xx_gpu *a6xx_gpu)
>>>>>> +{
>>>>>> + u32 *postamble = a6xx_gpu->preempt_postamble_ptr;
>>>>>> + u32 count = 0;
>>>>>> +
>>>>>> + postamble[count++] = PKT7(CP_REG_RMW, 3);
>>>>>> + postamble[count++] = REG_A6XX_RBBM_PERFCTR_SRAM_INIT_CMD;
>>>>>> + postamble[count++] = 0;
>>>>>> + postamble[count++] = 1;
>>>>>> +
>>>>>> + postamble[count++] = PKT7(CP_WAIT_REG_MEM, 6);
>>>>>> + postamble[count++] = CP_WAIT_REG_MEM_0_FUNCTION(WRITE_EQ);
>>>>>> + postamble[count++] = CP_WAIT_REG_MEM_1_POLL_ADDR_LO(
>>>>>> + REG_A6XX_RBBM_PERFCTR_SRAM_INIT_STATUS);
>>>>>> + postamble[count++] = CP_WAIT_REG_MEM_2_POLL_ADDR_HI(0);
>>>>>> + postamble[count++] = CP_WAIT_REG_MEM_3_REF(0x1);
>>>>>> + postamble[count++] = CP_WAIT_REG_MEM_4_MASK(0x1);
>>>>>> + postamble[count++] = CP_WAIT_REG_MEM_5_DELAY_LOOP_CYCLES(0);
>>>>>
>>>>> Isn't it better to just replace this with NOP packets when sysprof is
>>>>> enabled, just before triggering preemption? It will help to have an
>>>>> immediate effect.
>>>>>
>>>>> -Akhil
>>>>>
>>>>
>>>> Mmm, this being a postamble I wonder whether we have the guarantee that it
>>>> finishes execution before the IRQ is called so updating it doesn't race with
>>>> the CP executing it.
>>>
>>> Yes, it will be complete. But on a second thought now, this suggestion from me
>>> looks like an overkill.
>>
>> Thanks for confirming! I have actually already implemented something similar
>> to what you proposed https://gitlab.com/pac85/inux/-/commit/8b8ab1d89b0f611cfdbac4c3edba4192be91a7f9
>> so we can chose between the two. Let me know your prefence.
>
> That looks fine. Can we try to simplify that patch further? We can lean
> towards readability instead of saving few writes. I don't think there
> will be frequent sysprof toggles.
>
Sure yeah, I removed the patch argument on preempt_prepare_postamble so
when we enable the postamble we just re-emit the entire IB.
> -Akhil
>
>>
>>>
>>> -Akhil.
>>>
>>>>
>>>>>> +
>>>>>> + a6xx_gpu->preempt_postamble_len = count;
>>>>>> +}
>>>>>> +
>>>>>> void a6xx_preempt_fini(struct msm_gpu *gpu)
>>>>>> {
>>>>>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>>> @@ -376,6 +398,16 @@ void a6xx_preempt_init(struct msm_gpu *gpu)
>>>>>> a6xx_gpu->uses_gmem = 1;
>>>>>> a6xx_gpu->skip_save_restore = 1;
>>>>>> + a6xx_gpu->preempt_postamble_ptr = msm_gem_kernel_new(gpu->dev,
>>>>>> + PAGE_SIZE, MSM_BO_WC | MSM_BO_MAP_PRIV,
>>>>>> + gpu->aspace, &a6xx_gpu->preempt_postamble_bo,
>>>>>> + &a6xx_gpu->preempt_postamble_iova);
>>>>>> +
>>>>>> + preempt_prepare_postamble(a6xx_gpu);
>>>>>> +
>>>>>> + if (IS_ERR(a6xx_gpu->preempt_postamble_ptr))
>>>>>> + goto fail;
>>>>>> +
>>>>>> timer_setup(&a6xx_gpu->preempt_timer, a6xx_preempt_timer, 0);
>>>>>> return;
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>>>>> index 6b1888280a83..87098567483b 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>>>>> @@ -610,12 +610,15 @@ OUT_PKT4(struct msm_ringbuffer *ring, uint16_t regindx, uint16_t cnt)
>>>>>> OUT_RING(ring, PKT4(regindx, cnt));
>>>>>> }
>>>>>> +#define PKT7(opcode, cnt) \
>>>>>> + (CP_TYPE7_PKT | (cnt << 0) | (PM4_PARITY(cnt) << 15) | \
>>>>>> + ((opcode & 0x7F) << 16) | (PM4_PARITY(opcode) << 23))
>>>>>> +
>>>>>> static inline void
>>>>>> OUT_PKT7(struct msm_ringbuffer *ring, uint8_t opcode, uint16_t cnt)
>>>>>> {
>>>>>> adreno_wait_ring(ring, cnt + 1);
>>>>>> - OUT_RING(ring, CP_TYPE7_PKT | (cnt << 0) | (PM4_PARITY(cnt) << 15) |
>>>>>> - ((opcode & 0x7F) << 16) | (PM4_PARITY(opcode) << 23));
>>>>>> + OUT_RING(ring, PKT7(opcode, cnt));
>>>>>> }
>>>>>> struct msm_gpu *a2xx_gpu_init(struct drm_device *dev);
>>>>>>
>>>>>> --
>>>>>> 2.46.0
>>>>>>
>>>>
>>>> Best regards,
>>>> --
>>>> Antonino Maniscalco <antomani103@...il.com>
>>>>
>>
>> Best regards,
>> --
>> Antonino Maniscalco <antomani103@...il.com>
>>
Best regards,
--
Antonino Maniscalco <antomani103@...il.com>
Powered by blists - more mailing lists