Hi Pekka,
On Thu, Feb 10, 2022 at 6:50 AM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
On Fri, 21 Jan 2022 18:38:31 -0300
Igor Torrente <igormtorrente@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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?
I cannot say for sure, but I don't think so. I put it for people like me
that goes automatically to Floating-Point because never worked with
fixed-point before.
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.
It looks better, I will try to implement this.
> + * 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?
Well, I cannot guarantee anything, but every time that I intentionally/unintentionally
did anything related with floating-point it couldn't link the kernel.
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 = {
