[PATCH] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

[PATCH] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

pre_blend_color_transform() traverses the colorop pipeline and unpacks
each pixel into s32 for every line, even when all colorops are bypassed
or the pipeline is empty.  Add a line-level check to skip the entire
function in that case, avoiding per-pixel overhead.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index f3fb203946e8..1806c249da6f 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -190,13 +190,29 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
 	}
 }
 
+static bool pipeline_all_bypassed(struct drm_colorop *colorop)
+{
+	while (colorop) {
+		struct drm_colorop_state *colorop_state = colorop->state;
+
+		if (!colorop_state || !colorop_state->bypass)
+			return false;
+		colorop = colorop->next;
+	}
+	return true;
+}
+
 static void pre_blend_color_transform(const struct vkms_plane_state *plane_state,
 				      struct line_buffer *output_buffer)
 {
+	struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
 	struct pixel_argb_s32 pixel;
 
+	if (!colorop || pipeline_all_bypassed(colorop))
+		return;
+
 	for (size_t x = 0; x < output_buffer->n_pixels; x++) {
-		struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
+		colorop = plane_state->base.base.color_pipeline;
 
 		/*
 		 * Some operations, such as applying a BT709 encoding matrix,
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] VKMS composer accesses global colorop->state directly without shadowing it during atomic commit, leading to a data race and potential Use-After-Free.
--

commit 7feb24a5b3e1c9cb2aa23694b44e46aece887a34
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

This patch adds an optimization to pre_blend_color_transform() to skip
the entire pixel processing loop when all colorops in the pipeline are
bypassed or the pipeline is empty.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..18f58569f0169 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -190,13 +190,29 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
>  	}
>  }
>  
> +static bool pipeline_all_bypassed(struct drm_colorop *colorop)
> +{
> +	while (colorop) {
> +		struct drm_colorop_state *colorop_state = colorop->state;

[Severity: High]
This is a pre-existing issue, but does this direct access to colorop->state
introduce a potential data race and use-after-free?

The VKMS composer processes frames asynchronously in a workqueue
(vkms_composer_worker). While plane states are properly duplicated and
shadowed, reading the global colorop->state pointer directly here in
pipeline_all_bypassed() within drivers/gpu/drm/vkms/vkms_composer.c seems
unsafe.

If a non-blocking atomic commit swaps the state via
drm_atomic_helper_swap_state() and frees the old state via
drm_atomic_state_put() while the composer worker is preempted, will the
worker dereference a freed state pointer when it resumes? Local userspace
with DRM master privileges could potentially trigger this by submitting rapid
non-blocking atomic commits that modify the color pipeline while the virtual
hardware is compositing frames.

This same unsafe access pattern already exists in the inner loop of
pre_blend_color_transform(), but this patch expands the pattern into the new
pipeline_all_bypassed() function. Should the colorop state be properly
shadowed for the atomic commit instead of being read globally?

> +
> +		if (!colorop_state || !colorop_state->bypass)
> +			return false;
> +		colorop = colorop->next;
> +	}
> +	return true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909021028.105837-1-oushixiong1025@163.com?part=1