drm/i915: Reorganize plane register writes to make them more atomic

Some observations about the plane registers:
- the control register will self-arm if the plane is not already
  enabled, thus we want to write it as close to (or ideally after)
  the surface register
- tileoff/linoff/offset/aux_offset are self-arming as well so we want
  them close to the surface register as well
- color keying registers we maybe self arming before SKL. Not 100%
  sure but we can try to keep them near to the surface register
  as well
- chv pipe b csc register are double buffered but self arming so
  moving them down a bit
- the rest should be mostly armed by the surface register so we can
  safely write them first, and to just for some consistency let's try
  to follow keep them in order based on the register offset

None of this will have any effect of course unless the vblank evasion
fails (which it still does sometimes). Another potential future benefit
might be pulling the non-self armings registers outside the vblank
evasion since they won't latch until the arming register has been
written. This would make the critical section a bit lighter and thus
less likely to exceed the deadline.

v2: Rebase due to input CSC
v3: Swap LINOFF/TILEOFF and KEYMSK/KEYMAX to actually follow
    the last rule above (Matt)
    Add a bit more rationale to the commit message (Matt)

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20181114210729.16185-2-ville.syrjala@linux.intel.com
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9f4e22b2a4ec9b0998b37374e738e28b48b4d12..3516b3d04787ee338de6c1236fdc0b55042daa95 100644 (file)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3315,7 +3315,6 @@ static void i9xx_update_plane(struct intel_plane *plane,
        enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
        u32 linear_offset;
        u32 dspcntr = plane_state->ctl;
-       i915_reg_t reg = DSPCNTR(i9xx_plane);
        int x = plane_state->color_plane[0].x;
        int y = plane_state->color_plane[0].y;
        unsigned long irqflags;
@@ -3330,41 +3329,45 @@ static void i9xx_update_plane(struct intel_plane *plane,
 
        spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
+       I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
+
        if (INTEL_GEN(dev_priv) < 4) {
                /* pipesrc and dspsize control the size that is scaled from,
                 * which should always be the user's requested size.
                 */
+               I915_WRITE_FW(DSPPOS(i9xx_plane), 0);
                I915_WRITE_FW(DSPSIZE(i9xx_plane),
                              ((crtc_state->pipe_src_h - 1) << 16) |
                              (crtc_state->pipe_src_w - 1));
-               I915_WRITE_FW(DSPPOS(i9xx_plane), 0);
        } else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
+               I915_WRITE_FW(PRIMPOS(i9xx_plane), 0);
                I915_WRITE_FW(PRIMSIZE(i9xx_plane),
                              ((crtc_state->pipe_src_h - 1) << 16) |
                              (crtc_state->pipe_src_w - 1));
-               I915_WRITE_FW(PRIMPOS(i9xx_plane), 0);
                I915_WRITE_FW(PRIMCNSTALPHA(i9xx_plane), 0);
        }
 
-       I915_WRITE_FW(reg, dspcntr);
-
-       I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
        if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-               I915_WRITE_FW(DSPSURF(i9xx_plane),
-                             intel_plane_ggtt_offset(plane_state) +
-                             dspaddr_offset);
                I915_WRITE_FW(DSPOFFSET(i9xx_plane), (y << 16) | x);
        } else if (INTEL_GEN(dev_priv) >= 4) {
+               I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
+               I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
+       }
+
+       /*
+        * The control register self-arms if the plane was previously
+        * disabled. Try to make the plane enable atomic by writing
+        * the control register just before the surface register.
+        */
+       I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
+       if (INTEL_GEN(dev_priv) >= 4)
                I915_WRITE_FW(DSPSURF(i9xx_plane),
                              intel_plane_ggtt_offset(plane_state) +
                              dspaddr_offset);
-               I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
-               I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
-       } else {
+       else
                I915_WRITE_FW(DSPADDR(i9xx_plane),
                              intel_plane_ggtt_offset(plane_state) +
                              dspaddr_offset);
-       }
 
        spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
@@ -10087,8 +10090,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
         * On some platforms writing CURCNTR first will also
         * cause CURPOS to be armed by the CURBASE write.
         * Without the CURCNTR write the CURPOS write would
-        * arm itself. Thus we always start the full update
-        * with a CURCNTR write.
+        * arm itself. Thus we always update CURCNTR before
+        * CURPOS.
         *
         * On other platforms CURPOS always requires the
         * CURBASE write to arm the update. Additonally
@@ -10098,15 +10101,16 @@ static void i9xx_update_cursor(struct intel_plane *plane,
         * cursor that doesn't appear to move, or even change
         * shape. Thus we always write CURBASE.
         *
-        * CURCNTR and CUR_FBC_CTL are always
-        * armed by the CURBASE write only.
+        * The other registers are armed by by the CURBASE write
+        * except when the plane is getting enabled at which time
+        * the CURCNTR write arms the update.
         */
        if (plane->cursor.base != base ||
            plane->cursor.size != fbc_ctl ||
            plane->cursor.cntl != cntl) {
-               I915_WRITE_FW(CURCNTR(pipe), cntl);
                if (HAS_CUR_FBC(dev_priv))
                        I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl);
+               I915_WRITE_FW(CURCNTR(pipe), cntl);
                I915_WRITE_FW(CURPOS(pipe), pos);
                I915_WRITE_FW(CURBASE(pipe), base);
 

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index abe193815cccfde71d19d7d71e7ba1dd94fbd6c2..6cefe5f7a3743090856ec4a1c497b05a1bf8f452 100644 (file)
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -508,28 +508,12 @@ skl_program_plane(struct intel_plane *plane,
 
        spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-       if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-               I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
-                             plane_state->color_ctl);
-
-       if (fb->format->is_yuv && icl_is_hdr_plane(plane))
-               icl_program_input_csc_coeff(crtc_state, plane_state);
-
-       I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
-       I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
-       I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
-
-       I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
        I915_WRITE_FW(PLANE_STRIDE(pipe, plane_id), stride);
+       I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
        I915_WRITE_FW(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w);
        I915_WRITE_FW(PLANE_AUX_DIST(pipe, plane_id),
                      (plane_state->color_plane[1].offset - surf_addr) | aux_stride);
 
-       if (INTEL_GEN(dev_priv) < 11)
-               I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
-                             (plane_state->color_plane[1].y << 16) |
-                              plane_state->color_plane[1].x);
-
        if (icl_is_hdr_plane(plane)) {
                u32 cus_ctl = 0;
 
@@ -551,15 +535,36 @@ skl_program_plane(struct intel_plane *plane,
                I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
        }
 
-       if (!slave && plane_state->scaler_id >= 0)
-               skl_program_scaler(plane, crtc_state, plane_state);
+       if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+               I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
+                             plane_state->color_ctl);
 
-       I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
+       if (fb->format->is_yuv && icl_is_hdr_plane(plane))
+               icl_program_input_csc_coeff(crtc_state, plane_state);
+
+       I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
+       I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
+       I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
+
+       I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
+
+       if (INTEL_GEN(dev_priv) < 11)
+               I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
+                             (plane_state->color_plane[1].y << 16) |
+                             plane_state->color_plane[1].x);
 
+       /*
+        * The control register self-arms if the plane was previously
+        * disabled. Try to make the plane enable atomic by writing
+        * the control register just before the surface register.
+        */
        I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
        I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
                      intel_plane_ggtt_offset(plane_state) + surf_addr);
 
+       if (!slave && plane_state->scaler_id >= 0)
+               skl_program_scaler(plane, crtc_state, plane_state);
+
        spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
@@ -821,24 +826,29 @@ vlv_update_plane(struct intel_plane *plane,
 
        vlv_update_clrc(plane_state);
 
+       I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
+                     plane_state->color_plane[0].stride);
+       I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
+       I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
+       I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
+
        if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
                chv_update_csc(plane_state);
 
        if (key->flags) {
                I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
-               I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
                I915_WRITE_FW(SPKEYMSK(pipe, plane_id), key->channel_mask);
+               I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
        }
-       I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
-                     plane_state->color_plane[0].stride);
-       I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 
-       I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
        I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
+       I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
 
-       I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
-
-       I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
+       /*
+        * The control register self-arms if the plane was previously
+        * disabled. Try to make the plane enable atomic by writing
+        * the control register just before the surface register.
+        */
        I915_WRITE_FW(SPCNTR(pipe, plane_id), sprctl);
        I915_WRITE_FW(SPSURF(pipe, plane_id),
                      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
@@ -980,27 +990,32 @@ ivb_update_plane(struct intel_plane *plane,
 
        spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
+       I915_WRITE_FW(SPRSTRIDE(pipe), plane_state->color_plane[0].stride);
+       I915_WRITE_FW(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
+       I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
+       if (IS_IVYBRIDGE(dev_priv))
+               I915_WRITE_FW(SPRSCALE(pipe), sprscale);
+
        if (key->flags) {
                I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
-               I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
                I915_WRITE_FW(SPRKEYMSK(pipe), key->channel_mask);
+               I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
        }
 
-       I915_WRITE_FW(SPRSTRIDE(pipe), plane_state->color_plane[0].stride);
-       I915_WRITE_FW(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
-
        /* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
         * register */
        if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
                I915_WRITE_FW(SPROFFSET(pipe), (y << 16) | x);
        } else {
-               I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
                I915_WRITE_FW(SPRLINOFF(pipe), linear_offset);
+               I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
        }
 
-       I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
-       if (IS_IVYBRIDGE(dev_priv))
-               I915_WRITE_FW(SPRSCALE(pipe), sprscale);
+       /*
+        * The control register self-arms if the plane was previously
+        * disabled. Try to make the plane enable atomic by writing
+        * the control register just before the surface register.
+        */
        I915_WRITE_FW(SPRCTL(pipe), sprctl);
        I915_WRITE_FW(SPRSURF(pipe),
                      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
@@ -1018,7 +1033,7 @@ ivb_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
        spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
        I915_WRITE_FW(SPRCTL(pipe), 0);
-       /* Can't leave the scaler enabled... */
+       /* Disable the scaler */
        if (IS_IVYBRIDGE(dev_priv))
                I915_WRITE_FW(SPRSCALE(pipe), 0);
        I915_WRITE_FW(SPRSURF(pipe), 0);
@@ -1148,20 +1163,25 @@ g4x_update_plane(struct intel_plane *plane,
 
        spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
+       I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
+       I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
+       I915_WRITE_FW(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
+       I915_WRITE_FW(DVSSCALE(pipe), dvsscale);
+
        if (key->flags) {
                I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
-               I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
                I915_WRITE_FW(DVSKEYMSK(pipe), key->channel_mask);
+               I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
        }
 
-       I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
-       I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
-
-       I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
        I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
+       I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
 
-       I915_WRITE_FW(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
-       I915_WRITE_FW(DVSSCALE(pipe), dvsscale);
+       /*
+        * The control register self-arms if the plane was previously
+        * disabled. Try to make the plane enable atomic by writing
+        * the control register just before the surface register.
+        */
        I915_WRITE_FW(DVSCNTR(pipe), dvscntr);
        I915_WRITE_FW(DVSSURF(pipe),
                      intel_plane_ggtt_offset(plane_state) + dvssurf_offset);