Replace vkms_formats macros for fixed-point operations with functions
from drm/drm_fixed.h to do the same job and fix 32-bit compilation
errors.
Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
Tested-by: Sudip Mukherjee <[email protected]>
Reported-by: Sudip Mukherjee <[email protected]>
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/gpu/drm/vkms/vkms_formats.c | 53 +++++++++++------------------
1 file changed, 19 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 300abb4d1dfe..ddcd3cfeeaac 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -1,27 +1,12 @@
// SPDX-License-Identifier: GPL-2.0+
-#include <drm/drm_rect.h>
+#include <linux/kernel.h>
#include <linux/minmax.h>
+#include <drm/drm_rect.h>
+#include <drm/drm_fixed.h>
#include "vkms_formats.h"
-/* The following macros help doing fixed point arithmetic. */
-/*
- * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
- * parts respectively.
- * | 0000 0000 0000 0000 0.000 0000 0000 0000 |
- * 31 0
- */
-#define SHIFT 15
-
-#define INT_TO_FIXED(a) ((a) << SHIFT)
-#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
-#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
-/* This macro converts a fixed point number to int, and round half up it */
-#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
-#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
-#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
-
static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
{
return frame_info->offset + (y * frame_info->pitch)
@@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
stage_buffer->n_pixels);
- s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
- s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+ s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
+ s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
for (size_t x = 0; x < x_limit; x++, src_pixels++) {
u16 rgb_565 = le16_to_cpu(*src_pixels);
- s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
- s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
- s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
+ s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
+ s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
+ s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);
out_pixels[x].a = (u16)0xffff;
- out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, fp_rb_ratio));
- out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, fp_g_ratio));
- out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, fp_rb_ratio));
+ out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
+ out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
+ out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
}
}
@@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
src_buffer->n_pixels);
- s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
- s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+ s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
+ s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
- s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
- s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
- s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
+ s32 fp_r = drm_int2fixp(in_pixels[x].r);
+ s32 fp_g = drm_int2fixp(in_pixels[x].g);
+ s32 fp_b = drm_int2fixp(in_pixels[x].b);
- u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));
- u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
- u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
+ u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
+ u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
+ u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
*dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
}
--
2.35.1
On Fri, Sep 9, 2022 at 7:42 AM Melissa Wen <[email protected]> wrote:
>
> Replace vkms_formats macros for fixed-point operations with functions
> from drm/drm_fixed.h to do the same job and fix 32-bit compilation
> errors.
>
> Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
> Tested-by: Sudip Mukherjee <[email protected]>
> Reported-by: Sudip Mukherjee <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Melissa Wen <[email protected]>
Reviewed-by: Alex Deucher <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_formats.c | 53 +++++++++++------------------
> 1 file changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 300abb4d1dfe..ddcd3cfeeaac 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -1,27 +1,12 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> -#include <drm/drm_rect.h>
> +#include <linux/kernel.h>
> #include <linux/minmax.h>
> +#include <drm/drm_rect.h>
> +#include <drm/drm_fixed.h>
>
> #include "vkms_formats.h"
>
> -/* The following macros help doing fixed point arithmetic. */
> -/*
> - * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
> - * parts respectively.
> - * | 0000 0000 0000 0000 0.000 0000 0000 0000 |
> - * 31 0
> - */
> -#define SHIFT 15
> -
> -#define INT_TO_FIXED(a) ((a) << SHIFT)
> -#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
> -#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
> -/* This macro converts a fixed point number to int, and round half up it */
> -#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
> -
> static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> {
> return frame_info->offset + (y * frame_info->pitch)
> @@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> stage_buffer->n_pixels);
>
> - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
> - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
> + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
> + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>
> for (size_t x = 0; x < x_limit; x++, src_pixels++) {
> u16 rgb_565 = le16_to_cpu(*src_pixels);
> - s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
> - s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
> - s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
> + s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> + s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
> + s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);
>
> out_pixels[x].a = (u16)0xffff;
> - out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, fp_rb_ratio));
> - out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, fp_g_ratio));
> - out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, fp_rb_ratio));
> + out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> + out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> + out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> }
> }
>
> @@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> src_buffer->n_pixels);
>
> - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
> - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
> + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
> + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>
> for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
> - s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
> - s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
> - s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
> + s32 fp_r = drm_int2fixp(in_pixels[x].r);
> + s32 fp_g = drm_int2fixp(in_pixels[x].g);
> + s32 fp_b = drm_int2fixp(in_pixels[x].b);
>
> - u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));
> - u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
> - u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
> + u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
> + u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
> + u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
>
> *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
> }
> --
> 2.35.1
>
Hi Mellisa,
Thanks for the patch fixing my mistakes.
On 9/9/22 08:41, Melissa Wen wrote:
> Replace vkms_formats macros for fixed-point operations with functions
> from drm/drm_fixed.h to do the same job and fix 32-bit compilation
> errors.
>
> Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
> Tested-by: Sudip Mukherjee <[email protected]>
> Reported-by: Sudip Mukherjee <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Melissa Wen <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_formats.c | 53 +++++++++++------------------
> 1 file changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 300abb4d1dfe..ddcd3cfeeaac 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -1,27 +1,12 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> -#include <drm/drm_rect.h>
> +#include <linux/kernel.h>
> #include <linux/minmax.h>
> +#include <drm/drm_rect.h>
> +#include <drm/drm_fixed.h>
>
> #include "vkms_formats.h"
>
> -/* The following macros help doing fixed point arithmetic. */
> -/*
> - * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
> - * parts respectively.
> - * | 0000 0000 0000 0000 0.000 0000 0000 0000 |
> - * 31 0
> - */
> -#define SHIFT 15
> -
> -#define INT_TO_FIXED(a) ((a) << SHIFT)
> -#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
> -#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
> -/* This macro converts a fixed point number to int, and round half up it */
> -#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
> -
> static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> {
> return frame_info->offset + (y * frame_info->pitch)
> @@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> stage_buffer->n_pixels);
>
> - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
> - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
> + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
> + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
I think you need to add `drm_int2fixp` to 31 and 63.
>
> for (size_t x = 0; x < x_limit; x++, src_pixels++) {
> u16 rgb_565 = le16_to_cpu(*src_pixels);
> - s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
> - s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
> - s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
> + s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> + s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
> + s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);
And we are cast implicitly from 64 bits int to 32 bits which is
implementation-defined AFAIK. So, probably we should be using `s64` for
all of these variables.
I tested the patch. And I'm seeing some differences in the intermediate
results. From my testing, these changes solve those differences.
Another thing that may have an impact on the final output is the lack of
rounding in drm_fixed.h. This can potentially produce the wrong result.
Thanks,
---
Igor Torrente
>
> out_pixels[x].a = (u16)0xffff;
> - out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, fp_rb_ratio));
> - out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, fp_g_ratio));
> - out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, fp_rb_ratio));
> + out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> + out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> + out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> }
> }
>
> @@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> src_buffer->n_pixels);
>
> - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
> - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
> + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
> + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>
> for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
> - s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
> - s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
> - s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
> + s32 fp_r = drm_int2fixp(in_pixels[x].r);
> + s32 fp_g = drm_int2fixp(in_pixels[x].g);
> + s32 fp_b = drm_int2fixp(in_pixels[x].b);
>
> - u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));
> - u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
> - u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
> + u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
> + u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
> + u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
>
> *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
> }
On 09/09, Igor Matheus Andrade Torrente wrote:
> Hi Mellisa,
>
> Thanks for the patch fixing my mistakes.
>
> On 9/9/22 08:41, Melissa Wen wrote:
> > Replace vkms_formats macros for fixed-point operations with functions
> > from drm/drm_fixed.h to do the same job and fix 32-bit compilation
> > errors.
> >
> > Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
> > Tested-by: Sudip Mukherjee <[email protected]>
> > Reported-by: Sudip Mukherjee <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Melissa Wen <[email protected]>
> > ---
> > drivers/gpu/drm/vkms/vkms_formats.c | 53 +++++++++++------------------
> > 1 file changed, 19 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index 300abb4d1dfe..ddcd3cfeeaac 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -1,27 +1,12 @@
> > // SPDX-License-Identifier: GPL-2.0+
> > -#include <drm/drm_rect.h>
> > +#include <linux/kernel.h>
> > #include <linux/minmax.h>
> > +#include <drm/drm_rect.h>
> > +#include <drm/drm_fixed.h>
> > #include "vkms_formats.h"
> > -/* The following macros help doing fixed point arithmetic. */
> > -/*
> > - * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
> > - * parts respectively.
> > - * | 0000 0000 0000 0000 0.000 0000 0000 0000 |
> > - * 31 0
> > - */
> > -#define SHIFT 15
> > -
> > -#define INT_TO_FIXED(a) ((a) << SHIFT)
> > -#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
> > -#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
> > -/* This macro converts a fixed point number to int, and round half up it */
> > -#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
> > -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
> > -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
> > -
> > static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> > {
> > return frame_info->offset + (y * frame_info->pitch)
> > @@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
> > int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> > stage_buffer->n_pixels);
> > - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
> > - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
> > + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
> > + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>
> I think you need to add `drm_int2fixp` to 31 and 63.
>
> > for (size_t x = 0; x < x_limit; x++, src_pixels++) {
> > u16 rgb_565 = le16_to_cpu(*src_pixels);
> > - s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
> > - s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
> > - s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
> > + s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> > + s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
> > + s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);
>
> And we are cast implicitly from 64 bits int to 32 bits which is
> implementation-defined AFAIK. So, probably we should be using `s64` for all
> of these variables.
>
> I tested the patch. And I'm seeing some differences in the intermediate
> results. From my testing, these changes solve those differences.
Hi Igor,
Thanks for checking the calc results and all inputs provided. I just
sent a second version, can you take a look? I replicated your
suggestions for RGB565_to_argb_u16() in argb_u16_to_RGB565() and
double-checked for i386 and arm. Let me know what you think.
>
> Another thing that may have an impact on the final output is the lack of
> rounding in drm_fixed.h. This can potentially produce the wrong result.
Yeah, I see... I can include a comment about the rounding issue for
further improvements, or do you plan to work on it?
Thanks,
Melissa
>
> Thanks,
> ---
> Igor Torrente
>
> > out_pixels[x].a = (u16)0xffff;
> > - out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, fp_rb_ratio));
> > - out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, fp_g_ratio));
> > - out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, fp_rb_ratio));
> > + out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> > + out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> > + out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> > }
> > }
> > @@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
> > int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> > src_buffer->n_pixels);
> > - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
> > - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
> > + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
> > + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
> > for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
> > - s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
> > - s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
> > - s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
> > + s32 fp_r = drm_int2fixp(in_pixels[x].r);
> > + s32 fp_g = drm_int2fixp(in_pixels[x].g);
> > + s32 fp_b = drm_int2fixp(in_pixels[x].b);
> > - u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));
> > - u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
> > - u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
> > + u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
> > + u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
> > + u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
> > *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
> > }
>
On 9/10/22 16:10, Melissa Wen wrote:
> On 09/09, Igor Matheus Andrade Torrente wrote:
>> Hi Mellisa,
>>
>> Thanks for the patch fixing my mistakes.
>>
>> On 9/9/22 08:41, Melissa Wen wrote:
>>> Replace vkms_formats macros for fixed-point operations with functions
>>> from drm/drm_fixed.h to do the same job and fix 32-bit compilation
>>> errors.
>>>
>>> Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
>>> Tested-by: Sudip Mukherjee <[email protected]>
>>> Reported-by: Sudip Mukherjee <[email protected]>
>>> Reported-by: kernel test robot <[email protected]>
>>> Signed-off-by: Melissa Wen <[email protected]>
>>> ---
>>> drivers/gpu/drm/vkms/vkms_formats.c | 53 +++++++++++------------------
>>> 1 file changed, 19 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
>>> index 300abb4d1dfe..ddcd3cfeeaac 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_formats.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
>>> @@ -1,27 +1,12 @@
>>> // SPDX-License-Identifier: GPL-2.0+
>>> -#include <drm/drm_rect.h>
>>> +#include <linux/kernel.h>
>>> #include <linux/minmax.h>
>>> +#include <drm/drm_rect.h>
>>> +#include <drm/drm_fixed.h>
>>> #include "vkms_formats.h"
>>> -/* The following macros help doing fixed point arithmetic. */
>>> -/*
>>> - * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
>>> - * parts respectively.
>>> - * | 0000 0000 0000 0000 0.000 0000 0000 0000 |
>>> - * 31 0
>>> - */
>>> -#define SHIFT 15
>>> -
>>> -#define INT_TO_FIXED(a) ((a) << SHIFT)
>>> -#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
>>> -#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
>>> -/* This macro converts a fixed point number to int, and round half up it */
>>> -#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
>>> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
>>> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
>>> -
>>> static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
>>> {
>>> return frame_info->offset + (y * frame_info->pitch)
>>> @@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
>>> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>> stage_buffer->n_pixels);
>>> - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
>>> - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
>>> + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
>>> + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>>
>> I think you need to add `drm_int2fixp` to 31 and 63.
>>
>>> for (size_t x = 0; x < x_limit; x++, src_pixels++) {
>>> u16 rgb_565 = le16_to_cpu(*src_pixels);
>>> - s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
>>> - s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
>>> - s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
>>> + s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
>>> + s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
>>> + s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);
>>
>> And we are cast implicitly from 64 bits int to 32 bits which is
>> implementation-defined AFAIK. So, probably we should be using `s64` for all
>> of these variables.
>>
>> I tested the patch. And I'm seeing some differences in the intermediate
>> results. From my testing, these changes solve those differences.
>
> Hi Igor,
>
> Thanks for checking the calc results and all inputs provided. I just
> sent a second version, can you take a look? I replicated your
> suggestions for RGB565_to_argb_u16() in argb_u16_to_RGB565() and
> double-checked for i386 and arm. Let me know what yexiuou think.
I tested it here and everything seem to be working :).
>
>>
>> Another thing that may have an impact on the final output is the lack of
>> rounding in drm_fixed.h. This can potentially produce the wrong result.
>
> Yeah, I see... I can include a comment about the rounding issue for
> further improvements, or do you plan to work on it?
A comment would be good. Maybe add to the VKMS TODO list?
I'm busy with other stuffs, and can't work on this any time soon. But
It's pretty simple thing to implement.
>
> Thanks,
>
> Melissa
>>
>> Thanks,
>> ---
>> Igor Torrente
>>
>>> out_pixels[x].a = (u16)0xffff;
>>> - out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, fp_rb_ratio));
>>> - out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, fp_g_ratio));
>>> - out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, fp_rb_ratio));
>>> + out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
>>> + out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
>>> + out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
>>> }
>>> }
>>> @@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
>>> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>> src_buffer->n_pixels);
>>> - s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
>>> - s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
>>> + s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
>>> + s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>>> for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
>>> - s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
>>> - s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
>>> - s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
>>> + s32 fp_r = drm_int2fixp(in_pixels[x].r);
>>> + s32 fp_g = drm_int2fixp(in_pixels[x].g);
>>> + s32 fp_b = drm_int2fixp(in_pixels[x].b);
>>> - u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));
>>> - u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
>>> - u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
>>> + u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
>>> + u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
>>> + u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
>>> *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
>>> }
>>