Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 22 Aug 2018 08:39:49 -0400
From:   Sean Paul <sean@poorly.run>
To:     Lowry Li <lowry.li@arm.com>
Cc:     liviu.dudau@arm.com, airlied@linux.ie,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        emil.l.velikov@gmail.com, seanpaul@chromium.org,
        malidp@foss.arm.com, daniel.vetter@intel.com, nd@arm.com
Subject: Re: [PATCH v4 1/2] drm: Add per-plane pixel blend mode property
Message-ID: <20180822123949.GU164269@art_vandelay>
References: <1534314933-23182-1-git-send-email-lowry.li@arm.com>
 <1534314933-23182-2-git-send-email-lowry.li@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1534314933-23182-2-git-send-email-lowry.li@arm.com>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 15, 2018 at 02:35:32PM +0800, Lowry Li wrote:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
> 
> Adds a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
> 
> Defines three blend modes in drm_blend.h.
> 
> Changes since v1:
>  - Moves the blending equation into the DOC comment
>  - Refines the comments of drm_plane_create_blend_mode_property to not
>    enumerate the #defines, but instead the string values
>  - Uses fg.* instead of pixel.* and plane_alpha instead of plane.alpha
> Changes since v2:
>  - Refines the comments of drm_plane_create_blend_mode_property:
>       1) Puts the descriptions (after the ":") on a new line
>       2) Adds explaining why @supported_modes need PREMUL as default
> Changes since v3:
>  - Refines drm_plane_create_blend_mode_property(). drm_property_add_enum()
>    can calculate the index itself just fine, so no point in having the
>    caller pass it in.
>  - Since the current DRM assumption is that alpha is premultiplied
>    as default, define DRM_MODE_BLEND_PREMULTI as 0 will be better.
>  - Refines some comments.
> 
> Signed-off-by: Lowry Li <lowry.li@arm.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |   4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |   1 +
>  drivers/gpu/drm/drm_blend.c         | 126 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h             |   6 ++
>  include/drm/drm_plane.h             |   2 +
>  5 files changed, 139 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3eb061e..d0478ab 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -895,6 +895,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->src_h = val;
>  	} else if (property == plane->alpha_property) {
>  		state->alpha = val;
> +	} else if (property == plane->blend_mode_property) {
> +		state->pixel_blend_mode = val;
>  	} else if (property == plane->rotation_property) {
>  		if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) {
>  			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] bad rotation bitmask: 0x%llx\n",
> @@ -968,6 +970,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		*val = state->src_h;
>  	} else if (property == plane->alpha_property) {
>  		*val = state->alpha;
> +	} else if (property == plane->blend_mode_property) {
> +		*val = state->pixel_blend_mode;
>  	} else if (property == plane->rotation_property) {
>  		*val = state->rotation;
>  	} else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 6dd5036..563af09 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3569,6 +3569,7 @@ void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
>  	/* Reset the alpha value to fully opaque if it matters */
>  	if (plane->alpha_property)
>  		state->alpha = plane->alpha_property->values[1];
> +	plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;

Shouldn't this be state->pixel_blend_mode instead of
plane->state->pixel_blend_mode?

>  
>  	plane->state = state;
>  }

/snip

-- 
Sean Paul, Software Engineer, Google / Chromium OS