| Message ID | 1394266879-20522-4-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 08, 2014 at 01:51:18PM +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > This patch enables property for changin the pixel format > of plane to enable/disable pre-multiplied alpha format. > Client has to set BIT(DRM_BLEND_PREMULTIPLIED_ALPHA) | 0x0/0x1 > to disable/enable pre-multiplied alpha format. > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Signed-off-by: Srinivas, Vidya <vidya.srinivas@intel.com> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> Huum, the alpha being premultiplied or not seems to be a property of the framebuffer to me, not of the plane. It seems to me that we should define alternative premultiplied DRM_FORMATs and make the sprite planes advertise support for premultiplied fbs in the format list when the hardware indeed supports them.
Hi Damien, On Wed, 2014-03-19 at 15:10 +0000, Damien Lespiau wrote: > On Sat, Mar 08, 2014 at 01:51:18PM +0530, sagar.a.kamble@intel.com wrote: > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > This patch enables property for changin the pixel format > > of plane to enable/disable pre-multiplied alpha format. > > Client has to set BIT(DRM_BLEND_PREMULTIPLIED_ALPHA) | 0x0/0x1 > > to disable/enable pre-multiplied alpha format. > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Signed-off-by: Srinivas, Vidya <vidya.srinivas@intel.com> > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > > Huum, the alpha being premultiplied or not seems to be a property of the > framebuffer to me, not of the plane. It seems to me that we should > define alternative premultiplied DRM_FORMATs and make the sprite planes > advertise support for premultiplied fbs in the format list when the > hardware indeed supports them. This is what i think of usage of this property: Composer/user mode starts using plane with XRGB format and then it wants to add transparency to the plane. So it will set the format to ARGB format and provide buffer for that plane that will have pixels with pre-multiplied alpha (a*r, a*g, a*b, a). This can be done with primary plane(CRTC) as well, however I have not added this as CRTC property since CRTCs are going to be drm_plane soon. Will this kind of interface for usermode to toggle the pixel format's alpha be useful? Thanks, Sagar
On Thu, Mar 20, 2014 at 03:29:42PM +0530, Sagar Arun Kamble wrote: > Hi Damien, > > On Wed, 2014-03-19 at 15:10 +0000, Damien Lespiau wrote: > > On Sat, Mar 08, 2014 at 01:51:18PM +0530, sagar.a.kamble@intel.com wrote: > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > This patch enables property for changin the pixel format > > > of plane to enable/disable pre-multiplied alpha format. > > > Client has to set BIT(DRM_BLEND_PREMULTIPLIED_ALPHA) | 0x0/0x1 > > > to disable/enable pre-multiplied alpha format. > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: David Airlie <airlied@linux.ie> > > > Signed-off-by: Srinivas, Vidya <vidya.srinivas@intel.com> > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > > > > Huum, the alpha being premultiplied or not seems to be a property of the > > framebuffer to me, not of the plane. It seems to me that we should > > define alternative premultiplied DRM_FORMATs and make the sprite planes > > advertise support for premultiplied fbs in the format list when the > > hardware indeed supports them. > This is what i think of usage of this property: > > Composer/user mode starts using plane with XRGB format and then it wants > to add transparency to the plane. So it will set the format to ARGB > format and provide buffer for that plane that will have pixels with > pre-multiplied alpha (a*r, a*g, a*b, a). > This can be done with primary plane(CRTC) as well, however I have > not added this as CRTC property since CRTCs are going to be drm_plane > soon. > > Will this kind of interface for usermode to toggle the pixel format's > alpha be useful? I don't think so, nop. Besides being a convoluted apocalyptic scenario, one cannot simply change the format of the FB without re-adding it with AddFB2(). There's a usage model for the compositor to add a plane-global alpha to a plane (fades the client provided render target) and that's indeed a plane property. As far I as can tell, the premultiplied alpha format ban be sued support scanning out OpenGL blended fbs.
On Thu, Mar 20, 2014 at 11:38:18AM +0000, Damien Lespiau wrote: > On Thu, Mar 20, 2014 at 03:29:42PM +0530, Sagar Arun Kamble wrote: > > Hi Damien, > > > > On Wed, 2014-03-19 at 15:10 +0000, Damien Lespiau wrote: > > > On Sat, Mar 08, 2014 at 01:51:18PM +0530, sagar.a.kamble@intel.com wrote: > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > > > This patch enables property for changin the pixel format > > > > of plane to enable/disable pre-multiplied alpha format. > > > > Client has to set BIT(DRM_BLEND_PREMULTIPLIED_ALPHA) | 0x0/0x1 > > > > to disable/enable pre-multiplied alpha format. > > > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: David Airlie <airlied@linux.ie> > > > > Signed-off-by: Srinivas, Vidya <vidya.srinivas@intel.com> > > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > Huum, the alpha being premultiplied or not seems to be a property of the > > > framebuffer to me, not of the plane. It seems to me that we should > > > define alternative premultiplied DRM_FORMATs and make the sprite planes > > > advertise support for premultiplied fbs in the format list when the > > > hardware indeed supports them. > > This is what i think of usage of this property: > > > > Composer/user mode starts using plane with XRGB format and then it wants > > to add transparency to the plane. So it will set the format to ARGB > > format and provide buffer for that plane that will have pixels with > > pre-multiplied alpha (a*r, a*g, a*b, a). > > This can be done with primary plane(CRTC) as well, however I have > > not added this as CRTC property since CRTCs are going to be drm_plane > > soon. > > > > Will this kind of interface for usermode to toggle the pixel format's > > alpha be useful? > > I don't think so, nop. > > Besides being a convoluted apocalyptic scenario, one cannot simply > change the format of the FB without re-adding it with AddFB2(). > > There's a usage model for the compositor to add a plane-global alpha to > a plane (fades the client provided render target) and that's indeed a > plane property. > > As far I as can tell, the premultiplied alpha format ban be sued support > scanning out OpenGL blended fbs. I'm not sure I follow this discussion completely, but in my opinion may _never_ change the pixel format of a drm framebuffer object. Think of a drm framebuffer as a view of the underlying object(s) with strides, pixel format, dimensions and other stuff specified. If you need a different view, simply create a new drm framebuffer object. Note that drm framebuffer objects are never shared (as opposed to the underlying gem backing storage which can be shared with flink or dma-buf), so this doesn't need any synchronization outside of the compositor itself. I don't really have a decent opinion on the pre-multiplied vs non-premultiplied ARGB formats issue at hand. In case of doubt I think we should follow what gl does. But I have no clue how that's handled in gl ;-) And maybe I'm completely missing the point here ;-) -Daniel
On Thu, Mar 20, 2014 at 02:51:20PM +0100, Daniel Vetter wrote: > I don't really have a decent opinion on the pre-multiplied vs > non-premultiplied ARGB formats issue at hand. In case of doubt I think we > should follow what gl does. But I have no clue how that's handled in gl > ;-) I'd still like a premultipled DRM format, but let's wait for Ville opinion. In GL, the blending equation doesn't do anything automatically. So if you have a premultiplied source, you need to setup the blending function/factor accordingly.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e33124c..44c366d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -685,6 +685,7 @@ 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_premultiplied_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, diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 4c3d2a2..183bd80 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -120,6 +120,10 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, I915_WRITE(sprconstalpha, (blend_factor & SPRITE_CONSTANT_ALPHA_MASK) | SPRITE_CONSTANT_ALPHA_ENABLE); break; + case DRM_BLEND_PREMULTIPLIED_ALPHA: + sprctl |= control_premultiplied_alpha(sprctl & SP_PIXFORMAT_MASK, + blend_factor); + break; default: DRM_DEBUG_DRIVER("%x Blending operation not supported", intel_plane->blend_param.details.type); break; @@ -249,6 +253,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct intel_plane *intel_plane = to_intel_plane(plane); int pipe = intel_plane->pipe; u32 sprctl, sprscale = 0; + unsigned int blend_type, blend_factor; unsigned long sprsurf_offset, linear_offset; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); @@ -283,6 +288,22 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, BUG(); } + /* 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 DRM_BLEND_NONE: + break; + case DRM_BLEND_PREMULTIPLIED_ALPHA: + sprctl |= control_premultiplied_alpha(sprctl & SPRITE_PIXFORMAT_MASK, + blend_factor); + 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. @@ -434,6 +455,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, int pipe = intel_plane->pipe; unsigned long dvssurf_offset, linear_offset; u32 dvscntr, dvsscale; + unsigned int blend_type, blend_factor; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); dvscntr = I915_READ(DVSCNTR(pipe)); @@ -467,6 +489,22 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, BUG(); } + /* 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 DRM_BLEND_NONE: + break; + case DRM_BLEND_PREMULTIPLIED_ALPHA: + dvscntr |= control_premultiplied_alpha(dvscntr & DVS_PIXFORMAT_MASK, + blend_factor); + 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. @@ -1112,6 +1150,38 @@ static uint32_t vlv_plane_formats[] = { DRM_FORMAT_VYUY, }; +u32 control_premultiplied_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; + default: + if (alpha) + DRM_DEBUG_DRIVER("Pixel format 0x%08x does not support Alpha Control\n", pixformat); + break; + } + return pixformat; +} + int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) { @@ -1201,7 +1271,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) if (!dev_priv->blend_property) dev_priv->blend_property = drm_mode_create_blend_property(dev, BIT(DRM_BLEND_NONE) | - BIT(DRM_BLEND_CONSTANT_ALPHA)); + BIT(DRM_BLEND_CONSTANT_ALPHA) | + BIT(DRM_BLEND_PREMULTIPLIED_ALPHA)); if (dev_priv->blend_property) drm_object_attach_property(&intel_plane->base.base,