Re: [PATCH 5/9] drm/i915/skl: Add support for blending modes

Matt Roper
2016-01-21T22:33:51Z

On Mon, Jan 18, 2016 at 08:45:39PM +0530, Vandita Kulkarni wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
>
> This patch adds support for blending modes involving
> color.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_sprite.c | 12 +++++++-----
> 3 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e59a49..39e9db4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11901,6 +11901,7 @@ static int intel_plane_state_check_blend(struct drm_plane_state *plane_state)
>
> state->premultiplied_alpha = false;
> state->drop_alpha = false;
> + state->use_plane_alpha = false;
>
> switch (mode->func) {
> /*
> @@ -11929,6 +11930,27 @@ static int intel_plane_state_check_blend(struct drm_plane_state *plane_state)
> if (!has_per_pixel_blending)
> return -EINVAL;
> break;
> + /* plane alpha */
> + case DRM_BLEND_FUNC(CONSTANT_ALPHA, ONE_MINUS_CONSTANT_ALPHA):
> + if (has_per_pixel_blending)
> + state->drop_alpha = true;
> + state->use_plane_alpha = true;
> + break;
> + /* plane alpha, pre-multiplied fb */
> + case DRM_BLEND_FUNC(CONSTANT_ALPHA,
> + ONE_MINUS_CONSTANT_ALPHA_TIMES_SRC_ALPHA):
> + if (!has_per_pixel_blending)
> + return -EINVAL;
> + state->premultiplied_alpha = true;
> + state->use_plane_alpha = true;
> + break;
> + /* plane alpha, non pre-multiplied fb */
> + case DRM_BLEND_FUNC(CONSTANT_ALPHA_TIMES_SRC_ALPHA,
> + ONE_MINUS_CONSTANT_ALPHA_TIMES_SRC_ALPHA):
> + if (!has_per_pixel_blending)
> + return -EINVAL;
> + state->use_plane_alpha = true;
> + break;
> default:
> return -EINVAL;
> }
> @@ -14272,6 +14294,12 @@ void intel_plane_add_blend_properties(struct intel_plane *plane)
> if (prop)
> drm_object_attach_property(&plane->base.base, prop,
> DRM_BLEND_FUNC(AUTO, AUTO));
> +
> + prop = dev->mode_config.prop_blend_color;
> + if (prop)
> + drm_object_attach_property(&plane->base.base, prop,
> + DRM_MODE_COLOR(0xffff, 0xffff,
> + 0xffff, 0xffff));
> }
>
> static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f99e1d9..8226c8e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -299,6 +299,7 @@ struct intel_plane_state {
> */
> bool premultiplied_alpha; /* is the fb pre-multiplied? */
> bool drop_alpha; /* drop the fb alpha channel */
> + bool use_plane_alpha; /* use per-plane alpha */
>
> struct drm_intel_sprite_colorkey ckey;
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9538658..9175152 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -227,12 +227,14 @@ skl_update_plane(struct drm_plane *drm_plane,
> crtc_w--;
> crtc_h--;
>
> - if (key->flags) {
> + I915_WRITE(PLANE_KEYMAX(pipe, plane),
> + (DRM_MODE_COLOR_ALPHA_8(plane_state->base.blend_mode.color)
> + << 24) | (key->max_value & 0x00ffffff));
> + I915_WRITE(PLANE_KEYMSK(pipe, plane),
> + (plane_state->use_plane_alpha << 31) |
> + (key->channel_mask & GENMASK(0, 26)));

I'm having trouble reconciling the bspec description of these two
registers. The bspec says that PLANE_KEYMAX is only used for YUV
framebuffers and that the register is unused with RGB buffers. However
the description of PLANE_KEYMSK's bit 31 says it uses the top byte of
PLANE_KEYMAX as the constant alpha value (which seems to apply to all
buffer formats).

Is this a bug (or poor wording) in the bspec or am I completely
misinterpreting? Assuming the hardware works the way your code is
written here, it seems like the bspec description of PLANE_KEYMAX should
actually say that bits 0-23 are exclusive to YUV buffers but that 24-31
are usable on any buffer format?

> + if (key->flags)
> I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);

Does it matter that we're writing possible garbage bits from
key->max_value and key->channel_mask unconditionally now? Previously
they were guarded by key->flags.

Matt

> - I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> - I915_WRITE(PLANE_KEYMSK(pipe, plane), key->channel_mask);
> - }
> -
> if (key->flags & I915_SET_COLORKEY_DESTINATION)
> plane_ctl |= PLANE_CTL_KEY_ENABLE_DESTINATION;
> else if (key->flags & I915_SET_COLORKEY_SOURCE)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795