| Message ID | 1447945645-32005-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: > This removes pre/post_wm_update from intel_crtc->atomic, and > creates atomic state for it in intel_crtc. > > Changes since v1: > - Rebase on top of wm changes. > Changes since v2: > - Split disable_cxsr into a separate patch. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 1 + > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------ > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 15 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > b/drivers/gpu/drm/i915/intel_atomic.c > index 9f0638a37b6d..4625f8a9ba12 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > crtc_state->update_pipe = false; > crtc_state->disable_lp_wm = false; > crtc_state->disable_cxsr = false; > + crtc_state->wm_changed = false; > > return &crtc_state->base; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 5ee64e67ad8a..db4995406277 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc) > static void intel_post_plane_update(struct intel_crtc *crtc) > { > struct intel_crtc_atomic_commit *atomic = &crtc->atomic; > + struct intel_crtc_state *pipe_config = > + to_intel_crtc_state(crtc->base.state); > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc > *crtc) > > crtc->wm.cxsr_allowed = true; > > - if (crtc->atomic.update_wm_post) > + if (pipe_config->wm_changed) > intel_update_watermarks(&crtc->base); This adds an extra call to intel_update_watermarks() for the case where previously update_wm_pre would be set. This won't cause extra register writes because of the dirty check, but I think it deserves a note in the commit message. > > if (atomic->update_fbc) > @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc > *crtc) > crtc->wm.cxsr_allowed = false; > intel_set_memory_cxsr(dev_priv, false); > } > + > + if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed) > + intel_update_watermarks(&crtc->base); > } > > static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned > plane_mask) > @@ -11706,25 +11711,18 @@ 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 (turn_on) { > - intel_crtc->atomic.update_wm_pre = true; > - /* must disable cxsr around plane enable/disable */ > - if (plane->type != DRM_PLANE_TYPE_CURSOR) { > - pipe_config->disable_cxsr = true; > - /* to potentially re-enable cxsr */ > - intel_crtc->atomic.wait_vblank = true; > - intel_crtc->atomic.update_wm_post = true; > - } > - } else if (turn_off) { > - intel_crtc->atomic.update_wm_post = true; > + if (turn_on || turn_off) { > + pipe_config->wm_changed = true; > + > /* must disable cxsr around plane enable/disable */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > if (is_crtc_enabled) > intel_crtc->atomic.wait_vblank = true; > pipe_config->disable_cxsr = true; > } > - } else if (intel_wm_need_update(plane, plane_state)) { > - intel_crtc->atomic.update_wm_pre = true; > + } else if ((was_visible || visible) && So this avoids watermark changes when the plane is not visible before or after the update. Wouldn't it be better to fix intel_wm_need_update() instead if returns true in that case? Ander > + intel_wm_need_update(plane, plane_state)) { > + pipe_config->wm_changed = true; > } > > if (visible || was_visible) > @@ -11869,7 +11867,7 @@ static int intel_crtc_atomic_check(struct drm_crtc > *crtc, > } > > if (mode_changed && !crtc_state->active) > - intel_crtc->atomic.update_wm_post = true; > + pipe_config->wm_changed = true; > > if (mode_changed && crtc_state->enable && > dev_priv->display.crtc_compute_clock && > @@ -13762,9 +13760,6 @@ static void intel_begin_crtc_commit(struct drm_crtc > *crtc, > to_intel_crtc_state(old_crtc_state); > bool modeset = needs_modeset(crtc->state); > > - if (intel_crtc->atomic.update_wm_pre) > - intel_update_watermarks(crtc); > - > /* Perform vblank evasion around commit operation */ > intel_pipe_update_start(intel_crtc); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index dd89342832e2..db61c37dbf09 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -372,6 +372,7 @@ struct intel_crtc_state { > > bool update_pipe; /* can a fast modeset be performed? */ > bool disable_cxsr; > + bool wm_changed; /* watermarks are updated */ > > /* Pipe source size (ie. panel fitter input size) > * All planes will be positioned inside this space, > @@ -535,7 +536,6 @@ struct intel_crtc_atomic_commit { > bool disable_fbc; > bool disable_ips; > bool pre_disable_primary; > - bool update_wm_pre, update_wm_post; > > /* Sleepable operations to perform after commit */ > unsigned fb_bits;
Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira: > On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: >> This removes pre/post_wm_update from intel_crtc->atomic, and >> creates atomic state for it in intel_crtc. >> >> Changes since v1: >> - Rebase on top of wm changes. >> Changes since v2: >> - Split disable_cxsr into a separate patch. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic.c | 1 + >> drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------ >> drivers/gpu/drm/i915/intel_drv.h | 2 +- >> 3 files changed, 15 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c >> b/drivers/gpu/drm/i915/intel_atomic.c >> index 9f0638a37b6d..4625f8a9ba12 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) >> crtc_state->update_pipe = false; >> crtc_state->disable_lp_wm = false; >> crtc_state->disable_cxsr = false; >> + crtc_state->wm_changed = false; >> >> return &crtc_state->base; >> } >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 5ee64e67ad8a..db4995406277 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc) >> static void intel_post_plane_update(struct intel_crtc *crtc) >> { >> struct intel_crtc_atomic_commit *atomic = &crtc->atomic; >> + struct intel_crtc_state *pipe_config = >> + to_intel_crtc_state(crtc->base.state); >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc >> *crtc) >> >> crtc->wm.cxsr_allowed = true; >> >> - if (crtc->atomic.update_wm_post) >> + if (pipe_config->wm_changed) >> intel_update_watermarks(&crtc->base); > This adds an extra call to intel_update_watermarks() for the case where > previously update_wm_pre would be set. This won't cause extra register writes > because of the dirty check, but I think it deserves a note in the commit > message. I think intel_update_watermarks needs to be fixed to take a crtc_state and whether pre/post commit is used. It looks to me like doing anything else could bug if watermarks are changed with 1 plane becoming visible and the other invisible.. >> >> if (atomic->update_fbc) >> @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc >> *crtc) >> crtc->wm.cxsr_allowed = false; >> intel_set_memory_cxsr(dev_priv, false); >> } >> + >> + if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed) >> + intel_update_watermarks(&crtc->base); >> } >> >> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned >> plane_mask) >> @@ -11706,25 +11711,18 @@ 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 (turn_on) { >> - intel_crtc->atomic.update_wm_pre = true; >> - /* must disable cxsr around plane enable/disable */ >> - if (plane->type != DRM_PLANE_TYPE_CURSOR) { >> - pipe_config->disable_cxsr = true; >> - /* to potentially re-enable cxsr */ >> - intel_crtc->atomic.wait_vblank = true; >> - intel_crtc->atomic.update_wm_post = true; >> - } >> - } else if (turn_off) { >> - intel_crtc->atomic.update_wm_post = true; >> + if (turn_on || turn_off) { >> + pipe_config->wm_changed = true; >> + >> /* must disable cxsr around plane enable/disable */ >> if (plane->type != DRM_PLANE_TYPE_CURSOR) { >> if (is_crtc_enabled) >> intel_crtc->atomic.wait_vblank = true; >> pipe_config->disable_cxsr = true; >> } >> - } else if (intel_wm_need_update(plane, plane_state)) { >> - intel_crtc->atomic.update_wm_pre = true; >> + } else if ((was_visible || visible) && > So this avoids watermark changes when the plane is not visible before or after > the update. Wouldn't it be better to fix intel_wm_need_update() instead if > returns true in that case? If visible is changed wm_changed = true is always set. It would go through the (turn_on || turn_off) case. I guess checking was_visible || visible is overkill, and could be changed to just visible && intel_wm_need_update(plane, plane_state).
On Tue, 2015-11-24 at 15:55 +0100, Maarten Lankhorst wrote: > Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira: > > On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: > > > This removes pre/post_wm_update from intel_crtc->atomic, and > > > creates atomic state for it in intel_crtc. > > > > > > Changes since v1: > > > - Rebase on top of wm changes. > > > Changes since v2: > > > - Split disable_cxsr into a separate patch. > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_atomic.c | 1 + > > > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------ > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > 3 files changed, 15 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > > > b/drivers/gpu/drm/i915/intel_atomic.c > > > index 9f0638a37b6d..4625f8a9ba12 100644 > > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > > @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > > crtc_state->update_pipe = false; > > > crtc_state->disable_lp_wm = false; > > > crtc_state->disable_cxsr = false; > > > + crtc_state->wm_changed = false; > > > > > > return &crtc_state->base; > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 5ee64e67ad8a..db4995406277 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc) > > > static void intel_post_plane_update(struct intel_crtc *crtc) > > > { > > > struct intel_crtc_atomic_commit *atomic = &crtc->atomic; > > > + struct intel_crtc_state *pipe_config = > > > + to_intel_crtc_state(crtc->base.state); > > > struct drm_device *dev = crtc->base.dev; > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct > > > intel_crtc > > > *crtc) > > > > > > crtc->wm.cxsr_allowed = true; > > > > > > - if (crtc->atomic.update_wm_post) > > > + if (pipe_config->wm_changed) > > > intel_update_watermarks(&crtc->base); > > This adds an extra call to intel_update_watermarks() for the case where > > previously update_wm_pre would be set. This won't cause extra register > > writes > > because of the dirty check, but I think it deserves a note in the commit > > message. > I think intel_update_watermarks needs to be fixed to take a crtc_state and > whether pre/post commit is used. > > It looks to me like doing anything else could bug if watermarks are changed > with 1 plane becoming visible and the other invisible.. > > > > > > if (atomic->update_fbc) > > > @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc > > > *crtc) > > > crtc->wm.cxsr_allowed = false; > > > intel_set_memory_cxsr(dev_priv, false); > > > } > > > + > > > + if (!needs_modeset(&pipe_config->base) && pipe_config > > > ->wm_changed) > > > + intel_update_watermarks(&crtc->base); > > > } > > > > > > static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned > > > plane_mask) > > > @@ -11706,25 +11711,18 @@ 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 (turn_on) { > > > - intel_crtc->atomic.update_wm_pre = true; > > > - /* must disable cxsr around plane enable/disable */ > > > - if (plane->type != DRM_PLANE_TYPE_CURSOR) { > > > - pipe_config->disable_cxsr = true; > > > - /* to potentially re-enable cxsr */ > > > - intel_crtc->atomic.wait_vblank = true; > > > - intel_crtc->atomic.update_wm_post = true; > > > - } > > > - } else if (turn_off) { > > > - intel_crtc->atomic.update_wm_post = true; > > > + if (turn_on || turn_off) { > > > + pipe_config->wm_changed = true; > > > + > > > /* must disable cxsr around plane enable/disable */ > > > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > > > if (is_crtc_enabled) > > > intel_crtc->atomic.wait_vblank = true; > > > pipe_config->disable_cxsr = true; > > > } > > > - } else if (intel_wm_need_update(plane, plane_state)) { > > > - intel_crtc->atomic.update_wm_pre = true; > > > + } else if ((was_visible || visible) && > > So this avoids watermark changes when the plane is not visible before or > > after > > the update. Wouldn't it be better to fix intel_wm_need_update() instead if > > returns true in that case? > > If visible is changed wm_changed = true is always set. It would go through the > (turn_on || turn_off) case. > > I guess checking was_visible || visible is overkill, and could be changed to > just visible && intel_wm_need_update(plane, plane_state). What I meant is that I find odd that intel_wm_need_update() returns true when both was_visible and visible is false. Since that function is supposed to compare two plane states and tell us if a wm update is needed, I'd rather add the visible check there.
Op 25-11-15 om 10:22 schreef Ander Conselvan De Oliveira: > On Tue, 2015-11-24 at 15:55 +0100, Maarten Lankhorst wrote: >> Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira: >>> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: >>>> This removes pre/post_wm_update from intel_crtc->atomic, and >>>> creates atomic state for it in intel_crtc. >>>> >>>> Changes since v1: >>>> - Rebase on top of wm changes. >>>> Changes since v2: >>>> - Split disable_cxsr into a separate patch. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_atomic.c | 1 + >>>> drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------ >>>> drivers/gpu/drm/i915/intel_drv.h | 2 +- >>>> 3 files changed, 15 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c >>>> b/drivers/gpu/drm/i915/intel_atomic.c >>>> index 9f0638a37b6d..4625f8a9ba12 100644 >>>> --- a/drivers/gpu/drm/i915/intel_atomic.c >>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c >>>> @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) >>>> crtc_state->update_pipe = false; >>>> crtc_state->disable_lp_wm = false; >>>> crtc_state->disable_cxsr = false; >>>> + crtc_state->wm_changed = false; >>>> >>>> return &crtc_state->base; >>>> } >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c >>>> b/drivers/gpu/drm/i915/intel_display.c >>>> index 5ee64e67ad8a..db4995406277 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc) >>>> static void intel_post_plane_update(struct intel_crtc *crtc) >>>> { >>>> struct intel_crtc_atomic_commit *atomic = &crtc->atomic; >>>> + struct intel_crtc_state *pipe_config = >>>> + to_intel_crtc_state(crtc->base.state); >>>> struct drm_device *dev = crtc->base.dev; >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>> >>>> @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct >>>> intel_crtc >>>> *crtc) >>>> >>>> crtc->wm.cxsr_allowed = true; >>>> >>>> - if (crtc->atomic.update_wm_post) >>>> + if (pipe_config->wm_changed) >>>> intel_update_watermarks(&crtc->base); >>> This adds an extra call to intel_update_watermarks() for the case where >>> previously update_wm_pre would be set. This won't cause extra register >>> writes >>> because of the dirty check, but I think it deserves a note in the commit >>> message. >> I think intel_update_watermarks needs to be fixed to take a crtc_state and >> whether pre/post commit is used. >> >> It looks to me like doing anything else could bug if watermarks are changed >> with 1 plane becoming visible and the other invisible.. >>>> >>>> if (atomic->update_fbc) >>>> @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc >>>> *crtc) >>>> crtc->wm.cxsr_allowed = false; >>>> intel_set_memory_cxsr(dev_priv, false); >>>> } >>>> + >>>> + if (!needs_modeset(&pipe_config->base) && pipe_config >>>> ->wm_changed) >>>> + intel_update_watermarks(&crtc->base); >>>> } >>>> >>>> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned >>>> plane_mask) >>>> @@ -11706,25 +11711,18 @@ 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 (turn_on) { >>>> - intel_crtc->atomic.update_wm_pre = true; >>>> - /* must disable cxsr around plane enable/disable */ >>>> - if (plane->type != DRM_PLANE_TYPE_CURSOR) { >>>> - pipe_config->disable_cxsr = true; >>>> - /* to potentially re-enable cxsr */ >>>> - intel_crtc->atomic.wait_vblank = true; >>>> - intel_crtc->atomic.update_wm_post = true; >>>> - } >>>> - } else if (turn_off) { >>>> - intel_crtc->atomic.update_wm_post = true; >>>> + if (turn_on || turn_off) { >>>> + pipe_config->wm_changed = true; >>>> + >>>> /* must disable cxsr around plane enable/disable */ >>>> if (plane->type != DRM_PLANE_TYPE_CURSOR) { >>>> if (is_crtc_enabled) >>>> intel_crtc->atomic.wait_vblank = true; >>>> pipe_config->disable_cxsr = true; >>>> } >>>> - } else if (intel_wm_need_update(plane, plane_state)) { >>>> - intel_crtc->atomic.update_wm_pre = true; >>>> + } else if ((was_visible || visible) && >>> So this avoids watermark changes when the plane is not visible before or >>> after >>> the update. Wouldn't it be better to fix intel_wm_need_update() instead if >>> returns true in that case? >> If visible is changed wm_changed = true is always set. It would go through the >> (turn_on || turn_off) case. >> >> I guess checking was_visible || visible is overkill, and could be changed to >> just visible && intel_wm_need_update(plane, plane_state). > What I meant is that I find odd that intel_wm_need_update() returns true when > both was_visible and visible is false. Since that function is supposed to > compare two plane states and tell us if a wm update is needed, I'd rather add > the visible check there. This could be fixed there. if (visible != was_visible) return true; if (!visible) return false; and then remove the fb checks.
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 9f0638a37b6d..4625f8a9ba12 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) crtc_state->update_pipe = false; crtc_state->disable_lp_wm = false; crtc_state->disable_cxsr = false; + crtc_state->wm_changed = false; return &crtc_state->base; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5ee64e67ad8a..db4995406277 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc) static void intel_post_plane_update(struct intel_crtc *crtc) { struct intel_crtc_atomic_commit *atomic = &crtc->atomic; + struct intel_crtc_state *pipe_config = + to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc) crtc->wm.cxsr_allowed = true; - if (crtc->atomic.update_wm_post) + if (pipe_config->wm_changed) intel_update_watermarks(&crtc->base); if (atomic->update_fbc) @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) crtc->wm.cxsr_allowed = false; intel_set_memory_cxsr(dev_priv, false); } + + if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed) + intel_update_watermarks(&crtc->base); } static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) @@ -11706,25 +11711,18 @@ 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 (turn_on) { - intel_crtc->atomic.update_wm_pre = true; - /* must disable cxsr around plane enable/disable */ - if (plane->type != DRM_PLANE_TYPE_CURSOR) { - pipe_config->disable_cxsr = true; - /* to potentially re-enable cxsr */ - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_wm_post = true; - } - } else if (turn_off) { - intel_crtc->atomic.update_wm_post = true; + if (turn_on || turn_off) { + pipe_config->wm_changed = true; + /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) { if (is_crtc_enabled) intel_crtc->atomic.wait_vblank = true; pipe_config->disable_cxsr = true; } - } else if (intel_wm_need_update(plane, plane_state)) { - intel_crtc->atomic.update_wm_pre = true; + } else if ((was_visible || visible) && + intel_wm_need_update(plane, plane_state)) { + pipe_config->wm_changed = true; } if (visible || was_visible) @@ -11869,7 +11867,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, } if (mode_changed && !crtc_state->active) - intel_crtc->atomic.update_wm_post = true; + pipe_config->wm_changed = true; if (mode_changed && crtc_state->enable && dev_priv->display.crtc_compute_clock && @@ -13762,9 +13760,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, to_intel_crtc_state(old_crtc_state); bool modeset = needs_modeset(crtc->state); - if (intel_crtc->atomic.update_wm_pre) - intel_update_watermarks(crtc); - /* Perform vblank evasion around commit operation */ intel_pipe_update_start(intel_crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index dd89342832e2..db61c37dbf09 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -372,6 +372,7 @@ struct intel_crtc_state { bool update_pipe; /* can a fast modeset be performed? */ bool disable_cxsr; + bool wm_changed; /* watermarks are updated */ /* Pipe source size (ie. panel fitter input size) * All planes will be positioned inside this space, @@ -535,7 +536,6 @@ struct intel_crtc_atomic_commit { bool disable_fbc; bool disable_ips; bool pre_disable_primary; - bool update_wm_pre, update_wm_post; /* Sleepable operations to perform after commit */ unsigned fb_bits;
This removes pre/post_wm_update from intel_crtc->atomic, and creates atomic state for it in intel_crtc. Changes since v1: - Rebase on top of wm changes. Changes since v2: - Split disable_cxsr into a separate patch. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 1 + drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++------------------ drivers/gpu/drm/i915/intel_drv.h | 2 +- 3 files changed, 15 insertions(+), 19 deletions(-)