Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <17957593-33fb-a63d-181e-daee2e4689fc@gmail.com>
Date:   Sun, 11 Sep 2022 11:44:32 -0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH] drm/vkms: fix 32bit compilation error by replacing macros
Content-Language: en-US
To:     Melissa Wen <mwen@igalia.com>
Cc:     rodrigosiqueiramelo@gmail.com, melissa.srw@gmail.com,
        airlied@linux.ie, daniel@ffwll.ch,
        Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20220909114133.267689-1-mwen@igalia.com>
 <29f87796-b288-7cdc-86dd-050cf7f0b5dd@gmail.com>
 <20220910191035.ukxhlhbc3mscbqis@mail.igalia.com>
From:   Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
In-Reply-To: <20220910191035.ukxhlhbc3mscbqis@mail.igalia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/10/22 16:10, Melissa Wen wrote:
> On 09/09, Igor Matheus Andrade Torrente wrote:
>> Hi Mellisa,
>>
>> Thanks for the patch fixing my mistakes.
>>
>> On 9/9/22 08:41, Melissa Wen wrote:
>>> Replace vkms_formats macros for fixed-point operations with functions
>>> from drm/drm_fixed.h to do the same job and fix 32-bit compilation
>>> errors.
>>>
>>> Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
>>> Tested-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
>>> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>> ---
>>>    drivers/gpu/drm/vkms/vkms_formats.c | 53 +++++++++++------------------
>>>    1 file changed, 19 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
>>> index 300abb4d1dfe..ddcd3cfeeaac 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_formats.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
>>> @@ -1,27 +1,12 @@
>>>    // SPDX-License-Identifier: GPL-2.0+
>>> -#include <drm/drm_rect.h>
>>> +#include <linux/kernel.h>
>>>    #include <linux/minmax.h>
>>> +#include <drm/drm_rect.h>
>>> +#include <drm/drm_fixed.h>
>>>    #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 SHIFT 15
>>> -
>>> -#define INT_TO_FIXED(a) ((a) << SHIFT)
>>> -#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
>>> -#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
>>> -/* This macro converts a fixed point number to int, and round half up it */
>>> -#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
>>> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
>>> -#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
>>> -
>>>    static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
>>>    {
>>>    	return frame_info->offset + (y * frame_info->pitch)
>>> @@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
>>>    	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>>    			       stage_buffer->n_pixels);
>>> -	s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
>>> -	s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
>>> +	s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
>>> +	s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>>
>> I think you need to add `drm_int2fixp` to 31 and 63.
>>
>>>    	for (size_t x = 0; x < x_limit; x++, src_pixels++) {
>>>    		u16 rgb_565 = le16_to_cpu(*src_pixels);
>>> -		s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
>>> -		s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
>>> -		s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
>>> +		s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
>>> +		s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
>>> +		s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);
>>
>> And we are cast implicitly from 64 bits int to 32 bits which is
>> implementation-defined AFAIK. So, probably we should be using `s64` for all
>> of these variables.
>>
>> I tested the patch. And I'm seeing some differences in the intermediate
>> results. From my testing, these changes solve those differences.
> 
> Hi Igor,
> 
> Thanks for checking the calc results and all inputs provided.  I just
> sent a second version, can you take a look? I replicated your
> suggestions for RGB565_to_argb_u16() in argb_u16_to_RGB565() and
> double-checked for i386 and arm. Let me know what yexiuou think.

I tested it here and everything seem to be working :).

> 
>>
>> Another thing that may have an impact on the final output is the lack of
>> rounding in drm_fixed.h. This can potentially produce the wrong result.
> 
> Yeah, I see... I can include a comment about the rounding issue for
> further improvements, or do you plan to work on it?

A comment would be good. Maybe add to the VKMS TODO list?

I'm busy with other stuffs, and can't work on this any time soon. But 
It's pretty simple thing to implement.

> 
> Thanks,
> 
> Melissa
>>
>> Thanks,
>> ---
>> Igor Torrente
>>
>>>    		out_pixels[x].a = (u16)0xffff;
>>> -		out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, fp_rb_ratio));
>>> -		out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, fp_g_ratio));
>>> -		out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, fp_rb_ratio));
>>> +		out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
>>> +		out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
>>> +		out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
>>>    	}
>>>    }
>>> @@ -248,17 +233,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);
>>> -	s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
>>> -	s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
>>> +	s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
>>> +	s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);
>>>    	for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
>>> -		s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
>>> -		s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
>>> -		s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
>>> +		s32 fp_r = drm_int2fixp(in_pixels[x].r);
>>> +		s32 fp_g = drm_int2fixp(in_pixels[x].g);
>>> +		s32 fp_b = drm_int2fixp(in_pixels[x].b);
>>> -		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));
>>> +		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));
>>>    		*dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
>>>    	}
>>