From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: igt-dev@lists.freedesktop.org, Lowry Li <lowry.li@arm.com>,
dri-devel@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH] drm/i915: Add plane alpha blending support based on RFC
Date: Mon, 4 Jun 2018 19:47:29 +0300 [thread overview]
Message-ID: <20180604164729.GE23723@intel.com> (raw)
In-Reply-To: <164ce4fb-f203-4ada-a9ae-a578436dc5c5@linux.intel.com>
On Mon, Jun 04, 2018 at 06:27:20PM +0200, Maarten Lankhorst wrote:
> 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@linux.intel.com>
> >> ---
> >> 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.
The only reason for the has_fence is because we don't have that
information elsewhere. I'd rather not duplicate things that are easily
available in the state already.
Also, the fbc thing needs to be rewritten anyway. There is no reason
to copy over all these things into the cache, and instead we should
just look at the plane state during the plane atomic check and decide
if fbc can or can't be used based on the new state.
>
> >> + }
> >> +
> >> 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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2018-06-04 16:47 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-01 12:41 [PATCH v3 0/2] drm/blend: Add per-plane pixel blend mode property Lowry Li
2018-06-01 12:41 ` [PATCH v3 1/2] drm: " Lowry Li
2018-06-04 13:49 ` Emil Velikov
2018-06-05 9:07 ` Lowry Li
[not found] ` <20180605090729.GA2686@lowry.li@arm.com>
2018-08-13 10:49 ` Maarten Lankhorst
2018-08-14 3:11 ` Lowry Li
[not found] ` <20180814031102.GA8541@lowry.li@arm.com>
2018-08-14 9:15 ` Maarten Lankhorst
2018-08-14 9:22 ` Lowry Li
2018-08-14 9:28 ` Daniel Vetter
2018-08-14 11:38 ` Lowry Li
2018-06-04 13:49 ` [PATCH i-g-t 1/2] lib/igt_kms: Add set_prop_enum for mode objects Maarten Lankhorst
2018-06-04 13:49 ` [PATCH i-g-t 2/2] tests: Add kms plane alpha blending test Maarten Lankhorst
2018-06-04 13:50 ` [PATCH] drm/i915: Add plane alpha blending support based on RFC Maarten Lankhorst
2018-06-04 14:29 ` [igt-dev] " Ville Syrjälä
2018-06-04 16:27 ` Maarten Lankhorst
2018-06-04 16:47 ` Ville Syrjälä [this message]
2018-06-01 12:41 ` [PATCH v3 2/2] drm/mali-dp: Implement plane alpha and pixel blend on malidp Lowry Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180604164729.GE23723@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=igt-dev@lists.freedesktop.org \
--cc=lowry.li@arm.com \
--cc=maarten.lankhorst@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).