From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Shankar, Uma" <uma.shankar@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 09/13] drm/i915: Track pipe gamma enable/disable in crtc state
Date: Thu, 17 Jan 2019 16:57:59 +0200 [thread overview]
Message-ID: <20190117145759.GC20097@intel.com> (raw)
In-Reply-To: <E7C9878FBA1C6D42A1CA3F62AEB6945F81ED478B@BGSMSX104.gar.corp.intel.com>
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@linux.intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@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.syrjala@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.syrjala@linux.intel.com>
> >
> >Reviewed-by: Matt Roper <matthew.d.roper@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.
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@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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-01-17 14:58 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-11 17:08 [PATCH 00/13] Enable/disable gamma/csc dynamically and fix C8 Ville Syrjala
2019-01-11 17:08 ` [PATCH 01/13] drm/i915: Clean up intel_plane_atomic_check_with_state() Ville Syrjala
2019-01-12 0:41 ` Matt Roper
2019-01-16 16:08 ` Shankar, Uma
2019-01-11 17:08 ` [PATCH 02/13] drm/i915: Split the gamma/csc enable bits from the plane_ctl() function Ville Syrjala
2019-01-12 0:41 ` Matt Roper
2019-01-14 17:11 ` Ville Syrjälä
2019-01-14 19:11 ` Ville Syrjälä
2019-01-16 17:11 ` Shankar, Uma
2019-01-17 16:34 ` Ville Syrjälä
2019-01-11 17:08 ` [PATCH 03/13] drm/i915: Precompute gamma_mode Ville Syrjala
2019-01-12 0:41 ` Matt Roper
2019-01-16 17:18 ` Shankar, Uma
2019-01-11 17:08 ` [PATCH 04/13] drm/i915: Constify the state arguments to the color management stuff Ville Syrjala
2019-01-12 0:42 ` Matt Roper
2019-01-16 17:21 ` Shankar, Uma
2019-01-11 17:08 ` [PATCH 05/13] drm/i915: Pull GAMMA_MODE write out from haswell_load_luts() Ville Syrjala
2019-01-12 0:57 ` Matt Roper
2019-01-16 17:26 ` Shankar, Uma
2019-01-11 17:08 ` [PATCH 06/13] drm/i915: Split color mgmt based on single vs. double buffered registers Ville Syrjala
2019-01-15 0:56 ` Matt Roper
2019-01-16 18:22 ` Shankar, Uma
2019-01-11 17:08 ` [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits Ville Syrjala
2019-01-16 17:38 ` Matt Roper
2019-01-16 18:02 ` Ville Syrjälä
2019-01-17 15:00 ` Ville Syrjälä
2019-01-11 17:08 ` [PATCH 08/13] drm/i915: Populate gamma_mode for all platforms Ville Syrjala
2019-01-16 18:31 ` Matt Roper
2019-01-16 18:58 ` Ville Syrjälä
2019-01-16 19:51 ` Ville Syrjälä
2019-01-29 15:59 ` Ville Syrjälä
2019-01-11 17:08 ` [PATCH 09/13] drm/i915: Track pipe gamma enable/disable in crtc state Ville Syrjala
2019-01-16 19:36 ` Matt Roper
2019-01-17 5:14 ` Shankar, Uma
2019-01-17 14:57 ` Ville Syrjälä [this message]
2019-01-11 17:08 ` [PATCH 10/13] drm/i915: Track pipe csc enable " Ville Syrjala
2019-01-16 19:43 ` Matt Roper
2019-01-17 5:17 ` Shankar, Uma
2019-01-11 17:08 ` [PATCH 11/13] drm/i915: Turn off pipe gamma when it's not needed Ville Syrjala
2019-01-17 5:32 ` Shankar, Uma
2019-01-17 18:40 ` Matt Roper
2019-01-17 18:48 ` Ville Syrjälä
2019-01-11 17:08 ` [PATCH 12/13] drm/i915: Turn off pipe CSC " Ville Syrjala
2019-01-17 5:37 ` Shankar, Uma
2019-01-17 18:54 ` Matt Roper
2019-01-11 17:08 ` [PATCH 13/13] drm/i915: Disable pipe gamma when C8 pixel format is used Ville Syrjala
2019-01-17 5:58 ` Shankar, Uma
2019-01-17 19:13 ` Matt Roper
2019-01-17 19:27 ` Ville Syrjälä
2019-01-11 17:25 ` ✗ Fi.CI.CHECKPATCH: warning for Enable/disable gamma/csc dynamically and fix C8 Patchwork
2019-01-11 17:44 ` ✓ Fi.CI.BAT: success " Patchwork
2019-01-11 22:03 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190117145759.GC20097@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=uma.shankar@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.