Re: [Intel-gfx] [PATCH v2 01/13] drm/i915: Reorganize plane register writes to make them more atomic
On Wed, Nov 14, 2018 at 11:07:17PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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
> Signed-off-by: Ville Syrjälä
Reviewed-by: Matt Roper
> ---
> 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_plan
[Intel-gfx] [PATCH v2 01/13] drm/i915: Reorganize plane register writes to make them more atomic
From: Ville Syrjälä
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
Signed-off-by: Ville Syrjälä
---
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),
