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@xxxxxxxxxxxxxxx>
> >
> > 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.
With the gamma_enable/csc_enable flags properly tracked we shouldn't
need anything like that. Or am I missing something?
>
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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.
>
>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
