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
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'
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,
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,
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
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 +
