| Message ID | 20190306064728.8234-5-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/9] drm/i915/psr: Remove PSR2 FIXME | expand |
On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote: > Other features like PSR2 also needs to be disabled while getting CRC > so lets rename ips_force_disable to crc_enabled, drop all this checks > for pipe A and HSW and BDW and make it generic and > hsw_compute_ips_config() will take care of all the checks removed > from here. > > v2: Renaming and parameter changes to the functions that prepares the > commit (Ville) > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 10 ++++-- > drivers/gpu/drm/i915/intel_drv.h | 3 +- > drivers/gpu/drm/i915/intel_pipe_crc.c | 47 +++++++++++------------ > ---- > 3 files changed, 29 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 9312b3f35eb0..b3a5d8462251 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct > intel_crtc_state *crtc_state) > if (!hsw_crtc_state_ips_capable(crtc_state)) > return false; > > - if (crtc_state->ips_force_disable) > + /* > + * When IPS gets enabled, the pipe CRC changes. Since IPS gets > + * enabled and disabled dynamically based on package C states, > + * user space can't make reliable use of the CRCs, so let's > just > + * completely disable it. > + */ > + if (crtc_state->crc_enabled) > return false; > > /* IPS should be fine as long as at least one plane is enabled. > */ > @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct > intel_crtc_state *crtc_state) > saved_state->shared_dpll = crtc_state->shared_dpll; > saved_state->dpll_hw_state = crtc_state->dpll_hw_state; > saved_state->pch_pfit.force_thru = crtc_state- > >pch_pfit.force_thru; > - saved_state->ips_force_disable = crtc_state->ips_force_disable; > + saved_state->crc_enabled = crtc_state->crc_enabled; > if (IS_G4X(dev_priv) || > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > saved_state->wm = crtc_state->wm; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 5412373e2f98..2be64529e4a2 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -999,7 +999,8 @@ struct intel_crtc_state { > struct intel_link_m_n fdi_m_n; > > bool ips_enabled; > - bool ips_force_disable; > + > + bool crc_enabled; > > bool enable_fbc; > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c > b/drivers/gpu/drm/i915/intel_pipe_crc.c > index 53d4ec68d3c4..af64597c5c6e 100644 > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum > intel_pipe_crc_source *source, > return 0; > } > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > - bool enable) > +static void > +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool > enable) > { > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > struct drm_device *dev = &dev_priv->drm; We could eliminate this local. > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, > PIPE_A); > struct intel_crtc_state *pipe_config; > struct drm_atomic_state *state; > struct drm_modeset_acquire_ctx ctx; > - int ret = 0; > + int ret; > > drm_modeset_acquire_init(&ctx, 0); > > @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct > drm_i915_private *dev_priv, > goto put_state; > } > > - if (HAS_IPS(dev_priv)) { > - /* > - * When IPS gets enabled, the pipe CRC changes. Since > IPS gets > - * enabled and disabled dynamically based on package C > states, > - * user space can't make reliable use of the CRCs, so > let's just > - * completely disable it. > - */ > - pipe_config->ips_force_disable = enable; > - } > + pipe_config->crc_enabled = enable; > > - if (IS_HASWELL(dev_priv)) { > + if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) { > pipe_config->pch_pfit.force_thru = enable; > if (pipe_config->cpu_transcoder == TRANSCODER_EDP && > pipe_config->pch_pfit.enabled != enable) > @@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct > drm_i915_private *dev_priv, > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv, > enum pipe pipe, > enum intel_pipe_crc_source *source, > - u32 *val, > - bool set_wa) > + u32 *val) > { > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) > *source = INTEL_PIPE_CRC_SOURCE_PIPE; > @@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct > drm_i915_private *dev_priv, > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB; > break; > case INTEL_PIPE_CRC_SOURCE_PIPE: > - if (set_wa && (IS_HASWELL(dev_priv) || > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A) > - hsw_pipe_A_crc_wa(dev_priv, true); > - > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB; > break; > case INTEL_PIPE_CRC_SOURCE_NONE: > @@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct > drm_i915_private *dev_priv, > > static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv, > enum pipe pipe, > - enum intel_pipe_crc_source *source, u32 > *val, > - bool set_wa) > + enum intel_pipe_crc_source *source, u32 > *val) > { > if (IS_GEN(dev_priv, 2)) > return i8xx_pipe_crc_ctl_reg(source, val); > @@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct > drm_i915_private *dev_priv, > else if (IS_GEN_RANGE(dev_priv, 5, 6)) > return ilk_pipe_crc_ctl_reg(source, val); > else if (INTEL_GEN(dev_priv) < 9) > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, > val, set_wa); > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, > val); > else > return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, > val); > } > @@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc > *crtc, const char *source_name) > intel_wakeref_t wakeref; > u32 val = 0; /* shut up gcc */ > int ret = 0; > + bool enable; > > if (display_crc_ctl_parse_source(source_name, &source) < 0) { > DRM_DEBUG_DRIVER("unknown source %s\n", source_name); > @@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc > *crtc, const char *source_name) > return -EIO; > } > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, > true); > + enable = source != INTEL_PIPE_CRC_SOURCE_NONE; > + if (enable) > + intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), > enable); I personally, find it easier to read when we pass true/false, it's just clear that we have already worked out what do when enable is true. intel_crtc_crc_setup_workaround(to_intel_crtc(crtc), true); > + > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, > &val); > if (ret != 0) > goto out; > > @@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc > *crtc, const char *source_name) > if (!source) { > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > vlv_undo_pipe_scramble_reset(dev_priv, crtc- > >index); > - else if ((IS_HASWELL(dev_priv) || > - IS_BROADWELL(dev_priv)) && crtc->index == > PIPE_A) > - hsw_pipe_A_crc_wa(dev_priv, false); > } > > pipe_crc->skipped = 0; > > out: > + if (!enable) > + intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), > enable); > + > intel_display_power_put(dev_priv, power_domain, wakeref); > > return ret; > @@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc > *intel_crtc) > if (!crtc->crc.opened) > return; > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc- > >source, &val, false) < 0) > + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc- > >source, &val) < 0) Now that 'ips_force_disable' is 'crc_enabled', I guess we should set it here for the sake of completeness. Anyway, doing that doesn't have any functional benefit since we weren't doing it earlier. Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > return; > > /* Don't need pipe_crc->lock here, IRQs are not generated. */
On Thu, 2019-03-07 at 12:18 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote: > > Other features like PSR2 also needs to be disabled while getting > > CRC > > so lets rename ips_force_disable to crc_enabled, drop all this > > checks > > for pipe A and HSW and BDW and make it generic and > > hsw_compute_ips_config() will take care of all the checks removed > > from here. > > > > v2: Renaming and parameter changes to the functions that prepares > > the > > commit (Ville) > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 10 ++++-- > > drivers/gpu/drm/i915/intel_drv.h | 3 +- > > drivers/gpu/drm/i915/intel_pipe_crc.c | 47 +++++++++++------------ > > ---- > > 3 files changed, 29 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 9312b3f35eb0..b3a5d8462251 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct > > intel_crtc_state *crtc_state) > > if (!hsw_crtc_state_ips_capable(crtc_state)) > > return false; > > > > - if (crtc_state->ips_force_disable) > > + /* > > + * When IPS gets enabled, the pipe CRC changes. Since IPS gets > > + * enabled and disabled dynamically based on package C states, > > + * user space can't make reliable use of the CRCs, so let's > > just > > + * completely disable it. > > + */ > > + if (crtc_state->crc_enabled) > > return false; > > > > /* IPS should be fine as long as at least one plane is enabled. > > */ > > @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct > > intel_crtc_state *crtc_state) > > saved_state->shared_dpll = crtc_state->shared_dpll; > > saved_state->dpll_hw_state = crtc_state->dpll_hw_state; > > saved_state->pch_pfit.force_thru = crtc_state- > > > pch_pfit.force_thru; > > - saved_state->ips_force_disable = crtc_state->ips_force_disable; > > + saved_state->crc_enabled = crtc_state->crc_enabled; > > if (IS_G4X(dev_priv) || > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > saved_state->wm = crtc_state->wm; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 5412373e2f98..2be64529e4a2 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -999,7 +999,8 @@ struct intel_crtc_state { > > struct intel_link_m_n fdi_m_n; > > > > bool ips_enabled; > > - bool ips_force_disable; > > + > > + bool crc_enabled; > > > > bool enable_fbc; > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c > > b/drivers/gpu/drm/i915/intel_pipe_crc.c > > index 53d4ec68d3c4..af64597c5c6e 100644 > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum > > intel_pipe_crc_source *source, > > return 0; > > } > > > > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > > - bool enable) > > +static void > > +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool > > enable) > > { > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > struct drm_device *dev = &dev_priv->drm; > We could eliminate this local. Done > > > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, > > PIPE_A); > > struct intel_crtc_state *pipe_config; > > struct drm_atomic_state *state; > > struct drm_modeset_acquire_ctx ctx; > > - int ret = 0; > > + int ret; > > > > drm_modeset_acquire_init(&ctx, 0); > > > > @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct > > drm_i915_private *dev_priv, > > goto put_state; > > } > > > > - if (HAS_IPS(dev_priv)) { > > - /* > > - * When IPS gets enabled, the pipe CRC changes. Since > > IPS gets > > - * enabled and disabled dynamically based on package C > > states, > > - * user space can't make reliable use of the CRCs, so > > let's just > > - * completely disable it. > > - */ > > - pipe_config->ips_force_disable = enable; > > - } > > + pipe_config->crc_enabled = enable; > > > > - if (IS_HASWELL(dev_priv)) { > > + if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) { > > pipe_config->pch_pfit.force_thru = enable; > > if (pipe_config->cpu_transcoder == TRANSCODER_EDP && > > pipe_config->pch_pfit.enabled != enable) > > @@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct > > drm_i915_private *dev_priv, > > static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv, > > enum pipe pipe, > > enum intel_pipe_crc_source *source, > > - u32 *val, > > - bool set_wa) > > + u32 *val) > > { > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) > > *source = INTEL_PIPE_CRC_SOURCE_PIPE; > > @@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct > > drm_i915_private *dev_priv, > > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB; > > break; > > case INTEL_PIPE_CRC_SOURCE_PIPE: > > - if (set_wa && (IS_HASWELL(dev_priv) || > > - IS_BROADWELL(dev_priv)) && pipe == PIPE_A) > > - hsw_pipe_A_crc_wa(dev_priv, true); > > - > > *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB; > > break; > > case INTEL_PIPE_CRC_SOURCE_NONE: > > @@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct > > drm_i915_private *dev_priv, > > > > static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv, > > enum pipe pipe, > > - enum intel_pipe_crc_source *source, u32 > > *val, > > - bool set_wa) > > + enum intel_pipe_crc_source *source, u32 > > *val) > > { > > if (IS_GEN(dev_priv, 2)) > > return i8xx_pipe_crc_ctl_reg(source, val); > > @@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct > > drm_i915_private *dev_priv, > > else if (IS_GEN_RANGE(dev_priv, 5, 6)) > > return ilk_pipe_crc_ctl_reg(source, val); > > else if (INTEL_GEN(dev_priv) < 9) > > - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, > > val, set_wa); > > + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, > > val); > > else > > return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, > > val); > > } > > @@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc > > *crtc, const char *source_name) > > intel_wakeref_t wakeref; > > u32 val = 0; /* shut up gcc */ > > int ret = 0; > > + bool enable; > > > > if (display_crc_ctl_parse_source(source_name, &source) < 0) { > > DRM_DEBUG_DRIVER("unknown source %s\n", source_name); > > @@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc > > *crtc, const char *source_name) > > return -EIO; > > } > > > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, > > true); > > + enable = source != INTEL_PIPE_CRC_SOURCE_NONE; > > + if (enable) > > + intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), > > enable); > I personally, find it easier to read when we pass true/false, it's > just > clear that we have already worked out what do when enable is true. > intel_crtc_crc_setup_workaround(to_intel_crtc(crtc), true); Done > > > > + > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, > > &val); > > if (ret != 0) > > goto out; > > > > @@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc > > *crtc, const char *source_name) > > if (!source) { > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > vlv_undo_pipe_scramble_reset(dev_priv, crtc- > > > index); > > - else if ((IS_HASWELL(dev_priv) || > > - IS_BROADWELL(dev_priv)) && crtc->index == > > PIPE_A) > > - hsw_pipe_A_crc_wa(dev_priv, false); > > } > > > > pipe_crc->skipped = 0; > > > > out: > > + if (!enable) > > + intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), > > enable); > > + > > intel_display_power_put(dev_priv, power_domain, wakeref); > > > > return ret; > > @@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct > > intel_crtc > > *intel_crtc) > > if (!crtc->crc.opened) > > return; > > > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc- > > > source, &val, false) < 0) > > + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc- > > > source, &val) < 0) > > Now that 'ips_force_disable' is 'crc_enabled', I guess we should set > it > here for the sake of completeness. Anyway, doing that doesn't have > any > functional benefit since we weren't doing it earlier. This is only executed when doing a full modeset with CRC alread enabled, so crc_enabled will be kept set and the workarounds will be applied again, that is why we don't need to call it here. > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > return; > > > > /* Don't need pipe_crc->lock here, IRQs are not generated. */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9312b3f35eb0..b3a5d8462251 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *crtc_state) if (!hsw_crtc_state_ips_capable(crtc_state)) return false; - if (crtc_state->ips_force_disable) + /* + * When IPS gets enabled, the pipe CRC changes. Since IPS gets + * enabled and disabled dynamically based on package C states, + * user space can't make reliable use of the CRCs, so let's just + * completely disable it. + */ + if (crtc_state->crc_enabled) return false; /* IPS should be fine as long as at least one plane is enabled. */ @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) saved_state->shared_dpll = crtc_state->shared_dpll; saved_state->dpll_hw_state = crtc_state->dpll_hw_state; saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru; - saved_state->ips_force_disable = crtc_state->ips_force_disable; + saved_state->crc_enabled = crtc_state->crc_enabled; if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) saved_state->wm = crtc_state->wm; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 5412373e2f98..2be64529e4a2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -999,7 +999,8 @@ struct intel_crtc_state { struct intel_link_m_n fdi_m_n; bool ips_enabled; - bool ips_force_disable; + + bool crc_enabled; bool enable_fbc; diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c index 53d4ec68d3c4..af64597c5c6e 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source, return 0; } -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, - bool enable) +static void +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable) { + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A); struct intel_crtc_state *pipe_config; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; - int ret = 0; + int ret; drm_modeset_acquire_init(&ctx, 0); @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, goto put_state; } - if (HAS_IPS(dev_priv)) { - /* - * When IPS gets enabled, the pipe CRC changes. Since IPS gets - * enabled and disabled dynamically based on package C states, - * user space can't make reliable use of the CRCs, so let's just - * completely disable it. - */ - pipe_config->ips_force_disable = enable; - } + pipe_config->crc_enabled = enable; - if (IS_HASWELL(dev_priv)) { + if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) { pipe_config->pch_pfit.force_thru = enable; if (pipe_config->cpu_transcoder == TRANSCODER_EDP && pipe_config->pch_pfit.enabled != enable) @@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv, enum pipe pipe, enum intel_pipe_crc_source *source, - u32 *val, - bool set_wa) + u32 *val) { if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) *source = INTEL_PIPE_CRC_SOURCE_PIPE; @@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv, *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB; break; case INTEL_PIPE_CRC_SOURCE_PIPE: - if (set_wa && (IS_HASWELL(dev_priv) || - IS_BROADWELL(dev_priv)) && pipe == PIPE_A) - hsw_pipe_A_crc_wa(dev_priv, true); - *val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB; break; case INTEL_PIPE_CRC_SOURCE_NONE: @@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv, static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv, enum pipe pipe, - enum intel_pipe_crc_source *source, u32 *val, - bool set_wa) + enum intel_pipe_crc_source *source, u32 *val) { if (IS_GEN(dev_priv, 2)) return i8xx_pipe_crc_ctl_reg(source, val); @@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv, else if (IS_GEN_RANGE(dev_priv, 5, 6)) return ilk_pipe_crc_ctl_reg(source, val); else if (INTEL_GEN(dev_priv) < 9) - return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa); + return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val); else return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val); } @@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) intel_wakeref_t wakeref; u32 val = 0; /* shut up gcc */ int ret = 0; + bool enable; if (display_crc_ctl_parse_source(source_name, &source) < 0) { DRM_DEBUG_DRIVER("unknown source %s\n", source_name); @@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) return -EIO; } - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true); + enable = source != INTEL_PIPE_CRC_SOURCE_NONE; + if (enable) + intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), enable); + + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val); if (ret != 0) goto out; @@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) if (!source) { if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) vlv_undo_pipe_scramble_reset(dev_priv, crtc->index); - else if ((IS_HASWELL(dev_priv) || - IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A) - hsw_pipe_A_crc_wa(dev_priv, false); } pipe_crc->skipped = 0; out: + if (!enable) + intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), enable); + intel_display_power_put(dev_priv, power_domain, wakeref); return ret; @@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc) if (!crtc->crc.opened) return; - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0) + if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0) return; /* Don't need pipe_crc->lock here, IRQs are not generated. */