[Intel-gfx] [PATCH 06/13] drm/i915: Split color mgmt based on single vs. double buffered registers
Matt Roper
matthew.d.roper at intel.com
Tue Jan 15 00:56:31 UTC 2019
On Fri, Jan 11, 2019 at 07:08:16PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Split the color managemnt hooks along the single vs. double
> buffered registers line. Of the currently progammed registers
> GAMMA_MODE and the ilk+ pipe CSC are double buffered, the
> LUTS and CHV CGM block are single buffered.
>
> The double buffered register will be programmed during the
> normal pipe update with evasion, and also during pipe enable
> so that the settings will already be correct when the pipe
> starts up before the planes are enabled.
>
> The single buffered registers are currently programmed before
> the vblank evade. Which is totally wrong, but we'll correct
> that later.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_color.c | 49 +++++++++++++---------------
> drivers/gpu/drm/i915/intel_display.c | 16 +++++----
> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> 4 files changed, 34 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7182a580002c..354858b2019b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -320,7 +320,7 @@ struct drm_i915_display_funcs {
> /* display clock increase/decrease */
> /* pll clock increase/decrease */
>
> - void (*load_csc_matrix)(const struct intel_crtc_state *crtc_state);
> + void (*color_commit)(const struct intel_crtc_state *crtc_state);
> void (*load_luts)(const struct intel_crtc_state *crtc_state);
Logic-wise this patch looks good, but we should probably add some
kerneldoc to these to make it clear that color_commit() is programming
anything that's expected to take effect at the vblank, and load_luts()
takes effect immediately when called.
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index df3567686c45..f9e0855162f3 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -304,14 +304,6 @@ static void cherryview_load_csc_matrix(const struct intel_crtc_state *crtc_state
> I915_WRITE(CGM_PIPE_MODE(pipe), mode);
> }
>
> -void intel_color_set_csc(const struct intel_crtc_state *crtc_state)
> -{
> - struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> -
> - if (dev_priv->display.load_csc_matrix)
> - dev_priv->display.load_csc_matrix(crtc_state);
> -}
> -
> /* Loads the legacy palette/gamma unit for the CRTC. */
> static void i9xx_load_luts_internal(const struct intel_crtc_state *crtc_state,
> const struct drm_property_blob *blob)
> @@ -359,6 +351,16 @@ static void i9xx_load_luts(const struct intel_crtc_state *crtc_state)
> i9xx_load_luts_internal(crtc_state, crtc_state->base.gamma_lut);
> }
>
> +static void hsw_color_commit(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);
> +
> + I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
> +
> + 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)
> {
> @@ -376,8 +378,6 @@ static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
> reenable_ips = true;
> }
>
> - I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
> -
> i9xx_load_luts(crtc_state);
>
> if (reenable_ips)
> @@ -485,8 +485,6 @@ static void broadwell_load_luts(const struct intel_crtc_state *crtc_state)
> */
> I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> }
> -
> - I915_WRITE(GAMMA_MODE(pipe), crtc_state->gamma_mode);
> }
>
> static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> @@ -539,8 +537,6 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
> */
> I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> }
> -
> - I915_WRITE(GAMMA_MODE(pipe), crtc_state->gamma_mode);
> }
>
> static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
> @@ -551,10 +547,9 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
> const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> enum pipe pipe = crtc->pipe;
>
> + cherryview_load_csc_matrix(crtc_state);
> +
Might be worth adding a comment here to note that CHV's CSC is
single-buffered since this is different from our other platforms and
doesn't seem to be spelled out anywhere in the bspec that I can find
either.
Matt
> if (crtc_state_is_legacy_gamma(crtc_state)) {
> - /* Turn off degamma/gamma on CGM block. */
> - I915_WRITE(CGM_PIPE_MODE(pipe),
> - (crtc_state->base.ctm ? CGM_PIPE_MODE_CSC : 0));
> i9xx_load_luts_internal(crtc_state, gamma_lut);
> return;
> }
> @@ -595,11 +590,6 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
> }
> }
>
> - I915_WRITE(CGM_PIPE_MODE(pipe),
> - (crtc_state->base.ctm ? CGM_PIPE_MODE_CSC : 0) |
> - (degamma_lut ? CGM_PIPE_MODE_DEGAMMA : 0) |
> - (gamma_lut ? CGM_PIPE_MODE_GAMMA : 0));
> -
> /*
> * Also program a linear LUT in the legacy block (behind the
> * CGM block).
> @@ -614,6 +604,14 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
> dev_priv->display.load_luts(crtc_state);
> }
>
> +void intel_color_commit(const struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +
> + if (dev_priv->display.color_commit)
> + dev_priv->display.color_commit(crtc_state);
> +}
> +
> int intel_color_check(struct intel_crtc_state *crtc_state)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> @@ -660,18 +658,17 @@ void intel_color_init(struct intel_crtc *crtc)
> drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>
> if (IS_CHERRYVIEW(dev_priv)) {
> - dev_priv->display.load_csc_matrix = cherryview_load_csc_matrix;
> dev_priv->display.load_luts = cherryview_load_luts;
> } else if (IS_HASWELL(dev_priv)) {
> - dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> dev_priv->display.load_luts = haswell_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)) {
> - dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> dev_priv->display.load_luts = broadwell_load_luts;
> + dev_priv->display.color_commit = hsw_color_commit;
> } else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> - dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> dev_priv->display.load_luts = glk_load_luts;
> + dev_priv->display.color_commit = hsw_color_commit;
> } else {
> dev_priv->display.load_luts = i9xx_load_luts;
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a3871db4703b..96c78566b8e6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5705,6 +5705,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> * clocks enabled
> */
> intel_color_load_luts(pipe_config);
> + intel_color_commit(pipe_config);
>
> if (dev_priv->display.initial_watermarks != NULL)
> dev_priv->display.initial_watermarks(old_intel_state, pipe_config);
> @@ -5815,8 +5816,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>
> haswell_set_pipemisc(pipe_config);
>
> - intel_color_set_csc(pipe_config);
> -
> intel_crtc->active = true;
>
> /* Display WA #1180: WaDisableScalarClockGating: glk, cnl */
> @@ -5835,6 +5834,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> * clocks enabled
> */
> intel_color_load_luts(pipe_config);
> + intel_color_commit(pipe_config);
>
> /*
> * Display WA #1153: enable hardware to bypass the alpha math
> @@ -6180,8 +6180,6 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
>
> i9xx_set_pipeconf(pipe_config);
>
> - intel_color_set_csc(pipe_config);
> -
> intel_crtc->active = true;
>
> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> @@ -6201,6 +6199,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
> i9xx_pfit_enable(pipe_config);
>
> intel_color_load_luts(pipe_config);
> + intel_color_commit(pipe_config);
>
> dev_priv->display.initial_watermarks(old_intel_state,
> pipe_config);
> @@ -6257,6 +6256,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
> i9xx_pfit_enable(pipe_config);
>
> intel_color_load_luts(pipe_config);
> + intel_color_commit(pipe_config);
>
> if (dev_priv->display.initial_watermarks != NULL)
> dev_priv->display.initial_watermarks(old_intel_state,
> @@ -13634,10 +13634,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>
> if (!modeset &&
> (intel_cstate->base.color_mgmt_changed ||
> - intel_cstate->update_pipe)) {
> - intel_color_set_csc(intel_cstate);
> + intel_cstate->update_pipe))
> intel_color_load_luts(intel_cstate);
> - }
>
> /* Perform vblank evasion around commit operation */
> intel_pipe_update_start(intel_cstate);
> @@ -13645,6 +13643,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> if (modeset)
> goto out;
>
> + if (intel_cstate->base.color_mgmt_changed ||
> + intel_cstate->update_pipe)
> + intel_color_commit(intel_cstate);
> +
> if (intel_cstate->update_pipe)
> intel_update_pipe_config(old_intel_cstate, intel_cstate);
> else if (INTEL_GEN(dev_priv) >= 9)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 96743f50b13a..59f8d4270e82 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2338,7 +2338,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> /* intel_color.c */
> void intel_color_init(struct intel_crtc *crtc);
> int intel_color_check(struct intel_crtc_state *crtc_state);
> -void intel_color_set_csc(const struct intel_crtc_state *crtc_state);
> +void intel_color_commit(const struct intel_crtc_state *crtc_state);
> void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>
> /* intel_lspcon.c */
> --
> 2.19.2
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list