On Wed, Nov 14, 2018 at 11:07:17PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 52 ++++++------
> drivers/gpu/drm/i915/intel_sprite.c | 118 ++++++++++++++++-----------
> 2 files changed, 97 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 132e978227fb..3c760a2eacc8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3314,7 +3314,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;
> @@ -3329,41 +3328,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(DSPSIZE(i9xx_plane),
> - ((crtc_state->pipe_src_h - 1) << 16) |
> - (crtc_state->pipe_src_w - 1));
> 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));
> } else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> - 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(PRIMSIZE(i9xx_plane),
> + ((crtc_state->pipe_src_h - 1) << 16) |
> + (crtc_state->pipe_src_w - 1));
> 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(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 {
> + 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);
> + else
> I915_WRITE_FW(DSPADDR(i9xx_plane),
> intel_plane_ggtt_offset(plane_state) +
> dspaddr_offset);
> - }
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
> @@ -10056,8 +10059,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
> @@ -10067,15 +10070,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 5e0f7b575a50..a80773211265 100644
> --- 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);
>
> - 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(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(SPRKEYMSK(pipe), key->channel_mask);
> + I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
> + }
>
> /* 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);
>
> - 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(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(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(DVSKEYMSK(pipe), key->channel_mask);
> + I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
> + }
> +
> + I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
> + I915_WRITE_FW(DVSTILEOFF(pipe), (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(DVSCNTR(pipe), dvscntr);
> I915_WRITE_FW(DVSSURF(pipe),
> intel_plane_ggtt_offset(plane_state) + dvssurf_offset);
> --
> 2.18.1
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
