On 12/16/2024 10:28 PM, Connor Abbott wrote:
> On Mon, Dec 16, 2024 at 11:55 AM Akhil P Oommen
> <quic_akhilpo@xxxxxxxxxxx> wrote:
>>
>> On 12/13/2024 10:40 PM, Antonino Maniscalco wrote:
>>> On 12/13/24 5:50 PM, Akhil P Oommen wrote:
>>>> On 12/12/2024 9:44 PM, Antonino Maniscalco wrote:
>>>>> On 12/12/24 4:58 PM, Akhil P Oommen wrote:
>>>>>> On 12/5/2024 10:24 PM, Rob Clark wrote:
>>>>>>> From: Rob Clark <robdclark@xxxxxxxxxxxx>
>>>>>>>
>>>>>>> Performance counter usage falls into two categories:
>>>>>>>
>>>>>>> 1. Local usage, where the counter configuration, start, and end read
>>>>>>> happen within (locally to) a single SUBMIT. In this case,
>>>>>>> there is
>>>>>>> no dependency on counter configuration or values between submits,
>>>>>>> and
>>>>>>> in fact counters are normally cleared on context switches,
>>>>>>> making it
>>>>>>> impossible to rely on cross-submit state.
>>>>>>>
>>>>>>> 2. Global usage, where a single privilaged daemon/process is sampling
>>>>>>> counter values across all processes for profiling.
>>>>>>>
>>>>>>> The two categories are mutually exclusive. While you can have many
>>>>>>> processes making local counter usage, you cannot combine global and
>>>>>>> local usage without the two stepping on each others feet (by changing
>>>>>>> counter configuration).
>>>>>>>
>>>>>>> For global counter usage, there is already a SYSPROF param (since
>>>>>>> global
>>>>>>> counter usage requires disabling counter clearing on context switch).
>>>>>>> This patch adds a REQ_CNTRS param to request local counter usage. If
>>>>>>> one or more processes has requested counter usage, then a SYSPROF
>>>>>>> request will fail with -EBUSY. And if SYSPROF is active, then
>>>>>>> REQ_CNTRS
>>>>>>> will fail with -EBUSY, maintaining the mutual exclusivity.
>>>>>>>
>>>>>>> This is purely an advisory interface to help coordinate userspace.
>>>>>>> There is no real means of enforcement, but the worst that can
>>>>>>> happen if
>>>>>>> userspace ignores a REQ_CNTRS failure is that you'll get nonsense
>>>>>>> profiling data.
>>>>>>>
>>>>>>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> kgsl takes a different approach, which involves a lot more UABI for
>>>>>>> assigning counters to different processes. But I think by taking
>>>>>>> advantage of the fact that mesa (freedreno+turnip) reconfigure the
>>>>>>> counters they need in each SUBMIT, for their respective gl/vk perf-
>>>>>>> counter extensions, we can take this simpler approach.
>>>>>>
>>>>>> KGSL's approach is preemption and ifpc safe (also whatever HW changes
>>>>>> that will come up in future generations). How will we ensure that here?
>>>>>>
>>>>>> I have plans to bring up IFPC support in near future. Also, I
>>>>>> brought up
>>>>>> this point during preemption series. But from the responses, I felt
>>>>>> that
>>>>>> profiling was not considered a serious usecase. Still I wonder how the
>>>>>> perfcounter extensions work accurately with preemption.
>>>>>
>>>>> So back then I implemented the postamble IB to clear perf counters and
>>>>> that gets disabled when sysprof (so global usage) is happening. The
>>>>> kernel is oblivious to "Local isage" of profiling but in that case
>>>>> really what we want to do is disable preemption which in my
>>>>> understanding can be done from userspace with a PKT. In my understanding
>>>>> this had us covered for all usecases.
>>>>
>>>> I think this wasn't mentioned at that time. Which UMD PKT in a6x+ did
>>>> you mean?
>>>
>>> Ah, I thought it wasmentioned, sorry.
>>> The packet I was referring to is:
>>> <doc> Make next dword 1 to disable preemption, 0 to re-enable it. </
>>> doc>
>>> <value name="CP_PREEMPT_DISABLE" value="0x6c" variants="A6XX"/>
>>
>> Ah! Okay. I think this packet is not used by the downstream blob. IMO,
>> disabling preemption is still a suboptimal solution.
>
> Downstream doesn't expose userspace perfcounters (i.e.
> VK_KHR_performance_query) at all. My understanding is that Android
> requires you not to expose them for security reasons, but I could be
> wrong there. In any case, Qualcomm clearly hasn't really thought
> through what it would take to make everything work well with userspace
> perfcounters and hasn't implemented the necessary firmware bits for
> that, so we're left muddling through and doing what we can.
>
Honestly, I don't know what you meant by the missing support.
-Akhil.
> Connor
>
>>
>>>
>>> BTW you mentioned wanting to look into IFPC. Since I too wanted to look
>>> into implementing it wonder if you could let me know when you planned on
>>> working on it.
>>
>> I have few patches in progress. Nothing final yet and need verification
>> on the hw side. Also, I need to do some housekeeping here to debug gmu
>> issues since IFPC increases the probability of those a lot.
>>
>> I will try to send out the patches very soon.
>>
>> -Akhil.
>>
>>>
>>>>
>>>> -Akhil.
>>>>
>>>>>
>>>>> So what would you expect instead we should do kernel side to make
>>>>> profiling preemption safe?
>>>>>
>>>>>>
>>>>>> -Akhil
>>>>>>
>>>>>>>
>>>>>>> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +
>>>>>>> drivers/gpu/drm/msm/msm_drv.c | 5 ++-
>>>>>>> drivers/gpu/drm/msm/msm_gpu.c | 1 +
>>>>>>> drivers/gpu/drm/msm/msm_gpu.h | 29 +++++++++++++-
>>>>>>> drivers/gpu/drm/msm/msm_submitqueue.c | 52 +++++++++++++++++++
>>>>>>> +++++-
>>>>>>> include/uapi/drm/msm_drm.h | 1 +
>>>>>>> 6 files changed, 85 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/
>>>>>>> drm/msm/adreno/adreno_gpu.c
>>>>>>> index 31bbf2c83de4..f688e37059b8 100644
>>>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>>>>> @@ -441,6 +441,8 @@ int adreno_set_param(struct msm_gpu *gpu, struct
>>>>>>> msm_file_private *ctx,
>>>>>>> if (!capable(CAP_SYS_ADMIN))
>>>>>>> return UERR(EPERM, drm, "invalid permissions");
>>>>>>> return msm_file_private_set_sysprof(ctx, gpu, value);
>>>>>>> + case MSM_PARAM_REQ_CNTRS:
>>>>>>> + return msm_file_private_request_counters(ctx, gpu, value);
>>>>>>> default:
>>>>>>> return UERR(EINVAL, drm, "%s: invalid param: %u", gpu-
>>>>>>>> name, param);
>>>>>>> }
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/
>>>>>>> msm_drv.c
>>>>>>> index 6416d2cb4efc..bf8314ff4a25 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>>>>>> @@ -377,9 +377,12 @@ static void msm_postclose(struct drm_device
>>>>>>> *dev, struct drm_file *file)
>>>>>>> * It is not possible to set sysprof param to non-zero if gpu
>>>>>>> * is not initialized:
>>>>>>> */
>>>>>>> - if (priv->gpu)
>>>>>>> + if (ctx->sysprof)
>>>>>>> msm_file_private_set_sysprof(ctx, priv->gpu, 0);
>>>>>>> + if (ctx->counters_requested)
>>>>>>> + msm_file_private_request_counters(ctx, priv->gpu, 0);
>>>>>>> +
>>>>>>> context_close(ctx);
>>>>>>> }
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/
>>>>>>> msm_gpu.c
>>>>>>> index 82f204f3bb8f..013b59ca3bb1 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>>>>>>> @@ -991,6 +991,7 @@ int msm_gpu_init(struct drm_device *drm, struct
>>>>>>> platform_device *pdev,
>>>>>>> gpu->nr_rings = nr_rings;
>>>>>>> refcount_set(&gpu->sysprof_active, 1);
>>>>>>> + refcount_set(&gpu->local_counters_active, 1);
>>>>>>> return 0;
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/
>>>>>>> msm_gpu.h
>>>>>>> index e25009150579..83c61e523b1b 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_gpu.h
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
>>>>>>> @@ -195,12 +195,28 @@ struct msm_gpu {
>>>>>>> int nr_rings;
>>>>>>> /**
>>>>>>> - * sysprof_active:
>>>>>>> + * @sysprof_active:
>>>>>>> *
>>>>>>> - * The count of contexts that have enabled system profiling.
>>>>>>> + * The count of contexts that have enabled system profiling plus
>>>>>>> one.
>>>>>>> + *
>>>>>>> + * Note: refcount_t does not like 0->1 transitions.. we want to
>>>>>>> keep
>>>>>>> + * the under/overflow checks that refcount_t provides, but allow
>>>>>>> + * multiple on/off transitions so we track the logical value
>>>>>>> plus one.)
>>>>>>> */
>>>>>>> refcount_t sysprof_active;
>>>>>>> + /**
>>>>>>> + * @local_counters_active:
>>>>>>> + *
>>>>>>> + * The count of contexts that have requested local (intra-submit)
>>>>>>> + * performance counter usage plus one.
>>>>>>> + *
>>>>>>> + * Note: refcount_t does not like 0->1 transitions.. we want to
>>>>>>> keep
>>>>>>> + * the under/overflow checks that refcount_t provides, but allow
>>>>>>> + * multiple on/off transitions so we track the logical value
>>>>>>> plus one.)
>>>>>>> + */
>>>>>>> + refcount_t local_counters_active;
>>>>>>> +
>>>>>>> /**
>>>>>>> * lock:
>>>>>>> *
>>>>>>> @@ -383,6 +399,13 @@ struct msm_file_private {
>>>>>>> */
>>>>>>> int sysprof;
>>>>>>> + /**
>>>>>>> + * @counters_requested:
>>>>>>> + *
>>>>>>> + * Has the context requested local perfcntr usage.
>>>>>>> + */
>>>>>>> + bool counters_requested;
>>>>>>> +
>>>>>>> /**
>>>>>>> * comm: Overridden task comm, see MSM_PARAM_COMM
>>>>>>> *
>>>>>>> @@ -626,6 +649,8 @@ void msm_submitqueue_destroy(struct kref *kref);
>>>>>>> int msm_file_private_set_sysprof(struct msm_file_private *ctx,
>>>>>>> struct msm_gpu *gpu, int sysprof);
>>>>>>> +int msm_file_private_request_counters(struct msm_file_private *ctx,
>>>>>>> + struct msm_gpu *gpu, int reqcntrs);
>>>>>>> void __msm_file_private_destroy(struct kref *kref);
>>>>>>> static inline void msm_file_private_put(struct msm_file_private
>>>>>>> *ctx)
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/
>>>>>>> msm/msm_submitqueue.c
>>>>>>> index 7fed1de63b5d..1e1e21e6f7ae 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
>>>>>>> @@ -10,6 +10,15 @@
>>>>>>> int msm_file_private_set_sysprof(struct msm_file_private *ctx,
>>>>>>> struct msm_gpu *gpu, int sysprof)
>>>>>>> {
>>>>>>> + int ret = 0;
>>>>>>> +
>>>>>>> + mutex_lock(&gpu->lock);
>>>>>>> +
>>>>>>> + if (sysprof && (refcount_read(&gpu->local_counters_active) >
>>>>>>> 1)) {
>>>>>>> + ret = UERR(EBUSY, gpu->dev, "Local counter usage active");
>>>>>>> + goto out_unlock;
>>>>>>> + }
>>>>>>> +
>>>>>>> /*
>>>>>>> * Since pm_runtime and sysprof_active are both refcounts, we
>>>>>>> * call apply the new value first, and then unwind the previous
>>>>>>> @@ -18,7 +27,8 @@ int msm_file_private_set_sysprof(struct
>>>>>>> msm_file_private *ctx,
>>>>>>> switch (sysprof) {
>>>>>>> default:
>>>>>>> - return UERR(EINVAL, gpu->dev, "Invalid sysprof: %d",
>>>>>>> sysprof);
>>>>>>> + ret = UERR(EINVAL, gpu->dev, "Invalid sysprof: %d", sysprof);
>>>>>>> + goto out_unlock;
>>>>>>> case 2:
>>>>>>> pm_runtime_get_sync(&gpu->pdev->dev);
>>>>>>> fallthrough;
>>>>>>> @@ -43,7 +53,45 @@ int msm_file_private_set_sysprof(struct
>>>>>>> msm_file_private *ctx,
>>>>>>> ctx->sysprof = sysprof;
>>>>>>> - return 0;
>>>>>>> +out_unlock:
>>>>>>> + mutex_unlock(&gpu->lock);
>>>>>>> +
>>>>>>> + return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int msm_file_private_request_counters(struct msm_file_private *ctx,
>>>>>>> + struct msm_gpu *gpu, int reqctrs)
>>>>>>> +{
>>>>>>> + int ret = 0;
>>>>>>> +
>>>>>>> + mutex_lock(&gpu->lock);
>>>>>>> +
>>>>>>> + if (reqctrs && (refcount_read(&gpu->sysprof_active) > 1)) {
>>>>>>> + ret = UERR(EBUSY, gpu->dev, "System profiling active");
>>>>>>> + goto out_unlock;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (reqctrs) {
>>>>>>> + if (ctx->counters_requested) {
>>>>>>> + ret = UERR(EINVAL, gpu->dev, "Already requested");
>>>>>>> + goto out_unlock;
>>>>>>> + }
>>>>>>> +
>>>>>>> + ctx->counters_requested = true;
>>>>>>> + refcount_inc(&gpu->local_counters_active);
>>>>>>> + } else {
>>>>>>> + if (!ctx->counters_requested) {
>>>>>>> + ret = UERR(EINVAL, gpu->dev, "Not requested");
>>>>>>> + goto out_unlock;
>>>>>>> + }
>>>>>>> + refcount_dec(&gpu->local_counters_active);
>>>>>>> + ctx->counters_requested = false;
>>>>>>> + }
>>>>>>> +
>>>>>>> +out_unlock:
>>>>>>> + mutex_unlock(&gpu->lock);
>>>>>>> +
>>>>>>> + return ret;
>>>>>>> }
>>>>>>> void __msm_file_private_destroy(struct kref *kref)
>>>>>>> diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
>>>>>>> index 2342cb90857e..ae7fb355e4a1 100644
>>>>>>> --- a/include/uapi/drm/msm_drm.h
>>>>>>> +++ b/include/uapi/drm/msm_drm.h
>>>>>>> @@ -91,6 +91,7 @@ struct drm_msm_timespec {
>>>>>>> #define MSM_PARAM_UBWC_SWIZZLE 0x12 /* RO */
>>>>>>> #define MSM_PARAM_MACROTILE_MODE 0x13 /* RO */
>>>>>>> #define MSM_PARAM_UCHE_TRAP_BASE 0x14 /* RO */
>>>>>>> +#define MSM_PARAM_REQ_CNTRS 0x15 /* WO: request "local" (intra-
>>>>>>> submit) perfcntr usage */
>>>>>>> /* For backwards compat. The original support for preemption was
>>>>>>> based on
>>>>>>> * a single ring per priority level so # of priority levels equals
>>>>>>> the #
>>>>>>
>>>>>
>>>>>
>>>>> Best regards,
>>>>
>>>
>>>
>>> Best regards,
>>
