1. Patchew
  2. linux
  3. [PATCH v11 00/15] drm/vkms: Reimplement line-per-line pixel conversion for plane reading
  4. View patch

[PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend

Louis Chauvet posted 15 patches 1 month, 4 weeks ago
There is a newer version of this series
[PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend
Posted by Louis Chauvet 1 month, 4 weeks ago 
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`.

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 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 931e214b225c..4d220bbb023c 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
+ * @pixel_count: The number of pixels to blend
  *
- * 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.
+ * The pixels 0..@pixel_count in stage_buffer are blended at @x_start..@x_start+@pixel_count in
+ * output_buffer.
  *
  * 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, pixel_count;
 
 	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;
+			pixel_count = min_t(int, drm_rect_width(&plane[i]->frame_info->dst),
+					    (int)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, pixel_count);
 		}
 
 		apply_lut(crtc_state, output_buffer);

-- 
2.46.2
Re: [PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend
Posted by Randy Dunlap 1 month, 3 weeks ago 
Hi--

On 9/30/24 8:31 AM, Louis Chauvet 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`.
> 
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  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 931e214b225c..4d220bbb023c 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
> + * @pixel_count: The number of pixels to blend

so is this actually pixel count + 1; or

>   *
> - * 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.
> + * The pixels 0..@pixel_count in stage_buffer are blended at @x_start..@x_start+@pixel_count in

should these ranges include a "- 1"?
Else please explain.

> + * output_buffer.
Re: [PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend
Posted by Louis Chauvet 1 month, 3 weeks ago 
On 01/10/24 - 20:54, Randy Dunlap wrote:
> Hi--
> 
> On 9/30/24 8:31 AM, Louis Chauvet 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`.
> > 
> > Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  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 931e214b225c..4d220bbb023c 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
> > + * @pixel_count: The number of pixels to blend
> 
> so is this actually pixel count + 1; or
> 
> >   *
> > - * 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.
> > + * The pixels 0..@pixel_count in stage_buffer are blended at @x_start..@x_start+@pixel_count in
> 
> should these ranges include a "- 1"?
> Else please explain.

Hi Randy,

For the next version, I will use standard mathematical notation to clarify 
the "inclusiveness" of the interval: [0;pixel_count[

Thanks,
Louis Chauvet
 
> > + * output_buffer.
>
Re: [PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend
Posted by Randy Dunlap 1 month, 3 weeks ago 
Hi Louis,

On 10/2/24 1:57 AM, Louis Chauvet wrote:
> On 01/10/24 - 20:54, Randy Dunlap wrote:
>> Hi--
>>
>> On 9/30/24 8:31 AM, Louis Chauvet 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`.
>>>
>>> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> ---
>>>  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 931e214b225c..4d220bbb023c 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
>>> + * @pixel_count: The number of pixels to blend
>>
>> so is this actually pixel count + 1; or
>>
>>>   *
>>> - * 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.
>>> + * The pixels 0..@pixel_count in stage_buffer are blended at @x_start..@x_start+@pixel_count in
>>
>> should these ranges include a "- 1"?
>> Else please explain.
> 
> Hi Randy,
> 
> For the next version, I will use standard mathematical notation to clarify 
> the "inclusiveness" of the interval: [0;pixel_count[

Hm, I can read that after a second or two.

My math classes always used:  [0,pixel_count)
for that range, and that is what most of the internet says as well.

or you could just stick with
  The pixels from 0 through @pixel_count - 1 in stage_buffer are blended at @x_start
  through @x_start through @x_start + @pixel_count - 1.

but after writing all of that, I think using range notation is better.

thanks.
Re: [PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend
Posted by Louis Chauvet 1 month, 3 weeks ago 
On 02/10/24 - 08:49, Randy Dunlap wrote:
> Hi Louis,
> 
> On 10/2/24 1:57 AM, Louis Chauvet wrote:
> > On 01/10/24 - 20:54, Randy Dunlap wrote:
> >> Hi--
> >>
> >> On 9/30/24 8:31 AM, Louis Chauvet 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`.
> >>>
> >>> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> >>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> >>> ---
> >>>  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 931e214b225c..4d220bbb023c 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
> >>> + * @pixel_count: The number of pixels to blend
> >>
> >> so is this actually pixel count + 1; or
> >>
> >>>   *
> >>> - * 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.
> >>> + * The pixels 0..@pixel_count in stage_buffer are blended at @x_start..@x_start+@pixel_count in
> >>
> >> should these ranges include a "- 1"?
> >> Else please explain.
> > 
> > Hi Randy,
> > 
> > For the next version, I will use standard mathematical notation to clarify 
> > the "inclusiveness" of the interval: [0;pixel_count[
> 
> Hm, I can read that after a second or two.
> 
> My math classes always used:  [0,pixel_count)
> for that range, and that is what most of the internet says as well.

I'm french, and we use ]a;b[ notation at school :-)

Both are valids according to ISO80000-2, but I will change it for the next 
revision.
 
> or you could just stick with
>   The pixels from 0 through @pixel_count - 1 in stage_buffer are blended at @x_start
>   through @x_start through @x_start + @pixel_count - 1.
> 
> but after writing all of that, I think using range notation is better.

I also prefer ranges, way shorter to write, and easier to understand at 
first sight. 

> thanks.
Re: [PATCH v11 06/15] drm/vkms: Avoid computing blending limits inside pre_mul_alpha_blend
Posted by Randy Dunlap 1 month, 3 weeks ago 

On 10/2/24 9:11 AM, Louis Chauvet wrote:
> On 02/10/24 - 08:49, Randy Dunlap wrote:
>> Hi Louis,
>>
>> On 10/2/24 1:57 AM, Louis Chauvet wrote:
>>> On 01/10/24 - 20:54, Randy Dunlap wrote:
>>>> Hi--
>>>>
>>>> On 9/30/24 8:31 AM, Louis Chauvet 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`.
>>>>>
>>>>> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>>> ---
>>>>>  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 931e214b225c..4d220bbb023c 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
>>>>> + * @pixel_count: The number of pixels to blend
>>>>
>>>> so is this actually pixel count + 1; or
>>>>
>>>>>   *
>>>>> - * 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.
>>>>> + * The pixels 0..@pixel_count in stage_buffer are blended at @x_start..@x_start+@pixel_count in
>>>>
>>>> should these ranges include a "- 1"?
>>>> Else please explain.
>>>
>>> Hi Randy,
>>>
>>> For the next version, I will use standard mathematical notation to clarify 
>>> the "inclusiveness" of the interval: [0;pixel_count[
>>
>> Hm, I can read that after a second or two.
>>
>> My math classes always used:  [0,pixel_count)
>> for that range, and that is what most of the internet says as well.
> 
> I'm french, and we use ]a;b[ notation at school :-)

The one reference that I found to that notation was to a French author.
> 
> Both are valids according to ISO80000-2, but I will change it for the next 
> revision.
>  
>> or you could just stick with
>>   The pixels from 0 through @pixel_count - 1 in stage_buffer are blended at @x_start
>>   through @x_start through @x_start + @pixel_count - 1.
>>
>> but after writing all of that, I think using range notation is better.
> 
> I also prefer ranges, way shorter to write, and easier to understand at 
> first sight. 

Yes, thanks.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.