| Message ID | 1392804488-5551-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Gentle Reminder !!! Kindly review and provide feedback thanks, Sagar On Wed, 2014-02-19 at 15:38 +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > With this patch two properties are added. One for CRTC+Sprite planes > and another for Cursor planes. Through these client will be able to > change the pixel format of the planes w.r.t Alpha channel. > Number of drm properties are limited so should we restrain from adding this > as drm property or should we expose this as IOCTL itself. > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_reg.h | 6 ++ > drivers/gpu/drm/i915/intel_display.c | 156 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 8 +- > drivers/gpu/drm/i915/intel_sprite.c | 59 +++++++++++-- > 5 files changed, 222 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8c64831..5ced53d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1560,6 +1560,8 @@ typedef struct drm_i915_private { > > struct drm_property *broadcast_rgb_property; > struct drm_property *force_audio_property; > + struct drm_property *plane_alpha_property; > + struct drm_property *cursor_alpha_property; > > uint32_t hw_context_size; > struct list_head context_list; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2f564ce..039270e 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3522,7 +3522,11 @@ > /* New style CUR*CNTR flags */ > #define CURSOR_MODE 0x27 > #define CURSOR_MODE_DISABLE 0x00 > +#define CURSOR_MODE_128_32B_AX 0x02 > +#define CURSOR_MODE_256_32B_AX 0x03 > #define CURSOR_MODE_64_32B_AX 0x07 > +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) > +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) > #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) > #define MCURSOR_PIPE_SELECT (1 << 28) > #define MCURSOR_PIPE_A 0x00 > @@ -3569,7 +3573,9 @@ > #define DISPPLANE_RGBX101010 (0x8<<26) > #define DISPPLANE_RGBA101010 (0x9<<26) > #define DISPPLANE_BGRX101010 (0xa<<26) > +#define DISPPLANE_BGRA101010 (0xb<<26) > #define DISPPLANE_RGBX161616 (0xc<<26) > +#define DISPPLANE_RGBA161616 (0xd<<26) > #define DISPPLANE_RGBX888 (0xe<<26) > #define DISPPLANE_RGBA888 (0xf<<26) > #define DISPPLANE_STEREO_ENABLE (1<<25) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f19e6ea..a854bcb 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2047,6 +2047,86 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y, > } > } > > +u32 control_plane_alpha(u32 pixformat, unsigned int alpha) > +{ > + switch (pixformat) { > + case DISPPLANE_RGBX888: > + case DISPPLANE_RGBA888: > + if (alpha) > + pixformat = DISPPLANE_RGBA888; > + else > + pixformat = DISPPLANE_RGBX888; > + break; > + case DISPPLANE_BGRX888: > + case DISPPLANE_BGRA888: > + if (alpha) > + pixformat = DISPPLANE_BGRA888; > + else > + pixformat = DISPPLANE_BGRX888; > + break; > + case DISPPLANE_RGBX101010: > + case DISPPLANE_RGBA101010: > + if (alpha) > + pixformat = DISPPLANE_RGBA101010; > + else > + pixformat = DISPPLANE_RGBX101010; > + break; > + case DISPPLANE_BGRX101010: > + case DISPPLANE_BGRA101010: > + if (alpha) > + pixformat = DISPPLANE_BGRA101010; > + else > + pixformat = DISPPLANE_BGRX101010; > + break; > + case DISPPLANE_RGBX161616: > + case DISPPLANE_RGBA161616: > + if (alpha) > + pixformat = DISPPLANE_RGBA161616; > + else > + pixformat = DISPPLANE_RGBX161616; > + break; > + default: > + if (alpha) > + DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat); > + break; > + } > + return pixformat; > +} > + > +u32 control_cursor_alpha(u32 pixformat, unsigned int alpha) > +{ > + switch (pixformat) { > + case CURSOR_MODE_128_32B_AX: > + case CURSOR_MODE_128_ARGB_AX: > + if (alpha) > + pixformat = CURSOR_MODE_128_ARGB_AX; > + else > + pixformat = CURSOR_MODE_128_32B_AX; > + break; > + > + case CURSOR_MODE_256_ARGB_AX: > + case CURSOR_MODE_256_32B_AX: > + if (alpha) > + pixformat = CURSOR_MODE_256_ARGB_AX; > + else > + pixformat = CURSOR_MODE_256_32B_AX; > + break; > + > + case CURSOR_MODE_64_ARGB_AX: > + case CURSOR_MODE_64_32B_AX: > + if (alpha) > + pixformat = CURSOR_MODE_64_ARGB_AX; > + else > + pixformat = CURSOR_MODE_64_32B_AX; > + break; > + default: > + if (alpha) > + DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat); > + break; > + } > + return pixformat; > +} > + > static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb, > int x, int y) > { > @@ -2107,6 +2187,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb, > BUG(); > } > > + dspcntr |= control_plane_alpha(dspcntr & DISPPLANE_PIXFORMAT_MASK, > + intel_crtc->primary_alpha); > + > if (INTEL_INFO(dev)->gen >= 4) { > if (obj->tiling_mode != I915_TILING_NONE) > dspcntr |= DISPPLANE_TILED; > @@ -7415,6 +7498,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > if (base) { > cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + cntl |= control_cursor_alpha(cntl & CURSOR_MODE, intel_crtc->cursor_alpha); > cntl |= pipe << 28; /* Connect to correct pipe */ > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > @@ -8755,6 +8839,59 @@ free_work: > return ret; > } > > +static int intel_set_primary_plane_alpha(struct intel_crtc *crtc, > + unsigned int alpha) > +{ > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned int old_alpha; > + int ret = 0; > + > + old_alpha = crtc->primary_alpha; > + crtc->primary_alpha = alpha; > + > + if (!crtc->active) > + return 0; > + > + intel_crtc_wait_for_pending_flips(&crtc->base); > + > + ret = dev_priv->display.update_plane(&crtc->base, crtc->base.fb, 0, 0); > + if (ret) > + crtc->primary_alpha = old_alpha; > + > + return ret; > +} > + > +static void intel_set_cursor_plane_alpha(struct intel_crtc *crtc, > + unsigned int alpha) > +{ > + crtc->cursor_alpha = alpha; > + > + if (crtc->active) > + intel_crtc_update_cursor(&crtc->base, true); > +} > + > +static int intel_crtc_set_property(struct drm_crtc *crtc, > + struct drm_property *prop, > + uint64_t val) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == dev_priv->plane_alpha_property) { > + ret = intel_set_primary_plane_alpha(intel_crtc, > + intel_crtc->primary_alpha); > + } else if (prop == dev_priv->cursor_alpha_property) { > + intel_set_cursor_plane_alpha(intel_crtc, val); > + return 0; > + } > + > + return ret; > +} > + > static struct drm_crtc_helper_funcs intel_helper_funcs = { > .mode_set_base_atomic = intel_pipe_set_base_atomic, > .load_lut = intel_crtc_load_lut, > @@ -10167,6 +10304,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = { > .set_config = intel_crtc_set_config, > .destroy = intel_crtc_destroy, > .page_flip = intel_crtc_page_flip, > + .set_property = intel_crtc_set_property > }; > > static void intel_cpu_pll_init(struct drm_device *dev) > @@ -10305,6 +10443,24 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; > dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; > > + if (!dev_priv->plane_alpha_property) > + dev_priv->plane_alpha_property = > + drm_property_create_range(dev, 0, "plane-alpha", 0, 1); > + > + if (dev_priv->plane_alpha_property) > + drm_object_attach_property(&intel_crtc->base.base, > + dev_priv->plane_alpha_property, > + intel_crtc->primary_alpha); > + > + if (!dev_priv->cursor_alpha_property) > + dev_priv->cursor_alpha_property = > + drm_property_create_range(dev, 0, "cursor-alpha", 0, 1); > + > + if (dev_priv->cursor_alpha_property) > + drm_object_attach_property(&intel_crtc->base.base, > + dev_priv->cursor_alpha_property, > + intel_crtc->cursor_alpha); > + > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index a4ffc02..0080d3a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -339,6 +339,9 @@ struct intel_crtc { > struct drm_crtc base; > enum pipe pipe; > enum plane plane; > + unsigned int primary_alpha; /* primary plane alpha control */ > + unsigned int cursor_alpha; /* cursor plane alpha control */ > + > u8 lut_r[256], lut_g[256], lut_b[256]; > /* > * Whether the crtc and the connected output pipeline is active. Implies > @@ -405,6 +408,7 @@ struct intel_plane { > unsigned int crtc_w, crtc_h; > uint32_t src_x, src_y; > uint32_t src_w, src_h; > + unsigned int plane_alpha; > > /* Since we need to change the watermarks before/after > * enabling/disabling the planes, we need to store the parameters here > @@ -676,6 +680,8 @@ int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, > enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, > enum pipe pipe); > void intel_wait_for_vblank(struct drm_device *dev, int pipe); > +u32 control_plane_alpha(u32 pixformat, unsigned int alpha); > +u32 control_cursor_alpha(u32 pixformat, unsigned int alpha); > void intel_wait_for_pipe_off(struct drm_device *dev, int pipe); > int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp); > void vlv_wait_port_ready(struct drm_i915_private *dev_priv, > @@ -906,7 +912,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob); > int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane); > void intel_flush_primary_plane(struct drm_i915_private *dev_priv, > enum plane plane); > -void intel_plane_restore(struct drm_plane *plane); > +int intel_plane_restore(struct drm_plane *plane); > void intel_plane_disable(struct drm_plane *plane); > int intel_sprite_set_colorkey(struct drm_device *dev, void *data, > struct drm_file *file_priv); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 336ae6c..9f6c91a 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -104,6 +104,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > break; > } > > + sprctl |= control_plane_alpha(sprctl & SP_PIXFORMAT_MASK, > + intel_plane->plane_alpha); > + > /* > * Enable gamma to match primary/cursor plane behaviour. > * FIXME should be user controllable via propertiesa. > @@ -262,6 +265,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > BUG(); > } > > + sprctl |= control_plane_alpha(sprctl & SPRITE_PIXFORMAT_MASK, > + intel_plane->plane_alpha); > + > /* > * Enable gamma to match primary/cursor plane behaviour. > * FIXME should be user controllable via propertiesa. > @@ -446,6 +452,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > BUG(); > } > > + dvscntr |= control_plane_alpha(dvscntr & DVS_PIXFORMAT_MASK, > + intel_plane->plane_alpha); > + > /* > * Enable gamma to match primary/cursor plane behaviour. > * FIXME should be user controllable via propertiesa. > @@ -1011,18 +1020,38 @@ out_unlock: > return ret; > } > > -void intel_plane_restore(struct drm_plane *plane) > +static int intel_plane_set_property(struct drm_plane *plane, > + struct drm_property *prop, > + uint64_t val) > +{ > + struct drm_i915_private *dev_priv = plane->dev->dev_private; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == dev_priv->plane_alpha_property) { > + old_val = intel_plane->plane_alpha; > + intel_plane->plane_alpha = val; > + ret = intel_plane_restore(plane); > + if (ret) > + intel_plane->plane_alpha = old_val; > + } > + > + return ret; > +} > + > +int intel_plane_restore(struct drm_plane *plane) > { > struct intel_plane *intel_plane = to_intel_plane(plane); > > if (!plane->crtc || !plane->fb) > - return; > + return 0; > > - intel_update_plane(plane, plane->crtc, plane->fb, > - intel_plane->crtc_x, intel_plane->crtc_y, > - intel_plane->crtc_w, intel_plane->crtc_h, > - intel_plane->src_x, intel_plane->src_y, > - intel_plane->src_w, intel_plane->src_h); > + return intel_update_plane(plane, plane->crtc, plane->fb, > + intel_plane->crtc_x, intel_plane->crtc_y, > + intel_plane->crtc_w, intel_plane->crtc_h, > + intel_plane->src_x, intel_plane->src_y, > + intel_plane->src_w, intel_plane->src_h); > } > > void intel_plane_disable(struct drm_plane *plane) > @@ -1037,6 +1066,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { > .update_plane = intel_update_plane, > .disable_plane = intel_disable_plane, > .destroy = intel_destroy_plane, > + .set_property = intel_plane_set_property, > }; > > static uint32_t ilk_plane_formats[] = { > @@ -1073,6 +1103,7 @@ static uint32_t vlv_plane_formats[] = { > int > intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_plane *intel_plane; > unsigned long possible_crtcs; > const uint32_t *plane_formats; > @@ -1146,8 +1177,20 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > &intel_plane_funcs, > plane_formats, num_plane_formats, > false); > - if (ret) > + if (ret) { > kfree(intel_plane); > + goto out; > + } > + > + if (!dev_priv->plane_alpha_property) > + dev_priv->plane_alpha_property = > + drm_property_create_range(dev, 0, "plane-alpha", 0, 1); > + > + if (dev_priv->plane_alpha_property) > + drm_object_attach_property(&intel_plane->base.base, > + dev_priv->plane_alpha_property, > + intel_plane->plane_alpha); > > +out: > return ret; > }
On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > With this patch two properties are added. One for CRTC+Sprite planes > and another for Cursor planes. Through these client will be able to > change the pixel format of the planes w.r.t Alpha channel. > Number of drm properties are limited so should we restrain from adding this > as drm property or should we expose this as IOCTL itself. We need to stop adding properties on a whim and design and document them properly. So what I'd like to see is someone going over the current properties and collecting them up in some (even bares bones) documentation in Documentation/Docbook. The current way will lead to a huge mess when userspace actually starts to depend on properties. So far properties have been mostly some optional extra junk on the side people that may want to frob, but that's going to change as we add more of them, especially with plane and crtc properties, which actually affect how the scene gets composed together. And I think we need to put a hold on adding the plane properties to the crtc since the plan is to convert everything to drm_plane. With the current rate we're going to have a ton of properties on the crtc that no one will use. Adding properties to the sprite planes seems OK in the meantime. As far as alpha blending is concerned I've had the following ideas: - we need a plane property for constant alpha. Some drivers might already have this, so might be good to check. Although I'm fairly sure what's there won't be entirely future proof. I was thinking that we should standardize of using 16bits for color components in properties. That way you can still stick a full ARGB value into one property, and we should be good for a few more years until someone has the idea to move beyond 16bits per channel. And it's more or less hardware agnostic. Obviously if the hardware won't use the full precision, you get to throw away the low bits, but I don't think there's any other good way to go. - we need another property to indicate whether the source pixels are premultiplied or not. Or maybe it's easier for people to think in terms of what operations the hardware will do, in which case we should make the property indicate whether the hardware will do the premultiplication during blending or not. I'm not sure which approach feels more natural to people. - And finally we need to figure out how to blend it all together. It might make sense to model this after glBlendFunc(), so it would be an enum property, or maybe two if we want separate properties for source and destination factors. Obviously the final result will depend on additional things like the z-order, which is going to be another property. I think this one might already exists in some form in other drivers. So we should definitely look at what's there and try to do the same if possible. Which again underlines the need to collect up the current properties into some central documentation.
On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote: > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote: > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > With this patch two properties are added. One for CRTC+Sprite planes > > and another for Cursor planes. Through these client will be able to > > change the pixel format of the planes w.r.t Alpha channel. > > Number of drm properties are limited so should we restrain from adding this > > as drm property or should we expose this as IOCTL itself. > > We need to stop adding properties on a whim and design and document them > properly. So what I'd like to see is someone going over the current > properties and collecting them up in some (even bares bones) documentation > in Documentation/Docbook. The current way will lead to a huge mess when > userspace actually starts to depend on properties. So far properties > have been mostly some optional extra junk on the side people that may > want to frob, but that's going to change as we add more of them, > especially with plane and crtc properties, which actually affect how > the scene gets composed together. > > And I think we need to put a hold on adding the plane properties to the > crtc since the plan is to convert everything to drm_plane. With the > current rate we're going to have a ton of properties on the crtc that no > one will use. Adding properties to the sprite planes seems OK in the > meantime. > > As far as alpha blending is concerned I've had the following ideas: > - we need a plane property for constant alpha. Some drivers might > already have this, so might be good to check. Although I'm fairly > sure what's there won't be entirely future proof. I was thinking that > we should standardize of using 16bits for color components in > properties. That way you can still stick a full ARGB value into > one property, and we should be good for a few more years until > someone has the idea to move beyond 16bits per channel. And it's > more or less hardware agnostic. Obviously if the hardware won't > use the full precision, you get to throw away the low bits, but I > don't think there's any other good way to go. > - we need another property to indicate whether the source pixels > are premultiplied or not. Or maybe it's easier for people to think > in terms of what operations the hardware will do, in which case > we should make the property indicate whether the hardware will > do the premultiplication during blending or not. I'm not sure > which approach feels more natural to people. > - And finally we need to figure out how to blend it all together. > It might make sense to model this after glBlendFunc(), so it would > be an enum property, or maybe two if we want separate properties > for source and destination factors. > > Obviously the final result will depend on additional things like the > z-order, which is going to be another property. I think this one might > already exists in some form in other drivers. So we should definitely > look at what's there and try to do the same if possible. Which again > underlines the need to collect up the current properties into some > central documentation. Concurred on stealing the blending model from GL. It seems what everyon else is aiming for at least at both the hw and sw level ... One issue with that is handling color keys, since those aren't supported by glBlendFunc. But I guess we could just add those as additional modes, since the usual blend funcs already require a constant blend color, which could be reused as the color key. -Daniel
On Tue, Mar 04, 2014 at 10:42:38AM +0100, Daniel Vetter wrote: > On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote: > > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote: > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > With this patch two properties are added. One for CRTC+Sprite planes > > > and another for Cursor planes. Through these client will be able to > > > change the pixel format of the planes w.r.t Alpha channel. > > > Number of drm properties are limited so should we restrain from adding this > > > as drm property or should we expose this as IOCTL itself. > > > > We need to stop adding properties on a whim and design and document them > > properly. So what I'd like to see is someone going over the current > > properties and collecting them up in some (even bares bones) documentation > > in Documentation/Docbook. The current way will lead to a huge mess when > > userspace actually starts to depend on properties. So far properties > > have been mostly some optional extra junk on the side people that may > > want to frob, but that's going to change as we add more of them, > > especially with plane and crtc properties, which actually affect how > > the scene gets composed together. > > > > And I think we need to put a hold on adding the plane properties to the > > crtc since the plan is to convert everything to drm_plane. With the > > current rate we're going to have a ton of properties on the crtc that no > > one will use. Adding properties to the sprite planes seems OK in the > > meantime. > > > > As far as alpha blending is concerned I've had the following ideas: > > - we need a plane property for constant alpha. Some drivers might > > already have this, so might be good to check. Although I'm fairly > > sure what's there won't be entirely future proof. I was thinking that > > we should standardize of using 16bits for color components in > > properties. That way you can still stick a full ARGB value into > > one property, and we should be good for a few more years until > > someone has the idea to move beyond 16bits per channel. And it's > > more or less hardware agnostic. Obviously if the hardware won't > > use the full precision, you get to throw away the low bits, but I > > don't think there's any other good way to go. > > - we need another property to indicate whether the source pixels > > are premultiplied or not. Or maybe it's easier for people to think > > in terms of what operations the hardware will do, in which case > > we should make the property indicate whether the hardware will > > do the premultiplication during blending or not. I'm not sure > > which approach feels more natural to people. > > - And finally we need to figure out how to blend it all together. > > It might make sense to model this after glBlendFunc(), so it would > > be an enum property, or maybe two if we want separate properties > > for source and destination factors. > > > > Obviously the final result will depend on additional things like the > > z-order, which is going to be another property. I think this one might > > already exists in some form in other drivers. So we should definitely > > look at what's there and try to do the same if possible. Which again > > underlines the need to collect up the current properties into some > > central documentation. > > Concurred on stealing the blending model from GL. It seems what everyon > else is aiming for at least at both the hw and sw level ... One issue with > that is handling color keys, since those aren't supported by glBlendFunc. > But I guess we could just add those as additional modes, since the usual > blend funcs already require a constant blend color, which could be reused > as the color key. I've been thinking that color key stuff could just be implemented as separate properties. Color keying anyway may require min+max values or value+mask, so a single value is not enough. But we might want to model the keying function in a similar fashion to the blendfunc. Some old ATI hardware had a very flexible color keying functinality where you would specify the graphics and video keying functions (true,false,eq,neq) and then you had to choose how to combine the result from those (and,or). If the combined result was true it would pick the video data, and for false it'd pick the graphics data. I've not seen that on any other hardware since, but it's certainly flexible enough to represent the typical src/dst keying modes.
On Tue, 2014-03-04 at 14:06 +0200, Ville Syrjälä wrote: > On Tue, Mar 04, 2014 at 10:42:38AM +0100, Daniel Vetter wrote: > > On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote: > > > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote: > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > > > With this patch two properties are added. One for CRTC+Sprite planes > > > > and another for Cursor planes. Through these client will be able to > > > > change the pixel format of the planes w.r.t Alpha channel. > > > > Number of drm properties are limited so should we restrain from adding this > > > > as drm property or should we expose this as IOCTL itself. > > > > > > We need to stop adding properties on a whim and design and document them > > > properly. So what I'd like to see is someone going over the current > > > properties and collecting them up in some (even bares bones) documentation > > > in Documentation/Docbook. The current way will lead to a huge mess when > > > userspace actually starts to depend on properties. So far properties > > > have been mostly some optional extra junk on the side people that may > > > want to frob, but that's going to change as we add more of them, > > > especially with plane and crtc properties, which actually affect how > > > the scene gets composed together. > > > > > > And I think we need to put a hold on adding the plane properties to the > > > crtc since the plan is to convert everything to drm_plane. With the > > > current rate we're going to have a ton of properties on the crtc that no > > > one will use. Adding properties to the sprite planes seems OK in the > > > meantime. > > > > > > As far as alpha blending is concerned I've had the following ideas: > > > - we need a plane property for constant alpha. Some drivers might > > > already have this, so might be good to check. Although I'm fairly > > > sure what's there won't be entirely future proof. I was thinking that > > > we should standardize of using 16bits for color components in > > > properties. That way you can still stick a full ARGB value into > > > one property, and we should be good for a few more years until > > > someone has the idea to move beyond 16bits per channel. And it's > > > more or less hardware agnostic. Obviously if the hardware won't > > > use the full precision, you get to throw away the low bits, but I > > > don't think there's any other good way to go. > > > - we need another property to indicate whether the source pixels > > > are premultiplied or not. Or maybe it's easier for people to think > > > in terms of what operations the hardware will do, in which case > > > we should make the property indicate whether the hardware will > > > do the premultiplication during blending or not. I'm not sure > > > which approach feels more natural to people. > > > - And finally we need to figure out how to blend it all together. > > > It might make sense to model this after glBlendFunc(), so it would > > > be an enum property, or maybe two if we want separate properties > > > for source and destination factors. > > > > > > Obviously the final result will depend on additional things like the > > > z-order, which is going to be another property. I think this one might > > > already exists in some form in other drivers. So we should definitely > > > look at what's there and try to do the same if possible. Which again > > > underlines the need to collect up the current properties into some > > > central documentation. > > > > Concurred on stealing the blending model from GL. It seems what everyon > > else is aiming for at least at both the hw and sw level ... One issue with > > that is handling color keys, since those aren't supported by glBlendFunc. > > But I guess we could just add those as additional modes, since the usual > > blend funcs already require a constant blend color, which could be reused > > as the color key. > > I've been thinking that color key stuff could just be implemented as > separate properties. > > Color keying anyway may require min+max values or value+mask, so a single > value is not enough. But we might want to model the keying function in > a similar fashion to the blendfunc. Some old ATI hardware had a very > flexible color keying functinality where you would specify the graphics > and video keying functions (true,false,eq,neq) and then you had to > choose how to combine the result from those (and,or). If the combined > result was true it would pick the video data, and for false it'd pick > the graphics data. I've not seen that on any other hardware since, but > it's certainly flexible enough to represent the typical src/dst keying > modes. We can only model GL_CONSTANT_ALPHA given we have sprite control registers for that. How do we model other pixel arithmetic related to color blending? How do we model source and destination pixel arithmetic? We can only set alpha/pre-multiplied alpha for individual planes. >
On Thu, Mar 6, 2014 at 11:28 AM, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > We can only model GL_CONSTANT_ALPHA given we have sprite control > registers for that. How do we model other pixel arithmetic related to > color blending? > How do we model source and destination pixel arithmetic? We can only set > alpha/pre-multiplied alpha for individual planes. We reject it if userspace attempts it ;-) But we kinda want a model which also works for the next few hw platforms and also on other drivers, so going with this sounds like the right approach to me. -Daniel
On Mon, Feb 24, 2014 at 09:14:41PM +0530, Sagar Arun Kamble wrote: > Gentle Reminder !!! > Kindly review and provide feedback This main issue here is that we don't have yet the split between the CRTC and the primary plane in the DRM API. Why is this a problem? because you need to define a 'plane-alpha' property on the CRTC and we'll have to maintain it forever and we don't really want that when a neat split between the CRTC and primary plane is not far off. Work is ongoing on this, Matt bravely took the task: http://lists.freedesktop.org/archives/dri-devel/2014-February/054719.html With a v2 in the queue. I believe we need to sort this first, before adding more plane properties.
On Thu, Mar 06, 2014 at 12:03:07PM +0000, Damien Lespiau wrote: > On Mon, Feb 24, 2014 at 09:14:41PM +0530, Sagar Arun Kamble wrote: > > Gentle Reminder !!! > > Kindly review and provide feedback > > This main issue here is that we don't have yet the split between the > CRTC and the primary plane in the DRM API. > > Why is this a problem? because you need to define a 'plane-alpha' > property on the CRTC and we'll have to maintain it forever and we don't > really want that when a neat split between the CRTC and primary plane is > not far off. > > Work is ongoing on this, Matt bravely took the task: > > http://lists.freedesktop.org/archives/dri-devel/2014-February/054719.html > > With a v2 in the queue. > > I believe we need to sort this first, before adding more plane properties. Ah, didn't see the rest of the thread because my MUA decided to split the thread between two directories. Scrap that mail :)
From: Sagar Kamble <sagar.a.kamble@intel.com>
This patch series introduces drm property modelled after glBlendFuc function. For i915
constant alpha is exposed through this property to start with. Additional new property
value for controlling pre-multiplied alpha is added.
i-g-t test case is to be added.
These patches are based on following patches which are already under review/reviewed:
4ae74f3 Documentation: drm: describing drm properties exposed by various drivers
134bdfe Propagate the error from intel_update_plane() up through intel_plane_restore() to the caller.
a6ad21c Make drm_property_create_bitmask() a bit more generic by allowing the caller to specify which bits are in fact supported.
Sagar Kamble (4):
drm: Added plane alpha and color blending property
drm/i915: Enabling constant alpha drm property
drm/i915: Enabling pre-multiplied alpha drm property
Documentation: drm: describing plane alpha and color blending property
Documentation/DocBook/drm.tmpl | 13 +++-
drivers/gpu/drm/drm_crtc.c | 33 +++++++++
drivers/gpu/drm/i915/i915_dma.c | 11 ++-
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_reg.h | 2 +
drivers/gpu/drm/i915/intel_drv.h | 8 +++
drivers/gpu/drm/i915/intel_sprite.c | 132 +++++++++++++++++++++++++++++++++++-
include/drm/drm_crtc.h | 25 +++++++
8 files changed, 222 insertions(+), 3 deletions(-)
On Sat, Mar 08, 2014 at 01:51:15PM +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > This patch series introduces drm property modelled after glBlendFuc function. For i915 > constant alpha is exposed through this property to start with. Additional new property > value for controlling pre-multiplied alpha is added. > i-g-t test case is to be added. > > These patches are based on following patches which are already under review/reviewed: > > 4ae74f3 Documentation: drm: describing drm properties exposed by various drivers > 134bdfe Propagate the error from intel_update_plane() up through intel_plane_restore() to the caller. > a6ad21c Make drm_property_create_bitmask() a bit more generic by allowing the caller to specify which bits are in fact supported. > > Sagar Kamble (4): > drm: Added plane alpha and color blending property > drm/i915: Enabling constant alpha drm property > drm/i915: Enabling pre-multiplied alpha drm property > Documentation: drm: describing plane alpha and color blending property I'm not sure I follow what's being done with the GL blending equation here. If we want to support a global alpha on the source plane (ie the plane that is going to be blended into the "render target", what's already there), the blending equation looks like: (source is non-premultiplied, note the use of glBlendFuncSeparate()) RGB = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) A = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA) (source is premultiplied) RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) The gl blending stuff doesn't know anything about the premultiplied nature of the incoming source color, it has to be programmed accordingly.
On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote: > (source is premultiplied) > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) Grr, copy/paste error. If the source is already premultiplied: RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA)
Hi Damien, On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote: > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote: > > (source is premultiplied) > > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > Grr, copy/paste error. If the source is already premultiplied: > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA) > 1. Currently there is no interface to advertise the DRM_FORMATS plane supportes to user mode? Should we add new IOCTL for that and include pre-multiplied alpha formats while advertising? Or am I missing any such API already available? 2. About constant alpha property - when we program constant alpha register will hardware be able to take care of the blending as per equations you have specified for non-premultiplied-alpha and premultiplied-alpha cases or we have to do any additional setting? Confusion is because of two combinations: a. pre-multiplied alpha+constant alpha b. non-pre-multiplied alpha+constant alpha Kindly clarify. thanks, Sagar
On Fri, Mar 21, 2014 at 07:06:46PM +0530, Sagar Arun Kamble wrote: > Hi Damien, > > On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote: > > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote: > > > (source is premultiplied) > > > > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > Grr, copy/paste error. If the source is already premultiplied: > > > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > 1. Currently there is no interface to advertise the DRM_FORMATS plane > supportes to user mode? Should we add new IOCTL for that and include > pre-multiplied alpha formats while advertising? Or am I missing any such > API already available? There's a 'formats' array in drmModePlane. > 2. About constant alpha property - when we program constant alpha > register will hardware be able to take care of the blending as per > equations you have specified for non-premultiplied-alpha and > premultiplied-alpha cases or we have to do any additional setting? > Confusion is because of two combinations: > a. pre-multiplied alpha+constant alpha > b. non-pre-multiplied alpha+constant alpha The first part of the question should be in the spec. I really do expect the hw to work correctly with any combination of (premul, non-premult) x (plane alpha, no plane alpha) To be more clear: I don't think the glBlendFunc constants can represent everything we want: - It'd need to differentiate what we want do to between RGB and A (the same reason glBlendFuncSeparate was introduced). - We can't represent blending of an unpremultied fb with a plane-global alpha with just the GL blending equation as it needs two multiplications (so in GL, you would do one of them in the fragment shader). I would just proceed with a premultipled FB format and the alpha plane property. Let's wait until Ville returns, I may be totally not getting what he meant.
Hi Damien, Could you please clarify following queries. Thanks, Sagar On Fri, 2014-03-21 at 19:06 +0530, Sagar Arun Kamble wrote: > Hi Damien, > > On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote: > > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote: > > > (source is premultiplied) > > > > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > Grr, copy/paste error. If the source is already premultiplied: > > > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > 1. Currently there is no interface to advertise the DRM_FORMATS plane > supportes to user mode? Should we add new IOCTL for that and include > pre-multiplied alpha formats while advertising? Or am I missing any such > API already available? > > 2. About constant alpha property - when we program constant alpha > register will hardware be able to take care of the blending as per > equations you have specified for non-premultiplied-alpha and > premultiplied-alpha cases or we have to do any additional setting? > Confusion is because of two combinations: > a. pre-multiplied alpha+constant alpha > b. non-pre-multiplied alpha+constant alpha > > Kindly clarify. > > thanks, > Sagar > >
On Tue, Mar 25, 2014 at 03:33:42PM +0530, Sagar Arun Kamble wrote: > Hi Damien, > > Could you please clarify following queries. He did, in a reply to your mail ... -Daniel > > Thanks, > Sagar > On Fri, 2014-03-21 at 19:06 +0530, Sagar Arun Kamble wrote: > > Hi Damien, > > > > On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote: > > > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote: > > > > (source is premultiplied) > > > > > > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > > > Grr, copy/paste error. If the source is already premultiplied: > > > > > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > > > > 1. Currently there is no interface to advertise the DRM_FORMATS plane > > supportes to user mode? Should we add new IOCTL for that and include > > pre-multiplied alpha formats while advertising? Or am I missing any such > > API already available? > > > > 2. About constant alpha property - when we program constant alpha > > register will hardware be able to take care of the blending as per > > equations you have specified for non-premultiplied-alpha and > > premultiplied-alpha cases or we have to do any additional setting? > > Confusion is because of two combinations: > > a. pre-multiplied alpha+constant alpha > > b. non-pre-multiplied alpha+constant alpha > > > > Kindly clarify. > > > > thanks, > > Sagar > > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 25, 2014 at 11:51:57AM +0100, Daniel Vetter wrote: > On Tue, Mar 25, 2014 at 03:33:42PM +0530, Sagar Arun Kamble wrote: > > Hi Damien, > > > > Could you please clarify following queries. > > He did, in a reply to your mail ... In case you cannot find it: http://lists.freedesktop.org/archives/intel-gfx/2014-March/042136.html
On Fri, Mar 21, 2014 at 07:23:31PM +0000, Damien Lespiau wrote: > On Fri, Mar 21, 2014 at 07:06:46PM +0530, Sagar Arun Kamble wrote: > > Hi Damien, > > > > On Thu, 2014-03-20 at 14:45 +0000, Damien Lespiau wrote: > > > On Thu, Mar 20, 2014 at 02:11:40PM +0000, Damien Lespiau wrote: > > > > (source is premultiplied) > > > > > > > > RGBA = ADD(SRC_COLOR*SRC_ALPHA, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > > > Grr, copy/paste error. If the source is already premultiplied: > > > > > > RGBA = ADD(SRC_COLOR, DST_COLOR*ONE_MINUS_SRC_ALPHA) > > > > > > > 1. Currently there is no interface to advertise the DRM_FORMATS plane > > supportes to user mode? Should we add new IOCTL for that and include > > pre-multiplied alpha formats while advertising? Or am I missing any such > > API already available? > > There's a 'formats' array in drmModePlane. > > > 2. About constant alpha property - when we program constant alpha > > register will hardware be able to take care of the blending as per > > equations you have specified for non-premultiplied-alpha and > > premultiplied-alpha cases or we have to do any additional setting? > > Confusion is because of two combinations: > > a. pre-multiplied alpha+constant alpha > > b. non-pre-multiplied alpha+constant alpha > > The first part of the question should be in the spec. I really do expect > the hw to work correctly with any combination of (premul, non-premult) x > (plane alpha, no plane alpha) > > To be more clear: I don't think the glBlendFunc constants can represent > everything we want: > > - It'd need to differentiate what we want do to between RGB and A (the > same reason glBlendFuncSeparate was introduced). We won't be writing out the alpha anywhere so I'm thinking we don't need to worry about it. > > - We can't represent blending of an unpremultied fb with a plane-global > alpha with just the GL blending equation as it needs two > multiplications (so in GL, you would do one of them in the fragment > shader). If I'm interpreting the docs correctly these are the kind of blend equations we will have soonish: Sc = Sc Dc = Sc * Ca + (1-Ca) * Dc Sc = Sc Dc = Sc * 1 + (1-Sa) * Dc premultiplied? 0: Sc = Sc * Sa 1: Sc = Sc Dc = Sc * Sa*Ca + (1-Sa*Ca) * Dc premultiplied? 0: Sc = Sc * Sa 1: Sc = Sc Dc = Sc * 1 + (1-Sa) * Dc For reference OMAP36xx was something like this I think: premultiplied? 0: Sc = Sc * Sa 1: Sc = Sc Dc = Sc * Ca + (1-Sa*Ca) * Dc and OMAP34xx was something like this: Sc = Sc Dc = Sc * Sa*Ca + (1-Sa*Ca) * Dc So we have src factors of: 1, Ca, Sa*Ca and dst factors of: 0, 1-Ca, 1-Sa, 1-Sa*Ca If we add the missing Sa src factor we have a fairly sensible looking set. Obviously the constant alpha related factors aren't part of GL, but I don't think we need to worry about that, we're not implementing GL here after all, just something that resembles it a bit to make it easier for people to grasp it. So to make stuff easier for userspace to figure out what we actually support, I'm thinking it could be just an enum property with the values being something like '(dstf << 32) | (srcf << 0)' with the factors defined something like so: enum { ZERO, ONE, SRC_ALPHA, ONE_MINUS_SRC_ALPHA, CONST_ALPHA, ONE_MINUS_CONST_ALPHA, SRC_CONST_ALPHA, ONE_MINUS_SRC_CONST_ALPHA, }; If it's an enum then the user can just pick out one of the supported blend equations. > I would just proceed with a premultipled FB format and the alpha plane > property. I'm not very enthusiastic about adding the pre-multiplied info to the FB. That means adding new versions of all the alpha formats we already have. I almost regret making the A vs. X distinction already. Might have been neater to leave it entirely up to the blending properties to figure out whether there's alpha or not. The one not so annoying option would be to snatch one of the remaining high bits and use that to indicate premult or not. But we still need to figure out whether we make the already existing formats be the premultiplied or non-premultiplied ones. That could have some implication for user space as we probably can't radically change which formats we accept to keep existing userspace functional. So the other option is to not think of it as a property of the framebuffer, but rather think of it in terms of what operations the hardware will perform during blending. That way we could just add a flag which says "yes please, premultiply the data for me". One thing I didn't much think about is whether we need per-plane blend functions. That would be the more flexible option, but at least the current hardware doesn't seem to allow such flexibility. I guess being future proof shouldn't hurt. We do have the option of per-plane premultiplication at least. Hmm, actually I have to take that back as the first two example functions I listed can appear simultaneosly AFAICS. So this does make things a bit more complicated for userspace as it can't freely pick blend functions when dealing with multiple planes. Oh and also for OMAPs the bottom plane wouldn't be blended with the background AFAICS, so it would always have a blend function of ONE,ZERO. OK, so that's my thoughts on the matter. Pardon me if I repeated some nonsense that has already been shot down. I didn't have time to look at the actual patches yet.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8c64831..5ced53d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1560,6 +1560,8 @@ typedef struct drm_i915_private { struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property; + struct drm_property *plane_alpha_property; + struct drm_property *cursor_alpha_property; uint32_t hw_context_size; struct list_head context_list; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2f564ce..039270e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3522,7 +3522,11 @@ /* New style CUR*CNTR flags */ #define CURSOR_MODE 0x27 #define CURSOR_MODE_DISABLE 0x00 +#define CURSOR_MODE_128_32B_AX 0x02 +#define CURSOR_MODE_256_32B_AX 0x03 #define CURSOR_MODE_64_32B_AX 0x07 +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) #define MCURSOR_PIPE_SELECT (1 << 28) #define MCURSOR_PIPE_A 0x00 @@ -3569,7 +3573,9 @@ #define DISPPLANE_RGBX101010 (0x8<<26) #define DISPPLANE_RGBA101010 (0x9<<26) #define DISPPLANE_BGRX101010 (0xa<<26) +#define DISPPLANE_BGRA101010 (0xb<<26) #define DISPPLANE_RGBX161616 (0xc<<26) +#define DISPPLANE_RGBA161616 (0xd<<26) #define DISPPLANE_RGBX888 (0xe<<26) #define DISPPLANE_RGBA888 (0xf<<26) #define DISPPLANE_STEREO_ENABLE (1<<25) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f19e6ea..a854bcb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2047,6 +2047,86 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y, } } +u32 control_plane_alpha(u32 pixformat, unsigned int alpha) +{ + switch (pixformat) { + case DISPPLANE_RGBX888: + case DISPPLANE_RGBA888: + if (alpha) + pixformat = DISPPLANE_RGBA888; + else + pixformat = DISPPLANE_RGBX888; + break; + case DISPPLANE_BGRX888: + case DISPPLANE_BGRA888: + if (alpha) + pixformat = DISPPLANE_BGRA888; + else + pixformat = DISPPLANE_BGRX888; + break; + case DISPPLANE_RGBX101010: + case DISPPLANE_RGBA101010: + if (alpha) + pixformat = DISPPLANE_RGBA101010; + else + pixformat = DISPPLANE_RGBX101010; + break; + case DISPPLANE_BGRX101010: + case DISPPLANE_BGRA101010: + if (alpha) + pixformat = DISPPLANE_BGRA101010; + else + pixformat = DISPPLANE_BGRX101010; + break; + case DISPPLANE_RGBX161616: + case DISPPLANE_RGBA161616: + if (alpha) + pixformat = DISPPLANE_RGBA161616; + else + pixformat = DISPPLANE_RGBX161616; + break; + default: + if (alpha) + DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat); + break; + } + return pixformat; +} + +u32 control_cursor_alpha(u32 pixformat, unsigned int alpha) +{ + switch (pixformat) { + case CURSOR_MODE_128_32B_AX: + case CURSOR_MODE_128_ARGB_AX: + if (alpha) + pixformat = CURSOR_MODE_128_ARGB_AX; + else + pixformat = CURSOR_MODE_128_32B_AX; + break; + + case CURSOR_MODE_256_ARGB_AX: + case CURSOR_MODE_256_32B_AX: + if (alpha) + pixformat = CURSOR_MODE_256_ARGB_AX; + else + pixformat = CURSOR_MODE_256_32B_AX; + break; + + case CURSOR_MODE_64_ARGB_AX: + case CURSOR_MODE_64_32B_AX: + if (alpha) + pixformat = CURSOR_MODE_64_ARGB_AX; + else + pixformat = CURSOR_MODE_64_32B_AX; + break; + default: + if (alpha) + DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat); + break; + } + return pixformat; +} + static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb, int x, int y) { @@ -2107,6 +2187,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb, BUG(); } + dspcntr |= control_plane_alpha(dspcntr & DISPPLANE_PIXFORMAT_MASK, + intel_crtc->primary_alpha); + if (INTEL_INFO(dev)->gen >= 4) { if (obj->tiling_mode != I915_TILING_NONE) dspcntr |= DISPPLANE_TILED; @@ -7415,6 +7498,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) if (base) { cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + cntl |= control_cursor_alpha(cntl & CURSOR_MODE, intel_crtc->cursor_alpha); cntl |= pipe << 28; /* Connect to correct pipe */ } else { cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); @@ -8755,6 +8839,59 @@ free_work: return ret; } +static int intel_set_primary_plane_alpha(struct intel_crtc *crtc, + unsigned int alpha) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned int old_alpha; + int ret = 0; + + old_alpha = crtc->primary_alpha; + crtc->primary_alpha = alpha; + + if (!crtc->active) + return 0; + + intel_crtc_wait_for_pending_flips(&crtc->base); + + ret = dev_priv->display.update_plane(&crtc->base, crtc->base.fb, 0, 0); + if (ret) + crtc->primary_alpha = old_alpha; + + return ret; +} + +static void intel_set_cursor_plane_alpha(struct intel_crtc *crtc, + unsigned int alpha) +{ + crtc->cursor_alpha = alpha; + + if (crtc->active) + intel_crtc_update_cursor(&crtc->base, true); +} + +static int intel_crtc_set_property(struct drm_crtc *crtc, + struct drm_property *prop, + uint64_t val) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + uint64_t old_val; + int ret = -ENOENT; + + if (prop == dev_priv->plane_alpha_property) { + ret = intel_set_primary_plane_alpha(intel_crtc, + intel_crtc->primary_alpha); + } else if (prop == dev_priv->cursor_alpha_property) { + intel_set_cursor_plane_alpha(intel_crtc, val); + return 0; + } + + return ret; +} + static struct drm_crtc_helper_funcs intel_helper_funcs = { .mode_set_base_atomic = intel_pipe_set_base_atomic, .load_lut = intel_crtc_load_lut, @@ -10167,6 +10304,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = { .set_config = intel_crtc_set_config, .destroy = intel_crtc_destroy, .page_flip = intel_crtc_page_flip, + .set_property = intel_crtc_set_property }; static void intel_cpu_pll_init(struct drm_device *dev) @@ -10305,6 +10443,24 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; + if (!dev_priv->plane_alpha_property) + dev_priv->plane_alpha_property = + drm_property_create_range(dev, 0, "plane-alpha", 0, 1); + + if (dev_priv->plane_alpha_property) + drm_object_attach_property(&intel_crtc->base.base, + dev_priv->plane_alpha_property, + intel_crtc->primary_alpha); + + if (!dev_priv->cursor_alpha_property) + dev_priv->cursor_alpha_property = + drm_property_create_range(dev, 0, "cursor-alpha", 0, 1); + + if (dev_priv->cursor_alpha_property) + drm_object_attach_property(&intel_crtc->base.base, + dev_priv->cursor_alpha_property, + intel_crtc->cursor_alpha); + drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a4ffc02..0080d3a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -339,6 +339,9 @@ struct intel_crtc { struct drm_crtc base; enum pipe pipe; enum plane plane; + unsigned int primary_alpha; /* primary plane alpha control */ + unsigned int cursor_alpha; /* cursor plane alpha control */ + u8 lut_r[256], lut_g[256], lut_b[256]; /* * Whether the crtc and the connected output pipeline is active. Implies @@ -405,6 +408,7 @@ struct intel_plane { unsigned int crtc_w, crtc_h; uint32_t src_x, src_y; uint32_t src_w, src_h; + unsigned int plane_alpha; /* Since we need to change the watermarks before/after * enabling/disabling the planes, we need to store the parameters here @@ -676,6 +680,8 @@ int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, enum pipe pipe); void intel_wait_for_vblank(struct drm_device *dev, int pipe); +u32 control_plane_alpha(u32 pixformat, unsigned int alpha); +u32 control_cursor_alpha(u32 pixformat, unsigned int alpha); void intel_wait_for_pipe_off(struct drm_device *dev, int pipe); int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp); void vlv_wait_port_ready(struct drm_i915_private *dev_priv, @@ -906,7 +912,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob); int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane); void intel_flush_primary_plane(struct drm_i915_private *dev_priv, enum plane plane); -void intel_plane_restore(struct drm_plane *plane); +int intel_plane_restore(struct drm_plane *plane); void intel_plane_disable(struct drm_plane *plane); int intel_sprite_set_colorkey(struct drm_device *dev, void *data, struct drm_file *file_priv); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 336ae6c..9f6c91a 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -104,6 +104,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, break; } + sprctl |= control_plane_alpha(sprctl & SP_PIXFORMAT_MASK, + intel_plane->plane_alpha); + /* * Enable gamma to match primary/cursor plane behaviour. * FIXME should be user controllable via propertiesa. @@ -262,6 +265,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, BUG(); } + sprctl |= control_plane_alpha(sprctl & SPRITE_PIXFORMAT_MASK, + intel_plane->plane_alpha); + /* * Enable gamma to match primary/cursor plane behaviour. * FIXME should be user controllable via propertiesa. @@ -446,6 +452,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, BUG(); } + dvscntr |= control_plane_alpha(dvscntr & DVS_PIXFORMAT_MASK, + intel_plane->plane_alpha); + /* * Enable gamma to match primary/cursor plane behaviour. * FIXME should be user controllable via propertiesa. @@ -1011,18 +1020,38 @@ out_unlock: return ret; } -void intel_plane_restore(struct drm_plane *plane) +static int intel_plane_set_property(struct drm_plane *plane, + struct drm_property *prop, + uint64_t val) +{ + struct drm_i915_private *dev_priv = plane->dev->dev_private; + struct intel_plane *intel_plane = to_intel_plane(plane); + uint64_t old_val; + int ret = -ENOENT; + + if (prop == dev_priv->plane_alpha_property) { + old_val = intel_plane->plane_alpha; + intel_plane->plane_alpha = val; + ret = intel_plane_restore(plane); + if (ret) + intel_plane->plane_alpha = old_val; + } + + return ret; +} + +int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); if (!plane->crtc || !plane->fb) - return; + return 0; - intel_update_plane(plane, plane->crtc, plane->fb, - intel_plane->crtc_x, intel_plane->crtc_y, - intel_plane->crtc_w, intel_plane->crtc_h, - intel_plane->src_x, intel_plane->src_y, - intel_plane->src_w, intel_plane->src_h); + return intel_update_plane(plane, plane->crtc, plane->fb, + intel_plane->crtc_x, intel_plane->crtc_y, + intel_plane->crtc_w, intel_plane->crtc_h, + intel_plane->src_x, intel_plane->src_y, + intel_plane->src_w, intel_plane->src_h); } void intel_plane_disable(struct drm_plane *plane) @@ -1037,6 +1066,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane, .destroy = intel_destroy_plane, + .set_property = intel_plane_set_property, }; static uint32_t ilk_plane_formats[] = { @@ -1073,6 +1103,7 @@ static uint32_t vlv_plane_formats[] = { int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) { + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane; unsigned long possible_crtcs; const uint32_t *plane_formats; @@ -1146,8 +1177,20 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) &intel_plane_funcs, plane_formats, num_plane_formats, false); - if (ret) + if (ret) { kfree(intel_plane); + goto out; + } + + if (!dev_priv->plane_alpha_property) + dev_priv->plane_alpha_property = + drm_property_create_range(dev, 0, "plane-alpha", 0, 1); + + if (dev_priv->plane_alpha_property) + drm_object_attach_property(&intel_plane->base.base, + dev_priv->plane_alpha_property, + intel_plane->plane_alpha); +out: return ret; }