[v2,3/4] drm/i915: Enabling constant alpha drm property

Message ID	1395757947-16492-3-git-send-email-sagar.a.kamble@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: sagar.a.kamble@intel.com To: intel-gfx@lists.freedesktop.org Date: Tue, 25 Mar 2014 20:02:26 +0530 Message-Id: <1395757947-16492-3-git-send-email-sagar.a.kamble@intel.com> In-Reply-To: <1395757947-16492-1-git-send-email-sagar.a.kamble@intel.com> References: <20140320135151.GO6912@strange.amr.corp.intel.com> <1395757947-16492-1-git-send-email-sagar.a.kamble@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Sagar Kamble <sagar.a.kamble@intel.com> Subject: [Intel-gfx] [PATCH v2 3/4] drm/i915: Enabling constant alpha drm property Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

sagar.a.kamble@intel.com March 25, 2014, 2:32 p.m. UTC

From: Sagar Kamble <sagar.a.kamble@intel.com>

This patch enables constant alpha property for Sprite planes.
Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
for applying constant alpha on a plane. To disable constant alpha,
client has to set BIT(DRM_BLEND_SRC_COLOR)

v2: Fixing property value comparison in vlv_update_plane with the use
of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
Reverting line spacing change in i915_driver_lastclose. Return value
of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]

Testcase: igt/kms_blend
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_reg.h     |  2 ++
 drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
 drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 80 insertions(+), 1 deletion(-)

sagar.a.kamble@intel.com April 1, 2014, 4:51 a.m. UTC | #1

Gentle Reminder for reviewing this and related patches:
http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html


Also, provide views on "how do we handle pre-multiplied alpha pixel
formats or is there need to convey pre-multiplied alpha pixel format
through additional drm property?"
Since we are already advertising supported pre-multiplied formats
during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
etc.


On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> This patch enables constant alpha property for Sprite planes.
> Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> for applying constant alpha on a plane. To disable constant alpha,
> client has to set BIT(DRM_BLEND_SRC_COLOR)
> 
> v2: Fixing property value comparison in vlv_update_plane with the use
> of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> Reverting line spacing change in i915_driver_lastclose. Return value
> of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> 
> Testcase: igt/kms_blend
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
>  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
>  5 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e4d2b9f..e3a94a3 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  
> +	struct intel_plane *plane;
>  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
>  	 * goes right around and calls lastclose. Check for this and don't clean
>  	 * up anything. */
>  	if (!dev_priv)
>  		return;
>  
> +	if (dev_priv->blend_property) {
> +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> +			drm_object_property_set_value(&plane->base.base,
> +						dev_priv->blend_property,
> +						plane->blend_param.value);
> +		}
> +	}
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		intel_fbdev_restore_mode(dev);
>  		vga_switcheroo_process_delayed_switch();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 70fbe90..0d1be70 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
>  
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
> +	struct drm_property *blend_property;
>  
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6174fda..bfcfe72 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3774,6 +3774,8 @@ enum punit_power_well {
>  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
>  #define   SPRITE_TILED			(1<<10)
>  #define   SPRITE_DEST_KEY		(1<<2)
> +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
>  #define _SPRA_LINOFF		0x70284
>  #define _SPRA_STRIDE		0x70288
>  #define _SPRA_POS		0x7028c
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f4e0615..039a800 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -409,6 +409,13 @@ struct intel_plane {
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y;
>  	uint32_t src_w, src_h;
> +	union {
> +		uint64_t value;
> +		struct {
> +			unsigned int type;
> +			unsigned int factor;
> +		} details;
> +	} blend_param;
>  
>  	/* Since we need to change the watermarks before/after
>  	 * enabling/disabling the planes, we need to store the parameters here
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9436f18..9ec84bb 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  	u32 sprctl;
> +	u32 sprconstalpha;
> +	unsigned int blend_type, blend_factor;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	sprctl = I915_READ(SPCNTR(pipe, plane));
>  
> +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> +
>  	/* Mask out pixel format bits in case we change it */
>  	sprctl &= ~SP_PIXFORMAT_MASK;
>  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		break;
>  	}
>  
> +	/* Handle plane alpha and color blending properties */
> +	blend_type = intel_plane->blend_param.details.type;
> +	blend_factor = intel_plane->blend_param.details.factor;
> +
> +	switch (blend_type) {
> +	case BIT(DRM_BLEND_SRC_COLOR):
> +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> +		break;
> +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> +						SPRITE_CONSTANT_ALPHA_ENABLE);
> +		break;
> +	default:
> +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> +		break;
> +	}
> +
>  	/*
>  	 * Enable gamma to match primary/cursor plane behaviour.
>  	 * FIXME should be user controllable via propertiesa.
> @@ -1011,6 +1032,26 @@ out_unlock:
>  	return ret;
>  }
>  
> +static int intel_plane_set_property(struct drm_plane *plane,
> +				    struct drm_property *prop,
> +				    uint64_t val)
> +{
> +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	uint64_t old_val;
> +	int ret = -EINVAL;
> +
> +	if (prop == dev_priv->blend_property) {
> +		old_val = intel_plane->blend_param.value;
> +		intel_plane->blend_param.value = val;
> +		ret = intel_plane_restore(plane);
> +		if (ret)
> +			intel_plane->blend_param.value = old_val;
> +	}
> +
> +	return ret;
> +}
> +
>  int intel_plane_restore(struct drm_plane *plane)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
>  	.update_plane = intel_update_plane,
>  	.disable_plane = intel_disable_plane,
>  	.destroy = intel_destroy_plane,
> +	.set_property = intel_plane_set_property,
>  };
>  
>  static uint32_t ilk_plane_formats[] = {
> @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
>  int
>  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane;
>  	unsigned long possible_crtcs;
>  	const uint32_t *plane_formats;
> @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  
>  	intel_plane->pipe = pipe;
>  	intel_plane->plane = plane;
> +	/* Initialize drm properties for plane */
> +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> +	intel_plane->blend_param.details.factor = 0;
> +
>  	possible_crtcs = (1 << pipe);
>  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
>  			     &intel_plane_funcs,
>  			     plane_formats, num_plane_formats,
>  			     false);
> -	if (ret)
> +	if (ret) {
>  		kfree(intel_plane);
> +		goto out;
> +	}
> +
> +	if (!dev_priv->blend_property)
> +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> +							BIT(DRM_BLEND_SRC_COLOR) |
> +							BIT(DRM_BLEND_CONSTANT_ALPHA));
>  
> +	if (dev_priv->blend_property)
> +		drm_object_attach_property(&intel_plane->base.base,
> +				dev_priv->blend_property,
> +				intel_plane->blend_param.value);
> +out:
>  	return ret;
>  }

sagar.a.kamble@intel.com April 2, 2014, 6:12 a.m. UTC | #2

Reminder for review :)

On Tue, 2014-04-01 at 10:21 +0530, Sagar Arun Kamble wrote:
> Gentle Reminder for reviewing this and related patches:
> http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
> http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
> http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
> http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
> http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html
> 
> 
> Also, provide views on "how do we handle pre-multiplied alpha pixel
> formats or is there need to convey pre-multiplied alpha pixel format
> through additional drm property?"
> Since we are already advertising supported pre-multiplied formats
> during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
> etc.
> 
> 
> On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > 
> > This patch enables constant alpha property for Sprite planes.
> > Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> > for applying constant alpha on a plane. To disable constant alpha,
> > client has to set BIT(DRM_BLEND_SRC_COLOR)
> > 
> > v2: Fixing property value comparison in vlv_update_plane with the use
> > of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> > Reverting line spacing change in i915_driver_lastclose. Return value
> > of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> > 
> > Testcase: igt/kms_blend
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> >  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
> >  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
> >  5 files changed, 80 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e4d2b9f..e3a94a3 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  
> > +	struct intel_plane *plane;
> >  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
> >  	 * goes right around and calls lastclose. Check for this and don't clean
> >  	 * up anything. */
> >  	if (!dev_priv)
> >  		return;
> >  
> > +	if (dev_priv->blend_property) {
> > +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> > +			drm_object_property_set_value(&plane->base.base,
> > +						dev_priv->blend_property,
> > +						plane->blend_param.value);
> > +		}
> > +	}
> > +
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		intel_fbdev_restore_mode(dev);
> >  		vga_switcheroo_process_delayed_switch();
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 70fbe90..0d1be70 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
> >  
> >  	struct drm_property *broadcast_rgb_property;
> >  	struct drm_property *force_audio_property;
> > +	struct drm_property *blend_property;
> >  
> >  	uint32_t hw_context_size;
> >  	struct list_head context_list;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 6174fda..bfcfe72 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3774,6 +3774,8 @@ enum punit_power_well {
> >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> >  #define   SPRITE_TILED			(1<<10)
> >  #define   SPRITE_DEST_KEY		(1<<2)
> > +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> > +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
> >  #define _SPRA_LINOFF		0x70284
> >  #define _SPRA_STRIDE		0x70288
> >  #define _SPRA_POS		0x7028c
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f4e0615..039a800 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -409,6 +409,13 @@ struct intel_plane {
> >  	unsigned int crtc_w, crtc_h;
> >  	uint32_t src_x, src_y;
> >  	uint32_t src_w, src_h;
> > +	union {
> > +		uint64_t value;
> > +		struct {
> > +			unsigned int type;
> > +			unsigned int factor;
> > +		} details;
> > +	} blend_param;
> >  
> >  	/* Since we need to change the watermarks before/after
> >  	 * enabling/disabling the planes, we need to store the parameters here
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 9436f18..9ec84bb 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  	int pipe = intel_plane->pipe;
> >  	int plane = intel_plane->plane;
> >  	u32 sprctl;
> > +	u32 sprconstalpha;
> > +	unsigned int blend_type, blend_factor;
> >  	unsigned long sprsurf_offset, linear_offset;
> >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >  
> >  	sprctl = I915_READ(SPCNTR(pipe, plane));
> >  
> > +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> > +
> >  	/* Mask out pixel format bits in case we change it */
> >  	sprctl &= ~SP_PIXFORMAT_MASK;
> >  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> > @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  		break;
> >  	}
> >  
> > +	/* Handle plane alpha and color blending properties */
> > +	blend_type = intel_plane->blend_param.details.type;
> > +	blend_factor = intel_plane->blend_param.details.factor;
> > +
> > +	switch (blend_type) {
> > +	case BIT(DRM_BLEND_SRC_COLOR):
> > +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> > +		break;
> > +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> > +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> > +						SPRITE_CONSTANT_ALPHA_ENABLE);
> > +		break;
> > +	default:
> > +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> > +		break;
> > +	}
> > +
> >  	/*
> >  	 * Enable gamma to match primary/cursor plane behaviour.
> >  	 * FIXME should be user controllable via propertiesa.
> > @@ -1011,6 +1032,26 @@ out_unlock:
> >  	return ret;
> >  }
> >  
> > +static int intel_plane_set_property(struct drm_plane *plane,
> > +				    struct drm_property *prop,
> > +				    uint64_t val)
> > +{
> > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	uint64_t old_val;
> > +	int ret = -EINVAL;
> > +
> > +	if (prop == dev_priv->blend_property) {
> > +		old_val = intel_plane->blend_param.value;
> > +		intel_plane->blend_param.value = val;
> > +		ret = intel_plane_restore(plane);
> > +		if (ret)
> > +			intel_plane->blend_param.value = old_val;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  int intel_plane_restore(struct drm_plane *plane)
> >  {
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> >  	.update_plane = intel_update_plane,
> >  	.disable_plane = intel_disable_plane,
> >  	.destroy = intel_destroy_plane,
> > +	.set_property = intel_plane_set_property,
> >  };
> >  
> >  static uint32_t ilk_plane_formats[] = {
> > @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
> >  int
> >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  {
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane;
> >  	unsigned long possible_crtcs;
> >  	const uint32_t *plane_formats;
> > @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  
> >  	intel_plane->pipe = pipe;
> >  	intel_plane->plane = plane;
> > +	/* Initialize drm properties for plane */
> > +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> > +	intel_plane->blend_param.details.factor = 0;
> > +
> >  	possible_crtcs = (1 << pipe);
> >  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> >  			     &intel_plane_funcs,
> >  			     plane_formats, num_plane_formats,
> >  			     false);
> > -	if (ret)
> > +	if (ret) {
> >  		kfree(intel_plane);
> > +		goto out;
> > +	}
> > +
> > +	if (!dev_priv->blend_property)
> > +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> > +							BIT(DRM_BLEND_SRC_COLOR) |
> > +							BIT(DRM_BLEND_CONSTANT_ALPHA));
> >  
> > +	if (dev_priv->blend_property)
> > +		drm_object_attach_property(&intel_plane->base.base,
> > +				dev_priv->blend_property,
> > +				intel_plane->blend_param.value);
> > +out:
> >  	return ret;
> >  }
>

sagar.a.kamble@intel.com April 3, 2014, 5:43 a.m. UTC | #3

Hi Ville,

Could you please review these patches.

thanks,
Sagar


On Wed, 2014-04-02 at 11:42 +0530, Sagar Arun Kamble wrote:
> Reminder for review :)
> 
> On Tue, 2014-04-01 at 10:21 +0530, Sagar Arun Kamble wrote:
> > Gentle Reminder for reviewing this and related patches:
> > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
> > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
> > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
> > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
> > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html
> > 
> > 
> > Also, provide views on "how do we handle pre-multiplied alpha pixel
> > formats or is there need to convey pre-multiplied alpha pixel format
> > through additional drm property?"
> > Since we are already advertising supported pre-multiplied formats
> > during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
> > etc.
> > 
> > 
> > On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > 
> > > This patch enables constant alpha property for Sprite planes.
> > > Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> > > for applying constant alpha on a plane. To disable constant alpha,
> > > client has to set BIT(DRM_BLEND_SRC_COLOR)
> > > 
> > > v2: Fixing property value comparison in vlv_update_plane with the use
> > > of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> > > Reverting line spacing change in i915_driver_lastclose. Return value
> > > of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> > > 
> > > Testcase: igt/kms_blend
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > > Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
> > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> > >  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
> > >  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
> > >  5 files changed, 80 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index e4d2b9f..e3a94a3 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
> > >  {
> > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > >  
> > > +	struct intel_plane *plane;
> > >  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
> > >  	 * goes right around and calls lastclose. Check for this and don't clean
> > >  	 * up anything. */
> > >  	if (!dev_priv)
> > >  		return;
> > >  
> > > +	if (dev_priv->blend_property) {
> > > +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > > +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> > > +			drm_object_property_set_value(&plane->base.base,
> > > +						dev_priv->blend_property,
> > > +						plane->blend_param.value);
> > > +		}
> > > +	}
> > > +
> > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > >  		intel_fbdev_restore_mode(dev);
> > >  		vga_switcheroo_process_delayed_switch();
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 70fbe90..0d1be70 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
> > >  
> > >  	struct drm_property *broadcast_rgb_property;
> > >  	struct drm_property *force_audio_property;
> > > +	struct drm_property *blend_property;
> > >  
> > >  	uint32_t hw_context_size;
> > >  	struct list_head context_list;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 6174fda..bfcfe72 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3774,6 +3774,8 @@ enum punit_power_well {
> > >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> > >  #define   SPRITE_TILED			(1<<10)
> > >  #define   SPRITE_DEST_KEY		(1<<2)
> > > +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> > > +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
> > >  #define _SPRA_LINOFF		0x70284
> > >  #define _SPRA_STRIDE		0x70288
> > >  #define _SPRA_POS		0x7028c
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index f4e0615..039a800 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -409,6 +409,13 @@ struct intel_plane {
> > >  	unsigned int crtc_w, crtc_h;
> > >  	uint32_t src_x, src_y;
> > >  	uint32_t src_w, src_h;
> > > +	union {
> > > +		uint64_t value;
> > > +		struct {
> > > +			unsigned int type;
> > > +			unsigned int factor;
> > > +		} details;
> > > +	} blend_param;
> > >  
> > >  	/* Since we need to change the watermarks before/after
> > >  	 * enabling/disabling the planes, we need to store the parameters here
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 9436f18..9ec84bb 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > >  	int pipe = intel_plane->pipe;
> > >  	int plane = intel_plane->plane;
> > >  	u32 sprctl;
> > > +	u32 sprconstalpha;
> > > +	unsigned int blend_type, blend_factor;
> > >  	unsigned long sprsurf_offset, linear_offset;
> > >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > >  
> > >  	sprctl = I915_READ(SPCNTR(pipe, plane));
> > >  
> > > +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> > > +
> > >  	/* Mask out pixel format bits in case we change it */
> > >  	sprctl &= ~SP_PIXFORMAT_MASK;
> > >  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> > > @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > >  		break;
> > >  	}
> > >  
> > > +	/* Handle plane alpha and color blending properties */
> > > +	blend_type = intel_plane->blend_param.details.type;
> > > +	blend_factor = intel_plane->blend_param.details.factor;
> > > +
> > > +	switch (blend_type) {
> > > +	case BIT(DRM_BLEND_SRC_COLOR):
> > > +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> > > +		break;
> > > +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> > > +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> > > +						SPRITE_CONSTANT_ALPHA_ENABLE);
> > > +		break;
> > > +	default:
> > > +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> > > +		break;
> > > +	}
> > > +
> > >  	/*
> > >  	 * Enable gamma to match primary/cursor plane behaviour.
> > >  	 * FIXME should be user controllable via propertiesa.
> > > @@ -1011,6 +1032,26 @@ out_unlock:
> > >  	return ret;
> > >  }
> > >  
> > > +static int intel_plane_set_property(struct drm_plane *plane,
> > > +				    struct drm_property *prop,
> > > +				    uint64_t val)
> > > +{
> > > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > +	uint64_t old_val;
> > > +	int ret = -EINVAL;
> > > +
> > > +	if (prop == dev_priv->blend_property) {
> > > +		old_val = intel_plane->blend_param.value;
> > > +		intel_plane->blend_param.value = val;
> > > +		ret = intel_plane_restore(plane);
> > > +		if (ret)
> > > +			intel_plane->blend_param.value = old_val;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  int intel_plane_restore(struct drm_plane *plane)
> > >  {
> > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> > >  	.update_plane = intel_update_plane,
> > >  	.disable_plane = intel_disable_plane,
> > >  	.destroy = intel_destroy_plane,
> > > +	.set_property = intel_plane_set_property,
> > >  };
> > >  
> > >  static uint32_t ilk_plane_formats[] = {
> > > @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
> > >  int
> > >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > >  {
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct intel_plane *intel_plane;
> > >  	unsigned long possible_crtcs;
> > >  	const uint32_t *plane_formats;
> > > @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > >  
> > >  	intel_plane->pipe = pipe;
> > >  	intel_plane->plane = plane;
> > > +	/* Initialize drm properties for plane */
> > > +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> > > +	intel_plane->blend_param.details.factor = 0;
> > > +
> > >  	possible_crtcs = (1 << pipe);
> > >  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> > >  			     &intel_plane_funcs,
> > >  			     plane_formats, num_plane_formats,
> > >  			     false);
> > > -	if (ret)
> > > +	if (ret) {
> > >  		kfree(intel_plane);
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!dev_priv->blend_property)
> > > +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> > > +							BIT(DRM_BLEND_SRC_COLOR) |
> > > +							BIT(DRM_BLEND_CONSTANT_ALPHA));
> > >  
> > > +	if (dev_priv->blend_property)
> > > +		drm_object_attach_property(&intel_plane->base.base,
> > > +				dev_priv->blend_property,
> > > +				intel_plane->blend_param.value);
> > > +out:
> > >  	return ret;
> > >  }
> > 
>

sagar.a.kamble@intel.com April 15, 2014, 9:44 a.m. UTC | #4

Hi Ville,

Somehow I did not receive your review comments
(http://lists.freedesktop.org/archives/intel-gfx/2014-April/042956.html
) in my mailbox.

Here is my input w.r.t patches I have sent for review:

Currently my patches are modeling constant alpha blend factor (assuming
pre-multiplied format) as 

Dc = Sc * Ca + (1 - Ca) * Dc

User space has to supply (<alpha value>, CONSTANT_ALPHA) through drm
property to invoke this blending.

Here are my queries on your proposed approach:

1. src factors are 1, Ca, Sa*Ca and dst factors are 0, 1-Ca, 1-Sa,
1-Sa*Ca: How is factor 0 derived and applicable to Dst and not
applicable to src.

2. With this approach user will know what blend equation is supported.
How does user supply the blend parameters? Should we have another
property that will have blend equation selected and corresponding
parameters?

3. Is flag suggested for pre-multiply ("yes please, premultiply the data
for me") to be set during AddFB?

Kindly check my patches and provide feedback on the approach of stuffing
blend type and blend parameter into single 64bit drm property.


Thanks,
Sagar




On Thu, 2014-04-03 at 11:13 +0530, Sagar Arun Kamble wrote:
> Hi Ville,
> 
> Could you please review these patches.
> 
> thanks,
> Sagar
> 
> 
> On Wed, 2014-04-02 at 11:42 +0530, Sagar Arun Kamble wrote:
> > Reminder for review :)
> > 
> > On Tue, 2014-04-01 at 10:21 +0530, Sagar Arun Kamble wrote:
> > > Gentle Reminder for reviewing this and related patches:
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html
> > > 
> > > 
> > > Also, provide views on "how do we handle pre-multiplied alpha pixel
> > > formats or is there need to convey pre-multiplied alpha pixel format
> > > through additional drm property?"
> > > Since we are already advertising supported pre-multiplied formats
> > > during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
> > > etc.
> > > 
> > > 
> > > On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > 
> > > > This patch enables constant alpha property for Sprite planes.
> > > > Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> > > > for applying constant alpha on a plane. To disable constant alpha,
> > > > client has to set BIT(DRM_BLEND_SRC_COLOR)
> > > > 
> > > > v2: Fixing property value comparison in vlv_update_plane with the use
> > > > of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> > > > Reverting line spacing change in i915_driver_lastclose. Return value
> > > > of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> > > > 
> > > > Testcase: igt/kms_blend
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
> > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > >  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> > > >  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
> > > >  5 files changed, 80 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > index e4d2b9f..e3a94a3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
> > > >  {
> > > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > > >  
> > > > +	struct intel_plane *plane;
> > > >  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
> > > >  	 * goes right around and calls lastclose. Check for this and don't clean
> > > >  	 * up anything. */
> > > >  	if (!dev_priv)
> > > >  		return;
> > > >  
> > > > +	if (dev_priv->blend_property) {
> > > > +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > > > +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> > > > +			drm_object_property_set_value(&plane->base.base,
> > > > +						dev_priv->blend_property,
> > > > +						plane->blend_param.value);
> > > > +		}
> > > > +	}
> > > > +
> > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > >  		intel_fbdev_restore_mode(dev);
> > > >  		vga_switcheroo_process_delayed_switch();
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 70fbe90..0d1be70 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
> > > >  
> > > >  	struct drm_property *broadcast_rgb_property;
> > > >  	struct drm_property *force_audio_property;
> > > > +	struct drm_property *blend_property;
> > > >  
> > > >  	uint32_t hw_context_size;
> > > >  	struct list_head context_list;
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 6174fda..bfcfe72 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -3774,6 +3774,8 @@ enum punit_power_well {
> > > >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> > > >  #define   SPRITE_TILED			(1<<10)
> > > >  #define   SPRITE_DEST_KEY		(1<<2)
> > > > +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> > > > +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
> > > >  #define _SPRA_LINOFF		0x70284
> > > >  #define _SPRA_STRIDE		0x70288
> > > >  #define _SPRA_POS		0x7028c
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index f4e0615..039a800 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -409,6 +409,13 @@ struct intel_plane {
> > > >  	unsigned int crtc_w, crtc_h;
> > > >  	uint32_t src_x, src_y;
> > > >  	uint32_t src_w, src_h;
> > > > +	union {
> > > > +		uint64_t value;
> > > > +		struct {
> > > > +			unsigned int type;
> > > > +			unsigned int factor;
> > > > +		} details;
> > > > +	} blend_param;
> > > >  
> > > >  	/* Since we need to change the watermarks before/after
> > > >  	 * enabling/disabling the planes, we need to store the parameters here
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 9436f18..9ec84bb 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > >  	int pipe = intel_plane->pipe;
> > > >  	int plane = intel_plane->plane;
> > > >  	u32 sprctl;
> > > > +	u32 sprconstalpha;
> > > > +	unsigned int blend_type, blend_factor;
> > > >  	unsigned long sprsurf_offset, linear_offset;
> > > >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > > >  
> > > >  	sprctl = I915_READ(SPCNTR(pipe, plane));
> > > >  
> > > > +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> > > > +
> > > >  	/* Mask out pixel format bits in case we change it */
> > > >  	sprctl &= ~SP_PIXFORMAT_MASK;
> > > >  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> > > > @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > >  		break;
> > > >  	}
> > > >  
> > > > +	/* Handle plane alpha and color blending properties */
> > > > +	blend_type = intel_plane->blend_param.details.type;
> > > > +	blend_factor = intel_plane->blend_param.details.factor;
> > > > +
> > > > +	switch (blend_type) {
> > > > +	case BIT(DRM_BLEND_SRC_COLOR):
> > > > +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > +		break;
> > > > +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> > > > +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> > > > +						SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > +		break;
> > > > +	default:
> > > > +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> > > > +		break;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * Enable gamma to match primary/cursor plane behaviour.
> > > >  	 * FIXME should be user controllable via propertiesa.
> > > > @@ -1011,6 +1032,26 @@ out_unlock:
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static int intel_plane_set_property(struct drm_plane *plane,
> > > > +				    struct drm_property *prop,
> > > > +				    uint64_t val)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > +	uint64_t old_val;
> > > > +	int ret = -EINVAL;
> > > > +
> > > > +	if (prop == dev_priv->blend_property) {
> > > > +		old_val = intel_plane->blend_param.value;
> > > > +		intel_plane->blend_param.value = val;
> > > > +		ret = intel_plane_restore(plane);
> > > > +		if (ret)
> > > > +			intel_plane->blend_param.value = old_val;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  int intel_plane_restore(struct drm_plane *plane)
> > > >  {
> > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> > > >  	.update_plane = intel_update_plane,
> > > >  	.disable_plane = intel_disable_plane,
> > > >  	.destroy = intel_destroy_plane,
> > > > +	.set_property = intel_plane_set_property,
> > > >  };
> > > >  
> > > >  static uint32_t ilk_plane_formats[] = {
> > > > @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
> > > >  int
> > > >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > >  {
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	struct intel_plane *intel_plane;
> > > >  	unsigned long possible_crtcs;
> > > >  	const uint32_t *plane_formats;
> > > > @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > >  
> > > >  	intel_plane->pipe = pipe;
> > > >  	intel_plane->plane = plane;
> > > > +	/* Initialize drm properties for plane */
> > > > +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> > > > +	intel_plane->blend_param.details.factor = 0;
> > > > +
> > > >  	possible_crtcs = (1 << pipe);
> > > >  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> > > >  			     &intel_plane_funcs,
> > > >  			     plane_formats, num_plane_formats,
> > > >  			     false);
> > > > -	if (ret)
> > > > +	if (ret) {
> > > >  		kfree(intel_plane);
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!dev_priv->blend_property)
> > > > +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> > > > +							BIT(DRM_BLEND_SRC_COLOR) |
> > > > +							BIT(DRM_BLEND_CONSTANT_ALPHA));
> > > >  
> > > > +	if (dev_priv->blend_property)
> > > > +		drm_object_attach_property(&intel_plane->base.base,
> > > > +				dev_priv->blend_property,
> > > > +				intel_plane->blend_param.value);
> > > > +out:
> > > >  	return ret;
> > > >  }
> > > 
> > 
>

Ville Syrjälä April 15, 2014, 10:35 a.m. UTC | #5

On Tue, Apr 15, 2014 at 03:14:04PM +0530, Sagar Arun Kamble wrote:
> Hi Ville,
> 
> Somehow I did not receive your review comments
> (http://lists.freedesktop.org/archives/intel-gfx/2014-April/042956.html
> ) in my mailbox.
> 
> Here is my input w.r.t patches I have sent for review:
> 
> Currently my patches are modeling constant alpha blend factor (assuming
> pre-multiplied format) as 
> 
> Dc = Sc * Ca + (1 - Ca) * Dc
> 
> User space has to supply (<alpha value>, CONSTANT_ALPHA) through drm
> property to invoke this blending.
> 
> Here are my queries on your proposed approach:
> 
> 1. src factors are 1, Ca, Sa*Ca and dst factors are 0, 1-Ca, 1-Sa,
> 1-Sa*Ca: How is factor 0 derived and applicable to Dst and not
> applicable to src.

If blending is disabled the function will be Dc = Sc * 1 + 0 * Dc

Hence we get the ONE and ZERO blend factors.

> 
> 2. With this approach user will know what blend equation is supported.
> How does user supply the blend parameters? Should we have another
> property that will have blend equation selected and corresponding
> parameters?

By parameters you mean the constant alpha and background color for
instance? I think having those as separate properties is the cleanest
option.

> 
> 3. Is flag suggested for pre-multiply ("yes please, premultiply the data
> for me") to be set during AddFB?

If we define the flag that way, then I think we should make it part of
the blend equation rather than apply it to the FB. Or it could even be
a separate plane property. 

> 
> Kindly check my patches and provide feedback on the approach of stuffing
> blend type and blend parameter into single 64bit drm property.

I think we should avoid trying to stuff too much data into a single
property. IMO blend equation can be one property, constant alpha,
background color, pre-multiplication may be other properties.

> 
> 
> Thanks,
> Sagar
> 
> 
> 
> 
> On Thu, 2014-04-03 at 11:13 +0530, Sagar Arun Kamble wrote:
> > Hi Ville,
> > 
> > Could you please review these patches.
> > 
> > thanks,
> > Sagar
> > 
> > 
> > On Wed, 2014-04-02 at 11:42 +0530, Sagar Arun Kamble wrote:
> > > Reminder for review :)
> > > 
> > > On Tue, 2014-04-01 at 10:21 +0530, Sagar Arun Kamble wrote:
> > > > Gentle Reminder for reviewing this and related patches:
> > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
> > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
> > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
> > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
> > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html
> > > > 
> > > > 
> > > > Also, provide views on "how do we handle pre-multiplied alpha pixel
> > > > formats or is there need to convey pre-multiplied alpha pixel format
> > > > through additional drm property?"
> > > > Since we are already advertising supported pre-multiplied formats
> > > > during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
> > > > etc.
> > > > 
> > > > 
> > > > On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > 
> > > > > This patch enables constant alpha property for Sprite planes.
> > > > > Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> > > > > for applying constant alpha on a plane. To disable constant alpha,
> > > > > client has to set BIT(DRM_BLEND_SRC_COLOR)
> > > > > 
> > > > > v2: Fixing property value comparison in vlv_update_plane with the use
> > > > > of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> > > > > Reverting line spacing change in i915_driver_lastclose. Return value
> > > > > of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> > > > > 
> > > > > Testcase: igt/kms_blend
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
> > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> > > > >  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
> > > > >  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
> > > > >  5 files changed, 80 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > > index e4d2b9f..e3a94a3 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > > @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
> > > > >  {
> > > > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > > > >  
> > > > > +	struct intel_plane *plane;
> > > > >  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
> > > > >  	 * goes right around and calls lastclose. Check for this and don't clean
> > > > >  	 * up anything. */
> > > > >  	if (!dev_priv)
> > > > >  		return;
> > > > >  
> > > > > +	if (dev_priv->blend_property) {
> > > > > +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > > > > +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> > > > > +			drm_object_property_set_value(&plane->base.base,
> > > > > +						dev_priv->blend_property,
> > > > > +						plane->blend_param.value);
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > >  		intel_fbdev_restore_mode(dev);
> > > > >  		vga_switcheroo_process_delayed_switch();
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 70fbe90..0d1be70 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
> > > > >  
> > > > >  	struct drm_property *broadcast_rgb_property;
> > > > >  	struct drm_property *force_audio_property;
> > > > > +	struct drm_property *blend_property;
> > > > >  
> > > > >  	uint32_t hw_context_size;
> > > > >  	struct list_head context_list;
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 6174fda..bfcfe72 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -3774,6 +3774,8 @@ enum punit_power_well {
> > > > >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> > > > >  #define   SPRITE_TILED			(1<<10)
> > > > >  #define   SPRITE_DEST_KEY		(1<<2)
> > > > > +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> > > > > +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
> > > > >  #define _SPRA_LINOFF		0x70284
> > > > >  #define _SPRA_STRIDE		0x70288
> > > > >  #define _SPRA_POS		0x7028c
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index f4e0615..039a800 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -409,6 +409,13 @@ struct intel_plane {
> > > > >  	unsigned int crtc_w, crtc_h;
> > > > >  	uint32_t src_x, src_y;
> > > > >  	uint32_t src_w, src_h;
> > > > > +	union {
> > > > > +		uint64_t value;
> > > > > +		struct {
> > > > > +			unsigned int type;
> > > > > +			unsigned int factor;
> > > > > +		} details;
> > > > > +	} blend_param;
> > > > >  
> > > > >  	/* Since we need to change the watermarks before/after
> > > > >  	 * enabling/disabling the planes, we need to store the parameters here
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index 9436f18..9ec84bb 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > > >  	int pipe = intel_plane->pipe;
> > > > >  	int plane = intel_plane->plane;
> > > > >  	u32 sprctl;
> > > > > +	u32 sprconstalpha;
> > > > > +	unsigned int blend_type, blend_factor;
> > > > >  	unsigned long sprsurf_offset, linear_offset;
> > > > >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > > > >  
> > > > >  	sprctl = I915_READ(SPCNTR(pipe, plane));
> > > > >  
> > > > > +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> > > > > +
> > > > >  	/* Mask out pixel format bits in case we change it */
> > > > >  	sprctl &= ~SP_PIXFORMAT_MASK;
> > > > >  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> > > > > @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > > >  		break;
> > > > >  	}
> > > > >  
> > > > > +	/* Handle plane alpha and color blending properties */
> > > > > +	blend_type = intel_plane->blend_param.details.type;
> > > > > +	blend_factor = intel_plane->blend_param.details.factor;
> > > > > +
> > > > > +	switch (blend_type) {
> > > > > +	case BIT(DRM_BLEND_SRC_COLOR):
> > > > > +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > > +		break;
> > > > > +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> > > > > +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> > > > > +						SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > > +		break;
> > > > > +	default:
> > > > > +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > >  	/*
> > > > >  	 * Enable gamma to match primary/cursor plane behaviour.
> > > > >  	 * FIXME should be user controllable via propertiesa.
> > > > > @@ -1011,6 +1032,26 @@ out_unlock:
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > +static int intel_plane_set_property(struct drm_plane *plane,
> > > > > +				    struct drm_property *prop,
> > > > > +				    uint64_t val)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > > > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > +	uint64_t old_val;
> > > > > +	int ret = -EINVAL;
> > > > > +
> > > > > +	if (prop == dev_priv->blend_property) {
> > > > > +		old_val = intel_plane->blend_param.value;
> > > > > +		intel_plane->blend_param.value = val;
> > > > > +		ret = intel_plane_restore(plane);
> > > > > +		if (ret)
> > > > > +			intel_plane->blend_param.value = old_val;
> > > > > +	}
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > >  int intel_plane_restore(struct drm_plane *plane)
> > > > >  {
> > > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> > > > >  	.update_plane = intel_update_plane,
> > > > >  	.disable_plane = intel_disable_plane,
> > > > >  	.destroy = intel_destroy_plane,
> > > > > +	.set_property = intel_plane_set_property,
> > > > >  };
> > > > >  
> > > > >  static uint32_t ilk_plane_formats[] = {
> > > > > @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
> > > > >  int
> > > > >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > >  {
> > > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > >  	struct intel_plane *intel_plane;
> > > > >  	unsigned long possible_crtcs;
> > > > >  	const uint32_t *plane_formats;
> > > > > @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > >  
> > > > >  	intel_plane->pipe = pipe;
> > > > >  	intel_plane->plane = plane;
> > > > > +	/* Initialize drm properties for plane */
> > > > > +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> > > > > +	intel_plane->blend_param.details.factor = 0;
> > > > > +
> > > > >  	possible_crtcs = (1 << pipe);
> > > > >  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> > > > >  			     &intel_plane_funcs,
> > > > >  			     plane_formats, num_plane_formats,
> > > > >  			     false);
> > > > > -	if (ret)
> > > > > +	if (ret) {
> > > > >  		kfree(intel_plane);
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	if (!dev_priv->blend_property)
> > > > > +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> > > > > +							BIT(DRM_BLEND_SRC_COLOR) |
> > > > > +							BIT(DRM_BLEND_CONSTANT_ALPHA));
> > > > >  
> > > > > +	if (dev_priv->blend_property)
> > > > > +		drm_object_attach_property(&intel_plane->base.base,
> > > > > +				dev_priv->blend_property,
> > > > > +				intel_plane->blend_param.value);
> > > > > +out:
> > > > >  	return ret;
> > > > >  }
> > > > 
> > > 
> > 
>

sagar.a.kamble@intel.com April 15, 2014, 11:23 a.m. UTC | #6

Hi Ville,

Thanks for the reply.
I have few more queries.
On Tue, 2014-04-15 at 13:35 +0300, Ville Syrjälä wrote:
> On Tue, Apr 15, 2014 at 03:14:04PM +0530, Sagar Arun Kamble wrote:
> > Hi Ville,
> > 
> > Somehow I did not receive your review comments
> > (http://lists.freedesktop.org/archives/intel-gfx/2014-April/042956.html
> > ) in my mailbox.
> > 
> > Here is my input w.r.t patches I have sent for review:
> > 
> > Currently my patches are modeling constant alpha blend factor (assuming
> > pre-multiplied format) as 
> > 
> > Dc = Sc * Ca + (1 - Ca) * Dc
> > 
> > User space has to supply (<alpha value>, CONSTANT_ALPHA) through drm
> > property to invoke this blending.
> > 
> > Here are my queries on your proposed approach:
> > 
> > 1. src factors are 1, Ca, Sa*Ca and dst factors are 0, 1-Ca, 1-Sa,
> > 1-Sa*Ca: How is factor 0 derived and applicable to Dst and not
> > applicable to src.
> 
> If blending is disabled the function will be Dc = Sc * 1 + 0 * Dc
> 
> Hence we get the ONE and ZERO blend factors.
> 
> > 
> > 2. With this approach user will know what blend equation is supported.
> > How does user supply the blend parameters? Should we have another
> > property that will have blend equation selected and corresponding
> > parameters?
> 
> By parameters you mean the constant alpha and background color for
> instance? I think having those as separate properties is the cleanest
> option.
Yes. I meant constant alpha value. For what type of blending background
color will be used?

So, the overall flow will be:

1. User queries supported blending equations.
2. Sets required blend equation.
3. Sets required blend parameters like constant alpha value etc.

Step 2 and 3 need to be done atomically.

> 
> > 
> > 3. Is flag suggested for pre-multiply ("yes please, premultiply the data
> > for me") to be set during AddFB?
> 
> If we define the flag that way, then I think we should make it part of
> the blend equation rather than apply it to the FB. Or it could even be
> a separate plane property. 
> 
This flag is actually part of following factors: SRC_ALPHA,
ONE_MINUS_SRC_ALPHA,SRC_CONST_ALPHA, ONE_MINUS_SRC_CONST_ALPHA.

What action does driver take if user selects these blend factors?
Change the pixel format to non-pre-multiplied format? This is again
coming back to our previous discussion where changing pixel format on
fly should not be done.

 
> > 
> > Kindly check my patches and provide feedback on the approach of stuffing
> > blend type and blend parameter into single 64bit drm property.
> 
> I think we should avoid trying to stuff too much data into a single
> property. IMO blend equation can be one property, constant alpha,
> background color, pre-multiplication may be other properties.
> 
> > 
> > 
> > Thanks,
> > Sagar
> > 
> > 
> > 
> > 
> > On Thu, 2014-04-03 at 11:13 +0530, Sagar Arun Kamble wrote:
> > > Hi Ville,
> > > 
> > > Could you please review these patches.
> > > 
> > > thanks,
> > > Sagar
> > > 
> > > 
> > > On Wed, 2014-04-02 at 11:42 +0530, Sagar Arun Kamble wrote:
> > > > Reminder for review :)
> > > > 
> > > > On Tue, 2014-04-01 at 10:21 +0530, Sagar Arun Kamble wrote:
> > > > > Gentle Reminder for reviewing this and related patches:
> > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
> > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
> > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
> > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
> > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html
> > > > > 
> > > > > 
> > > > > Also, provide views on "how do we handle pre-multiplied alpha pixel
> > > > > formats or is there need to convey pre-multiplied alpha pixel format
> > > > > through additional drm property?"
> > > > > Since we are already advertising supported pre-multiplied formats
> > > > > during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
> > > > > etc.
> > > > > 
> > > > > 
> > > > > On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > 
> > > > > > This patch enables constant alpha property for Sprite planes.
> > > > > > Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> > > > > > for applying constant alpha on a plane. To disable constant alpha,
> > > > > > client has to set BIT(DRM_BLEND_SRC_COLOR)
> > > > > > 
> > > > > > v2: Fixing property value comparison in vlv_update_plane with the use
> > > > > > of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> > > > > > Reverting line spacing change in i915_driver_lastclose. Return value
> > > > > > of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> > > > > > 
> > > > > > Testcase: igt/kms_blend
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
> > > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > > >  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> > > > > >  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
> > > > > >  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
> > > > > >  5 files changed, 80 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > > > index e4d2b9f..e3a94a3 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > > > @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
> > > > > >  {
> > > > > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > > > > >  
> > > > > > +	struct intel_plane *plane;
> > > > > >  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
> > > > > >  	 * goes right around and calls lastclose. Check for this and don't clean
> > > > > >  	 * up anything. */
> > > > > >  	if (!dev_priv)
> > > > > >  		return;
> > > > > >  
> > > > > > +	if (dev_priv->blend_property) {
> > > > > > +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > > > > > +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> > > > > > +			drm_object_property_set_value(&plane->base.base,
> > > > > > +						dev_priv->blend_property,
> > > > > > +						plane->blend_param.value);
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > > >  		intel_fbdev_restore_mode(dev);
> > > > > >  		vga_switcheroo_process_delayed_switch();
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 70fbe90..0d1be70 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
> > > > > >  
> > > > > >  	struct drm_property *broadcast_rgb_property;
> > > > > >  	struct drm_property *force_audio_property;
> > > > > > +	struct drm_property *blend_property;
> > > > > >  
> > > > > >  	uint32_t hw_context_size;
> > > > > >  	struct list_head context_list;
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 6174fda..bfcfe72 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -3774,6 +3774,8 @@ enum punit_power_well {
> > > > > >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> > > > > >  #define   SPRITE_TILED			(1<<10)
> > > > > >  #define   SPRITE_DEST_KEY		(1<<2)
> > > > > > +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> > > > > > +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
> > > > > >  #define _SPRA_LINOFF		0x70284
> > > > > >  #define _SPRA_STRIDE		0x70288
> > > > > >  #define _SPRA_POS		0x7028c
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index f4e0615..039a800 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -409,6 +409,13 @@ struct intel_plane {
> > > > > >  	unsigned int crtc_w, crtc_h;
> > > > > >  	uint32_t src_x, src_y;
> > > > > >  	uint32_t src_w, src_h;
> > > > > > +	union {
> > > > > > +		uint64_t value;
> > > > > > +		struct {
> > > > > > +			unsigned int type;
> > > > > > +			unsigned int factor;
> > > > > > +		} details;
> > > > > > +	} blend_param;
> > > > > >  
> > > > > >  	/* Since we need to change the watermarks before/after
> > > > > >  	 * enabling/disabling the planes, we need to store the parameters here
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > index 9436f18..9ec84bb 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > > > >  	int pipe = intel_plane->pipe;
> > > > > >  	int plane = intel_plane->plane;
> > > > > >  	u32 sprctl;
> > > > > > +	u32 sprconstalpha;
> > > > > > +	unsigned int blend_type, blend_factor;
> > > > > >  	unsigned long sprsurf_offset, linear_offset;
> > > > > >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > > > > >  
> > > > > >  	sprctl = I915_READ(SPCNTR(pipe, plane));
> > > > > >  
> > > > > > +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> > > > > > +
> > > > > >  	/* Mask out pixel format bits in case we change it */
> > > > > >  	sprctl &= ~SP_PIXFORMAT_MASK;
> > > > > >  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> > > > > > @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > > > >  		break;
> > > > > >  	}
> > > > > >  
> > > > > > +	/* Handle plane alpha and color blending properties */
> > > > > > +	blend_type = intel_plane->blend_param.details.type;
> > > > > > +	blend_factor = intel_plane->blend_param.details.factor;
> > > > > > +
> > > > > > +	switch (blend_type) {
> > > > > > +	case BIT(DRM_BLEND_SRC_COLOR):
> > > > > > +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > > > +		break;
> > > > > > +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> > > > > > +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> > > > > > +						SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > > > +		break;
> > > > > > +	default:
> > > > > > +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> > > > > > +		break;
> > > > > > +	}
> > > > > > +
> > > > > >  	/*
> > > > > >  	 * Enable gamma to match primary/cursor plane behaviour.
> > > > > >  	 * FIXME should be user controllable via propertiesa.
> > > > > > @@ -1011,6 +1032,26 @@ out_unlock:
> > > > > >  	return ret;
> > > > > >  }
> > > > > >  
> > > > > > +static int intel_plane_set_property(struct drm_plane *plane,
> > > > > > +				    struct drm_property *prop,
> > > > > > +				    uint64_t val)
> > > > > > +{
> > > > > > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > > > > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > > +	uint64_t old_val;
> > > > > > +	int ret = -EINVAL;
> > > > > > +
> > > > > > +	if (prop == dev_priv->blend_property) {
> > > > > > +		old_val = intel_plane->blend_param.value;
> > > > > > +		intel_plane->blend_param.value = val;
> > > > > > +		ret = intel_plane_restore(plane);
> > > > > > +		if (ret)
> > > > > > +			intel_plane->blend_param.value = old_val;
> > > > > > +	}
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > >  int intel_plane_restore(struct drm_plane *plane)
> > > > > >  {
> > > > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > > @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> > > > > >  	.update_plane = intel_update_plane,
> > > > > >  	.disable_plane = intel_disable_plane,
> > > > > >  	.destroy = intel_destroy_plane,
> > > > > > +	.set_property = intel_plane_set_property,
> > > > > >  };
> > > > > >  
> > > > > >  static uint32_t ilk_plane_formats[] = {
> > > > > > @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
> > > > > >  int
> > > > > >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > > >  {
> > > > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > >  	struct intel_plane *intel_plane;
> > > > > >  	unsigned long possible_crtcs;
> > > > > >  	const uint32_t *plane_formats;
> > > > > > @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > > >  
> > > > > >  	intel_plane->pipe = pipe;
> > > > > >  	intel_plane->plane = plane;
> > > > > > +	/* Initialize drm properties for plane */
> > > > > > +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> > > > > > +	intel_plane->blend_param.details.factor = 0;
> > > > > > +
> > > > > >  	possible_crtcs = (1 << pipe);
> > > > > >  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> > > > > >  			     &intel_plane_funcs,
> > > > > >  			     plane_formats, num_plane_formats,
> > > > > >  			     false);
> > > > > > -	if (ret)
> > > > > > +	if (ret) {
> > > > > >  		kfree(intel_plane);
> > > > > > +		goto out;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (!dev_priv->blend_property)
> > > > > > +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> > > > > > +							BIT(DRM_BLEND_SRC_COLOR) |
> > > > > > +							BIT(DRM_BLEND_CONSTANT_ALPHA));
> > > > > >  
> > > > > > +	if (dev_priv->blend_property)
> > > > > > +		drm_object_attach_property(&intel_plane->base.base,
> > > > > > +				dev_priv->blend_property,
> > > > > > +				intel_plane->blend_param.value);
> > > > > > +out:
> > > > > >  	return ret;
> > > > > >  }
> > > > > 
> > > > 
> > > 
> > 
>

Ville Syrjälä April 15, 2014, 11:44 a.m. UTC | #7

On Tue, Apr 15, 2014 at 04:53:22PM +0530, Sagar Arun Kamble wrote:
> Hi Ville,
> 
> Thanks for the reply.
> I have few more queries.
> On Tue, 2014-04-15 at 13:35 +0300, Ville Syrjälä wrote:
> > On Tue, Apr 15, 2014 at 03:14:04PM +0530, Sagar Arun Kamble wrote:
> > > Hi Ville,
> > > 
> > > Somehow I did not receive your review comments
> > > (http://lists.freedesktop.org/archives/intel-gfx/2014-April/042956.html
> > > ) in my mailbox.
> > > 
> > > Here is my input w.r.t patches I have sent for review:
> > > 
> > > Currently my patches are modeling constant alpha blend factor (assuming
> > > pre-multiplied format) as 
> > > 
> > > Dc = Sc * Ca + (1 - Ca) * Dc
> > > 
> > > User space has to supply (<alpha value>, CONSTANT_ALPHA) through drm
> > > property to invoke this blending.
> > > 
> > > Here are my queries on your proposed approach:
> > > 
> > > 1. src factors are 1, Ca, Sa*Ca and dst factors are 0, 1-Ca, 1-Sa,
> > > 1-Sa*Ca: How is factor 0 derived and applicable to Dst and not
> > > applicable to src.
> > 
> > If blending is disabled the function will be Dc = Sc * 1 + 0 * Dc
> > 
> > Hence we get the ONE and ZERO blend factors.
> > 
> > > 
> > > 2. With this approach user will know what blend equation is supported.
> > > How does user supply the blend parameters? Should we have another
> > > property that will have blend equation selected and corresponding
> > > parameters?
> > 
> > By parameters you mean the constant alpha and background color for
> > instance? I think having those as separate properties is the cleanest
> > option.
> Yes. I meant constant alpha value. For what type of blending background
> color will be used?

The background color just defines what color is at the bottom of the
stack. Typically it's black, but on certain hardware you can configure
it. The only really good use for this feature I can think of is power
saving. If most of the screen is supposed to be painted with a single
solid color, we could configure that background color accordingly and
configure the planes to cover only the parts of the screen where there's
some other content. This way there would be no memory fetches for most
of screen.

> 
> So, the overall flow will be:
> 
> 1. User queries supported blending equations.
> 2. Sets required blend equation.
> 3. Sets required blend parameters like constant alpha value etc.
> 
> Step 2 and 3 need to be done atomically.

Nuclear flip ftw. Before we get that, the best option is probably to
turn off the plane, configure all the properties, and re-enable the
plane. Obviosuly you still risk glitches during the plane
enable/disable, but that's already the case anyway.

> 
> > 
> > > 
> > > 3. Is flag suggested for pre-multiply ("yes please, premultiply the data
> > > for me") to be set during AddFB?
> > 
> > If we define the flag that way, then I think we should make it part of
> > the blend equation rather than apply it to the FB. Or it could even be
> > a separate plane property. 
> > 
> This flag is actually part of following factors: SRC_ALPHA,
> ONE_MINUS_SRC_ALPHA,SRC_CONST_ALPHA, ONE_MINUS_SRC_CONST_ALPHA.

No, based on the docs I have our hardware can effectively perform the
Sc*Sa twice. Once as the premultiplication step, and a second time as
part of the pipe blending. The pipe blending is what the blend equation
will control, and in addition the premultiplication can be performed by
the plane before the data even enters the pipe blender. So we need both
the SRC_ALPHA blend factors and some kind of a premultiplication control.

> 
> What action does driver take if user selects these blend factors?
> Change the pixel format to non-pre-multiplied format? This is again
> coming back to our previous discussion where changing pixel format on
> fly should not be done.
> 
>  
> > > 
> > > Kindly check my patches and provide feedback on the approach of stuffing
> > > blend type and blend parameter into single 64bit drm property.
> > 
> > I think we should avoid trying to stuff too much data into a single
> > property. IMO blend equation can be one property, constant alpha,
> > background color, pre-multiplication may be other properties.
> > 
> > > 
> > > 
> > > Thanks,
> > > Sagar
> > > 
> > > 
> > > 
> > > 
> > > On Thu, 2014-04-03 at 11:13 +0530, Sagar Arun Kamble wrote:
> > > > Hi Ville,
> > > > 
> > > > Could you please review these patches.
> > > > 
> > > > thanks,
> > > > Sagar
> > > > 
> > > > 
> > > > On Wed, 2014-04-02 at 11:42 +0530, Sagar Arun Kamble wrote:
> > > > > Reminder for review :)
> > > > > 
> > > > > On Tue, 2014-04-01 at 10:21 +0530, Sagar Arun Kamble wrote:
> > > > > > Gentle Reminder for reviewing this and related patches:
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042350.html
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042351.html
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042352.html
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042587.html
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/042354.html
> > > > > > 
> > > > > > 
> > > > > > Also, provide views on "how do we handle pre-multiplied alpha pixel
> > > > > > formats or is there need to convey pre-multiplied alpha pixel format
> > > > > > through additional drm property?"
> > > > > > Since we are already advertising supported pre-multiplied formats
> > > > > > during intel_plane_init/drm_plane_init by supplying vlv_plane_formats
> > > > > > etc.
> > > > > > 
> > > > > > 
> > > > > > On Tue, 2014-03-25 at 20:02 +0530, sagar.a.kamble@intel.com wrote:
> > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > > 
> > > > > > > This patch enables constant alpha property for Sprite planes.
> > > > > > > Client has to set BIT(DRM_BLEND_CONSTANT_ALPHA) | ((alpha value) << 32)
> > > > > > > for applying constant alpha on a plane. To disable constant alpha,
> > > > > > > client has to set BIT(DRM_BLEND_SRC_COLOR)
> > > > > > > 
> > > > > > > v2: Fixing property value comparison in vlv_update_plane with the use
> > > > > > > of BIT(). Replacing DRM_BLEND_NONE by DRM_BLEND_SRC_COLOR.
> > > > > > > Reverting line spacing change in i915_driver_lastclose. Return value
> > > > > > > of intel_plane_set_property is changed to -ENVAL [Damien's Review Comments]
> > > > > > > 
> > > > > > > Testcase: igt/kms_blend
> > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > > Tested-by: Sharma, Ankitprasad R <ankitprasad.r.sharma@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_dma.c     | 10 ++++++
> > > > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > > > >  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h    |  7 +++++
> > > > > > >  drivers/gpu/drm/i915/intel_sprite.c | 61 ++++++++++++++++++++++++++++++++++++-
> > > > > > >  5 files changed, 80 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > > > > index e4d2b9f..e3a94a3 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > > > > @@ -1898,12 +1898,22 @@ void i915_driver_lastclose(struct drm_device * dev)
> > > > > > >  {
> > > > > > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > > > > > >  
> > > > > > > +	struct intel_plane *plane;
> > > > > > >  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
> > > > > > >  	 * goes right around and calls lastclose. Check for this and don't clean
> > > > > > >  	 * up anything. */
> > > > > > >  	if (!dev_priv)
> > > > > > >  		return;
> > > > > > >  
> > > > > > > +	if (dev_priv->blend_property) {
> > > > > > > +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > > > > > > +			plane->blend_param.value = BIT(DRM_BLEND_SRC_COLOR);
> > > > > > > +			drm_object_property_set_value(&plane->base.base,
> > > > > > > +						dev_priv->blend_property,
> > > > > > > +						plane->blend_param.value);
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > > > >  		intel_fbdev_restore_mode(dev);
> > > > > > >  		vga_switcheroo_process_delayed_switch();
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > index 70fbe90..0d1be70 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > > @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private {
> > > > > > >  
> > > > > > >  	struct drm_property *broadcast_rgb_property;
> > > > > > >  	struct drm_property *force_audio_property;
> > > > > > > +	struct drm_property *blend_property;
> > > > > > >  
> > > > > > >  	uint32_t hw_context_size;
> > > > > > >  	struct list_head context_list;
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > index 6174fda..bfcfe72 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > @@ -3774,6 +3774,8 @@ enum punit_power_well {
> > > > > > >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> > > > > > >  #define   SPRITE_TILED			(1<<10)
> > > > > > >  #define   SPRITE_DEST_KEY		(1<<2)
> > > > > > > +#define	  SPRITE_CONSTANT_ALPHA_ENABLE  (1<<31)
> > > > > > > +#define   SPRITE_CONSTANT_ALPHA_MASK	(0xFF)
> > > > > > >  #define _SPRA_LINOFF		0x70284
> > > > > > >  #define _SPRA_STRIDE		0x70288
> > > > > > >  #define _SPRA_POS		0x7028c
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > index f4e0615..039a800 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > @@ -409,6 +409,13 @@ struct intel_plane {
> > > > > > >  	unsigned int crtc_w, crtc_h;
> > > > > > >  	uint32_t src_x, src_y;
> > > > > > >  	uint32_t src_w, src_h;
> > > > > > > +	union {
> > > > > > > +		uint64_t value;
> > > > > > > +		struct {
> > > > > > > +			unsigned int type;
> > > > > > > +			unsigned int factor;
> > > > > > > +		} details;
> > > > > > > +	} blend_param;
> > > > > > >  
> > > > > > >  	/* Since we need to change the watermarks before/after
> > > > > > >  	 * enabling/disabling the planes, we need to store the parameters here
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > > index 9436f18..9ec84bb 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > > @@ -51,11 +51,15 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > > > > >  	int pipe = intel_plane->pipe;
> > > > > > >  	int plane = intel_plane->plane;
> > > > > > >  	u32 sprctl;
> > > > > > > +	u32 sprconstalpha;
> > > > > > > +	unsigned int blend_type, blend_factor;
> > > > > > >  	unsigned long sprsurf_offset, linear_offset;
> > > > > > >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > > > > > >  
> > > > > > >  	sprctl = I915_READ(SPCNTR(pipe, plane));
> > > > > > >  
> > > > > > > +	sprconstalpha = SPCONSTALPHA(pipe, plane);
> > > > > > > +
> > > > > > >  	/* Mask out pixel format bits in case we change it */
> > > > > > >  	sprctl &= ~SP_PIXFORMAT_MASK;
> > > > > > >  	sprctl &= ~SP_YUV_BYTE_ORDER_MASK;
> > > > > > > @@ -104,6 +108,23 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > > > > > >  		break;
> > > > > > >  	}
> > > > > > >  
> > > > > > > +	/* Handle plane alpha and color blending properties */
> > > > > > > +	blend_type = intel_plane->blend_param.details.type;
> > > > > > > +	blend_factor = intel_plane->blend_param.details.factor;
> > > > > > > +
> > > > > > > +	switch (blend_type) {
> > > > > > > +	case BIT(DRM_BLEND_SRC_COLOR):
> > > > > > > +		I915_WRITE(sprconstalpha, ~SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > > > > +		break;
> > > > > > > +	case BIT(DRM_BLEND_CONSTANT_ALPHA):
> > > > > > > +		I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) |
> > > > > > > +						SPRITE_CONSTANT_ALPHA_ENABLE);
> > > > > > > +		break;
> > > > > > > +	default:
> > > > > > > +		DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type);
> > > > > > > +		break;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/*
> > > > > > >  	 * Enable gamma to match primary/cursor plane behaviour.
> > > > > > >  	 * FIXME should be user controllable via propertiesa.
> > > > > > > @@ -1011,6 +1032,26 @@ out_unlock:
> > > > > > >  	return ret;
> > > > > > >  }
> > > > > > >  
> > > > > > > +static int intel_plane_set_property(struct drm_plane *plane,
> > > > > > > +				    struct drm_property *prop,
> > > > > > > +				    uint64_t val)
> > > > > > > +{
> > > > > > > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > > > > > > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > > > +	uint64_t old_val;
> > > > > > > +	int ret = -EINVAL;
> > > > > > > +
> > > > > > > +	if (prop == dev_priv->blend_property) {
> > > > > > > +		old_val = intel_plane->blend_param.value;
> > > > > > > +		intel_plane->blend_param.value = val;
> > > > > > > +		ret = intel_plane_restore(plane);
> > > > > > > +		if (ret)
> > > > > > > +			intel_plane->blend_param.value = old_val;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > >  int intel_plane_restore(struct drm_plane *plane)
> > > > > > >  {
> > > > > > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > > > @@ -1037,6 +1078,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> > > > > > >  	.update_plane = intel_update_plane,
> > > > > > >  	.disable_plane = intel_disable_plane,
> > > > > > >  	.destroy = intel_destroy_plane,
> > > > > > > +	.set_property = intel_plane_set_property,
> > > > > > >  };
> > > > > > >  
> > > > > > >  static uint32_t ilk_plane_formats[] = {
> > > > > > > @@ -1073,6 +1115,7 @@ static uint32_t vlv_plane_formats[] = {
> > > > > > >  int
> > > > > > >  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > > > >  {
> > > > > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > >  	struct intel_plane *intel_plane;
> > > > > > >  	unsigned long possible_crtcs;
> > > > > > >  	const uint32_t *plane_formats;
> > > > > > > @@ -1141,13 +1184,29 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > > > > > >  
> > > > > > >  	intel_plane->pipe = pipe;
> > > > > > >  	intel_plane->plane = plane;
> > > > > > > +	/* Initialize drm properties for plane */
> > > > > > > +	intel_plane->blend_param.details.type = BIT(DRM_BLEND_SRC_COLOR);
> > > > > > > +	intel_plane->blend_param.details.factor = 0;
> > > > > > > +
> > > > > > >  	possible_crtcs = (1 << pipe);
> > > > > > >  	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> > > > > > >  			     &intel_plane_funcs,
> > > > > > >  			     plane_formats, num_plane_formats,
> > > > > > >  			     false);
> > > > > > > -	if (ret)
> > > > > > > +	if (ret) {
> > > > > > >  		kfree(intel_plane);
> > > > > > > +		goto out;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (!dev_priv->blend_property)
> > > > > > > +		dev_priv->blend_property = drm_mode_create_blend_property(dev,
> > > > > > > +							BIT(DRM_BLEND_SRC_COLOR) |
> > > > > > > +							BIT(DRM_BLEND_CONSTANT_ALPHA));
> > > > > > >  
> > > > > > > +	if (dev_priv->blend_property)
> > > > > > > +		drm_object_attach_property(&intel_plane->base.base,
> > > > > > > +				dev_priv->blend_property,
> > > > > > > +				intel_plane->blend_param.value);
> > > > > > > +out:
> > > > > > >  	return ret;
> > > > > > >  }
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
>

[v2,3/4] drm/i915: Enabling constant alpha drm property

Commit Message

Comments

Patch