| Message ID | 1527856871-3007-2-git-send-email-lowry.li@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: > Pixel blend modes represent the alpha blending equation > selection, describing how the pixels from the current > plane are composited with the background. > > Add a pixel_blend_mode to drm_plane_state and a > blend_mode_property to drm_plane, and related support > functions. > > Defines three blend modes in drm_blend.h. > > Signed-off-by: Lowry Li <lowry.li@arm.com> > --- > drivers/gpu/drm/drm_atomic.c | 4 ++ > drivers/gpu/drm/drm_atomic_helper.c | 1 + > drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ > include/drm/drm_blend.h | 6 ++ > include/drm/drm_plane.h | 5 ++ > 5 files changed, 142 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 07fef42..1d18389 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > state->src_h = val; > } else if (property == plane->alpha_property) { > state->alpha = val; > + } else if (property == plane->blend_mode_property) { > + state->pixel_blend_mode = val; > } else if (property == plane->rotation_property) { > if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) > return -EINVAL; > @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > *val = state->src_h; > } else if (property == plane->alpha_property) { > *val = state->alpha; > + } else if (property == plane->blend_mode_property) { > + *val = state->pixel_blend_mode; > } else if (property == plane->rotation_property) { > *val = state->rotation; > } else if (property == plane->zpos_property) { > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 130da51..7f5d463 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) > /* Reset the alpha value to fully opaque if it matters */ > if (plane->alpha_property) > plane->state->alpha = plane->alpha_property->values[1]; > + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > } > } > EXPORT_SYMBOL(drm_atomic_helper_plane_reset); > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > index a16a74d..ac6f19c 100644 > --- a/drivers/gpu/drm/drm_blend.c > +++ b/drivers/gpu/drm/drm_blend.c > @@ -107,6 +107,52 @@ > * planes. Without this property the primary plane is always below the cursor > * plane, and ordering between all other planes is undefined. > * > + * pixel blend mode: > + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > + * It adds a blend mode for alpha blending equation selection, describing > + * how the pixels from the current plane are composited with the > + * background. > + * > + * Three alpha blending equations are defined: > + * > + * "None": > + * Blend formula that ignores the pixel alpha:: > + * > + * out.rgb = plane_alpha * fg.rgb + > + * (1 - plane_alpha) * bg.rgb > + * > + * "Pre-multiplied": > + * Blend formula that assumes the pixel color values > + * have been already pre-multiplied with the alpha > + * channel values:: > + * > + * out.rgb = plane_alpha * fg.rgb + > + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > + * > + * "Coverage": > + * Blend formula that assumes the pixel color values have not > + * been pre-multiplied and will do so when blending them to the > + * background color values:: > + * > + * out.rgb = plane_alpha * fg.alpha * fg.rgb + > + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > + * > + * Using the following symbols: > + * > + * ``fg.rgb``: > + * Each of the RGB component values from the plane's pixel > + * ``fg.alpha``: > + * Alpha component value from the plane's pixel. If the plane's > + * pixel format has no alpha component, then this is assumed to be > + * 1.0. In these cases, this property has no effect, as all three > + * equations become equivalent. > + * ``bg.rgb``: > + * Each of the RGB component values from the background > + * ``plane_alpha``: > + * Plane alpha value set by the plane "alpha" property. If the > + * plane does not expose the "alpha" property, then this is > + * assumed to be 1.0 > + * > * Note that all the property extensions described here apply either to the > * plane or the CRTC (e.g. for the background color, which currently is not > * exposed and assumed to be black). > @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > return 0; > } > EXPORT_SYMBOL(drm_atomic_normalize_zpos); > + > +/** > + * drm_plane_create_blend_mode_property - create a new blend mode property > + * @plane: drm plane > + * @supported_modes: bitmask of supported modes, must include > + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is > + * that alpha is premultiplied, and old userspace can break if > + * the property defaults to coverage. > + * Thanks for the explanation Lowry. Sigh, old userspace :-\ One pedantic suggestion s/defaults to coverage/defaults to anything else/ Since defaulting to "none" or any future blend mode is also a no-go. I wouldn't bother resending solely for ^^. Perhaps squash locally before merging? With that, the patch is Reviewed-by: Emil Velikov <emil.velikov@collabora.com> -Emil
On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: > On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: > > Pixel blend modes represent the alpha blending equation > > selection, describing how the pixels from the current > > plane are composited with the background. > > > > Add a pixel_blend_mode to drm_plane_state and a > > blend_mode_property to drm_plane, and related support > > functions. > > > > Defines three blend modes in drm_blend.h. > > > > Signed-off-by: Lowry Li <lowry.li@arm.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 4 ++ > > drivers/gpu/drm/drm_atomic_helper.c | 1 + > > drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ > > include/drm/drm_blend.h | 6 ++ > > include/drm/drm_plane.h | 5 ++ > > 5 files changed, 142 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 07fef42..1d18389 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > state->src_h = val; > > } else if (property == plane->alpha_property) { > > state->alpha = val; > > + } else if (property == plane->blend_mode_property) { > > + state->pixel_blend_mode = val; > > } else if (property == plane->rotation_property) { > > if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) > > return -EINVAL; > > @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > *val = state->src_h; > > } else if (property == plane->alpha_property) { > > *val = state->alpha; > > + } else if (property == plane->blend_mode_property) { > > + *val = state->pixel_blend_mode; > > } else if (property == plane->rotation_property) { > > *val = state->rotation; > > } else if (property == plane->zpos_property) { > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 130da51..7f5d463 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) > > /* Reset the alpha value to fully opaque if it matters */ > > if (plane->alpha_property) > > plane->state->alpha = plane->alpha_property->values[1]; > > + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > > } > > } > > EXPORT_SYMBOL(drm_atomic_helper_plane_reset); > > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > > index a16a74d..ac6f19c 100644 > > --- a/drivers/gpu/drm/drm_blend.c > > +++ b/drivers/gpu/drm/drm_blend.c > > @@ -107,6 +107,52 @@ > > * planes. Without this property the primary plane is always below the cursor > > * plane, and ordering between all other planes is undefined. > > * > > + * pixel blend mode: > > + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > > + * It adds a blend mode for alpha blending equation selection, describing > > + * how the pixels from the current plane are composited with the > > + * background. > > + * > > + * Three alpha blending equations are defined: > > + * > > + * "None": > > + * Blend formula that ignores the pixel alpha:: > > + * > > + * out.rgb = plane_alpha * fg.rgb + > > + * (1 - plane_alpha) * bg.rgb > > + * > > + * "Pre-multiplied": > > + * Blend formula that assumes the pixel color values > > + * have been already pre-multiplied with the alpha > > + * channel values:: > > + * > > + * out.rgb = plane_alpha * fg.rgb + > > + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > > + * > > + * "Coverage": > > + * Blend formula that assumes the pixel color values have not > > + * been pre-multiplied and will do so when blending them to the > > + * background color values:: > > + * > > + * out.rgb = plane_alpha * fg.alpha * fg.rgb + > > + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > > + * > > + * Using the following symbols: > > + * > > + * ``fg.rgb``: > > + * Each of the RGB component values from the plane's pixel > > + * ``fg.alpha``: > > + * Alpha component value from the plane's pixel. If the plane's > > + * pixel format has no alpha component, then this is assumed to be > > + * 1.0. In these cases, this property has no effect, as all three > > + * equations become equivalent. > > + * ``bg.rgb``: > > + * Each of the RGB component values from the background > > + * ``plane_alpha``: > > + * Plane alpha value set by the plane "alpha" property. If the > > + * plane does not expose the "alpha" property, then this is > > + * assumed to be 1.0 > > + * > > * Note that all the property extensions described here apply either to the > > * plane or the CRTC (e.g. for the background color, which currently is not > > * exposed and assumed to be black). > > @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > > return 0; > > } > > EXPORT_SYMBOL(drm_atomic_normalize_zpos); > > + > > +/** > > + * drm_plane_create_blend_mode_property - create a new blend mode property > > + * @plane: drm plane > > + * @supported_modes: bitmask of supported modes, must include > > + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is > > + * that alpha is premultiplied, and old userspace can break if > > + * the property defaults to coverage. > > + * > > Thanks for the explanation Lowry. > Sigh, old userspace :-\ > > One pedantic suggestion s/defaults to coverage/defaults to anything else/ > Since defaulting to "none" or any future blend mode is also a no-go. > > I wouldn't bother resending solely for ^^. Perhaps squash locally > before merging? > > With that, the patch is > Reviewed-by: Emil Velikov <emil.velikov@collabora.com> > > -Emil Hi Emil, Thanks a lot for your review and will change that.
Op 05-06-18 om 11:07 schreef Lowry Li: > On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: >> On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: >>> Pixel blend modes represent the alpha blending equation >>> selection, describing how the pixels from the current >>> plane are composited with the background. >>> >>> Add a pixel_blend_mode to drm_plane_state and a >>> blend_mode_property to drm_plane, and related support >>> functions. >>> >>> Defines three blend modes in drm_blend.h. >>> >>> Signed-off-by: Lowry Li <lowry.li@arm.com> >>> --- >>> drivers/gpu/drm/drm_atomic.c | 4 ++ >>> drivers/gpu/drm/drm_atomic_helper.c | 1 + >>> drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ >>> include/drm/drm_blend.h | 6 ++ >>> include/drm/drm_plane.h | 5 ++ >>> 5 files changed, 142 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>> index 07fef42..1d18389 100644 >>> --- a/drivers/gpu/drm/drm_atomic.c >>> +++ b/drivers/gpu/drm/drm_atomic.c >>> @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>> state->src_h = val; >>> } else if (property == plane->alpha_property) { >>> state->alpha = val; >>> + } else if (property == plane->blend_mode_property) { >>> + state->pixel_blend_mode = val; >>> } else if (property == plane->rotation_property) { >>> if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) >>> return -EINVAL; >>> @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>> *val = state->src_h; >>> } else if (property == plane->alpha_property) { >>> *val = state->alpha; >>> + } else if (property == plane->blend_mode_property) { >>> + *val = state->pixel_blend_mode; >>> } else if (property == plane->rotation_property) { >>> *val = state->rotation; >>> } else if (property == plane->zpos_property) { >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>> index 130da51..7f5d463 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) >>> /* Reset the alpha value to fully opaque if it matters */ >>> if (plane->alpha_property) >>> plane->state->alpha = plane->alpha_property->values[1]; >>> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; >>> } >>> } >>> EXPORT_SYMBOL(drm_atomic_helper_plane_reset); >>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c >>> index a16a74d..ac6f19c 100644 >>> --- a/drivers/gpu/drm/drm_blend.c >>> +++ b/drivers/gpu/drm/drm_blend.c >>> @@ -107,6 +107,52 @@ >>> * planes. Without this property the primary plane is always below the cursor >>> * plane, and ordering between all other planes is undefined. >>> * >>> + * pixel blend mode: >>> + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). >>> + * It adds a blend mode for alpha blending equation selection, describing >>> + * how the pixels from the current plane are composited with the >>> + * background. >>> + * >>> + * Three alpha blending equations are defined: >>> + * >>> + * "None": >>> + * Blend formula that ignores the pixel alpha:: >>> + * >>> + * out.rgb = plane_alpha * fg.rgb + >>> + * (1 - plane_alpha) * bg.rgb >>> + * >>> + * "Pre-multiplied": >>> + * Blend formula that assumes the pixel color values >>> + * have been already pre-multiplied with the alpha >>> + * channel values:: >>> + * >>> + * out.rgb = plane_alpha * fg.rgb + >>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb >>> + * >>> + * "Coverage": >>> + * Blend formula that assumes the pixel color values have not >>> + * been pre-multiplied and will do so when blending them to the >>> + * background color values:: >>> + * >>> + * out.rgb = plane_alpha * fg.alpha * fg.rgb + >>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb >>> + * >>> + * Using the following symbols: >>> + * >>> + * ``fg.rgb``: >>> + * Each of the RGB component values from the plane's pixel >>> + * ``fg.alpha``: >>> + * Alpha component value from the plane's pixel. If the plane's >>> + * pixel format has no alpha component, then this is assumed to be >>> + * 1.0. In these cases, this property has no effect, as all three >>> + * equations become equivalent. >>> + * ``bg.rgb``: >>> + * Each of the RGB component values from the background >>> + * ``plane_alpha``: >>> + * Plane alpha value set by the plane "alpha" property. If the >>> + * plane does not expose the "alpha" property, then this is >>> + * assumed to be 1.0 >>> + * >>> * Note that all the property extensions described here apply either to the >>> * plane or the CRTC (e.g. for the background color, which currently is not >>> * exposed and assumed to be black). >>> @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, >>> return 0; >>> } >>> EXPORT_SYMBOL(drm_atomic_normalize_zpos); >>> + >>> +/** >>> + * drm_plane_create_blend_mode_property - create a new blend mode property >>> + * @plane: drm plane >>> + * @supported_modes: bitmask of supported modes, must include >>> + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is >>> + * that alpha is premultiplied, and old userspace can break if >>> + * the property defaults to coverage. >>> + * >> Thanks for the explanation Lowry. >> Sigh, old userspace :-\ >> >> One pedantic suggestion s/defaults to coverage/defaults to anything else/ >> Since defaulting to "none" or any future blend mode is also a no-go. >> >> I wouldn't bother resending solely for ^^. Perhaps squash locally >> before merging? >> >> With that, the patch is >> Reviewed-by: Emil Velikov <emil.velikov@collabora.com> >> >> -Emil > Hi Emil, > > Thanks a lot for your review and will change that. Ping? Any updates? ~Maarten
On Mon, Aug 13, 2018 at 12:49:13PM +0200, Maarten Lankhorst wrote: > Op 05-06-18 om 11:07 schreef Lowry Li: > > On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: > >> On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: > >>> Pixel blend modes represent the alpha blending equation > >>> selection, describing how the pixels from the current > >>> plane are composited with the background. > >>> > >>> Add a pixel_blend_mode to drm_plane_state and a > >>> blend_mode_property to drm_plane, and related support > >>> functions. > >>> > >>> Defines three blend modes in drm_blend.h. > >>> > >>> Signed-off-by: Lowry Li <lowry.li@arm.com> > >>> --- > >>> drivers/gpu/drm/drm_atomic.c | 4 ++ > >>> drivers/gpu/drm/drm_atomic_helper.c | 1 + > >>> drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ > >>> include/drm/drm_blend.h | 6 ++ > >>> include/drm/drm_plane.h | 5 ++ > >>> 5 files changed, 142 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>> index 07fef42..1d18389 100644 > >>> --- a/drivers/gpu/drm/drm_atomic.c > >>> +++ b/drivers/gpu/drm/drm_atomic.c > >>> @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > >>> state->src_h = val; > >>> } else if (property == plane->alpha_property) { > >>> state->alpha = val; > >>> + } else if (property == plane->blend_mode_property) { > >>> + state->pixel_blend_mode = val; > >>> } else if (property == plane->rotation_property) { > >>> if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) > >>> return -EINVAL; > >>> @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > >>> *val = state->src_h; > >>> } else if (property == plane->alpha_property) { > >>> *val = state->alpha; > >>> + } else if (property == plane->blend_mode_property) { > >>> + *val = state->pixel_blend_mode; > >>> } else if (property == plane->rotation_property) { > >>> *val = state->rotation; > >>> } else if (property == plane->zpos_property) { > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>> index 130da51..7f5d463 100644 > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>> @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) > >>> /* Reset the alpha value to fully opaque if it matters */ > >>> if (plane->alpha_property) > >>> plane->state->alpha = plane->alpha_property->values[1]; > >>> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > >>> } > >>> } > >>> EXPORT_SYMBOL(drm_atomic_helper_plane_reset); > >>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > >>> index a16a74d..ac6f19c 100644 > >>> --- a/drivers/gpu/drm/drm_blend.c > >>> +++ b/drivers/gpu/drm/drm_blend.c > >>> @@ -107,6 +107,52 @@ > >>> * planes. Without this property the primary plane is always below the cursor > >>> * plane, and ordering between all other planes is undefined. > >>> * > >>> + * pixel blend mode: > >>> + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > >>> + * It adds a blend mode for alpha blending equation selection, describing > >>> + * how the pixels from the current plane are composited with the > >>> + * background. > >>> + * > >>> + * Three alpha blending equations are defined: > >>> + * > >>> + * "None": > >>> + * Blend formula that ignores the pixel alpha:: > >>> + * > >>> + * out.rgb = plane_alpha * fg.rgb + > >>> + * (1 - plane_alpha) * bg.rgb > >>> + * > >>> + * "Pre-multiplied": > >>> + * Blend formula that assumes the pixel color values > >>> + * have been already pre-multiplied with the alpha > >>> + * channel values:: > >>> + * > >>> + * out.rgb = plane_alpha * fg.rgb + > >>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > >>> + * > >>> + * "Coverage": > >>> + * Blend formula that assumes the pixel color values have not > >>> + * been pre-multiplied and will do so when blending them to the > >>> + * background color values:: > >>> + * > >>> + * out.rgb = plane_alpha * fg.alpha * fg.rgb + > >>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > >>> + * > >>> + * Using the following symbols: > >>> + * > >>> + * ``fg.rgb``: > >>> + * Each of the RGB component values from the plane's pixel > >>> + * ``fg.alpha``: > >>> + * Alpha component value from the plane's pixel. If the plane's > >>> + * pixel format has no alpha component, then this is assumed to be > >>> + * 1.0. In these cases, this property has no effect, as all three > >>> + * equations become equivalent. > >>> + * ``bg.rgb``: > >>> + * Each of the RGB component values from the background > >>> + * ``plane_alpha``: > >>> + * Plane alpha value set by the plane "alpha" property. If the > >>> + * plane does not expose the "alpha" property, then this is > >>> + * assumed to be 1.0 > >>> + * > >>> * Note that all the property extensions described here apply either to the > >>> * plane or the CRTC (e.g. for the background color, which currently is not > >>> * exposed and assumed to be black). > >>> @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > >>> return 0; > >>> } > >>> EXPORT_SYMBOL(drm_atomic_normalize_zpos); > >>> + > >>> +/** > >>> + * drm_plane_create_blend_mode_property - create a new blend mode property > >>> + * @plane: drm plane > >>> + * @supported_modes: bitmask of supported modes, must include > >>> + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is > >>> + * that alpha is premultiplied, and old userspace can break if > >>> + * the property defaults to coverage. > >>> + * > >> Thanks for the explanation Lowry. > >> Sigh, old userspace :-\ > >> > >> One pedantic suggestion s/defaults to coverage/defaults to anything else/ > >> Since defaulting to "none" or any future blend mode is also a no-go. > >> > >> I wouldn't bother resending solely for ^^. Perhaps squash locally > >> before merging? > >> > >> With that, the patch is > >> Reviewed-by: Emil Velikov <emil.velikov@collabora.com> > >> > >> -Emil > > Hi Emil, > > > > Thanks a lot for your review and will change that. > > Ping? Any updates? > > ~Maarten Hi Maarten, So far the merge request on user space patch has been verified by John Stultz. The comments from Sean has been fixed and now we are waiting the reviewed-tag from Sean. Once it merged, we'll send a new kernel patch to review.
Op 14-08-18 om 05:11 schreef Lowry Li: > On Mon, Aug 13, 2018 at 12:49:13PM +0200, Maarten Lankhorst wrote: >> Op 05-06-18 om 11:07 schreef Lowry Li: >>> On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: >>>> On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: >>>>> Pixel blend modes represent the alpha blending equation >>>>> selection, describing how the pixels from the current >>>>> plane are composited with the background. >>>>> >>>>> Add a pixel_blend_mode to drm_plane_state and a >>>>> blend_mode_property to drm_plane, and related support >>>>> functions. >>>>> >>>>> Defines three blend modes in drm_blend.h. >>>>> >>>>> Signed-off-by: Lowry Li <lowry.li@arm.com> >>>>> --- >>>>> drivers/gpu/drm/drm_atomic.c | 4 ++ >>>>> drivers/gpu/drm/drm_atomic_helper.c | 1 + >>>>> drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ >>>>> include/drm/drm_blend.h | 6 ++ >>>>> include/drm/drm_plane.h | 5 ++ >>>>> 5 files changed, 142 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>>> index 07fef42..1d18389 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>> @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>>>> state->src_h = val; >>>>> } else if (property == plane->alpha_property) { >>>>> state->alpha = val; >>>>> + } else if (property == plane->blend_mode_property) { >>>>> + state->pixel_blend_mode = val; >>>>> } else if (property == plane->rotation_property) { >>>>> if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) >>>>> return -EINVAL; >>>>> @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>>>> *val = state->src_h; >>>>> } else if (property == plane->alpha_property) { >>>>> *val = state->alpha; >>>>> + } else if (property == plane->blend_mode_property) { >>>>> + *val = state->pixel_blend_mode; >>>>> } else if (property == plane->rotation_property) { >>>>> *val = state->rotation; >>>>> } else if (property == plane->zpos_property) { >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>>> index 130da51..7f5d463 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>> @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) >>>>> /* Reset the alpha value to fully opaque if it matters */ >>>>> if (plane->alpha_property) >>>>> plane->state->alpha = plane->alpha_property->values[1]; >>>>> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; >>>>> } >>>>> } >>>>> EXPORT_SYMBOL(drm_atomic_helper_plane_reset); >>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c >>>>> index a16a74d..ac6f19c 100644 >>>>> --- a/drivers/gpu/drm/drm_blend.c >>>>> +++ b/drivers/gpu/drm/drm_blend.c >>>>> @@ -107,6 +107,52 @@ >>>>> * planes. Without this property the primary plane is always below the cursor >>>>> * plane, and ordering between all other planes is undefined. >>>>> * >>>>> + * pixel blend mode: >>>>> + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). >>>>> + * It adds a blend mode for alpha blending equation selection, describing >>>>> + * how the pixels from the current plane are composited with the >>>>> + * background. >>>>> + * >>>>> + * Three alpha blending equations are defined: >>>>> + * >>>>> + * "None": >>>>> + * Blend formula that ignores the pixel alpha:: >>>>> + * >>>>> + * out.rgb = plane_alpha * fg.rgb + >>>>> + * (1 - plane_alpha) * bg.rgb >>>>> + * >>>>> + * "Pre-multiplied": >>>>> + * Blend formula that assumes the pixel color values >>>>> + * have been already pre-multiplied with the alpha >>>>> + * channel values:: >>>>> + * >>>>> + * out.rgb = plane_alpha * fg.rgb + >>>>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb >>>>> + * >>>>> + * "Coverage": >>>>> + * Blend formula that assumes the pixel color values have not >>>>> + * been pre-multiplied and will do so when blending them to the >>>>> + * background color values:: >>>>> + * >>>>> + * out.rgb = plane_alpha * fg.alpha * fg.rgb + >>>>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb >>>>> + * >>>>> + * Using the following symbols: >>>>> + * >>>>> + * ``fg.rgb``: >>>>> + * Each of the RGB component values from the plane's pixel >>>>> + * ``fg.alpha``: >>>>> + * Alpha component value from the plane's pixel. If the plane's >>>>> + * pixel format has no alpha component, then this is assumed to be >>>>> + * 1.0. In these cases, this property has no effect, as all three >>>>> + * equations become equivalent. >>>>> + * ``bg.rgb``: >>>>> + * Each of the RGB component values from the background >>>>> + * ``plane_alpha``: >>>>> + * Plane alpha value set by the plane "alpha" property. If the >>>>> + * plane does not expose the "alpha" property, then this is >>>>> + * assumed to be 1.0 >>>>> + * >>>>> * Note that all the property extensions described here apply either to the >>>>> * plane or the CRTC (e.g. for the background color, which currently is not >>>>> * exposed and assumed to be black). >>>>> @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, >>>>> return 0; >>>>> } >>>>> EXPORT_SYMBOL(drm_atomic_normalize_zpos); >>>>> + >>>>> +/** >>>>> + * drm_plane_create_blend_mode_property - create a new blend mode property >>>>> + * @plane: drm plane >>>>> + * @supported_modes: bitmask of supported modes, must include >>>>> + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is >>>>> + * that alpha is premultiplied, and old userspace can break if >>>>> + * the property defaults to coverage. >>>>> + * >>>> Thanks for the explanation Lowry. >>>> Sigh, old userspace :-\ >>>> >>>> One pedantic suggestion s/defaults to coverage/defaults to anything else/ >>>> Since defaulting to "none" or any future blend mode is also a no-go. >>>> >>>> I wouldn't bother resending solely for ^^. Perhaps squash locally >>>> before merging? >>>> >>>> With that, the patch is >>>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com> >>>> >>>> -Emil >>> Hi Emil, >>> >>> Thanks a lot for your review and will change that. >> Ping? Any updates? >> >> ~Maarten > Hi Maarten, > > So far the merge request on user space patch has been verified by John > Stultz. The comments from Sean has been fixed and now we are waiting the > reviewed-tag from Sean. Once it merged, we'll send a new kernel patch to > review. > Hey, In general the kernel patch should be merged first, because userspace cannot rely on the behavior until the next -rc1 that integrates it. However, in order to merge the kernel patch, a userspace proof of concept for validation is needed first. Since you're close to merging, I would say the userspace requirement is satisfied. It's safe to send the kernel patch. :) ~Maarten
On Tue, Aug 14, 2018 at 11:15:43AM +0200, Maarten Lankhorst wrote: > Op 14-08-18 om 05:11 schreef Lowry Li: > > On Mon, Aug 13, 2018 at 12:49:13PM +0200, Maarten Lankhorst wrote: > >> Op 05-06-18 om 11:07 schreef Lowry Li: > >>> On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: > >>>> On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: > >>>>> Pixel blend modes represent the alpha blending equation > >>>>> selection, describing how the pixels from the current > >>>>> plane are composited with the background. > >>>>> > >>>>> Add a pixel_blend_mode to drm_plane_state and a > >>>>> blend_mode_property to drm_plane, and related support > >>>>> functions. > >>>>> > >>>>> Defines three blend modes in drm_blend.h. > >>>>> > >>>>> Signed-off-by: Lowry Li <lowry.li@arm.com> > >>>>> --- > >>>>> drivers/gpu/drm/drm_atomic.c | 4 ++ > >>>>> drivers/gpu/drm/drm_atomic_helper.c | 1 + > >>>>> drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ > >>>>> include/drm/drm_blend.h | 6 ++ > >>>>> include/drm/drm_plane.h | 5 ++ > >>>>> 5 files changed, 142 insertions(+) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>>>> index 07fef42..1d18389 100644 > >>>>> --- a/drivers/gpu/drm/drm_atomic.c > >>>>> +++ b/drivers/gpu/drm/drm_atomic.c > >>>>> @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > >>>>> state->src_h = val; > >>>>> } else if (property == plane->alpha_property) { > >>>>> state->alpha = val; > >>>>> + } else if (property == plane->blend_mode_property) { > >>>>> + state->pixel_blend_mode = val; > >>>>> } else if (property == plane->rotation_property) { > >>>>> if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) > >>>>> return -EINVAL; > >>>>> @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > >>>>> *val = state->src_h; > >>>>> } else if (property == plane->alpha_property) { > >>>>> *val = state->alpha; > >>>>> + } else if (property == plane->blend_mode_property) { > >>>>> + *val = state->pixel_blend_mode; > >>>>> } else if (property == plane->rotation_property) { > >>>>> *val = state->rotation; > >>>>> } else if (property == plane->zpos_property) { > >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>>>> index 130da51..7f5d463 100644 > >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>>>> @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) > >>>>> /* Reset the alpha value to fully opaque if it matters */ > >>>>> if (plane->alpha_property) > >>>>> plane->state->alpha = plane->alpha_property->values[1]; > >>>>> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > >>>>> } > >>>>> } > >>>>> EXPORT_SYMBOL(drm_atomic_helper_plane_reset); > >>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > >>>>> index a16a74d..ac6f19c 100644 > >>>>> --- a/drivers/gpu/drm/drm_blend.c > >>>>> +++ b/drivers/gpu/drm/drm_blend.c > >>>>> @@ -107,6 +107,52 @@ > >>>>> * planes. Without this property the primary plane is always below the cursor > >>>>> * plane, and ordering between all other planes is undefined. > >>>>> * > >>>>> + * pixel blend mode: > >>>>> + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > >>>>> + * It adds a blend mode for alpha blending equation selection, describing > >>>>> + * how the pixels from the current plane are composited with the > >>>>> + * background. > >>>>> + * > >>>>> + * Three alpha blending equations are defined: > >>>>> + * > >>>>> + * "None": > >>>>> + * Blend formula that ignores the pixel alpha:: > >>>>> + * > >>>>> + * out.rgb = plane_alpha * fg.rgb + > >>>>> + * (1 - plane_alpha) * bg.rgb > >>>>> + * > >>>>> + * "Pre-multiplied": > >>>>> + * Blend formula that assumes the pixel color values > >>>>> + * have been already pre-multiplied with the alpha > >>>>> + * channel values:: > >>>>> + * > >>>>> + * out.rgb = plane_alpha * fg.rgb + > >>>>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > >>>>> + * > >>>>> + * "Coverage": > >>>>> + * Blend formula that assumes the pixel color values have not > >>>>> + * been pre-multiplied and will do so when blending them to the > >>>>> + * background color values:: > >>>>> + * > >>>>> + * out.rgb = plane_alpha * fg.alpha * fg.rgb + > >>>>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > >>>>> + * > >>>>> + * Using the following symbols: > >>>>> + * > >>>>> + * ``fg.rgb``: > >>>>> + * Each of the RGB component values from the plane's pixel > >>>>> + * ``fg.alpha``: > >>>>> + * Alpha component value from the plane's pixel. If the plane's > >>>>> + * pixel format has no alpha component, then this is assumed to be > >>>>> + * 1.0. In these cases, this property has no effect, as all three > >>>>> + * equations become equivalent. > >>>>> + * ``bg.rgb``: > >>>>> + * Each of the RGB component values from the background > >>>>> + * ``plane_alpha``: > >>>>> + * Plane alpha value set by the plane "alpha" property. If the > >>>>> + * plane does not expose the "alpha" property, then this is > >>>>> + * assumed to be 1.0 > >>>>> + * > >>>>> * Note that all the property extensions described here apply either to the > >>>>> * plane or the CRTC (e.g. for the background color, which currently is not > >>>>> * exposed and assumed to be black). > >>>>> @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > >>>>> return 0; > >>>>> } > >>>>> EXPORT_SYMBOL(drm_atomic_normalize_zpos); > >>>>> + > >>>>> +/** > >>>>> + * drm_plane_create_blend_mode_property - create a new blend mode property > >>>>> + * @plane: drm plane > >>>>> + * @supported_modes: bitmask of supported modes, must include > >>>>> + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is > >>>>> + * that alpha is premultiplied, and old userspace can break if > >>>>> + * the property defaults to coverage. > >>>>> + * > >>>> Thanks for the explanation Lowry. > >>>> Sigh, old userspace :-\ > >>>> > >>>> One pedantic suggestion s/defaults to coverage/defaults to anything else/ > >>>> Since defaulting to "none" or any future blend mode is also a no-go. > >>>> > >>>> I wouldn't bother resending solely for ^^. Perhaps squash locally > >>>> before merging? > >>>> > >>>> With that, the patch is > >>>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com> > >>>> > >>>> -Emil > >>> Hi Emil, > >>> > >>> Thanks a lot for your review and will change that. > >> Ping? Any updates? > >> > >> ~Maarten > > Hi Maarten, > > > > So far the merge request on user space patch has been verified by John > > Stultz. The comments from Sean has been fixed and now we are waiting the > > reviewed-tag from Sean. Once it merged, we'll send a new kernel patch to > > review. > > > Hey, > > In general the kernel patch should be merged first, because userspace cannot rely on the behavior until the next -rc1 that integrates it. > However, in order to merge the kernel patch, a userspace proof of concept for validation is needed first. Since you're close to merging, > I would say the userspace requirement is satisfied. It's safe to send the kernel patch. :) > > ~Maarten Hi Maarten, Sure, will send a new patchset(version-4) to review. Thanks for your time:)
On Tue, Aug 14, 2018 at 11:15:43AM +0200, Maarten Lankhorst wrote: > Op 14-08-18 om 05:11 schreef Lowry Li: > > On Mon, Aug 13, 2018 at 12:49:13PM +0200, Maarten Lankhorst wrote: > >> Op 05-06-18 om 11:07 schreef Lowry Li: > >>> On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: > >>>> On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: > >>>>> Pixel blend modes represent the alpha blending equation > >>>>> selection, describing how the pixels from the current > >>>>> plane are composited with the background. > >>>>> > >>>>> Add a pixel_blend_mode to drm_plane_state and a > >>>>> blend_mode_property to drm_plane, and related support > >>>>> functions. > >>>>> > >>>>> Defines three blend modes in drm_blend.h. > >>>>> > >>>>> Signed-off-by: Lowry Li <lowry.li@arm.com> > >>>>> --- > >>>>> drivers/gpu/drm/drm_atomic.c | 4 ++ > >>>>> drivers/gpu/drm/drm_atomic_helper.c | 1 + > >>>>> drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ > >>>>> include/drm/drm_blend.h | 6 ++ > >>>>> include/drm/drm_plane.h | 5 ++ > >>>>> 5 files changed, 142 insertions(+) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>>>> index 07fef42..1d18389 100644 > >>>>> --- a/drivers/gpu/drm/drm_atomic.c > >>>>> +++ b/drivers/gpu/drm/drm_atomic.c > >>>>> @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > >>>>> state->src_h = val; > >>>>> } else if (property == plane->alpha_property) { > >>>>> state->alpha = val; > >>>>> + } else if (property == plane->blend_mode_property) { > >>>>> + state->pixel_blend_mode = val; > >>>>> } else if (property == plane->rotation_property) { > >>>>> if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) > >>>>> return -EINVAL; > >>>>> @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > >>>>> *val = state->src_h; > >>>>> } else if (property == plane->alpha_property) { > >>>>> *val = state->alpha; > >>>>> + } else if (property == plane->blend_mode_property) { > >>>>> + *val = state->pixel_blend_mode; > >>>>> } else if (property == plane->rotation_property) { > >>>>> *val = state->rotation; > >>>>> } else if (property == plane->zpos_property) { > >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>>>> index 130da51..7f5d463 100644 > >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>>>> @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) > >>>>> /* Reset the alpha value to fully opaque if it matters */ > >>>>> if (plane->alpha_property) > >>>>> plane->state->alpha = plane->alpha_property->values[1]; > >>>>> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > >>>>> } > >>>>> } > >>>>> EXPORT_SYMBOL(drm_atomic_helper_plane_reset); > >>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > >>>>> index a16a74d..ac6f19c 100644 > >>>>> --- a/drivers/gpu/drm/drm_blend.c > >>>>> +++ b/drivers/gpu/drm/drm_blend.c > >>>>> @@ -107,6 +107,52 @@ > >>>>> * planes. Without this property the primary plane is always below the cursor > >>>>> * plane, and ordering between all other planes is undefined. > >>>>> * > >>>>> + * pixel blend mode: > >>>>> + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > >>>>> + * It adds a blend mode for alpha blending equation selection, describing > >>>>> + * how the pixels from the current plane are composited with the > >>>>> + * background. > >>>>> + * > >>>>> + * Three alpha blending equations are defined: > >>>>> + * > >>>>> + * "None": > >>>>> + * Blend formula that ignores the pixel alpha:: > >>>>> + * > >>>>> + * out.rgb = plane_alpha * fg.rgb + > >>>>> + * (1 - plane_alpha) * bg.rgb > >>>>> + * > >>>>> + * "Pre-multiplied": > >>>>> + * Blend formula that assumes the pixel color values > >>>>> + * have been already pre-multiplied with the alpha > >>>>> + * channel values:: > >>>>> + * > >>>>> + * out.rgb = plane_alpha * fg.rgb + > >>>>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > >>>>> + * > >>>>> + * "Coverage": > >>>>> + * Blend formula that assumes the pixel color values have not > >>>>> + * been pre-multiplied and will do so when blending them to the > >>>>> + * background color values:: > >>>>> + * > >>>>> + * out.rgb = plane_alpha * fg.alpha * fg.rgb + > >>>>> + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > >>>>> + * > >>>>> + * Using the following symbols: > >>>>> + * > >>>>> + * ``fg.rgb``: > >>>>> + * Each of the RGB component values from the plane's pixel > >>>>> + * ``fg.alpha``: > >>>>> + * Alpha component value from the plane's pixel. If the plane's > >>>>> + * pixel format has no alpha component, then this is assumed to be > >>>>> + * 1.0. In these cases, this property has no effect, as all three > >>>>> + * equations become equivalent. > >>>>> + * ``bg.rgb``: > >>>>> + * Each of the RGB component values from the background > >>>>> + * ``plane_alpha``: > >>>>> + * Plane alpha value set by the plane "alpha" property. If the > >>>>> + * plane does not expose the "alpha" property, then this is > >>>>> + * assumed to be 1.0 > >>>>> + * > >>>>> * Note that all the property extensions described here apply either to the > >>>>> * plane or the CRTC (e.g. for the background color, which currently is not > >>>>> * exposed and assumed to be black). > >>>>> @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > >>>>> return 0; > >>>>> } > >>>>> EXPORT_SYMBOL(drm_atomic_normalize_zpos); > >>>>> + > >>>>> +/** > >>>>> + * drm_plane_create_blend_mode_property - create a new blend mode property > >>>>> + * @plane: drm plane > >>>>> + * @supported_modes: bitmask of supported modes, must include > >>>>> + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is > >>>>> + * that alpha is premultiplied, and old userspace can break if > >>>>> + * the property defaults to coverage. > >>>>> + * > >>>> Thanks for the explanation Lowry. > >>>> Sigh, old userspace :-\ > >>>> > >>>> One pedantic suggestion s/defaults to coverage/defaults to anything else/ > >>>> Since defaulting to "none" or any future blend mode is also a no-go. > >>>> > >>>> I wouldn't bother resending solely for ^^. Perhaps squash locally > >>>> before merging? > >>>> > >>>> With that, the patch is > >>>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com> > >>>> > >>>> -Emil > >>> Hi Emil, > >>> > >>> Thanks a lot for your review and will change that. > >> Ping? Any updates? > >> > >> ~Maarten > > Hi Maarten, > > > > So far the merge request on user space patch has been verified by John > > Stultz. The comments from Sean has been fixed and now we are waiting the > > reviewed-tag from Sean. Once it merged, we'll send a new kernel patch to > > review. > > > Hey, > > In general the kernel patch should be merged first, because userspace cannot rely on the behavior until the next -rc1 that integrates it. > However, in order to merge the kernel patch, a userspace proof of concept for validation is needed first. Since you're close to merging, > I would say the userspace requirement is satisfied. It's safe to send the kernel patch. :) Quick correction: Generally merged into -next is considered good enough. -Daniel > > ~Maarten > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Jun 04, 2018 at 02:49:26PM +0100, Emil Velikov wrote: > On 1 June 2018 at 13:41, Lowry Li <lowry.li@arm.com> wrote: > > Pixel blend modes represent the alpha blending equation > > selection, describing how the pixels from the current > > plane are composited with the background. > > > > Add a pixel_blend_mode to drm_plane_state and a > > blend_mode_property to drm_plane, and related support > > functions. > > > > Defines three blend modes in drm_blend.h. > > > > Signed-off-by: Lowry Li <lowry.li@arm.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 4 ++ > > drivers/gpu/drm/drm_atomic_helper.c | 1 + > > drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ > > include/drm/drm_blend.h | 6 ++ > > include/drm/drm_plane.h | 5 ++ > > 5 files changed, 142 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 07fef42..1d18389 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > state->src_h = val; > > } else if (property == plane->alpha_property) { > > state->alpha = val; > > + } else if (property == plane->blend_mode_property) { > > + state->pixel_blend_mode = val; > > } else if (property == plane->rotation_property) { > > if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) > > return -EINVAL; > > @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > *val = state->src_h; > > } else if (property == plane->alpha_property) { > > *val = state->alpha; > > + } else if (property == plane->blend_mode_property) { > > + *val = state->pixel_blend_mode; > > } else if (property == plane->rotation_property) { > > *val = state->rotation; > > } else if (property == plane->zpos_property) { > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 130da51..7f5d463 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) > > /* Reset the alpha value to fully opaque if it matters */ > > if (plane->alpha_property) > > plane->state->alpha = plane->alpha_property->values[1]; > > + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; > > } > > } > > EXPORT_SYMBOL(drm_atomic_helper_plane_reset); > > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > > index a16a74d..ac6f19c 100644 > > --- a/drivers/gpu/drm/drm_blend.c > > +++ b/drivers/gpu/drm/drm_blend.c > > @@ -107,6 +107,52 @@ > > * planes. Without this property the primary plane is always below the cursor > > * plane, and ordering between all other planes is undefined. > > * > > + * pixel blend mode: > > + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > > + * It adds a blend mode for alpha blending equation selection, describing > > + * how the pixels from the current plane are composited with the > > + * background. > > + * > > + * Three alpha blending equations are defined: > > + * > > + * "None": > > + * Blend formula that ignores the pixel alpha:: > > + * > > + * out.rgb = plane_alpha * fg.rgb + > > + * (1 - plane_alpha) * bg.rgb > > + * > > + * "Pre-multiplied": > > + * Blend formula that assumes the pixel color values > > + * have been already pre-multiplied with the alpha > > + * channel values:: > > + * > > + * out.rgb = plane_alpha * fg.rgb + > > + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > > + * > > + * "Coverage": > > + * Blend formula that assumes the pixel color values have not > > + * been pre-multiplied and will do so when blending them to the > > + * background color values:: > > + * > > + * out.rgb = plane_alpha * fg.alpha * fg.rgb + > > + * (1 - (plane_alpha * fg.alpha)) * bg.rgb > > + * > > + * Using the following symbols: > > + * > > + * ``fg.rgb``: > > + * Each of the RGB component values from the plane's pixel > > + * ``fg.alpha``: > > + * Alpha component value from the plane's pixel. If the plane's > > + * pixel format has no alpha component, then this is assumed to be > > + * 1.0. In these cases, this property has no effect, as all three > > + * equations become equivalent. > > + * ``bg.rgb``: > > + * Each of the RGB component values from the background > > + * ``plane_alpha``: > > + * Plane alpha value set by the plane "alpha" property. If the > > + * plane does not expose the "alpha" property, then this is > > + * assumed to be 1.0 > > + * > > * Note that all the property extensions described here apply either to the > > * plane or the CRTC (e.g. for the background color, which currently is not > > * exposed and assumed to be black). > > @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, > > return 0; > > } > > EXPORT_SYMBOL(drm_atomic_normalize_zpos); > > + > > +/** > > + * drm_plane_create_blend_mode_property - create a new blend mode property > > + * @plane: drm plane > > + * @supported_modes: bitmask of supported modes, must include > > + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is > > + * that alpha is premultiplied, and old userspace can break if > > + * the property defaults to coverage. > > + * > > Thanks for the explanation Lowry. > Sigh, old userspace :-\ > > One pedantic suggestion s/defaults to coverage/defaults to anything else/ > Since defaulting to "none" or any future blend mode is also a no-go. > > I wouldn't bother resending solely for ^^. Perhaps squash locally > before merging? > > With that, the patch is > Reviewed-by: Emil Velikov <emil.velikov@collabora.com> > > -Emil Hi Emil, The comments has been changed as you request in the v4 patchset. Since it also did some refine in other places (please refer to the cover letter), please let me know if I can have your reviewed-tag :) Thanks a lot for your time.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 07fef42..1d18389 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->src_h = val; } else if (property == plane->alpha_property) { state->alpha = val; + } else if (property == plane->blend_mode_property) { + state->pixel_blend_mode = val; } else if (property == plane->rotation_property) { if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) return -EINVAL; @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, *val = state->src_h; } else if (property == plane->alpha_property) { *val = state->alpha; + } else if (property == plane->blend_mode_property) { + *val = state->pixel_blend_mode; } else if (property == plane->rotation_property) { *val = state->rotation; } else if (property == plane->zpos_property) { diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 130da51..7f5d463 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) /* Reset the alpha value to fully opaque if it matters */ if (plane->alpha_property) plane->state->alpha = plane->alpha_property->values[1]; + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; } } EXPORT_SYMBOL(drm_atomic_helper_plane_reset); diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d..ac6f19c 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,52 @@ * planes. Without this property the primary plane is always below the cursor * plane, and ordering between all other planes is undefined. * + * pixel blend mode: + * Pixel blend mode is set up with drm_plane_create_blend_mode_property(). + * It adds a blend mode for alpha blending equation selection, describing + * how the pixels from the current plane are composited with the + * background. + * + * Three alpha blending equations are defined: + * + * "None": + * Blend formula that ignores the pixel alpha:: + * + * out.rgb = plane_alpha * fg.rgb + + * (1 - plane_alpha) * bg.rgb + * + * "Pre-multiplied": + * Blend formula that assumes the pixel color values + * have been already pre-multiplied with the alpha + * channel values:: + * + * out.rgb = plane_alpha * fg.rgb + + * (1 - (plane_alpha * fg.alpha)) * bg.rgb + * + * "Coverage": + * Blend formula that assumes the pixel color values have not + * been pre-multiplied and will do so when blending them to the + * background color values:: + * + * out.rgb = plane_alpha * fg.alpha * fg.rgb + + * (1 - (plane_alpha * fg.alpha)) * bg.rgb + * + * Using the following symbols: + * + * ``fg.rgb``: + * Each of the RGB component values from the plane's pixel + * ``fg.alpha``: + * Alpha component value from the plane's pixel. If the plane's + * pixel format has no alpha component, then this is assumed to be + * 1.0. In these cases, this property has no effect, as all three + * equations become equivalent. + * ``bg.rgb``: + * Each of the RGB component values from the background + * ``plane_alpha``: + * Plane alpha value set by the plane "alpha" property. If the + * plane does not expose the "alpha" property, then this is + * assumed to be 1.0 + * * Note that all the property extensions described here apply either to the * plane or the CRTC (e.g. for the background color, which currently is not * exposed and assumed to be black). @@ -448,3 +494,83 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos); + +/** + * drm_plane_create_blend_mode_property - create a new blend mode property + * @plane: drm plane + * @supported_modes: bitmask of supported modes, must include + * BIT(DRM_MODE_BLEND_PREMULTI). Current DRM assumption is + * that alpha is premultiplied, and old userspace can break if + * the property defaults to coverage. + * + * This creates a new property describing the blend mode. + * + * The property exposed to userspace is an enumeration property (see + * drm_property_create_enum()) called "pixel blend mode" and has the + * following enumeration values: + * + * "None": + * Blend formula that ignores the pixel alpha. + * + * "Pre-multiplied": + * Blend formula that assumes the pixel color values have been already + * pre-multiplied with the alpha channel values. + * + * "Coverage": + * Blend formula that assumes the pixel color values have not been + * pre-multiplied and will do so when blending them to the background color + * values. + * + * RETURNS: + * Zero for success or -errno + */ +int drm_plane_create_blend_mode_property(struct drm_plane *plane, + unsigned int supported_modes) +{ + struct drm_device *dev = plane->dev; + struct drm_property *prop; + static const struct drm_prop_enum_list props[] = { + { DRM_MODE_BLEND_PIXEL_NONE, "None" }, + { DRM_MODE_BLEND_PREMULTI, "Pre-multiplied" }, + { DRM_MODE_BLEND_COVERAGE, "Coverage" }, + }; + unsigned int valid_mode_mask = BIT(DRM_MODE_BLEND_PIXEL_NONE) | + BIT(DRM_MODE_BLEND_PREMULTI) | + BIT(DRM_MODE_BLEND_COVERAGE); + int i, j = 0; + + if (WARN_ON((supported_modes & ~valid_mode_mask) || + ((supported_modes & BIT(DRM_MODE_BLEND_PREMULTI)) == 0))) + return -EINVAL; + + prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, + "pixel blend mode", + hweight32(supported_modes)); + if (!prop) + return -ENOMEM; + + for (i = 0; i < ARRAY_SIZE(props); i++) { + int ret; + + if (!(BIT(props[i].type) & supported_modes)) + continue; + + ret = drm_property_add_enum(prop, j++, props[i].type, + props[i].name); + + if (ret) { + drm_property_destroy(dev, prop); + + return ret; + } + } + + drm_object_attach_property(&plane->base, prop, DRM_MODE_BLEND_PREMULTI); + plane->blend_mode_property = prop; + + if (plane->state) + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_blend_mode_property); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561..28c5076 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -27,6 +27,10 @@ #include <linux/ctype.h> #include <drm/drm_mode.h> +#define DRM_MODE_BLEND_PIXEL_NONE 0 +#define DRM_MODE_BLEND_PREMULTI 1 +#define DRM_MODE_BLEND_COVERAGE 2 + struct drm_device; struct drm_atomic_state; struct drm_plane; @@ -52,4 +56,6 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state); +int drm_plane_create_blend_mode_property(struct drm_plane *plane, + unsigned int supported_modes); #endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 14b1607..6e2c045 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -44,6 +44,8 @@ * @src_w: width of visible portion of plane (in 16.16) * @src_h: height of visible portion of plane (in 16.16) * @alpha: opacity of the plane + * @pixel_blend_mode: how the plane's framebuffer alpha channel is used when + * blending with the background colour. * @rotation: rotation of the plane * @zpos: priority of the given plane on crtc (optional) * Note that multiple active planes on the same crtc can have an identical @@ -116,6 +118,7 @@ struct drm_plane_state { /* Plane opacity */ u16 alpha; + uint16_t pixel_blend_mode; /* Plane rotation */ unsigned int rotation; @@ -513,6 +516,7 @@ enum drm_plane_type { * @alpha_property: alpha property for this plane * @zpos_property: zpos property for this plane * @rotation_property: rotation property for this plane + * @blend_mode_property: blend mode property for this plane * @helper_private: mid-layer private data */ struct drm_plane { @@ -589,6 +593,7 @@ struct drm_plane { struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property; + struct drm_property *blend_mode_property; /** * @color_encoding_property:
Pixel blend modes represent the alpha blending equation selection, describing how the pixels from the current plane are composited with the background. Add a pixel_blend_mode to drm_plane_state and a blend_mode_property to drm_plane, and related support functions. Defines three blend modes in drm_blend.h. Signed-off-by: Lowry Li <lowry.li@arm.com> --- drivers/gpu/drm/drm_atomic.c | 4 ++ drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_blend.c | 126 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 6 ++ include/drm/drm_plane.h | 5 ++ 5 files changed, 142 insertions(+)