Tested-by: Marta Lofstedt <marta.lofstedt@xxxxxxxxx> > -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > Sent: Thursday, August 10, 2017 12:55 PM > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Lofstedt, Marta <marta.lofstedt@xxxxxxxxx>; Maarten Lankhorst > <maarten.lankhorst@xxxxxxxxxxxxxxx> > Subject: [PATCH] drm/i915: Keep IPS disabled during CRC collection > > We need to look at crtc->state->active for the legacy crc to match the drm crc > api. Even though the drm api explictily checks it, there's no harm in doing the > same there so keep it for both. > > Introduce a intel_crtc->ips_disabled flag for CRC, which is protected by crtc- > >mutex and set it to true while collecting. This way CRC will not interfere with > ips, regardless of visibility. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101664 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++-------------- > drivers/gpu/drm/i915/intel_drv.h | 3 ++ > drivers/gpu/drm/i915/intel_pipe_crc.c | 58 > +++++++++++++++++++++++++++-------- > 3 files changed, 71 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a8775ed817d1..88dfb4bd8db0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -117,7 +117,8 @@ static void ironlake_pfit_disable(struct intel_crtc > *crtc, bool force); static void ironlake_pfit_enable(struct intel_crtc *crtc); > static void intel_modeset_setup_hw_state(struct drm_device *dev, > > struct drm_modeset_acquire_ctx *ctx); -static void > intel_pre_disable_primary_noatomic(struct drm_crtc *crtc); > +static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc, > + > struct intel_crtc_state *pipe_config); > > struct intel_limit { > struct { > @@ -2723,7 +2724,8 @@ intel_find_initial_plane_obj(struct intel_crtc > *intel_crtc, > intel_set_plane_visible(to_intel_crtc_state(crtc_state), > > to_intel_plane_state(plane_state), > false); > - intel_pre_disable_primary_noatomic(&intel_crtc->base); > + intel_pre_disable_primary_noatomic(&intel_crtc->base, > + > to_intel_crtc_state(crtc_state)); > trace_intel_disable_plane(primary, intel_crtc); > intel_plane->disable_plane(intel_plane, intel_crtc); > > @@ -4668,9 +4670,6 @@ void hsw_enable_ips(struct intel_crtc *crtc) > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > > - if (!crtc->config->ips_enabled) > - return; > - > /* > * We can only enable IPS after we enable a plane and wait for > a vblank > * This function is called from post_plane_update, which is run > after @@ -4706,9 +4705,6 @@ void hsw_disable_ips(struct intel_crtc *crtc) > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > > - if (!crtc->config->ips_enabled) > - return; > - > assert_plane_enabled(dev_priv, crtc->plane); > if (IS_BROADWELL(dev_priv)) { > mutex_lock(&dev_priv->rps.hw_lock); > @@ -4754,7 +4750,7 @@ static void intel_crtc_dpms_overlay_disable(struct > intel_crtc *intel_crtc) > * completely hide the primary plane. > */ > static void > -intel_post_enable_primary(struct drm_crtc *crtc) > +intel_post_enable_primary(struct drm_crtc *crtc, struct > +intel_crtc_state *pipe_config) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); @@ -4767,7 > +4763,8 @@ intel_post_enable_primary(struct drm_crtc *crtc) > * when going from primary only to sprite only and vice > * versa. > */ > - hsw_enable_ips(intel_crtc); > + if (pipe_config->ips_enabled && !intel_crtc->ips_disabled) > + hsw_enable_ips(intel_crtc); > > /* > * Gen2 reports pipe underruns whenever all planes are > disabled. > @@ -4786,7 +4783,7 @@ intel_post_enable_primary(struct drm_crtc *crtc) > > /* FIXME move all this to pre_plane_update() with proper state tracking */ > static void -intel_pre_disable_primary(struct drm_crtc *crtc) > +intel_pre_disable_primary(struct drm_crtc *crtc, struct > +intel_crtc_state *pipe_config) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); @@ - > 4808,19 +4805,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc) > * when going from primary only to sprite only and vice > * versa. > */ > - hsw_disable_ips(intel_crtc); > + if (pipe_config->ips_enabled) > + hsw_disable_ips(intel_crtc); > } > > /* FIXME get rid of this and use pre_plane_update */ static void - > intel_pre_disable_primary_noatomic(struct drm_crtc *crtc) > +intel_pre_disable_primary_noatomic(struct drm_crtc *crtc, struct > +intel_crtc_state *pipe_config) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > int pipe = intel_crtc->pipe; > > - intel_pre_disable_primary(crtc); > + intel_pre_disable_primary(crtc, pipe_config); > > /* > * Vblank time updates from the shadow to live plane control > register @@ -4858,7 +4856,7 @@ static void intel_post_plane_update(struct > intel_crtc_state *old_crtc_state) > if (new_primary_state->visible && > (needs_modeset(&pipe_config->base) || > !old_primary_state->visible)) > - intel_post_enable_primary(&crtc- > >base); > + intel_post_enable_primary(&crtc- > >base, pipe_config); > } > } > > @@ -4885,7 +4883,7 @@ static void intel_pre_plane_update(struct > intel_crtc_state *old_crtc_state, > > if (old_primary_state->visible && > (modeset || !new_primary_state->visible)) > - intel_pre_disable_primary(&crtc- > >base); > + intel_pre_disable_primary(&crtc- > >base, pipe_config); > } > > /* > @@ -5700,13 +5698,6 @@ static void intel_crtc_disable_noatomic(struct > drm_crtc *crtc, > if (!intel_crtc->active) > return; > > - if (crtc->primary->state->visible) { > - intel_pre_disable_primary_noatomic(crtc); > - > - intel_crtc_disable_planes(crtc, 1 << > drm_plane_index(crtc->primary)); > - crtc->primary->state->visible = false; > - } > - > state = drm_atomic_state_alloc(crtc->dev); > if (!state) { > DRM_DEBUG_KMS("failed to disable > [CRTC:%d:%s], out of memory", @@ -5722,6 +5713,14 @@ static void > intel_crtc_disable_noatomic(struct drm_crtc *crtc, > > WARN_ON(IS_ERR(crtc_state) || ret); > > + if (crtc->primary->state->visible) { > + intel_pre_disable_primary_noatomic(crtc, > crtc_state); > + > + intel_crtc_disable_planes(crtc, 1 << > drm_plane_index(crtc->primary)); > + crtc->primary->state->visible = false; > + } > + > + > dev_priv->display.crtc_disable(crtc_state, state); > > drm_atomic_state_put(state); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index a07d06fbc4b4..872b1fce1d0e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -828,6 +828,9 @@ struct intel_crtc { > bool cpu_fifo_underrun_disabled; > bool pch_fifo_underrun_disabled; > > + /* Whether ips is force-disabled for collecting CRC */ > + bool ips_disabled; > + > /* per-pipe watermark state */ > struct { > /* watermarks currently being used */ diff --git > a/drivers/gpu/drm/i915/intel_pipe_crc.c > b/drivers/gpu/drm/i915/intel_pipe_crc.c > index 8fbd2bd0877f..fc3682c4c055 100644 > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > @@ -601,6 +601,33 @@ static int get_new_crc_ctl_reg(struct > drm_i915_private *dev_priv, > return ivb_pipe_crc_ctl_reg(dev_priv, pipe, > source, val); } > > +static int toggle_ips_and_test_active(struct intel_crtc *crtc, > + > bool has_source) > +{ > + bool has_ips = false; > + int ret; > + > + > + ret = drm_modeset_lock_interruptible(&crtc->base.mutex, > NULL); > + if (ret) > + return ret; > + > + if (to_intel_crtc_state(crtc->base.state)->ips_enabled && > + crtc->base.state->plane_mask & BIT(drm_plane_index(crtc- > >base.primary))) > + has_ips = crtc->base.primary->state->visible; > + > + if (!crtc->base.state->active && has_source) { > + DRM_DEBUG_KMS("Trying to capture CRC while > pipe is off\n"); > + ret = -EIO; > + } > + > + crtc->ips_disabled = has_source; > + > + drm_modeset_unlock(&crtc->base.mutex); > + > + return ret ?: has_ips; > +} > + > static int pipe_crc_set_source(struct drm_i915_private *dev_priv, > enum pipe pipe, > enum intel_pipe_crc_source > source) @@ -610,6 +637,7 @@ static int pipe_crc_set_source(struct > drm_i915_private *dev_priv, > enum intel_display_power_domain power_domain; > u32 val = 0; /* shut up gcc */ > int ret; > + int has_ips; > > if (pipe_crc->source == source) > return 0; > @@ -618,11 +646,12 @@ static int pipe_crc_set_source(struct > drm_i915_private *dev_priv, > if (pipe_crc->source && source) > return -EINVAL; > > + has_ips = toggle_ips_and_test_active(crtc, source); > + if (has_ips < 0) > + return has_ips; > + > power_domain = POWER_DOMAIN_PIPE(pipe); > - if (!intel_display_power_get_if_enabled(dev_priv, > power_domain)) { > - DRM_DEBUG_KMS("Trying to capture CRC while > pipe is off\n"); > - return -EIO; > - } > + intel_display_power_get(dev_priv, power_domain); > > ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val); > if (ret != 0) > @@ -649,7 +678,8 @@ static int pipe_crc_set_source(struct > drm_i915_private *dev_priv, > * user space can't make reliable use of the > CRCs, so let's just > * completely disable it. > */ > - hsw_disable_ips(crtc); > + if (has_ips) > + hsw_disable_ips(crtc); > > spin_lock_irq(&pipe_crc->lock); > kfree(pipe_crc->entries); > @@ -694,7 +724,8 @@ static int pipe_crc_set_source(struct > drm_i915_private *dev_priv, > else if (IS_HASWELL(dev_priv) && pipe == > PIPE_A) > > hsw_trans_edp_pipe_A_crc_wa(dev_priv, false); > > - hsw_enable_ips(crtc); > + if (has_ips) > + hsw_enable_ips(crtc); > } > > ret = 0; > @@ -919,23 +950,25 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, > const char *source_name, > enum intel_pipe_crc_source source; > u32 val = 0; /* shut up gcc */ > int ret = 0; > + int has_ips; > > if (display_crc_ctl_parse_source(source_name, &source) < 0) { > DRM_DEBUG_DRIVER("unknown source %s\n", > source_name); > return -EINVAL; > } > > + has_ips = toggle_ips_and_test_active(intel_crtc, source); > + if (has_ips < 0) > + return has_ips; > + > power_domain = POWER_DOMAIN_PIPE(crtc->index); > - if (!intel_display_power_get_if_enabled(dev_priv, > power_domain)) { > - DRM_DEBUG_KMS("Trying to capture CRC while > pipe is off\n"); > - return -EIO; > - } > + intel_display_power_get(dev_priv, power_domain); > > ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, > &val); > if (ret != 0) > goto out; > > - if (source) { > + if (source && has_ips) { > /* > * When IPS gets enabled, the pipe CRC > changes. Since IPS gets > * enabled and disabled dynamically based on > package C states, @@ -956,7 +989,8 @@ int intel_crtc_set_crc_source(struct > drm_crtc *crtc, const char *source_name, > else if (IS_HASWELL(dev_priv) && crtc->index > == PIPE_A) > > hsw_trans_edp_pipe_A_crc_wa(dev_priv, false); > > - hsw_enable_ips(intel_crtc); > + if (has_ips) > + hsw_enable_ips(intel_crtc); > } > > pipe_crc->skipped = 0; > -- > 2.11.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
