| Message ID | 20240930-yuv-v11-6-4b1a26bcfc96@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Reimplement line-per-line pixel conversion for plane reading | expand |
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.
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. >
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.
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.
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.
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);