On Wed, 28 Feb 2024 22:52:09 -0300
Arthur Grillo <[email protected]> wrote:

> On 27/02/24 17:01, Arthur Grillo wrote:
> >
> >
> > On 27/02/24 12:02, Louis Chauvet wrote:
> >> Hi Pekka,
> >>
> >> For all the comment related to the conversion part, maybe Arthur have an
> >> opinion on it, I took his patch as a "black box" (I did not want to
> >> break (and debug) it).
> >>
> >> Le 26/02/24 - 14:19, Pekka Paalanen a écrit :
> >>> On Fri, 23 Feb 2024 12:37:26 +0100
> >>> Louis Chauvet <[email protected]> wrote:
> >>>
> >>>> From: Arthur Grillo <[email protected]>
> >>>>
> >>>> Add support to the YUV formats bellow:
> >>>>
> >>>> - NV12
> >>>> - NV16
> >>>> - NV24
> >>>> - NV21
> >>>> - NV61
> >>>> - NV42
> >>>> - YUV420
> >>>> - YUV422
> >>>> - YUV444
> >>>> - YVU420
> >>>> - YVU422
> >>>> - YVU444
> >>>>
> >>>> The conversion matrices of each encoding and range were obtained by
> >>>> rounding the values of the original conversion matrices multiplied by
> >>>> 2^8. This is done to avoid the use of fixed point operations.
> >>>>
> >>>> Signed-off-by: Arthur Grillo <[email protected]>
> >>>> [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t
> >>>> callbacks for yuv formats]
> >>>> Signed-off-by: Louis Chauvet <[email protected]>
> >>>> ---
> >>>> drivers/gpu/drm/vkms/vkms_composer.c | 2 +-
> >>>> drivers/gpu/drm/vkms/vkms_drv.h | 6 +-
> >>>> drivers/gpu/drm/vkms/vkms_formats.c | 289 +++++++++++++++++++++++++++++++++--
> >>>> drivers/gpu/drm/vkms/vkms_formats.h | 4 +
> >>>> drivers/gpu/drm/vkms/vkms_plane.c | 14 +-
> >>>> 5 files changed, 295 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> >>>> index e555bf9c1aee..54fc5161d565 100644
> >>>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> >>>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> >>>> @@ -312,7 +312,7 @@ static void blend(struct vkms_writeback_job *wb,
> >>>> * buffer [1]
> >>>> */
> >>>> current_plane->pixel_read_line(
> >>>> - current_plane->frame_info,
> >>>> + current_plane,
> >>>> x_start,
> >>>> y_start,
> >>>> direction,
> >>>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >>>> index ccc5be009f15..a4f6456cb971 100644
> >>>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> >>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >>>> @@ -75,6 +75,8 @@ enum pixel_read_direction {
> >>>> READ_RIGHT
> >>>> };
> >>>>
> >>>> +struct vkms_plane_state;
> >>>> +
> >>>> /**
> >>>> <<<<<<< HEAD
> >>>> * typedef pixel_read_line_t - These functions are used to read a pixel line in the source frame,
> >>>> @@ -87,8 +89,8 @@ enum pixel_read_direction {
> >>>> * @out_pixel: Pointer where to write the pixel value. Pixels will be written between x_start and
> >>>> * x_end.
> >>>> */
> >>>> -typedef void (*pixel_read_line_t)(struct vkms_frame_info *frame_info, int x_start, int y_start, enum
> >>>> - pixel_read_direction direction, int count, struct pixel_argb_u16 out_pixel[]);
> >>>> +typedef void (*pixel_read_line_t)(struct vkms_plane_state *frame_info, int x_start, int y_start,
> >>>> + enum pixel_read_direction direction, int count, struct pixel_argb_u16 out_pixel[]);
> >>>
> >>> This is the second or third time in this one series changing this type.
> >>> Could you not do the change once, in its own patch if possible?
> >>
> >> Sorry, this is not a change here, but a wrong formatting (missed when
> >> rebasing).
> >>
> >> Do you think that it make sense to re-order my patches and put this
> >> typedef at the end? This way it is never updated.

I'm not sure, I haven't checked how it would change your patches. The
intermediate changes might get a lot uglier?

Just try to fold changes so that you don't need to change something
twice over the series unless there is a good reason to. "How hard would
it be to review this?" is my measure stick.

> >>
> >>>>
> >>>> /**
> >>>> * vkms_plane_state - Driver specific plane state
> >>>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> >>>> index 46daea6d3ee9..515c80866a58 100644
> >>>> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> >>>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> >>>> @@ -33,7 +33,8 @@ static size_t packed_pixels_offset(const struct vkms_frame_info *frame_info, int
> >>>> */
> >>>> return fb->offsets[plane_index] +
> >>>> (y / drm_format_info_block_width(format, plane_index)) * fb->pitches[plane_index] +
> >>>> - (x / drm_format_info_block_height(format, plane_index)) * format->char_per_block[plane_index];
> >>>> + (x / drm_format_info_block_height(format, plane_index)) *
> >>>> + format->char_per_block[plane_index];
> >>>
> >>> Shouldn't this be in the patch that added this code in the first place?
> >>
> >> Same as above, a wrong formatting, I will remove this change and keep
> >> everything on one line (even if it's more than 100 chars, it is easier to
> >> read).

Personally I agree that readability is more important than strict line
length limits. I'm not sure how the kernel rolls there.

> >>
> >>>> }
> >>>>
> >>>> /**
> >>>> @@ -84,6 +85,32 @@ static int get_step_1x1(struct drm_framebuffer *fb, enum pixel_read_direction di
> >>>> }
> >>>> }
> >>>>
> >>>> +/**
> >>>> + * get_subsampling() - Get the subsampling value on a specific direction
> >>>
> >>> subsampling divisor
> >>
> >> Thanks for this precision.
> >>
> >>>> + */
> >>>> +static int get_subsampling(const struct drm_format_info *format,
> >>>> + enum pixel_read_direction direction)
> >>>> +{
> >>>> + if (direction == READ_LEFT || direction == READ_RIGHT)
> >>>> + return format->hsub;
> >>>> + else if (direction == READ_DOWN || direction == READ_UP)
> >>>> + return format->vsub;
> >>>> + return 1;
> >>>
> >>> In this and the below function, personally I'd prefer switch-case, with
> >>> a cannot-happen-scream after the switch, so the compiler can warn about
> >>> unhandled enum values.
> >>
> >> As for the previous patch, I did not know about this compiler feature,
> >> thanks!
> >>
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * get_subsampling_offset() - Get the subsampling offset to use when incrementing the pixel counter
> >>>> + */
> >>>> +static int get_subsampling_offset(const struct drm_format_info *format,
> >>>> + enum pixel_read_direction direction, int x_start, int y_start)
> >>>
> >>> 'start' values as "increments" for a pixel counter? Is something
> >>> misnamed here?
> >>>
> >>> Is it an increment or an offset?
> >>
> >> I don't really know how to name the function. I'm open to suggestions
> >> x_start and y_start are really the coordinate of the starting reading point.

I looks like it's an offset, so "offset" and "start" are good words.
Then the only misleading piece is the doc:

"Get the subsampling offset to use when incrementing the pixel counter"

This sounds like the offset is used when incrementing a counter, that
is, counter is increment by offset each time. That's my problem with
this.

Fix just the doc, and it's good, I think.

> >>
> >> To explain what it does:
> >>
> >> When using subsampling, you have to read the next pixel of planes[1..4]
> >> not at the same "speed" as plane[0]. But I can't only rely on
> >> "read_pixel_count % subsampling == 0", because it means that the pixel
> >> incrementation on planes[1..4] may not be aligned with the buffer (if
> >> hsub=2 and the start pixel is 1, I need to increment planes[1..4] only
> >> for x=2,4,6... not 1,3,5...).
> >>
> >> A way to ensure this is to add an "offset" to count, which ensure that the
> >> count % subsampling == 0 on the correct pixel.

Yes, I think I did get that feeling from the code eventually somehow,
but it wouldn't hurt to explain it in the comment.

"An offset for keeping the chroma siting consistent regardless of
x_start and y_start" maybe?

> >>
> >> I made an error, the switch case must be (as count is always counting up,
> >> for "inverted" reading direction a negative number ensure that
> >> %subsampling == 0 on the correct pixel):
> >>
> >> switch (direction) {
> >> case READ_UP:
> >> return -y_start;
> >> case READ_DOWN:
> >> return y_start;
> >> case READ_LEFT:
> >> return -x_start;
> >> case READ_RIGHT:
> >> return x_start;
> >> }

Yes, the inverted reading directions are different indeed. I did not
think through if this works also for sub-sampling divisors > 2 which I
don't think are ever used.

Does IGT find this mistake? If not, maybe IGT should be extended.

> >>
> >>>> +{
> >>>> + if (direction == READ_RIGHT || direction == READ_LEFT)
> >>>> + return x_start;
> >>>> + else if (direction == READ_DOWN || direction == READ_UP)
> >>>> + return y_start;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>
> >> [...]
> >>
> >>>> +static void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16, const struct pixel_yuv_u8 *yuv_u8,
> >>>> + enum drm_color_encoding encoding, enum drm_color_range range)
> >>>> +{
> >>>> + static const s16 bt601_full[3][3] = {
> >>>> + { 256, 0, 359 },
> >>>> + { 256, -88, -183 },
> >>>> + { 256, 454, 0 },
> >>>> + };
> >>
> >> [...]
> >>
> >>>> +
> >>>> + u8 r = 0;
> >>>> + u8 g = 0;
> >>>> + u8 b = 0;
> >>>> + bool full = range == DRM_COLOR_YCBCR_FULL_RANGE;
> >>>> + unsigned int y_offset = full ? 0 : 16;
> >>>> +
> >>>> + switch (encoding) {
> >>>> + case DRM_COLOR_YCBCR_BT601:
> >>>> + ycbcr2rgb(full ? bt601_full : bt601,
> >>>
> >>> Doing all these conditional again pixel by pixel is probably
> >>> inefficient. Just like with the line reading functions, you could pick
> >>> the matrix in advance.
> >>
> >> I don't think the performance impact is huge (it's only a pair of if), but
> >> yes, it's an easy optimization.
> >>
> >> I will create a conversion_matrix structure:
> >>
> >> struct conversion_matrix {
> >> s16 matrix[3][3];
> >> u16 y_offset;
> >> }

When defining such a struct type, it would be good to document the
matrix layout (which one is row, which one is column), and what the s16
mean (fixed point?).

Try to not mix signed and unsigned types, too. The C implicit type
promotion rules can be surprising. Just make everything signed while
computing, and convert to/from unsigned only for storage.

> >>
> >> I will create a `get_conversion_matrix_to_argb_u16` function to get this
> >> structure from a format+encoding+range.
> >>
> >> I will also add a field `conversion_matrix` in struct vkms_plane_state to
> >> get this matrix only once per plane setup.

Alright. Let's see how that works.

> >>
> >>
> >>>> + yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
> >>>> + break;
> >>>> + case DRM_COLOR_YCBCR_BT709:
> >>>> + ycbcr2rgb(full ? rec709_full : rec709,
> >>>> + yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
> >>>> + break;
> >>>> + case DRM_COLOR_YCBCR_BT2020:
> >>>> + ycbcr2rgb(full ? bt2020_full : bt2020,
> >>>> + yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
> >>>> + break;
> >>>> + default:
> >>>> + pr_warn_once("Not supported color encoding\n");
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + argb_u16->r = r * 257;
> >>>> + argb_u16->g = g * 257;
> >>>> + argb_u16->b = b * 257;
> >>>
> >>> I wonder. Using 8-bit fixed point precision seems quite coarse for
> >>> 8-bit pixel formats, and it's going to be insufficient for higher bit
> >>> depths. Was supporting e.g. 10-bit YUV considered? There is even
> >>> deeper, too, like DRM_FORMAT_P016.
> >>
> >> It's a good point, as I explained above, I took the conversion part as a
> >> "black box" to avoid breaking (and debugging) stuff. I think it's easy to
> >> switch to s32 bits matrix with 16.16 bits (or anything with more than 16 bits in
> >> the float part).
> >>
> >> Maybe Arthur have an opinion on this?
> >
> > Yeah, I too don't see why not we could do that. The 8-bit precision was
> > sufficient for those formats, but as well noted by Pekka this could be a
> > problem for higher bit depths. I just need to make my terrible python
> > script spit those values XD.
>
> Finally, I got it working with 32-bit precision.
>
> I basically threw all my untrusted python code away, and started using
> the colour python framework suggested by Sebastian[1]. After knowing the
> right values (and staring at numbers for hours), I found that with a
> little bit of rounding, the conversion works.
>
> Also, while at it, I changed the name rec709 to bt709 to follow the
> pattern and added "_full" to the full ranges matrices.
>
> While using the library, I noticed that the red component is wrong on
> the color red in one test case.
>
> [1]: https://lore.kernel.org/all/20240115150600.GC160656@toolbox/

That all sounds good. I wish the kernel code contained comments
explaining how exactly you computed those matrices with python/colour.
If the python snippets are not too long, including them verbatim as
code comments would be really nice for both reviewers and posterity.

The same for the VKMS unit tests, too, how you got the expected result
values.

>
> Best Regards,
> ~Arthur Grillo
>
> ---
>
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> index f66584549827..4cee3c2d8d84 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> @@ -59,7 +59,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> {"white", {0xff, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
> {"gray", {0x80, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
> {"black", {0x00, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
> - {"red", {0x35, 0x63, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
> + {"red", {0x36, 0x63, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
> {"green", {0xb6, 0x1e, 0x0c}, {0x0000, 0x0000, 0xffff, 0x0000}},
> {"blue", {0x12, 0xff, 0x74}, {0x0000, 0x0000, 0x0000, 0xffff}},
> },
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index e06bbd7c0a67..043f23dbf80d 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -121,10 +121,12 @@ static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel
> out_pixel->b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
> }
>
> -static void ycbcr2rgb(const s16 m[3][3], u8 y, u8 cb, u8 cr, u8 y_offset, u8 *r, u8 *g, u8 *b)
> +#define BIT_DEPTH 32
> +
> +static void ycbcr2rgb(const s64 m[3][3], u8 y, u8 cb, u8 cr, u8 y_offset, u8 *r, u8 *g, u8 *b)
> {
> - s32 y_16, cb_16, cr_16;
> - s32 r_16, g_16, b_16;
> + s64 y_16, cb_16, cr_16;
> + s64 r_16, g_16, b_16;
>
> y_16 = y - y_offset;
> cb_16 = cb - 128;
> @@ -134,9 +136,18 @@ static void ycbcr2rgb(const s16 m[3][3], u8 y, u8 cb, u8 cr, u8 y_offset, u8 *r,
> g_16 = m[1][0] * y_16 + m[1][1] * cb_16 + m[1][2] * cr_16;
> b_16 = m[2][0] * y_16 + m[2][1] * cb_16 + m[2][2] * cr_16;
>
> - *r = clamp(r_16, 0, 0xffff) >> 8;
> - *g = clamp(g_16, 0, 0xffff) >> 8;
> - *b = clamp(b_16, 0, 0xffff) >> 8;
> + // rounding the values
> + r_16 = r_16 + (1LL << (BIT_DEPTH - 4));
> + g_16 = g_16 + (1LL << (BIT_DEPTH - 4));
> + b_16 = b_16 + (1LL << (BIT_DEPTH - 4));
> +
> + r_16 = clamp(r_16, 0, (1LL << (BIT_DEPTH + 8)) - 1);
> + g_16 = clamp(g_16, 0, (1LL << (BIT_DEPTH + 8)) - 1);
> + b_16 = clamp(b_16, 0, (1LL << (BIT_DEPTH + 8)) - 1);

Where do the BIT_DEPTH - 4 and BIT_DEPTH + 8 come from?

> +
> + *r = r_16 >> BIT_DEPTH;
> + *g = g_16 >> BIT_DEPTH;
> + *b = b_16 >> BIT_DEPTH;
> }

..

> >
> >> Just to be sure, the DRM subsystem don't have such matrix somewhere? It
> >> can be nice to avoid duplicating them.
> >
> > As to my knowledge it does not exist on DRM, I think those are normally
> > on the hardware itself (*please* correct me if I'm wrong).

I couldn't find a matrix type either on a quick glance, but there is
drm_fixed.h for a couple different fixed point formats, it seems. FWIW.
drm_fixed.h didn't feel very appealing for this here.

> >
> > But, v4l2 has a similar table on
> > drivers/media/common/v4l2-tpg/v4l2-tpg-core.c (Actually, I started my
> > code based on this), unfortunately it's only 8-bit too.

Thanks,
pq

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-02-29 10:21:41

On Wed, 6 Mar 2024 18:29:53 +0100
Louis Chauvet <[email protected]> wrote:

> [...]
>
> > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > > > > > > @@ -9,6 +9,17 @@
> > > > > > >
> > > > > > > #include "vkms_formats.h"
> > > > > > >
> > > > > > > +/**
> > > > > > > + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y
> > > > > > > + * in the first plane
> > > > > > > + *
> > > > > > > + * @frame_info: Buffer metadata
> > > > > > > + * @x: The x coordinate of the wanted pixel in the buffer
> > > > > > > + * @y: The y coordinate of the wanted pixel in the buffer
> > > > > > > + *
> > > > > > > + * The caller must be aware that this offset is not always a pointer to a pixel. If individual
> > > > > > > + * pixel values are needed, they have to be extracted from the resulting block.
> > > > > >
> > > > > > Just wondering how the caller will be able to extract the right pixel
> > > > > > from the block without re-using the knowledge already used in this
> > > > > > function. I'd also expect the function to round down x,y to be
> > > > > > divisible by block dimensions, but that's not visible in this email.
> > > > > > Then the caller needs the remainder from the round-down, too?
> > > > >
> > > > > You are right, the current implementation is only working when block_h ==
> > > > > block_w == 1. I think I wrote the documentation for PATCHv2 5/9, but when
> > > > > backporting this comment for PATCHv2 3/9 I forgot to update it.
> > > > > The new comment will be:
> > > > >
> > > > > * pixels_offset() - Get the offset of a given pixel data at coordinate
> > > > > * x/y in the first plane
> > > > > [...]
> > > > > * The caller must ensure that the framebuffer associated with this
> > > > > * request uses a pixel format where block_h == block_w == 1.
> > > > > * If this requirement is not fulfilled, the resulting offset can be
> > > > > * completly wrong.
> > > >
> > > > Hi Louis,
> > >
> > > Hi Pekka,
> > >
> > > > if there is no plan for how non-1x1 blocks would work yet, then I think
> > > > the above wording is fine. In my mind, the below wording would
> > > > encourage callers to seek out and try arbitrary tricks to make things
> > > > work for non-1x1 without rewriting the function to actually work.
> > > >
> > > > I believe something would need to change in the function signature to
> > > > make it properly usable for non-1x1 blocks, but I too cannot suggest
> > > > anything off-hand.
> > >
> > > I already made the change to support non-1x1 blocks in Patchv2 5/9
> > > (I will extract this modification in "drm/vkms: Update pixels accessor to
> > > support packed and multi-plane formats"), this function is now able
> > > to extract the pointer to the start of a block. But as stated in the
> > > comment, the caller must manually extract the correct pixel values (if the
> > > format is 2x2, the pointer will point to the first byte of this block, the
> > > caller must do some computation to access the bottom-right value).
> >
> > Patchv2 5/9 is not enough.
> >
> > "Manually extract the correct pixels" is the thing I have a problem
> > with here. The caller should not need to re-do any semantic
> > calculations this function already did. Most likely this function
> > should return the remainders from the x,y coordinate division, so that
> > the caller can extract the right pixels from the block, or something
> > else equivalent.
> >
> > That same semantic division should not be done in two different places.
> > It is too easy for someone later to come and change one site while
> > missing the other.
>
> I did not notice this, and I agree, thanks for this feedback. For the v5 I
> will change it and update the function signature to:
>
> static void packed_pixels_offset(const struct vkms_frame_info *frame_info, int x, int y,
> size_t plane_index, size_t *offset, size_t *rem_x, size_t *rem_y)
>
> where rem_x and rem_y are those reminder.

Ok, that's a start.

Why size_t? It's unsigned. You'll probably be mixing signed and
unsigned variables in computations again.

> > I have a hard time finding in "[PATCH v2 6/9] drm/vkms: Add YUV
> > support" how you actually handle blocks bigger than 1x1. I see
> > get_subsampling() which returns format->{hsub,vsub}, and I see
> > get_subsampling_offset() which combined with remainder-division gates U
> > and V plane pixel pointer increments.
> >
> > However, I do not see you ever using
> > drm_format_info_block_{width,height}() anywhere else. That makes me
> > think you have no code to actually handle non-1x1 block formats, which
> > means that you cannot get the function signature of
> > packed_pixels_offset() right in this series either. It would be better
> > to not even pretend the function works for non-1x1 blocks until you
> > have code handling at least one such format.
> >
> > All of the YUV formats that patch 6 adds support for use 1x1 blocks all
> > all their planes.
>
> Yes, none of the supported format have block_h != block_w != 1, so there
> is no need to drm_format_info_block*() helpers.
>
> I wrote the code for DRM_FORMAT_R*. They are packed, with block_w != 1. I
> will add this patch in the next revision. I also wrote the IGT test for
> DRM_FORMAT_R1 [1].

Excellent!

> Everything will be in the v5 (I will send it when you have the
> time to review the v4).

I'm too busy this week, I think. Maybe next.

Why should I review v4 when I already know you will be changing things
again? I'd probably flag the same things I've already said.

Thanks,
pq

> For information, I also have a series ready for adding more RGB variants
> (I introduced a macro to make it easier and avoid copy/pasting the same
> loop). I don't send them yet, because I realy want this series merged
> first. I also have the work for the writeback "line-by-line" algorithm
> ready (I just need to rebase it, but it will be fast).
>
> [1]: https://lore.kernel.org/igt-dev/[email protected]
>
> Kind regards,
> Louis Chauvet
>
> [...]
>

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-03-05 10:10:26

by Pekka Paalanen

[permalink] [raw]

Subject: Re: [PATCH v2 5/9] drm/vkms: Re-introduce line-per-line composition algorithm

On Mon, 4 Mar 2024 16:28:33 +0100
Louis Chauvet <[email protected]> wrote:

> Le 29/02/24 - 12:21, Pekka Paalanen a écrit :
> > On Tue, 27 Feb 2024 16:02:09 +0100
> > Louis Chauvet <[email protected]> wrote:
> >
> > > [...]
> > >
> > > > > -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(
> > > > > + 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];
> > > > > + struct pixel_argb_u16 *in = &stage_buffer->pixels[x_start];
> > > >
> > > > Input buffers and pointers should be const.
> > >
> > > They will be const in v4.
> > >
> > > > > +
> > > > > + 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);
> > > > > }
> > > > > }
> > > >
> > > > Somehow the hunk above does not feel like it is part of "re-introduce
> > > > line-per-line composition algorithm". This function was already running
> > > > line-by-line. Would it be easy enough to collect this and directly
> > > > related changes into a separate patch?
> > >
> > > It is not directly related to the reintroduction of line-by-line
> > > algorithm, but in the simplification and maintenability effort, I
> > > changed a bit the function to avoid having multiple place computing the
> > > x_start/pixel_count values. I don't see an interrest to extract it, it
> > > will be just a translation of the few lines into the calling place.
> >
> > It does make review more difficult, because it makes the patch bigger
> > and is not explained in the commit message. It is a surprise to a
> > reviewer, who then needs to think what this means and does it belong
> > here.
> >
> > If you explain it in the commit message and note it in the commit
> > summary line, I think it would become fairly obvious that this patch is
> > doing two things rather than one.
> >
> > Therefore, *if* it is easy to extract as a separate patch, then it
> > would be nice to do so. However, if doing so would require you to write
> > a bunch of temporary code that the next patch would just rewrite again,
> > then doing so would be counter-productive.
> >
> > Patch split is about finding a good trade-off to make things easy for
> > reviewers:
> >
> > - Smaller patches are better as long as they are self-standing and
> > understandable in isolation, and of course do not regress anything.
> >
> > - Rewriting the same thing multiple times in the same series is extra
> > work for a reviewer and therefore best avoided.
> >
> > - The simpler the semantic change, the bigger a patch can be and still
> > be easy to review.
> >
> > And all the patch writing rules specific to the kernel project that I
> > don't know about.
>
> I will extract it in "drm/vkms: Avoid computing blending limits inside the
> blend function". It's not very relevant by itself, but it make the main
> patch easier to read.

Thank you.

> > > [...]
> > >
> > > > > +/**
> > > > > + * direction_for_rotation() - Helper to get the correct reading direction for a specific rotation
> > > > > + *
> > > > > + * @rotation: rotation to analyze
> > > >
> > > > This is KMS plane rotation property, right?
> > > >
> > > > So the KMS plane has been rotated by this, and what we want to find is
> > > > the read direction on the attached FB so that reading returns pixels in
> > > > the CRTC line/scanout order, right?
> > > >
> > > > Maybe extend the doc to explain that.
> > >
> > > Is it better?
> > >
> > > * direction_for_rotation() - Get the correct reading direction for a given rotation
> > > *
> > > * This function will use the @rotation parameter to compute the correct reading direction to read
> > > * a line from the source buffer.
> > > * For example, if the buffer is reflected on X axis, the pixel must be read from right to left.
> > > * @rotation: Rotation to analyze. It correspond the the field @frame_info.rotation.
> >
> > I think it is important to define what determines the correct result.
> > In this case, we want the reading to produce pixels in the CRTC scanout
> > line order, I believe. If you don't say "CRTC", the reader does not
> > know what "the correct reading direction" should match to.
>
> Is this a better explanation?
>
> * This function will use the @rotation setting of a source plane to compute the reading
> * direction in this plane which correspond to a left to right writing in the CRTC.
> * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> * to be written from left to right on the CRTC.

Perfect!

>
> > > > > + */
> > > > > +enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > > > > +{
> > > > > + if (rotation & DRM_MODE_ROTATE_0) {
> > > > > + if (rotation & DRM_MODE_REFLECT_X)
> > > > > + return READ_LEFT;
> > > > > + else
> > > > > + return READ_RIGHT;
> > > > > + } else if (rotation & DRM_MODE_ROTATE_90) {
> > > > > + if (rotation & DRM_MODE_REFLECT_Y)
> > > > > + return READ_UP;
> > > > > + else
> > > > > + return READ_DOWN;
> > > > > + } else if (rotation & DRM_MODE_ROTATE_180) {
> > > > > + if (rotation & DRM_MODE_REFLECT_X)
> > > > > + return READ_RIGHT;
> > > > > + else
> > > > > + return READ_LEFT;
> > > > > + } else if (rotation & DRM_MODE_ROTATE_270) {
> > > > > + if (rotation & DRM_MODE_REFLECT_Y)
> > > > > + return READ_DOWN;
> > > > > + else
> > > > > + return READ_UP;
> > > > > + }
> > > > > + return READ_RIGHT;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * blend - blend the pixels from all planes and compute crc
> > > > > * @wb: The writeback frame buffer metadata
> > > > > @@ -183,11 +187,11 @@ 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;
> > > > >
> > > > > const struct pixel_argb_u16 background_color = { .a = 0xffff };
> > > > >
> > > > > size_t crtc_y_limit = crtc_state->base.crtc->mode.vdisplay;
> > > > > + size_t crtc_x_limit = crtc_state->base.crtc->mode.hdisplay;
> > > >
> > > > Wonder why these were size_t, causing needs to cast below...
> > >
> > > For crtc_x_limit I just copied the crtc_y_limit. I will change both to u16
> > > (the type of h/vdisplay).
> >
> > Don't go unsigned, that can cause unexpected results when mixed in
> > computations with signed variables.
>
> I will replace them with int.
>
> > Oh, the cast was probably not about size but signedness. Indeed, size_t
> > is unsigned.
> >
> > I don't see a reason to use a 16-bit size either, it just exposes the
> > computations to under/overflows that would then be needed to check for.
> > s32 should be as fast as any, and perhaps enough bits to never
> > under/overflow in these computations, but please verify that.
>
> I just suggested u16 because it's the type of vdisplay/hdisplay. It was
> not for performance reason.

Right. It's not uncommon store a value in a storage efficient type that
may also disallow illegal values, and then use a different type while
actually computing with it in order to not provoke too obscure C
language rules most people never heard of, to avoid over/underflows, or
to just avoid undefined behaviour.

..

> > > > > +static int get_step_1x1(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > > > > + int plane_index)
> > > > > {
> > > > > - int x_src = frame_info->src.x1 >> 16;
> > > > > - int y_src = y - frame_info->rotated.y1 + (frame_info->src.y1 >> 16);
> > > > > -
> > > > > - return packed_pixels_addr(frame_info, x_src, y_src);
> > > > > + switch (direction) {
> > > > > + default:
> > > > > + DRM_ERROR("Invalid direction for pixel reading: %d\n", direction);
> > > > > + return 0;
> > > >
> > > > What I'd do here is move the default: section outside of the switch
> > > > completely. Then the compiler can warn if any enum value is not handled
> > > > here. Since every case in the switch is a return statement, falling out
> > > > of the switch block is the default case.
> > >
> > > Hoo, I did not know that gcc can warn when using enums, I will definitly
> > > do it for the v4.
> > >
> > > > Maybe the enum variable containing an illegal value could be handled
> > > > more harshly so that callers could rely on this function always
> > > > returning a good value?
> > > >
> > > > Just like passing in fb=NULL is handled by the kernel as an OOPS.
> > >
> > > I don't think it's a good idea to OOPS inside a driver.
> >
> > Everyone already do that. Most functions that do not expect to be called
> > with NULL never check the arguments for NULL. They just OOPS on
> > dereference if someone passes in NULL. And for a good reason: adding
> > all those checks is both code churn and it casts doubt: "maybe it is
> > legal and expected to call this function with NULL sometimes, what good
> > does that do?".
>
> I agree that adding something like
>
> if (direction_is_valid) pr_err("Invalid direction")
>
> is useless, but as I already have the switch, it cost nothing to warn if
> something gone wrong. I will just replace this simple DRM_ERROR with a
> WARN_ONCE to be more verbose about "it is a bug".

Sounds good to me, and I hope kernel maintainers would agree.

> > > An error here is
> > > maybe dangerous, but is not fatal to the kernel. Maybe you know how to do
> > > a "local" OOPS to break only this driver and not the whole kernel?
> >
> > I don't know what the best practices are in the kernel.
> >
> > > For the v4 I will keep a DRM_ERROR and return 0.
> >
> > Does that require the caller to check for 0? Could the 0 cause
> > something else to end up in an endless loop? If it does return 0, how
> > should a caller handle this case that "cannot" ever happen? Why have
> > code for something that cannot happen?
>
> I have to return something, otherwise the compiler will complain about it.
>
> To avoid for future developers surprise, I added this information in the
> comment. This way the user don't have to read the code to understand how
> much he can rely on this value.
>
> If the caller can trust his direction, he don't have to worry about this.
> If he can't trust his direction, he know that the returned value can be
> zero, and thus can't be used for a loop variant.

There should not be "untrusted" values to begin with at this point,
anything that comes from outside of the kernel should have already been
sanitised. This is about kernel bugs though. Bugs cannot be predicted,
nor can anyone guarantee to write bug-free code. Hence, the direction
value is always "somewhat untrusted". We're being paranoid about bugs
that might happen and trying to ensure the kernel can limp along
regardless, while also trying to minimise the amount of code that
"cannot" ever be reached.

> The zero is also nice because it does not interfere with the normal
> behavior of this function. If the returned value is not zero, it's the
> correct step to use from one pixel to an other.

If you expect the caller needing to check for the "cannot happen" case,
returning a unique error value is fine. If you expect the caller to
never need to think of the "cannot happen" case, you should return a
value that is "safe", if such value exists. "Safe" here means using it
will not result in grave bugs like bad memory access, but it also won't
produce expected results unless by accident.

This getting perhaps a bit too philosophical, so don't mind about this
too much if it feels strange.

> > Of course it's a trade-off between correctness and limping along
> > injured, and the kernel tends to strongly lean toward the latter for the
> > obvious reasons.
> >
> > > > > + case READ_RIGHT:
> > > > > + return fb->format->char_per_block[plane_index];
> > > > > + case READ_LEFT:
> > > > > + return -fb->format->char_per_block[plane_index];
> > > > > + case READ_DOWN:
> > > > > + return (int)fb->pitches[plane_index];
> > > > > + case READ_UP:
> > > > > + return -(int)fb->pitches[plane_index];
> > > > > + }
> > > > > }
> > > > >
> > > > > -static int get_x_position(const struct vkms_frame_info *frame_info, int limit, int x)
> > > > > -{
> > > > > - if (frame_info->rotation & (DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_270))
> > > > > - return limit - x - 1;
> > > > > - return x;
> > > > > -}
> > > > >
> > > > > /*
> > > > > - * The following functions take pixel data from the buffer and convert them to the format
> > > > > + * The following functions take pixel data (a, r, g, b, pixel, ..), convert them to the format
> > > > > * ARGB16161616 in out_pixel.
> > > > > *
> > > > > - * They are used in the `vkms_compose_row` function to handle multiple formats.
> > > > > + * They are used in the `read_line`s functions to avoid duplicate work for some pixel formats.
> > > > > */
> > > > >
> > > > > -static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
> > > > > +static void ARGB8888_to_argb_u16(struct pixel_argb_u16 *out_pixel, int a, int r, int g, int b)
> > > >
> > > > The function name ARGB8888_to_argb_u16() is confusing. It's not taking
> > > > in ARGB8888 pixels but separate a,r,g,b ints. The only assumption it
> > > > needs from the pixel format is the 8888 part.
> > >
> > > I don't realy know how to name it. What I like with ARGB8888 is that it's
> > > clear that the values are 8 bits and in argb format.
> >
> > I could even propose
> >
> > static struct pixel_argb_u16
> > argb_u16_from_u8888(int a, int r, int g, int b)
> >
> > perhaps. Yes, returning a struct by value. I think it would fit, and
> > these are supposed to get fully inlined anyway, too.
> >
> > c.f argb_u16_from_u2101010().
>
> I don't find this method, but I got and like the idea, I will change the
> callback to this in the v4.

I mean, there is no support for 10-bpc formats in VKMS yet IIRC, but
there should be one day, so thinking about how that would fit in the
naming scheme is nice.

> > Not a big deal though, I think I'm getting a little bit too involved to
> > see what would be the most intuitively understandable naming scheme for
> > someone not familiar with the code.
> >
> > > Do you think that `argb_u8_to_argb_u16`, with a new structure
> > > pixel_argb_u8 will be better? (like PATCH 6/9 with pixel_yuv_u8).
> > >
> > > If so, I will introduce the argb_u8 structure in an other commit.
> >
> > How would you handle 10-bpc formats? Is there a need for
> > proliferation of bit-depth-specific struct types?
>
> No, I don't think it's good to multiply things. I will patch Arthur's
> patches to avoid the pixel_yuv_u8 structure.
>
> > > [...]
> > >
> > > > > + * The following functions are read_line function for each pixel format supported by VKMS.
> > > > > *
> > > > > - * This function composes a single row of a plane. It gets the source pixels
> > > > > - * through the y coordinate (see get_packed_src_addr()) and goes linearly
> > > > > - * through the source pixel, reading the pixels and converting it to
> > > > > - * ARGB16161616 (see the pixel_read() callback). For rotate-90 and rotate-270,
> > > > > - * the source pixels are not traversed linearly. The source pixels are queried
> > > > > - * on each iteration in order to traverse the pixels vertically.
> > > > > + * They read a line starting at the point @x_start,@y_start following the @direction. The result
> > > > > + * is stored in @out_pixel and in the format ARGB16161616.
> > > > > + *
> > > > > + * Those function are very similar, but it is required for performance reason. In the past, some
> > > > > + * experiment were done, and with a generic loop the performance are very reduced [1].
> > > > > + *
> > > > > + * [1]: https://lore.kernel.org/dri-devel/[email protected]/
> > > > > */
> > > > > -void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y)
> > > > > +
> > > > > +static void ARGB8888_read_line(struct vkms_frame_info *frame_info, int x_start, int y_start,
> > > > > + enum pixel_read_direction direction, int count,
> > > > > + struct pixel_argb_u16 out_pixel[])
> > > > > +{
> > > > > + u8 *src_pixels = packed_pixels_addr(frame_info, x_start, y_start, 0);
> > > > > +
> > > > > + int step = get_step_1x1(frame_info->fb, direction, 0);
> > > > > +
> > > > > + while (count) {
> > > > > + u8 *px = (u8 *)src_pixels;
> > > > > +
> > > > > + ARGB8888_to_argb_u16(out_pixel, px[3], px[2], px[1], px[0]);
> > > > > + out_pixel += 1;
> > > > > + src_pixels += step;
> > > > > + count--;
> > > >
> > > > btw. you could eliminate decrementing 'count' if you computed end
> > > > address and used while (out_pixel < end).
> > >
> > > Yes, you are right, but after thinking about it, neither out_pixel < end
> > > and while (count) are conveying "this loop will copy `count` pixels. I
> > > think a for-loop here is more understandable. There is no ambiguity in the
> > > number of pixels written and less error-prone. I will replace
> > > while (count)
> > > by
> > > for(int i = 0; i < count; i++)
> >
> > I agree that a for-loop is the most obvious way of saying it, but I
> > also think while (out_pixel < end) is very close too, and so is while (count).
> > None of those would make me think twice.
> >
> > However, I'm thinking of performance here. After all, this is the
> > hottest code path there is in VKMS. Is the compiler smart enough to
> > eliminate count-- or i to reduce the number of CPU cycles?
>
> You are proably right, I will change it to out_pixel < end.

Don't trust my word without benchmarking it. ;-)

Thanks,
pq

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature