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@xxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx