On Wed, Aug 15, 2018 at 12:34:05PM +0200, Maarten Lankhorst wrote: > Add plane alpha blending support with the different blend modes. > This has been tested on a icl to show the correct results, > on earlier platforms small rounding errors cause issues. But this > already happens case with fully transparant or fully opaque RGB8888 > fb's. > > The recommended HW workaround is to disable alpha blending when the > plane alpha is 0 (transparant, hide plane) or 0xff (opaque, disable blending). > This is easy to implement on any platform, so just do that. > > The tests for userspace are also available, and pass on gen11. > > Changes since v1: > - Change mistaken < 0xff0 to 0xff00. > - Only set PLANE_KEYMSK_ALPHA_ENABLE when plane alpha < 0xff00, ignore blend mode. > - Rework disabling FBC when per pixel alpha is used. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> One question and one minor suggestion inline below, but otherwise, Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_reg.h | 2 + > drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_fbc.c | 8 ++++ > drivers/gpu/drm/i915/intel_sprite.c | 23 ++++++++++- > 5 files changed, 73 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3fa56b960ef2..29f75da5fa8c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -545,6 +545,8 @@ struct intel_fbc { > int adjusted_y; > > int y; > + > + uint16_t pixel_blend_mode; > } plane; > > struct { > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 0c9f03dda569..93a1d87cdeb2 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6561,8 +6561,10 @@ enum { > #define _PLANE_KEYVAL_2_A 0x70294 > #define _PLANE_KEYMSK_1_A 0x70198 > #define _PLANE_KEYMSK_2_A 0x70298 > +#define PLANE_KEYMSK_ALPHA_ENABLE (1 << 31) > #define _PLANE_KEYMAX_1_A 0x701a0 > #define _PLANE_KEYMAX_2_A 0x702a0 > +#define PLANE_KEYMAX_ALPHA_SHIFT 24 > #define _PLANE_AUX_DIST_1_A 0x701c0 > #define _PLANE_AUX_DIST_2_A 0x702c0 > #define _PLANE_AUX_OFFSET_1_A 0x701c4 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a0345e7d3c2b..aedad3674b0d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3167,6 +3167,12 @@ int skl_check_plane_surface(const struct intel_crtc_state *crtc_state, > return -EINVAL; > } > > + /* HW only has 8 bits pixel precision, disable plane if invisible */ > + if (!(plane_state->base.alpha >> 8)) { > + plane_state->base.visible = false; > + return 0; > + } > + > if (!plane_state->base.visible) > return 0; > > @@ -3512,30 +3518,39 @@ static u32 skl_plane_ctl_format(uint32_t pixel_format) > return 0; > } > > -/* > - * 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. > - */ > -static u32 skl_plane_ctl_alpha(uint32_t pixel_format) > +static u32 skl_plane_ctl_alpha(const struct intel_plane_state *plane_state) > { > - switch (pixel_format) { > - case DRM_FORMAT_ABGR8888: > - case DRM_FORMAT_ARGB8888: > + if (!plane_state->base.fb->format->has_alpha) > + return PLANE_CTL_ALPHA_DISABLE; > + > + switch (plane_state->base.pixel_blend_mode) { > + case DRM_MODE_BLEND_PIXEL_NONE: > + return PLANE_CTL_ALPHA_DISABLE; > + case DRM_MODE_BLEND_PREMULTI: > return PLANE_CTL_ALPHA_SW_PREMULTIPLY; > + case DRM_MODE_BLEND_COVERAGE: > + return PLANE_CTL_ALPHA_HW_PREMULTIPLY; > default: > - return PLANE_CTL_ALPHA_DISABLE; > + MISSING_CASE(plane_state->base.pixel_blend_mode); > + return 0; Maybe just add the MISSING_CASE line before the current return? The macro still expands to 0, so leaving that makes it slightly more clear what the default fallback is. Same for the glk_ function below. > } > } > > -static u32 glk_plane_color_ctl_alpha(uint32_t pixel_format) > +static u32 glk_plane_color_ctl_alpha(const struct intel_plane_state *plane_state) > { > - switch (pixel_format) { > - case DRM_FORMAT_ABGR8888: > - case DRM_FORMAT_ARGB8888: > + if (!plane_state->base.fb->format->has_alpha) > + return PLANE_COLOR_ALPHA_DISABLE; > + > + switch (plane_state->base.pixel_blend_mode) { > + case DRM_MODE_BLEND_PIXEL_NONE: > + return PLANE_COLOR_ALPHA_DISABLE; > + case DRM_MODE_BLEND_PREMULTI: > return PLANE_COLOR_ALPHA_SW_PREMULTIPLY; > + case DRM_MODE_BLEND_COVERAGE: > + return PLANE_COLOR_ALPHA_HW_PREMULTIPLY; > default: > - return PLANE_COLOR_ALPHA_DISABLE; > + MISSING_CASE(plane_state->base.pixel_blend_mode); > + return 0; > } > } > > @@ -3611,7 +3626,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state, > plane_ctl = PLANE_CTL_ENABLE; > > if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) { > - plane_ctl |= skl_plane_ctl_alpha(fb->format->format); > + plane_ctl |= skl_plane_ctl_alpha(plane_state); > plane_ctl |= > PLANE_CTL_PIPE_GAMMA_ENABLE | > PLANE_CTL_PIPE_CSC_ENABLE | > @@ -3653,7 +3668,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state, > 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); > + plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > if (fb->format->is_yuv) { > if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) > @@ -13792,7 +13807,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) > DRM_MODE_ROTATE_0, > supported_rotations); > > - if (INTEL_GEN(dev_priv) >= 9) > + if (INTEL_GEN(dev_priv) >= 9) { > drm_plane_create_color_properties(&primary->base, > BIT(DRM_COLOR_YCBCR_BT601) | > BIT(DRM_COLOR_YCBCR_BT709), > @@ -13801,6 +13816,13 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) > DRM_COLOR_YCBCR_BT709, > DRM_COLOR_YCBCR_LIMITED_RANGE); > > + drm_plane_create_alpha_property(&primary->base); > + drm_plane_create_blend_mode_property(&primary->base, > + BIT(DRM_MODE_BLEND_PIXEL_NONE) | > + BIT(DRM_MODE_BLEND_PREMULTI) | > + BIT(DRM_MODE_BLEND_COVERAGE)); > + } > + > drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs); > > return primary; > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 75a6bbf3ada1..42a49fe34ba3 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -674,6 +674,8 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > cache->plane.adjusted_y = plane_state->main.y; > cache->plane.y = plane_state->base.src.y1 >> 16; > > + cache->plane.pixel_blend_mode = plane_state->base.pixel_blend_mode; > + > if (!cache->plane.visible) > return; > > @@ -748,6 +750,12 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > + if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE && > + cache->fb.format->has_alpha) { > + fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC"; > + return false; > + } This sounds reasonable, but is there somewhere in the b-spec that explains this explicitly? The FBC_CTL description lists pixel format restrictions (16 or 32 bit 888 RGB), but nothing explicitly about blending or alpha. Matt > + > /* WaFbcExceedCdClockThreshold:hsw,bdw */ > if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) && > cache->crtc.hsw_bdw_pixel_rate >= dev_priv->cdclk.hw.cdclk * 95 / 100) { > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index f7026e887fa9..4639ef9bde94 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -252,6 +252,7 @@ skl_update_plane(struct intel_plane *plane, > uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16; > uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16; > unsigned long irqflags; > + u32 keymsk = 0, keymax = 0; > > /* Sizes are 0 based */ > src_w--; > @@ -267,10 +268,19 @@ skl_update_plane(struct intel_plane *plane, > > 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); > - I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), key->channel_mask); > + > + keymax |= key->max_value & 0xffffff; > + keymsk |= key->channel_mask & 0x3ffffff; > } > > + keymax |= (plane_state->base.alpha >> 8) << PLANE_KEYMAX_ALPHA_SHIFT; > + > + if (plane_state->base.alpha < 0xff00) > + keymsk |= PLANE_KEYMSK_ALPHA_ENABLE; > + > + I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax); > + I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk); > + > I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x); > I915_WRITE_FW(PLANE_STRIDE(pipe, plane_id), stride); > I915_WRITE_FW(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w); > @@ -1650,6 +1660,15 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, > DRM_COLOR_YCBCR_BT709, > DRM_COLOR_YCBCR_LIMITED_RANGE); > > + if (INTEL_GEN(dev_priv) >= 9) { > + drm_plane_create_alpha_property(&intel_plane->base); > + > + drm_plane_create_blend_mode_property(&intel_plane->base, > + BIT(DRM_MODE_BLEND_PIXEL_NONE) | > + BIT(DRM_MODE_BLEND_PREMULTI) | > + BIT(DRM_MODE_BLEND_COVERAGE)); > + } > + > drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs); > > return intel_plane; > -- > 2.18.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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