Re: [PATCH 1/9] drm: Introduce the blend-func property

[Matt Roper <matthew.d.roper@intel.com>]

On Mon, Jan 18, 2016 at 08:45:35PM +0530, Vandita Kulkarni wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> We'd like to be able to program the blending modes of display planes.
> Ville suggested to use something similar to the GL blend states, which
> does seem like a good idea.
> 
> For now, we only consider blend factors, but room is left for
> extensions: blend equation, separate rgb/alpha blend factors, blend
> color.
> 
> V2: Added the belnd func property support in get property.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>

You'll want a Cc: dri-devel here as well for stuff like this that
touches the DRM core.

> ---
>  Documentation/DocBook/gpu.tmpl | 11 +++++++++--
>  drivers/gpu/drm/drm_atomic.c   | 14 ++++++++++++++
>  drivers/gpu/drm/drm_crtc.c     |  5 +++++
>  include/drm/drm_crtc.h         | 20 ++++++++++++++++++++
>  4 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 351e801..2017162 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1816,7 +1816,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="37" valign="top" >DRM</td>
> +	<td rowspan="38" valign="top" >DRM</td>
>  	<td valign="top" >Generic</td>
>  	<td valign="top" >“rotation”</td>
>  	<td valign="top" >BITMASK</td>
> @@ -1868,7 +1868,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >CRTC that connector is attached to (atomic)</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="11" valign="top" >Plane</td>
> +	<td rowspan="12" valign="top" >Plane</td>
>  	<td valign="top" >“type”</td>
>  	<td valign="top" >ENUM | IMMUTABLE</td>
>  	<td valign="top" >{ "Overlay", "Primary", "Cursor" }</td>
> @@ -1946,6 +1946,13 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >CRTC that plane is attached to (atomic)</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" >“blend_func”</td>
> +	<td valign="top" >None</td>
> +	<td valign="top" >DRM_BLEND_FUNC()</td>

Shouldn't these two be type 'range' with values 0x0-0xFFFFFFFF?  Your
description field below would then explain the actual meaning of the
high/low 16-bits.

> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Source and destination blending factors</td>

I feel like a bit more detailed description here would be worthwhile.

Actually, given that we're presumably only using this via atomic, it
doesn't seem unreasonable to me to split this into two separate enum
properties.  That would be a little bit more self-explanatory and the
atomic check code could still ensure that we don't wind up with invalid
combinations of src/dest factors.  Plus it will make it a little easier
for us to add additional blending factors in the future.

> +	</tr>
> +	<tr>
>  	<td rowspan="2" valign="top" >DVI-I</td>
>  	<td valign="top" >“subconnector”</td>
>  	<td valign="top" >ENUM</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3f74193..0c165c6 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -630,6 +630,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->src_h = val;
>  	} else if (property == config->rotation_property) {
>  		state->rotation = val;
> +	} else if (property == config->prop_blend_func) {
> +		enum drm_blend_factor src_factor, dst_factor;
> +
> +		src_factor = DRM_BLEND_FUNC_SRC_FACTOR(val);
> +		dst_factor = DRM_BLEND_FUNC_DST_FACTOR(val);
> +
> +		if (src_factor != dst_factor &&
> +		    (src_factor == DRM_BLEND_FACTOR_AUTO ||
> +		     dst_factor == DRM_BLEND_FACTOR_AUTO))
> +			return -EINVAL;

Might be better to defer this until the atomic check phase since that's
where we do most of our other tests and where we expect to get failures.
This function is more about shoving values into state objects (and other
tasks like refcounting where necessary) without doing anything else.

> +
> +		state->blend_mode.func = val & GENMASK(31, 0);

This check should also move to the atomic check phase, but raise a
failure if we have any unwanted bits.  If we let userspace be sloppy
right now and put garbage there, then we can't really give those bits
new meaning in the future without breaking existing userspace.


>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -686,6 +698,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->src_h;
>  	} else if (property == config->rotation_property) {
>  		*val = state->rotation;
> +	} else if (property == config->prop_blend_func) {
> +		*val = state->blend_mode.func;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62fa95f..b8dde06 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1542,6 +1542,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_mode_id = prop;
>  
> +	prop = drm_property_create_range(dev, 0, "blend_func", 0, U32_MAX);

I'd add the DRM_MODE_PROP_ATOMIC flag to this.  I think we have a
direction that all new functionality only be exposed via the atomic
interfaces.


Matt

> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_blend_func = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c65a212..9cfe601 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -99,6 +99,23 @@ static inline uint64_t I642U64(int64_t val)
>  #define DRM_REFLECT_X	4
>  #define DRM_REFLECT_Y	5
>  
> +enum drm_blend_factor {
> +	DRM_BLEND_FACTOR_AUTO,
> +	DRM_BLEND_FACTOR_ZERO,
> +	DRM_BLEND_FACTOR_ONE,
> +	DRM_BLEND_FACTOR_SRC_ALPHA,
> +	DRM_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA,
> +};
> +
> +#define DRM_BLEND_FUNC(src_factor, dst_factor)		\
> +	(DRM_BLEND_FACTOR_##src_factor << 16 | DRM_BLEND_FACTOR_##dst_factor)
> +#define DRM_BLEND_FUNC_SRC_FACTOR(val)	(((val) >> 16) & 0xffff)
> +#define DRM_BLEND_FUNC_DST_FACTOR(val)	((val) & 0xffff)
> +
> +struct drm_blend_mode {
> +	uint64_t func;
> +};
> +
>  enum drm_connector_force {
>  	DRM_FORCE_UNSPECIFIED,
>  	DRM_FORCE_OFF,
> @@ -1267,6 +1284,8 @@ struct drm_plane_state {
>  	/* Plane rotation */
>  	unsigned int rotation;
>  
> +	struct drm_blend_mode blend_mode;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> @@ -2099,6 +2118,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  	struct drm_property *prop_mode_id;
> +	struct drm_property *prop_blend_func;
>  
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
> -- 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx