>-----Original Message-----
>From: Roper, Matthew D
>Sent: Tuesday, January 15, 2019 6:27 AM
>To: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma <uma.shankar@xxxxxxxxx>
>Subject: Re: [PATCH 06/13] drm/i915: Split color mgmt based on single vs. double
>buffered registers
>
>On Fri, Jan 11, 2019 at 07:08:16PM +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>
>> Split the color managemnt hooks along the single vs. double buffered
Typo in management.
>> registers line. Of the currently progammed registers GAMMA_MODE and
Typo in programmed.
>> 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@xxxxxxxxxxxxxxx>
>> ---
>> 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.
This documentation or some comments in the function description in code will
definitely help.
>
>> };
>>
>> 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);
One issue which I see here is that we are mixing programming of csc and gamma mode
in a common function. There may be situations where we don't want either a csc or
gamma to be enabled. Also more so since the respective blocks are implemented as
separate properties. Not sure if this will cause any real issue though.
>> +}
>> +
>> /* 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
