On Fri, Jan 11, 2019 at 07:08:12PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> On g4x+ the pipe gamma enable bit for the primary plane affects
> the pipe bottom color as well. The same for the pipe csc enable
> bit on ilk+. Thus we must configure those bits correctly even
> when the primary plane is disabled.
This is only true for <gen9, right? Starting with gen9, we have
dedicated bits that control this, so I don't think the primay plane's
settings should have any impact when disabled. I.e., we also need the
bits set in this patch:
https://patchwork.freedesktop.org/patch/271109/
> To make the feasible let's split those settings from the
> plane_ctl() function into a seprate funciton that we can
> call from the ->disable_plane() hook as well.
Is calling it from ->disable_plane() enough? If we just disable the
primary plane, then those bits will remain set while the crtc remains
active. But if you then disable the whole crtc and re-enable it again
later, won't we have lost the bits at that point?
Matt
>
> For consistency we'll do that on all the plane types. While
> that has no real benefits at this time, it'll become useful
> when we start to control the pipe gamma/csc enable bits
> dynamically when we overhaul the color management code.
>
> On pre-g4x there doesn't appear to be any way to gamma
> correct the pipe bottom color, but sticking to the same
> pattern doesn't hurt. And it'll still help us to do
> crtc state readout correctly for the pipe gamma enable
> bit for the color management overhaul.
>
> An alternative apporach would be to still precompute these
> bits into plane_state->ctl, but that would require that we
> run through the plane check even when the plane isn't logically
> enabled on any crtc. Currently that condition causes us to
> short circuit the entire thing and not call ->check_plane().
> There would also be some chicken and egg problems with
> ->check_plane() vs. crtc color state check that would
> requite splitting certain things into multiple steps.
> So all in all this seems like the easier route.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_sprite.c | 54 ++++++++---
> 3 files changed, 139 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5dc0de89c49e..a3871db4703b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3180,28 +3180,38 @@ i9xx_plane_max_stride(struct intel_plane *plane,
> }
> }
>
> +static u32 i9xx_plane_ctl_crtc(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);
> + u32 dspcntr = 0;
> +
> + dspcntr |= DISPPLANE_GAMMA_ENABLE;
> +
> + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> + dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> +
> + if (INTEL_GEN(dev_priv) < 5)
> + dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> +
> + return dspcntr;
> +}
> +
> static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> struct drm_i915_private *dev_priv =
> to_i915(plane_state->base.plane->dev);
> - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> const struct drm_framebuffer *fb = plane_state->base.fb;
> unsigned int rotation = plane_state->base.rotation;
> u32 dspcntr;
>
> - dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
> + dspcntr = DISPLAY_PLANE_ENABLE;
>
> if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||
> IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>
> - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> - dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> -
> - if (INTEL_GEN(dev_priv) < 5)
> - dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> -
> switch (fb->format->format) {
> case DRM_FORMAT_C8:
> dspcntr |= DISPPLANE_8BPP;
> @@ -3329,11 +3339,13 @@ static void i9xx_update_plane(struct intel_plane *plane,
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> u32 linear_offset;
> - u32 dspcntr = plane_state->ctl;
> int x = plane_state->color_plane[0].x;
> int y = plane_state->color_plane[0].y;
> unsigned long irqflags;
> u32 dspaddr_offset;
> + u32 dspcntr;
> +
> + dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
>
> linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
>
> @@ -3393,10 +3405,23 @@ static void i9xx_disable_plane(struct intel_plane *plane,
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> unsigned long irqflags;
> + u32 dspcntr;
> +
> + /*
> + * DSPCNTR pipe gamma enable on g4x+ and pipe csc
> + * enable on ilk+ affect the pipe bottom color as
> + * well, so we must configure them even if the plane
> + * is disabled.
> + *
> + * On pre-g4x there is no way to gamma correct the
> + * pipe bottom color but we'll keep on doing this
> + * anyway.
> + */
> + dspcntr = i9xx_plane_ctl_crtc(crtc_state);
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> - I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
> + I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
> if (INTEL_GEN(dev_priv) >= 4)
> I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
> else
> @@ -3631,6 +3656,20 @@ static u32 cnl_plane_ctl_flip(unsigned int reflect)
> return 0;
> }
>
> +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> + u32 plane_ctl = 0;
> +
> + if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> + return plane_ctl;
> +
> + plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> + plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
> +
> + return plane_ctl;
> +}
> +
> u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> @@ -3645,10 +3684,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>
> if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
> plane_ctl |= skl_plane_ctl_alpha(plane_state);
> - plane_ctl |=
> - PLANE_CTL_PIPE_GAMMA_ENABLE |
> - PLANE_CTL_PIPE_CSC_ENABLE |
> - PLANE_CTL_PLANE_GAMMA_DISABLE;
> + plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>
> if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> plane_ctl |= PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
> @@ -3673,19 +3709,27 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> return plane_ctl;
> }
>
> +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> + u32 plane_color_ctl = 0;
> +
> + if (INTEL_GEN(dev_priv) >= 11)
> + return plane_color_ctl;
> +
> + plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> + plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> +
> + return plane_color_ctl;
> +}
> +
> u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> - struct drm_i915_private *dev_priv =
> - to_i915(plane_state->base.plane->dev);
> const struct drm_framebuffer *fb = plane_state->base.fb;
> struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> u32 plane_color_ctl = 0;
>
> - if (INTEL_GEN(dev_priv) < 11) {
> - plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> - plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> - }
> plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>
> @@ -9867,11 +9911,15 @@ i845_cursor_max_stride(struct intel_plane *plane,
> return 2048;
> }
>
> +static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> + return CURSOR_GAMMA_ENABLE;
> +}
> +
> static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> return CURSOR_ENABLE |
> - CURSOR_GAMMA_ENABLE |
> CURSOR_FORMAT_ARGB |
> CURSOR_STRIDE(plane_state->color_plane[0].stride);
> }
> @@ -9941,7 +9989,9 @@ static void i845_update_cursor(struct intel_plane *plane,
> unsigned int width = plane_state->base.crtc_w;
> unsigned int height = plane_state->base.crtc_h;
>
> - cntl = plane_state->ctl;
> + cntl = plane_state->ctl |
> + i845_cursor_ctl_crtc(crtc_state);
> +
> size = (height << 12) | width;
>
> base = intel_cursor_base(plane_state);
> @@ -10006,27 +10056,36 @@ i9xx_cursor_max_stride(struct intel_plane *plane,
> return plane->base.dev->mode_config.cursor_width * 4;
> }
>
> -static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> - const struct intel_plane_state *plane_state)
> +static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> {
> - struct drm_i915_private *dev_priv =
> - to_i915(plane_state->base.plane->dev);
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> u32 cntl = 0;
>
> - if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> - cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> + if (INTEL_GEN(dev_priv) >= 11)
> + return cntl;
>
> - if (INTEL_GEN(dev_priv) <= 10) {
> - cntl |= MCURSOR_GAMMA_ENABLE;
> + cntl |= MCURSOR_GAMMA_ENABLE;
>
> - if (HAS_DDI(dev_priv))
> - cntl |= MCURSOR_PIPE_CSC_ENABLE;
> - }
> + if (HAS_DDI(dev_priv))
> + cntl |= MCURSOR_PIPE_CSC_ENABLE;
>
> if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
>
> + return cntl;
> +}
> +
> +static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv =
> + to_i915(plane_state->base.plane->dev);
> + u32 cntl = 0;
> +
> + if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> + cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> +
> switch (plane_state->base.crtc_w) {
> case 64:
> cntl |= MCURSOR_MODE_64_ARGB_AX;
> @@ -10151,7 +10210,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> unsigned long irqflags;
>
> if (plane_state && plane_state->base.visible) {
> - cntl = plane_state->ctl;
> + cntl = plane_state->ctl |
> + i9xx_cursor_ctl_crtc(crtc_state);
>
> if (plane_state->base.crtc_h != plane_state->base.crtc_w)
> fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h - 1);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3b051fdd0fce..88ac42b2d7ed 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1752,9 +1752,10 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
>
> u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state);
> +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state);
> u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state);
> -u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
> u32 skl_plane_stride(const struct intel_plane_state *plane_state,
> int plane);
> int skl_check_plane_surface(struct intel_plane_state *plane_state);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8f3982c03925..a45ef98b2f8d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -484,9 +484,16 @@ skl_program_plane(struct intel_plane *plane,
> struct intel_plane *linked = plane_state->linked_plane;
> const struct drm_framebuffer *fb = plane_state->base.fb;
> u8 alpha = plane_state->base.alpha >> 8;
> + u32 plane_color_ctl = 0;
> unsigned long irqflags;
> u32 keymsk, keymax;
>
> + plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> +
> + if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> + plane_color_ctl = plane_state->color_ctl |
> + glk_plane_color_ctl_crtc(crtc_state);
> +
> /* Sizes are 0 based */
> src_w--;
> src_h--;
> @@ -533,8 +540,7 @@ skl_program_plane(struct intel_plane *plane,
> }
>
> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> - I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> - plane_state->color_ctl);
> + I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id), plane_color_ctl);
>
> if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> icl_program_input_csc(plane, crtc_state, plane_state);
> @@ -731,6 +737,11 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> }
>
> +static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> + return SP_GAMMA_ENABLE;
> +}
> +
> static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> @@ -739,7 +750,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 sprctl;
>
> - sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
> + sprctl = SP_ENABLE;
>
> switch (fb->format->format) {
> case DRM_FORMAT_YUYV:
> @@ -806,7 +817,6 @@ vlv_update_plane(struct intel_plane *plane,
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> enum pipe pipe = plane->pipe;
> enum plane_id plane_id = plane->id;
> - u32 sprctl = plane_state->ctl;
> u32 sprsurf_offset = plane_state->color_plane[0].offset;
> u32 linear_offset;
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> @@ -817,6 +827,9 @@ vlv_update_plane(struct intel_plane *plane,
> uint32_t x = plane_state->color_plane[0].x;
> uint32_t y = plane_state->color_plane[0].y;
> unsigned long irqflags;
> + u32 sprctl;
> +
> + sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
>
> /* Sizes are 0 based */
> crtc_w--;
> @@ -897,6 +910,19 @@ vlv_plane_get_hw_state(struct intel_plane *plane,
> return ret;
> }
>
> +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 (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> + sprctl |= SPRITE_PIPE_CSC_ENABLE;
> +
> + return sprctl;
> +}
> +
> static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> @@ -907,14 +933,11 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 sprctl;
>
> - sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
> + sprctl = SPRITE_ENABLE;
>
> if (IS_IVYBRIDGE(dev_priv))
> sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
>
> - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> - sprctl |= SPRITE_PIPE_CSC_ENABLE;
> -
> switch (fb->format->format) {
> case DRM_FORMAT_XBGR8888:
> sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
> @@ -966,7 +989,6 @@ ivb_update_plane(struct intel_plane *plane,
> {
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> enum pipe pipe = plane->pipe;
> - u32 sprctl = plane_state->ctl, sprscale = 0;
> u32 sprsurf_offset = plane_state->color_plane[0].offset;
> u32 linear_offset;
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> @@ -978,8 +1000,11 @@ ivb_update_plane(struct intel_plane *plane,
> uint32_t y = plane_state->color_plane[0].y;
> uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> + u32 sprctl, sprscale = 0;
> unsigned long irqflags;
>
> + sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> +
> /* Sizes are 0 based */
> src_w--;
> src_h--;
> @@ -1074,6 +1099,11 @@ g4x_sprite_max_stride(struct intel_plane *plane,
> return 16384;
> }
>
> +static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> + return DVS_GAMMA_ENABLE;
> +}
> +
> static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> @@ -1084,7 +1114,7 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 dvscntr;
>
> - dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
> + dvscntr = DVS_ENABLE;
>
> if (IS_GEN(dev_priv, 6))
> dvscntr |= DVS_TRICKLE_FEED_DISABLE;
> @@ -1140,7 +1170,6 @@ g4x_update_plane(struct intel_plane *plane,
> {
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> enum pipe pipe = plane->pipe;
> - u32 dvscntr = plane_state->ctl, dvsscale = 0;
> u32 dvssurf_offset = plane_state->color_plane[0].offset;
> u32 linear_offset;
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> @@ -1152,8 +1181,11 @@ g4x_update_plane(struct intel_plane *plane,
> uint32_t y = plane_state->color_plane[0].y;
> uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> + u32 dvscntr, dvsscale = 0;
> unsigned long irqflags;
>
> + dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
> +
> /* Sizes are 0 based */
> src_w--;
> src_h--;
> --
> 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
