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