Re: [PATCH 4/9] drm/i915: Use plane->can_async_flip() for alignment exceptions

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2024-10-09 at 21:22 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Async flips often require bigger alignment that sync flips.
> Currently we have HAS_ASYNC_FLIPS() checks strewn about to
> inidcate that async flips are generally supported and thus
> we want more alignment. Switch that over to using
> intel_plane_can_async_flip() so that we can handle these
> in a slightly less messy way. Currently we don't have cases
> where async flips would require different alignment for
> different modifiers on the same plane.
> 
> We'll also move the HAS_ASYNC_FLIPS() check to the plane init
> code so that we can still use that as a quick way to disable
> the async flips workarounds for testing purposes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/i9xx_plane.c     | 55 +++++++++--------
> --
>  .../drm/i915/display/skl_universal_plane.c    | 21 ++++---
>  2 files changed, 37 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c
> b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index 7455da863a26..8d3346199645 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -779,12 +779,11 @@ static unsigned int
> vlv_primary_min_alignment(struct intel_plane *plane,
>                                               const struct
> drm_framebuffer *fb,
>                                               int color_plane)
>  {
> -       struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +       if (intel_plane_can_async_flip(plane, fb->modifier))
> +               return 256 * 1024;
>  
>         switch (fb->modifier) {
>         case I915_FORMAT_MOD_X_TILED:
> -               if (HAS_ASYNC_FLIPS(i915))
> -                       return 256 * 1024;
>                 return 4 * 1024;
>         case DRM_FORMAT_MOD_LINEAR:
>                 return 128 * 1024;
> @@ -798,13 +797,11 @@ static unsigned int
> g4x_primary_min_alignment(struct intel_plane *plane,
>                                               const struct
> drm_framebuffer *fb,
>                                               int color_plane)
>  {
> -       struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +       if (intel_plane_can_async_flip(plane, fb->modifier))
> +               return 256 * 1024;
>  
>         switch (fb->modifier) {
>         case I915_FORMAT_MOD_X_TILED:
> -               if (HAS_ASYNC_FLIPS(i915))
> -                       return 256 * 1024;
> -               return 4 * 1024;
>         case DRM_FORMAT_MOD_LINEAR:
>                 return 4 * 1024;
>         default:
> @@ -959,27 +956,29 @@ intel_primary_plane_create(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>         plane->get_hw_state = i9xx_plane_get_hw_state;
>         plane->check_plane = i9xx_plane_check;
>  
> -       if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -               plane->async_flip = vlv_primary_async_flip;
> -               plane->enable_flip_done =
> vlv_primary_enable_flip_done;
> -               plane->disable_flip_done =
> vlv_primary_disable_flip_done;
> -               plane->can_async_flip = i9xx_plane_can_async_flip;
> -       } else if (IS_BROADWELL(dev_priv)) {
> -               plane->need_async_flip_toggle_wa = true;
> -               plane->async_flip = g4x_primary_async_flip;
> -               plane->enable_flip_done =
> bdw_primary_enable_flip_done;
> -               plane->disable_flip_done =
> bdw_primary_disable_flip_done;
> -               plane->can_async_flip = i9xx_plane_can_async_flip;
> -       } else if (DISPLAY_VER(dev_priv) >= 7) {
> -               plane->async_flip = g4x_primary_async_flip;
> -               plane->enable_flip_done =
> ivb_primary_enable_flip_done;
> -               plane->disable_flip_done =
> ivb_primary_disable_flip_done;
> -               plane->can_async_flip = i9xx_plane_can_async_flip;
> -       } else if (DISPLAY_VER(dev_priv) >= 5) {
> -               plane->async_flip = g4x_primary_async_flip;
> -               plane->enable_flip_done =
> ilk_primary_enable_flip_done;
> -               plane->disable_flip_done =
> ilk_primary_disable_flip_done;
> -               plane->can_async_flip = i9xx_plane_can_async_flip;
> +       if (HAS_ASYNC_FLIPS(dev_priv)) {

Check for HAS_ASYNC_FLIPS could be dropped. I assume you were
considering having it would be more clear? Anyways:

Reviewed-by: Jouni Högander <jouni.hogander@intel.com>

> +               if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv)) {
> +                       plane->async_flip = vlv_primary_async_flip;
> +                       plane->enable_flip_done =
> vlv_primary_enable_flip_done;
> +                       plane->disable_flip_done =
> vlv_primary_disable_flip_done;
> +                       plane->can_async_flip =
> i9xx_plane_can_async_flip;
> +               } else if (IS_BROADWELL(dev_priv)) {
> +                       plane->need_async_flip_toggle_wa = true;
> +                       plane->async_flip = g4x_primary_async_flip;
> +                       plane->enable_flip_done =
> bdw_primary_enable_flip_done;
> +                       plane->disable_flip_done =
> bdw_primary_disable_flip_done;
> +                       plane->can_async_flip =
> i9xx_plane_can_async_flip;
> +               } else if (DISPLAY_VER(dev_priv) >= 7) {
> +                       plane->async_flip = g4x_primary_async_flip;
> +                       plane->enable_flip_done =
> ivb_primary_enable_flip_done;
> +                       plane->disable_flip_done =
> ivb_primary_disable_flip_done;
> +                       plane->can_async_flip =
> i9xx_plane_can_async_flip;
> +               } else if (DISPLAY_VER(dev_priv) >= 5) {
> +                       plane->async_flip = g4x_primary_async_flip;
> +                       plane->enable_flip_done =
> ilk_primary_enable_flip_done;
> +                       plane->disable_flip_done =
> ilk_primary_disable_flip_done;
> +                       plane->can_async_flip =
> i9xx_plane_can_async_flip;
> +               }
>         }
>  
>         modifiers = intel_fb_plane_get_modifiers(dev_priv,
> INTEL_PLANE_CAP_TILING_X);
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index ca82cc55802e..ea72c54e8329 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -589,25 +589,24 @@ static u32 tgl_plane_min_alignment(struct
> intel_plane *plane,
>         if (intel_fb_is_ccs_aux_plane(fb, color_plane))
>                 return mult * 4 * 1024;
>  
> +       /*
> +        * FIXME ADL sees GGTT/DMAR faults with async
> +        * flips unless we align to 16k at least.
> +        * Figure out what's going on here...
> +        */
> +       if (IS_ALDERLAKE_P(i915) &&
> +           intel_plane_can_async_flip(plane, fb->modifier))
> +               return mult * 16 * 1024;
> +
>         switch (fb->modifier) {
>         case DRM_FORMAT_MOD_LINEAR:
>         case I915_FORMAT_MOD_X_TILED:
>         case I915_FORMAT_MOD_Y_TILED:
>         case I915_FORMAT_MOD_4_TILED:
> -               /*
> -                * FIXME ADL sees GGTT/DMAR faults with async
> -                * flips unless we align to 16k at least.
> -                * Figure out what's going on here...
> -                */
> -               if (IS_ALDERLAKE_P(i915) && HAS_ASYNC_FLIPS(i915))
> -                       return mult * 16 * 1024;
>                 return mult * 4 * 1024;
>         case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
>         case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
>         case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> -               if (IS_ALDERLAKE_P(i915) && HAS_ASYNC_FLIPS(i915))
> -                       return mult * 16 * 1024;
> -               fallthrough;
>         case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
>         case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
>         case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> @@ -2689,7 +2688,7 @@ skl_universal_plane_create(struct
> drm_i915_private *dev_priv,
>         plane->get_hw_state = skl_plane_get_hw_state;
>         plane->check_plane = skl_plane_check;
>  
> -       if (plane_id == PLANE_1) {
> +       if (HAS_ASYNC_FLIPS(dev_priv) && plane_id == PLANE_1) {
>                 plane->need_async_flip_toggle_wa =
> IS_DISPLAY_VER(dev_priv, 9, 10);
>                 plane->async_flip = skl_plane_async_flip;
>                 plane->enable_flip_done = skl_plane_enable_flip_done;