Re: [PATCH v4 9/9] drm: vkms: Add support to the RGB565 format

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8215 bytes --]

On Fri, 21 Jan 2022 18:38:31 -0300
Igor Torrente <igormtorrente@gmail.com> wrote:

> Adds this common format to vkms.
> 
> This commit also adds new helper macros to deal with fixed-point
> arithmetic.
> 
> It was done to improve the precision of the conversion to ARGB16161616
> since the "conversion ratio" is not an integer.
> 
> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
> ---
> V3: Adapt the handlers to the new format introduced in patch 7 V3.
> ---
>  drivers/gpu/drm/vkms/vkms_formats.c   | 74 +++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_formats.h   |  6 +++
>  drivers/gpu/drm/vkms/vkms_plane.c     |  6 ++-
>  drivers/gpu/drm/vkms/vkms_writeback.c |  3 +-
>  4 files changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 661da39d1276..dc612882dd8c 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -11,6 +11,8 @@ format_transform_func get_fmt_transform_function(u32 format)
>  		return &get_ARGB16161616;
>  	else if (format == DRM_FORMAT_XRGB16161616)
>  		return &XRGB16161616_to_ARGB16161616;
> +	else if (format == DRM_FORMAT_RGB565)
> +		return &RGB565_to_ARGB16161616;
>  	else
>  		return &XRGB8888_to_ARGB16161616;
>  }
> @@ -23,6 +25,8 @@ format_transform_func get_wb_fmt_transform_function(u32 format)
>  		return &convert_to_ARGB16161616;
>  	else if (format == DRM_FORMAT_XRGB16161616)
>  		return &convert_to_XRGB16161616;
> +	else if (format == DRM_FORMAT_RGB565)
> +		return &convert_to_RGB565;
>  	else
>  		return &convert_to_XRGB8888;
>  }
> @@ -33,6 +37,26 @@ static int pixel_offset(struct vkms_frame_info *frame_info, int x, int y)
>  				  + (x * frame_info->cpp);
>  }
>  
> +/*
> + * FP stands for _Fixed Point_ and **not** _Float Point_

Is it common in the kernel that FP always means fixed-point?

If there is any doubt about that, I'd suggest using "fixed" and "float"
to avoid misunderstandings.

And, since you are not supposed to use floats in the kernel unless you
really really must and you do all the preparations necessary (which you
don't here), maybe replace the "float" with a fraction.

In other words, write a macro that takes (65535, 31) as arguments
instead of a float, when converting to fixed-point. Then you don't have
to use those strange decimal constants either.

> + * LF stands for Long Float (i.e. double)
> + * The following macros help doing fixed point arithmetic.
> + */
> +/*
> + * With FP 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 FP_SCALE 15
> +
> +#define LF_TO_FP(a) ((a) * (u64)(1 << FP_SCALE))
> +#define INT_TO_FP(a) ((a) << FP_SCALE)
> +#define FP_MUL(a, b) ((s32)(((s64)(a) * (b)) >> FP_SCALE))
> +#define FP_DIV(a, b) ((s32)(((s64)(a) << FP_SCALE) / (b)))
> +/* This macro converts a fixed point number to int, and round half up it */
> +#define FP_TO_INT_ROUND_UP(a) (((a) + (1 << (FP_SCALE - 1))) >> FP_SCALE)
> +
>  /*
>   * packed_pixels_addr - Get the pointer to pixel of a given pair of coordinates
>   *
> @@ -125,6 +149,33 @@ void XRGB16161616_to_ARGB16161616(struct vkms_frame_info *frame_info, int y,
>  	}
>  }
>  
> +void RGB565_to_ARGB16161616(struct vkms_frame_info *frame_info, int y,
> +			    struct line_buffer *stage_buffer)
> +{
> +	u16 *src_pixels = get_packed_src_addr(frame_info, y);
> +	int x, x_limit = drm_rect_width(&frame_info->dst);
> +
> +	for (x = 0; x < x_limit; x++, src_pixels++) {
> +		u16 rgb_565 = le16_to_cpu(*src_pixels);
> +		int fp_r = INT_TO_FP((rgb_565 >> 11) & 0x1f);
> +		int fp_g = INT_TO_FP((rgb_565 >> 5) & 0x3f);
> +		int fp_b = INT_TO_FP(rgb_565 & 0x1f);
> +
> +		/*
> +		 * The magic constants is the "conversion ratio" and is calculated
> +		 * dividing 65535(2^16 - 1) by 31(2^5 -1) and 63(2^6 - 1)
> +		 * respectively.
> +		 */
> +		int fp_rb_ratio = LF_TO_FP(2114.032258065);
> +		int fp_g_ratio = LF_TO_FP(1040.238095238);
> +
> +		stage_buffer[x].a = (u16)0xffff;
> +		stage_buffer[x].r = FP_TO_INT_ROUND_UP(FP_MUL(fp_r, fp_rb_ratio));
> +		stage_buffer[x].g = FP_TO_INT_ROUND_UP(FP_MUL(fp_g, fp_g_ratio));
> +		stage_buffer[x].b = FP_TO_INT_ROUND_UP(FP_MUL(fp_b, fp_rb_ratio));
> +	}
> +}
> +
>  
>  /*
>   * The following  functions take an line of ARGB16161616 pixels from the
> @@ -203,3 +254,26 @@ void convert_to_XRGB16161616(struct vkms_frame_info *frame_info, int y,
>  		dst_pixels[0] = src_buffer[x].b;
>  	}
>  }
> +
> +void convert_to_RGB565(struct vkms_frame_info *frame_info, int y,
> +		       struct line_buffer *src_buffer)
> +{
> +	int x, x_dst = frame_info->dst.x1;
> +	u16 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
> +	int x_limit = drm_rect_width(&frame_info->dst);
> +
> +	for (x = 0; x < x_limit; x++, dst_pixels++) {
> +		int fp_r = INT_TO_FP(src_buffer[x].r);
> +		int fp_g = INT_TO_FP(src_buffer[x].g);
> +		int fp_b = INT_TO_FP(src_buffer[x].b);
> +
> +		int fp_rb_ratio = LF_TO_FP(2114.032258065);
> +		int fp_g_ratio = LF_TO_FP(1040.238095238);

Are there any guarantees that this will not result in floating-point
CPU instructions being used? Like a compiler error if it did?

Yes, it's a constant expression, but I think there were some funny
rules in C that floating-point operations may not be evaluated at
compile time. Maybe I'm just paranoid?


Thanks,
pq

> +
> +		u16 r = FP_TO_INT_ROUND_UP(FP_DIV(fp_r, fp_rb_ratio));
> +		u16 g = FP_TO_INT_ROUND_UP(FP_DIV(fp_g, fp_g_ratio));
> +		u16 b = FP_TO_INT_ROUND_UP(FP_DIV(fp_b, fp_rb_ratio));
> +
> +		*dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
> +	}
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index 22358f3a33ab..836d6e43ea90 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -21,6 +21,9 @@ void get_ARGB16161616(struct vkms_frame_info *frame_info, int y,
>  void XRGB16161616_to_ARGB16161616(struct vkms_frame_info *frame_info, int y,
>  				  struct line_buffer *stage_buffer);
>  
> +void RGB565_to_ARGB16161616(struct vkms_frame_info *frame_info, int y,
> +			    struct line_buffer *stage_buffer);
> +
>  void convert_to_ARGB8888(struct vkms_frame_info *frame_info, int y,
>  			 struct line_buffer *src_buffer);
>  
> @@ -33,6 +36,9 @@ void convert_to_ARGB16161616(struct vkms_frame_info *frame_info, int y,
>  void convert_to_XRGB16161616(struct vkms_frame_info *frame_info, int y,
>  			     struct line_buffer *src_buffer);
>  
> +void convert_to_RGB565(struct vkms_frame_info *frame_info, int y,
> +		       struct line_buffer *src_buffer);
> +
>  typedef void (*format_transform_func)(struct vkms_frame_info *frame_info, int y,
>  				      struct line_buffer *buffer);
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 1d70c9e8f109..4643eefcdf29 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -13,14 +13,16 @@
>  
>  static const u32 vkms_formats[] = {
>  	DRM_FORMAT_XRGB8888,
> -	DRM_FORMAT_XRGB16161616
> +	DRM_FORMAT_XRGB16161616,
> +	DRM_FORMAT_RGB565
>  };
>  
>  static const u32 vkms_plane_formats[] = {
>  	DRM_FORMAT_ARGB8888,
>  	DRM_FORMAT_XRGB8888,
>  	DRM_FORMAT_XRGB16161616,
> -	DRM_FORMAT_ARGB16161616
> +	DRM_FORMAT_ARGB16161616,
> +	DRM_FORMAT_RGB565
>  };
>  
>  static struct drm_plane_state *
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 393d3fc7966f..1aaa630090d3 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -15,7 +15,8 @@
>  static const u32 vkms_wb_formats[] = {
>  	DRM_FORMAT_XRGB8888,
>  	DRM_FORMAT_XRGB16161616,
> -	DRM_FORMAT_ARGB16161616
> +	DRM_FORMAT_ARGB16161616,
> +	DRM_FORMAT_RGB565
>  };
>  
>  static const struct drm_connector_funcs vkms_wb_connector_funcs = {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]