[Intel-gfx] [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Jan 16 18:02:25 UTC 2019
On Wed, Jan 16, 2019 at 09:38:59AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:17PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > The LUTs are single buffered so we should program them after
> > the double buffered pipe updates have been latched by the
> > hardware.
> >
> > We'll also fix up the IPS vs. split gamma w/a to do the IPS
> > disable like everyone else. Note that this is currently dead
> > code as we don't use the split gamma mode on HSW, but that
> > will be fixed up shortly.
>
> I don't think this is quite dead code...we don't use split gamma
> ourselves, but we could potentially inherit that setup from the BIOS
> (which will stick around until it eventually gets clobbered by the first
> modeset/fastset).
>
> Uma's series added some logic to sanitize the LUT's immediately on boot,
> but that hasn't landed yet.
>
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_color.c | 25 +--------------
> > drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++----
> > 2 files changed, 42 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index f9e0855162f3..0c0da7ed0fd7 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -361,29 +361,6 @@ static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
> > ilk_load_csc_matrix(crtc_state);
> > }
> >
> > -/* Loads the legacy palette/gamma unit for the CRTC on Haswell. */
> > -static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
> > -{
> > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > - bool reenable_ips = false;
> > -
> > - /*
> > - * Workaround : Do not read or write the pipe palette/gamma data while
> > - * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > - */
> > - if (IS_HASWELL(dev_priv) && crtc_state->ips_enabled &&
> > - (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> > - hsw_disable_ips(crtc_state);
> > - reenable_ips = true;
> > - }
> > -
> > - i9xx_load_luts(crtc_state);
> > -
> > - if (reenable_ips)
> > - hsw_enable_ips(crtc_state);
> > -}
> > -
> > static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > @@ -660,7 +637,7 @@ void intel_color_init(struct intel_crtc *crtc)
> > if (IS_CHERRYVIEW(dev_priv)) {
> > dev_priv->display.load_luts = cherryview_load_luts;
> > } else if (IS_HASWELL(dev_priv)) {
> > - dev_priv->display.load_luts = haswell_load_luts;
> > + dev_priv->display.load_luts = i9xx_load_luts;
> > dev_priv->display.color_commit = hsw_color_commit;
> > } else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> > IS_BROXTON(dev_priv)) {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 96c78566b8e6..1caee4128974 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5299,24 +5299,54 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> > static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
> > const struct intel_crtc_state *new_crtc_state)
> > {
> > + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > if (!old_crtc_state->ips_enabled)
> > return false;
> >
> > if (needs_modeset(&new_crtc_state->base))
> > return true;
> >
> > + /*
> > + * Workaround : Do not read or write the pipe palette/gamma data while
> > + * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > + *
> > + * Disable IPS before we program the LUT.
> > + */
> > + if (IS_HASWELL(dev_priv) &&
> > + (new_crtc_state->base.color_mgmt_changed ||
> > + new_crtc_state->update_pipe) &&
> > + new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
>
> Wouldn't we want old_crtc_state for the gamma_mode test? We need to
> disable IPS if we're already in split gamma mode (inherited from BIOS),
> regardless of whether we're moving to non-split gamma.
We're going to update the gamma mode before programming the LUT.
But I think I'll probably change this to disable IPS around all LUT
updates to prevent IPS from using the old LUT during the vblank. That's
assuming IPS does actually prefill during vblank (which would make sense
to me).
>
>
> Matt
>
> > + return true;
> > +
> > return !new_crtc_state->ips_enabled;
> > }
> >
> > static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
> > const struct intel_crtc_state *new_crtc_state)
> > {
> > + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > if (!new_crtc_state->ips_enabled)
> > return false;
> >
> > if (needs_modeset(&new_crtc_state->base))
> > return true;
> >
> > + /*
> > + * Workaround : Do not read or write the pipe palette/gamma data while
> > + * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > + *
> > + * Re-enable IPS after the LUT has been programmed.
> > + */
> > + if (IS_HASWELL(dev_priv) &&
> > + (new_crtc_state->base.color_mgmt_changed ||
> > + new_crtc_state->update_pipe) &&
> > + new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
> > + return true;
> > +
> > /*
> > * We can't read out IPS on broadwell, assume the worst and
> > * forcibly enable IPS on the first fastset.
> > @@ -11050,7 +11080,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > return ret;
> > }
> >
> > - if (crtc_state->color_mgmt_changed) {
> > + if (mode_changed || crtc_state->color_mgmt_changed) {
> > ret = intel_color_check(pipe_config);
> > if (ret)
> > return ret;
> > @@ -13117,6 +13147,16 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> > */
> > drm_atomic_helper_wait_for_flip_done(dev, state);
> >
> > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > + new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> > +
> > + if (new_crtc_state->active &&
> > + !needs_modeset(new_crtc_state) &&
> > + (new_intel_crtc_state->base.color_mgmt_changed ||
> > + new_intel_crtc_state->update_pipe))
> > + intel_color_load_luts(new_intel_crtc_state);
> > + }
> > +
> > /*
> > * Now that the vblank has passed, we can go ahead and program the
> > * optimal watermarks on platforms that need two-step watermark
> > @@ -13632,11 +13672,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> > intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
> > bool modeset = needs_modeset(&intel_cstate->base);
> >
> > - if (!modeset &&
> > - (intel_cstate->base.color_mgmt_changed ||
> > - intel_cstate->update_pipe))
> > - intel_color_load_luts(intel_cstate);
> > -
> > /* Perform vblank evasion around commit operation */
> > intel_pipe_update_start(intel_cstate);
> >
> > --
> > 2.19.2
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list