diff mbox series

drm/vkms: Fix RGB565 pixel conversion

Message ID 20230425153353.238844-1-mcanal@igalia.com (mailing list archive)
State New, archived
Headers show
Series drm/vkms: Fix RGB565 pixel conversion | expand

Commit Message

Maíra Canal April 25, 2023, 3:33 p.m. UTC
Perform the correct casting of the intermediate coefficients of the
RGB565 pixel conversion. Currently, the pixel conversion is using s64
for the intermediate coefficients, which is causing the IGT pixel-format
tests to fail. So, cast the operands to s32 in order to improve the
vkms' test coverage.

Tested with igt@kms_plane@pixel-format and igt@kms_plane@pixel-format-source-clamping.

Fixes: 89b03aeaef16 ("drm/vkms: fix 32bit compilation error by replacing macros")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Melissa Wen May 6, 2023, 7:43 p.m. UTC | #1
On 04/25, Maíra Canal wrote:
> Perform the correct casting of the intermediate coefficients of the
> RGB565 pixel conversion. Currently, the pixel conversion is using s64
> for the intermediate coefficients, which is causing the IGT pixel-format
> tests to fail. So, cast the operands to s32 in order to improve the
> vkms' test coverage.
> 
> Tested with igt@kms_plane@pixel-format and igt@kms_plane@pixel-format-source-clamping.
> 
> Fixes: 89b03aeaef16 ("drm/vkms: fix 32bit compilation error by replacing macros")
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/vkms/vkms_formats.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 8d948c73741e..f6aeaea81902 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -88,8 +88,8 @@ static void RGB565_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>  {
>  	u16 *pixels = (u16 *)src_pixels;
>  
> -	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> -	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
> +	s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> +	s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
>  
>  	u16 rgb_565 = le16_to_cpu(*pixels);
>  	s64 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> @@ -97,9 +97,9 @@ static void RGB565_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>  	s64 fp_b = drm_int2fixp(rgb_565 & 0x1f);
>  
>  	out_pixel->a = (u16)0xffff;
> -	out_pixel->r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> -	out_pixel->g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> -	out_pixel->b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> +	out_pixel->r = drm_fixp2int((s32)drm_fixp_mul(fp_r, fp_rb_ratio));
> +	out_pixel->g = drm_fixp2int((s32)drm_fixp_mul(fp_g, fp_g_ratio));
> +	out_pixel->b = drm_fixp2int((s32)drm_fixp_mul(fp_b, fp_rb_ratio));

Hi Maíra,

First, good catch!

Instead of all casting, wouldn't it be better to implement a rounding
operation or something like drm_fixp2int_round()?

Thanks,

Melissa

>  }
>  
>  void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y)
> @@ -208,17 +208,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);
>  
> -	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> -	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
> +	s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> +	s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
>  
>  	for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
>  		s64 fp_r = drm_int2fixp(in_pixels[x].r);
>  		s64 fp_g = drm_int2fixp(in_pixels[x].g);
>  		s64 fp_b = drm_int2fixp(in_pixels[x].b);
>  
> -		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));
> +		u16 r = drm_fixp2int((s32)drm_fixp_div(fp_r, fp_rb_ratio));
> +		u16 g = drm_fixp2int((s32)drm_fixp_div(fp_g, fp_g_ratio));
> +		u16 b = drm_fixp2int((s32)drm_fixp_div(fp_b, fp_rb_ratio));
>  
>  		*dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
>  	}
> -- 
> 2.40.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 8d948c73741e..f6aeaea81902 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -88,8 +88,8 @@  static void RGB565_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
 	u16 *pixels = (u16 *)src_pixels;
 
-	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
-	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
+	s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
+	s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
 
 	u16 rgb_565 = le16_to_cpu(*pixels);
 	s64 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
@@ -97,9 +97,9 @@  static void RGB565_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 	s64 fp_b = drm_int2fixp(rgb_565 & 0x1f);
 
 	out_pixel->a = (u16)0xffff;
-	out_pixel->r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
-	out_pixel->g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
-	out_pixel->b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
+	out_pixel->r = drm_fixp2int((s32)drm_fixp_mul(fp_r, fp_rb_ratio));
+	out_pixel->g = drm_fixp2int((s32)drm_fixp_mul(fp_g, fp_g_ratio));
+	out_pixel->b = drm_fixp2int((s32)drm_fixp_mul(fp_b, fp_rb_ratio));
 }
 
 void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y)
@@ -208,17 +208,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);
 
-	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
-	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
+	s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
+	s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
 
 	for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
 		s64 fp_r = drm_int2fixp(in_pixels[x].r);
 		s64 fp_g = drm_int2fixp(in_pixels[x].g);
 		s64 fp_b = drm_int2fixp(in_pixels[x].b);
 
-		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));
+		u16 r = drm_fixp2int((s32)drm_fixp_div(fp_r, fp_rb_ratio));
+		u16 g = drm_fixp2int((s32)drm_fixp_div(fp_g, fp_g_ratio));
+		u16 b = drm_fixp2int((s32)drm_fixp_div(fp_b, fp_rb_ratio));
 
 		*dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
 	}