Re: [PATCH] drm/i915: Add plane alpha blending support, v2.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Maarten Lankhorst (2018-10-02 11:51:45)
> Op 23-08-18 om 00:57 schreef Matt Roper:
> > 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.
> >
> Thanks for review btw, finally pushed now that DINQ backmerged the original patch. :)

Since landing, we now have sporadic failures, but frequent enough across
the whole set of subtests, on kms_cursor_crc for gen9+. Coincidence?

I'd be tempted to say that we don't reset some residual state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux