On Thu, Jan 17, 2019 at 05:14:06AM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Roper, Matthew D
> >Sent: Thursday, January 17, 2019 1:07 AM
> >To: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma <uma.shankar@xxxxxxxxx>
> >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.syrjala@xxxxxxxxxxxxxxx>
> >>
> >> 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.syrjala@xxxxxxxxxxxxxxx>
> >
> >Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >
> >> ---
> >> 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.
Ack.
>
> >> + 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.
Some random rebase fail probably.
>
> With the above minor nits fixed:
> Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>
> >> 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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
