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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]

On Tue, 26 Apr 2022 22:22:22 -0300
Igor Torrente <igormtorrente@gmail.com> wrote:

> On April 26, 2022 10:03:09 PM GMT-03:00, Igor Torrente <igormtorrente@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@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 changed from 0xff to 0xbe and the `writeback-check-output` started to fail.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]