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@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > 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 | 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@xxxxxxxxx>
> > return;
> >
> > /* Don't need pipe_crc->lock here, IRQs are not generated. */
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
