Op 04-06-18 om 16:29 schreef Ville Syrjälä:
> On Mon, Jun 04, 2018 at 03:50:13PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 2 +
>> drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++--------
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> drivers/gpu/drm/i915/intel_fbc.c | 4 ++
>> drivers/gpu/drm/i915/intel_sprite.c | 24 +++++++++-
>> 5 files changed, 79 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 327829cc61f8..23f0cb0fad0e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6425,8 +6425,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
> PLANE_KEYMAX_ALPHA(x)
>
>> #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 d48256f89a50..8ddff09c3110 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3170,6 +3170,21 @@ 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.alpha < 0xff00) {
>> + plane_state->flags |= PLANE_ALPHA_ENABLED;
>> +
>> + /* FBC cannot be enabled with per pixel alpha */
>> + if (plane_state->base.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
>> + fb->format->has_alpha)
>> + plane_state->flags |= PLANE_ALPHA_NO_FBC;
> Why is this fbc per-pixel alpha check inside the constant alpha if block?
>
> Also I'm not convinced by these plane state flags. They're not
> consistent with how we do everything else, so I would drop them.
The only reason I'm using it is because we already have PLANE_HAS_FENCE there. Because the FBC
code already copies flags, it makes sense to re-use it rather than adding another field.
>> + }
>> +
>> if (!plane_state->base.visible)
>> return 0;
>>
>> @@ -3496,30 +3511,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;
>> }
>> }
>>
>> -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;
>> }
>> }
>>
>> @@ -3595,7 +3619,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 |
>> @@ -3637,7 +3661,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 (intel_format_is_yuv(fb->format->format)) {
>> if (fb->format->format == DRM_FORMAT_NV12) {
>> @@ -13623,7 +13647,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),
>> @@ -13632,6 +13656,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 2dc01be0c7cc..675dbe927a06 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -500,6 +500,8 @@ struct intel_plane_state {
>> struct i915_vma *vma;
>> unsigned long flags;
>> #define PLANE_HAS_FENCE BIT(0)
>> +#define PLANE_ALPHA_ENABLED BIT(1)
>> +#define PLANE_ALPHA_NO_FBC BIT(2)
>>
>> struct {
>> u32 offset;
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 1f2f41e67dcd..20a2dba78cad 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -732,6 +732,10 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>> fbc->no_fbc_reason = "framebuffer not tiled or fenced";
>> return false;
>> }
>> + if (cache->flags & PLANE_ALPHA_NO_FBC) {
>> + fbc->no_fbc_reason = "per pixel alpha blending enabled";
>> + return false;
>> + }
>> if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
>> cache->plane.rotation != DRM_MODE_ROTATE_0) {
>> fbc->no_fbc_reason = "rotation unsupported";
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 0b1bd5de5192..4fcc7ce09a9c 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -254,6 +254,7 @@ skl_update_plane(struct intel_plane *plane,
>> uint32_t y = plane_state->main.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 keymsk = 0, keymax = 0;
>>
>> /* Sizes are 0 based */
>> src_w--;
>> @@ -267,10 +268,20 @@ 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;
> What's up with |= vs. = here?
Well I guess it can be just |= for both.
>
>> }
>>
>> + keymax |= (plane_state->base.alpha >> 8) << PLANE_KEYMAX_ALPHA_SHIFT;
>> +
>> + if (plane_state->flags & PLANE_ALPHA_ENABLED ||
>> + plane_state->base.fb->format->has_alpha)
>> + keymsk |= PLANE_KEYMSK_ALPHA_ENABLE;
> Why are we checking the format has_alpha here? Isn't this about the
> constant alpha?
Hm looks wrong, must be just the initial check.
>
>> +
>> + 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);
>> @@ -1525,6 +1536,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.17.1
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
