The pre_mul_alpha_blend is dedicated to blending, so to avoid mixing
different concepts (coordinate calculation and color management), extract
the x_limit and x_dst computation outside of this helper.
It also increases the maintainability by grouping the computation related
to coordinates in the same place: the loop in `blend`.
Signed-off-by: Louis Chauvet <[email protected]>
---
drivers/gpu/drm/vkms/vkms_composer.c | 40 +++++++++++++++++-------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index da0651a94c9b..9254086f23ff 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -24,34 +24,30 @@ static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
/**
* pre_mul_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
+ * @x_start: The start offset to avoid useless copy
+ * @count: The number of byte to copy
*
- * 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 @x_start and @count information, only few pixel can be blended instead of the whole line
+ * each time.
*
* 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.
*/
-static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
- struct line_buffer *stage_buffer,
- struct line_buffer *output_buffer)
+static void pre_mul_alpha_blend(const struct line_buffer *stage_buffer,
+ struct line_buffer *output_buffer, int x_start, int pixel_count)
{
- int x_dst = frame_info->dst.x1;
- struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
- struct pixel_argb_u16 *in = stage_buffer->pixels;
- int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
- stage_buffer->n_pixels);
-
- 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);
+ struct pixel_argb_u16 *out = &output_buffer->pixels[x_start];
+ const struct pixel_argb_u16 *in = stage_buffer->pixels;
+
+ for (int i = 0; i < pixel_count; i++) {
+ out[i].a = (u16)0xffff;
+ out[i].r = pre_mul_blend_channel(in[i].r, out[i].r, in[i].a);
+ out[i].g = pre_mul_blend_channel(in[i].g, out[i].g, in[i].a);
+ out[i].b = pre_mul_blend_channel(in[i].b, out[i].b, in[i].a);
}
}
@@ -183,7 +179,7 @@ static void blend(struct vkms_writeback_job *wb,
{
struct vkms_plane_state **plane = crtc_state->active_planes;
u32 n_active_planes = crtc_state->num_active_planes;
- int y_pos;
+ int y_pos, x_dst, x_limit;
const struct pixel_argb_u16 background_color = { .a = 0xffff };
@@ -201,14 +197,16 @@ static void blend(struct vkms_writeback_job *wb,
/* The active planes are composed associatively in z-order. */
for (size_t i = 0; i < n_active_planes; i++) {
+ x_dst = plane[i]->frame_info->dst.x1;
+ x_limit = min_t(size_t, drm_rect_width(&plane[i]->frame_info->dst),
+ stage_buffer->n_pixels);
y_pos = get_y_pos(plane[i]->frame_info, y);
if (!check_limit(plane[i]->frame_info, y_pos))
continue;
vkms_compose_row(stage_buffer, plane[i], y_pos);
- pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
- output_buffer);
+ pre_mul_alpha_blend(stage_buffer, output_buffer, x_dst, x_limit);
}
apply_lut(crtc_state, output_buffer);
--
2.43.0
On Wed, 13 Mar 2024 18:45:02 +0100
Louis Chauvet <[email protected]> wrote:
> The pre_mul_alpha_blend is dedicated to blending, so to avoid mixing
> different concepts (coordinate calculation and color management), extract
> the x_limit and x_dst computation outside of this helper.
> It also increases the maintainability by grouping the computation related
> to coordinates in the same place: the loop in `blend`.
>
> Signed-off-by: Louis Chauvet <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 40 +++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index da0651a94c9b..9254086f23ff 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -24,34 +24,30 @@ static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
>
> /**
> * pre_mul_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
> + * @x_start: The start offset to avoid useless copy
I'd say just:
+ * @x_start: The start offset
It describes the parameter, and the paragraph below explains the why.
It would be explaining, that x_start applies to output_buffer, but
input_buffer is always read starting from 0.
> + * @count: The number of byte to copy
You named it pixel_count, and it counts pixels, not bytes. It's not a
copy but a blend into output_buffer.
> *
> - * 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 @x_start and @count information, only few pixel can be blended instead of the whole line
> + * each time.
> *
> * 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.
> */
> -static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
> - struct line_buffer *stage_buffer,
> - struct line_buffer *output_buffer)
> +static void pre_mul_alpha_blend(const struct line_buffer *stage_buffer,
> + struct line_buffer *output_buffer, int x_start, int pixel_count)
> {
> - int x_dst = frame_info->dst.x1;
> - struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
> - struct pixel_argb_u16 *in = stage_buffer->pixels;
> - int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> - stage_buffer->n_pixels);
> -
> - 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);
> + struct pixel_argb_u16 *out = &output_buffer->pixels[x_start];
> + const struct pixel_argb_u16 *in = stage_buffer->pixels;
> +
> + for (int i = 0; i < pixel_count; i++) {
> + out[i].a = (u16)0xffff;
> + out[i].r = pre_mul_blend_channel(in[i].r, out[i].r, in[i].a);
> + out[i].g = pre_mul_blend_channel(in[i].g, out[i].g, in[i].a);
> + out[i].b = pre_mul_blend_channel(in[i].b, out[i].b, in[i].a);
> }
> }
>
> @@ -183,7 +179,7 @@ static void blend(struct vkms_writeback_job *wb,
> {
> struct vkms_plane_state **plane = crtc_state->active_planes;
> u32 n_active_planes = crtc_state->num_active_planes;
> - int y_pos;
> + int y_pos, x_dst, x_limit;
>
> const struct pixel_argb_u16 background_color = { .a = 0xffff };
>
> @@ -201,14 +197,16 @@ static void blend(struct vkms_writeback_job *wb,
>
> /* The active planes are composed associatively in z-order. */
> for (size_t i = 0; i < n_active_planes; i++) {
> + x_dst = plane[i]->frame_info->dst.x1;
> + x_limit = min_t(size_t, drm_rect_width(&plane[i]->frame_info->dst),
> + stage_buffer->n_pixels);
Are those input values to min_t() really of type size_t? Or why is
size_t here?
> y_pos = get_y_pos(plane[i]->frame_info, y);
>
> if (!check_limit(plane[i]->frame_info, y_pos))
> continue;
>
> vkms_compose_row(stage_buffer, plane[i], y_pos);
> - pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> - output_buffer);
> + pre_mul_alpha_blend(stage_buffer, output_buffer, x_dst, x_limit);
I thought it was a count, not a limit?
"Limit" sounds to me like "end", and end - start = count.
> }
>
> apply_lut(crtc_state, output_buffer);
>
The details aside, this is a good move.
Thanks,
pq
Le 25/03/24 - 14:41, Pekka Paalanen a ?crit :
> On Wed, 13 Mar 2024 18:45:02 +0100
> Louis Chauvet <[email protected]> wrote:
>
> > The pre_mul_alpha_blend is dedicated to blending, so to avoid mixing
> > different concepts (coordinate calculation and color management), extract
> > the x_limit and x_dst computation outside of this helper.
> > It also increases the maintainability by grouping the computation related
> > to coordinates in the same place: the loop in `blend`.
> >
> > Signed-off-by: Louis Chauvet <[email protected]>
> > ---
> > drivers/gpu/drm/vkms/vkms_composer.c | 40 +++++++++++++++++-------------------
> > 1 file changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index da0651a94c9b..9254086f23ff 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -24,34 +24,30 @@ static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
> >
> > /**
> > * pre_mul_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
> > + * @x_start: The start offset to avoid useless copy
>
> I'd say just:
>
> + * @x_start: The start offset
>
> It describes the parameter, and the paragraph below explains the why.
>
> It would be explaining, that x_start applies to output_buffer, but
> input_buffer is always read starting from 0.
I will change it to:
* Using @x_start and @count information, only few pixel can be blended instead of the whole line
* each time. @x_start is only used for the output buffer. The staging buffer is always read from
* the start (0..@count in stage_buffer is blended at @x_start..@x_start+@count in output_buffer).
> > + * @count: The number of byte to copy
>
> You named it pixel_count, and it counts pixels, not bytes. It's not a
> copy but a blend into output_buffer.
Oops, fixed in v6.
> > *
> > - * 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 @x_start and @count information, only few pixel can be blended instead of the whole line
> > + * each time.
> > *
> > * 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.
> > */
> > -static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
> > - struct line_buffer *stage_buffer,
> > - struct line_buffer *output_buffer)
> > +static void pre_mul_alpha_blend(const struct line_buffer *stage_buffer,
> > + struct line_buffer *output_buffer, int x_start, int pixel_count)
> > {
> > - int x_dst = frame_info->dst.x1;
> > - struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
> > - struct pixel_argb_u16 *in = stage_buffer->pixels;
> > - int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> > - stage_buffer->n_pixels);
> > -
> > - 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);
> > + struct pixel_argb_u16 *out = &output_buffer->pixels[x_start];
> > + const struct pixel_argb_u16 *in = stage_buffer->pixels;
> > +
> > + for (int i = 0; i < pixel_count; i++) {
> > + out[i].a = (u16)0xffff;
> > + out[i].r = pre_mul_blend_channel(in[i].r, out[i].r, in[i].a);
> > + out[i].g = pre_mul_blend_channel(in[i].g, out[i].g, in[i].a);
> > + out[i].b = pre_mul_blend_channel(in[i].b, out[i].b, in[i].a);
> > }
> > }
> >
> > @@ -183,7 +179,7 @@ static void blend(struct vkms_writeback_job *wb,
> > {
> > struct vkms_plane_state **plane = crtc_state->active_planes;
> > u32 n_active_planes = crtc_state->num_active_planes;
> > - int y_pos;
> > + int y_pos, x_dst, x_limit;
> >
> > const struct pixel_argb_u16 background_color = { .a = 0xffff };
> >
> > @@ -201,14 +197,16 @@ static void blend(struct vkms_writeback_job *wb,
> >
> > /* The active planes are composed associatively in z-order. */
> > for (size_t i = 0; i < n_active_planes; i++) {
> > + x_dst = plane[i]->frame_info->dst.x1;
> > + x_limit = min_t(size_t, drm_rect_width(&plane[i]->frame_info->dst),
> > + stage_buffer->n_pixels);
>
> Are those input values to min_t() really of type size_t? Or why is
> size_t here?
n_pixel is size_t, drm_rect_width is int. I will change everything to int.
Is there a way to ask the compiler "please don't do implicit conversion
and report them as warn/errors"?
> > y_pos = get_y_pos(plane[i]->frame_info, y);
> >
> > if (!check_limit(plane[i]->frame_info, y_pos))
> > continue;
> >
> > vkms_compose_row(stage_buffer, plane[i], y_pos);
> > - pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> > - output_buffer);
> > + pre_mul_alpha_blend(stage_buffer, output_buffer, x_dst, x_limit);
>
> I thought it was a count, not a limit?
>
> "Limit" sounds to me like "end", and end - start = count.
It is effectively a pixel count. I just took those naming from the
original pre_mul_alpha_blend. I will change it to pixel_count.
Thanks,
Louis Chauvet
> > }
> >
> > apply_lut(crtc_state, output_buffer);
> >
>
> The details aside, this is a good move.
>
>
> Thanks,
> pq
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, 26 Mar 2024 16:57:00 +0100
Louis Chauvet <[email protected]> wrote:
> Le 25/03/24 - 14:41, Pekka Paalanen a écrit :
> > On Wed, 13 Mar 2024 18:45:02 +0100
> > Louis Chauvet <[email protected]> wrote:
> >
> > > The pre_mul_alpha_blend is dedicated to blending, so to avoid mixing
> > > different concepts (coordinate calculation and color management), extract
> > > the x_limit and x_dst computation outside of this helper.
> > > It also increases the maintainability by grouping the computation related
> > > to coordinates in the same place: the loop in `blend`.
> > >
> > > Signed-off-by: Louis Chauvet <[email protected]>
> > > ---
> > > drivers/gpu/drm/vkms/vkms_composer.c | 40 +++++++++++++++++-------------------
> > > 1 file changed, 19 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index da0651a94c9b..9254086f23ff 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -24,34 +24,30 @@ static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
> > >
> > > /**
> > > * pre_mul_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
> > > + * @x_start: The start offset to avoid useless copy
> >
> > I'd say just:
> >
> > + * @x_start: The start offset
> >
> > It describes the parameter, and the paragraph below explains the why.
> >
> > It would be explaining, that x_start applies to output_buffer, but
> > input_buffer is always read starting from 0.
>
> I will change it to:
>
> * Using @x_start and @count information, only few pixel can be blended instead of the whole line
> * each time. @x_start is only used for the output buffer. The staging buffer is always read from
> * the start (0..@count in stage_buffer is blended at @x_start..@x_start+@count in output_buffer).
The important part is
0..@count in stage_buffer is blended at @x_start..@x_start+@count in output_buffer
and everything else from that paragraph is not really adding much.
Remember to update the doc in "drm/vkms: Re-introduce line-per-line
composition algorithm" to follow the changes.
> > > + * @count: The number of byte to copy
> >
> > You named it pixel_count, and it counts pixels, not bytes. It's not a
> > copy but a blend into output_buffer.
>
> Oops, fixed in v6.
>
> > > *
> > > - * 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 @x_start and @count information, only few pixel can be blended instead of the whole line
> > > + * each time.
> > > *
> > > * 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.
> > > */
> > > -static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
> > > - struct line_buffer *stage_buffer,
> > > - struct line_buffer *output_buffer)
> > > +static void pre_mul_alpha_blend(const struct line_buffer *stage_buffer,
> > > + struct line_buffer *output_buffer, int x_start, int pixel_count)
> > > {
> > > - int x_dst = frame_info->dst.x1;
> > > - struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
> > > - struct pixel_argb_u16 *in = stage_buffer->pixels;
> > > - int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> > > - stage_buffer->n_pixels);
> > > -
> > > - 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);
> > > + struct pixel_argb_u16 *out = &output_buffer->pixels[x_start];
> > > + const struct pixel_argb_u16 *in = stage_buffer->pixels;
> > > +
> > > + for (int i = 0; i < pixel_count; i++) {
> > > + out[i].a = (u16)0xffff;
> > > + out[i].r = pre_mul_blend_channel(in[i].r, out[i].r, in[i].a);
> > > + out[i].g = pre_mul_blend_channel(in[i].g, out[i].g, in[i].a);
> > > + out[i].b = pre_mul_blend_channel(in[i].b, out[i].b, in[i].a);
> > > }
> > > }
> > >
> > > @@ -183,7 +179,7 @@ static void blend(struct vkms_writeback_job *wb,
> > > {
> > > struct vkms_plane_state **plane = crtc_state->active_planes;
> > > u32 n_active_planes = crtc_state->num_active_planes;
> > > - int y_pos;
> > > + int y_pos, x_dst, x_limit;
> > >
> > > const struct pixel_argb_u16 background_color = { .a = 0xffff };
> > >
> > > @@ -201,14 +197,16 @@ static void blend(struct vkms_writeback_job *wb,
> > >
> > > /* The active planes are composed associatively in z-order. */
> > > for (size_t i = 0; i < n_active_planes; i++) {
> > > + x_dst = plane[i]->frame_info->dst.x1;
> > > + x_limit = min_t(size_t, drm_rect_width(&plane[i]->frame_info->dst),
> > > + stage_buffer->n_pixels);
> >
> > Are those input values to min_t() really of type size_t? Or why is
> > size_t here?
>
> n_pixel is size_t, drm_rect_width is int. I will change everything to int
> Is there a way to ask the compiler "please don't do implicit conversion
> and report them as warn/errors"?
There probably is, you can find it in the gcc manual. However, I suspect
you would drown in warnings for cases where the implicit conversion is
wanted and an explicit cast is unwanted.
Thanks,
pq
> > > y_pos = get_y_pos(plane[i]->frame_info, y);
> > >
> > > if (!check_limit(plane[i]->frame_info, y_pos))
> > > continue;
> > >
> > > vkms_compose_row(stage_buffer, plane[i], y_pos);
> > > - pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> > > - output_buffer);
> > > + pre_mul_alpha_blend(stage_buffer, output_buffer, x_dst, x_limit);
> >
> > I thought it was a count, not a limit?
> >
> > "Limit" sounds to me like "end", and end - start = count.
>
> It is effectively a pixel count. I just took those naming from the
> original pre_mul_alpha_blend. I will change it to pixel_count.
>
> Thanks,
> Louis Chauvet
>
> > > }
> > >
> > > apply_lut(crtc_state, output_buffer);
> > >
> >
> > The details aside, this is a good move.
> >
> >
> > Thanks,
> > pq
>
>
>
Le 27/03/24 - 13:48, Pekka Paalanen a ?crit :
> On Tue, 26 Mar 2024 16:57:00 +0100
> Louis Chauvet <[email protected]> wrote:
>
> > Le 25/03/24 - 14:41, Pekka Paalanen a ?crit :
> > > On Wed, 13 Mar 2024 18:45:02 +0100
> > > Louis Chauvet <[email protected]> wrote:
> > >
> > > > The pre_mul_alpha_blend is dedicated to blending, so to avoid mixing
> > > > different concepts (coordinate calculation and color management), extract
> > > > the x_limit and x_dst computation outside of this helper.
> > > > It also increases the maintainability by grouping the computation related
> > > > to coordinates in the same place: the loop in `blend`.
> > > >
> > > > Signed-off-by: Louis Chauvet <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/vkms/vkms_composer.c | 40 +++++++++++++++++-------------------
> > > > 1 file changed, 19 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > index da0651a94c9b..9254086f23ff 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > @@ -24,34 +24,30 @@ static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
> > > >
> > > > /**
> > > > * pre_mul_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
> > > > + * @x_start: The start offset to avoid useless copy
> > >
> > > I'd say just:
> > >
> > > + * @x_start: The start offset
> > >
> > > It describes the parameter, and the paragraph below explains the why.
> > >
> > > It would be explaining, that x_start applies to output_buffer, but
> > > input_buffer is always read starting from 0.
> >
> > I will change it to:
> >
> > * Using @x_start and @count information, only few pixel can be blended instead of the whole line
> > * each time. @x_start is only used for the output buffer. The staging buffer is always read from
> > * the start (0..@count in stage_buffer is blended at @x_start..@x_start+@count in output_buffer).
>
> The important part is
>
> 0..@count in stage_buffer is blended at @x_start..@x_start+@count in output_buffer
>
> and everything else from that paragraph is not really adding much.
Ok, I will only keep this sentence.
> Remember to update the doc in "drm/vkms: Re-introduce line-per-line
> composition algorithm" to follow the changes.
Thanks for the reminder, I will check!
>
> > > > + * @count: The number of byte to copy
> > >
> > > You named it pixel_count, and it counts pixels, not bytes. It's not a
> > > copy but a blend into output_buffer.
> >
> > Oops, fixed in v6.
> >
> > > > *
> > > > - * 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 @x_start and @count information, only few pixel can be blended instead of the whole line
> > > > + * each time.
> > > > *
> > > > * 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.
> > > > */
> > > > -static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
> > > > - struct line_buffer *stage_buffer,
> > > > - struct line_buffer *output_buffer)
> > > > +static void pre_mul_alpha_blend(const struct line_buffer *stage_buffer,
> > > > + struct line_buffer *output_buffer, int x_start, int pixel_count)
> > > > {
> > > > - int x_dst = frame_info->dst.x1;
> > > > - struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
> > > > - struct pixel_argb_u16 *in = stage_buffer->pixels;
> > > > - int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> > > > - stage_buffer->n_pixels);
> > > > -
> > > > - 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);
> > > > + struct pixel_argb_u16 *out = &output_buffer->pixels[x_start];
> > > > + const struct pixel_argb_u16 *in = stage_buffer->pixels;
> > > > +
> > > > + for (int i = 0; i < pixel_count; i++) {
> > > > + out[i].a = (u16)0xffff;
> > > > + out[i].r = pre_mul_blend_channel(in[i].r, out[i].r, in[i].a);
> > > > + out[i].g = pre_mul_blend_channel(in[i].g, out[i].g, in[i].a);
> > > > + out[i].b = pre_mul_blend_channel(in[i].b, out[i].b, in[i].a);
> > > > }
> > > > }
> > > >
> > > > @@ -183,7 +179,7 @@ static void blend(struct vkms_writeback_job *wb,
> > > > {
> > > > struct vkms_plane_state **plane = crtc_state->active_planes;
> > > > u32 n_active_planes = crtc_state->num_active_planes;
> > > > - int y_pos;
> > > > + int y_pos, x_dst, x_limit;
> > > >
> > > > const struct pixel_argb_u16 background_color = { .a = 0xffff };
> > > >
> > > > @@ -201,14 +197,16 @@ static void blend(struct vkms_writeback_job *wb,
> > > >
> > > > /* The active planes are composed associatively in z-order. */
> > > > for (size_t i = 0; i < n_active_planes; i++) {
> > > > + x_dst = plane[i]->frame_info->dst.x1;
> > > > + x_limit = min_t(size_t, drm_rect_width(&plane[i]->frame_info->dst),
> > > > + stage_buffer->n_pixels);
> > >
> > > Are those input values to min_t() really of type size_t? Or why is
> > > size_t here?
> >
> > n_pixel is size_t, drm_rect_width is int. I will change everything to int.
> > Is there a way to ask the compiler "please don't do implicit conversion
> > and report them as warn/errors"?
>
> There probably is, you can find it in the gcc manual. However, I suspect
> you would drown in warnings for cases where the implicit conversion is
> wanted and an explicit cast is unwanted.
That true, I found it (-Wconversion), but very noisy...
Thanks,
Louis Chauvet
>
> Thanks,
> pq
>
> > > > y_pos = get_y_pos(plane[i]->frame_info, y);
> > > >
> > > > if (!check_limit(plane[i]->frame_info, y_pos))
> > > > continue;
> > > >
> > > > vkms_compose_row(stage_buffer, plane[i], y_pos);
> > > > - pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
> > > > - output_buffer);
> > > > + pre_mul_alpha_blend(stage_buffer, output_buffer, x_dst, x_limit);
> > >
> > > I thought it was a count, not a limit?
> > >
> > > "Limit" sounds to me like "end", and end - start = count.
> >
> > It is effectively a pixel count. I just took those naming from the
> > original pre_mul_alpha_blend. I will change it to pixel_count.
> >
> > Thanks,
> > Louis Chauvet
> >
> > > > }
> > > >
> > > > apply_lut(crtc_state, output_buffer);
> > > >
> > >
> > > The details aside, this is a good move.
> > >
> > >
> > > Thanks,
> > > pq
> >
> >
> >
>
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com