On Tue, 26 Apr 2022 21:53:19 -0300 Igor Torrente wrote: > Hi Pekka, > > On 4/21/22 07:58, Pekka Paalanen wrote: > > On Mon, 4 Apr 2022 17:45:15 -0300 > > Igor Torrente 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. > >> > >> V3: Adapt the handlers to the new format introduced in patch 7 V3. > >> V5: Minor improvements > >> > >> Signed-off-by: Igor Torrente > >> --- > >> drivers/gpu/drm/vkms/vkms_formats.c | 70 +++++++++++++++++++++++++++ > >> drivers/gpu/drm/vkms/vkms_plane.c | 6 ++- > >> drivers/gpu/drm/vkms/vkms_writeback.c | 3 +- > >> 3 files changed, 76 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c > >> index 8d913fa7dbde..4af8b295f31e 100644 > >> --- a/drivers/gpu/drm/vkms/vkms_formats.c > >> +++ b/drivers/gpu/drm/vkms/vkms_formats.c > >> @@ -5,6 +5,23 @@ > >> > >> #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 FIXED_SCALE 15 > > > > I think this would usually be called a "shift" since it's used in > > bit-shifts. > > Ok, I will rename this. > > > > >> + > >> +#define INT_TO_FIXED(a) ((a) << FIXED_SCALE) > >> +#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> FIXED_SCALE)) > >> +#define FIXED_DIV(a, b) ((s32)(((s64)(a) << FIXED_SCALE) / (b))) > > > > A truncating div, ok. > > > >> +/* This macro converts a fixed point number to int, and round half up it */ > >> +#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (FIXED_SCALE - 1))) >> FIXED_SCALE) > > > > Yes. > > > >> +/* Convert divisor and dividend to Fixed-Point and performs the division */ > >> +#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b))) > > > > Ok, this is obvious to read, even though it's the same as FIXED_DIV() > > alone. Not sure the compiler would optimize that extra bit-shift away... > > > > If one wanted to, it would be possible to write type-safe functions for > > these so that fixed and integer could not be mixed up. > > Ok, I will move to a function. That's not all. If you want it type-safe, then you need something like struct vkms_fixed_point { s32 value; }; And use `struct vkms_fixed_point` (by value) everywhere where you pass a fixed point value, and never as a plain s32 type. Then it will be impossible to do incorrect arithmetic or conversions by accident on fixed point values. Is it worth it? I don't know, since it's limited into this one file. A simple 'typedef s32 vkms_fixed_point' does not work, because it does not prevent computing with vkms_fixed_point as if it was just a normal s32. Using a struct prevents that. I wonder if the kernel doesn't already have something like this available in general... > >> + 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)); > >> + > >> + *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b); > > > > Looks good. > > > > You are using signed variables (int, s64, s32) when negative values > > should never occur. It doesn't seem wrong, just unexpected. > > I left the signal so I can reuse them in the YUV formats. Good point. > > > > > The use of int in code vs. s32 in the macros is a bit inconsistent as > > well. > > Right. I think I will stick with s32 and s64 then. ... > >> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c > >> index cb63a5da9af1..98da7bee0f4b 100644 > >> --- a/drivers/gpu/drm/vkms/vkms_writeback.c > >> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c > >> @@ -16,7 +16,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 = { > > > > I wonder, would it be possible to add a unit test to make sure that > > get_plane_fmt_transform_function() or get_wb_fmt_transform_function() > > does not return NULL for any of the listed formats, respectively? > > Or is that too paranoid? > > I'm not opposed to it. But I also don't think it needs to be in this > series of patches either. > > A new todo maybe? If it's a good thing, then it belongs in this series, because those function getters are introduced in this series, opening the door for the mistakes that the tests would prevent. I don't mean IGT tests but kernel internal tests. I think there is a unit test framework? You really should get a kernel maintainer's opinion on these questions, as I am not a kernel developer. Thanks, pq