Re: [PATCH v2] drm/vkms: Implement all blend mode properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Melissa,

On 7/23/23 18:00, Melissa Wen wrote:
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@kms_plane_alpha_blend.

Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
---

v1 -> v2: https://lore.kernel.org/dri-devel/20230428122751.24271-1-mcanal@xxxxxxxxxx/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?

The idea here is the same as before, but now that we have two factors
multiplying src and dst (alpha and frame_info->alpha), so we need to divide by 0xffff * 0xffff = 0xfffe0001. This helps us not to lose
precision when dividing the alpha and frame_info->alpha values by
0xffff.

I'm using the formulas specified in the DRM documentation [1]:

"None": out.rgb = plane_alpha * fg.rgb + (1 - plane_alpha) * bg.rgb

"Pre-multiplied": out.rgb = plane_alpha * fg.rgb +
                            (1 - (plane_alpha * fg.alpha)) * bg.rgb

"Coverage": out.rgb = plane_alpha * fg.alpha * fg.rgb +
                      (1 - (plane_alpha * fg.alpha)) * bg.rgb

I can add some comments in the code to make it clear that 0xfffe0001 is
0xffff * 0xffff.

[1] https://docs.kernel.org/gpu/drm-kms.html#plane-composition-properties


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?


I verified a sample of the resulting pixels while debugging the
implementation with the IGT tests. It looked okay to me, but let me know
if you find something suspecious.

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.

I was able to reproduce this failure as well. Maybe we need some
improviments on the IGT tests to make sure about the correctness of the
pixel blend mode. For this patch, I based myself on the correctness of
the formulas provided by the DRM documentation.

Would you need more IGT tests to approve this feature?

Best Regards,
- Maíra


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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux