| Message ID | 1435023033-18043-3-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote: > We never removed the sprite watermark updates from our low-level > foo_update_plane() functions; since our hardware updates happen under > vblank evasion, we're not supposed to be calling potentially sleeping > functions there (since interrupts are disabled). We do already have a > mechanism that's supposed to be used to update sprite watermarks in the > post-evasion function intel_post_plane_update(), but at the moment it's > only being used for updates caused by plane disables. > > To fix this oversight we need to make a few changes: > > * When intel_plane_atomic_calc_changes() calls intel_wm_need_update() > to determine whether watermarks need an update, we need to set the > 'atomic.update_sprite_watermarks' flag rather than the > 'atomic.update_wm' flag when the plane in question is a sprite. Some > platforms don't care about this change since the two types of update > do the same thing, but some platforms (e.g., ILK-style watermarks) > need to specifically use the sprite update function to update cached > watermark parameters. > > * intel_wm_need_update() needs to also look for plane size changes > (previously it only checked tiling & rotation). > > With the above changes, the need for sprite watermark updates should be > properly flagged at atomic 'check' time and handled at 'commit' time in > post-evasion, so we can drop the direct calls to > intel_update_sprite_watermarks() from all of the > intel_plane->update_plane() handlers. We'll also toss a > WARN_ON(irqs_disabled()) into the watermark update functions to avoid > such mistakes in the future. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Why do even still need a separate sprite wm update hooks? Aren't we yet recomputing wm values for all planes if something changes, then diffing them with the current one and updating if anything changed? That seems to be what update_sprite_wm essentially does, except it's pre-atomic and open-codes the updates ... Ofc there's still that ilk w/a around which needs to be moved to a suitable place. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++------- > drivers/gpu/drm/i915/intel_pm.c | 2 ++ > drivers/gpu/drm/i915/intel_sprite.c | 12 ------------ > 3 files changed, 18 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5e8e01c..181aedd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11575,13 +11575,17 @@ retry: > static bool intel_wm_need_update(struct drm_plane *plane, > struct drm_plane_state *state) > { > - /* Update watermarks on tiling changes. */ > + struct intel_plane_state *new = to_intel_plane_state(state); > + struct intel_plane_state *cur = to_intel_plane_state(plane->state); > + > + /* Update watermarks on tiling changes or size changes. */ > if (!plane->state->fb || !state->fb || > plane->state->fb->modifier[0] != state->fb->modifier[0] || > - plane->state->rotation != state->rotation) > - return true; > - > - if (plane->state->crtc_w != state->crtc_w) > + plane->state->rotation != state->rotation || > + drm_rect_width(&new->src) != drm_rect_width(&cur->src) || > + drm_rect_height(&new->src) != drm_rect_height(&cur->src) || > + drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) || > + drm_rect_height(&new->dst) != drm_rect_height(&cur->dst)) > return true; > > return false; > @@ -11645,8 +11649,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > plane->base.id, was_visible, visible, > turn_off, turn_on, mode_changed); > > - if (intel_wm_need_update(plane, plane_state)) > - intel_crtc->atomic.update_wm = true; > + if (intel_wm_need_update(plane, plane_state)) { > + if (plane->type == DRM_PLANE_TYPE_OVERLAY) > + intel_crtc->atomic.update_sprite_watermarks |= > + 1 << i; > + else > + intel_crtc->atomic.update_wm = true; > + } > > switch (plane->type) { > case DRM_PLANE_TYPE_PRIMARY: > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 4d3cb70..2470ede 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > > + WARN_ON(irqs_disabled()); > if (dev_priv->display.update_wm) > dev_priv->display.update_wm(crtc); > } > @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane, > unsigned int dst_h = drm_rect_height(&state->dst); > bool scaled = (src_w != dst_w || src_h != dst_h); > > + WARN_ON(irqs_disabled()); > if (dev_priv->display.update_sprite_wm) > dev_priv->display.update_sprite_wm(plane, crtc, > sprite_width, sprite_height, > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index b627067..ce2fa92 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > rotation = drm_plane->state->rotation; > plane_ctl |= skl_plane_ctl_rotation(rotation); > > - intel_update_sprite_watermarks(drm_plane, crtc); > - > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > fb->pixel_format); > > @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > I915_WRITE(PLANE_SURF(pipe, plane), 0); > POSTING_READ(PLANE_SURF(pipe, plane)); > - > - intel_update_sprite_watermarks(dplane, crtc); > } > > static void > @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > if (obj->tiling_mode != I915_TILING_NONE) > sprctl |= SP_TILED; > > - intel_update_sprite_watermarks(dplane, crtc); > - > /* Sizes are 0 based */ > src_w--; > src_h--; > @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > I915_WRITE(SPSURF(pipe, plane), 0); > POSTING_READ(SPSURF(pipe, plane)); > - > - intel_update_sprite_watermarks(dplane, crtc); > } > > static void > @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > sprctl |= SPRITE_PIPE_CSC_ENABLE; > > - intel_update_sprite_watermarks(plane, crtc); > - > /* Sizes are 0 based */ > src_w--; > src_h--; > @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > if (IS_GEN6(dev)) > dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */ > > - intel_update_sprite_watermarks(plane, crtc); > - > /* Sizes are 0 based */ > src_w--; > src_h--; > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 23, 2015 at 09:34:50AM +0200, Daniel Vetter wrote: > On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote: > > We never removed the sprite watermark updates from our low-level > > foo_update_plane() functions; since our hardware updates happen under > > vblank evasion, we're not supposed to be calling potentially sleeping > > functions there (since interrupts are disabled). We do already have a > > mechanism that's supposed to be used to update sprite watermarks in the > > post-evasion function intel_post_plane_update(), but at the moment it's > > only being used for updates caused by plane disables. > > > > To fix this oversight we need to make a few changes: > > > > * When intel_plane_atomic_calc_changes() calls intel_wm_need_update() > > to determine whether watermarks need an update, we need to set the > > 'atomic.update_sprite_watermarks' flag rather than the > > 'atomic.update_wm' flag when the plane in question is a sprite. Some > > platforms don't care about this change since the two types of update > > do the same thing, but some platforms (e.g., ILK-style watermarks) > > need to specifically use the sprite update function to update cached > > watermark parameters. > > > > * intel_wm_need_update() needs to also look for plane size changes > > (previously it only checked tiling & rotation). > > > > With the above changes, the need for sprite watermark updates should be > > properly flagged at atomic 'check' time and handled at 'commit' time in > > post-evasion, so we can drop the direct calls to > > intel_update_sprite_watermarks() from all of the > > intel_plane->update_plane() handlers. We'll also toss a > > WARN_ON(irqs_disabled()) into the watermark update functions to avoid > > such mistakes in the future. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Why do even still need a separate sprite wm update hooks? Aren't we yet > recomputing wm values for all planes if something changes, then diffing > them with the current one and updating if anything changed? That seems to > be what update_sprite_wm essentially does, except it's pre-atomic and > open-codes the updates ... Some platforms stage a copy of plane/pipe state in 'wm parameter' structures and only update the pipe wm params with the new plane values when update_sprite_wm is called. That's definitely something my main watermark series kills off, but until we get to that point and remove the extra state staging in 'params' I think we still need to make sure the update_sprite_wm is called appropriately to make sure watermarks use the right values on all platforms. I figured these two patches were a quick fix for a known bug while we get the main watermark series hammered out. Matt > > Ofc there's still that ilk w/a around which needs to be moved to a > suitable place. > -Daniel > > --- > > drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++------- > > drivers/gpu/drm/i915/intel_pm.c | 2 ++ > > drivers/gpu/drm/i915/intel_sprite.c | 12 ------------ > > 3 files changed, 18 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 5e8e01c..181aedd 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11575,13 +11575,17 @@ retry: > > static bool intel_wm_need_update(struct drm_plane *plane, > > struct drm_plane_state *state) > > { > > - /* Update watermarks on tiling changes. */ > > + struct intel_plane_state *new = to_intel_plane_state(state); > > + struct intel_plane_state *cur = to_intel_plane_state(plane->state); > > + > > + /* Update watermarks on tiling changes or size changes. */ > > if (!plane->state->fb || !state->fb || > > plane->state->fb->modifier[0] != state->fb->modifier[0] || > > - plane->state->rotation != state->rotation) > > - return true; > > - > > - if (plane->state->crtc_w != state->crtc_w) > > + plane->state->rotation != state->rotation || > > + drm_rect_width(&new->src) != drm_rect_width(&cur->src) || > > + drm_rect_height(&new->src) != drm_rect_height(&cur->src) || > > + drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) || > > + drm_rect_height(&new->dst) != drm_rect_height(&cur->dst)) > > return true; > > > > return false; > > @@ -11645,8 +11649,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > > plane->base.id, was_visible, visible, > > turn_off, turn_on, mode_changed); > > > > - if (intel_wm_need_update(plane, plane_state)) > > - intel_crtc->atomic.update_wm = true; > > + if (intel_wm_need_update(plane, plane_state)) { > > + if (plane->type == DRM_PLANE_TYPE_OVERLAY) > > + intel_crtc->atomic.update_sprite_watermarks |= > > + 1 << i; > > + else > > + intel_crtc->atomic.update_wm = true; > > + } > > > > switch (plane->type) { > > case DRM_PLANE_TYPE_PRIMARY: > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 4d3cb70..2470ede 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc) > > { > > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > > > > + WARN_ON(irqs_disabled()); > > if (dev_priv->display.update_wm) > > dev_priv->display.update_wm(crtc); > > } > > @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane, > > unsigned int dst_h = drm_rect_height(&state->dst); > > bool scaled = (src_w != dst_w || src_h != dst_h); > > > > + WARN_ON(irqs_disabled()); > > if (dev_priv->display.update_sprite_wm) > > dev_priv->display.update_sprite_wm(plane, crtc, > > sprite_width, sprite_height, > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index b627067..ce2fa92 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > > rotation = drm_plane->state->rotation; > > plane_ctl |= skl_plane_ctl_rotation(rotation); > > > > - intel_update_sprite_watermarks(drm_plane, crtc); > > - > > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > > fb->pixel_format); > > > > @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > I915_WRITE(PLANE_SURF(pipe, plane), 0); > > POSTING_READ(PLANE_SURF(pipe, plane)); > > - > > - intel_update_sprite_watermarks(dplane, crtc); > > } > > > > static void > > @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > > if (obj->tiling_mode != I915_TILING_NONE) > > sprctl |= SP_TILED; > > > > - intel_update_sprite_watermarks(dplane, crtc); > > - > > /* Sizes are 0 based */ > > src_w--; > > src_h--; > > @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > I915_WRITE(SPSURF(pipe, plane), 0); > > POSTING_READ(SPSURF(pipe, plane)); > > - > > - intel_update_sprite_watermarks(dplane, crtc); > > } > > > > static void > > @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > sprctl |= SPRITE_PIPE_CSC_ENABLE; > > > > - intel_update_sprite_watermarks(plane, crtc); > > - > > /* Sizes are 0 based */ > > src_w--; > > src_h--; > > @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > if (IS_GEN6(dev)) > > dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */ > > > > - intel_update_sprite_watermarks(plane, crtc); > > - > > /* Sizes are 0 based */ > > src_w--; > > src_h--; > > -- > > 2.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Jun 23, 2015 at 07:29:58AM -0700, Matt Roper wrote: > On Tue, Jun 23, 2015 at 09:34:50AM +0200, Daniel Vetter wrote: > > On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote: > > > We never removed the sprite watermark updates from our low-level > > > foo_update_plane() functions; since our hardware updates happen under > > > vblank evasion, we're not supposed to be calling potentially sleeping > > > functions there (since interrupts are disabled). We do already have a > > > mechanism that's supposed to be used to update sprite watermarks in the > > > post-evasion function intel_post_plane_update(), but at the moment it's > > > only being used for updates caused by plane disables. > > > > > > To fix this oversight we need to make a few changes: > > > > > > * When intel_plane_atomic_calc_changes() calls intel_wm_need_update() > > > to determine whether watermarks need an update, we need to set the > > > 'atomic.update_sprite_watermarks' flag rather than the > > > 'atomic.update_wm' flag when the plane in question is a sprite. Some > > > platforms don't care about this change since the two types of update > > > do the same thing, but some platforms (e.g., ILK-style watermarks) > > > need to specifically use the sprite update function to update cached > > > watermark parameters. > > > > > > * intel_wm_need_update() needs to also look for plane size changes > > > (previously it only checked tiling & rotation). > > > > > > With the above changes, the need for sprite watermark updates should be > > > properly flagged at atomic 'check' time and handled at 'commit' time in > > > post-evasion, so we can drop the direct calls to > > > intel_update_sprite_watermarks() from all of the > > > intel_plane->update_plane() handlers. We'll also toss a > > > WARN_ON(irqs_disabled()) into the watermark update functions to avoid > > > such mistakes in the future. > > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > Why do even still need a separate sprite wm update hooks? Aren't we yet > > recomputing wm values for all planes if something changes, then diffing > > them with the current one and updating if anything changed? That seems to > > be what update_sprite_wm essentially does, except it's pre-atomic and > > open-codes the updates ... > > Some platforms stage a copy of plane/pipe state in 'wm parameter' > structures and only update the pipe wm params with the new plane values > when update_sprite_wm is called. That's definitely something my main > watermark series kills off, but until we get to that point and remove > the extra state staging in 'params' I think we still need to make sure > the update_sprite_wm is called appropriately to make sure watermarks use > the right values on all platforms. I figured these two patches were a > quick fix for a known bug while we get the main watermark series > hammered out. Oh I thought that update_wm hooks already walk over all planes for all platforms and so if we call that for all (atomic) modeset we should be covered. I did see that update_sprite_wm do update some of those cached values before calling intel_update_wm though and so wondered whether we still need this all. Sounds like we still do :( -Daniel > > > Matt > > > > > Ofc there's still that ilk w/a around which needs to be moved to a > > suitable place. > > -Daniel > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++------- > > > drivers/gpu/drm/i915/intel_pm.c | 2 ++ > > > drivers/gpu/drm/i915/intel_sprite.c | 12 ------------ > > > 3 files changed, 18 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 5e8e01c..181aedd 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -11575,13 +11575,17 @@ retry: > > > static bool intel_wm_need_update(struct drm_plane *plane, > > > struct drm_plane_state *state) > > > { > > > - /* Update watermarks on tiling changes. */ > > > + struct intel_plane_state *new = to_intel_plane_state(state); > > > + struct intel_plane_state *cur = to_intel_plane_state(plane->state); > > > + > > > + /* Update watermarks on tiling changes or size changes. */ > > > if (!plane->state->fb || !state->fb || > > > plane->state->fb->modifier[0] != state->fb->modifier[0] || > > > - plane->state->rotation != state->rotation) > > > - return true; > > > - > > > - if (plane->state->crtc_w != state->crtc_w) > > > + plane->state->rotation != state->rotation || > > > + drm_rect_width(&new->src) != drm_rect_width(&cur->src) || > > > + drm_rect_height(&new->src) != drm_rect_height(&cur->src) || > > > + drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) || > > > + drm_rect_height(&new->dst) != drm_rect_height(&cur->dst)) > > > return true; > > > > > > return false; > > > @@ -11645,8 +11649,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > > > plane->base.id, was_visible, visible, > > > turn_off, turn_on, mode_changed); > > > > > > - if (intel_wm_need_update(plane, plane_state)) > > > - intel_crtc->atomic.update_wm = true; > > > + if (intel_wm_need_update(plane, plane_state)) { > > > + if (plane->type == DRM_PLANE_TYPE_OVERLAY) > > > + intel_crtc->atomic.update_sprite_watermarks |= > > > + 1 << i; > > > + else > > > + intel_crtc->atomic.update_wm = true; > > > + } > > > > > > switch (plane->type) { > > > case DRM_PLANE_TYPE_PRIMARY: > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 4d3cb70..2470ede 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc) > > > { > > > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > > > > > > + WARN_ON(irqs_disabled()); > > > if (dev_priv->display.update_wm) > > > dev_priv->display.update_wm(crtc); > > > } > > > @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane, > > > unsigned int dst_h = drm_rect_height(&state->dst); > > > bool scaled = (src_w != dst_w || src_h != dst_h); > > > > > > + WARN_ON(irqs_disabled()); > > > if (dev_priv->display.update_sprite_wm) > > > dev_priv->display.update_sprite_wm(plane, crtc, > > > sprite_width, sprite_height, > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > index b627067..ce2fa92 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > > > rotation = drm_plane->state->rotation; > > > plane_ctl |= skl_plane_ctl_rotation(rotation); > > > > > > - intel_update_sprite_watermarks(drm_plane, crtc); > > > - > > > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > > > fb->pixel_format); > > > > > > @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > > > I915_WRITE(PLANE_SURF(pipe, plane), 0); > > > POSTING_READ(PLANE_SURF(pipe, plane)); > > > - > > > - intel_update_sprite_watermarks(dplane, crtc); > > > } > > > > > > static void > > > @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > > > if (obj->tiling_mode != I915_TILING_NONE) > > > sprctl |= SP_TILED; > > > > > > - intel_update_sprite_watermarks(dplane, crtc); > > > - > > > /* Sizes are 0 based */ > > > src_w--; > > > src_h--; > > > @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > > > I915_WRITE(SPSURF(pipe, plane), 0); > > > POSTING_READ(SPSURF(pipe, plane)); > > > - > > > - intel_update_sprite_watermarks(dplane, crtc); > > > } > > > > > > static void > > > @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > sprctl |= SPRITE_PIPE_CSC_ENABLE; > > > > > > - intel_update_sprite_watermarks(plane, crtc); > > > - > > > /* Sizes are 0 based */ > > > src_w--; > > > src_h--; > > > @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > if (IS_GEN6(dev)) > > > dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */ > > > > > > - intel_update_sprite_watermarks(plane, crtc); > > > - > > > /* Sizes are 0 based */ > > > src_w--; > > > src_h--; > > > -- > > > 2.1.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5e8e01c..181aedd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11575,13 +11575,17 @@ retry: static bool intel_wm_need_update(struct drm_plane *plane, struct drm_plane_state *state) { - /* Update watermarks on tiling changes. */ + struct intel_plane_state *new = to_intel_plane_state(state); + struct intel_plane_state *cur = to_intel_plane_state(plane->state); + + /* Update watermarks on tiling changes or size changes. */ if (!plane->state->fb || !state->fb || plane->state->fb->modifier[0] != state->fb->modifier[0] || - plane->state->rotation != state->rotation) - return true; - - if (plane->state->crtc_w != state->crtc_w) + plane->state->rotation != state->rotation || + drm_rect_width(&new->src) != drm_rect_width(&cur->src) || + drm_rect_height(&new->src) != drm_rect_height(&cur->src) || + drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) || + drm_rect_height(&new->dst) != drm_rect_height(&cur->dst)) return true; return false; @@ -11645,8 +11649,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, plane->base.id, was_visible, visible, turn_off, turn_on, mode_changed); - if (intel_wm_need_update(plane, plane_state)) - intel_crtc->atomic.update_wm = true; + if (intel_wm_need_update(plane, plane_state)) { + if (plane->type == DRM_PLANE_TYPE_OVERLAY) + intel_crtc->atomic.update_sprite_watermarks |= + 1 << i; + else + intel_crtc->atomic.update_wm = true; + } switch (plane->type) { case DRM_PLANE_TYPE_PRIMARY: diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4d3cb70..2470ede 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = crtc->dev->dev_private; + WARN_ON(irqs_disabled()); if (dev_priv->display.update_wm) dev_priv->display.update_wm(crtc); } @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane, unsigned int dst_h = drm_rect_height(&state->dst); bool scaled = (src_w != dst_w || src_h != dst_h); + WARN_ON(irqs_disabled()); if (dev_priv->display.update_sprite_wm) dev_priv->display.update_sprite_wm(plane, crtc, sprite_width, sprite_height, diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index b627067..ce2fa92 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, rotation = drm_plane->state->rotation; plane_ctl |= skl_plane_ctl_rotation(rotation); - intel_update_sprite_watermarks(drm_plane, crtc); - stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], fb->pixel_format); @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) I915_WRITE(PLANE_SURF(pipe, plane), 0); POSTING_READ(PLANE_SURF(pipe, plane)); - - intel_update_sprite_watermarks(dplane, crtc); } static void @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, if (obj->tiling_mode != I915_TILING_NONE) sprctl |= SP_TILED; - intel_update_sprite_watermarks(dplane, crtc); - /* Sizes are 0 based */ src_w--; src_h--; @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) I915_WRITE(SPSURF(pipe, plane), 0); POSTING_READ(SPSURF(pipe, plane)); - - intel_update_sprite_watermarks(dplane, crtc); } static void @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (IS_HASWELL(dev) || IS_BROADWELL(dev)) sprctl |= SPRITE_PIPE_CSC_ENABLE; - intel_update_sprite_watermarks(plane, crtc); - /* Sizes are 0 based */ src_w--; src_h--; @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (IS_GEN6(dev)) dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */ - intel_update_sprite_watermarks(plane, crtc); - /* Sizes are 0 based */ src_w--; src_h--;
We never removed the sprite watermark updates from our low-level foo_update_plane() functions; since our hardware updates happen under vblank evasion, we're not supposed to be calling potentially sleeping functions there (since interrupts are disabled). We do already have a mechanism that's supposed to be used to update sprite watermarks in the post-evasion function intel_post_plane_update(), but at the moment it's only being used for updates caused by plane disables. To fix this oversight we need to make a few changes: * When intel_plane_atomic_calc_changes() calls intel_wm_need_update() to determine whether watermarks need an update, we need to set the 'atomic.update_sprite_watermarks' flag rather than the 'atomic.update_wm' flag when the plane in question is a sprite. Some platforms don't care about this change since the two types of update do the same thing, but some platforms (e.g., ILK-style watermarks) need to specifically use the sprite update function to update cached watermark parameters. * intel_wm_need_update() needs to also look for plane size changes (previously it only checked tiling & rotation). With the above changes, the need for sprite watermark updates should be properly flagged at atomic 'check' time and handled at 'commit' time in post-evasion, so we can drop the direct calls to intel_update_sprite_watermarks() from all of the intel_plane->update_plane() handlers. We'll also toss a WARN_ON(irqs_disabled()) into the watermark update functions to avoid such mistakes in the future. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++------- drivers/gpu/drm/i915/intel_pm.c | 2 ++ drivers/gpu/drm/i915/intel_sprite.c | 12 ------------ 3 files changed, 18 insertions(+), 19 deletions(-)