On Tue, 2022-03-08 at 19:32 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> I'd like to see the SAGV block time we got from the mailbox
> in the logs regardless of whether other factors prevent the
> use of SAGV.
>
> So let's adjust the code to always query the SAGV block time,
> log it, and then reset it if SAGV is not actually supported.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Hi Ville,
I wonder if it is better to comprehend if we clean assigning -1 to a u32 variable first before
this? I see that you are doing that anyway in the next patch in this series. Was there any
particular reason?
BR
Vinod
> ---
> drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++++++--------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8ee31c9590a7..21c37115c36e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3670,8 +3670,8 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
> dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
> }
>
> -static void
> -skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> +static u32
> +intel_sagv_block_time(struct drm_i915_private *dev_priv)
> {
> if (DISPLAY_VER(dev_priv) >= 12) {
> u32 val = 0;
> @@ -3680,24 +3680,31 @@ skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> ret = snb_pcode_read(dev_priv,
> GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
> &val, NULL);
> - if (!ret) {
> - dev_priv->sagv_block_time_us = val;
> - return;
> + if (ret) {
> + drm_dbg_kms(&dev_priv->drm, "Couldn't read SAGV block time!\n");
> + return -1;
> }
>
> - drm_dbg(&dev_priv->drm, "Couldn't read SAGV block time!\n");
> + return val;
> } else if (DISPLAY_VER(dev_priv) == 11) {
> - dev_priv->sagv_block_time_us = 10;
> - return;
> - } else if (DISPLAY_VER(dev_priv) == 9) {
> - dev_priv->sagv_block_time_us = 30;
> - return;
> + return 10;
> + } else if (DISPLAY_VER(dev_priv) == 9 && !IS_LP(dev_priv)) {
> + return 30;
> } else {
> - MISSING_CASE(DISPLAY_VER(dev_priv));
> + /* Default to an unusable block time */
> + return -1;
> }
> +}
>
> - /* Default to an unusable block time */
> - dev_priv->sagv_block_time_us = -1;
> +static void intel_sagv_init(struct drm_i915_private *i915)
> +{
> + i915->sagv_block_time_us = intel_sagv_block_time(i915);
> +
> + drm_dbg_kms(&i915->drm, "SAGV supported: %s, original SAGV block time: %u us\n",
> + str_yes_no(intel_has_sagv(i915)), i915->sagv_block_time_us);
> +
> + if (!intel_has_sagv(i915))
> + i915->sagv_block_time_us = -1;
> }
>
> /*
> @@ -8173,8 +8180,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
> else if (GRAPHICS_VER(dev_priv) == 5)
> ilk_get_mem_freq(dev_priv);
>
> - if (intel_has_sagv(dev_priv))
> - skl_setup_sagv_block_time(dev_priv);
> + intel_sagv_init(dev_priv);
>
> /* For FIFO watermark updates */
> if (DISPLAY_VER(dev_priv) >= 9) {
