[PATCH v5 6/9] drm: vkms: Refactor the plane composer to accept new formats

[Igor Torrente]

On 4/27/22 04:43, Pekka Paalanen wrote:
> On Tue, 26 Apr 2022 22:22:22 -0300
> Igor Torrente <igormtorrente at gmail.com> wrote:
> 
>> On April 26, 2022 10:03:09 PM GMT-03:00, Igor Torrente <igormtorrente at gmail.com> wrote:
>>>
>>>
>>> On 4/25/22 22:54, Igor Torrente wrote:
>>>> Hi Pekka,
>>>>
>>>> On 4/25/22 05:10, Pekka Paalanen wrote:
>>>>> On Sat, 23 Apr 2022 15:53:20 -0300
>>>>> Igor Torrente <igormtorrente at gmail.com> wrote:
>>>>>    
> 
> ...
> 
>>>>>>>>> +static void argb_u16_to_XRGB8888(struct vkms_frame_info *frame_info,
>>>>>>>>> +				 const struct line_buffer *src_buffer, int y)
>>>>>>>>> +{
>>>>>>>>> +	int x, x_dst = frame_info->dst.x1;
>>>>>>>>> +	u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
>>>>>>>>> +	struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
>>>>>>>>> +	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>>>>>>>> +			    src_buffer->n_pixels);
>>>>>>>>> +
>>>>>>>>> +	for (x = 0; x < x_limit; x++, dst_pixels += 4) {
>>>>>>>>> +		dst_pixels[3] = (u8)0xff;
>>>>>>>>
>>>>>>>> When writing to XRGB, it's not necessary to ensure the X channel has
>>>>>>>> any sensible value. Anyone reading from XRGB must ignore that value
>>>>>>>> anyway. So why not write something wacky here, like 0xa1, that is far
>>>>>>>> enough from both 0x00 or 0xff to not be confused with them even
>>>>>>>> visually? Also not 0x7f or 0x80 which are close to half of 0xff.
>>>>>>>>
>>>>>>>> Or, you could save a whole function and just use argb_u16_to_ARGBxxxx()
>>>>>>>> instead, even for XRGB destination.
>>>>>>>
>>>>>>>
>>>>>>> Right. Maybe I could just leave the channel untouched.
>>>>>
>>>>> Untouched may not be a good idea. Leaving anything untouched always has
>>>>> the risk of leaking information through uninitialized memory. Maybe not
>>>>> in this case because the destination is allocated by userspace already,
>>>>> but nothing beats being obviously correct.
>>>>
>>>> Makes sense.
>>>>    
>>>>>
>>>>> Whatever you decide here, be prepared for it becoming de-facto kernel
>>>>> UABI, because it is easy for userspace to (accidentally) rely on the
>>>>> value, no matter what you pick.
>>>>
>>>> I hope to make the right decision then.
>>>
>>> The de-facto UABI seems to be already in place for {A, X}RGB8888.
>>
>> "Only XRGB_8888
> 
> If that's only IGT, then you should raise an issue with IGT about this,
> to figure out if IGT is wrong by accident or if it is deliberate, and
> are we stuck with it.
> 
> This is why I would want to fill X with garbage, to make the
> expectations clear before the "obvious and logical constant value for X"
> makes a mess by making XRGB indistinguishable from ARGB. Then the next
> question is, do we need a special function to write out XRGB values, or
> can we simply re-use the ARGB function.
> 
> Do the tests expect X channel to be filled with 0xff or with the actual
> A values? This question will matter when all planes have ARGB
> framebuffers and no background color. Then even more questions will
> arise about what should actually happen with A values (blending
> equation).

I dig into the igt code a little bit and found that it's waiting for the 
channel to not be changed.
It fills all the pixels in the line with a value and calculates the CRC 
of the entire buffer, including the alpha.

I will crate an issue asking if this is intended.

> 
>>
>>>
>>> I changed from 0xff to 0xbe and the `writeback-check-output` started to fail.
> 
> 
> Thanks,
> pq