Re: [PATCH v2 6/9] drm/vkms: Add YUV support
Le 06/03/24 - 17:09, Arthur Grillo a écrit : > > > On 04/03/24 13:51, Arthur Grillo wrote: > > > > > > On 04/03/24 12:48, Louis Chauvet wrote: > [...] > >>> > Regarding the YUV part, I don't feel confortable adressing Pekka's > comments, would you mind doing it? > >>> > >>> I'm already doing that, how do you want me to send those changes? I reply > >>> to > >>> your series, like a did before? > >> > >> Yes, simply reply to my series, so I can rebase everything on the > >> line-by-line work. > > > > OK, I will do that. > > Hi, > > I know that I said that, but it would be very difficult to that with my > b4 workflow. So, I sent a separate series based on the v4: > > https://lore.kernel.org/all/20240306-louis-vkms-conv-v1-0-5bfe7d129...@riseup.net/ > > I hope that it does not difficult things for you. Thanks for this work! I completly understood, and a "real" patch is even better as I can fetch them through patchwork. The v5 is (almost, see my comment) ready, but I want to wait for Pekka's comments/replies on the v4 before sending it. Kind regards, Louis Chauvet > Best Regards, > ~Arthur Grillo > > > > > Best Regards, > > ~Arthur Grillo > > > >> Kind regards, > >> Louis Chauvet > >> > >>> Best Regards, > >>> ~Arthur Grillo > >>> > > Kind regards, > Louis Chauvet > > [...] > > >> -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On 04/03/24 13:51, Arthur Grillo wrote: > > > On 04/03/24 12:48, Louis Chauvet wrote: [...] >>> Regarding the YUV part, I don't feel confortable adressing Pekka's comments, would you mind doing it? >>> >>> I'm already doing that, how do you want me to send those changes? I reply to >>> your series, like a did before? >> >> Yes, simply reply to my series, so I can rebase everything on the >> line-by-line work. > > OK, I will do that. Hi, I know that I said that, but it would be very difficult to that with my b4 workflow. So, I sent a separate series based on the v4: https://lore.kernel.org/all/20240306-louis-vkms-conv-v1-0-5bfe7d129...@riseup.net/ I hope that it does not difficult things for you. Best Regards, ~Arthur Grillo > > Best Regards, > ~Arthur Grillo > >> Kind regards, >> Louis Chauvet >> >>> Best Regards, >>> ~Arthur Grillo >>> Kind regards, Louis Chauvet [...] >>
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On 04/03/24 12:48, Louis Chauvet wrote: > [...] > >>> @arthur, I will submit a v4 with this: >>> - matrix selection in plane_atomic_update (so it's selected only once) >>> - s64 numbers for matrix >>> - avoiding multiple loop implementation by switching matrix columns >> >> This looks good to me. >> >>> >>> Regarding the YUV part, I don't feel confortable adressing Pekka's >>> comments, would you mind doing it? >> >> I'm already doing that, how do you want me to send those changes? I reply to >> your series, like a did before? > > Yes, simply reply to my series, so I can rebase everything on the > line-by-line work. OK, I will do that. Best Regards, ~Arthur Grillo > Kind regards, > Louis Chauvet > >> Best Regards, >> ~Arthur Grillo >> >>> >>> Kind regards, >>> Louis Chauvet >>> >>> [...] >>> >
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
[...] > > @arthur, I will submit a v4 with this: > > - matrix selection in plane_atomic_update (so it's selected only once) > > - s64 numbers for matrix > > - avoiding multiple loop implementation by switching matrix columns > > This looks good to me. > > > > > Regarding the YUV part, I don't feel confortable adressing Pekka's > > comments, would you mind doing it? > > I'm already doing that, how do you want me to send those changes? I reply to > your series, like a did before? Yes, simply reply to my series, so I can rebase everything on the line-by-line work. Kind regards, Louis Chauvet > Best Regards, > ~Arthur Grillo > > > > > Kind regards, > > Louis Chauvet > > > > [...] > > -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On 04/03/24 12:28, Louis Chauvet wrote: > Le 29/02/24 - 14:12, Pekka Paalanen a écrit : >> On Wed, 28 Feb 2024 22:52:09 -0300 >> Arthur Grillo 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 wrote: >> >>> From: Arthur Grillo >>> >>> 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 >>> [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t >>> callbacks for yuv formats] >>> Signed-off-by: Louis Chauvet >>> --- >>> 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. > > It will not be uglier, it was just the order I did things. I first cleaned > the code and created this typedef (PATCHv2 4/9), and then rewrote the > composition, for which I had to change the typedef. > > I also wanted to make my series easy to understand and make clear what is > my "main contribution" and what are "quality stuff, not related to my > contribution": > - Prepare things (document existing state, format, typedef) > - Big change (and update related doc, typedef) > - Rebase some other stuff on my big change (YUV) > > So yes, some parts are changed twice in preparation step and the "big > change". > >>
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
Le 29/02/24 - 14:12, Pekka Paalanen a écrit : > On Wed, 28 Feb 2024 22:52:09 -0300 > Arthur Grillo 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 wrote: > > >>> > > From: Arthur Grillo > > > > 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 > > [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t > > callbacks for yuv formats] > > Signed-off-by: Louis Chauvet > > --- > > 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. It will not be uglier, it was just the order I did things. I first cleaned the code and created this typedef (PATCHv2 4/9), and then rewrote the composition, for which I had to change the typedef. I also wanted to make my series easy to understand and make clear what is my "main contribution" and what are "quality stuff, not related to my contribution": - Prepare things (document existing state, format, typedef) - Big change (and update related doc, typedef) - Rebase some other stuff on my big change (YUV) So yes, some parts are changed twice in preparation step and the "big change
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On 01/03/24 08:53, Pekka Paalanen wrote: > On Thu, 29 Feb 2024 14:57:06 -0300 > Arthur Grillo wrote: > >> On 29/02/24 09:12, Pekka Paalanen wrote: >>> On Wed, 28 Feb 2024 22:52:09 -0300 >>> Arthur Grillo 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 wrote: >>> From: Arthur Grillo 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 [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t callbacks for yuv formats] Signed-off-by: Louis Chauvet --- 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/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}, {0x, 0x, 0x, 0x}}, {"gray", {0x80, 0x80, 0x80}, {0x, 0x8000, 0x8000, 0x8000}}, {"black", {0x00, 0x80, 0x80}, {0x, 0x, 0x, 0x}}, - {"red", {0x35, 0x63, 0xff}, {0x, 0x, 0x, 0x}}, + {"red", {0x36, 0x63, 0xff}, {0x, 0x, 0x, 0x}}, {"green", {0xb6, 0x1e, 0x0c}, {0x, 0x, 0x, 0x}}, {"blue", {0x12, 0xff, 0x74}, {0x, 0x, 0x, 0x}}, }, 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, 0x) >> 8; - *g = clamp(g_16, 0, 0x) >> 8; - *b = clamp(b_16, 0, 0x) >> 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? >> >> Basically, the numbers are in this form in hex: >> >> 0xss >> >> In the end, we only want the 's' bits. >> >> The matrix multiplication is not giving us perfect results, making some >> of KUnit test not pass, This is because the values end up a little bit >> off. KUnit expects 0xfe, but this functions is returning 0xfd. >> >> I noticed that before shifting the values to get the 's' byt
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On Thu, 29 Feb 2024 14:57:06 -0300 Arthur Grillo wrote: > On 29/02/24 09:12, Pekka Paalanen wrote: > > On Wed, 28 Feb 2024 22:52:09 -0300 > > Arthur Grillo 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 wrote: > > > >> From: Arthur Grillo > >> > >> 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 > >> [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t > >> callbacks for yuv formats] > >> Signed-off-by: Louis Chauvet > >> --- > >> 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/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}, {0x, 0x, 0x, > >> 0x}}, > >>{"gray", {0x80, 0x80, 0x80}, {0x, 0x8000, 0x8000, > >> 0x8000}}, > >>{"black", {0x00, 0x80, 0x80}, {0x, 0x, 0x, > >> 0x}}, > >> - {"red", {0x35, 0x63, 0xff}, {0x, 0x, 0x, > >> 0x}}, > >> + {"red", {0x36, 0x63, 0xff}, {0x, 0x, 0x, > >> 0x}}, > >>{"green", {0xb6, 0x1e, 0x0c}, {0x, 0x, 0x, > >> 0x}}, > >>{"blue", {0x12, 0xff, 0x74}, {0x, 0x, 0x, > >> 0x}}, > >>}, > >> 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, 0x) >> 8; > >> - *g = clamp(g_16, 0, 0x) >> 8; > >> - *b = clamp(b_16, 0, 0x) >> 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? > > Basically, the numbers are in this form in hex: > > 0xss > > In the end, we only want the 's' bits. > > The matrix multiplication is not giving us perfect results, making some > of KUnit test not pass, This is because the values end up a little bit > off. KUnit expects 0xfe, but this functions is returning 0xfd. > > I noticed that before shifting the values to get the 's' bytes the > values were a lot close to what is expected, something
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On 29/02/24 09:12, Pekka Paalanen wrote: > On Wed, 28 Feb 2024 22:52:09 -0300 > Arthur Grillo 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 wrote: > >> From: Arthur Grillo >> >> 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 >> [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t >> callbacks for yuv formats] >> Signed-off-by: Louis Chauvet >> --- >> 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]; >> +
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On Wed, 28 Feb 2024 22:52:09 -0300 Arthur Grillo 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 wrote: > >>> > From: Arthur Grillo > > 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 > [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t > callbacks for yuv formats] > Signed-off-by: Louis Chauvet > --- > 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))
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
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 wrote: >>> From: Arthur Grillo 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 [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t callbacks for yuv formats] Signed-off-by: Louis Chauvet --- 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. >> /** * 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). >> } /** @@ -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, >>
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
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 wrote: >> >>> From: Arthur Grillo >>> >>> 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 >>> [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t >>> callbacks for yuv formats] >>> Signed-off-by: Louis Chauvet >>> --- >>> 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. > >>> >>> /** >>> * 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). > >>> } >>> >>> /** >>> @@ -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 form
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
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 wrote: > > > From: Arthur Grillo > > > > 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 > > [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t > > callbacks for yuv formats] > > Signed-off-by: Louis Chauvet > > --- > > 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. > > > > /** > > * 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). > > } > > > > /** > > @@ -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) > > +
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
On Fri, 23 Feb 2024 12:37:26 +0100 Louis Chauvet wrote: > From: Arthur Grillo > > 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 > [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t > callbacks for yuv formats] > Signed-off-by: Louis Chauvet > --- > 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? > > /** > * 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? > } > > /** > @@ -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 > + */ > +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. > +} > + > +/** > + * 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? > +{ > + if (direction == READ_RIGHT || direction == READ_LEFT) > + return x_start; > + else if (direction == READ_DOWN || direction == READ_UP) > +
Re: [PATCH v2 6/9] drm/vkms: Add YUV support
Am 23.02.24 um 12:37 schrieb Louis Chauvet: From: Arthur Grillo 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 [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t callbacks for yuv formats] Signed-off-by: Louis Chauvet --- 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 Noise * 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[]); /** * 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]; } /** @@ -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 + */ +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; +} + +/** + * 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) +{ + if (direction == READ_RIGHT || direction == READ_LEFT) + return x_start; + else if (direction == READ_DOWN || direction == READ_UP) + return y_start; + return 0; +} + /* * The following functions take pixel data (a, r, g, b, pixel, ...), convert them to the format @@ -130,6 +157,87 @@ static void RGB565_to_argb_u16(struct pixel_argb_u16 *out_pixel, const u16 *pixe 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) +{ + s32 y_16, cb_16, cr_16; + s32 r_16, g_16, b_16; + + y_16 = y - y_offset; + cb_16 = cb - 128; + cr_16 = cr - 128; + + r_16 = m[0][0] * y_16 + m[0][1] * cb_16 + m[0][2] * cr_16; + g_16 = m[1][0] * y_16 + m[1][