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@xxxxxxxxx>
> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > ---
> > > > 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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
