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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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;
> > > >  }
> > > 
> > 
>