Re: [PATCH v7 3/3] drm: Add GUD USB Display driver

["Noralf Trønnes" <noralf@tronnes.org>]

Den 10.03.2021 05.55, skrev Peter Stuge:
> Noralf Trønnes wrote:
>>> Depending on how long it takes for the DMA mask dependency patch to show
>>> up in drm-misc-next, I will either publish a new version or apply the
>>> current and provide patches with the necessary fixes. 
>>
>> In case I apply this version, are you happy enough with it that you want
>> to give an ack or r-b?
> 
> I've now tested R1 and RGB111 and I think I've found two more things:
> 
> I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
> I think because of how components were extracted in gud_xrgb8888_to_color().
> 
> Changing to the following gets me the expected (X R1 G1 B1 X R2 G2 B2) bytes:
> 
> 			r = (*pix32 >> 8) & 0xff;
> 			g = (*pix32 >> 16) & 0xff;
> 			b = (*pix32++ >> 24) & 0xff;
> 

We're accessing the whole word here through pix32, no byte access, so
endianess doesn't come into play. This change will flip r and b, which
gives: XRGB8888 -> XBGR1111

BGR is a common thing on controllers, are you sure yours are set to RGB
and not BGR?

And the 0xff masking isn't necessary since we're assigning to a byte, right?

I haven't got a native R1G1B1 display so I have emulated and I do get
the expected colors. This is the conversion function I use on the device
which I think is correct:

static size_t rgb111_to_rgb565(uint16_t *dst, uint8_t *src,
                               uint16_t src_width, uint16_t src_height)
{
    uint8_t rgb111, val = 0;
    size_t len = 0;

    for (uint16_t y = 0; y < src_height; y++) {
        for (uint16_t x = 0; x < src_width; x++) {
            if (!(x % 2))
                val = *src++;
            rgb111 = val >> 4;
            *dst++ = ((rgb111 & 0x04) << 13) | ((rgb111 & 0x02) << 9) |
                     ((rgb111 & 0x01) << 4);
            len += sizeof(*dst);
            val <<= 4;
        }
    }

   return len;
}

> 
> Then, gud_xrgb8888_to_color() and maybe even gud_xrgb8888_to_r124() seem
> to be host endian dependent, at least because of that pix32, but maybe more?
> I don't know whether drm guarantees "native" XRGB byte sequence in memory,
> then I guess the pix32 is okay? Please take a look?
> 
> 
> Finally my very last ask: Please consider renaming GUD_PIXEL_FORMAT_RGB111
> to GUD_PIXEL_FORMAT_XRGB1111?
> 

It has crossed my mind, I'll change it.

Noralf.