| Message ID | 20190228013259.30026-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] drm/i915/psr: Remove PSR2 FIXME | expand |
On Wed, Feb 27, 2019 at 05:32:57PM -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. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++------------------ > 3 files changed, 24 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 816e8f124b3b..328967c642b3 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; Hmm. I was wondering how we even manage to pass the state checker with the current code. But apparently we don't have state checking for IPS. I would suggest moving this into hsw_compute_ips_config() and then adding the state checker (for HSW only though since BDW can't do the readout). > > /* IPS should be fine as long as at least one plane is enabled. */ > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc, Just pass in the intel_crtc > + bool enable) > { > struct drm_device *dev = &dev_priv->drm; > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); and then we don't have to have an ugly name for this. Also pasing in dev_priv is redundant when you're already passing in the crtc. The function name isn't super descriptive. It makes me think we're preparing for CRC capture, when in fact it just adds/removes the w/as. > struct intel_crtc_state *pipe_config; > struct drm_atomic_state *state; > struct drm_modeset_acquire_ctx ctx; > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, > state->acquire_ctx = &ctx; > > retry: > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > if (IS_ERR(pipe_config)) { > ret = PTR_ERR(pipe_config); > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > } > @@ -618,7 +605,9 @@ 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); > + intel_crtc_crc_prepare(dev_priv, crtc, source != INTEL_PIPE_CRC_SOURCE_NONE); > + Aren't we potentially corrupting the last CRC(s) by turning off the w/as before disbling CRC capture? > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val); > if (ret != 0) > goto out; > > @@ -629,9 +618,6 @@ 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; > @@ -652,7 +638,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. */ > -- > 2.21.0
On Thu, Feb 28, 2019 at 06:56:48PM +0200, Ville Syrjälä wrote: > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++------------------ > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 816e8f124b3b..328967c642b3 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; > > Hmm. I was wondering how we even manage to pass the state checker with > the current code. But apparently we don't have state checking for IPS. > I would suggest moving this into hsw_compute_ips_config() and then > adding the state checker (for HSW only though since BDW can't do the > readout). And of course this _is_ hsw_compute_ips_config(). Not sure what code I was reading.
On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote: > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++------------ > > ------ > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 816e8f124b3b..328967c642b3 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; > > Hmm. I was wondering how we even manage to pass the state checker > with > the current code. But apparently we don't have state checking for > IPS. > I would suggest moving this into hsw_compute_ips_config() and then > adding the state checker (for HSW only though since BDW can't do the > readout). > > > > > /* IPS should be fine as long as at least one plane is enabled. > > */ > > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, struct > > drm_crtc *crtc, > > Just pass in the intel_crtc Okay > > > + bool enable) > > { > > struct drm_device *dev = &dev_priv->drm; > > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, > > PIPE_A); > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > and then we don't have to have an ugly name for this. > > Also pasing in dev_priv is redundant when you're already passing in > the > crtc. > okay > The function name isn't super descriptive. It makes me think we're > preparing for CRC capture, when in fact it just adds/removes the > w/as. What about: intel_crtc_crc_workarounds_setup()? > > > struct intel_crtc_state *pipe_config; > > struct drm_atomic_state *state; > > struct drm_modeset_acquire_ctx ctx; > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > drm_i915_private *dev_priv, > > state->acquire_ctx = &ctx; > > > > retry: > > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > if (IS_ERR(pipe_config)) { > > ret = PTR_ERR(pipe_config); > > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > > } > > @@ -618,7 +605,9 @@ 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); > > + intel_crtc_crc_prepare(dev_priv, crtc, source != > > INTEL_PIPE_CRC_SOURCE_NONE); > > + > > Aren't we potentially corrupting the last CRC(s) by turning off the > w/as > before disbling CRC capture? It will only be NULL when uses closes the file descriptor to CRC data, so even if its corrupted user will never get those CRCs. > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, > > &val); > > if (ret != 0) > > goto out; > > > > @@ -629,9 +618,6 @@ 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; > > @@ -652,7 +638,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. */ > > -- > > 2.21.0
On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote: > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote: > > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++------------ > > > ------ > > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 816e8f124b3b..328967c642b3 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; > > > > Hmm. I was wondering how we even manage to pass the state checker > > with > > the current code. But apparently we don't have state checking for > > IPS. > > I would suggest moving this into hsw_compute_ips_config() and then > > adding the state checker (for HSW only though since BDW can't do > > the > > readout). > > > > > > > > /* IPS should be fine as long as at least one plane is enabled. > > > */ > > > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, struct > > > drm_crtc *crtc, > > > > Just pass in the intel_crtc > > Okay > > > > > > + bool enable) > > > { > > > struct drm_device *dev = &dev_priv->drm; > > > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, > > > PIPE_A); > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > and then we don't have to have an ugly name for this. > > > > Also pasing in dev_priv is redundant when you're already passing in > > the > > crtc. > > > > okay > > > The function name isn't super descriptive. It makes me think we're > > preparing for CRC capture, when in fact it just adds/removes the > > w/as. > > What about: intel_crtc_crc_workarounds_setup()? > > > > > > struct intel_crtc_state *pipe_config; > > > struct drm_atomic_state *state; > > > struct drm_modeset_acquire_ctx ctx; > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > > drm_i915_private *dev_priv, > > > state->acquire_ctx = &ctx; > > > > > > retry: > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > > if (IS_ERR(pipe_config)) { > > > ret = PTR_ERR(pipe_config); > > > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > > > } > > > @@ -618,7 +605,9 @@ 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); > > > + intel_crtc_crc_prepare(dev_priv, crtc, source != > > > INTEL_PIPE_CRC_SOURCE_NONE); > > > + > > > > Aren't we potentially corrupting the last CRC(s) by turning off the > > w/as > > before disbling CRC capture? > > It will only be NULL when uses closes the file descriptor to CRC > data, > so even if its corrupted user will never get those CRCs. Even if the userspace reads from another thread? Probably not a common use case though. > > > > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, > > > &val); > > > if (ret != 0) > > > goto out; > > > > > > @@ -629,9 +618,6 @@ 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; > > > @@ -652,7 +638,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. */ > > > -- > > > 2.21.0
On Thu, 2019-02-28 at 17:06 -0800, Dhinakaran Pandiyan wrote: > On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote: > > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote: > > > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++-------- > > > > ---- > > > > ------ > > > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 816e8f124b3b..328967c642b3 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; > > > > > > Hmm. I was wondering how we even manage to pass the state checker > > > with > > > the current code. But apparently we don't have state checking for > > > IPS. > > > I would suggest moving this into hsw_compute_ips_config() and > > > then > > > adding the state checker (for HSW only though since BDW can't do > > > the > > > readout). > > > > > > > > > > > /* IPS should be fine as long as at least one plane is > > > > enabled. > > > > */ > > > > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, > > > > struct > > > > drm_crtc *crtc, > > > > > > Just pass in the intel_crtc > > > > Okay > > > > > > + bool enable) > > > > { > > > > struct drm_device *dev = &dev_priv->drm; > > > > - struct intel_crtc *crtc = > > > > intel_get_crtc_for_pipe(dev_priv, > > > > PIPE_A); > > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > > > and then we don't have to have an ugly name for this. > > > > > > Also pasing in dev_priv is redundant when you're already passing > > > in > > > the > > > crtc. > > > > > > > okay > > > > > The function name isn't super descriptive. It makes me think > > > we're > > > preparing for CRC capture, when in fact it just adds/removes the > > > w/as. > > > > What about: intel_crtc_crc_workarounds_setup()? > > > > > > struct intel_crtc_state *pipe_config; > > > > struct drm_atomic_state *state; > > > > struct drm_modeset_acquire_ctx ctx; > > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > > > drm_i915_private *dev_priv, > > > > state->acquire_ctx = &ctx; > > > > > > > > retry: > > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > > + pipe_config = intel_atomic_get_crtc_state(state, > > > > intel_crtc); > > > > if (IS_ERR(pipe_config)) { > > > > ret = PTR_ERR(pipe_config); > > > > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > > > > } > > > > @@ -618,7 +605,9 @@ 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); > > > > + intel_crtc_crc_prepare(dev_priv, crtc, source != > > > > INTEL_PIPE_CRC_SOURCE_NONE); > > > > + > > > > > > Aren't we potentially corrupting the last CRC(s) by turning off > > > the > > > w/as > > > before disbling CRC capture? > > > > It will only be NULL when uses closes the file descriptor to CRC > > data, > > so even if its corrupted user will never get those CRCs. > Even if the userspace reads from another thread? Probably not a > common > use case though. It is called with NULL from the release() hook, so when all filedescriptors to this file is closed. But anyways the drm crc code only allows one open() of this file at time, so if other thread tries to read it after the main thread have closed it it will fail before get into any drm code. > > > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, > > > > &source, > > > > &val); > > > > if (ret != 0) > > > > goto out; > > > > > > > > @@ -629,9 +618,6 @@ 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; > > > > @@ -652,7 +638,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. */ > > > > -- > > > > 2.21.0
On Thu, Feb 28, 2019 at 11:26:57PM +0000, Souza, Jose wrote: > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote: > > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++------------ > > > ------ > > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 816e8f124b3b..328967c642b3 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; > > > > Hmm. I was wondering how we even manage to pass the state checker > > with > > the current code. But apparently we don't have state checking for > > IPS. > > I would suggest moving this into hsw_compute_ips_config() and then > > adding the state checker (for HSW only though since BDW can't do the > > readout). > > > > > > > > /* IPS should be fine as long as at least one plane is enabled. > > > */ > > > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, struct > > > drm_crtc *crtc, > > > > Just pass in the intel_crtc > > Okay > > > > > > + bool enable) > > > { > > > struct drm_device *dev = &dev_priv->drm; > > > - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, > > > PIPE_A); > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > and then we don't have to have an ugly name for this. > > > > Also pasing in dev_priv is redundant when you're already passing in > > the > > crtc. > > > > okay > > > The function name isn't super descriptive. It makes me think we're > > preparing for CRC capture, when in fact it just adds/removes the > > w/as. > > What about: intel_crtc_crc_workarounds_setup()? Or _setup_workarounds() so that it reads more naturally? > > > > > > struct intel_crtc_state *pipe_config; > > > struct drm_atomic_state *state; > > > struct drm_modeset_acquire_ctx ctx; > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > > drm_i915_private *dev_priv, > > > state->acquire_ctx = &ctx; > > > > > > retry: > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); > > > if (IS_ERR(pipe_config)) { > > > ret = PTR_ERR(pipe_config); > > > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > > > } > > > @@ -618,7 +605,9 @@ 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); > > > + intel_crtc_crc_prepare(dev_priv, crtc, source != > > > INTEL_PIPE_CRC_SOURCE_NONE); > > > + > > > > Aren't we potentially corrupting the last CRC(s) by turning off the > > w/as > > before disbling CRC capture? > > It will only be NULL when uses closes the file descriptor to CRC data, > so even if its corrupted user will never get those CRCs. Userspace may not get it but the tracepoint will still see it. Which may cause confusion. > > > > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, > > > &val); > > > if (ret != 0) > > > goto out; > > > > > > @@ -629,9 +618,6 @@ 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; > > > @@ -652,7 +638,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. */ > > > -- > > > 2.21.0
On Fri, 2019-03-01 at 15:35 +0200, Ville Syrjälä wrote: > On Thu, Feb 28, 2019 at 11:26:57PM +0000, Souza, Jose wrote: > > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote: > > > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++-------- > > > > ---- > > > > ------ > > > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 816e8f124b3b..328967c642b3 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; > > > > > > Hmm. I was wondering how we even manage to pass the state checker > > > with > > > the current code. But apparently we don't have state checking for > > > IPS. > > > I would suggest moving this into hsw_compute_ips_config() and > > > then > > > adding the state checker (for HSW only though since BDW can't do > > > the > > > readout). > > > > > > > > > > > /* IPS should be fine as long as at least one plane is > > > > enabled. > > > > */ > > > > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, > > > > struct > > > > drm_crtc *crtc, > > > > > > Just pass in the intel_crtc > > > > Okay > > > > > > + bool enable) > > > > { > > > > struct drm_device *dev = &dev_priv->drm; > > > > - struct intel_crtc *crtc = > > > > intel_get_crtc_for_pipe(dev_priv, > > > > PIPE_A); > > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > > > and then we don't have to have an ugly name for this. > > > > > > Also pasing in dev_priv is redundant when you're already passing > > > in > > > the > > > crtc. > > > > > > > okay > > > > > The function name isn't super descriptive. It makes me think > > > we're > > > preparing for CRC capture, when in fact it just adds/removes the > > > w/as. > > > > What about: intel_crtc_crc_workarounds_setup()? > > Or _setup_workarounds() so that it reads more naturally? > > > > > struct intel_crtc_state *pipe_config; > > > > struct drm_atomic_state *state; > > > > struct drm_modeset_acquire_ctx ctx; > > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > > > drm_i915_private *dev_priv, > > > > state->acquire_ctx = &ctx; > > > > > > > > retry: > > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > > + pipe_config = intel_atomic_get_crtc_state(state, > > > > intel_crtc); > > > > if (IS_ERR(pipe_config)) { > > > > ret = PTR_ERR(pipe_config); > > > > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > > > > } > > > > @@ -618,7 +605,9 @@ 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); > > > > + intel_crtc_crc_prepare(dev_priv, crtc, source != > > > > INTEL_PIPE_CRC_SOURCE_NONE); > > > > + > > > > > > Aren't we potentially corrupting the last CRC(s) by turning off > > > the > > > w/as > > > before disbling CRC capture? > > > > It will only be NULL when uses closes the file descriptor to CRC > > data, > > so even if its corrupted user will never get those CRCs. > > Userspace may not get it but the tracepoint will still see it. Which > may > cause confusion. Okay changing to: if (source != INTEL_PIPE_CRC_SOURCE_NONE) intel_crtc_crc_setup_workarounds() ret = get_new_crc_ctl_reg() ... if (source == INTEL_PIPE_CRC_SOURCE_NONE) intel_crtc_crc_setup_workarounds() > > > > > + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, > > > > &source, > > > > &val); > > > > if (ret != 0) > > > > goto out; > > > > > > > > @@ -629,9 +618,6 @@ 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; > > > > @@ -652,7 +638,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. */ > > > > -- > > > > 2.21.0 > >
On Thu, 2019-02-28 at 17:14 -0800, Souza, Jose wrote: > On Thu, 2019-02-28 at 17:06 -0800, Dhinakaran Pandiyan wrote: > > On Thu, 2019-02-28 at 15:26 -0800, Souza, Jose wrote: > > > On Thu, 2019-02-28 at 18:56 +0200, Ville Syrjälä wrote: > > > > On Wed, Feb 27, 2019 at 05:32:57PM -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. > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++-------- > > > > > ---- > > > > > ------ > > > > > 3 files changed, 24 insertions(+), 31 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index 816e8f124b3b..328967c642b3 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; > > > > > > > > Hmm. I was wondering how we even manage to pass the state > > > > checker > > > > with > > > > the current code. But apparently we don't have state checking > > > > for > > > > IPS. > > > > I would suggest moving this into hsw_compute_ips_config() and > > > > then > > > > adding the state checker (for HSW only though since BDW can't > > > > do > > > > the > > > > readout). > > > > > > > > > > > > > > /* IPS should be fine as long as at least one plane is > > > > > enabled. > > > > > */ > > > > > @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c > > > > > @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, > > > > > struct > > > > > drm_crtc *crtc, > > > > > > > > Just pass in the intel_crtc > > > > > > Okay > > > > > > > > + bool enable) > > > > > { > > > > > struct drm_device *dev = &dev_priv->drm; > > > > > - struct intel_crtc *crtc = > > > > > intel_get_crtc_for_pipe(dev_priv, > > > > > PIPE_A); > > > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > > > > > and then we don't have to have an ugly name for this. > > > > > > > > Also pasing in dev_priv is redundant when you're already > > > > passing > > > > in > > > > the > > > > crtc. > > > > > > > > > > okay > > > > > > > The function name isn't super descriptive. It makes me think > > > > we're > > > > preparing for CRC capture, when in fact it just adds/removes > > > > the > > > > w/as. > > > > > > What about: intel_crtc_crc_workarounds_setup()? > > > > > > > > struct intel_crtc_state *pipe_config; > > > > > struct drm_atomic_state *state; > > > > > struct drm_modeset_acquire_ctx ctx; > > > > > @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct > > > > > drm_i915_private *dev_priv, > > > > > state->acquire_ctx = &ctx; > > > > > > > > > > retry: > > > > > - pipe_config = intel_atomic_get_crtc_state(state, crtc); > > > > > + pipe_config = intel_atomic_get_crtc_state(state, > > > > > intel_crtc); > > > > > if (IS_ERR(pipe_config)) { > > > > > ret = PTR_ERR(pipe_config); > > > > > 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) && intel_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 +336,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 +349,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 +406,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 +417,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); > > > > > } > > > > > @@ -618,7 +605,9 @@ 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); > > > > > + intel_crtc_crc_prepare(dev_priv, crtc, source != > > > > > INTEL_PIPE_CRC_SOURCE_NONE); > > > > > + > > > > > > > > Aren't we potentially corrupting the last CRC(s) by turning off > > > > the > > > > w/as > > > > before disbling CRC capture? > > > > > > It will only be NULL when uses closes the file descriptor to CRC > > > data, > > > so even if its corrupted user will never get those CRCs. > > > > Even if the userspace reads from another thread? Probably not a > > common > > use case though. > > It is called with NULL from the release() hook, so when all > filedescriptors to this file is closed. > > But anyways the drm crc code only allows one open() of this file at > time, so if other thread tries to read it after the main thread have > closed it it will fail before get into any drm code. > Makes sense, thanks.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 816e8f124b3b..328967c642b3 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. */ @@ -11684,7 +11690,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..f6d0b2aaffe2 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -280,11 +280,12 @@ 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_prepare(struct drm_i915_private *dev_priv, struct drm_crtc *crtc, + bool enable) { struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A); + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_crtc_state *pipe_config; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; @@ -301,23 +302,15 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv, state->acquire_ctx = &ctx; retry: - pipe_config = intel_atomic_get_crtc_state(state, crtc); + pipe_config = intel_atomic_get_crtc_state(state, intel_crtc); if (IS_ERR(pipe_config)) { ret = PTR_ERR(pipe_config); 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) && intel_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 +336,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 +349,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 +406,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 +417,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); } @@ -618,7 +605,9 @@ 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); + intel_crtc_crc_prepare(dev_priv, crtc, source != INTEL_PIPE_CRC_SOURCE_NONE); + + ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val); if (ret != 0) goto out; @@ -629,9 +618,6 @@ 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; @@ -652,7 +638,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. */
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. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.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 | 42 +++++++++------------------ 3 files changed, 24 insertions(+), 31 deletions(-)