[PATCH v2] drm/vkms: Implement all blend mode properties
Melissa Wen
mwen at igalia.com
Sun Jul 23 21:00:44 UTC 2023
On 07/23, Maíra Canal wrote:
> Following the DRM assumption, VKMS currently assumes that the alpha is
> pre-multiplied. Moreover, it doesn't support the alpha property.
>
> So, first, implement the alpha property to VKMS and then, the blend
> mode property. In order to support all possible supported modes,
> change the pre_mul_blend_channel() function to check the plane blend
> mode and apply the correct blend formula, following the DRM
> convention, using the proper plane alpha value.
>
> Tested with igt at kms_plane_alpha_blend.
>
> Signed-off-by: Maíra Canal <mcanal at igalia.com>
> ---
>
> v1 -> v2: https://lore.kernel.org/dri-devel/20230428122751.24271-1-mcanal@igalia.com/T/
>
> * Rebased on top of drm-misc-next.
>
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 49 ++++++++++++++++++----------
> drivers/gpu/drm/vkms/vkms_drv.h | 2 ++
> drivers/gpu/drm/vkms/vkms_plane.c | 9 +++++
> 3 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index d170a8e89b95..68a476461824 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -12,33 +12,47 @@
>
> #include "vkms_drv.h"
>
> -static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
> +static u16 blend_channel(struct vkms_frame_info *frame_info, u16 src, u16 dst, u16 alpha)
> {
> u32 new_color;
>
> - new_color = (src * 0xffff + dst * (0xffff - alpha));
> + switch (frame_info->pixel_blend_mode) {
> + case DRM_MODE_BLEND_PIXEL_NONE:
> + new_color = 0xffff * frame_info->alpha * src
> + + (0xfffe0001 - 0xffff * frame_info->alpha) * dst;
> + break;
> + case DRM_MODE_BLEND_COVERAGE:
> + new_color = alpha * frame_info->alpha * src
> + + (0xfffe0001 - alpha * frame_info->alpha) * dst;
> + break;
> + case DRM_MODE_BLEND_PREMULTI:
> + default:
> + new_color = 0xffff * frame_info->alpha * src
> + + (0xfffe0001 - alpha * frame_info->alpha) * dst;
Hi Maíra,
First, thank you for keep working on this feature and sorry for feedback
delay.
I didn't complete understand this 0xfffe0001 calculation. Could you
describe a little more (adding a comment here) each formula that you are
using here?
Also, as we are changing pixels of both outputs that are part of the
comparisons, but we just compare the CRC (not pixel by pixel). Did you
verify a sample of the pixels resulted from this calculation to check if
they make sense?
IIRC, we don't have a negative test for this property in
kms_plane_alpha_blend. In this sense, did you verify if the test fail
when setting a property value but using a different equation that not
correspond to the pixel blend mode setup? Overall the change makes
sense, but I tried some invalid scenarios and didn't get the expected
result.
Melissa
> + break;
> + }
>
> - return DIV_ROUND_CLOSEST(new_color, 0xffff);
> + return DIV_ROUND_CLOSEST(new_color, 0xfffe0001);
> }
>
> /**
> - * pre_mul_alpha_blend - alpha blending equation
> + * alpha_blend - alpha blending equation
> * @frame_info: Source framebuffer's metadata
> * @stage_buffer: The line with the pixels from src_plane
> * @output_buffer: A line buffer that receives all the blends output
> *
> * Using the information from the `frame_info`, this blends only the
> * necessary pixels from the `stage_buffer` to the `output_buffer`
> - * using premultiplied blend formula.
> + * using the adequate blend formula depending on the plane blend mode
> + * (see blend_channel()).
> *
> - * The current DRM assumption is that pixel color values have been already
> - * pre-multiplied with the alpha channel values. See more
> - * drm_plane_create_blend_mode_property(). Also, this formula assumes a
> - * completely opaque background.
> + * By default, the current DRM assumption is that pixel color values have
> + * been already pre-multiplied with the alpha channel values. See more
> + * drm_plane_create_blend_mode_property().
> */
> -static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
> - struct line_buffer *stage_buffer,
> - struct line_buffer *output_buffer)
> +static void alpha_blend(struct vkms_frame_info *frame_info,
> + struct line_buffer *stage_buffer,
> + struct line_buffer *output_buffer)
> {
> int x_dst = frame_info->dst.x1;
> struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
> @@ -48,9 +62,9 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
>
> for (int x = 0; x < x_limit; x++) {
> out[x].a = (u16)0xffff;
> - out[x].r = pre_mul_blend_channel(in[x].r, out[x].r, in[x].a);
> - out[x].g = pre_mul_blend_channel(in[x].g, out[x].g, in[x].a);
> - out[x].b = pre_mul_blend_channel(in[x].b, out[x].b, in[x].a);
> + out[x].r = blend_channel(frame_info, in[x].r, out[x].r, in[x].a);
> + out[x].g = blend_channel(frame_info, in[x].g, out[x].g, in[x].a);
> + out[x].b = blend_channel(frame_info, in[x].b, out[x].b, in[x].a);
> }
> }
>
> @@ -98,7 +112,7 @@ static void fill_background(const struct pixel_argb_u16 *background_color,
> * @stage_buffer: The line with the pixels from plane being blend to the output
> * @row_size: The size, in bytes, of a single row
> *
> - * This function blends the pixels (Using the `pre_mul_alpha_blend`)
> + * This function blends the pixels (Using the `alpha_blend()`)
> * from all planes, calculates the crc32 of the output from the former step,
> * and, if necessary, convert and store the output to the writeback buffer.
> */
> @@ -126,8 +140,7 @@ static void blend(struct vkms_writeback_job *wb,
> continue;
>
> vkms_compose_row(stage_buffer, plane[i], y_pos);
> - pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> - output_buffer);
> + alpha_blend(plane[i]->frame_info, stage_buffer, output_buffer);
> }
>
> *crc32 = crc32_le(*crc32, (void *)output_buffer->pixels, row_size);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index d25e0aae91f2..20e2c520885e 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -28,6 +28,8 @@ struct vkms_frame_info {
> struct drm_rect src, dst;
> struct drm_rect rotated;
> struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> + u16 alpha;
> + u16 pixel_blend_mode;
> unsigned int rotation;
> unsigned int offset;
> unsigned int pitch;
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index e5c625ab8e3e..891aa566acda 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -113,6 +113,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
> memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
> memcpy(&frame_info->rotated, &new_state->dst, sizeof(struct drm_rect));
> + frame_info->alpha = new_state->alpha;
> + frame_info->pixel_blend_mode = new_state->pixel_blend_mode;
> frame_info->fb = fb;
> memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
> drm_framebuffer_get(frame_info->fb);
> @@ -212,6 +214,13 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>
> drm_plane_helper_add(&plane->base, &vkms_plane_helper_funcs);
>
> + drm_plane_create_alpha_property(&plane->base);
> +
> + drm_plane_create_blend_mode_property(&plane->base,
> + BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE));
> +
> drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
> DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK);
>
> --
> 2.41.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230723/8380a0bf/attachment.sig>
More information about the dri-devel
mailing list