[igt-dev] [PATCH] tests/kms_writeback.c: fix the `fill_fb` function

[Igor Matheus Andrade Torrente]

On 10/7/21 9:58 AM, Petri Latvala wrote:
> On Tue, Oct 05, 2021 at 10:56:01AM -0300, Igor Matheus Andrade Torrente wrote:
>> Currently, the `fill_fb` function, instead of filling the framebuffer
>> with the whole pixel, is only filling it with the rightmost byte.
>>
>> This is happening because, even though the memset accepts an int as
>> a parameter, it's casting the value to unsigned char and will fill
>> the memory with this 8 bits value.
>>
>> If we setup the pixel to 0xff0000|f0|, the buffer will filled with:
>>
>> | addr     | = 0f
>> | addr + 1 | = 0f
>> | addr + 2 | = 0f
>> | addr + 3 | = 0f
>>      ...
>> | addr + N | = 0f
>>
>> This patch address this issue by manually copying the entire integer
>> in the little-endian format.
>>
>> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente at gmail.com>
>> ---
>>   tests/kms_writeback.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>> index 60dbda2a..60d9b74e 100644
>> --- a/tests/kms_writeback.c
>> +++ b/tests/kms_writeback.c
>> @@ -227,14 +227,26 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
>>   
>>   static void fill_fb(igt_fb_t *fb, uint32_t pixel)
>>   {
>> -	void *ptr;
>> +	uint8_t *ptr, x, r, g, b, step = sizeof(uint32_t);
>> +	int64_t pixel_count, i;
>>   
>>   	igt_assert(fb->drm_format == DRM_FORMAT_XRGB8888);
>>   
>>   	ptr = igt_fb_map_buffer(fb->fd, fb);
>>   	igt_assert(ptr);
>>   
>> -	memset(ptr, pixel, fb->strides[0] * fb->height);
>> +	pixel_count = fb->strides[0] * fb->height;
>> +	x = (pixel & (0xff << 24)) >> 24;
>> +	r = (pixel & (0xff << 16)) >> 16;
>> +	g = (pixel & (0xff << 8)) >> 8;
>> +	b = pixel & 0xff;
>> +
>> +	for (i = 0; i <= pixel_count - step; i += step) {
>> +		ptr[i + 3] = x;
>> +		ptr[i + 2] = r;
>> +		ptr[i + 1] = g;
>> +		ptr[i + 0] = b;
>> +	}
> 
> Is there a reason to explicitly go for little endian here? Can't the
> pixel be written as is, like
> 
> uint32_t *p = ptr;
> 
> while (p++ < ptr + fb->strides[0] * fb->height)
>    *p = pixel;
> 
>

I did it to cover the possible case of this code being running on 
big-endian machines. But, I realized that isn't necessary, since I'm 
just copying the bytes and not manipulating them.

I will send a V2 without all this code.