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ä <[email protected]> > > > > 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ä <[email protected]> > > --- > > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
