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

[Peter Stuge]

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_xrgb8888_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_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;


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?

Someone may want to add the bit fiddling to output actual RGB111 data
later, then that name would be occupied, and XRGB1111 would describe
the current data format more accurately.


With the two ret=0 fixes, your gist and the three things above addressed
I'm happy to say:

Reviewed-By: Peter Stuge <peter at stuge.se>


R1 and RGB111 are: (if v8 perhaps for a separate RGB commit)

Tested-By: Peter Stuge <peter at stuge.se>



Later I think there's good potential for performance optimization. I
noticed that the drm pipe flush submits the frame as 64 byte bulk
transfers, which becomes slow. The USB stack can take at least 16kb
if not far more and will split as needed. The split into single
64-byte bulk packets is even done by the host controller hardware
if it is given the chance. Some numbers:

usbmon during drm flush of 400x240 RGB111 (48000 byte) shows 750ms
from first submit to last complete:

c1b883c0 0.503674 S Bo:4:022:1 - 64 =
4f7f2900 1.254213 C Bo:4:022:1 0 64 >

A simple userspace program with one libusb_bulk_transfer() call needs
just 300ms for the same data (libusb splits it into 3x16kb transfers):

$ time ./bo < test266.xrgb1111
will output 48000 bytes

real    0m0.298s
user    0m0.002s
sys     0m0.008s

$ 

The display is slow, but the difference is noticeable. Something for later.


Kind regards

//Peter