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: 3822 bytes --]

On Wed, 27 Apr 2022 21:44:34 -0300
Igor Torrente <igormtorrente@gmail.com> wrote:

> On 4/27/22 04:43, Pekka Paalanen wrote:
> > 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 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 just remembered this:
https://lists.freedesktop.org/archives/igt-dev/2022-March/039920.html


Thanks,
pq

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