Re: [PATCH v3 1/2] drm: Add per-plane pixel blend mode property

[Maarten Lankhorst]

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@xxxxxxx> 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@xxxxxxx>
>>>>> ---
>>>>>  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@xxxxxxxxxxxxx>
>>>>
>>>> -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