>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, January 17, 2019 1:07 AM
>To: Ville Syrjala <ville.syrj...@linux.intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shan...@intel.com>
>Subject: Re: [PATCH 09/13] drm/i915: Track pipe gamma enable/disable in crtc
>state
>
>On Fri, Jan 11, 2019 at 07:08:19PM +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>
>> Track whether pipe gamma is enabled or disabled. For now we stick to
>> the current behaviour of always enabling gamma. But we do get working
>> state readout for this now. On SKL+ we use the pipe bottom color as
>> our hardware state. On pre-SKL we read the state back from the primary
>> plane control register.
>> That only really correct for g4x+, as older platforms never gamma
>> correct pipe bottom color. But doing the readout the same way on all
>> platforms is fine, and there is no other way to do it really.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
>Reviewed-by: Matt Roper <matthew.d.ro...@intel.com>
>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 8 ++++
>> drivers/gpu/drm/i915/intel_color.c | 24 +++++++++++-
>> drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++------
>> drivers/gpu/drm/i915/intel_drv.h | 3 ++
>> drivers/gpu/drm/i915/intel_sprite.c | 17 +++++++--
>> 5 files changed, 92 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 9d17ba199be4..7f0913bc1b47
>> 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5707,6 +5707,14 @@ enum {
>> #define PIPEMISC_DITHER_TYPE_SP (0 << 2)
>> #define PIPEMISC(pipe) _MMIO_PIPE2(pipe,
>_PIPE_MISC_A)
>>
>> +/* SKL+ pipe bottom color */
>> +#define _PIPE_BOTTOM_COLOR_A 0x70034
>> +#define _PIPE_BOTTOM_COLOR_B 0x71034
>> +#define PIPE_BOTTOM_GAMMA_ENABLE (1 << 31)
>> +#define PIPE_BOTTOM_CSC_ENABLE (1 << 30)
>> +#define PIPE_BOTTOM_COLOR_MASK 0x3FFFFFFF
>> +#define PIPE_BOTTOM_COLOR(pipe) _MMIO_PIPE2(pipe,
>_PIPE_BOTTOM_COLOR_A)
>> +
>> #define VLV_DPFLIPSTAT
> _MMIO(VLV_DISPLAY_BASE + 0x70028)
>> #define PIPEB_LINE_COMPARE_INT_EN (1 << 29)
>> #define PIPEB_HLINE_INT_EN (1 << 28)
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 6fdbfa8c4008..313b281204fa 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -387,6 +387,24 @@ static void hsw_color_commit(const struct
>intel_crtc_state *crtc_state)
>> ilk_load_csc_matrix(crtc_state);
>> }
>>
>> +static void skl_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);
>> + enum pipe pipe = crtc->pipe;
>> + u32 val;
>> +
>> + val = 0;
We can initialize this above itself.
>> + if (crtc_state->gamma_enable)
>> + val |= PIPE_BOTTOM_GAMMA_ENABLE;
>> + val |= PIPE_BOTTOM_CSC_ENABLE;
>> + I915_WRITE(PIPE_BOTTOM_COLOR(pipe), val);
>> +
>> + I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
>> +
>> + ilk_load_csc_matrix(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);
>> @@ -624,6 +642,8 @@ int intel_color_check(struct intel_crtc_state
>*crtc_state)
>> degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
>> gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>>
>> + crtc_state->gamma_enable = true;
>> +
>> /*
>> * We also allow no degamma lut/ctm and a gamma lut at the legacy
>> * size (256 entries).
>> @@ -674,7 +694,9 @@ void intel_color_init(struct intel_crtc *crtc)
>> else
>> dev_priv->display.load_luts = i9xx_load_luts;
>>
>> - if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>> + if (INTEL_GEN(dev_priv) >= 9)
>> + dev_priv->display.color_commit = skl_color_commit;
>> + else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
>> dev_priv->display.color_commit = hsw_color_commit;
>> else
>> dev_priv->display.color_commit = ilk_color_commit; diff
>--git
>> a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 90afcae91b30..896ce95790cb 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3186,7 +3186,8 @@ static u32 i9xx_plane_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> u32 dspcntr = 0;
>>
>> - dspcntr |= DISPPLANE_GAMMA_ENABLE;
>> + if (crtc_state->gamma_enable)
>> + dspcntr |= DISPPLANE_GAMMA_ENABLE;
>>
>> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>> dspcntr |= DISPPLANE_PIPE_CSC_ENABLE; @@ -3664,7 +3665,9
>@@ u32
>> skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>> return plane_ctl;
>>
>> - plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
>> + if (crtc_state->gamma_enable)
>> + plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
>> +
>> plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
>>
>> return plane_ctl;
>> @@ -3717,7 +3720,9 @@ u32 glk_plane_color_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>> if (INTEL_GEN(dev_priv) >= 11)
>> return plane_color_ctl;
>>
>> - plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
>> + if (crtc_state->gamma_enable)
>> + plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
>> +
>> plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
>>
>> return plane_color_ctl;
>> @@ -3925,7 +3930,6 @@ static void intel_update_pipe_config(const struct
>intel_crtc_state *old_crtc_sta
>> ((new_crtc_state->pipe_src_w - 1) << 16) |
>> (new_crtc_state->pipe_src_h - 1));
>>
>> - /* on skylake this is done by detaching scalers */
Is this intentional ? Seems unrelated to the patch.
With the above minor nits fixed:
Reviewed-by: Uma Shankar <uma.shan...@intel.com>
>> if (INTEL_GEN(dev_priv) >= 9) {
>> skl_detach_scalers(new_crtc_state);
>>
>> @@ -8036,6 +8040,20 @@ static void intel_get_crtc_ycbcr_config(struct
>intel_crtc *crtc,
>> pipe_config->output_format = output; }
>>
>> +static void i9xx_get_pipe_color_config(struct intel_crtc_state
>> +*crtc_state) {
>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> + struct intel_plane *plane = to_intel_plane(crtc->base.primary);
>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> + enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
>> + u32 tmp;
>> +
>> + tmp = I915_READ(DSPCNTR(i9xx_plane));
>> +
>> + if (tmp & DISPPLANE_GAMMA_ENABLE)
>> + crtc_state->gamma_enable = true;
>> +}
>> +
>> static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>> struct intel_crtc_state *pipe_config) { @@ -
>8082,6 +8100,8 @@
>> static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>> pipe_config->gamma_mode = (tmp &
>PIPECONF_GAMMA_MODE_MASK_I9XX) >>
>> PIPECONF_GAMMA_MODE_SHIFT;
>>
>> + i9xx_get_pipe_color_config(pipe_config);
>> +
>> if (INTEL_GEN(dev_priv) < 4)
>> pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>>
>> @@ -9157,6 +9177,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc
>*crtc,
>> pipe_config->gamma_mode = (tmp &
>PIPECONF_GAMMA_MODE_MASK_ILK) >>
>> PIPECONF_GAMMA_MODE_SHIFT;
>>
>> + i9xx_get_pipe_color_config(pipe_config);
>> +
>> if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>> struct intel_shared_dpll *pll;
>> enum intel_dpll_id pll_id;
>> @@ -9785,6 +9807,15 @@ static bool haswell_get_pipe_config(struct
>intel_crtc *crtc,
>> pipe_config->gamma_mode =
>> I915_READ(GAMMA_MODE(crtc->pipe)) &
>GAMMA_MODE_MODE_MASK;
>>
>> + if (INTEL_GEN(dev_priv) >= 9) {
>> + u32 tmp = I915_READ(PIPE_BOTTOM_COLOR(crtc->pipe));
>> +
>> + if (tmp & PIPE_BOTTOM_GAMMA_ENABLE)
>> + pipe_config->gamma_enable = true;
>> + } else {
>> + i9xx_get_pipe_color_config(pipe_config);
>> + }
>> +
>> power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>> power_domain_mask |= BIT_ULL(power_domain); @@ -9953,7
>+9984,12 @@
>> i845_cursor_max_stride(struct intel_plane *plane,
>>
>> static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state
>> *crtc_state) {
>> - return CURSOR_GAMMA_ENABLE;
>> + u32 cntl = 0;
>> +
>> + if (crtc_state->gamma_enable)
>> + cntl |= CURSOR_GAMMA_ENABLE;
>> +
>> + return cntl;
>> }
>>
>> static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
>> @@ -10105,7 +10141,8 @@ static u32 i9xx_cursor_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>> if (INTEL_GEN(dev_priv) >= 11)
>> return cntl;
>>
>> - cntl |= MCURSOR_GAMMA_ENABLE;
>> + if (crtc_state->gamma_enable)
>> + cntl = MCURSOR_GAMMA_ENABLE;
>>
>> if (HAS_DDI(dev_priv))
>> cntl |= MCURSOR_PIPE_CSC_ENABLE;
>> @@ -11094,12 +11131,6 @@ static int intel_crtc_atomic_check(struct
>drm_crtc *crtc,
>> ret = intel_color_check(pipe_config);
>> if (ret)
>> return ret;
>> -
>> - /*
>> - * Changing color management on Intel hardware is
>> - * handled as part of planes update.
>> - */
>> - crtc_state->planes_changed = true;
>> }
>>
>> ret = 0;
>> @@ -11988,6 +12019,7 @@ intel_pipe_config_compare(struct
>drm_i915_private *dev_priv,
>> PIPE_CONF_CHECK_BOOL(double_wide);
>>
>> PIPE_CONF_CHECK_X(gamma_mode);
>> + PIPE_CONF_CHECK_BOOL(gamma_enable);
>>
>> PIPE_CONF_CHECK_P(shared_dpll);
>> PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 59f8d4270e82..eee734b48919 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -946,6 +946,9 @@ struct intel_crtc_state {
>> /* Output down scaling is done in LSPCON device */
>> bool lspcon_downsampling;
>>
>> + /* enable pipe gamma? */
>> + bool gamma_enable;
>> +
>> /* Display Stream compression state */
>> struct {
>> bool compression_enable;
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>> b/drivers/gpu/drm/i915/intel_sprite.c
>> index a45ef98b2f8d..034a355692db 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -739,7 +739,12 @@ vlv_update_clrc(const struct intel_plane_state
>> *plane_state)
>>
>> static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state
>> *crtc_state) {
>> - return SP_GAMMA_ENABLE;
>> + u32 sprctl = 0;
>> +
>> + if (crtc_state->gamma_enable)
>> + sprctl |= SP_GAMMA_ENABLE;
>> +
>> + return sprctl;
>> }
>>
>> static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>> @@ -915,7 +920,8 @@ static u32 ivb_sprite_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>> struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>> u32 sprctl = 0;
>>
>> - sprctl |= SPRITE_GAMMA_ENABLE;
>> + if (crtc_state->gamma_enable)
>> + sprctl |= SPRITE_GAMMA_ENABLE;
>>
>> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>> sprctl |= SPRITE_PIPE_CSC_ENABLE;
>> @@ -1101,7 +1107,12 @@ g4x_sprite_max_stride(struct intel_plane
>> *plane,
>>
>> static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state
>> *crtc_state) {
>> - return DVS_GAMMA_ENABLE;
>> + u32 dvscntr = 0;
>> +
>> + if (crtc_state->gamma_enable)
>> + dvscntr |= DVS_GAMMA_ENABLE;
>> +
>> + return dvscntr;
>> }
>>
>> static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
>> --
>> 2.19.2
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
