| Message ID | 1395757947-16492-3-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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; > > } >
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; > > > } > > >
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;
> > > > }
> > >
> >
>
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; > > > > > } > > > > > > > > > >
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; > > > > > > } > > > > > > > > > > > > > > >
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; > > > > > > > } > > > > > > > > > > > > > > > > > > > > >
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; }