On Wed, Oct 27, 2021 at 08:11:37PM +0300, Lisovskiy, Stanislav wrote:
> On Mon, Oct 18, 2021 at 02:50:26PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > 
> > Chop skl_program_plane() into two halves. Fist half becomes
> > the _noarm() variant, second part the _arm() variant.
> > 
> > Fortunately I have already previously grouped the register
> > writes into roughtly the correct order, so the split looks
> > surprisingly clean.
> > 
> > A few notable oddities I did not realize were self arming
> > are AUX_DIST and COLOR_CTL.
> > 
> > i915_update_info doesn't look too terrible on my cfl running
> > kms_atomic_transition --r plane-all-transition --extended:
> > w/o patch                           w/ patch
> > Updates: 2178                       Updates: 2018
> >        |                                   |
> >    1us |                               1us |
> >        |                                   |
> >    4us |                               4us |*****
> >        |*********                          |**********
> >   16us |**********                    16us |*******
> >        |***                                |
> >   66us |                              66us |
> >        |                                   |
> >  262us |                             262us |
> >        |                                   |
> >    1ms |                               1ms |
> >        |                                   |
> >    4ms |                               4ms |
> >        |                                   |
> >   17ms |                              17ms |
> >        |                                   |
> > Min update: 8332ns                  Min update: 6164ns
> > Max update: 48758ns                 Max update: 31808ns
> > Average update: 19959ns             Average update: 13159ns
> > Overruns > 100us: 0                 Overruns > 100us: 0
> > 
> > And with lockdep enabled:
> > w/o patch                           w/ patch
> > Updates: 2177                           Updates: 2172
> >        |                               |
> >    1us |                           1us |
> >        |                               |
> >    4us |                           4us |
> >        |*******                                |*********
> >   16us |**********                16us |**********
> >        |*******                                |*
> >   66us |                          66us |
> >        |                               |
> >  262us |                         262us |
> >        |                               |
> >    1ms |                           1ms |
> >        |                               |
> >    4ms |                           4ms |
> >        |                               |
> >   17ms |                          17ms |
> >        |                               |
> > Min update: 12645ns             Min update: 9980ns
> > Max update: 50153ns             Max update: 33533ns
> > Average update: 25337ns                 Average update: 18245ns
> > Overruns > 250us: 0             Overruns > 250us: 0
> > 
> > TODO: On icl+ everything seems to be armed by PLANE_SURF, so we
> >       can optimize this even further on modern platforms. But I
> >       think there's a bit of refactoring to be done first to
> >       figure out the best way to go about it (eg. just reusing
> >       the current skl+ functions, or doing a lower level split).
> > 
> > TODO: Split scaler programming as well, but IIRC the scaler
> >       has some oddball double buffering behaviour on some
> >       platforms, so needs proper reverse engineering
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovs...@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > ---
> >  .../drm/i915/display/skl_universal_plane.c    | 113 +++++++++++-------
> >  1 file changed, 72 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 74f3870d39b1..2a822e1e465e 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1050,60 +1050,32 @@ static void icl_plane_csc_load_black(struct 
> > intel_plane *plane)
> >  }
> >  
> >  static void
> > -skl_program_plane(struct intel_plane *plane,
> > -             const struct intel_crtc_state *crtc_state,
> > -             const struct intel_plane_state *plane_state,
> > -             int color_plane)
> > +skl_program_plane_noarm(struct intel_plane *plane,
> > +                   const struct intel_crtc_state *crtc_state,
> > +                   const struct intel_plane_state *plane_state,
> > +                   int color_plane)
> >  {
> >     struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >     enum plane_id plane_id = plane->id;
> >     enum pipe pipe = plane->pipe;
> > -   const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >     u32 stride = skl_plane_stride(plane_state, color_plane);
> >     const struct drm_framebuffer *fb = plane_state->hw.fb;
> > -   int aux_plane = skl_main_to_aux_plane(fb, color_plane);
> >     int crtc_x = plane_state->uapi.dst.x1;
> >     int crtc_y = plane_state->uapi.dst.y1;
> > -   u32 x = plane_state->view.color_plane[color_plane].x;
> > -   u32 y = plane_state->view.color_plane[color_plane].y;
> >     u32 src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> >     u32 src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> > -   u8 alpha = plane_state->hw.alpha >> 8;
> > -   u32 plane_color_ctl = 0, aux_dist = 0;
> >     unsigned long irqflags;
> > -   u32 keymsk, keymax;
> > -   u32 plane_ctl = plane_state->ctl;
> > -
> > -   plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > -
> > -   if (DISPLAY_VER(dev_priv) >= 10)
> > -           plane_color_ctl = plane_state->color_ctl |
> > -                   glk_plane_color_ctl_crtc(crtc_state);
> >  
> >     /* Sizes are 0 based */
> >     src_w--;
> >     src_h--;
> >  
> > -   keymax = (key->max_value & 0xffffff) | PLANE_KEYMAX_ALPHA(alpha);
> > -
> > -   keymsk = key->channel_mask & 0x7ffffff;
> > -   if (alpha < 0xff)
> > -           keymsk |= PLANE_KEYMSK_ALPHA_ENABLE;
> > -
> >     /* The scaler will handle the output position */
> >     if (plane_state->scaler_id >= 0) {
> >             crtc_x = 0;
> >             crtc_y = 0;
> >     }
> >  
> > -   if (aux_plane) {
> > -           aux_dist = skl_surf_address(plane_state, aux_plane) -
> > -                   skl_surf_address(plane_state, color_plane);
> > -
> > -           if (DISPLAY_VER(dev_priv) < 12)
> > -                   aux_dist |= skl_plane_stride(plane_state, aux_plane);
> > -   }
> > -
> >     spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >  
> >     /*
> > @@ -1119,16 +1091,10 @@ skl_program_plane(struct intel_plane *plane,
> >     intel_de_write_fw(dev_priv, PLANE_SIZE(pipe, plane_id),
> >                       (src_h << 16) | src_w);
> >  
> > -   intel_de_write_fw(dev_priv, PLANE_AUX_DIST(pipe, plane_id), aux_dist);
> > -
> >     if (icl_is_hdr_plane(dev_priv, plane_id))
> >             intel_de_write_fw(dev_priv, PLANE_CUS_CTL(pipe, plane_id),
> >                               plane_state->cus_ctl);
> >  
> > -   if (DISPLAY_VER(dev_priv) >= 10)
> > -           intel_de_write_fw(dev_priv, PLANE_COLOR_CTL(pipe, plane_id),
> > -                             plane_color_ctl);
> > -
> >     if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
> >             icl_program_input_csc(plane, crtc_state, plane_state);
> >  
> > @@ -1138,6 +1104,52 @@ skl_program_plane(struct intel_plane *plane,
> >  
> >     skl_write_plane_wm(plane, crtc_state);
> >  
> > +   intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, 
> > color_plane);
> > +
> > +   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > +}
> > +
> > +static void
> > +skl_program_plane_arm(struct intel_plane *plane,
> > +                 const struct intel_crtc_state *crtc_state,
> > +                 const struct intel_plane_state *plane_state,
> > +                 int color_plane)
> > +{
> > +   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +   enum plane_id plane_id = plane->id;
> > +   enum pipe pipe = plane->pipe;
> > +   const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > +   const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +   int aux_plane = skl_main_to_aux_plane(fb, color_plane);
> > +   u32 x = plane_state->view.color_plane[color_plane].x;
> > +   u32 y = plane_state->view.color_plane[color_plane].y;
> > +   u32 keymsk, keymax, aux_dist = 0, plane_color_ctl = 0;
> > +   u8 alpha = plane_state->hw.alpha >> 8;
> > +   u32 plane_ctl = plane_state->ctl;
> > +   unsigned long irqflags;
> > +
> > +   plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > +
> > +   if (DISPLAY_VER(dev_priv) >= 10)
> > +           plane_color_ctl = plane_state->color_ctl |
> > +                   glk_plane_color_ctl_crtc(crtc_state);
> > +
> > +   keymax = (key->max_value & 0xffffff) | PLANE_KEYMAX_ALPHA(alpha);
> > +
> > +   keymsk = key->channel_mask & 0x7ffffff;
> > +   if (alpha < 0xff)
> > +           keymsk |= PLANE_KEYMSK_ALPHA_ENABLE;
> > +
> > +   if (aux_plane) {
> > +           aux_dist = skl_surf_address(plane_state, aux_plane) -
> > +                   skl_surf_address(plane_state, color_plane);
> > +
> > +           if (DISPLAY_VER(dev_priv) < 12)
> > +                   aux_dist |= skl_plane_stride(plane_state, aux_plane);
> > +   }
> > +
> > +   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > +
> >     intel_de_write_fw(dev_priv, PLANE_KEYVAL(pipe, plane_id),
> >                       key->min_value);
> >     intel_de_write_fw(dev_priv, PLANE_KEYMSK(pipe, plane_id), keymsk);
> > @@ -1146,17 +1158,22 @@ skl_program_plane(struct intel_plane *plane,
> >     intel_de_write_fw(dev_priv, PLANE_OFFSET(pipe, plane_id),
> >                       (y << 16) | x);
> 
> Wondering, if we could also move PLANE_OFFSET register to non-arming
> section.

No. It is self arming.

> BSpec states, that it is not arming and being armed by
> writing PLANE_SURF. I think we even agreed to check  if that was true,
> so I made some tests with DG2, I think and writes to PLANE_OFFSET
> didn't anyhow affect the hw until PLANE_SURF was written(was checking
> it by reading the crcs...)

Your test was invalid since it was done on icl+ where everything is
armed by PLANE_SURF.

> 
> >  
> > +   intel_de_write_fw(dev_priv, PLANE_AUX_DIST(pipe, plane_id), aux_dist);
> > +
> >     if (DISPLAY_VER(dev_priv) < 11)
> >             intel_de_write_fw(dev_priv, PLANE_AUX_OFFSET(pipe, plane_id),
> >                               (plane_state->view.color_plane[1].y << 16) |
> >                                plane_state->view.color_plane[1].x);
> >  
> > -   intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, 
> > color_plane);
> > +   if (DISPLAY_VER(dev_priv) >= 10)
> > +           intel_de_write_fw(dev_priv, PLANE_COLOR_CTL(pipe, plane_id), 
> > plane_color_ctl);
> >  
> >     /*
> >      * Enable the scaler before the plane so that we don't
> >      * get a catastrophic underrun even if the two operations
> >      * end up happening in two different frames.
> > +    *
> > +    * TODO: split into noarm+arm pair
> >      */
> >     if (plane_state->scaler_id >= 0)
> >             skl_program_plane_scaler(plane, crtc_state, plane_state);
> > @@ -1199,7 +1216,20 @@ skl_plane_async_flip(struct intel_plane *plane,
> >     spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >  
> > -/* TODO: split into noarm+arm pair */
> > +static void
> > +skl_plane_update_noarm(struct intel_plane *plane,
> > +                  const struct intel_crtc_state *crtc_state,
> > +                  const struct intel_plane_state *plane_state)
> > +{
> > +   int color_plane = 0;
> > +
> > +   if (plane_state->planar_linked_plane && !plane_state->planar_slave)
> > +           /* Program the UV plane on planar master */
> > +           color_plane = 1;
> > +
> > +   skl_program_plane_noarm(plane, crtc_state, plane_state, color_plane);
> > +}
> > +
> >  static void
> >  skl_plane_update_arm(struct intel_plane *plane,
> >                  const struct intel_crtc_state *crtc_state,
> > @@ -1211,7 +1241,7 @@ skl_plane_update_arm(struct intel_plane *plane,
> >             /* Program the UV plane on planar master */
> >             color_plane = 1;
> >  
> > -   skl_program_plane(plane, crtc_state, plane_state, color_plane);
> > +   skl_program_plane_arm(plane, crtc_state, plane_state, color_plane);
> >  }
> >  
> >  static bool intel_format_is_p01x(u32 format)
> > @@ -2159,6 +2189,7 @@ skl_universal_plane_create(struct drm_i915_private 
> > *dev_priv,
> >     }
> >  
> >     plane->max_stride = skl_plane_max_stride;
> > +   plane->update_noarm = skl_plane_update_noarm;
> >     plane->update_arm = skl_plane_update_arm;
> >     plane->disable_arm = skl_plane_disable_arm;
> >     plane->get_hw_state = skl_plane_get_hw_state;
> > -- 
> > 2.32.0
> > 

-- 
Ville Syrjälä
Intel

Reply via email to