* [PATCH] drm/vkms: fix 32bit compilation error by replacing macros
@ 2022-09-09 11:41 Melissa Wen
2022-09-09 13:32 ` Alex Deucher
2022-09-10 2:08 ` Igor Matheus Andrade Torrente
0 siblings, 2 replies; 5+ messages in thread
From: Melissa Wen @ 2022-09-09 11:41 UTC (permalink / raw)
To: rodrigosiqueiramelo, melissa.srw, airlied, daniel, igormtorrente
Cc: Sudip Mukherjee, Melissa Wen, kernel test robot, dri-devel,
linux-kernel
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 <sudipm.mukherjee@gmail.com>
Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/vkms: fix 32bit compilation error by replacing macros
2022-09-09 11:41 [PATCH] drm/vkms: fix 32bit compilation error by replacing macros Melissa Wen
@ 2022-09-09 13:32 ` Alex Deucher
2022-09-10 2:08 ` Igor Matheus Andrade Torrente
1 sibling, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2022-09-09 13:32 UTC (permalink / raw)
To: Melissa Wen
Cc: rodrigosiqueiramelo, melissa.srw, airlied, daniel, igormtorrente,
linux-kernel, Sudip Mukherjee, dri-devel, kernel test robot
On Fri, Sep 9, 2022 at 7:42 AM Melissa Wen <mwen@igalia.com> 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 <sudipm.mukherjee@gmail.com>
> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/vkms: fix 32bit compilation error by replacing macros
2022-09-09 11:41 [PATCH] drm/vkms: fix 32bit compilation error by replacing macros Melissa Wen
2022-09-09 13:32 ` Alex Deucher
@ 2022-09-10 2:08 ` Igor Matheus Andrade Torrente
2022-09-10 19:10 ` Melissa Wen
1 sibling, 1 reply; 5+ messages in thread
From: Igor Matheus Andrade Torrente @ 2022-09-10 2:08 UTC (permalink / raw)
To: Melissa Wen, rodrigosiqueiramelo, melissa.srw, airlied, daniel
Cc: Sudip Mukherjee, dri-devel, linux-kernel
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 <sudipm.mukherjee@gmail.com>
> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
> 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);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/vkms: fix 32bit compilation error by replacing macros
2022-09-10 2:08 ` Igor Matheus Andrade Torrente
@ 2022-09-10 19:10 ` Melissa Wen
2022-09-11 14:44 ` Igor Matheus Andrade Torrente
0 siblings, 1 reply; 5+ messages in thread
From: Melissa Wen @ 2022-09-10 19:10 UTC (permalink / raw)
To: Igor Matheus Andrade Torrente
Cc: rodrigosiqueiramelo, melissa.srw, airlied, daniel,
Sudip Mukherjee, dri-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5893 bytes --]
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 <sudipm.mukherjee@gmail.com>
> > Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> > 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);
> > }
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/vkms: fix 32bit compilation error by replacing macros
2022-09-10 19:10 ` Melissa Wen
@ 2022-09-11 14:44 ` Igor Matheus Andrade Torrente
0 siblings, 0 replies; 5+ messages in thread
From: Igor Matheus Andrade Torrente @ 2022-09-11 14:44 UTC (permalink / raw)
To: Melissa Wen
Cc: rodrigosiqueiramelo, melissa.srw, airlied, daniel,
Sudip Mukherjee, dri-devel, linux-kernel
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 <sudipm.mukherjee@gmail.com>
>>> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>> ---
>>> 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);
>>> }
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-11 14:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-09 11:41 [PATCH] drm/vkms: fix 32bit compilation error by replacing macros Melissa Wen
2022-09-09 13:32 ` Alex Deucher
2022-09-10 2:08 ` Igor Matheus Andrade Torrente
2022-09-10 19:10 ` Melissa Wen
2022-09-11 14:44 ` Igor Matheus Andrade Torrente
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox