Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Den 12.03.2021 05.32, skrev Peter Stuge: > Ilia Mirkin wrote: >> XRGB means that the memory layout should match a 32-bit integer, >> stored as LE, with the low bits being B, next bits being G, etc. This >> translates to byte 0 = B, byte 1 = G, etc. If you're on a BE system, >> and you're handed a XRGB buffer, it still expects that byte 0 = B, >> etc (except as I outlined, some drivers which are from before these >> formats were a thing, sort of do their own thing). Thankfully this is >> equivalent to BGRX (big-endian packing), so you can just munge the >> format. > > I understand! Thanks a lot for clarifying. > > It makes much more sense to me that the format indeed describes > what is in memory rather than how pixels look to software. > > I'm not sure why you guys were talking about BE in the first place, >>> >>> I was worried that the translation didn't consider endianess. >> >> The translation in gud_xrgb_to_color definitely seems suspect. > > So to me this means that the gud_pipe translations from XRGB to the > 1-bit formats *do* have to adjust for the reversed order on BE. > > >> There's also a gud_is_big_endian, but I'm guessing this applies to the >> downstream device rather than the host system. > > gud_is_big_endian() is a static bool wrapper around defined(__BIG_ENDIAN) > so yes, it applies to the host. > > With memory layout being constant I again think gud_xrgb_to_color() > needs to take further steps to work correctly also on BE hosts. (Maybe > that's le32_to_cpu(*pix32), maybe drm_fb_swab(), maybe something else?) > > >> I didn't check if dev->mode_config.quirk_addfb_prefer_host_byte_order >> is set > > I can't tell if that's helpful, probably Noralf can. > I skimmed a discussion a while back around BE and all it's problems through the whole graphics stack and my take away was that I needed a BE machine so I could test the whole stack to make sure it works. Judging from what Ilia says I've been wrong in my assumptions so the driver is probably broken on BE (along with my SPI display drivers). I'm not going to try and fix this now, I need someone with a BE machine that can test the whole stack and make sure it actually works. Me doing more guesswork is not going to work out well I think :) I'll add a FIXME in gud_pipe.c that BE is probably broken and link to this discussion. It seems Gerd did some work to fix BE in the kernel: drm: byteorder fixes https://patchwork.freedesktop.org/series/49073/ But that seems to deal with the format value itself and not the buffer contents. Noralf. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Ilia Mirkin wrote: > XRGB means that the memory layout should match a 32-bit integer, > stored as LE, with the low bits being B, next bits being G, etc. This > translates to byte 0 = B, byte 1 = G, etc. If you're on a BE system, > and you're handed a XRGB buffer, it still expects that byte 0 = B, > etc (except as I outlined, some drivers which are from before these > formats were a thing, sort of do their own thing). Thankfully this is > equivalent to BGRX (big-endian packing), so you can just munge the > format. I understand! Thanks a lot for clarifying. It makes much more sense to me that the format indeed describes what is in memory rather than how pixels look to software. > > > I'm not sure why you guys were talking about BE in the first place, > > > > I was worried that the translation didn't consider endianess. > > The translation in gud_xrgb_to_color definitely seems suspect. So to me this means that the gud_pipe translations from XRGB to the 1-bit formats *do* have to adjust for the reversed order on BE. > There's also a gud_is_big_endian, but I'm guessing this applies to the > downstream device rather than the host system. gud_is_big_endian() is a static bool wrapper around defined(__BIG_ENDIAN) so yes, it applies to the host. With memory layout being constant I again think gud_xrgb_to_color() needs to take further steps to work correctly also on BE hosts. (Maybe that's le32_to_cpu(*pix32), maybe drm_fb_swab(), maybe something else?) > I didn't check if dev->mode_config.quirk_addfb_prefer_host_byte_order > is set I can't tell if that's helpful, probably Noralf can. Thanks a lot //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
On Thu, Mar 11, 2021 at 5:58 PM Peter Stuge wrote:
>
> Ilia Mirkin wrote:
> > > > #define DRM_FORMAT_XRGB fourcc_code('X', 'R', '2', '4') /* [31:0]
> > > > x:R:G:B 8:8:8:8 little endian */
> > >
> > > Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
> > > [31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
> > > becomes B G R X in memory, for which your pix32 code is correct.
> > >
> > > That's the reverse *memory* layout of what the name says :)
> >
> > The definition of the formats is memory layout in little endian.
>
> To clarify, my new (hopefully correct?) understanding is this:
>
> XRGB does *not* mean that address 0=X, 1=R, 2=G, 3=B, but that
> the most significant byte in a packed XRGB 32-bit integer is X
> and the least significant byte is B, and that this is the case both
> on LE and BE machines.
Not quite.
XRGB means that the memory layout should match a 32-bit integer,
stored as LE, with the low bits being B, next bits being G, etc. This
translates to byte 0 = B, byte 1 = G, etc. If you're on a BE system,
and you're handed a XRGB buffer, it still expects that byte 0 = B,
etc (except as I outlined, some drivers which are from before these
formats were a thing, sort of do their own thing). Thankfully this is
equivalent to BGRX (big-endian packing), so you can just munge the
format. Not so with e.g. RGB565 though (since the components don't
fall on byte boundaries).
> I previously thought that XRGB indicated the memory byte order of
> components being X R G B regardless of machine endianess, but now
> understand XRGB to mean the MSB..LSB order of component bytes within
> the 32-bit integer, as seen by software, not the order of bytes in memory.
There are about 100 conventions, and they all manage to be different
from each other. Packed vs array. BE vs LE. If you're *not* confused,
that should be a red flag.
[...]
> > I'm not sure why you guys were talking about BE in the first place,
>
> I was worried that the translation didn't consider endianess.
The translation in gud_xrgb_to_color definitely seems suspect.
There's also a gud_is_big_endian, but I'm guessing this applies to the
downstream device rather than the host system. I didn't check if
dev->mode_config.quirk_addfb_prefer_host_byte_order is set -- that
setting dictates whether these formats are in host-byte-order (and
AddFB2 is disabled, so buffers can only be specified with depth/bpp
and ambiguous component orders) or in LE byte order (and userspace can
use AddFB2 which gives allows precise formats for these buffers). Not
100% sure what something like Xorg's modesetting driver does, TBH.
This is a very poorly-tested scenario.
-ilia
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Ilia Mirkin wrote:
> > > #define DRM_FORMAT_XRGB fourcc_code('X', 'R', '2', '4') /* [31:0]
> > > x:R:G:B 8:8:8:8 little endian */
> >
> > Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
> > [31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
> > becomes B G R X in memory, for which your pix32 code is correct.
> >
> > That's the reverse *memory* layout of what the name says :)
>
> The definition of the formats is memory layout in little endian.
To clarify, my new (hopefully correct?) understanding is this:
XRGB does *not* mean that address 0=X, 1=R, 2=G, 3=B, but that
the most significant byte in a packed XRGB 32-bit integer is X
and the least significant byte is B, and that this is the case both
on LE and BE machines.
I previously thought that XRGB indicated the memory byte order of
components being X R G B regardless of machine endianess, but now
understand XRGB to mean the MSB..LSB order of component bytes within
the 32-bit integer, as seen by software, not the order of bytes in memory.
> The definition you see is of a 32-bit packed little-endian integer,
> which is a fixed memory layout.
In the header definition I'm not completely sure what the "little endian"
means - I guess it refers to how the 32-bit integer will be stored in memory,
but it could also refer to the order of component packing within.
Noralf's code and testing and also what fbset tells me seems to support
this understanding, at least on LE machines.
> Now, if you're on an actual big-endian platform, and you want to
> accept big-endian-packed formats, there's a bit of unpleasantness that
> goes on.
In the particular case of XRGB that Noralf has implemented and
I've tested every pixel is translated "manually" anyway; each
component byte is downconverted to a single bit, but this use case
is mostly for smaller resolutions, so no too big deal.
> I'm not sure why you guys were talking about BE in the first place,
I was worried that the translation didn't consider endianess.
Noralf, looking at the 3/3 patch again now, drm_fb_swab() gets called
on BE when format == fb->format, but does it also need to be called
on BE they are different, or will circumstances be such that it's
never neccessary then?
Thanks and sorry if I'm confusing things needlessly
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
On Thu, Mar 11, 2021 at 3:02 PM Peter Stuge wrote:
> > > Hence the question: What does DRM promise about the XRGB mode?
> >
> > That it's a 32-bit value. From include/uapi/drm/drm_fourcc.h:
> >
> > /* 32 bpp RGB */
> > #define DRM_FORMAT_XRGB fourcc_code('X', 'R', '2', '4') /* [31:0]
> > x:R:G:B 8:8:8:8 little endian */
>
> Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
> [31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
> becomes B G R X in memory, for which your pix32 code is correct.
>
> That's the reverse *memory* layout of what the name says :) but yes,
> the name then matches the representation seen by software. That's the
> "abstracted" case that I didn't expect, because I thought the name was
> refering to memory layout and because I was thinking about how traditional
> graphics adapter video memory has the R component at the lower
> address, at least in early linear modes.
The definition of the formats is memory layout in little endian. The
definition you see is of a 32-bit packed little-endian integer, which
is a fixed memory layout.
Now, if you're on an actual big-endian platform, and you want to
accept big-endian-packed formats, there's a bit of unpleasantness that
goes on. Basically there are two options:
1. Ignore the above definition and interpret the formats as
*big-endian* layouts. This is what nouveau and radeon do. They also
don't support AddFB2 (which is what allows supplying a format) -- only
AddFB which just has depth (and bpp). That's fine for nouveau and
radeon because the relevant userspace just uses AddFB, and knows what
the drivers want, so it all works out.
2. Comply with the above definition and set
dev->mode_config.quirk_addfb_prefer_host_byte_order to false. This
loses you native host packing of RGB565/etc, since they're just not
defined as formats. There's a DRM_FORMAT_BIG_ENDIAN bit but it's not
properly supported for anything but the formats.
I'm not sure why you guys were talking about BE in the first place,
but since this is a topic I've looked into (in the context of moving
nouveau from 1 to 2 - but that can't happen due to the reduced format
availability), figured I'd share some of the current sad state.
Cheers,
-ilia
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Noralf Trønnes wrote:
> > Endianness matters because parts of pix32 are used.
>
> This code:
..
> prints:
>
> xrgb=aabbccdd
> 32-bit access:
> r=bb
> g=cc
> b=dd
> Byte access on LE:
> r=cc
> g=bb
> b=aa
As expected, and:
xrgb=aabbccdd
32-bit access:
r=bb
g=cc
b=dd
Byte access on BE:
r=bb
g=cc
b=dd
I've done similar tests in the past and did another before my last mail.
We agree about endian effects. Apologies if I came across as overbearing!
> > Hence the question: What does DRM promise about the XRGB mode?
>
> That it's a 32-bit value. From include/uapi/drm/drm_fourcc.h:
>
> /* 32 bpp RGB */
> #define DRM_FORMAT_XRGB fourcc_code('X', 'R', '2', '4') /* [31:0]
> x:R:G:B 8:8:8:8 little endian */
Okay, "[31:0] x:R:G:B 8:8:8:8" can certainly mean
[31:24]=x [23:16]=R [15:8]=G [7:0]=B, which when stored "little endian"
becomes B G R X in memory, for which your pix32 code is correct.
That's the reverse *memory* layout of what the name says :) but yes,
the name then matches the representation seen by software. That's the
"abstracted" case that I didn't expect, because I thought the name was
refering to memory layout and because I was thinking about how traditional
graphics adapter video memory has the R component at the lower
address, at least in early linear modes.
I also didn't pay attention to the fbset output:
rgba 8/16,8/8,8/0,0/0
With drm format describing software pixel representation and per the
fbset rgba description my test file was incorrect. I've recreated it
with B G R X bytes and it shows correctly with your pix32 code.
Sending data directly to the device without the gud driver uses
different data, so isn't actually a fair comparison, but I didn't
change the device at all now, and that still works.
> If a raw buffer was passed from a BE to an LE machine, there would be
> problems because of how the value is stored,
And swab would be required on a LE machine with a graphics adapter in
a mode with X R G B memory layout, or that system would just never
present XRGB for that adapter/mode but perhaps something called
BGRX instead? I see.
> but here it's the same endianness in userspace and kernel space.
Ack.
> There is code in gud_prep_flush() that handles a BE host with a
> multibyte format:
>
> } else if (gud_is_big_endian() && format->cpp[0] > 1) {
> drm_fb_swab(buf, vaddr, fb, rect, !import_attach);
>
> In this case we can't just pass on the raw buffer to the device since
> the protocol is LE, and thus have to swap the bytes to match up how
> they're stored in memory on the device.
Ack.
> I'm not loosing any of the colors when running modetest. This is the
> test image that modetest uses and it comes through just like that:
> https://commons.wikimedia.org/wiki/File:SMPTE_Color_Bars.svg
So your destination rgb565 buffer has a [15:11]=R [10:5]=G [4:0]=B
pixel format, which stores as B+G G+R in memory, as opposed to R+G G+B.
All right.
Thanks a lot for clearing up my misunderstanding of drm format names
and my endianess concerns!
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Den 11.03.2021 15.48, skrev Peter Stuge:
> Noralf Trønnes wrote:
>>> I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
>>> I think because of how components were extracted in gud_xrgb_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.
>
> Endianness matters because parts of pix32 are used.
>
This code:
#include
#include
void main()
{
volatile uint32_t endian = 0x01234567;
uint32_t v = 0xaabbccdd;
uint32_t *pix32 =
uint8_t r, g, b, *p;
r = *pix32 >> 16;
g = *pix32 >> 8;
b = *pix32++;
printf("xrgb=%08x\n", v);
printf("32-bit access:\n");
printf("r=%02x\n", r);
printf("g=%02x\n", g);
printf("b=%02x\n", b);
printf("Byte access on %s:\n", (*((uint8_t*)())) == 0x67 ? "LE"
: "BE");
p = (uint8_t *)
printf("r=%02x\n", p[1]);
printf("g=%02x\n", p[2]);
printf("b=%02x\n", p[3]);
}
prints:
xrgb=aabbccdd
32-bit access:
r=bb
g=cc
b=dd
Byte access on LE:
r=cc
g=bb
b=aa
> Software only sees bytes (or larger) because addresses are byte granular,
> but must pay attention to the bit order when dealing with smaller values
> inside larger memory accesses.
>
> Given 4 bytes of memory {0x11, 0x22, 0x33, 0x44} at address A, both LE
> and BE machines appear the same when accessing individual bytes, but with
> uint32_t *a32 = A then a32[0] is 0x44332211 on LE and 0x11223344 on BE.
>
>
> Hence the question: What does DRM promise about the XRGB mode?
>
That it's a 32-bit value. From include/uapi/drm/drm_fourcc.h:
/* 32 bpp RGB */
#define DRM_FORMAT_XRGB fourcc_code('X', 'R', '2', '4') /* [31:0]
x:R:G:B 8:8:8:8 little endian */
If a raw buffer was passed from a BE to an LE machine, there would be
problems because of how the value is stored, but here it's the same
endianness in userspace and kernel space.
There is code in gud_prep_flush() that handles a BE host with a
multibyte format:
} else if (gud_is_big_endian() && format->cpp[0] > 1) {
drm_fb_swab(buf, vaddr, fb, rect, !import_attach);
In this case we can't just pass on the raw buffer to the device since
the protocol is LE, and thus have to swap the bytes to match up how
they're stored in memory on the device.
I'm not loosing any of the colors when running modetest. This is the
test image that modetest uses and it comes through just like that:
https://commons.wikimedia.org/wiki/File:SMPTE_Color_Bars.svg
Noralf.
> Is it guaranteed that the first byte in memory is always unused, the second
> represents red, the third green and the fourth blue (endianess agnostic)?
> I'd expect this and I guess that it is the case, but I don't know DRM?
>
> Or is it instead guaranteed that when accessed natively as one 32-bit
> value the blue component is always in the most significant byte (endianess
> abstracted, always LE in memory) or always in the least significant byte
> (endianess abstracted, always BE in memory)?
> This would be annoying for userspace, but well, it's possible.
>
> In the abstracted (latter) case pix32 would work, but could still be
> questioned on style, and in fact, pix32 didn't work for me, so at a
> minimum the byte order would be the reverse.
>
>
> In the agnostic (former) case your code was correct for BE and mine
> for LE, but I'd then suggest using a u8 * to both work correctly
> everywhere and be obvious.
>
>
>> This change will flip r and b, which gives: XRGB -> XBGR
>
> The old code was:
> r = *pix32 >> 16;
> g = *pix32 >> 8;
> b = *pix32++;
>
> On my LE machine this set r to the third byte (G), g to the second (R)
> and b to the first (X), explaining the color confusion that I saw.
>
>
>> BGR is a common thing on controllers, are you sure yours are set to RGB
>> and not BGR?
>
> Yes; I've verified that my display takes red in MSB both in its data
> sheet and by writing raw bits to it on a system without the gud driver.
>
>
>> And the 0xff masking isn't necessary since we're assigning to a byte, right?
>
> Not strictly neccessary but I like to do it anyway, both to be explicit
> and also to ensure that the compiler will never sign extend, if types
> are changed or if values become treated as signed and/or larger by the
> compiler because the code is changed.
>
> It's frustrating to debug such unexpected changes in behavior due to
> a type change or calculation change, but if you find it too defensive
> then go ahead and remove it, if pix32 does stay.
>
>
>> I haven't got a native R1G1B1 display so I have emulated and I do get
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Noralf Trønnes wrote:
> > I didn't receive the expected bits/bytes for RGB111 on the bulk endpoint,
> > I think because of how components were extracted in gud_xrgb_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.
Endianness matters because parts of pix32 are used.
Software only sees bytes (or larger) because addresses are byte granular,
but must pay attention to the bit order when dealing with smaller values
inside larger memory accesses.
Given 4 bytes of memory {0x11, 0x22, 0x33, 0x44} at address A, both LE
and BE machines appear the same when accessing individual bytes, but with
uint32_t *a32 = A then a32[0] is 0x44332211 on LE and 0x11223344 on BE.
Hence the question: What does DRM promise about the XRGB mode?
Is it guaranteed that the first byte in memory is always unused, the second
represents red, the third green and the fourth blue (endianess agnostic)?
I'd expect this and I guess that it is the case, but I don't know DRM?
Or is it instead guaranteed that when accessed natively as one 32-bit
value the blue component is always in the most significant byte (endianess
abstracted, always LE in memory) or always in the least significant byte
(endianess abstracted, always BE in memory)?
This would be annoying for userspace, but well, it's possible.
In the abstracted (latter) case pix32 would work, but could still be
questioned on style, and in fact, pix32 didn't work for me, so at a
minimum the byte order would be the reverse.
In the agnostic (former) case your code was correct for BE and mine
for LE, but I'd then suggest using a u8 * to both work correctly
everywhere and be obvious.
> This change will flip r and b, which gives: XRGB -> XBGR
The old code was:
r = *pix32 >> 16;
g = *pix32 >> 8;
b = *pix32++;
On my LE machine this set r to the third byte (G), g to the second (R)
and b to the first (X), explaining the color confusion that I saw.
> BGR is a common thing on controllers, are you sure yours are set to RGB
> and not BGR?
Yes; I've verified that my display takes red in MSB both in its data
sheet and by writing raw bits to it on a system without the gud driver.
> And the 0xff masking isn't necessary since we're assigning to a byte, right?
Not strictly neccessary but I like to do it anyway, both to be explicit
and also to ensure that the compiler will never sign extend, if types
are changed or if values become treated as signed and/or larger by the
compiler because the code is changed.
It's frustrating to debug such unexpected changes in behavior due to
a type change or calculation change, but if you find it too defensive
then go ahead and remove it, if pix32 does stay.
> 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);
I'm afraid this isn't correct. Two wrongs end up cancelling each other
out and it's not so obvious because the destination has symmetric (565)
components.
If you were to convert to xrgb in the same way I think you'd also
see some color confusion, and in any case blue is getting lost already
in gud_xrgb_to_color() on LE.
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
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_xrgb_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: XRGB -> XBGR
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_xrgb_to_color() and maybe even gud_xrgb_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_XRGB?
>
It has crossed my mind, I'll change it.
Noralf.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Noralf Trønnes wrote:
> > The u16 index parameter to gud_usb_transfer() and at least also
> > gud_usb_{get,set,get_u8,set_u8}() is eventually passed in u16 value
> > in the call to gud_usb_control_msg(), which had me confused for a bit.
> >
> > What do you think about renaming all of those parameters to wValue,
>
> Only connector requests use this value and in that case it's the
> connector index. I need to get this driver applied now, and can't
> spend more time polishing it, so I'll just keep it as-is.
Okay.
> > I found this because I can't get my device to send 0 bytes IN when
> > the host requests more, if I provide no data the request STALLs. This
> > is for sure a bug in my device
..
> > What do you think about formalizing this, adding an actual dummy property?
>
> I want to avoid that.
Ack. I too prefer not adding workarounds, this needs to be fixed in
my device, but I wanted to bring it up.
> I'd rather add a descriptor flag GUD_DISPLAY_FLAG_CANT_RETURN_ZERO or
> something for such devices.
> Also this property warning is just a debug message so not a very big
> problem.
I'd rather not squat a currently-unused property that may be allocated
later.
> Worse is EDID if you don't want to support that since it prints
> an error in that case.
I'll fix my device. If that proves impossible I can still add EDID bytes.
> > Or maybe adding flags to the display descriptor for "I have properties",
> > "I have connector properties" and "I have EDID" ?
>
> Then I might as well go back to the previous version and have count in
> the structs won't I ;)
That's true. Forget the workarounds.
> gud_xrgb_to_color() handles that, although broken in this version
> for partial updates. Here's the fix:
> https://gist.github.com/notro/a94d381cf98b7e15fcddbc90e4af0a21
Thanks.
> I've chosen to only print probe errors, flush errors and devices
> returning incorrect reply lengths. I hate drivers that fill the logs
> with errors, but I might have gone to far on the silent side, I don't
> know. But one nice thing about DRM is that debug is always builtin and
> can be enabled.
I think you've struck a good balance. The error messages I've seen were
all unique and immediately led me to the right place in the driver.
That's really helpful.
> > Finally, here's the drm debug output when I connect my device:
..
> > It looks good, I think? However, neither GUD_REQ_SET_CONTROLLER_ENABLE
> > nor GUD_REQ_SET_DISPLAY_ENABLE is ever called, and there is no bulk
> > output if I write to /dev/fb0.
> >
> > Can you tell what's missing?
>
> IIRC you need ioctl FBIOPUT_VSCREENINFO to enable the pipeline. You can
> use 'con2fbmap' to put a console on the fbdev or use 'fbi' to show an
> image. They will do the necessary ioctls.
Thanks for the hints! I've used fbi, but remembered writing directly
to /dev/fb0 without ioctl:s. I think that must have been after the
console had already been moved to the framebuffer and thus activated
the pipe. I dug up a version of fbset which calls that ioctl.
> I use modetest for simple testing:
> https://github.com/notro/tinydrm/wiki/Development#modetest
Ack, libdrm just has an inconveniently long dependency tail for my
test environment. But fbset + dd works.
> I have made tool that employs usbmon to show what happens just outside
> the wire (runs on the host):
> https://github.com/notro/gud/blob/master/tools/monitor.py
> I haven't got a proper USB analyser, so this was second best.
Nice idea! I've been using usbmon as well during development, it's great.
I find a hardware analyzer is only really neccessary for lowlevel problems
like my no-0-bytes bug.
> 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.
I look forward to it landing.
> This is one of those things that would never have happened if I knew
> how long it would take :)
Thanks again for working on this great idea! I'm happy that I can help.
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_xrgb_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;
Then, gud_xrgb_to_color() and maybe even gud_xrgb_to_r124() seem
to be host endian dependent, at least because of that pix32, but maybe
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Den 09.03.2021 19.07, skrev Noralf Trønnes: > > > Den 09.03.2021 15.02, skrev Peter Stuge: >> Hello Noralf, >> >> I've made some progress with my test device. I'm implementing R1 >> first and once that works I'll test RGB111 as well. Along the way >> I've found a couple of things in the code: >> > 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? Noralf. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Den 09.03.2021 15.02, skrev Peter Stuge:
> Hello Noralf,
>
> I've made some progress with my test device. I'm implementing R1
> first and once that works I'll test RGB111 as well. Along the way
> I've found a couple of things in the code:
>
> Noralf Trønnes wrote:
>> +++ b/drivers/gpu/drm/gud/gud_drv.c
> ..
>> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id
>> *id)
> ..
>> +if (format == GUD_DRM_FORMAT_R1)
>> +continue; /* Internal not for userspace */
>
> You already found RGB111 missing here.
>
>
>> +static int gud_usb_control_msg(struct usb_interface *intf, bool in,
>> + u8 request, u16 value, void *buf, size_t len)
> ..
>> +static int gud_usb_transfer(struct gud_device *gdrm, bool in, u8 request,
>> u16 index,
> ..
>> +ret = gud_usb_control_msg(intf, in, request, index, buf, len);
>
> The u16 index parameter to gud_usb_transfer() and at least also
> gud_usb_{get,set,get_u8,set_u8}() is eventually passed in u16 value
> in the call to gud_usb_control_msg(), which had me confused for a bit.
>
> What do you think about renaming all of those parameters to wValue,
> to show that and where they are part of the control request? I think
> it would help make the protocol more clear.
>
Only connector requests use this value and in that case it's the
connector index. I need to get this driver applied now, and can't spend
more time polishing it, so I'll just keep it as-is.
>
> Finally, an actual bug:
>
>> +ret = gud_get_properties(gdrm);
>> +if (ret) {
>> +dev_err(dev, "Failed to get properties (error=%d)\n", ret);
>> +return ret;
>> +}
>
> If gud_get_properties() and gud_connector_add_properties() receive
> and process (only!) one or more unknown properties then they return
> the number of bytes received from the device rather than 0.
>
> I fixed this by setting ret = 0; before the for() loop, but maybe you
> want to do it another way.
>
I'll do that.
>
> I found this because I can't get my device to send 0 bytes IN when
> the host requests more, if I provide no data the request STALLs. This
> is for sure a bug in my device and I'll come back to it, but for now
> I added a dummy 65535 property as a workaround.
>
> What do you think about formalizing this, adding an actual dummy property?
>
I want to avoid that. I'd rather add a descriptor flag
GUD_DISPLAY_FLAG_CANT_RETURN_ZERO or something for such devices.
Also this property warning is just a debug message so not a very big
problem. Worse is EDID if you don't want to support that since it prints
an error in that case.
> Or maybe adding flags to the display descriptor for "I have properties",
> "I have connector properties" and "I have EDID" ?
>
Then I might as well go back to the previous version and have count in
the structs won't I ;)
>
>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> ..
>> +static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer
>> *fb,
> ..
>> +/*
>> + * Imported buffers are assumed to be write-combined and thus uncached
>> + * with slow reads (at least on ARM).
>> + */
>> +if (format != fb->format) {
>> +if (format->format == GUD_DRM_FORMAT_R1) {
>> +len = gud_xrgb_to_r124(buf, format, vaddr, fb,
>> rect);
>> +if (!len) {
>> +ret = -ENOMEM;
>> +goto end_cpu_access;
>> +}
>> +} else if (format->format == DRM_FORMAT_RGB565) {
>> +drm_fb_xrgb_to_rgb565(buf, vaddr, fb, rect,
>> gud_is_big_endian());
>> +} else {
>> +len = gud_xrgb_to_color(buf, format, vaddr, fb,
>> rect);
>> +}
>
> Does this section also need a RGB111 case?
>
gud_xrgb_to_color() handles that, although broken in this version
for partial updates. Here's the fix:
https://gist.github.com/notro/a94d381cf98b7e15fcddbc90e4af0a21
>
>> +void gud_pipe_update(struct drm_simple_display_pipe *pipe,
> ..
>> +if (fb && (crtc->state->mode_changed ||
>> crtc->state->connectors_changed))
>> +gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
>
> You mentioned that commit must not fail; what happens/should happen
> if a request does fail in pipe_update()? Some reasons could be that
> the device was unplugged, a bad cable is glitchy or even that some
> device doesn't even implement STATE_COMMIT or does it incorrectly and
> will report back failure?
>
The failure is just ignored since there's nothing to be done. The error
counter for the debufgs file is increased, making it somewhat visible.
I've chosen to only print probe errors, flush errors and devices
returning incorrect reply lengths. I hate drivers that fill the logs
with errors, but I might have gone to far on the silent side, I don't
know. But one nice thing about DRM is that debug is always builtin and
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Hello Noralf,
I've made some progress with my test device. I'm implementing R1
first and once that works I'll test RGB111 as well. Along the way
I've found a couple of things in the code:
Noralf Trønnes wrote:
> +++ b/drivers/gpu/drm/gud/gud_drv.c
..
> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id
> *id)
..
> + if (format == GUD_DRM_FORMAT_R1)
> + continue; /* Internal not for userspace */
You already found RGB111 missing here.
> +static int gud_usb_control_msg(struct usb_interface *intf, bool in,
> +u8 request, u16 value, void *buf, size_t len)
..
> +static int gud_usb_transfer(struct gud_device *gdrm, bool in, u8 request,
> u16 index,
..
> + ret = gud_usb_control_msg(intf, in, request, index, buf, len);
The u16 index parameter to gud_usb_transfer() and at least also
gud_usb_{get,set,get_u8,set_u8}() is eventually passed in u16 value
in the call to gud_usb_control_msg(), which had me confused for a bit.
What do you think about renaming all of those parameters to wValue,
to show that and where they are part of the control request? I think
it would help make the protocol more clear.
Finally, an actual bug:
> + ret = gud_get_properties(gdrm);
> + if (ret) {
> + dev_err(dev, "Failed to get properties (error=%d)\n", ret);
> + return ret;
> + }
If gud_get_properties() and gud_connector_add_properties() receive
and process (only!) one or more unknown properties then they return
the number of bytes received from the device rather than 0.
I fixed this by setting ret = 0; before the for() loop, but maybe you
want to do it another way.
I found this because I can't get my device to send 0 bytes IN when
the host requests more, if I provide no data the request STALLs. This
is for sure a bug in my device and I'll come back to it, but for now
I added a dummy 65535 property as a workaround.
What do you think about formalizing this, adding an actual dummy property?
Or maybe adding flags to the display descriptor for "I have properties",
"I have connector properties" and "I have EDID" ?
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
..
> +static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer
> *fb,
..
> + /*
> + * Imported buffers are assumed to be write-combined and thus uncached
> + * with slow reads (at least on ARM).
> + */
> + if (format != fb->format) {
> + if (format->format == GUD_DRM_FORMAT_R1) {
> + len = gud_xrgb_to_r124(buf, format, vaddr, fb,
> rect);
> + if (!len) {
> + ret = -ENOMEM;
> + goto end_cpu_access;
> + }
> + } else if (format->format == DRM_FORMAT_RGB565) {
> + drm_fb_xrgb_to_rgb565(buf, vaddr, fb, rect,
> gud_is_big_endian());
> + } else {
> + len = gud_xrgb_to_color(buf, format, vaddr, fb,
> rect);
> + }
Does this section also need a RGB111 case?
> +void gud_pipe_update(struct drm_simple_display_pipe *pipe,
..
> + if (fb && (crtc->state->mode_changed ||
> crtc->state->connectors_changed))
> + gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
You mentioned that commit must not fail; what happens/should happen
if a request does fail in pipe_update()? Some reasons could be that
the device was unplugged, a bad cable is glitchy or even that some
device doesn't even implement STATE_COMMIT or does it incorrectly and
will report back failure?
> +++ b/include/drm/gud.h
..
> + #define GUD_STATUS_REQUEST_NOT_SUPPORTED 0x02
Maybe this can be removed? SET_VERSION has been removed so it's no
longer used anywhere, and in any case devices typically signal that
requests are unsupported using a protocol STALL, which comes back
as -EPIPE from the USB stack.
Finally, here's the drm debug output when I connect my device:
Mar 09 14:57:19 vm kernel: usb 1-1: new full-speed USB device number 24 using
uhci_hcd
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_probe] version=1 flags=0x2
compression=0x0 max_buffer_size=0
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get:
request=0x40 index=0 len=32
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get:
request=0x41 index=0 len=320
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_probe] Ignoring unknown
property: 65535
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get:
request=0x50 index=0 len=256
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] Connector:
index=0 type=0 flags=0x0
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_usb_transfer] get:
request=0x51 index=0 len=320
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] property:
65535 = 0(0x0)
Mar 09 14:57:19 vm kernel: gud 1-1:27.0: [drm:gud_get_connectors] Ignoring
unknown property:
Re: [PATCH v7 3/3] drm: Add GUD USB Display driver
Den 05.03.2021 17.31, skrev Noralf Trønnes:
> This adds a USB display driver with the intention that it can be
> used with future USB interfaced low end displays/adapters. The Linux
> gadget device driver will serve as the canonical device implementation.
>
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id
> *id)
> +{
> + num_formats_dev = ret;
> + for (i = 0; i < num_formats_dev; i++) {
> + const struct drm_format_info *info;
> + size_t fmt_buf_size;
> + u32 format;
> +
> + format = gud_to_fourcc(formats_dev[i]);
> + if (!format) {
> + drm_dbg(drm, "Unsupported format: 0x%02x\n",
> formats_dev[i]);
> + continue;
> + }
> +
> + if (format == GUD_DRM_FORMAT_R1)
> + info = _drm_format_r1;
> + else if (format == GUD_DRM_FORMAT_RGB111)
> + info = _drm_format_rgb111;
> + else
> + info = drm_format_info(format);
> +
> + switch (format) {
> + case GUD_DRM_FORMAT_R1:
> + fallthrough;
> + case GUD_DRM_FORMAT_RGB111:
> + xrgb_emulation_format = info;
> + break;
> + case DRM_FORMAT_RGB565:
> + rgb565_supported = true;
> + if (!xrgb_emulation_format)
> + xrgb_emulation_format = info;
> + break;
> + case DRM_FORMAT_XRGB:
> + xrgb_supported = true;
> + break;
> + };
> +
> + fmt_buf_size = drm_format_info_min_pitch(info, 0,
> drm->mode_config.max_width) *
> +drm->mode_config.max_height;
> + max_buffer_size = max(max_buffer_size, fmt_buf_size);
> +
> + if (format == GUD_DRM_FORMAT_R1)
This line should be:
if (format == GUD_DRM_FORMAT_R1 || format ==
GUD_DRM_FORMAT_RGB111)
There's also a bug in the format conversion functions that shows up on
transfers with widths that are not pixels per byte aligned. I've fixed
R1, I'll look at RGB111 tomorrow.
Noralf.
> + continue; /* Internal not for userspace */
> +
> + formats[num_formats++] = format;
> + }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
