Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
> Don't trust cached DDB values. Recalculate the ddb value if cached value
> is zero.
>
> If i915 is build as a module, there may be a race condition when
> cursor_disable call comes even before intel_fbdev_initial_config.
> Which may lead to cached value being 0. And further commit will fail
> until a modeset.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 936eef1634c7..b67be1355e39 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>         struct drm_i915_private *dev_priv = to_i915(dev);
>         struct drm_crtc *for_crtc = cstate->base.crtc;
>         unsigned int pipe_size, ddb_size;
> +       unsigned int active_crtcs;
>         int nth_active_pipe;
>
>         if (WARN_ON(!state) || !cstate->base.active) {
> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>         }
>
>         if (intel_state->active_pipe_changes)
> -               *num_active = hweight32(intel_state->active_crtcs);
> +               active_crtcs = intel_state->active_crtcs;
>         else
> -               *num_active = hweight32(dev_priv->active_crtcs);
> +               active_crtcs = dev_priv->active_crtcs;
>
> +       *num_active = hweight32(active_crtcs);
>         ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>         WARN_ON(ddb_size == 0);
>
> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>                  * copy from old state to be sure
>                  */
>                 *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
> -               return;
> +               if (!skl_ddb_entry_size(alloc))
> +                       DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
> +               else
> +                       return;

I see 2 different patches inside this patch here. One is this chunk
here that does what commit message tells.

>         }
>
> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
> +       nth_active_pipe = hweight32(active_crtcs &
>                                     (drm_crtc_mask(for_crtc) - 1));
> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> +       pipe_size = ddb_size / hweight32(active_crtcs);

While this is fixing another bug where we were still using
intel_state->active_crtcs here even when we use dev_priv->active_crtcs
for num_active.

Am I missing or misunderstanding something?

>         alloc->start = nth_active_pipe * ddb_size / *num_active;
>         alloc->end = alloc->start + pipe_size;
>  }
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx