Re: gud: set PATH connector property
Simon Ser wrote: > > > Would it be possible to set the PATH connector property based on the > > > USB port used by gud? > > > > Sadly not really easily. > > > > The physical topology underneath each host controller is stable but > > bus numbers (usb1, usb2 etc.) are not. > > Oh, that's news to me. So if I unplug and replug a USB device, the bus > number and bus device number might change? The bus number is stable as long as the bus (host controller) exists. > Or does this happen after a power-cycle? Or is this hardware-specific? Consider a host controller on a plug-in card, like ExpressCard (usb1) and perhaps Thunderbolt (usb2) for a more modern example. The bus on each new host controller gets the next available bus number. Plug ExpressCard before Thunderbolt to get the order above. Unplug both (usb1+usb2 disappear) then plug Thunderbolt back in before ExpressCard; now Thunderbolt is usb1 and ExpressCard usb2. > > For onboard host controllers it could be possible to anchor to a > > PCI or platform bus device. > > But the PCI bus device isn't enough I think? If I have a onboard host > controller with 2 physical USB ports, the PCI bus device isn't enough > to tell these 2 physical ports apart? *Only* the PCI bus device is not enough, but it could serve as a unique anchor and some host controller-specific information (e.g. root port number) can be added to that. > > How about using e.g. the serial number of the gud USB device instead > > of host topology, or maybe some other information from the panel > > behind it? > > The PATH property is really about the port path, not the sink. IOW, if > I have two USB displays, one USB port, and switch between the two > displays on this one port, the PATH property isn't supposed to change. Hmm. What is "port path" supposed to mean across a hot-pluggable bus? Should PATH refer to the one ExpressCard host controller if it moves between slots (assuming a computer with two slots) or should it rather refer to "the upper USB port on the right hand side ExpressCard" so I can insert another brand host controller in that slot without PATH changing? //Peter
Re: gud: set PATH connector property
Hi Simon, Simon Ser wrote: > Would it be possible to set the PATH connector property based on the > USB port used by gud? Sadly not really easily. The physical topology underneath each host controller is stable but bus numbers (usb1, usb2 etc.) are not. For onboard host controllers it could be possible to anchor to a PCI or platform bus device. But busses on expansion cards can't be recognized so easily without using maybe serial numbers - which may be cloned across multiple devices - we can't know. > This would give user-space a persistent identifier for the connector: > if the user plugs in a USB display on a given port, the PATH would be > the same even if the machine rebooted or the displays were plugged in > in a different order. How about using e.g. the serial number of the gud USB device instead of host topology, or maybe some other information from the panel behind it? //Peter
Re: [PATCH 2/7] drm/format-helper: Add drm_fb_xrgb8888_to_rgb332()
Daniel Vetter wrote: > Also I just realized we've totally ignored endianess on these, which is > not great, because strictly speaking all the drm_fourcc codes should be > little endian. But I'm also not sure that's worth fixing ... We discussed framebuffer endianess when introducing the driver, in the thread linked near the FIXME comment in the code. I proposed an untested fix but Noralf wanted to wait for testing, which I find fair. I don't think anyone has tested on BE yet. It's on my nice-to-have list, but not at the top, and has blockers, so if anyone else can test on BE please do. I'd recommend testing with an actual device to compare LE and BE behavior easily. Kind regards //Peter
Re: [PATCH] drm/omap: Depend on CONFIG_OF
Laurent Pinchart wrote: > +++ b/drivers/gpu/drm/omapdrm/Kconfig > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > config DRM_OMAP > tristate "OMAP DRM" > - depends on DRM > + depends on DRM && OF > depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM > select OMAP2_DSS > select DRM_KMS_HELPER Would it make sense to select OF instead? //Peter
Re: [PATCH 2/2] drm/gud: Add async_flush module parameter
Noralf Trønnes wrote: > Provide a way for userspace to choose synchronous flushing/pageflips. > This helps save CPU and power. > > It is also useful for test scripts since userspace can know when a flush > has happended and wait before doing the next visual test. > > Cc: Peter Stuge > Cc: Linus Walleij > Signed-off-by: Noralf Trønnes Reviewed-by: Peter Stuge
Re: [PATCH 1/2] drm/gud: Add Raspberry Pi Pico ID
Hi Noralf,
Noralf Trønnes wrote:
> Add VID/PID for the Raspberry Pi Pico implementation.
> Source: https://github.com/notro/gud-pico
>
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -660,6 +660,7 @@ static int gud_resume(struct usb_interface *intf)
>
> static const struct usb_device_id gud_id_table[] = {
> { USB_DEVICE_INTERFACE_CLASS(0x1d50, 0x614d, USB_CLASS_VENDOR_SPEC) },
> + { USB_DEVICE_INTERFACE_CLASS(0x16d0, 0x10a9, USB_CLASS_VENDOR_SPEC) },
> { }
> };
A VID/PID isn't neccessarily tied to one implementation; as long as an
implementation is in fact compatible with the driver I consider it okay
to reuse a VID/PID, and the 0x1d50 conditions are met by gud-pico too.
That said, there's no harm in adding another id. :)
Reviewed-by: Peter Stuge
//Peter
Re: [PATCH 2/2] drm/gud: Use scatter-gather USB bulk transfer
Hi Noralf, Noralf Trønnes wrote: > >> +static int gud_usb_bulk(struct gud_device *gdrm, size_t len) .. > >> + timer_setup_on_stack(&ctx.timer, gud_usb_bulk_timeout, 0); > >> + mod_timer(&ctx.timer, jiffies + msecs_to_jiffies(3000)); > >> + > >> + usb_sg_wait(&ctx.sgr); > >> + > >> + if (!del_timer_sync(&ctx.timer)) > >> + ret = -ETIMEDOUT; .. > > Mention in the commit message that sending USB bulk transfers with > > an sglist could be unstable Can you explain a bit about /how/ it is unstable? As you write, usb_bulk_msg() (as used before) has a timeout which is passed to the host controller hardware and implemented there. I haven't used SG with kernel USB but I would expect such a timeout to still be available with SG? > usb_bulk_msg() has builtin timeout handling and during development of > a microcontroller gadget implementation I've triggered this timeout > several times when the uC usb interrupts stopped firing. The device not responding to bulk packets scheduled and sent by the host is a real error /in the device/ and thus not neccessarily something the kernel must handle gracefully.. I think it's quite nice to do so, but one can argue that it's not strictly required. But more importantly: Remember that bulk transfer has no delivery time guarantee. It can take indefinitely long until a bulk transfer is scheduled by the host on a busy bus which is starved with more important things (control, interrupt, iso transfers) - that's not an error at all, and may be indistinguishable from the device not responding to packets actually sent by the host. Having a timeout is important, I just expect the USB SG interface to support it since it is the hardware that times out in the non-SG case. And since this is essentially real time data maybe a shorter timeout is better? 3 seconds seems really long. The timeout must include all latency for a frame, so e.g. 16ms (60 Hz) is too short for sure. But maybe something like 500ms? //Peter
Re: [PATCH v8 3/3] drm: Add GUD USB Display driver
Hi Noralf,
super fair call with the BE testing, let's hope for some testing soonish.
I was thinking about my device doing protocol STALL when I try to
return 0 bytes, and while it *is* a bug in my device, from a standards
point of view it's actually completely valid, if not expected:
--8<-- usb_20.pdf 8.5.3.4 STALL Handshakes Returned by Control Pipes
If the device is unable to complete a command, it returns a STALL in the
Data and/or Status stages of the control transfer. Unlike the case of a
functional stall, protocol stall does not indicate an error with the device.
-->8--
I think it's fair to say that a device can't complete the command
when it has no data to return.
So how about allowing STALL for optional GUD_REQ_GET_:s to mean the same
as a 0 byte response? Should I propose a separate patch for it later?
Noralf Trønnes wrote:
> +++ b/drivers/gpu/drm/gud/gud_connector.c
..
> +static int gud_connector_get_modes(struct drm_connector *connector)
..
> + ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_EDID, connector->index,
> + edid_ctx.buf, GUD_CONNECTOR_MAX_EDID_LEN);
if (ret == -EPIPE)
ret = 0;
> + if (ret > 0 && ret % EDID_LENGTH) {
> + gud_conn_err(connector, "Invalid EDID size", ret);
> + } else if (ret > 0) {
> + edid_ctx.len = ret;
> + edid = drm_do_get_edid(connector, gud_connector_get_edid_block,
> &edid_ctx);
> + }
> +static int gud_connector_add_properties(struct gud_device *gdrm, struct
> gud_connector *gconn)
..
> + ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_PROPERTIES,
> connector->index,
> + properties, GUD_CONNECTOR_PROPERTIES_MAX_NUM *
> sizeof(*properties));
if (ret == -EPIPE)
ret = 0;
> + if (ret <= 0)
> + goto out;
> + if (ret % sizeof(*properties)) {
> + ret = -EIO;
> + goto out;
> + }
> +++ b/drivers/gpu/drm/gud/gud_drv.c
..
..
> +static int gud_get_properties(struct gud_device *gdrm)
..
> + ret = gud_usb_get(gdrm, GUD_REQ_GET_PROPERTIES, 0,
> + properties, GUD_PROPERTIES_MAX_NUM *
> sizeof(*properties));
if (ret == -EPIPE)
ret = 0;
> + if (ret <= 0)
> + goto out;
> + if (ret % sizeof(*properties)) {
> + ret = -EIO;
> + goto out;
> + }
Then I looked whether a device could cause trouble in the driver by
returning complex/unexpected data, and found this:
> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id
> *id)
..
> + /* Add room for emulated XRGB */
> + formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1,
> sizeof(*formats), GFP_KERNEL);
It looks like this +1 and the way xrgb_emulation_format works means
that an interface will not always work correctly if multiple emulated
formats (R1, XRGB, RGB565) are returned, because only one emulated
mode is added after the loop, with struct drm_format_info for the last
emulated format returned by the device. So userspace would only see the
last emulated mode and the bulk output would only ever use that
particular pixel format, any earlier ones would be unavailable?
If this is EWONTFIX then how about adding an error message if multiple
emulated modes are returned and ignore all but the first, rather than
all but the last?
Related: Can userspace find out which GUD_PIXEL_FORMAT_* is behind an
emulated format? It's needed to decide how the emulated framebuffer
should be used, in particular to not use G or B if GUD_PIXEL_FORMAT_R1.
> + switch (format) {
> + case GUD_DRM_FORMAT_R1:
> + fallthrough;
> + case GUD_DRM_FORMAT_XRGB:
> + if (!xrgb_emulation_format)
> + xrgb_emulation_format = info;
> + break;
> + case DRM_FORMAT_RGB565:
> + rgb565_supported = true;
> + if (!xrgb_emulation_format)
> + xrgb_emulation_format = info;
> + break;
Could RGB565 go before XRGB111 (or R1) and also fallthrough; in this
construct? Not terribly important, but the repetition caught my eye.
Then, in gud_connector.c I saw this, which surprised me:
+int gud_connector_fill_properties(struct drm_connector_state *connector_state,
..
+ if (prop == GUD_PROPERTY_BACKLIGHT_BRIGHTNESS) {
+ val = connector_state->tv.brightness;
+ } else {
Why is this using tv.brightness rather than say gconn->initial_brightness?
It looks like the end result might be the same because tv.brightness is
set to gconn->initial_brightness in gud_connector_reset() but it's a
little confusing to me, since a GUD backlight isn't a drm/TV thing?
Thanks a lot
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.fr
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
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
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
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
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 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? Someone may want to add the bit fiddling to output actual RGB111 data later, then that name would be occupied, and XRGB 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 R1 and RGB111 are: (if v8 perhaps for a separate RGB commit) Tested-By: Peter Stuge 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.xrgb will output 48000 bytes real0m0.298s user0m0.002s sys 0m0.008s $ The display is slow, but the difference is noticeable. Something for later. Kind regards //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
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: 6553
Re: [PATCH v6 3/3] drm: Add GUD USB Display driver
Hi Noralf, Peter Stuge wrote: > I'll prepare a test setup for the RGB-TFT on the weekend. So implementing a GUD and looking at the protocol from yet another angle gives more new insights - surprise. :) Here are some thoughts so far: * GUD_REQ_SET_VERSION does still rub me wrong; it seems potentially quite complex to maintain compatibility in two places; both for host and device. I don't want to insist on removing it, but at a minimum it's quite unusual. Part of the idea in USB is that host software updates are easy if not fully automated but device firmware updates less so, thus complexity is rather placed in the host. * It's unclear to me from reading the header files in this v6 patch set which GUD_REQ:s and which properties are actually mandatory in devices. I'm getting hints from your STM32 device and reading the driver code in detail, but what do you think is a good way to document what's required vs. optional? * GUD_REQ_SET_BUFFER my old nemesis. :) It's great that it's optional! But do you see any way to turn it into a bulk message, in order to remove the memory barrier effect of a control transfer before bulk? I think it would be possible to noticeably improve performance later, by changing the host driver to submit asynchronous bulk transfers for frame data rather than waiting for each transfer to finish; bulk transfers will then pack optimally on the wire - but with a control transfer in between there's no chance of achieving that. Having only one kind of transfer in the hot path would also simplify canceling still pending transfers (when using async later) if new data gets flushed before the previous frame is completely transfered. * A fair bit of the EDID isn't used or has dummy values. Have you already considered and dismissed some other ways of accomplishing the same? * Sorry if I've asked before - but what's the purpose of GUD_REQ_SET_STATE_CHECK and GUD_REQ_SET_STATE_COMMIT? Why/when does drm do pipe check vs. update? * How do you feel about passing the parameters for GUD_REQ_SET_CONTROLLER_ENABLE and GUD_REQ_SET_DISPLAY_ENABLE in wValue? It would save the transfer data stage. Kind regards //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/3] drm: Add GUD USB Display driver
Noralf Trønnes wrote:
> Peter, please have a look at this diff and see if I'm on the right track
> here: https://gist.github.com/notro/a43a93a3aa0cc75d930890b7b254fc0a
Yes that's exactly what I meant; this way the possibility for contradicting
sizes is eliminated by protocol and not just by implementation - very nice!
Some more comments, sorry if this is just because of ongoing work:
Perhaps the functions taking usb_device + ifnum could take usb_interface
instead - but I don't know if that would simplify or complicate things.
Alan mentioned this idea in similar circumstances in another thread.
I don't feel strongly, but perhaps it's cleaner.
gud_usb_control_msg() now seems almost redundant, maybe it could be removed.
In gud_usb_set() if NULL == buf then that's passed to usb_control_msg()
along with len, which likely crashes if len > 0, so it may be good to
check or enforce that, maybe with else len=0; before the gud_usb_transfer()
call.
Finally a small style note that I'd personally change a few if (ret > 0) {
blocks to have one indent level less and do each check right away, e.g. in
gud_connector_get_modes():
ret = gud_usb_get()
if (ret % EDID_LENGTH) {
drm_err();
} else if (ret > 0) {
edid_ctx.len = ret;
edid = drm_do_get_edid();
}
and later on in the function by the display modes one indent level
could be saved with a goto:
if (ret <= 0)
goto out;
but obviously no huge deal.
In general it's really helpful for device development to see error messages
when the device behaves incorrectly, the "Invalid .. size" errors are great
examples of this, but e.g. gud_get_display_descriptor() returns -EIO without
a message. Maybe there are opportunities for further helpful error messages?
Thanks a lot and kind regards
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/3] drm: Add GUD USB Display driver
Noralf Trønnes wrote:
> > I forgot, but I have a two-tone (black/red) e-ink display here, and I
> > also have a 3-bpp RGB TFT display.
>
> I've been anticipating the need for more formats, but I didn't want to
> add them without having a user. Otherwise I could end up adding stuff
> that would never be used. If you can test, there's no problem adding
> support for more formats now.
That's great! I can't promise to to test the three-color (b/w/r) e-ink
but I'll prepare a test setup for the RGB-TFT on the weekend.
> Building on that I would define a 2 bpp RG format like this in the driver:
>
> static const struct drm_format_info gud_drm_format_rg11 = {
> .format = GUD_DRM_FORMAT_RG11,
> .num_planes = 1,
> .char_per_block = { 1, 0, 0 },
> .block_w = { 4, 0, 0 }, /* 4 pixels per block/byte */
> .block_h = { 1, 0, 0 },
> .hsub = 1,
> .vsub = 1,
> };
>
> And a 3 bpp RGB format like this:
>
> static const struct drm_format_info gud_drm_format_rgb111 = {
> .format = GUD_DRM_FORMAT_RGB111,
> .num_planes = 1,
> .char_per_block = { 1, 0, 0 },
> .block_w = { 2, 0, 0 }, /* 2 pixels per block/byte */
> .block_h = { 1, 0, 0 },
> .hsub = 1,
> .vsub = 1,
> };
I can't really comment; I know next to nothing about the drm subsystem. :)
> The MIPI DBI standard defines 2 ways to transmit 2x 3-bpp pixels in one
> byte (X=pad bit):
> - Option 1: X X R1 G1 B1 R2 G2 B2
> - Option 2: X R1 G1 B1 X R2 G2 B2
>
> So maybe we should have GUD_DRM_FORMAT_RGB111_OPTION1 and
> GUD_DRM_FORMAT_RGB111_OPTION2?
> Or just use option 2 and let the display fix it up if needed?
It would of course be lovely to be able to set up an automated DMA from
a USB endpoint to the panel in the device and not have to touch the data,
but that would require the DRM driver to support all the combinations,
which quickly becomes complicated.
> What format does your 3 bpp display use?
It supports three formats:
- R1 G1 B1 R2 G2 B2
- R1 G1 B1 X R2 G2 B2 X (your option 2)
- R1 R2 R3 (simulated monochrome, same data bit to all three subpixels)
> And then something like this for the conversion function:
>
> static size_t gud_xrgb_to_color(u8 *dst, const struct
All right!
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/3] drm: Add GUD USB Display driver
Hi Noralf, Noralf Trønnes wrote: > The driver supports a one bit monochrome transfer format: R1. This is not > implemented in the gadget driver. It is added in preparation for future > monochrome e-ink displays. I forgot, but I have a two-tone (black/red) e-ink display here, and I also have a 3-bpp RGB TFT display. Should we add maybe R2 and R3? (or R3/R8 for number of colours?) I'm particularly considering the 3-bpp RGB panel for GUD use now, and while it will surely work with say a 16-bit RGB mode many bits will be wasted in the process. What are your thoughts? Would you take a patch for that now, later, never? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/3] drm: Add GUD USB Display driver
Hi Noralf,
Noralf Trønnes wrote:
> +++ b/drivers/gpu/drm/gud/gud_connector.c
..
> +static int gud_connector_get_edid_block(void *data, u8 *buf, unsigned int
> block, size_t len)
..
> + struct gud_connector *gconn = ctx->gconn;
> + size_t start = block * EDID_LENGTH;
> +
> + if (start + len > gconn->edid_len)
> + return -1;
> +
> + if (!block) {
> + struct gud_device *gdrm = to_gud_device(gconn->connector.dev);
> + int ret;
> +
> + /* Check because drm_do_get_edid() will retry on failure */
> + if (!ctx->buf)
> + ctx->buf = kmalloc(gconn->edid_len, GFP_KERNEL);
> + if (!ctx->buf)
> + return -1;
> +
> + ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_EDID,
> gconn->connector.index,
> + ctx->buf, gconn->edid_len);
..
> + memcpy(buf, ctx->buf + start, len);
Danger, danger?
gconn->edid_len in this call to gud_usb_get() comes from the device in
gud_connector_status_request() where the only validation is that
edid_len % EDID_LENGTH == 0, so a device could write past the buffer
if drm_do_get_edid() passes a buffer smaller than edid_len.
I guess the buffer passed is just 128, EDID_LENGTH, so a malicious
or buggy device could overwrite 64k-128 kernel memory? Ouch!
More generally it's not very typical in USB to report the data size
separately from the data itself, if reporting size explicitly at all.
Sizes can be part of the data structure itself (like in descriptors) but
on the application layer (like here) it's convenient to just decide a
sensible fixed maximum size and let the host try to always transfer
that size while accepting short transfers. Unlike read() a short
transfer only ever happens if and when a device intends for it,
so that's like an in-band handshake but "for free".
Oh, and does/should the GUD EDID change if the panel "behind" the device
CPU on a hotpluggable connector changes? It wouldn't be great to require
GUD driver reprobe in that case. But maybe DRM requires that anyway?
I'm sorry I didn't spot this pattern earlier, I understand that it's late
in the game and that changing it needs the gadget to change as well, but I
do really think this is a worthwhile change throughout the protocol.
And I think it applies to more than EDID, e.g. both GUD and connector
properties, maybe formats, something else?
Unfortunately, the gud_usb_control_msg() check (ret != len) creates a
requirement to know in advance how much data will be transfered.
That could be revised at least for the general case, even if not used
everywhere; maybe something like adding a size_t required_min_len
parameter to gud_usb_control_msg()?
> +static int gud_connector_get_modes(struct drm_connector *connector)
> +{
> + struct gud_connector *gconn = to_gud_connector(connector);
> + struct gud_device *gdrm = to_gud_device(connector->dev);
> + struct gud_connector_get_edid_ctx edid_ctx = {
> + .gconn = gconn,
> + };
> + struct gud_display_mode_req *reqmodes = NULL;
> + unsigned int i, num_modes = 0;
The error path of this function executes "return num_modes" with num_modes
unmodified; ie. 0. Is that intentional?
> +static int gud_connector_add_tv_mode(struct gud_device *gdrm,
..
> + buf_len = num_modes * GUD_CONNECTOR_TV_MODE_NAME_LEN;
> + modes = kmalloc_array(num_modes, sizeof(*modes), GFP_KERNEL);
> + buf = kmalloc(buf_len, GFP_KERNEL);
Maybe moving the buf assignment immediately following the buf_len assignment
would help readability? This is quite minor.
> +static int gud_connector_add_properties(struct gud_device *gdrm, struct
> gud_connector *gconn,
> + unsigned int num_properties)
> +{
> + struct drm_device *drm = &gdrm->drm;
> + struct drm_connector *connector = &gconn->connector;
> + struct gud_property_req *properties;
> + unsigned int i;
> + int ret;
> +
> + gconn->properties = kcalloc(num_properties, sizeof(*gconn->properties),
> GFP_KERNEL);
> + if (!gconn->properties)
> + return -ENOMEM;
> +
> + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL);
> + if (!properties)
> + return -ENOMEM;
I think this error path leaks gconn->properties?
> +int gud_connector_create(struct gud_device *gdrm, unsigned int index)
Most error paths in this function seem to leak both gconn and connector?
> +++ b/drivers/gpu/drm/gud/gud_drv.c
..
> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum, u8 *status)
..
> + ret = gud_usb_control_msg(usb, ifnum, true, GUD_REQ_GET_STATUS, 0, buf,
> sizeof(*buf));
> + *status = *buf;
Maybe make this conditional on 0 == ret.
> +static int gud_set_version(struct usb_device *usb, u8 ifnum, u32 flags, u8
> version)
..
> + if (ret == -EPIPE)
> + return -EPROTONOSUPPORT;
So yeah, this isn't typical, devices usually
Re: [PATCH v5 3/3] drm: Add Generic USB Display driver
Hi Noralf,
I was happy to see v4 - thanks for accepting so much of my feedback -
and I have to say that the new recursive acronym makes me smile! :)
Noralf Trønnes wrote:
> +++ b/drivers/gpu/drm/gud/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config DRM_GUD
> + tristate "GUD USB Display"
> + depends on DRM && USB
> + select LZ4_COMPRESS
Just a thought: Maybe LZ4_COMPRESS should be optional also on the host?
Ie. not select it here and make lz4 code conditional on CONFIG_LZ4_COMPRESS?
> +++ b/drivers/gpu/drm/gud/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +gud-objs := gud_drv.o gud_pipe.o gud_connector.o
Should this be gud-y instead, like in other drm/*/Makefile ?
> +++ b/drivers/gpu/drm/gud/gud_connector.c
..
> +static int gud_connector_atomic_check(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
This always returns 0, so could be void?
> +int gud_connector_create(struct gud_device *gdrm, unsigned int index)
> +{
> + struct gud_connector_descriptor_req desc;
> + struct drm_device *drm = &gdrm->drm;
> + struct gud_connector *gconn;
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> + int ret, connector_type;
> + u32 flags;
> +
> + ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR, index, &desc,
> sizeof(desc));
Watch out for endianness bugs.
I'd suggest to stay with the pattern "little-endian on wire" and add
complexity on the host to deserialize/convert transfered data to native,
but perhaps with some generic method that scales better than explicitly
converting values on every use.
> +++ b/drivers/gpu/drm/gud/gud_drv.c
..
> +static int gud_usb_control_msg(struct usb_device *usb, u8 ifnum, bool in,
> +u8 request, u16 value, void *buf, size_t len)
> +{
> + u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;
> + unsigned int pipe;
> + int ret;
> +
> + if (in) {
> + pipe = usb_rcvctrlpipe(usb, 0);
> + requesttype |= USB_DIR_IN;
> + } else {
> + pipe = usb_sndctrlpipe(usb, 0);
> + requesttype |= USB_DIR_OUT;
The above line seems unneccessary since USB_DIR_OUT is 0 by spec.
> +static int gud_get_display_descriptor(struct usb_interface *interface,
> + struct gud_display_descriptor_req *desc)
> +{
> + u8 ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
> + struct usb_device *usb = interface_to_usbdev(interface);
> + void *buf;
> + int ret;
> +
> + buf = kmalloc(sizeof(*desc), GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = gud_usb_control_msg(usb, ifnum, true, GUD_REQ_GET_DESCRIPTOR, 0,
> buf, sizeof(*desc));
> + memcpy(desc, buf, sizeof(*desc));
> + kfree(buf);
Is buf neccessary here? This isn't the hot path, but less dynamic memory
and copying is always nicer.
> + if (desc->magic != GUD_DISPLAY_MAGIC)
> + return -ENODATA;
It seems like this checks overlooks endianness, which happens very easily.
Maybe it's a good idea to create a function to fix endianness directly
after data transfers?
Such a function could take a pointer to memory and a kind of format
string made up of 'b', 'w', 'l' and 'q' or '1', '2', '4' and '8' to
describe field sizes, and would then convert wlq fields to native
endianness in-place.
Or are there some parts of the code that could really benefit from
keeping wire-endian values in host memory?
> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum, u8 *status)
> +{
> + u8 *buf;
> + int ret;
> +
> + buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = gud_usb_control_msg(usb, ifnum, true, GUD_REQ_GET_STATUS, 0, buf,
> sizeof(*buf));
> + *status = *buf;
> + kfree(buf);
Ouch, kmalloc for a single byte here, this is the extreme case! :)
If it's not cool to transfer data directly through to the provided pointer
then how about bouncing onto a stack variable rather than kmalloc memory?
Ie:
u8 val;
gud_usb_control_msg(.. GUD_REQ_GET_STATUS, 0, &val, sizeof val)
> +static int gud_usb_transfer(struct gud_device *gdrm, bool in, u8 request,
> u16 index,
> + void *buf, size_t len)
> +{
> + struct usb_device *usb = gud_to_usb_device(gdrm);
> + void *trbuf = NULL;
> + int idx, ret;
> +
> + drm_dbg(&gdrm->drm, "%s: request=0x%x index=%u len=%zu\n",
> + in ? "get" : "set", request, index, len);
> +
> + if (!drm_dev_enter(&gdrm->drm, &idx))
> + return -ENODEV;
> +
> + mutex_lock(&gdrm->ctrl_lock);
> +
> + if (buf) {
> + if (in)
> + trbuf = kmalloc(len, GFP_KERNEL);
> + else
> + trbuf = kmemdup(buf, len, GFP_KERNEL);
Also not the hot path, b
Re: udl: Unrecognized vendor firmware descriptor (all zeroes)
Meelis Roos wrote: > Is it something wrong with the device, Linux USB stack or UDL driver? It looks like the device. > [379953.534772] usb 1-3.4: new high-speed USB device number 23 using xhci_hcd > [379953.630502] usb 1-3.4: New USB device found, idVendor=17e9, > idProduct=01e0, bcdDevice= 0.03 > [379953.635493] usb 1-3.4: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [379953.640411] usb 1-3.4: Product: Lenovo Enhanced USB Port Replicator > [379953.645573] usb 1-3.4: Manufacturer: DisplayLink > [379953.650430] usb 1-3.4: SerialNumber: 01>0-128530 > [379953.662128] [drm] vendor descriptor length:e0 data:00 00 00 00 00 00 00 > 00 00 00 00 Do the descriptors perhaps change, if you run lsusb -v for the device a second or two after plugging it in? (Of course they shouldn't, but ..) Also, the device connects as a high-speed (480Mbps) device through xhci (so perhaps on a super-speed port?). Can you try connecting the device to a high-speed-only port and see if it behaves the same? It might be enough to place a high-speed hub between computer and device for that test. Or if you already tested with a high-speed-only port on your xHCI but the device is actually super-speed-capable then can you try connecting it to a super-speed port? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/6] Generic USB Display driver
Noralf Trønnes wrote: > > In all cases, the driver on the host knows/has available how many bytes > > were successfully transfered. > > I was thinking about the device, that it could get out of sync. Let's > say the host sends a 1k framebuffer and half of it gets transferred and > the rest fails for some reason. Lubomir's MCU implementation has an > endpoint max size of 64 bytes and a callback is called for each packet. > If the 1k transfer fails at some point, will the device be able to > detect this and know that the next time the rx callback is called that > this is the start of a new framebuffer update? Ah! No, a device can not detect that the host intended to send more (bulk) packets but e.g. timed out. I can't immediately think of other reasons for a larger transfer to fail, which still allow communication to continue. When the host recognizes a timeout with partial data transfer it could simply send the remaining data in a new transfer. While the device can not detect host intent, the protocol could allow devices to specify requirements, e.g. that the host always sends full frames. In any case, please avoid making a control request mandatory for frame transfer. Because control requests are scheduled differently onto the wire and because they consist of more packets than bulk data, a control request will interrupt a bulk data stream and likely introduce unneccessary latency. If synchronization is always required then I'd suggest to place it inline with frame data, e.g. in the first packet byte. If synchronization is only required in rare cases then a control transfer is probably the better choice, to not waste any inline bytes. But the optimum would be that the device can describe its needs to the host and the host driver ensures that the device always receives the data it needs. Do you think this is possible? Kind regards //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/6] Generic USB Display driver
Hi Noralf, Noralf Trønnes wrote: > I would like to keep the SET_BUFFER request since it will serve as a > syncing point between the host and the device. I'm no USB expert but I > assume that a bulk transfer can fail halfway through and result in the > next update starting where the previous one failed and thus writing > beyond the end of the display buffer. Transfers either succeed completely (possibly after many retries), time out (after zero or more transfered bytes) or fail catastrophically (e.g. from device disconnect). In all cases, the driver on the host knows/has available how many bytes were successfully transfered. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Alan Stern wrote: > > > A gadget driver can STALL in response to a control-OUT data packet, > > > but only before it has seen the packet. > > > > How can it do that for OUT, and IN if possible there too? > > In the way described just above: The gadget driver's SETUP handler tells > the UDC to stall the data packet. > > > Is it related to f->setup() returning < 0 ? > > Yes; the composite core interprets such a value as meaning to STALL > endpoint 0. Thanks a lot for confirming this. > > The spec also allows NAK, but the gadget code seems to not (need to?) > > explicitly support that. Can you comment on this as well? > > If the gadget driver doesn't submit a usb_request then the UDC will > reply with NAK. And thanks for this as well. > > a status request so I can know the result of the operation the device has > > performed. .. > There are two reasonable approaches you could use. One is to have a > vendor-specific control request to get the result of the preceding > operation. .. > The other approach is to send the status data over a bulk-IN endpoint. I've tried to explain a third approach, which I think fits well because the status is only a "Ready" flag - ie. a memory barrier or flow control, to make the host not send data OUT. I'm proposing that the gadget should NAK on data OUT when it isn't Ready, and that the host driver shouldn't query status but simply send data when it has. Per Alans description the NAK happens automatically if the gadget driver has no usb_request pending while it is processing previously received data. On the host that NAK makes the host controller retry automatically until transfer success, timeout or fatal error. > It would have to be formatted in such a way that the host could > recognize it as a status packet and not some other sort of data packet. For host notification of status changes other than Ready I really like such an IN endpoint, but preferably an interrupt endpoint. To avoid the formatting problem each data packet could be one full status message. That way the host would always receive a known data structure. Interrupt packets can be max 64 byte. Noralf, do you think that's enough for everyone in the first version? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Hi Alan, Alan Stern wrote: > > The way I read composite_setup() after try_fun_setup: it calls f->setup() > > when available, and that can return < 0 to stall. > > > > I expect that composite_setup() and thus f->setup() run when the > > SETUP packet has arrived, thus before the data packet arrives, and if > > composite_setup() stalls then the device/function should never see the > > data packet. > > > > For an OUT transaction I think the host controller might still send > > the DATA packet, but the device controllers that I know don't make it > > visible to the application in that case. > > ... > > Are you guys interested in comments from other people who know more > about the kernel and how it works with USB? I am, especially when it comes to the gadget code. > The USB protocol forbids a device from sending a STALL response to a > SETUP packet. The only valid response is ACK. Thus, there is no way > to prevent the host from sending its DATA packet for a control-OUT > transfer. Right; a STALL handshake only after a DATA packet, but a udc could silently drop the first DATA packet if instructed to STALL during SETUP processing. I don't know how common that is for the udc:s supported by gadget, but some MCU:s behave like that. > A gadget driver can STALL in response to a control-OUT data packet, > but only before it has seen the packet. How can it do that for OUT, and IN if possible there too? Is it related to f->setup() returning < 0 ? The spec also allows NAK, but the gadget code seems to not (need to?) explicitly support that. Can you comment on this as well? > Once the driver knows what the data packet contains, the gadget API > doesn't provide any way to STALL the status stage. Thanks. I think this particular gadget driver doesn't need to decide late. Ideally the control transfers can even be avoided. Thanks and kind regards //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Hi Noralf,
Thanks a lot for going into more detail.
Noralf Trønnes wrote:
> > Several Linux/DRM internals have "leaked" into the USB protocol - this
> > should be avoided if you want device implementations other than your
> > gadget, because those internals can change within Linux in the future,
> > while the protocol must not.
> >
>
> That's intentional, I see no point in recreating uapi values that won't
> change:
>
> Linux errno: /include/uapi/asm-generic/errno{-base,}.h
> Pixel formats: include/uapi/drm/drm_fourcc.h
> Connector types and status: include/uapi/drm/drm_mode.h
I understand, and it's good that these are uapi values, but I will still
disagree with using errno and DRM connector types in the USB protocol,
which is a "namespace" of its own.
That is an important point here; GUD is three things: a Linux DRM driver,
a Linux gadget driver and a USB protocol. USB protocols (good ones) are
OS-agnostic.
> >> If the device transfer buffer can't fit an uncompressed framebuffer
> >> update, the update is split up into parts that do fit.
> >
> > Does "device transfer buffer" refer to something like display RAM on
> > the device side? If so, its size is a device implementation detail
> > which shouldn't be exposed over USB.
>
> The reason for exposing this is that the Linux gadget driver needs to
> decompress the transfer into a buffer and the host needs to know how big
> this is (the host can choose to lower this if it can't allocate a
> continuous buffer of this size).
I understand; so it's only required for some compression types - then
it should at least be completely optional, but in any case I find
exposing/having to expose this to be awful USB protocol design and I
hope we can find a better way.
Maybe it's premature anyway? How would you feel about skipping compression
to begin with?
> lz4 (in the kernel) is block compression and can't be used for
> decompressing just a stream of bytes. There is a lz4 frame protocol
> which looks like it could be useful, but it's not available in the
> kernel. I hardly know anything about compression so I'm in no position
> to add that to the kernel. Maybe someone will add it at a later time if
> it is useful.
Why did you choose to use lz4?
Whether compression comes now or later, maybe there is a more suitable
algorithm?
> > The R1 idea is great!
>
> My plan was to remove R1 support from this version of the patchset, but
> I forgot. The reason is that it's cumbersome to test when the gadget
> driver doesn't support it.
Why not support R1 also in the gadget?
> You mention further down that you have use cases for this, do you have a
> timeplan for when you will make use of R1?
No strict plan, but if it helps I could make a demo device and -firmware
without much effort. You mentioned that you would like to have a
microcontroller device for testing?
> >> - Use donated Openmoko USB pid
> >
> > If Linux will be the reference for this protocol then perhaps a PID
> > under the Linux Foundation VID (1d6b) makes more sense?
>
> Do they hand out pid's?
I don't know. :) The root hub drivers each have one.
> To me it's no big deal, it can be added later if someones cares about it.
Okay, hopefully we can do without a PID anyway.
> > But: A PID applies on device level, not to interfaces.
>
> Indeed. This is a USB interface driver though, so it only looks at
> vendor class interfaces. Then it checks if there's a bulk out endpoint,
> if not it returns -ENXIO and the device subsytem moves on to another
> interface driver if any. Next it tries to fetch the display descriptor
> and if not succesful it returns -ENODEV to give another driver a chance.
Thanks for clarifying this flow. It's nice not to require particular
endpoint addresses - that makes the protocol/driver much more generic.
> I have tried my best to let the driver tolerate other vendor class
> interfaces on the device.
Ack, this is clear now.
> I don't understand why PID should not be necessary, I'm using a vendor
> class interface and the driver can't probe all of those, so it has to
> look at specific vid:pid's.
Why can't the driver probe all vendor class interfaces?
To probe fewer interfaces, a criteria other than PID can still be defined,
and doing so would enable immediate plug-and-play for non-gadget and especially
composite devices, without requiring the addition of PIDs in the host driver.
I find this possibility especially attractive for composite devices, which
may already have some VID:PID and a non-GUD primary function/interface that
is handled by another driver, such that a GUD PID effectively can't be adopted
for that device.
One example of such a criteria would be to require that the iInterface
string descriptor contains the (sub)string "Generic USB Display".
> I have tried together with a HID interface and that worked.
Great. Thanks!
> >> +static int gud_get_vendor_descriptor(struct usb_interface *interface,
> >> + struct gud_dr
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Hi Noralf,
Noralf Trønnes wrote:
> This adds a generic USB display driver with the intention that it can be
> used with future USB interfaced low end displays/adapters.
Fun!
> The Linux gadget device driver will serve as the canonical device
> implementation.
That's a great goal, but as proposed it isn't as generic as I would like.
Several Linux/DRM internals have "leaked" into the USB protocol - this
should be avoided if you want device implementations other than your
gadget, because those internals can change within Linux in the future,
while the protocol must not.
> If the device transfer buffer can't fit an uncompressed framebuffer
> update, the update is split up into parts that do fit.
Does "device transfer buffer" refer to something like display RAM on
the device side? If so, its size is a device implementation detail
which shouldn't be exposed over USB.
It's true that the host drives USB communication but the device decides
whether it will accept data or not. If not, it responds with a NAK
handshake in the OUT transaction, and the host controller will then
try to resend the data later, until the transfer timeout given by the
host software expires. Retries are invisible to host software.
The point is: USB has native flow control on the lowest level; that's
far more efficient than anything the application can construct, and
flow control in the application protocol would be redundant.
When using gadgetfs IIRC device controllers NAK as long as the
userspace process doesn't write new data to the ep?out-bulk fd.
Have you tried/seen this?
> The driver supports a one bit monochrome transfer format: R1. This is not
> implemented in the gadget driver. It is added in preparation for future
> monochrome e-ink displays.
The R1 idea is great!
> - Use donated Openmoko USB pid
If Linux will be the reference for this protocol then perhaps a PID
under the Linux Foundation VID (1d6b) makes more sense?
But: A PID applies on device level, not to interfaces.
Until this protocol becomes a USB-IF device class maybe it's better
to create a probe for GUD interface(s) rather than binding to PID?
Does the driver btw. already support a composite device with multiple GUD
interfaces? Say a microcontroller with two independent panels. It seems no?
If yes, would any of the control requests currently sent to the interface
be better directed at the device? If so, then a PID might make sense again,
but it's still not possible to create a composite device which uses this
protocol, without risking collissions with other vendor specific requests
on other (vendor specific) interfaces, that would be a real shame.
I can imagine a composite device wanting to implement HID and GUD,
let's make sure that it's possible.
On to the code.
> +static int gud_drm_usb_control_msg(struct usb_device *usb, u8 ifnum, bool in,
> +u8 request, u16 value, void *buf, size_t len,
> +bool check_len)
> +{
> + u8 requesttype = USB_TYPE_VENDOR | USB_RECIP_INTERFACE;
This takes struct usb_device rather than struct usb_interface - again,
would this actually work with a composite device? The driver doesn't
ever claim the interface so I guess no?
> +static int gud_get_vendor_descriptor(struct usb_interface *interface,
> + struct gud_drm_display_descriptor *desc)
> +{
..
> + ret = gud_drm_usb_control_msg(usb, ifnum, true, USB_REQ_GET_DESCRIPTOR,
> + GUD_DRM_USB_DT_DISPLAY << 8, buf,
> sizeof(*desc), false);
GUD_DRM_USB_DT_DISPLAY is defined as (USB_TYPE_VENDOR | 0x4),
but USB_TYPE_VENDOR only applies to bmRequestType[6:5] in control transfers,
nowhere else. I know of no standardized way to introduce vendor-specific
descriptors. Squatting is possible, but I think it would be nice to do
better here. It is easy enough.
It could be argued that the vendor specific interface gives flexibility here,
but actually it just means that the semantics of the standardized and
well-defined USB_REQ_GET_DESCRIPTOR have been duplicated by this protocol,
that is not very common - but if you want to go ahead then at least drop
USB_TYPE_VENDOR from the GUD_DRM_USB_DT_DISPLAY definition.
Maybe it's good to think about the data exchange some more - anything not
transfered by standardized USB_REQ_GET_DESCRIPTOR (bmRequestType 1000B;
Device-to-host data, Standard type, Device recipient) isn't actually
a descriptor, it's vendor-specific, free-format data. Does that enable
any simplifications?
> +static int gud_usb_get_status(struct usb_device *usb, u8 ifnum)
> +{
> + struct gud_drm_req_get_status *status;
> + int ret, status_retries = 2000 / 5; /* maximum wait ~2 seconds */
> + unsigned long delay = 500;
> +
> + status = kmalloc(sizeof(*status), GFP_KERNEL);
> + if (!status)
> + return -ENOMEM;
> +
> + /*
> + * Poll due to lack of data/status stage control on the gadget side.
I h
Re: [PATCH 0/7] drm/vc4: Allow for more boot-time configuration
Hi, Maxime Ripard wrote: > properties to initialise the overscan or rotation parameters, or the > one to deal with broken displays. How does that work on systems with multiple connectors? On SBCs with only one output I guess it's fine to have a global option, but it may be important for new options to also be usable on more complex systems. And maybe even SPI displays are relevant? Also, some of the forward-ported patches still have your old email address, maybe you want to change that. Thanks //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Lenovo resume from suspends hangs in i915_gpu_busy or so
Norbert Preining wrote: > > Do you see what scrolls by above the text you took a picture of > > More than what I can see on the screen shot I cannot grasp. Maybe try serial console, netconsole or usb debug device. //Peter
Re: Lenovo resume from suspends hangs in i915_gpu_busy or so
Norbert Preining wrote: > > Do you see what scrolls by above the text you took a picture of > > More than what I can see on the screen shot I cannot grasp. Maybe try serial console, netconsole or usb debug device. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[git pull] drm fixes
Michel D?nzer wrote: > You won't have any trouble finding plenty of examples of newcomers > having a better experience, as most of them thankfully don't come in > with such a bad attitude and conduct. To a bystander it would seem that Ilija has great attitude and conduct. //Peter
Re: [git pull] drm fixes
Michel Dänzer wrote: > You won't have any trouble finding plenty of examples of newcomers > having a better experience, as most of them thankfully don't come in > with such a bad attitude and conduct. To a bystander it would seem that Ilija has great attitude and conduct. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] ACPI/Intel: Rework Opregion support
Indan Zupancic wrote: > Everything would be a lot simpler if the BIOSes were open source. coreboot has existed for about eleven years and some 250 mainboards of varying shapes and sizes (from laptop to server) are supported, but it's only just recently that things are really taking off, with the code release from AMD to initialize their most recent Fusion platform. http://www.coreboot.org/Welcome_to_coreboot http://blogs.amd.com/work/2011/02/28/amd-coreboot/ http://news.slashdot.org/story/11/03/04/1736241/AMD-Provides-Fusion-Support-For-Coreboot //Peter
Re: [PATCH] ACPI/Intel: Rework Opregion support
Indan Zupancic wrote: > Everything would be a lot simpler if the BIOSes were open source. coreboot has existed for about eleven years and some 250 mainboards of varying shapes and sizes (from laptop to server) are supported, but it's only just recently that things are really taking off, with the code release from AMD to initialize their most recent Fusion platform. http://www.coreboot.org/Welcome_to_coreboot http://blogs.amd.com/work/2011/02/28/amd-coreboot/ http://news.slashdot.org/story/11/03/04/1736241/AMD-Provides-Fusion-Support-For-Coreboot //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Indan Zupancic wrote: > What I don't understand is how BIOS makers could know about those bits. They have relationships with Intel since 30 years, ie. they get what they need in one form or other. //Peter
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Indan Zupancic wrote: > What I don't understand is how BIOS makers could know about those bits. They have relationships with Intel since 30 years, ie. they get what they need in one form or other. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[BUG] drm/i915 Screen corruption introduced by a00b10c360b35d6431a94cb
Indan Zupancic wrote: > > Confirm I also have this issue on my X40, but there are other bugs > > that are much more significant so I haven't bothered mentioning this. > > What issues? The one I confirmed is corrupted graphics within Gecko. I haven't had Xv working for a long time either. Not sure if I've tried it with this kernel though. > If it's backlight related, try my patch at: > http://lkml.org/lkml/2011/2/16/447 Yeah also have backlight issue whenever the backlight level changes by other means than Fn+Home/End. (Lid switch, screen blank) I noticed the patch and that it solved the issue for someone. I'm not too inconvenienced by this issue though. > Or if the screen is black after suspend/screen blank Oh I've stopped using suspend since using KMS. I get way too angry about all the state that I lose if resume fails so I don't risk it. > Actually, a lot of bugs were recently introduced and fixed, with > two months ago you're probably in the new-bugs-only period, so I > can recommend trying 2.6.38-rc5. Yeah, I think it's time to pull Linus' git. I've been keeping an eye on things i915 on the list for a good while already. > This screen corruption is the only problem for me, but I don't do > anything fancy with my laptop. The ipw2200 wireless driver is quite > crappy, but it has always been as far as I know. I've used ipw2200 with great success for many years, but these days I'm having fun (no, not at all) with ath9k where there is some very fundamental hardware issue between laptop and card. I'd need to hook up logic analyzer to say anything concrete, but I have no end of problems with internal ath9k in my machine. It's completely unusable. The only other annoying issue I have is that as wine enumerates available screen resolutions i915 goes out to the VGA connector, which on 855 always means a 600ms timeout when nothing is connected, but this is a bit tricky because the hardware just can not tell if anything is connected. I would be very happy if there was a knob for enabling/disabling the VGA connector though. > Good luck, Thanks, you too! //Peter
Re: [BUG] drm/i915 Screen corruption introduced by a00b10c360b35d6431a94cb
Indan Zupancic wrote: > > Confirm I also have this issue on my X40, but there are other bugs > > that are much more significant so I haven't bothered mentioning this. > > What issues? The one I confirmed is corrupted graphics within Gecko. I haven't had Xv working for a long time either. Not sure if I've tried it with this kernel though. > If it's backlight related, try my patch at: > http://lkml.org/lkml/2011/2/16/447 Yeah also have backlight issue whenever the backlight level changes by other means than Fn+Home/End. (Lid switch, screen blank) I noticed the patch and that it solved the issue for someone. I'm not too inconvenienced by this issue though. > Or if the screen is black after suspend/screen blank Oh I've stopped using suspend since using KMS. I get way too angry about all the state that I lose if resume fails so I don't risk it. > Actually, a lot of bugs were recently introduced and fixed, with > two months ago you're probably in the new-bugs-only period, so I > can recommend trying 2.6.38-rc5. Yeah, I think it's time to pull Linus' git. I've been keeping an eye on things i915 on the list for a good while already. > This screen corruption is the only problem for me, but I don't do > anything fancy with my laptop. The ipw2200 wireless driver is quite > crappy, but it has always been as far as I know. I've used ipw2200 with great success for many years, but these days I'm having fun (no, not at all) with ath9k where there is some very fundamental hardware issue between laptop and card. I'd need to hook up logic analyzer to say anything concrete, but I have no end of problems with internal ath9k in my machine. It's completely unusable. The only other annoying issue I have is that as wine enumerates available screen resolutions i915 goes out to the VGA connector, which on 855 always means a 600ms timeout when nothing is connected, but this is a bit tricky because the hardware just can not tell if anything is connected. I would be very happy if there was a knob for enabling/disabling the VGA connector though. > Good luck, Thanks, you too! //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[BUG] drm/i915 Screen corruption introduced by a00b10c360b35d6431a94cb
Daniel Vetter wrote: > Please provide the usual details about your system (especially what gpu > this is on). Also, screenshots of what typical corruptions look like can > help a lot in tracking down such things. Confirm I also have this issue on my X40, but there are other bugs that are much more significant so I haven't bothered mentioning this. On the other hand my kernel is nearly two months old. Please don't mistake this as a complaint. I'm overall rather happy even with current state and it's issues, and especially I think the situation is consistently getting better, with some bumps here and there along the way. Last time I used a projector I had some trouble, because the graphics driver was too clever; it automatically set the correct resolution for the projector, but I needed the resolution of my internal screen. :) //Peter
Re: [BUG] drm/i915 Screen corruption introduced by a00b10c360b35d6431a94cb
Daniel Vetter wrote: > Please provide the usual details about your system (especially what gpu > this is on). Also, screenshots of what typical corruptions look like can > help a lot in tracking down such things. Confirm I also have this issue on my X40, but there are other bugs that are much more significant so I haven't bothered mentioning this. On the other hand my kernel is nearly two months old. Please don't mistake this as a complaint. I'm overall rather happy even with current state and it's issues, and especially I think the situation is consistently getting better, with some bumps here and there along the way. Last time I used a projector I had some trouble, because the graphics driver was too clever; it automatically set the correct resolution for the projector, but I needed the resolution of my internal screen. :) //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[git pull] drm fixes
Linus Torvalds wrote: > You should always take for granted that the BIOS is wrong. Better yet, that there is no BIOS. Maybe one happy day, in the future. //Peter
Re: [git pull] drm fixes
Linus Torvalds wrote: > You should always take for granted that the BIOS is wrong. Better yet, that there is no BIOS. Maybe one happy day, in the future. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[stable] [PATCH] drm/kms: remove spaces from connector names
Sergej Pupykin wrote: > > And it's ugly; can't we fix grub instead? > > I am searching for bootloader which can pass whitespaces. It looks > like we should patch grub-legacy (0.97), grub (1.98) and lilo... > > (I did not try lilo yet, but man page says nothing about passing > spaces in 'append=' option description) append can contain any arbitrary string, which will be parsed by the kernel. I e.g. use: append="root=/dev/sda2 snd_ac97_codec.power_save=1 usbcore.autosuspend=1 hpet=force quiet drm_kms_helper.poll=0" And grub also supports an append string with spaces, that is probably used by every distribution. The question is, what will the kernel parser do about that space? //Peter
Re: [stable] [PATCH] drm/kms: remove spaces from connector names
Sergej Pupykin wrote: > > And it's ugly; can't we fix grub instead? > > I am searching for bootloader which can pass whitespaces. It looks > like we should patch grub-legacy (0.97), grub (1.98) and lilo... > > (I did not try lilo yet, but man page says nothing about passing > spaces in 'append=' option description) append can contain any arbitrary string, which will be parsed by the kernel. I e.g. use: append="root=/dev/sda2 snd_ac97_codec.power_save=1 usbcore.autosuspend=1 hpet=force quiet drm_kms_helper.poll=0" And grub also supports an append string with spaces, that is probably used by every distribution. The question is, what will the kernel parser do about that space? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[stable] [PATCH] drm/kms: remove spaces from connector names
Jesse Barnes wrote: > An alternative to fixing grub would be to add aliases like you mention, > and/or change the parser to accept "_" as an alias for " ". Then we > could leave the sysfs values and string table alone. Is it already case insensitive? //Peter
Re: [stable] [PATCH] drm/kms: remove spaces from connector names
Jesse Barnes wrote: > An alternative to fixing grub would be to add aliases like you mention, > and/or change the parser to accept "_" as an alias for " ". Then we > could leave the sysfs values and string table alone. Is it already case insensitive? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 09/10] MCDE: Add build files and bus
Russell King - ARM Linux wrote: > I feel it would be better to allow the current situation to continue. I think this misses the point, and is somewhat redundant; I think everyone knows that it is easiest to never change anything. But then nothing improves. > If we start telling people that they can't use statically declared > devices without first having an alternative, I would consider it early warning that the way things have been done before will go away. And I would thus write drivers in a way that demonstrates and includes that understanding. The same problem exists in hardware products needing any kind of longish lifetime. Deal with evolving components by having clean and simple interfaces, and by not relying on a particular interface very deep on either side of the edge. Simple I think. //Peter
Re: [PATCH 09/10] MCDE: Add build files and bus
Russell King - ARM Linux wrote: > I feel it would be better to allow the current situation to continue. I think this misses the point, and is somewhat redundant; I think everyone knows that it is easiest to never change anything. But then nothing improves. > If we start telling people that they can't use statically declared > devices without first having an alternative, I would consider it early warning that the way things have been done before will go away. And I would thus write drivers in a way that demonstrates and includes that understanding. The same problem exists in hardware products needing any kind of longish lifetime. Deal with evolving components by having clean and simple interfaces, and by not relying on a particular interface very deep on either side of the edge. Simple I think. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915/sdvo: Fix harmless build warning
Dongdong Deng wrote:
> Removing the following harmless build warning to let compiler happy.
> @@ -1170,7 +1170,8 @@ static void intel_sdvo_mode_set(struct drm_encoder
> *encoder,
> switch (sdvo_pixel_multiply) {
> case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break;
> case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break;
> - case 4: rate = SDVO_CLOCK_RATE_MULT_4X; break;
> + case 4:
> + default: rate = SDVO_CLOCK_RATE_MULT_4X;
> }
> if (!intel_sdvo_set_clock_rate_mult(intel_sdvo, rate))
> return;
Even if the default case can not trigger, is 4x a good default multiplier?
Also, shouldn't there be a break; also in the default: case?
//Peter
Re: [PATCH] drm/i915/sdvo: Fix harmless build warning
Dongdong Deng wrote:
> Removing the following harmless build warning to let compiler happy.
> @@ -1170,7 +1170,8 @@ static void intel_sdvo_mode_set(struct drm_encoder
> *encoder,
> switch (sdvo_pixel_multiply) {
> case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break;
> case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break;
> - case 4: rate = SDVO_CLOCK_RATE_MULT_4X; break;
> + case 4:
> + default: rate = SDVO_CLOCK_RATE_MULT_4X;
> }
> if (!intel_sdvo_set_clock_rate_mult(intel_sdvo, rate))
> return;
Even if the default case can not trigger, is 4x a good default multiplier?
Also, shouldn't there be a break; also in the default: case?
//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] intel: Listen for hotplug uevents (V2)
Keith Packard wrote: > This connects the kernel uevent indicating monitor hotplugging to the > RandR notification events so that X applications can be notified > automatically when monitors are connected or disconnected. Are these events actually being generated? If there is the infrastructure to do so, then it seems that the 600ms delay while polling unconnected monitors could easily be removed. Or has that already been done, but just not for the older hardware? //Peter
Re: [PATCH] intel: Listen for hotplug uevents (V2)
Keith Packard wrote: > This connects the kernel uevent indicating monitor hotplugging to the > RandR notification events so that X applications can be notified > automatically when monitors are connected or disconnected. Are these events actually being generated? If there is the infrastructure to do so, then it seems that the 600ms delay while polling unconnected monitors could easily be removed. Or has that already been done, but just not for the older hardware? //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Two 600ms pauses when starting wine on i915
Peter Stuge wrote: > I hope it can be resolved. There was talk about knobs. Maybe a simple kill switch for VGA1? I want my machine to require explicit enable of VGA1 before it will be used anyway. A disable knob would work great for me. :) //Peter
Two 600ms pauses when starting wine on i915
Chris Wilson wrote: > > a few seconds of very irritating stuttering whenever I run wine. > > i915 > > wine triggers an xrandr storm for its own unknown reasons. Same underlying reason as for the 600ms keyboard pauses I saw. One #winehq person claims that ATI and NV drivers track monitor state and cache resolution information, which is what wine queries on startup. > The delay you see on starting is the driver (on wine's behalf) > repeatedly querying the outputs for connected displays It feels like it is being done twice. I'm taking a look in the wine code. I hope it can be resolved. I guess it's this call to XRRSizes(): http://source.winehq.org/git/wine.git/?a=blob;f=dlls/winex11.drv/xrandr.c#l258 Maybe make_modes() is also involved: http://source.winehq.org/git/wine.git/?a=blob;f=dlls/winex11.drv/xrandr.c#l111 //Peter
i915 blocked for more than 120 seconds.
Toralf F?rster wrote: > today I run at the latest 2.6.35.6 kernel few apps like Lotus Notes > under wine, I'm still stuck with 2.6.36-rc1, and both here and on many/most earlier kernels I get a few seconds of very irritating stuttering whenever I run wine. It is definately related to interaction with X. wine folks' reaction was sortof "mh i915 is kinda slow may be it can not do any better" but I think it's remarkably bad. Didn't think to mention it here earlier. I've never seen a problem like Toralf's however, but I also don't use wine for complex GUI things. //Peter
Re: Two 600ms pauses when starting wine on i915
Peter Stuge wrote: > I hope it can be resolved. There was talk about knobs. Maybe a simple kill switch for VGA1? I want my machine to require explicit enable of VGA1 before it will be used anyway. A disable knob would work great for me. :) //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Two 600ms pauses when starting wine on i915
Chris Wilson wrote: > > a few seconds of very irritating stuttering whenever I run wine. > > i915 > > wine triggers an xrandr storm for its own unknown reasons. Same underlying reason as for the 600ms keyboard pauses I saw. One #winehq person claims that ATI and NV drivers track monitor state and cache resolution information, which is what wine queries on startup. > The delay you see on starting is the driver (on wine's behalf) > repeatedly querying the outputs for connected displays It feels like it is being done twice. I'm taking a look in the wine code. I hope it can be resolved. I guess it's this call to XRRSizes(): http://source.winehq.org/git/wine.git/?a=blob;f=dlls/winex11.drv/xrandr.c#l258 Maybe make_modes() is also involved: http://source.winehq.org/git/wine.git/?a=blob;f=dlls/winex11.drv/xrandr.c#l111 //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 blocked for more than 120 seconds.
Toralf Förster wrote: > today I run at the latest 2.6.35.6 kernel few apps like Lotus Notes > under wine, I'm still stuck with 2.6.36-rc1, and both here and on many/most earlier kernels I get a few seconds of very irritating stuttering whenever I run wine. It is definately related to interaction with X. wine folks' reaction was sortof "mh i915 is kinda slow may be it can not do any better" but I think it's remarkably bad. Didn't think to mention it here earlier. I've never seen a problem like Toralf's however, but I also don't use wine for complex GUI things. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Chris Wilson wrote: > > > > Xv performance > > > > > > That should now be fixed in -intel, > > > > Sounds good! I'd love to test. Which branch/commit is that? > > Master with -vo xv should be good, if it doesn't hang. Doesn't hang, mplayer says: X11 error: BadAlloc (insufficient resources for operation) X11 error: BadAlloc (insufficient resources for operation) X11 error: BadAlloc (insufficient resources for operation) over and over, with a window just showing the blue overlay keying. I'm still working on the merge with your overlay branch. > > Incidentally, apropos the backlight commit > > That wasn't meant to be pushed. :( I was working on rectifying lack > of backlight controls in on T61 and trying to reduce the number of > pieces of code trying to accomplish the same thing through > different means. I thought that work was contained on the T61... It could be - the resetting backlight thing happened also before the backlight commit. Before pulling I was using the directly preceding commit. //Peter
[REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Hi again, Chris Wilson wrote: > > In general, Xv performance with KMS leaves me with a feeling that > > something is not quite right in terms of playback, both with mplayer > > and vlc. But while movies are nice, this stalling issue is more > > important. > > That should now be fixed in -intel, Sounds good! I'd love to test. Which branch/commit is that? I just pulled and am using 68a5ad4. mplayer -vo gl or gl2 works but uses sw rendering and is very slow. Incidentally, apropos the backlight commit in xf86-video-intel, it seems like the driver always sets the backlight to 0. Whenver I start X, backlight goes to minimum. I can crank it up with Fn+Home, but if I restart X then it goes back down again. /proc/acpi/ibm/brightness is always accurate, showing 0 after X starts. > However, we have we replaced it with a worse bug... Oh well.. > > I get the stall precisely every 10 seconds. > > This is caused by the hotplug polling code taking around 600ms to > determine that the VGA is not connected. Ouch. Could it rely on DDC? Maybe a quick DDC query could be done first, and if that seems to indicate that something is connected only then do the heavy work reading the border color? > https://bugs.freedesktop.org/show_bug.cgi?id=29536 Lovely! The patch works well for me, booting with drm_kms_helper.poll=0 on the kernel command line, and I'm back in X now. I need to sort out some keyboard issues, but they are unrelated. The commit message mentions runtime switching of the polling - how is that done? As I understand it I might have to do that in order to run xrandr e.g. to set up a projector, or possibly after - or no? > > I've just upgraded my userspace and have xorg-server-1.8.2. No Xv > > image with either xf86-video-intel driver. I haven't tried this > > server with the 07-21 kernel to find out if Xv works there. The > > stalling is more important. > > https://bugs.freedesktop.org/show_bug.cgi?id=29574 (I think, Daniel > reported something closer to your issue on IRC, but I don't have an > actual bug for that.) Disabling polling made no difference for Xv. I will try to merge your overlay branch. > > drm-intel.git (Should I look elsewhere?) > > No, sounds like you've hit almost all of the recent 855 regressions. .36-rc2 is the first kernel in what seems like a long time that feels like a solid improvement, even if there are still some issues. Well done! > But you have a cursor, that is more than most people! Funny you should say that! :) Actually I *don't* have a cursor in X on cold boot, but it shows up after suspend+resume. The cursor I mentioned is the VT blinking one. > very pessimistic guess for the memory fetch Aha. Thanks for the explanation! //Peter
[REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Peter Stuge wrote: > Hello, new on list but not to the kernel. I also have the issue on > ThinkPad X40. As another data point, it seems that there was a similar problem with drm/radeon: https://bugs.freedesktop.org/show_bug.cgi?id=28411 [Bug 28411] Output polling causes latency every 10 seconds //Peter
[REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Hello, new on list but not to the kernel. I also have the issue on ThinkPad X40. 00:02.0 VGA compatible controller: Intel Corporation 82852/855GM Integrated Graphics Device (rev 02) (prog-if 00 [VGA controller]) Subsystem: IBM Device 0557 Flags: bus master, fast devsel, latency 0, IRQ 16 Memory at e000 (32-bit, prefetchable) [size=128M] Memory at d000 (32-bit, non-prefetchable) [size=512K] I/O ports at 1800 [size=8] Expansion ROM at [disabled] Capabilities: [d0] Power Management version 1 Kernel driver in use: i915 I've been running KMS and drm-intel.git since around 2.6.30, roughly the start of this year, and I've had lots of "fun" with KMS. :p I'm very positive to the KMS concept but it's been a bumpy road so far. In general, Xv performance with KMS leaves me with a feeling that something is not quite right in terms of playback, both with mplayer and vlc. But while movies are nice, this stalling issue is more important. drm-intel.git as of 07-21 has sortof worked for me, but could not suspend. Later drm-intel.git would come up with the panel disabled, so basically make the system unusable. I've been using 07-21 for about a month, and just avoiding suspending. I'm now running drm-intel.git/drm-intel-next (which seems to not get the activity first?) and the latest commit is Linus 2.6.36-rc2. I've merged wireless-testing.git on top of this for ath9k woes. I get the stall precisely every 10 seconds. In VT mode the system stalls for about a second (the cursor stops blinking) and if I'm typing during the stall then my keypresses appear correctly after the stall ends. No errors in dmesg. Should I set drm.debug to something? If I'm running X (I've tried xf86-video-intel 2.12.0 and current .git master) then my keypresses appear twice after the stall ends, but honouring shift. If I hold shift, type X, release shift during a stall then I'll get "Xx" after the stall ends. This makes X practically unusable for me, so I'm at a plain VT for now. Suspend works both in VT and in X, so some things have definately improved for me since 07-21. :) I've just upgraded my userspace and have xorg-server-1.8.2. No Xv image with either xf86-video-intel driver. I haven't tried this server with the 07-21 kernel to find out if Xv works there. The stalling is more important. Chris' patch to i915_irq.c disabling wake-ups has no effect on the stalls in VT mode for me. I haven't bisected, would that be a big help? I'll probably have to spend a fair amount of time on that, since the latest previous kernel I could get to start is over a month old. I've tried drm-intel.git a couple of times over this month, but for long periods of time there were no changes. (Should I look elsewhere?) I also get intermittent FIFO errors, but I don't know how relevant they are for this issue. It seems like the stalling is a very distinct issue. Please let me know how I can help fix this. //Peter
Re: [REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Chris Wilson wrote: > > > > Xv performance > > > > > > That should now be fixed in -intel, > > > > Sounds good! I'd love to test. Which branch/commit is that? > > Master with -vo xv should be good, if it doesn't hang. Doesn't hang, mplayer says: X11 error: BadAlloc (insufficient resources for operation) X11 error: BadAlloc (insufficient resources for operation) X11 error: BadAlloc (insufficient resources for operation) over and over, with a window just showing the blue overlay keying. I'm still working on the merge with your overlay branch. > > Incidentally, apropos the backlight commit > > That wasn't meant to be pushed. :( I was working on rectifying lack > of backlight controls in on T61 and trying to reduce the number of > pieces of code trying to accomplish the same thing through > different means. I thought that work was contained on the T61... It could be - the resetting backlight thing happened also before the backlight commit. Before pulling I was using the directly preceding commit. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Hi again, Chris Wilson wrote: > > In general, Xv performance with KMS leaves me with a feeling that > > something is not quite right in terms of playback, both with mplayer > > and vlc. But while movies are nice, this stalling issue is more > > important. > > That should now be fixed in -intel, Sounds good! I'd love to test. Which branch/commit is that? I just pulled and am using 68a5ad4. mplayer -vo gl or gl2 works but uses sw rendering and is very slow. Incidentally, apropos the backlight commit in xf86-video-intel, it seems like the driver always sets the backlight to 0. Whenver I start X, backlight goes to minimum. I can crank it up with Fn+Home, but if I restart X then it goes back down again. /proc/acpi/ibm/brightness is always accurate, showing 0 after X starts. > However, we have we replaced it with a worse bug... Oh well.. > > I get the stall precisely every 10 seconds. > > This is caused by the hotplug polling code taking around 600ms to > determine that the VGA is not connected. Ouch. Could it rely on DDC? Maybe a quick DDC query could be done first, and if that seems to indicate that something is connected only then do the heavy work reading the border color? > https://bugs.freedesktop.org/show_bug.cgi?id=29536 Lovely! The patch works well for me, booting with drm_kms_helper.poll=0 on the kernel command line, and I'm back in X now. I need to sort out some keyboard issues, but they are unrelated. The commit message mentions runtime switching of the polling - how is that done? As I understand it I might have to do that in order to run xrandr e.g. to set up a projector, or possibly after - or no? > > I've just upgraded my userspace and have xorg-server-1.8.2. No Xv > > image with either xf86-video-intel driver. I haven't tried this > > server with the 07-21 kernel to find out if Xv works there. The > > stalling is more important. > > https://bugs.freedesktop.org/show_bug.cgi?id=29574 (I think, Daniel > reported something closer to your issue on IRC, but I don't have an > actual bug for that.) Disabling polling made no difference for Xv. I will try to merge your overlay branch. > > drm-intel.git (Should I look elsewhere?) > > No, sounds like you've hit almost all of the recent 855 regressions. .36-rc2 is the first kernel in what seems like a long time that feels like a solid improvement, even if there are still some issues. Well done! > But you have a cursor, that is more than most people! Funny you should say that! :) Actually I *don't* have a cursor in X on cold boot, but it shows up after suspend+resume. The cursor I mentioned is the VT blinking one. > very pessimistic guess for the memory fetch Aha. Thanks for the explanation! //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Peter Stuge wrote: > Hello, new on list but not to the kernel. I also have the issue on > ThinkPad X40. As another data point, it seems that there was a similar problem with drm/radeon: https://bugs.freedesktop.org/show_bug.cgi?id=28411 [Bug 28411] Output polling causes latency every 10 seconds //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[REGRESSION, i915]: Periodic stalls with 2.6.36-rc2
Hello, new on list but not to the kernel. I also have the issue on ThinkPad X40. 00:02.0 VGA compatible controller: Intel Corporation 82852/855GM Integrated Graphics Device (rev 02) (prog-if 00 [VGA controller]) Subsystem: IBM Device 0557 Flags: bus master, fast devsel, latency 0, IRQ 16 Memory at e000 (32-bit, prefetchable) [size=128M] Memory at d000 (32-bit, non-prefetchable) [size=512K] I/O ports at 1800 [size=8] Expansion ROM at [disabled] Capabilities: [d0] Power Management version 1 Kernel driver in use: i915 I've been running KMS and drm-intel.git since around 2.6.30, roughly the start of this year, and I've had lots of "fun" with KMS. :p I'm very positive to the KMS concept but it's been a bumpy road so far. In general, Xv performance with KMS leaves me with a feeling that something is not quite right in terms of playback, both with mplayer and vlc. But while movies are nice, this stalling issue is more important. drm-intel.git as of 07-21 has sortof worked for me, but could not suspend. Later drm-intel.git would come up with the panel disabled, so basically make the system unusable. I've been using 07-21 for about a month, and just avoiding suspending. I'm now running drm-intel.git/drm-intel-next (which seems to not get the activity first?) and the latest commit is Linus 2.6.36-rc2. I've merged wireless-testing.git on top of this for ath9k woes. I get the stall precisely every 10 seconds. In VT mode the system stalls for about a second (the cursor stops blinking) and if I'm typing during the stall then my keypresses appear correctly after the stall ends. No errors in dmesg. Should I set drm.debug to something? If I'm running X (I've tried xf86-video-intel 2.12.0 and current .git master) then my keypresses appear twice after the stall ends, but honouring shift. If I hold shift, type X, release shift during a stall then I'll get "Xx" after the stall ends. This makes X practically unusable for me, so I'm at a plain VT for now. Suspend works both in VT and in X, so some things have definately improved for me since 07-21. :) I've just upgraded my userspace and have xorg-server-1.8.2. No Xv image with either xf86-video-intel driver. I haven't tried this server with the 07-21 kernel to find out if Xv works there. The stalling is more important. Chris' patch to i915_irq.c disabling wake-ups has no effect on the stalls in VT mode for me. I haven't bisected, would that be a big help? I'll probably have to spend a fair amount of time on that, since the latest previous kernel I could get to start is over a month old. I've tried drm-intel.git a couple of times over this month, but for long periods of time there were no changes. (Should I look elsewhere?) I also get intermittent FIFO errors, but I don't know how relevant they are for this issue. It seems like the stalling is a very distinct issue. Please let me know how I can help fix this. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/8]drivers:tmp.c Fix warning: variable 'rc' set but not used
Justin P. Mattock wrote: > > *baffled* Why did you think that would work? transmit_cmd()s signature > > has 4 parameters. > > I have no manual in front of me. Did a quick google, but came up with > (no hits) info on what that function does. grep showed too many entries > to really see why/what this is. Check out the tool cscope. (Or kscope, if you prefer a GUI.) //Peter
Re: [PATCH 4/8]drivers:tmp.c Fix warning: variable 'rc' set but not used
Justin P. Mattock wrote: > > *baffled* Why did you think that would work? transmit_cmd()s signature > > has 4 parameters. > > I have no manual in front of me. Did a quick google, but came up with > (no hits) info on what that function does. grep showed too many entries > to really see why/what this is. Check out the tool cscope. (Or kscope, if you prefer a GUI.) //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
