diff mbox

[RFC,1/1] drm/i915: Added support for setting plane alpha through drm property

Message ID 1392804488-5551-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sagar.a.kamble@intel.com Feb. 19, 2014, 10:08 a.m. UTC
From: Sagar Kamble <sagar.a.kamble@intel.com>

With this patch two properties are added. One for CRTC+Sprite planes
and another for Cursor planes. Through these client will be able to
change the pixel format of the planes w.r.t Alpha channel.
Number of drm properties are limited so should we restrain from adding this
as drm property or should we expose this as IOCTL itself.

Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   2 +
 drivers/gpu/drm/i915/i915_reg.h      |   6 ++
 drivers/gpu/drm/i915/intel_display.c | 156 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   8 +-
 drivers/gpu/drm/i915/intel_sprite.c  |  59 +++++++++++--
 5 files changed, 222 insertions(+), 9 deletions(-)

Comments

sagar.a.kamble@intel.com Feb. 24, 2014, 3:44 p.m. UTC | #1
Gentle Reminder !!!
Kindly review and provide feedback

thanks,
Sagar

On Wed, 2014-02-19 at 15:38 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> With this patch two properties are added. One for CRTC+Sprite planes
> and another for Cursor planes. Through these client will be able to
> change the pixel format of the planes w.r.t Alpha channel.
> Number of drm properties are limited so should we restrain from adding this
> as drm property or should we expose this as IOCTL itself.
> 
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +
>  drivers/gpu/drm/i915/i915_reg.h      |   6 ++
>  drivers/gpu/drm/i915/intel_display.c | 156 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |   8 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  59 +++++++++++--
>  5 files changed, 222 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8c64831..5ced53d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1560,6 +1560,8 @@ typedef struct drm_i915_private {
>  
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
> +	struct drm_property *plane_alpha_property;
> +	struct drm_property *cursor_alpha_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 2f564ce..039270e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3522,7 +3522,11 @@
>  /* New style CUR*CNTR flags */
>  #define   CURSOR_MODE		0x27
>  #define   CURSOR_MODE_DISABLE   0x00
> +#define   CURSOR_MODE_128_32B_AX 0x02
> +#define   CURSOR_MODE_256_32B_AX 0x03
>  #define   CURSOR_MODE_64_32B_AX 0x07
> +#define   CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX)
> +#define   CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX)
>  #define   CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX)
>  #define   MCURSOR_PIPE_SELECT	(1 << 28)
>  #define   MCURSOR_PIPE_A	0x00
> @@ -3569,7 +3573,9 @@
>  #define   DISPPLANE_RGBX101010			(0x8<<26)
>  #define   DISPPLANE_RGBA101010			(0x9<<26)
>  #define   DISPPLANE_BGRX101010			(0xa<<26)
> +#define   DISPPLANE_BGRA101010			(0xb<<26)
>  #define   DISPPLANE_RGBX161616			(0xc<<26)
> +#define   DISPPLANE_RGBA161616			(0xd<<26)
>  #define   DISPPLANE_RGBX888			(0xe<<26)
>  #define   DISPPLANE_RGBA888			(0xf<<26)
>  #define   DISPPLANE_STEREO_ENABLE		(1<<25)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f19e6ea..a854bcb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2047,6 +2047,86 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  	}
>  }
>  
> +u32 control_plane_alpha(u32 pixformat, unsigned int alpha)
> +{
> +	switch (pixformat) {
> +	case DISPPLANE_RGBX888:
> +	case DISPPLANE_RGBA888:
> +		if (alpha)
> +			pixformat = DISPPLANE_RGBA888;
> +		else
> +			pixformat = DISPPLANE_RGBX888;
> +		break;
> +	case DISPPLANE_BGRX888:
> +	case DISPPLANE_BGRA888:
> +		if (alpha)
> +			pixformat = DISPPLANE_BGRA888;
> +		else
> +			pixformat = DISPPLANE_BGRX888;
> +		break;
> +	case DISPPLANE_RGBX101010:
> +	case DISPPLANE_RGBA101010:
> +		if (alpha)
> +			pixformat = DISPPLANE_RGBA101010;
> +		else
> +			pixformat = DISPPLANE_RGBX101010;
> +		break;
> +	case DISPPLANE_BGRX101010:
> +	case DISPPLANE_BGRA101010:
> +		if (alpha)
> +			pixformat = DISPPLANE_BGRA101010;
> +		else
> +			pixformat = DISPPLANE_BGRX101010;
> +		break;
> +	case DISPPLANE_RGBX161616:
> +	case DISPPLANE_RGBA161616:
> +		if (alpha)
> +			pixformat = DISPPLANE_RGBA161616;
> +		else
> +			pixformat = DISPPLANE_RGBX161616;
> +		break;
> +	default:
> +		if (alpha)
> +			DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat);
> +		break;
> +	}
> +	return pixformat;
> +}
> +
> +u32 control_cursor_alpha(u32 pixformat, unsigned int alpha)
> +{
> +	switch (pixformat) {
> +	case CURSOR_MODE_128_32B_AX:
> +	case CURSOR_MODE_128_ARGB_AX:
> +		if (alpha)
> +			pixformat = CURSOR_MODE_128_ARGB_AX;
> +		else
> +			pixformat = CURSOR_MODE_128_32B_AX;
> +		break;
> +
> +	case CURSOR_MODE_256_ARGB_AX:
> +	case CURSOR_MODE_256_32B_AX:
> +		if (alpha)
> +			pixformat = CURSOR_MODE_256_ARGB_AX;
> +		else
> +			pixformat = CURSOR_MODE_256_32B_AX;
> +		break;
> +
> +	case CURSOR_MODE_64_ARGB_AX:
> +	case CURSOR_MODE_64_32B_AX:
> +		if (alpha)
> +			pixformat = CURSOR_MODE_64_ARGB_AX;
> +		else
> +			pixformat = CURSOR_MODE_64_32B_AX;
> +		break;
> +	default:
> +		if (alpha)
> +			DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat);
> +		break;
> +	}
> +	return pixformat;
> +}
> +
>  static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  			     int x, int y)
>  {
> @@ -2107,6 +2187,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  		BUG();
>  	}
>  
> +	dspcntr |= control_plane_alpha(dspcntr & DISPPLANE_PIXFORMAT_MASK,
> +				       intel_crtc->primary_alpha);
> +
>  	if (INTEL_INFO(dev)->gen >= 4) {
>  		if (obj->tiling_mode != I915_TILING_NONE)
>  			dspcntr |= DISPPLANE_TILED;
> @@ -7415,6 +7498,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  		if (base) {
>  			cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
>  			cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +			cntl |= control_cursor_alpha(cntl & CURSOR_MODE, intel_crtc->cursor_alpha);
>  			cntl |= pipe << 28; /* Connect to correct pipe */
>  		} else {
>  			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> @@ -8755,6 +8839,59 @@ free_work:
>  	return ret;
>  }
>  
> +static int intel_set_primary_plane_alpha(struct intel_crtc *crtc,
> +                                           unsigned int alpha)
> +{
> +       struct drm_device *dev = crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       unsigned int old_alpha;
> +       int ret = 0;
> +
> +       old_alpha = crtc->primary_alpha;
> +       crtc->primary_alpha = alpha;
> +
> +       if (!crtc->active)
> +               return 0;
> +
> +       intel_crtc_wait_for_pending_flips(&crtc->base);
> +
> +       ret = dev_priv->display.update_plane(&crtc->base, crtc->base.fb, 0, 0);
> +       if (ret)
> +               crtc->primary_alpha = old_alpha;
> +
> +       return ret;
> +}
> +
> +static void intel_set_cursor_plane_alpha(struct intel_crtc *crtc,
> +                                           unsigned int alpha)
> +{
> +       crtc->cursor_alpha = alpha;
> +
> +       if (crtc->active)
> +              intel_crtc_update_cursor(&crtc->base, true);
> +}
> +
> +static int intel_crtc_set_property(struct drm_crtc *crtc,
> +                                    struct drm_property *prop,
> +                                    uint64_t val)
> +{
> +        struct drm_device *dev = crtc->dev;
> +        struct drm_i915_private *dev_priv = dev->dev_private;
> +        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +        uint64_t old_val;
> +        int ret = -ENOENT;
> +
> +        if (prop == dev_priv->plane_alpha_property) {
> +               ret = intel_set_primary_plane_alpha(intel_crtc,
> +                                                   intel_crtc->primary_alpha);
> +	} else if (prop == dev_priv->cursor_alpha_property) {
> +		intel_set_cursor_plane_alpha(intel_crtc, val);
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>  	.load_lut = intel_crtc_load_lut,
> @@ -10167,6 +10304,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.set_config = intel_crtc_set_config,
>  	.destroy = intel_crtc_destroy,
>  	.page_flip = intel_crtc_page_flip,
> +	.set_property = intel_crtc_set_property
>  };
>  
>  static void intel_cpu_pll_init(struct drm_device *dev)
> @@ -10305,6 +10443,24 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
> +	if (!dev_priv->plane_alpha_property)
> +		dev_priv->plane_alpha_property =
> +			drm_property_create_range(dev, 0, "plane-alpha", 0, 1);
> +
> +	if (dev_priv->plane_alpha_property)
> +		drm_object_attach_property(&intel_crtc->base.base,
> +					dev_priv->plane_alpha_property,
> +					intel_crtc->primary_alpha);
> +
> +       if (!dev_priv->cursor_alpha_property)
> +	       dev_priv->cursor_alpha_property =
> +			drm_property_create_range(dev, 0, "cursor-alpha", 0, 1);
> +
> +       if (dev_priv->cursor_alpha_property)
> +	       drm_object_attach_property(&intel_crtc->base.base,
> +				       dev_priv->cursor_alpha_property,
> +				       intel_crtc->cursor_alpha);
> +
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a4ffc02..0080d3a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -339,6 +339,9 @@ struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
>  	enum plane plane;
> +	unsigned int primary_alpha; /* primary plane alpha control */
> +        unsigned int cursor_alpha; /* cursor plane alpha control */
> +
>  	u8 lut_r[256], lut_g[256], lut_b[256];
>  	/*
>  	 * Whether the crtc and the connected output pipeline is active. Implies
> @@ -405,6 +408,7 @@ struct intel_plane {
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y;
>  	uint32_t src_w, src_h;
> +	unsigned int plane_alpha;
>  
>  	/* Since we need to change the watermarks before/after
>  	 * enabling/disabling the planes, we need to store the parameters here
> @@ -676,6 +680,8 @@ int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
>  enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
>  					     enum pipe pipe);
>  void intel_wait_for_vblank(struct drm_device *dev, int pipe);
> +u32 control_plane_alpha(u32 pixformat, unsigned int alpha);
> +u32 control_cursor_alpha(u32 pixformat, unsigned int alpha);
>  void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> @@ -906,7 +912,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>  int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
>  void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
>  			       enum plane plane);
> -void intel_plane_restore(struct drm_plane *plane);
> +int intel_plane_restore(struct drm_plane *plane);
>  void intel_plane_disable(struct drm_plane *plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 336ae6c..9f6c91a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -104,6 +104,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		break;
>  	}
>  
> +	sprctl |= control_plane_alpha(sprctl & SP_PIXFORMAT_MASK,
> +				       intel_plane->plane_alpha);
> +
>  	/*
>  	 * Enable gamma to match primary/cursor plane behaviour.
>  	 * FIXME should be user controllable via propertiesa.
> @@ -262,6 +265,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		BUG();
>  	}
>  
> +	sprctl |= control_plane_alpha(sprctl & SPRITE_PIXFORMAT_MASK,
> +				       intel_plane->plane_alpha);
> +
>  	/*
>  	 * Enable gamma to match primary/cursor plane behaviour.
>  	 * FIXME should be user controllable via propertiesa.
> @@ -446,6 +452,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		BUG();
>  	}
>  
> +	dvscntr |= control_plane_alpha(dvscntr & DVS_PIXFORMAT_MASK,
> +				       intel_plane->plane_alpha);
> +
>  	/*
>  	 * Enable gamma to match primary/cursor plane behaviour.
>  	 * FIXME should be user controllable via propertiesa.
> @@ -1011,18 +1020,38 @@ out_unlock:
>  	return ret;
>  }
>  
> -void intel_plane_restore(struct drm_plane *plane)
> +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 = -ENOENT;
> +
> +        if (prop == dev_priv->plane_alpha_property) {
> +                old_val = intel_plane->plane_alpha;
> +                intel_plane->plane_alpha = val;
> +                ret = intel_plane_restore(plane);
> +                if (ret)
> +                        intel_plane->plane_alpha = old_val;
> +        }
> +
> +        return ret;
> +}
> +
> +int intel_plane_restore(struct drm_plane *plane)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  
>  	if (!plane->crtc || !plane->fb)
> -		return;
> +		return 0;
>  
> -	intel_update_plane(plane, plane->crtc, plane->fb,
> -			   intel_plane->crtc_x, intel_plane->crtc_y,
> -			   intel_plane->crtc_w, intel_plane->crtc_h,
> -			   intel_plane->src_x, intel_plane->src_y,
> -			   intel_plane->src_w, intel_plane->src_h);
> +	return intel_update_plane(plane, plane->crtc, plane->fb,
> +				  intel_plane->crtc_x, intel_plane->crtc_y,
> +				  intel_plane->crtc_w, intel_plane->crtc_h,
> +				  intel_plane->src_x, intel_plane->src_y,
> +				  intel_plane->src_w, intel_plane->src_h);
>  }
>  
>  void intel_plane_disable(struct drm_plane *plane)
> @@ -1037,6 +1066,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 +1103,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;
> @@ -1146,8 +1177,20 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  			     &intel_plane_funcs,
>  			     plane_formats, num_plane_formats,
>  			     false);
> -	if (ret)
> +	if (ret) {
>  		kfree(intel_plane);
> +		goto out;
> +	}
> +
> +	if (!dev_priv->plane_alpha_property)
> +		dev_priv->plane_alpha_property =
> +			drm_property_create_range(dev, 0, "plane-alpha", 0, 1);
> +
> +	if (dev_priv->plane_alpha_property)
> +		drm_object_attach_property(&intel_plane->base.base,
> +					dev_priv->plane_alpha_property,
> +					intel_plane->plane_alpha);
>  
> +out:
>  	return ret;
>  }
Ville Syrjala Feb. 25, 2014, 6:18 p.m. UTC | #2
On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> With this patch two properties are added. One for CRTC+Sprite planes
> and another for Cursor planes. Through these client will be able to
> change the pixel format of the planes w.r.t Alpha channel.
> Number of drm properties are limited so should we restrain from adding this
> as drm property or should we expose this as IOCTL itself.

We need to stop adding properties on a whim and design and document them
properly. So what I'd like to see is someone going over the current
properties and collecting them up in some (even bares bones) documentation
in Documentation/Docbook. The current way will lead to a huge mess when
userspace actually starts to depend on properties. So far properties
have been mostly some optional extra junk on the side people that may
want to frob, but that's going to change as we add more of them,
especially with plane and crtc properties, which actually affect how
the scene gets composed together.

And I think we need to put a hold on adding the plane properties to the
crtc since the plan is to convert everything to drm_plane. With the
current rate we're going to have a ton of properties on the crtc that no
one will use. Adding properties to the sprite planes seems OK in the
meantime.

As far as alpha blending is concerned I've had the following ideas:
- we need a plane property for constant alpha. Some drivers might
  already have this, so might be good to check. Although I'm fairly
  sure what's there won't be entirely future proof. I was thinking that
  we should standardize of using 16bits for color components in
  properties. That way you can still stick a full ARGB value into
  one property, and we should be good for a few more years until
  someone has the idea to move beyond 16bits per channel. And it's
  more or less hardware agnostic. Obviously if the hardware won't
  use the full precision, you get to throw away the low bits, but I
  don't think there's any other good way to go.
- we need another property to indicate whether the source pixels
  are premultiplied or not. Or maybe it's easier for people to think
  in terms of what operations the hardware will do, in which case
  we should make the property indicate whether the hardware will
  do the premultiplication during blending or not. I'm not sure
  which approach feels more natural to people.
- And finally we need to figure out how to blend it all together.
  It might make sense to model this after glBlendFunc(), so it would
  be an enum property, or maybe two if we want separate properties
  for source and destination factors.

Obviously the final result will depend on additional things like the
z-order, which is going to be another property. I think this one might
already exists in some form in other drivers. So we should definitely
look at what's there and try to do the same if possible. Which again
underlines the need to collect up the current properties into some
central documentation.
Daniel Vetter March 4, 2014, 9:42 a.m. UTC | #3
On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > 
> > With this patch two properties are added. One for CRTC+Sprite planes
> > and another for Cursor planes. Through these client will be able to
> > change the pixel format of the planes w.r.t Alpha channel.
> > Number of drm properties are limited so should we restrain from adding this
> > as drm property or should we expose this as IOCTL itself.
> 
> We need to stop adding properties on a whim and design and document them
> properly. So what I'd like to see is someone going over the current
> properties and collecting them up in some (even bares bones) documentation
> in Documentation/Docbook. The current way will lead to a huge mess when
> userspace actually starts to depend on properties. So far properties
> have been mostly some optional extra junk on the side people that may
> want to frob, but that's going to change as we add more of them,
> especially with plane and crtc properties, which actually affect how
> the scene gets composed together.
> 
> And I think we need to put a hold on adding the plane properties to the
> crtc since the plan is to convert everything to drm_plane. With the
> current rate we're going to have a ton of properties on the crtc that no
> one will use. Adding properties to the sprite planes seems OK in the
> meantime.
> 
> As far as alpha blending is concerned I've had the following ideas:
> - we need a plane property for constant alpha. Some drivers might
>   already have this, so might be good to check. Although I'm fairly
>   sure what's there won't be entirely future proof. I was thinking that
>   we should standardize of using 16bits for color components in
>   properties. That way you can still stick a full ARGB value into
>   one property, and we should be good for a few more years until
>   someone has the idea to move beyond 16bits per channel. And it's
>   more or less hardware agnostic. Obviously if the hardware won't
>   use the full precision, you get to throw away the low bits, but I
>   don't think there's any other good way to go.
> - we need another property to indicate whether the source pixels
>   are premultiplied or not. Or maybe it's easier for people to think
>   in terms of what operations the hardware will do, in which case
>   we should make the property indicate whether the hardware will
>   do the premultiplication during blending or not. I'm not sure
>   which approach feels more natural to people.
> - And finally we need to figure out how to blend it all together.
>   It might make sense to model this after glBlendFunc(), so it would
>   be an enum property, or maybe two if we want separate properties
>   for source and destination factors.
> 
> Obviously the final result will depend on additional things like the
> z-order, which is going to be another property. I think this one might
> already exists in some form in other drivers. So we should definitely
> look at what's there and try to do the same if possible. Which again
> underlines the need to collect up the current properties into some
> central documentation.

Concurred on stealing the blending model from GL. It seems what everyon
else is aiming for at least at both the hw and sw level ... One issue with
that is handling color keys, since those aren't supported by glBlendFunc.
But I guess we could just add those as additional modes, since the usual
blend funcs already require a constant blend color, which could be reused
as the color key.
-Daniel
Ville Syrjala March 4, 2014, 12:06 p.m. UTC | #4
On Tue, Mar 04, 2014 at 10:42:38AM +0100, Daniel Vetter wrote:
> On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > 
> > > With this patch two properties are added. One for CRTC+Sprite planes
> > > and another for Cursor planes. Through these client will be able to
> > > change the pixel format of the planes w.r.t Alpha channel.
> > > Number of drm properties are limited so should we restrain from adding this
> > > as drm property or should we expose this as IOCTL itself.
> > 
> > We need to stop adding properties on a whim and design and document them
> > properly. So what I'd like to see is someone going over the current
> > properties and collecting them up in some (even bares bones) documentation
> > in Documentation/Docbook. The current way will lead to a huge mess when
> > userspace actually starts to depend on properties. So far properties
> > have been mostly some optional extra junk on the side people that may
> > want to frob, but that's going to change as we add more of them,
> > especially with plane and crtc properties, which actually affect how
> > the scene gets composed together.
> > 
> > And I think we need to put a hold on adding the plane properties to the
> > crtc since the plan is to convert everything to drm_plane. With the
> > current rate we're going to have a ton of properties on the crtc that no
> > one will use. Adding properties to the sprite planes seems OK in the
> > meantime.
> > 
> > As far as alpha blending is concerned I've had the following ideas:
> > - we need a plane property for constant alpha. Some drivers might
> >   already have this, so might be good to check. Although I'm fairly
> >   sure what's there won't be entirely future proof. I was thinking that
> >   we should standardize of using 16bits for color components in
> >   properties. That way you can still stick a full ARGB value into
> >   one property, and we should be good for a few more years until
> >   someone has the idea to move beyond 16bits per channel. And it's
> >   more or less hardware agnostic. Obviously if the hardware won't
> >   use the full precision, you get to throw away the low bits, but I
> >   don't think there's any other good way to go.
> > - we need another property to indicate whether the source pixels
> >   are premultiplied or not. Or maybe it's easier for people to think
> >   in terms of what operations the hardware will do, in which case
> >   we should make the property indicate whether the hardware will
> >   do the premultiplication during blending or not. I'm not sure
> >   which approach feels more natural to people.
> > - And finally we need to figure out how to blend it all together.
> >   It might make sense to model this after glBlendFunc(), so it would
> >   be an enum property, or maybe two if we want separate properties
> >   for source and destination factors.
> > 
> > Obviously the final result will depend on additional things like the
> > z-order, which is going to be another property. I think this one might
> > already exists in some form in other drivers. So we should definitely
> > look at what's there and try to do the same if possible. Which again
> > underlines the need to collect up the current properties into some
> > central documentation.
> 
> Concurred on stealing the blending model from GL. It seems what everyon
> else is aiming for at least at both the hw and sw level ... One issue with
> that is handling color keys, since those aren't supported by glBlendFunc.
> But I guess we could just add those as additional modes, since the usual
> blend funcs already require a constant blend color, which could be reused
> as the color key.

I've been thinking that color key stuff could just be implemented as
separate properties.

Color keying anyway may require min+max values or value+mask, so a single
value is not enough. But we might want to model the keying function in
a similar fashion to the blendfunc. Some old ATI hardware had a very
flexible color keying functinality where you would specify the graphics
and video keying functions (true,false,eq,neq) and then you had to
choose how to combine the result from those (and,or). If the combined
result was true it would pick the video data, and for false it'd pick
the graphics data. I've not seen that on any other hardware since, but
it's certainly flexible enough to represent the typical src/dst keying
modes.
sagar.a.kamble@intel.com March 6, 2014, 10:28 a.m. UTC | #5
On Tue, 2014-03-04 at 14:06 +0200, Ville Syrjälä wrote:
> On Tue, Mar 04, 2014 at 10:42:38AM +0100, Daniel Vetter wrote:
> > On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > 
> > > > With this patch two properties are added. One for CRTC+Sprite planes
> > > > and another for Cursor planes. Through these client will be able to
> > > > change the pixel format of the planes w.r.t Alpha channel.
> > > > Number of drm properties are limited so should we restrain from adding this
> > > > as drm property or should we expose this as IOCTL itself.
> > > 
> > > We need to stop adding properties on a whim and design and document them
> > > properly. So what I'd like to see is someone going over the current
> > > properties and collecting them up in some (even bares bones) documentation
> > > in Documentation/Docbook. The current way will lead to a huge mess when
> > > userspace actually starts to depend on properties. So far properties
> > > have been mostly some optional extra junk on the side people that may
> > > want to frob, but that's going to change as we add more of them,
> > > especially with plane and crtc properties, which actually affect how
> > > the scene gets composed together.
> > > 
> > > And I think we need to put a hold on adding the plane properties to the
> > > crtc since the plan is to convert everything to drm_plane. With the
> > > current rate we're going to have a ton of properties on the crtc that no
> > > one will use. Adding properties to the sprite planes seems OK in the
> > > meantime.
> > > 
> > > As far as alpha blending is concerned I've had the following ideas:
> > > - we need a plane property for constant alpha. Some drivers might
> > >   already have this, so might be good to check. Although I'm fairly
> > >   sure what's there won't be entirely future proof. I was thinking that
> > >   we should standardize of using 16bits for color components in
> > >   properties. That way you can still stick a full ARGB value into
> > >   one property, and we should be good for a few more years until
> > >   someone has the idea to move beyond 16bits per channel. And it's
> > >   more or less hardware agnostic. Obviously if the hardware won't
> > >   use the full precision, you get to throw away the low bits, but I
> > >   don't think there's any other good way to go.
> > > - we need another property to indicate whether the source pixels
> > >   are premultiplied or not. Or maybe it's easier for people to think
> > >   in terms of what operations the hardware will do, in which case
> > >   we should make the property indicate whether the hardware will
> > >   do the premultiplication during blending or not. I'm not sure
> > >   which approach feels more natural to people.
> > > - And finally we need to figure out how to blend it all together.
> > >   It might make sense to model this after glBlendFunc(), so it would
> > >   be an enum property, or maybe two if we want separate properties
> > >   for source and destination factors.
> > > 
> > > Obviously the final result will depend on additional things like the
> > > z-order, which is going to be another property. I think this one might
> > > already exists in some form in other drivers. So we should definitely
> > > look at what's there and try to do the same if possible. Which again
> > > underlines the need to collect up the current properties into some
> > > central documentation.
> > 
> > Concurred on stealing the blending model from GL. It seems what everyon
> > else is aiming for at least at both the hw and sw level ... One issue with
> > that is handling color keys, since those aren't supported by glBlendFunc.
> > But I guess we could just add those as additional modes, since the usual
> > blend funcs already require a constant blend color, which could be reused
> > as the color key.
> 
> I've been thinking that color key stuff could just be implemented as
> separate properties.
> 
> Color keying anyway may require min+max values or value+mask, so a single
> value is not enough. But we might want to model the keying function in
> a similar fashion to the blendfunc. Some old ATI hardware had a very
> flexible color keying functinality where you would specify the graphics
> and video keying functions (true,false,eq,neq) and then you had to
> choose how to combine the result from those (and,or). If the combined
> result was true it would pick the video data, and for false it'd pick
> the graphics data. I've not seen that on any other hardware since, but
> it's certainly flexible enough to represent the typical src/dst keying
> modes.
We can only model GL_CONSTANT_ALPHA given we have sprite control
registers for that. How do we model other pixel arithmetic related to
color blending?
How do we model source and destination pixel arithmetic? We can only set
alpha/pre-multiplied alpha for individual planes.

>
Daniel Vetter March 6, 2014, 11:24 a.m. UTC | #6
On Thu, Mar 6, 2014 at 11:28 AM, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> We can only model GL_CONSTANT_ALPHA given we have sprite control
> registers for that. How do we model other pixel arithmetic related to
> color blending?
> How do we model source and destination pixel arithmetic? We can only set
> alpha/pre-multiplied alpha for individual planes.

We reject it if userspace attempts it ;-) But we kinda want a model
which also works for the next few hw platforms and also on other
drivers, so going with this sounds like the right approach to me.
-Daniel
Lespiau, Damien March 6, 2014, 12:03 p.m. UTC | #7
On Mon, Feb 24, 2014 at 09:14:41PM +0530, Sagar Arun Kamble wrote:
> Gentle Reminder !!!
> Kindly review and provide feedback

This main issue here is that we don't have yet the split between the
CRTC and the primary plane in the DRM API.

Why is this a problem? because you need to define a 'plane-alpha'
property on the CRTC and we'll have to maintain it forever and we don't
really want that when a neat split between the CRTC and primary plane is
not far off.

Work is ongoing on this, Matt bravely took the task:

  http://lists.freedesktop.org/archives/dri-devel/2014-February/054719.html

With a v2 in the queue.

I believe we need to sort this first, before adding more plane properties.
Lespiau, Damien March 6, 2014, 12:09 p.m. UTC | #8
On Thu, Mar 06, 2014 at 12:03:07PM +0000, Damien Lespiau wrote:
> On Mon, Feb 24, 2014 at 09:14:41PM +0530, Sagar Arun Kamble wrote:
> > Gentle Reminder !!!
> > Kindly review and provide feedback
> 
> This main issue here is that we don't have yet the split between the
> CRTC and the primary plane in the DRM API.
> 
> Why is this a problem? because you need to define a 'plane-alpha'
> property on the CRTC and we'll have to maintain it forever and we don't
> really want that when a neat split between the CRTC and primary plane is
> not far off.
> 
> Work is ongoing on this, Matt bravely took the task:
> 
>   http://lists.freedesktop.org/archives/dri-devel/2014-February/054719.html
> 
> With a v2 in the queue.
> 
> I believe we need to sort this first, before adding more plane properties.

Ah, didn't see the rest of the thread because my MUA decided to split
the thread between two directories. Scrap that mail :)
sagar.a.kamble@intel.com March 8, 2014, 8:21 a.m. UTC | #9
From: Sagar Kamble <sagar.a.kamble@intel.com>

This patch series introduces drm property modelled after glBlendFuc function. For i915
constant alpha is exposed through this property to start with. Additional new property
value for controlling pre-multiplied alpha is added.
i-g-t test case is to be added.

These patches are based on following patches which are already under review/reviewed:

4ae74f3 Documentation: drm: describing drm properties exposed by various drivers
134bdfe Propagate the error from intel_update_plane() up through intel_plane_restore() to the caller.
a6ad21c Make drm_property_create_bitmask() a bit more generic by allowing the caller to specify which bits are in fact supported.

Sagar Kamble (4):
  drm: Added plane alpha and color blending property
  drm/i915: Enabling constant alpha drm property
  drm/i915: Enabling pre-multiplied alpha drm property
  Documentation: drm: describing plane alpha and color blending property

 Documentation/DocBook/drm.tmpl      |  13 +++-
 drivers/gpu/drm/drm_crtc.c          |  33 +++++++++
 drivers/gpu/drm/i915/i915_dma.c     |  11 ++-
 drivers/gpu/drm/i915/i915_drv.h     |   1 +
 drivers/gpu/drm/i915/i915_reg.h     |   2 +
 drivers/gpu/drm/i915/intel_drv.h    |   8 +++
 drivers/gpu/drm/i915/intel_sprite.c | 132 +++++++++++++++++++++++++++++++++++-
 include/drm/drm_crtc.h              |  25 +++++++
 8 files changed, 222 insertions(+), 3 deletions(-)
Lespiau, Damien March 20, 2014, 2:11 p.m. UTC | #10
On Sat, Mar 08, 2014 at 01:51:15PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> This patch series introduces drm property modelled after glBlendFuc function. For i915
> constant alpha is exposed through this property to start with. Additional new property
> value for controlling pre-multiplied alpha is added.
> i-g-t test case is to be added.
> 
> These patches are based on following patches which are already under review/reviewed:
> 
> 4ae74f3 Documentation: drm: describing drm properties exposed by various drivers
> 134bdfe Propagate the error from intel_update_plane() up through intel_plane_restore() to the caller.
> a6ad21c Make drm_property_create_bitmask() a bit more generic by allowing the caller to specify which bits are in fact supported.
> 
> Sagar Kamble (4):
>   drm: Added plane alpha and color blending property
>   drm/i915: Enabling constant alpha drm property
>   drm/i915: Enabling pre-multiplied alpha drm property
>   Documentation: drm: describing plane alpha and color blending property

I'm not sure I follow what's being done with the GL blending equation
here. If we want to support a global alpha on the source plane (ie the
plane that is going to be blended into the "render target", what's
already there), the blending equation looks like:

(source is non-premultiplied, note the use of glBlendFuncSeparate())

RGB = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)
A   = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)

(source is premultiplied)

RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)

The gl blending stuff doesn't know anything about the premultiplied
nature of the incoming source color, it has to be programmed
accordingly.
Lespiau, Damien March 20, 2014, 2:45 p.m. UTC | #11
On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote:
> (source is premultiplied)
> 
> RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)

Grr, copy/paste error. If the source is already premultiplied:

RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
sagar.a.kamble@intel.com March 21, 2014, 1:36 p.m. UTC | #12
Hi Damien,

On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote:
> On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote:
> > (source is premultiplied)
> > 
> > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> 
> Grr, copy/paste error. If the source is already premultiplied:
> 
> RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> 

1. Currently there is no interface to advertise the DRM_FORMATS plane
supportes to user mode? Should we add new IOCTL for that and include
pre-multiplied alpha formats while advertising? Or am I missing any such
API already available?

2. About constant alpha property - when we program constant alpha
register will hardware be able to take care of the blending as per
equations you have specified for non-premultiplied-alpha and
premultiplied-alpha cases or we have to do any additional setting?
Confusion is because of two combinations:
a. pre-multiplied alpha+constant alpha
b. non-pre-multiplied alpha+constant alpha

Kindly clarify.

thanks,
Sagar
Lespiau, Damien March 21, 2014, 7:23 p.m. UTC | #13
On Fri, Mar 21, 2014 at 07:06:46PM +0530, Sagar Arun Kamble wrote:
> Hi Damien,
> 
> On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote:
> > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote:
> > > (source is premultiplied)
> > > 
> > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > 
> > Grr, copy/paste error. If the source is already premultiplied:
> > 
> > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > 
> 
> 1. Currently there is no interface to advertise the DRM_FORMATS plane
> supportes to user mode? Should we add new IOCTL for that and include
> pre-multiplied alpha formats while advertising? Or am I missing any such
> API already available?

There's a 'formats' array in drmModePlane.

> 2. About constant alpha property - when we program constant alpha
> register will hardware be able to take care of the blending as per
> equations you have specified for non-premultiplied-alpha and
> premultiplied-alpha cases or we have to do any additional setting?
> Confusion is because of two combinations:
> a. pre-multiplied alpha+constant alpha
> b. non-pre-multiplied alpha+constant alpha

The first part of the question should be in the spec. I really do expect
the hw to work correctly with any combination of (premul, non-premult) x
(plane alpha, no plane alpha)

To be more clear: I don't think the glBlendFunc constants can represent
everything we want:

- It'd need to differentiate what we want do to between RGB and A (the
  same reason glBlendFuncSeparate was introduced).

- We can't represent blending of an unpremultied fb with a plane-global
  alpha with just the GL blending equation as it needs two
  multiplications (so in GL, you would do one of them in the fragment
  shader).

I would just proceed with a premultipled FB format and the alpha plane
property.

Let's wait until Ville returns, I may be totally not getting what he
meant.
sagar.a.kamble@intel.com March 25, 2014, 10:03 a.m. UTC | #14
Hi Damien,

Could you please clarify following queries.

Thanks,
Sagar
On Fri, 2014-03-21 at 19:06 +0530, Sagar Arun Kamble wrote:
> Hi Damien,
> 
> On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote:
> > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote:
> > > (source is premultiplied)
> > > 
> > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > 
> > Grr, copy/paste error. If the source is already premultiplied:
> > 
> > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > 
> 
> 1. Currently there is no interface to advertise the DRM_FORMATS plane
> supportes to user mode? Should we add new IOCTL for that and include
> pre-multiplied alpha formats while advertising? Or am I missing any such
> API already available?
> 
> 2. About constant alpha property - when we program constant alpha
> register will hardware be able to take care of the blending as per
> equations you have specified for non-premultiplied-alpha and
> premultiplied-alpha cases or we have to do any additional setting?
> Confusion is because of two combinations:
> a. pre-multiplied alpha+constant alpha
> b. non-pre-multiplied alpha+constant alpha
> 
> Kindly clarify.
> 
> thanks,
> Sagar
> 
>
Daniel Vetter March 25, 2014, 10:51 a.m. UTC | #15
On Tue, Mar 25, 2014 at 03:33:42PM +0530, Sagar Arun Kamble wrote:
> Hi Damien,
> 
> Could you please clarify following queries.

He did, in a reply to your mail ...
-Daniel

> 
> Thanks,
> Sagar
> On Fri, 2014-03-21 at 19:06 +0530, Sagar Arun Kamble wrote:
> > Hi Damien,
> > 
> > On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote:
> > > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote:
> > > > (source is premultiplied)
> > > > 
> > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > > 
> > > Grr, copy/paste error. If the source is already premultiplied:
> > > 
> > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > > 
> > 
> > 1. Currently there is no interface to advertise the DRM_FORMATS plane
> > supportes to user mode? Should we add new IOCTL for that and include
> > pre-multiplied alpha formats while advertising? Or am I missing any such
> > API already available?
> > 
> > 2. About constant alpha property - when we program constant alpha
> > register will hardware be able to take care of the blending as per
> > equations you have specified for non-premultiplied-alpha and
> > premultiplied-alpha cases or we have to do any additional setting?
> > Confusion is because of two combinations:
> > a. pre-multiplied alpha+constant alpha
> > b. non-pre-multiplied alpha+constant alpha
> > 
> > Kindly clarify.
> > 
> > thanks,
> > Sagar
> > 
> > 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lespiau, Damien March 25, 2014, 12:26 p.m. UTC | #16
On Tue, Mar 25, 2014 at 11:51:57AM +0100, Daniel Vetter wrote:
> On Tue, Mar 25, 2014 at 03:33:42PM +0530, Sagar Arun Kamble wrote:
> > Hi Damien,
> > 
> > Could you please clarify following queries.
> 
> He did, in a reply to your mail ...

In case you cannot find it:

  http://lists.freedesktop.org/archives/intel-gfx/2014-March/042136.html
Ville Syrjala April 2, 2014, 7:36 p.m. UTC | #17
On Fri, Mar 21, 2014 at 07:23:31PM +0000, Damien Lespiau wrote:
> On Fri, Mar 21, 2014 at 07:06:46PM +0530, Sagar Arun Kamble wrote:
> > Hi Damien,
> > 
> > On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote:
> > > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote:
> > > > (source is premultiplied)
> > > > 
> > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > > 
> > > Grr, copy/paste error. If the source is already premultiplied:
> > > 
> > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
> > > 
> > 
> > 1. Currently there is no interface to advertise the DRM_FORMATS plane
> > supportes to user mode? Should we add new IOCTL for that and include
> > pre-multiplied alpha formats while advertising? Or am I missing any such
> > API already available?
> 
> There's a 'formats' array in drmModePlane.
> 
> > 2. About constant alpha property - when we program constant alpha
> > register will hardware be able to take care of the blending as per
> > equations you have specified for non-premultiplied-alpha and
> > premultiplied-alpha cases or we have to do any additional setting?
> > Confusion is because of two combinations:
> > a. pre-multiplied alpha+constant alpha
> > b. non-pre-multiplied alpha+constant alpha
> 
> The first part of the question should be in the spec. I really do expect
> the hw to work correctly with any combination of (premul, non-premult) x
> (plane alpha, no plane alpha)
> 
> To be more clear: I don't think the glBlendFunc constants can represent
> everything we want:
> 
> - It'd need to differentiate what we want do to between RGB and A (the
>   same reason glBlendFuncSeparate was introduced).

We won't be writing out the alpha anywhere so I'm thinking we don't need
to worry about it.

> 
> - We can't represent blending of an unpremultied fb with a plane-global
>   alpha with just the GL blending equation as it needs two
>   multiplications (so in GL, you would do one of them in the fragment
>   shader).

If I'm interpreting the docs correctly these are the kind of blend equations
we will have soonish:

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

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

premultiplied?
 0: Sc = Sc * Sa
 1: Sc = Sc
Dc = Sc * Sa*Ca + (1-Sa*Ca) * Dc

premultiplied?
 0: Sc = Sc * Sa
 1: Sc = Sc
Dc = Sc * 1 + (1-Sa) * Dc


For reference OMAP36xx was something like this I think:
premultiplied?
 0: Sc = Sc * Sa
 1: Sc = Sc
Dc = Sc * Ca + (1-Sa*Ca) * Dc

and OMAP34xx was something like this:
Sc = Sc
Dc = Sc * Sa*Ca + (1-Sa*Ca) * Dc


So we have src factors of:
1, Ca, Sa*Ca

and dst factors of:
0, 1-Ca, 1-Sa, 1-Sa*Ca

If we add the missing Sa src factor we have a fairly sensible looking
set. Obviously the constant alpha related factors aren't part of GL,
but I don't think we need to worry about that, we're not implementing
GL here after all, just something that resembles it a bit to make it
easier for people to grasp it.

So to make stuff easier for userspace to figure out what we actually
support, I'm thinking it could be just an enum property with the values
being something like '(dstf << 32) | (srcf << 0)' with the factors
defined something like so:

enum {
 ZERO,
 ONE,
 SRC_ALPHA,
 ONE_MINUS_SRC_ALPHA,
 CONST_ALPHA,
 ONE_MINUS_CONST_ALPHA,
 SRC_CONST_ALPHA,
 ONE_MINUS_SRC_CONST_ALPHA,
};

If it's an enum then the user can just pick out one of the
supported blend equations.

> I would just proceed with a premultipled FB format and the alpha plane
> property.

I'm not very enthusiastic about adding the pre-multiplied info to
the FB. That means adding new versions of all the alpha formats we
already have. I almost regret making the A vs. X distinction already.
Might have been neater to leave it entirely up to the blending
properties to figure out whether there's alpha or not.

The one not so annoying option would be to snatch one of the remaining
high bits and use that to indicate premult or not. But we still need to
figure out whether we make the already existing formats be the
premultiplied or non-premultiplied ones. That could have some implication
for user space as we probably can't radically change which formats we
accept to keep existing userspace functional.

So the other option is to not think of it as a property of the
framebuffer, but rather think of it in terms of what operations the
hardware will perform during blending. That way we could just add a flag
which says "yes please, premultiply the data for me".

One thing I didn't much think about is whether we need per-plane blend
functions. That would be the more flexible option, but at least the
current hardware doesn't seem to allow such flexibility. I guess being
future proof shouldn't hurt. We do have the option of per-plane
premultiplication at least. Hmm, actually I have to take that back as
the first two example functions I listed can appear simultaneosly
AFAICS. So this does make things a bit more complicated for userspace
as it can't freely pick blend functions when dealing with multiple
planes. Oh and also for OMAPs the bottom plane wouldn't be blended with
the background AFAICS, so it would always have a blend function of
ONE,ZERO.

OK, so that's my thoughts on the matter. Pardon me if I repeated some
nonsense that has already been shot down. I didn't have time to look at
the actual patches yet.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c64831..5ced53d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1560,6 +1560,8 @@  typedef struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
+	struct drm_property *plane_alpha_property;
+	struct drm_property *cursor_alpha_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 2f564ce..039270e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3522,7 +3522,11 @@ 
 /* New style CUR*CNTR flags */
 #define   CURSOR_MODE		0x27
 #define   CURSOR_MODE_DISABLE   0x00
+#define   CURSOR_MODE_128_32B_AX 0x02
+#define   CURSOR_MODE_256_32B_AX 0x03
 #define   CURSOR_MODE_64_32B_AX 0x07
+#define   CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX)
+#define   CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX)
 #define   CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX)
 #define   MCURSOR_PIPE_SELECT	(1 << 28)
 #define   MCURSOR_PIPE_A	0x00
@@ -3569,7 +3573,9 @@ 
 #define   DISPPLANE_RGBX101010			(0x8<<26)
 #define   DISPPLANE_RGBA101010			(0x9<<26)
 #define   DISPPLANE_BGRX101010			(0xa<<26)
+#define   DISPPLANE_BGRA101010			(0xb<<26)
 #define   DISPPLANE_RGBX161616			(0xc<<26)
+#define   DISPPLANE_RGBA161616			(0xd<<26)
 #define   DISPPLANE_RGBX888			(0xe<<26)
 #define   DISPPLANE_RGBA888			(0xf<<26)
 #define   DISPPLANE_STEREO_ENABLE		(1<<25)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f19e6ea..a854bcb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2047,6 +2047,86 @@  unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 	}
 }
 
+u32 control_plane_alpha(u32 pixformat, unsigned int alpha)
+{
+	switch (pixformat) {
+	case DISPPLANE_RGBX888:
+	case DISPPLANE_RGBA888:
+		if (alpha)
+			pixformat = DISPPLANE_RGBA888;
+		else
+			pixformat = DISPPLANE_RGBX888;
+		break;
+	case DISPPLANE_BGRX888:
+	case DISPPLANE_BGRA888:
+		if (alpha)
+			pixformat = DISPPLANE_BGRA888;
+		else
+			pixformat = DISPPLANE_BGRX888;
+		break;
+	case DISPPLANE_RGBX101010:
+	case DISPPLANE_RGBA101010:
+		if (alpha)
+			pixformat = DISPPLANE_RGBA101010;
+		else
+			pixformat = DISPPLANE_RGBX101010;
+		break;
+	case DISPPLANE_BGRX101010:
+	case DISPPLANE_BGRA101010:
+		if (alpha)
+			pixformat = DISPPLANE_BGRA101010;
+		else
+			pixformat = DISPPLANE_BGRX101010;
+		break;
+	case DISPPLANE_RGBX161616:
+	case DISPPLANE_RGBA161616:
+		if (alpha)
+			pixformat = DISPPLANE_RGBA161616;
+		else
+			pixformat = DISPPLANE_RGBX161616;
+		break;
+	default:
+		if (alpha)
+			DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat);
+		break;
+	}
+	return pixformat;
+}
+
+u32 control_cursor_alpha(u32 pixformat, unsigned int alpha)
+{
+	switch (pixformat) {
+	case CURSOR_MODE_128_32B_AX:
+	case CURSOR_MODE_128_ARGB_AX:
+		if (alpha)
+			pixformat = CURSOR_MODE_128_ARGB_AX;
+		else
+			pixformat = CURSOR_MODE_128_32B_AX;
+		break;
+
+	case CURSOR_MODE_256_ARGB_AX:
+	case CURSOR_MODE_256_32B_AX:
+		if (alpha)
+			pixformat = CURSOR_MODE_256_ARGB_AX;
+		else
+			pixformat = CURSOR_MODE_256_32B_AX;
+		break;
+
+	case CURSOR_MODE_64_ARGB_AX:
+	case CURSOR_MODE_64_32B_AX:
+		if (alpha)
+			pixformat = CURSOR_MODE_64_ARGB_AX;
+		else
+			pixformat = CURSOR_MODE_64_32B_AX;
+		break;
+	default:
+		if (alpha)
+			DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat);
+		break;
+	}
+	return pixformat;
+}
+
 static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 			     int x, int y)
 {
@@ -2107,6 +2187,9 @@  static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 		BUG();
 	}
 
+	dspcntr |= control_plane_alpha(dspcntr & DISPPLANE_PIXFORMAT_MASK,
+				       intel_crtc->primary_alpha);
+
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (obj->tiling_mode != I915_TILING_NONE)
 			dspcntr |= DISPPLANE_TILED;
@@ -7415,6 +7498,7 @@  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 		if (base) {
 			cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
 			cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
+			cntl |= control_cursor_alpha(cntl & CURSOR_MODE, intel_crtc->cursor_alpha);
 			cntl |= pipe << 28; /* Connect to correct pipe */
 		} else {
 			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
@@ -8755,6 +8839,59 @@  free_work:
 	return ret;
 }
 
+static int intel_set_primary_plane_alpha(struct intel_crtc *crtc,
+                                           unsigned int alpha)
+{
+       struct drm_device *dev = crtc->base.dev;
+       struct drm_i915_private *dev_priv = dev->dev_private;
+       unsigned int old_alpha;
+       int ret = 0;
+
+       old_alpha = crtc->primary_alpha;
+       crtc->primary_alpha = alpha;
+
+       if (!crtc->active)
+               return 0;
+
+       intel_crtc_wait_for_pending_flips(&crtc->base);
+
+       ret = dev_priv->display.update_plane(&crtc->base, crtc->base.fb, 0, 0);
+       if (ret)
+               crtc->primary_alpha = old_alpha;
+
+       return ret;
+}
+
+static void intel_set_cursor_plane_alpha(struct intel_crtc *crtc,
+                                           unsigned int alpha)
+{
+       crtc->cursor_alpha = alpha;
+
+       if (crtc->active)
+              intel_crtc_update_cursor(&crtc->base, true);
+}
+
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+                                    struct drm_property *prop,
+                                    uint64_t val)
+{
+        struct drm_device *dev = crtc->dev;
+        struct drm_i915_private *dev_priv = dev->dev_private;
+        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+        uint64_t old_val;
+        int ret = -ENOENT;
+
+        if (prop == dev_priv->plane_alpha_property) {
+               ret = intel_set_primary_plane_alpha(intel_crtc,
+                                                   intel_crtc->primary_alpha);
+	} else if (prop == dev_priv->cursor_alpha_property) {
+		intel_set_cursor_plane_alpha(intel_crtc, val);
+		return 0;
+	}
+
+	return ret;
+}
+
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
@@ -10167,6 +10304,7 @@  static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = intel_crtc_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_property = intel_crtc_set_property
 };
 
 static void intel_cpu_pll_init(struct drm_device *dev)
@@ -10305,6 +10443,24 @@  static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
+	if (!dev_priv->plane_alpha_property)
+		dev_priv->plane_alpha_property =
+			drm_property_create_range(dev, 0, "plane-alpha", 0, 1);
+
+	if (dev_priv->plane_alpha_property)
+		drm_object_attach_property(&intel_crtc->base.base,
+					dev_priv->plane_alpha_property,
+					intel_crtc->primary_alpha);
+
+       if (!dev_priv->cursor_alpha_property)
+	       dev_priv->cursor_alpha_property =
+			drm_property_create_range(dev, 0, "cursor-alpha", 0, 1);
+
+       if (dev_priv->cursor_alpha_property)
+	       drm_object_attach_property(&intel_crtc->base.base,
+				       dev_priv->cursor_alpha_property,
+				       intel_crtc->cursor_alpha);
+
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a4ffc02..0080d3a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -339,6 +339,9 @@  struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
 	enum plane plane;
+	unsigned int primary_alpha; /* primary plane alpha control */
+        unsigned int cursor_alpha; /* cursor plane alpha control */
+
 	u8 lut_r[256], lut_g[256], lut_b[256];
 	/*
 	 * Whether the crtc and the connected output pipeline is active. Implies
@@ -405,6 +408,7 @@  struct intel_plane {
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y;
 	uint32_t src_w, src_h;
+	unsigned int plane_alpha;
 
 	/* Since we need to change the watermarks before/after
 	 * enabling/disabling the planes, we need to store the parameters here
@@ -676,6 +680,8 @@  int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
 enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 					     enum pipe pipe);
 void intel_wait_for_vblank(struct drm_device *dev, int pipe);
+u32 control_plane_alpha(u32 pixformat, unsigned int alpha);
+u32 control_cursor_alpha(u32 pixformat, unsigned int alpha);
 void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
@@ -906,7 +912,7 @@  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
 void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 			       enum plane plane);
-void intel_plane_restore(struct drm_plane *plane);
+int intel_plane_restore(struct drm_plane *plane);
 void intel_plane_disable(struct drm_plane *plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..9f6c91a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -104,6 +104,9 @@  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		break;
 	}
 
+	sprctl |= control_plane_alpha(sprctl & SP_PIXFORMAT_MASK,
+				       intel_plane->plane_alpha);
+
 	/*
 	 * Enable gamma to match primary/cursor plane behaviour.
 	 * FIXME should be user controllable via propertiesa.
@@ -262,6 +265,9 @@  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		BUG();
 	}
 
+	sprctl |= control_plane_alpha(sprctl & SPRITE_PIXFORMAT_MASK,
+				       intel_plane->plane_alpha);
+
 	/*
 	 * Enable gamma to match primary/cursor plane behaviour.
 	 * FIXME should be user controllable via propertiesa.
@@ -446,6 +452,9 @@  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		BUG();
 	}
 
+	dvscntr |= control_plane_alpha(dvscntr & DVS_PIXFORMAT_MASK,
+				       intel_plane->plane_alpha);
+
 	/*
 	 * Enable gamma to match primary/cursor plane behaviour.
 	 * FIXME should be user controllable via propertiesa.
@@ -1011,18 +1020,38 @@  out_unlock:
 	return ret;
 }
 
-void intel_plane_restore(struct drm_plane *plane)
+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 = -ENOENT;
+
+        if (prop == dev_priv->plane_alpha_property) {
+                old_val = intel_plane->plane_alpha;
+                intel_plane->plane_alpha = val;
+                ret = intel_plane_restore(plane);
+                if (ret)
+                        intel_plane->plane_alpha = old_val;
+        }
+
+        return ret;
+}
+
+int intel_plane_restore(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 
 	if (!plane->crtc || !plane->fb)
-		return;
+		return 0;
 
-	intel_update_plane(plane, plane->crtc, plane->fb,
-			   intel_plane->crtc_x, intel_plane->crtc_y,
-			   intel_plane->crtc_w, intel_plane->crtc_h,
-			   intel_plane->src_x, intel_plane->src_y,
-			   intel_plane->src_w, intel_plane->src_h);
+	return intel_update_plane(plane, plane->crtc, plane->fb,
+				  intel_plane->crtc_x, intel_plane->crtc_y,
+				  intel_plane->crtc_w, intel_plane->crtc_h,
+				  intel_plane->src_x, intel_plane->src_y,
+				  intel_plane->src_w, intel_plane->src_h);
 }
 
 void intel_plane_disable(struct drm_plane *plane)
@@ -1037,6 +1066,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 +1103,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;
@@ -1146,8 +1177,20 @@  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 			     &intel_plane_funcs,
 			     plane_formats, num_plane_formats,
 			     false);
-	if (ret)
+	if (ret) {
 		kfree(intel_plane);
+		goto out;
+	}
+
+	if (!dev_priv->plane_alpha_property)
+		dev_priv->plane_alpha_property =
+			drm_property_create_range(dev, 0, "plane-alpha", 0, 1);
+
+	if (dev_priv->plane_alpha_property)
+		drm_object_attach_property(&intel_plane->base.base,
+					dev_priv->plane_alpha_property,
+					intel_plane->plane_alpha);
 
+out:
 	return ret;
 }