[PATCH 2/2] drm/amdgpu: attempt xgmi perfmon re-arm on failed arm
Felix Kuehling
felix.kuehling at amd.com
Tue Dec 17 22:18:16 UTC 2019
On 2019-12-17 12:28, Jonathan Kim wrote:
> The DF routines to arm xGMI performance will attempt to re-arm both on
> performance monitoring start and read on initial failure to arm.
>
> Signed-off-by: Jonathan Kim <Jonathan.Kim at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 153 ++++++++++++++++++++-------
> 1 file changed, 117 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
> index 4043ebcea5de..af445054305f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
> +++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
> @@ -162,25 +162,45 @@ static void df_v3_6_perfmon_rreg(struct amdgpu_device *adev,
> }
>
> /*
> - * df_v3_6_perfmon_wreg - write to perfmon lo and hi
> - *
> - * required to be atomic. no mmio method provided so subsequent reads after
> - * data writes cannot occur to preserve data fabrics finite state machine.
> + * retry arming counters every 100 usecs within 1 millisecond interval.
> + * if retry fails after time out, return error.
> */
> -static void df_v3_6_perfmon_wreg(struct amdgpu_device *adev, uint32_t lo_addr,
> - uint32_t lo_val, uint32_t hi_addr, uint32_t hi_val)
> +#define ARM_RETRY_USEC_TIMEOUT 1000
> +#define ARM_RETRY_USEC_INTERVAL 100
> +static int df_v3_6_perfmon_arm_with_retry(struct amdgpu_device *adev,
> + uint32_t lo_addr, uint32_t lo_val,
> + uint32_t hi_addr, uint32_t hi_val)
> {
> unsigned long flags, address, data;
> + uint32_t lo_val_rb, hi_val_rb;
> + int countdown = ARM_RETRY_USEC_TIMEOUT;
>
> address = adev->nbio.funcs->get_pcie_index_offset(adev);
> data = adev->nbio.funcs->get_pcie_data_offset(adev);
>
> spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> - WREG32(address, lo_addr);
> - WREG32(data, lo_val);
> - WREG32(address, hi_addr);
> - WREG32(data, hi_val);
> +
> + while (countdown) {
> + WREG32(address, lo_addr);
> + WREG32(data, lo_val);
> + WREG32(address, hi_addr);
> + WREG32(data, hi_val);
> +
> + WREG32(address, lo_addr);
> + lo_val_rb = RREG32(data);
> + WREG32(address, hi_addr);
> + hi_val_rb = RREG32(data);
> +
> + if (lo_val == lo_val_rb && hi_val == hi_val_rb)
> + break;
> +
> + countdown -= ARM_RETRY_USEC_INTERVAL;
> + udelay(ARM_RETRY_USEC_INTERVAL);
> + }
> +
> spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
I don't think it's a good idea to hold the spin lock for the entire
duration of this retry loop. Maybe put that inside the loop and release
the lock while waiting in udelay.
> +
> + return countdown > 0 ? 0 : -ETIME;
> }
>
> /* get the number of df counters available */
> @@ -334,20 +354,20 @@ static void df_v3_6_pmc_get_addr(struct amdgpu_device *adev,
> switch (target_cntr) {
>
> case 0:
> - *lo_base_addr = is_ctrl ? smnPerfMonCtlLo0 : smnPerfMonCtrLo0;
> - *hi_base_addr = is_ctrl ? smnPerfMonCtlHi0 : smnPerfMonCtrHi0;
> + *lo_base_addr = is_ctrl ? smnPerfMonCtlLo4 : smnPerfMonCtrLo4;
> + *hi_base_addr = is_ctrl ? smnPerfMonCtlHi4 : smnPerfMonCtrHi4;
> break;
> case 1:
> - *lo_base_addr = is_ctrl ? smnPerfMonCtlLo1 : smnPerfMonCtrLo1;
> - *hi_base_addr = is_ctrl ? smnPerfMonCtlHi1 : smnPerfMonCtrHi1;
> + *lo_base_addr = is_ctrl ? smnPerfMonCtlLo5 : smnPerfMonCtrLo5;
> + *hi_base_addr = is_ctrl ? smnPerfMonCtlHi5 : smnPerfMonCtrHi5;
> break;
> case 2:
> - *lo_base_addr = is_ctrl ? smnPerfMonCtlLo2 : smnPerfMonCtrLo2;
> - *hi_base_addr = is_ctrl ? smnPerfMonCtlHi2 : smnPerfMonCtrHi2;
> + *lo_base_addr = is_ctrl ? smnPerfMonCtlLo6 : smnPerfMonCtrLo6;
> + *hi_base_addr = is_ctrl ? smnPerfMonCtlHi6 : smnPerfMonCtrHi6;
> break;
> case 3:
> - *lo_base_addr = is_ctrl ? smnPerfMonCtlLo3 : smnPerfMonCtrLo3;
> - *hi_base_addr = is_ctrl ? smnPerfMonCtlHi3 : smnPerfMonCtrHi3;
> + *lo_base_addr = is_ctrl ? smnPerfMonCtlLo7 : smnPerfMonCtrLo7;
> + *hi_base_addr = is_ctrl ? smnPerfMonCtlHi7 : smnPerfMonCtrHi7;
> break;
>
> }
> @@ -422,6 +442,42 @@ static int df_v3_6_pmc_add_cntr(struct amdgpu_device *adev,
> return -ENOSPC;
> }
>
> +#define DEFERRED_ARM_MASK (1 << 31)
> +static int df_v3_6_pmc_defer_cntr(struct amdgpu_device *adev,
> + uint64_t config, int err)
Consider renaming this function. I found its usage confusing because
it's used to defer arming as well as clearing the deferred flag. Maybe
df_v3_6_pmc_set_deferred. The "err" parameter could be named "defer" to
better indicate its meaning and maybe make it bool, since that's what's
returned by the counterpart df_v3_6_pmc_is_deferred.
> +{
> + int target_cntr;
> +
> + target_cntr = df_v3_6_pmc_config_2_cntr(adev, config);
> +
> + if (target_cntr < 0)
> + return -EINVAL;
> +
> + if (err)
> + adev->df_perfmon_config_assign_mask[target_cntr] |=
> + DEFERRED_ARM_MASK;
> + else
> + adev->df_perfmon_config_assign_mask[target_cntr] &=
> + ~DEFERRED_ARM_MASK;
> +
> + return 0;
> +}
> +
> +static bool df_v3_6_pmc_is_deferred(struct amdgpu_device *adev,
> + uint64_t config)
> +{
> + int target_cntr;
> +
> + target_cntr = df_v3_6_pmc_config_2_cntr(adev, config);
> +
> + if (target_cntr < 0)
> + return -EINVAL;
> +
> + return (adev->df_perfmon_config_assign_mask[target_cntr]
> + & DEFERRED_ARM_MASK);
> +
> +}
> +
> /* release performance counter */
> static void df_v3_6_pmc_release_cntr(struct amdgpu_device *adev,
> uint64_t config)
> @@ -433,7 +489,7 @@ static void df_v3_6_pmc_release_cntr(struct amdgpu_device *adev,
> }
>
>
> -static void df_v3_6_reset_perfmon_cntr(struct amdgpu_device *adev,
> +static int df_v3_6_reset_perfmon_cntr(struct amdgpu_device *adev,
> uint64_t config)
> {
> uint32_t lo_base_addr, hi_base_addr;
> @@ -442,38 +498,54 @@ static void df_v3_6_reset_perfmon_cntr(struct amdgpu_device *adev,
> &hi_base_addr);
>
> if ((lo_base_addr == 0) || (hi_base_addr == 0))
> - return;
> + return -EINVAL;
>
> - df_v3_6_perfmon_wreg(adev, lo_base_addr, 0, hi_base_addr, 0);
> + return df_v3_6_perfmon_arm_with_retry(adev, lo_base_addr, 0,
> + hi_base_addr, 0);
> }
>
> static int df_v3_6_pmc_start(struct amdgpu_device *adev, uint64_t config,
> int is_enable)
> {
> uint32_t lo_base_addr, hi_base_addr, lo_val, hi_val;
> - int ret = 0;
> + int err = 0, ret = 0;
>
> switch (adev->asic_type) {
> case CHIP_VEGA20:
> -
> - df_v3_6_reset_perfmon_cntr(adev, config);
> -
> if (is_enable) {
> ret = df_v3_6_pmc_add_cntr(adev, config);
> - } else {
> - ret = df_v3_6_pmc_get_ctrl_settings(adev,
> + return ret;
This could be one line
return df_v3_6_pmc_get_cntr(adev, config);
> + }
> +
> + err = df_v3_6_reset_perfmon_cntr(adev, config);
> +
> + ret = df_v3_6_pmc_defer_cntr(adev, config, err);
This is confusing. This looks like you're always deferring, but in fact
this is conditional based on err. See my suggesting about renaming the
function above.
> +
> + if (err || ret)
> + return ret;
> +
> + ret = df_v3_6_pmc_get_ctrl_settings(adev,
> config,
> &lo_base_addr,
> &hi_base_addr,
> &lo_val,
> &hi_val);
>
> - if (ret)
> - return ret;
> + if (ret)
> + return ret;
>
> - df_v3_6_perfmon_wreg(adev, lo_base_addr, lo_val,
> - hi_base_addr, hi_val);
> - }
> + /*
> + * if arm with retries fail, mark reserved counter high bit to
> + * indicate that event arm has failed. retry will occur
> + * during pmc_count in this case.
You're explaining the implementation of df_v3_6_pmc_defer_cntr rather
than the abstract interface. That's more confusing than helpful.
> + */
> + err = df_v3_6_perfmon_arm_with_retry(adev,
> + lo_base_addr,
> + lo_val,
> + hi_base_addr,
> + hi_val);
> +
> + ret = df_v3_6_pmc_defer_cntr(adev, config, err);
>
> break;
> default:
> @@ -501,7 +573,7 @@ static int df_v3_6_pmc_stop(struct amdgpu_device *adev, uint64_t config,
> if (ret)
> return ret;
>
> - df_v3_6_perfmon_wreg(adev, lo_base_addr, 0, hi_base_addr, 0);
> + df_v3_6_reset_perfmon_cntr(adev, config);
Is this guaranteed to succeed? If not, we should check the return value.
>
> if (is_disable)
> df_v3_6_pmc_release_cntr(adev, config);
> @@ -518,18 +590,28 @@ static void df_v3_6_pmc_get_count(struct amdgpu_device *adev,
> uint64_t config,
> uint64_t *count)
> {
> - uint32_t lo_base_addr, hi_base_addr, lo_val, hi_val;
> + uint32_t lo_base_addr, hi_base_addr, lo_val = 0, hi_val = 0;
> + int rearm_err = 0;
> *count = 0;
>
> switch (adev->asic_type) {
> case CHIP_VEGA20:
> -
> df_v3_6_pmc_get_read_settings(adev, config, &lo_base_addr,
> &hi_base_addr);
>
> if ((lo_base_addr == 0) || (hi_base_addr == 0))
> return;
>
> + /* rearm the counter or throw away count value on failure */
> + if (df_v3_6_pmc_is_deferred(adev, config)) {
> + rearm_err = df_v3_6_perfmon_arm_with_retry(
> + adev, lo_base_addr, lo_val,
> + hi_base_addr, hi_val);
Is it a good idea to retry here? I believe this is called under a
spin-lock. The get_count should usually be fast. Is it worth retrying
and potentially delaying the entire perf framework for a millisecond?
Regards,
Felix
> + }
> +
> + if (rearm_err)
> + return;
> +
> df_v3_6_perfmon_rreg(adev, lo_base_addr, &lo_val,
> hi_base_addr, &hi_val);
>
> @@ -542,7 +624,6 @@ static void df_v3_6_pmc_get_count(struct amdgpu_device *adev,
> config, lo_base_addr, hi_base_addr, lo_val, hi_val);
>
> break;
> -
> default:
> break;
> }
More information about the amd-gfx
mailing list