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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 24, 2024 at 10:46:42AM +0000, Hogander, Jouni wrote:
> 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:

I mentioned this in the commit msg:
"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."

> 
> 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;
> 

-- 
Ville Syrjälä
Intel