Hi Pekka,
On Thu, Feb 10, 2022 at 6:50 AM Pekka Paalanen ppaalanen@gmail.com wrote:
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?
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 iscalculated
* 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 = {