| Message ID | 1457543247-13987-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") > broke thigns by removing the pre vs. post wm update distinction. We also > lost the pre plane wm update entirely for VLV/CHV from the crtc enable > path. > > This caused underruns on modeset and plane enable/disable on CHV, > and often those can lead to a dead pipe. > > So let's bring back the pre vs. post thing, and let's toss in an > explicit wm update to valleyview_crtc_enable() to avoid having to > put it into the common code. > > This is more or less a partial revert of the offending commit. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: drm-intel-fixes@lists.freedesktop.org > Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 3 ++- > drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++---------- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 22 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 6a661e796328..79448f1c8b8d 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -96,7 +96,8 @@ 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; > + crtc_state->update_wm_pre = false; > + crtc_state->update_wm_post = false; > crtc_state->fb_changed = false; > crtc_state->wm.need_postvbl_update = false; There's already a wm.need_postvbl_update. could we use it for non-atomic platforms, and add a wm.need_intermediate_update ? This could probably be used for ILK too. I believe that if a driver provides the atomic wm functions those functions should also set the wm.*_update members themselves, while for the watermarks that aren't converted yet it should be provided by the core. ~Maarten
On Thu, Mar 10, 2016 at 10:09:40AM +0100, Maarten Lankhorst wrote: > Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") > > broke thigns by removing the pre vs. post wm update distinction. We also > > lost the pre plane wm update entirely for VLV/CHV from the crtc enable > > path. > > > > This caused underruns on modeset and plane enable/disable on CHV, > > and often those can lead to a dead pipe. > > > > So let's bring back the pre vs. post thing, and let's toss in an > > explicit wm update to valleyview_crtc_enable() to avoid having to > > put it into the common code. > > > > This is more or less a partial revert of the offending commit. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: drm-intel-fixes@lists.freedesktop.org > > Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_atomic.c | 3 ++- > > drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++---------- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > 3 files changed, 22 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > > index 6a661e796328..79448f1c8b8d 100644 > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > @@ -96,7 +96,8 @@ 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; > > + crtc_state->update_wm_pre = false; > > + crtc_state->update_wm_post = false; > > crtc_state->fb_changed = false; > > crtc_state->wm.need_postvbl_update = false; > There's already a wm.need_postvbl_update. > could we use it for non-atomic platforms, and add a wm.need_intermediate_update ? > This could probably be used for ILK too. Dunno. All I know is the regression is now on it's way 4.5 and needs to be dealt with ASAP. So no major overhaul at this point. > > I believe that if a driver provides the atomic wm functions those functions should also set the wm.*_update members themselves, > while for the watermarks that aren't converted yet it should be provided by the core. > > ~Maarten
Op 10-03-16 om 11:33 schreef Ville Syrjälä: > On Thu, Mar 10, 2016 at 10:09:40AM +0100, Maarten Lankhorst wrote: >> Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") >>> broke thigns by removing the pre vs. post wm update distinction. We also >>> lost the pre plane wm update entirely for VLV/CHV from the crtc enable >>> path. >>> >>> This caused underruns on modeset and plane enable/disable on CHV, >>> and often those can lead to a dead pipe. >>> >>> So let's bring back the pre vs. post thing, and let's toss in an >>> explicit wm update to valleyview_crtc_enable() to avoid having to >>> put it into the common code. >>> >>> This is more or less a partial revert of the offending commit. >>> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: drm-intel-fixes@lists.freedesktop.org >>> Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_atomic.c | 3 ++- >>> drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++---------- >>> drivers/gpu/drm/i915/intel_drv.h | 2 +- >>> 3 files changed, 22 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >>> index 6a661e796328..79448f1c8b8d 100644 >>> --- a/drivers/gpu/drm/i915/intel_atomic.c >>> +++ b/drivers/gpu/drm/i915/intel_atomic.c >>> @@ -96,7 +96,8 @@ 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; >>> + crtc_state->update_wm_pre = false; >>> + crtc_state->update_wm_post = false; >>> crtc_state->fb_changed = false; >>> crtc_state->wm.need_postvbl_update = false; >> There's already a wm.need_postvbl_update. >> could we use it for non-atomic platforms, and add a wm.need_intermediate_update ? >> This could probably be used for ILK too. > Dunno. All I know is the regression is now on it's way 4.5 and needs to > be dealt with ASAP. So no major overhaul at this point. > Ok, since I lack the hardware to test for now this will have to do.. I'd like this to be revisited in the future. Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> ~Maarten
On Thu, Mar 10, 2016 at 12:53:37PM +0100, Maarten Lankhorst wrote: > Op 10-03-16 om 11:33 schreef Ville Syrjälä: > > On Thu, Mar 10, 2016 at 10:09:40AM +0100, Maarten Lankhorst wrote: > >> Op 09-03-16 om 18:07 schreef ville.syrjala@linux.intel.com: > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> > >>> commit 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") > >>> broke thigns by removing the pre vs. post wm update distinction. We also > >>> lost the pre plane wm update entirely for VLV/CHV from the crtc enable > >>> path. > >>> > >>> This caused underruns on modeset and plane enable/disable on CHV, > >>> and often those can lead to a dead pipe. > >>> > >>> So let's bring back the pre vs. post thing, and let's toss in an > >>> explicit wm update to valleyview_crtc_enable() to avoid having to > >>> put it into the common code. > >>> > >>> This is more or less a partial revert of the offending commit. > >>> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Cc: drm-intel-fixes@lists.freedesktop.org > >>> Fixes: 92826fcdfc14 ("drm/i915: Calculate watermark related members in the crtc_state, v4.") > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> --- > >>> drivers/gpu/drm/i915/intel_atomic.c | 3 ++- > >>> drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++---------- > >>> drivers/gpu/drm/i915/intel_drv.h | 2 +- > >>> 3 files changed, 22 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > >>> index 6a661e796328..79448f1c8b8d 100644 > >>> --- a/drivers/gpu/drm/i915/intel_atomic.c > >>> +++ b/drivers/gpu/drm/i915/intel_atomic.c > >>> @@ -96,7 +96,8 @@ 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; > >>> + crtc_state->update_wm_pre = false; > >>> + crtc_state->update_wm_post = false; > >>> crtc_state->fb_changed = false; > >>> crtc_state->wm.need_postvbl_update = false; > >> There's already a wm.need_postvbl_update. > >> could we use it for non-atomic platforms, and add a wm.need_intermediate_update ? > >> This could probably be used for ILK too. > > Dunno. All I know is the regression is now on it's way 4.5 and needs to > > be dealt with ASAP. So no major overhaul at this point. > > > Ok, since I lack the hardware to test for now this will have to do.. I'd like this to be revisited in the future. > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Thanks for the reviews everyone. Series pushed to dinq.
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 6a661e796328..79448f1c8b8d 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -96,7 +96,8 @@ 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; + crtc_state->update_wm_pre = false; + crtc_state->update_wm_post = false; crtc_state->fb_changed = false; crtc_state->wm.need_postvbl_update = false; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 62d36a7b3398..8b44e0652740 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4919,7 +4919,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc) crtc->wm.cxsr_allowed = true; - if (pipe_config->wm_changed && pipe_config->base.active) + if (pipe_config->update_wm_post && pipe_config->base.active) intel_update_watermarks(&crtc->base); if (atomic->update_fbc) @@ -5001,7 +5001,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) */ if (dev_priv->display.initial_watermarks != NULL) dev_priv->display.initial_watermarks(pipe_config); - else if (pipe_config->wm_changed) + else if (pipe_config->update_wm_pre) intel_update_watermarks(&crtc->base); } @@ -6372,6 +6372,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(intel_crtc); assert_vblank_disabled(crtc); @@ -11994,19 +11995,27 @@ 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 || turn_off) { - pipe_config->wm_changed = true; + if (turn_on) { + pipe_config->update_wm_pre = true; + + /* must disable cxsr around plane enable/disable */ + if (plane->type != DRM_PLANE_TYPE_CURSOR) + pipe_config->disable_cxsr = true; + } else if (turn_off) { + pipe_config->update_wm_post = true; /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) pipe_config->disable_cxsr = true; } else if (intel_wm_need_update(plane, plane_state)) { - pipe_config->wm_changed = true; + /* FIXME bollocks */ + pipe_config->update_wm_pre = true; + pipe_config->update_wm_post = true; } /* Pre-gen9 platforms need two-step watermark updates */ - if (pipe_config->wm_changed && INTEL_INFO(dev)->gen < 9 && - dev_priv->display.optimize_watermarks) + if ((pipe_config->update_wm_pre || pipe_config->update_wm_post) && + INTEL_INFO(dev)->gen < 9 && dev_priv->display.optimize_watermarks) to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true; if (visible || was_visible) @@ -12106,7 +12115,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, } if (mode_changed && !crtc_state->active) - pipe_config->wm_changed = true; + pipe_config->update_wm_post = true; if (mode_changed && crtc_state->enable && dev_priv->display.crtc_compute_clock && @@ -13645,12 +13654,12 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state) return true; /* wm changes, need vblank before final wm's */ - if (crtc_state->wm_changed) + if (crtc_state->update_wm_post) return true; /* * cxsr is re-enabled after vblank. - * This is already handled by crtc_state->wm_changed, + * This is already handled by crtc_state->update_wm_post, * but added for clarity. */ if (crtc_state->disable_cxsr) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7b2d66d8dd7f..c33e9690df51 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -420,7 +420,7 @@ struct intel_crtc_state { bool update_pipe; /* can a fast modeset be performed? */ bool disable_cxsr; - bool wm_changed; /* watermarks are updated */ + bool update_wm_pre, update_wm_post; /* watermarks are updated */ bool fb_changed; /* fb on any of the planes is changed */ /* Pipe source size (ie. panel fitter input size)