| Message ID | 20171110113503.16253-9-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 10, 2017 at 12:35:01PM +0100, Maarten Lankhorst wrote: > pre_plane_disable and post_plane_enable handle set ips correctly, > but if there is no modeset and the ips_enabled value changes > because of force disabling for crc, or hw state readout, then we > don't toggle ips correctly. Handle this special case, which prevents > us from having to do a full modeset when collecting pipe crc. > > Changes since v1: > - Simplify conditions. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Hm yeah, more hints that maybe ips should be a plane, or at least a wm state thing? This turns a bit into a rabbit hole, so if Ville thinks this is all still reasonably clean-ish I think we can go with this here too ... I'm just wondering a bit. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 6 ++++++ > drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4aa02e27985d..1b75af773ef7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5040,6 +5040,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > (needs_modeset(&pipe_config->base) || > !old_primary_state->base.visible)) > intel_post_enable_primary(&crtc->base, pipe_config); > + else if (pipe_config->update_pipe) > + /* IPS turned on after fastset or CRC collection disable. */ > + hsw_enable_ips(pipe_config); > } > } > > @@ -5069,6 +5072,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > if (old_primary_state->base.visible && > (modeset || !primary_state->base.visible)) > intel_pre_disable_primary(&crtc->base, old_crtc_state); > + else if (pipe_config->update_pipe && !pipe_config->ips_enabled) > + /* IPS turned off for CRC, disable it. */ > + hsw_disable_ips(old_crtc_state); > } > > /* > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c > index 899839f2f7c6..cb92befc16d7 100644 > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > @@ -542,7 +542,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > */ > pipe_config->ips_force_disable = enable; > if (pipe_config->ips_enabled == enable) > - pipe_config->base.connectors_changed = true; > + pipe_config->base.mode_changed = true; > } > > if (IS_HASWELL(dev_priv)) { > -- > 2.15.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 10, 2017 at 02:15:04PM +0100, Daniel Vetter wrote: > On Fri, Nov 10, 2017 at 12:35:01PM +0100, Maarten Lankhorst wrote: > > pre_plane_disable and post_plane_enable handle set ips correctly, > > but if there is no modeset and the ips_enabled value changes > > because of force disabling for crc, or hw state readout, then we > > don't toggle ips correctly. Handle this special case, which prevents > > us from having to do a full modeset when collecting pipe crc. > > > > Changes since v1: > > - Simplify conditions. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Hm yeah, more hints that maybe ips should be a plane, or at least a wm > state thing? I think we would have to write this as something like: pre_plane_update() if (old_crtc_state->ips && old_crtc_state->active_planes != 0 && (!new_crtc_state->ips || new_crtc_state->active_planes == 0) disable_ips(); post_plane_update() if ((!old_crtc_state->ips || old_crtc_state->active_planes == 0) && new_crtc_state->ips && new_crtc_state->active_planes != 0) enable_ips(); Probably needs a few mode needs_modeset() and crtc_state->active checks etc. to account for all the disabling pipe, enabling pipe, toggling pipe, and doing nothing to the pipe cases. To simplify maybe we should probably just account for active_planes in crtc_state->ips_enabled already. Ie. call hsw_compute_ips_config() even if we don't do a modeset. We'd have to change the bdw cdclk vs. ips workaround to work the other way around though since currently hsw_compute_ips_config() assumes that we can do a modeset to increase cdclk (assuming it's not already maxed out). But I think it would be a worthwile change to make the code less convoluted. > > This turns a bit into a rabbit hole, so if Ville thinks this is all still > reasonably clean-ish I think we can go with this here too ... I'm just > wondering a bit. > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 6 ++++++ > > drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 4aa02e27985d..1b75af773ef7 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5040,6 +5040,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > > (needs_modeset(&pipe_config->base) || > > !old_primary_state->base.visible)) > > intel_post_enable_primary(&crtc->base, pipe_config); > > + else if (pipe_config->update_pipe) > > + /* IPS turned on after fastset or CRC collection disable. */ > > + hsw_enable_ips(pipe_config); > > } > > } > > > > @@ -5069,6 +5072,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > > if (old_primary_state->base.visible && > > (modeset || !primary_state->base.visible)) > > intel_pre_disable_primary(&crtc->base, old_crtc_state); > > + else if (pipe_config->update_pipe && !pipe_config->ips_enabled) > > + /* IPS turned off for CRC, disable it. */ > > + hsw_disable_ips(old_crtc_state); > > } > > > > /* > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c > > index 899839f2f7c6..cb92befc16d7 100644 > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > @@ -542,7 +542,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > > */ > > pipe_config->ips_force_disable = enable; > > if (pipe_config->ips_enabled == enable) > > - pipe_config->base.connectors_changed = true; > > + pipe_config->base.mode_changed = true; > > } > > > > if (IS_HASWELL(dev_priv)) { > > -- > > 2.15.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4aa02e27985d..1b75af773ef7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5040,6 +5040,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) (needs_modeset(&pipe_config->base) || !old_primary_state->base.visible)) intel_post_enable_primary(&crtc->base, pipe_config); + else if (pipe_config->update_pipe) + /* IPS turned on after fastset or CRC collection disable. */ + hsw_enable_ips(pipe_config); } } @@ -5069,6 +5072,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, if (old_primary_state->base.visible && (modeset || !primary_state->base.visible)) intel_pre_disable_primary(&crtc->base, old_crtc_state); + else if (pipe_config->update_pipe && !pipe_config->ips_enabled) + /* IPS turned off for CRC, disable it. */ + hsw_disable_ips(old_crtc_state); } /* diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c index 899839f2f7c6..cb92befc16d7 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -542,7 +542,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, */ pipe_config->ips_force_disable = enable; if (pipe_config->ips_enabled == enable) - pipe_config->base.connectors_changed = true; + pipe_config->base.mode_changed = true; } if (IS_HASWELL(dev_priv)) {
pre_plane_disable and post_plane_enable handle set ips correctly, but if there is no modeset and the ips_enabled value changes because of force disabling for crc, or hw state readout, then we don't toggle ips correctly. Handle this special case, which prevents us from having to do a full modeset when collecting pipe crc. Changes since v1: - Simplify conditions. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 6 ++++++ drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)