On Fri, Nov 03, 2017 at 12:12:09PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 02, 2017 at 04:00:49PM -0700, James Ausmus wrote:
> > Since GLK, some plane configuration settings have moved to the
> > PLANE_COLOR_CTL register. Refactor handling of the register to work like
> > PLANE_CTL. This also allows us to fix the set/read of the plane Alpha
> > Mode for GLK+.
> >
> > Signed-off-by: James Ausmus <james.aus...@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 12 +++++---
> > drivers/gpu/drm/i915/intel_display.c | 60
> > +++++++++++++++++++++++++++++++++---
> > drivers/gpu/drm/i915/intel_drv.h | 5 +++
> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++++----
> > 4 files changed, 76 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 68a58cce6ab1..520ff9a15222 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6263,7 +6263,7 @@ enum {
> > #define _PLANE_CTL_2_A 0x70280
> > #define _PLANE_CTL_3_A 0x70380
> > #define PLANE_CTL_ENABLE (1 << 31)
> > -#define PLANE_CTL_PIPE_GAMMA_ENABLE (1 << 30)
> > +#define PLANE_CTL_PIPE_GAMMA_ENABLE (1 << 30) /* Pre-GLK
> > */
> > #define PLANE_CTL_FORMAT_MASK (0xf << 24)
> > #define PLANE_CTL_FORMAT_YUV422 ( 0 << 24)
> > #define PLANE_CTL_FORMAT_NV12 ( 1 << 24)
> > @@ -6273,7 +6273,7 @@ enum {
> > #define PLANE_CTL_FORMAT_AYUV ( 8 << 24)
> > #define PLANE_CTL_FORMAT_INDEXED ( 12 << 24)
> > #define PLANE_CTL_FORMAT_RGB_565 ( 14 << 24)
> > -#define PLANE_CTL_PIPE_CSC_ENABLE (1 << 23)
> > +#define PLANE_CTL_PIPE_CSC_ENABLE (1 << 23) /* Pre-GLK */
> > #define PLANE_CTL_KEY_ENABLE_MASK (0x3 << 21)
> > #define PLANE_CTL_KEY_ENABLE_SOURCE ( 1 << 21)
> > #define PLANE_CTL_KEY_ENABLE_DESTINATION ( 2 << 21)
> > @@ -6286,13 +6286,13 @@ enum {
> > #define PLANE_CTL_YUV422_VYUY ( 3 << 16)
> > #define PLANE_CTL_DECOMPRESSION_ENABLE (1 << 15)
> > #define PLANE_CTL_TRICKLE_FEED_DISABLE (1 << 14)
> > -#define PLANE_CTL_PLANE_GAMMA_DISABLE (1 << 13)
> > +#define PLANE_CTL_PLANE_GAMMA_DISABLE (1 << 13) /* Pre-GLK */
> > #define PLANE_CTL_TILED_MASK (0x7 << 10)
> > #define PLANE_CTL_TILED_LINEAR ( 0 << 10)
> > #define PLANE_CTL_TILED_X ( 1 << 10)
> > #define PLANE_CTL_TILED_Y ( 4 << 10)
> > #define PLANE_CTL_TILED_YF ( 5 << 10)
> > -#define PLANE_CTL_ALPHA_MASK (0x3 << 4)
> > +#define PLANE_CTL_ALPHA_MASK (0x3 << 4) /* Pre-GLK */
> > #define PLANE_CTL_ALPHA_DISABLE ( 0 << 4)
> > #define PLANE_CTL_ALPHA_SW_PREMULTIPLY ( 2 << 4)
> > #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4)
> > @@ -6332,6 +6332,10 @@ enum {
> > #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
> > #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
> > #define PLANE_COLOR_PLANE_GAMMA_DISABLE (1 << 13)
> > +#define PLANE_COLOR_ALPHA_MASK (0x3 << 4)
> > +#define PLANE_COLOR_ALPHA_DISABLE (0 << 4)
> > +#define PLANE_COLOR_ALPHA_SW_PREMULTIPLY (2 << 4)
> > +#define PLANE_COLOR_ALPHA_HW_PREMULTIPLY (3 << 4)
> > #define _PLANE_BUF_CFG_1_A 0x7027c
> > #define _PLANE_BUF_CFG_2_A 0x7037c
> > #define _PLANE_NV12_BUF_CFG_1_A 0x70278
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e2ac976844d8..0883e857dda9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3466,6 +3466,29 @@ static u32 skl_plane_ctl_format(uint32_t
> > pixel_format)
> > return 0;
> > }
> >
> > +static u32 glk_plane_ctl_format(uint32_t pixel_format)
> > +{
> > + /* GLK+ moves the alpha mask to a different register */
> > + return skl_plane_ctl_format(pixel_format) & ~PLANE_CTL_ALPHA_MASK;
> > +}
> > +
> > +static u32 glk_plane_color_ctl_alpha(uint32_t pixel_format)
> > +{
> > + switch (pixel_format) {
> > + /*
> > + * XXX: For ARBG/ABGR formats we default to expecting scanout buffers
> > + * to be already pre-multiplied. We need to add a knob (or a different
> > + * DRM_FORMAT) for user-space to configure that.
> > + */
>
> Why is this comment getting added to the glk function? It's a generic
> issue that affects all platforms with alpha blending.
Copy-pasta from the skl_plane_ctl_format function - I'll drop it.
Thanks for the review!
-James
>
> > + case DRM_FORMAT_ABGR8888:
> > + return PLANE_COLOR_ALPHA_SW_PREMULTIPLY;
> > + case DRM_FORMAT_ARGB8888:
> > + return PLANE_COLOR_ALPHA_SW_PREMULTIPLY;
>
> case A:
> case B:
> return FOO;
>
> > + default:
> > + return PLANE_COLOR_ALPHA_DISABLE;
> > + }
> > +}
> > +
> > static u32 skl_plane_ctl_tiling(uint64_t fb_modifier)
> > {
> > switch (fb_modifier) {
> > @@ -3522,14 +3545,16 @@ u32 skl_plane_ctl(const struct intel_crtc_state
> > *crtc_state,
> >
> > plane_ctl = PLANE_CTL_ENABLE;
> >
> > - if (!IS_GEMINILAKE(dev_priv) && !IS_CANNONLAKE(dev_priv)) {
> > + if (!IS_GEMINILAKE(dev_priv) && !(INTEL_GEN(dev_priv) >= 10)) {
>
> Please flip this entire logic around:
> if (GEN >= 10 || IS_GLK) {
> ...
>
> > plane_ctl |=
> > PLANE_CTL_PIPE_GAMMA_ENABLE |
> > PLANE_CTL_PIPE_CSC_ENABLE |
> > PLANE_CTL_PLANE_GAMMA_DISABLE;
> > + plane_ctl |= skl_plane_ctl_format(fb->format->format);
> > + } else {
> > + plane_ctl |= glk_plane_ctl_format(fb->format->format);
> > }
> >
> > - plane_ctl |= skl_plane_ctl_format(fb->format->format);
> > plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> > plane_ctl |= skl_plane_ctl_rotation(rotation);
> >
> > @@ -3541,6 +3566,20 @@ u32 skl_plane_ctl(const struct intel_crtc_state
> > *crtc_state,
> > return plane_ctl;
> > }
> >
> > +u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> > + const struct intel_plane_state *plane_state)
> > +{
> > + const struct drm_framebuffer *fb = plane_state->base.fb;
> > + u32 plane_color_ctl = 0;
> > +
> > + 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(fb->format->format);
> > +
> > + return plane_color_ctl;
> > +}
> > +
> > static int
> > __intel_display_resume(struct drm_device *dev,
> > struct drm_atomic_state *state,
> > @@ -8425,7 +8464,7 @@ skylake_get_initial_plane_config(struct intel_crtc
> > *crtc,
> > {
> > struct drm_device *dev = crtc->base.dev;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > - u32 val, base, offset, stride_mult, tiling;
> > + u32 val, base, offset, stride_mult, tiling, alpha;
> > int pipe = crtc->pipe;
> > int fourcc, pixel_format;
> > unsigned int aligned_height;
> > @@ -8447,9 +8486,16 @@ skylake_get_initial_plane_config(struct intel_crtc
> > *crtc,
> > goto error;
> >
> > pixel_format = val & PLANE_CTL_FORMAT_MASK;
> > +
> > + if (!IS_GEMINILAKE(dev_priv) && !(INTEL_GEN(dev_priv) >= 10)) {
>
> ditto
>
> > + alpha = val & PLANE_CTL_ALPHA_MASK;
> > + } else {
> > + alpha = I915_READ(PLANE_COLOR_CTL(pipe, 0));
> > + alpha &= PLANE_COLOR_ALPHA_MASK;
> > + }
> > +
> > fourcc = skl_format_to_fourcc(pixel_format,
> > - val & PLANE_CTL_ORDER_RGBX,
> > - val & PLANE_CTL_ALPHA_MASK);
> > + val & PLANE_CTL_ORDER_RGBX, alpha);
> > fb->format = drm_format_info(fourcc);
> >
> > tiling = val & PLANE_CTL_TILED_MASK;
> > @@ -12816,6 +12862,10 @@ intel_check_primary_plane(struct intel_plane
> > *plane,
> >
> > state->ctl = i9xx_plane_ctl(crtc_state, state);
> > }
> > + if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10)
>
> I prefer the order to go from new to old. Makes for a more logical
> read if we add another branch to the if statement with a more recent
> platforms eg.
>
> if (LATEST)
> ...
> else if (SLIGHTLY_OLDER || EVEN_OLDER)
> ...
>
> > + state->color_ctl = glk_plane_color_ctl(crtc_state, state);
> > + else
> > + state->color_ctl = 0;
> >
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 47d022d48718..7967b3705217 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -420,6 +420,9 @@ struct intel_plane_state {
> > /* plane control register */
> > u32 ctl;
> >
> > + /* plane color control register */
> > + u32 color_ctl;
> > +
> > /*
> > * scaler_id
> > * = -1 : not using a scaler
> > @@ -1492,6 +1495,8 @@ static inline u32 intel_plane_ggtt_offset(const
> > struct intel_plane_state *state)
> > return i915_ggtt_offset(state->vma);
> > }
> >
> > +u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> > + const struct intel_plane_state *plane_state);
> > u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state);
> > u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 4fcf80ca91dd..0c09657101b7 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -240,6 +240,7 @@ skl_update_plane(struct intel_plane *plane,
> > enum plane_id plane_id = plane->id;
> > enum pipe pipe = plane->pipe;
> > u32 plane_ctl = plane_state->ctl;
> > + u32 plane_color_ctl = plane_state->color_ctl;
> > const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > u32 surf_addr = plane_state->main.offset;
> > unsigned int rotation = plane_state->base.rotation;
> > @@ -263,13 +264,9 @@ skl_update_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > - if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> > + if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10)
>
> ditto
>
> > I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > - PLANE_COLOR_PIPE_CSC_ENABLE |
> > - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> > - }
> > -
> > + plane_color_ctl);
> > if (key->flags) {
> > I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> > I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), key->max_value);
> > @@ -978,6 +975,11 @@ intel_check_sprite_plane(struct intel_plane *plane,
> > state->ctl = g4x_sprite_ctl(crtc_state, state);
> > }
> >
> > + if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10)
>
> ditto
>
> > + state->color_ctl = glk_plane_color_ctl(crtc_state, state);
> > + else
> > + state->color_ctl = 0;
> > +
> > return 0;
> > }
> >
> > --
> > 2.14.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
