Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Den 02.06.2020 13.46, skrev Noralf Trønnes: > > > Den 02.06.2020 04.32, skrev Alan Stern: >> On Tue, Jun 02, 2020 at 12:12:07AM +, Peter Stuge 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? > > Absolutely, I want something thats works well in the kernel and doesn't > look odd to kernel USB people. > >> Your messages have been >> far too long to go into in any detail, but I will address this one issue. >> >> 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. >> >> A gadget driver can STALL in response to a control-OUT data packet, >> but only before it has seen the packet. Once the driver knows what >> the data packet contains, the gadget API doesn't provide any way to >> STALL the status stage. There has been a proposal to change the API >> to make this possible, but so far it hasn't gone forward. >> > > This confirms what I have seen in the kernel and the reason I added a > status request so I can know the result of the operation the device has > performed. > > I have a problem that I've encountered with this status request. > In my first version the gadget would usb_ep_queue() the status value > when the operation was done and as long as this happended within the > host timeout (5s) everything worked fine. > > Then I hit a 10s timeout in the gadget when performing a display modeset > operation (wait on missing vblank). The result of this was that the host > timed out and moved on. The gadget however didn't know that the host > gave up, so it queued up the status value. The result of this was that > all further requests from the host would time out. > Do you know a solution to this? > My work around is to just poll on the status request, which returns a > value immediately, until there's a result. The udc driver I use is dwc2. > I have now tried this on a Beaglebone Black (musb udc driver) and it works just fine there (it displays an error message on the next request). So it has to be a dwc2 driver problem. I will try and chase down this problem when I get the time. This means I don't need this status request polling in my host driver. Noralf. ___ 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
Den 02.06.2020 20.38, skrev Peter Stuge: > 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? > I'm going through some treatment that turned out to be worse than expected, so sadly I have to put this project on hold until my body recovers. Noralf. ___ 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
Den 02.06.2020 02.12, skrev Peter Stuge:
> 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.
>
I need to be more clear here about the word 'Generic' that I've used.
This is not a OS-agnostic protocol. It's written for Linux. I have had
the BSD's in mind, hence the MIT license, FreeBSD for instance has a DRM
subsystem. Other OS'es might not support all DRM properties, but should
be able to use it as a basic display. They will ofc need to translate
the Linux DRM specifics into their own environment. But first and
foremost this is for Linux.
The host driver is written against the capabilities of the Linux gadget
framework and the DRM subsystem. This will add some peculiarities that a
microcontroller implementation won't face. The focal point of the
project is providing as good performance as possible for Full HD desktop
use.
The word 'Generic' means that it's (should be) possible to make a USB
display for use on Linux without having to write a graphics driver. This
includes all kinds of tiny/small displays that currently are SPI or I2C
interfaced. These will probably use a microcontroller instead of a Linux
SoC to keep cost low.
>
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?
>
Performance is not good without compression so I need to keep that.
>
>> 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?
>
The kernel supports it and in actually performs quite well.
Decompression is much faster than compression which fits nicely with not
so powerful USB devices. Low resolution displays might not need
compression at all.
> Whether compression comes now or later, maybe there is a more suitable
> algorithm?
>
Could be, it's possible to support other compression methods. I'll leave
that to the professionals that need it for their display.
>
>>> 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?
>
The DRM subsystem doesn't have support for it so the gadget wouldn't
know when to use it. Monochrome DRM drivers advertise a XRGB format
and converts this into monochrome. The reason for using this format is
because all userspace supports it. AFAIK no DRM userspace support
monochrome.
>
>> 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?
>
I have done a microcontroller implementation, so I have tried it. It's
quite a different environment to work in than Linux for sure :-) It
might have been a more pleasant experience if I'd spent money on a
professional
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
On Tue, Jun 02, 2020 at 05:21:50AM +, Peter Stuge wrote: > > 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. It happens from time to time, such as when the host sends a SETUP packet that the gadget driver doesn't understand. > > 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. > 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. > > 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. On Tue, Jun 02, 2020 at 01:46:38PM +0200, Noralf Trønnes wrote: > > A gadget driver can STALL in response to a control-OUT data packet, > > but only before it has seen the packet. Once the driver knows what > > the data packet contains, the gadget API doesn't provide any way to > > STALL the status stage. There has been a proposal to change the API > > to make this possible, but so far it hasn't gone forward. > > > > This confirms what I have seen in the kernel and the reason I added a > status request so I can know the result of the operation the device has > performed. Does this status request use ep0 or some other endpoint? > I have a problem that I've encountered with this status request. > In my first version the gadget would usb_ep_queue() the status value > when the operation was done and as long as this happended within the > host timeout (5s) everything worked fine. > > Then I hit a 10s timeout in the gadget when performing a display modeset > operation (wait on missing vblank). The result of this was that the host > timed out and moved on. The gadget however didn't know that the host > gave up, so it queued up the status value. The result of this was that > all further requests from the host would time out. > Do you know a solution to this? > My work around is to just poll on the status request, which returns a > value immediately, until there's a result. The udc driver I use is dwc2. It's hard to give a precise answer without knowing the details of how your driver works. 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. This works but it has high overhead -- which may not matter if it happens infrequently and isn't sensitive to latency. The other approach is to send the status data over a bulk-IN endpoint. 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. That way, if the host received a stale status value, it could ignore it and move on. You also may want to give some thought to a "resynchronization" protocol, for use in situations where the host times out waiting for a response from the device while the device is waiting for something else (the host, a vblank, or whatever). This could be a special control request, or you could rely on the host doing a complete USB reset. Alan Stern ___ 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
Den 02.06.2020 04.32, skrev Alan Stern: > On Tue, Jun 02, 2020 at 12:12:07AM +, Peter Stuge 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? Absolutely, I want something thats works well in the kernel and doesn't look odd to kernel USB people. > Your messages have been > far too long to go into in any detail, but I will address this one issue. > > 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. > > A gadget driver can STALL in response to a control-OUT data packet, > but only before it has seen the packet. Once the driver knows what > the data packet contains, the gadget API doesn't provide any way to > STALL the status stage. There has been a proposal to change the API > to make this possible, but so far it hasn't gone forward. > This confirms what I have seen in the kernel and the reason I added a status request so I can know the result of the operation the device has performed. I have a problem that I've encountered with this status request. In my first version the gadget would usb_ep_queue() the status value when the operation was done and as long as this happended within the host timeout (5s) everything worked fine. Then I hit a 10s timeout in the gadget when performing a display modeset operation (wait on missing vblank). The result of this was that the host timed out and moved on. The gadget however didn't know that the host gave up, so it queued up the status value. The result of this was that all further requests from the host would time out. Do you know a solution to this? My work around is to just poll on the status request, which returns a value immediately, until there's a result. The udc driver I use is dwc2. Noralf. > Alan Stern > ___ 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
On Tue, Jun 02, 2020 at 12:12:07AM +, Peter Stuge 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? Your messages have been far too long to go into in any detail, but I will address this one issue. 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. A gadget driver can STALL in response to a control-OUT data packet, but only before it has seen the packet. Once the driver knows what the data packet contains, the gadget API doesn't provide any way to STALL the status stage. There has been a proposal to change the API to make this possible, but so far it hasn't gone forward. Alan Stern ___ 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
Re: [PATCH v3 4/6] drm: Add Generic USB Display driver
Hi Peter,
Den 30.05.2020 00.45, skrev Peter Stuge:
> 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.
>
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
>
>> 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).
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.
> 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!
>
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. I had some code to test the implementation
when I made it, but it's removed now. It's very easy to add R1 back in
when there's a display that makes use of it giving it a thorough testing.
You mention further down that you have use cases for this, do you have a
timeplan for when you will make use of R1?
>
>> - 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? To me it's no big deal, it can be added later if
someones cares about it.
> 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.
I have tried my best to let the driver tolerate other vendor class
interfaces on the device.
> 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?
>
Not sure what you mean here, this is an interface driver.
There exists a HID Display page:
Request #: HUTRR29
Title: Repurposing the Alphanumeric Display Page (0x14) as a generic
Auxiliary Display Page and adding bitmap and custom segment
capabilities
https://www.usb.org/sites/default/files/hutrr29b_0.pdf
I couldn't find anything that uses it. It could have been interesting if
there was a bulk endpoint here.
> Does the driver btw. already support a composite device with multiple GUD
> interfaces? Say a microcontroller with two independent panels. It seems no?
>
I haven't tried, but it should work.
> 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
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
[PATCH v3 4/6] drm: Add Generic USB Display driver
This adds a generic USB display driver with the intention that it can be used with future USB interfaced low end displays/adapters. The Linux gadget device driver will serve as the canonical device implementation. The following DRM properties are supported: - Plane rotation - Connector TV properties There is also support for backlight brightness exposed as a backlight device. Display modes can be made available to the host driver either as DRM display modes or through EDID. If both are present, EDID is just passed on to userspace. Performance is preferred over color depth, so if the device supports RGB565, DRM_CAP_DUMB_PREFERRED_DEPTH will return 16. If the device transfer buffer can't fit an uncompressed framebuffer update, the update is split up into parts that do fit. Optimal user experience is achieved by providing damage reports either by setting FB_DAMAGE_CLIPS on pageflips or calling DRM_IOCTL_MODE_DIRTYFB. LZ4 compression is used if the device supports it. 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 driver is MIT licensed to smooth the path for any BSD port of the driver. v2: - Use devm_drm_dev_alloc() and drmm_mode_config_init() - drm_fbdev_generic_setup: Use preferred_bpp=0, 16 was a copy paste error - The drm_backlight_helper is dropped, copy in the code - Support protocol version backwards compatibility for device v3: - Use donated Openmoko USB pid - Use direct compression from framebuffer when pitch matches, not only on full frames, so split updates can benefit - Use __le16 in struct gud_drm_req_get_connector_status - Set edid property when the device only provides edid - Clear compression fields in struct gud_drm_req_set_buffer - Fix protocol version negotiation - Remove mode->vrefresh, it's calculated Signed-off-by: Noralf Trønnes --- MAINTAINERS | 8 + drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/gud/Kconfig | 14 + drivers/gpu/drm/gud/Makefile| 4 + drivers/gpu/drm/gud/gud_drm_connector.c | 726 drivers/gpu/drm/gud/gud_drm_drv.c | 648 + drivers/gpu/drm/gud/gud_drm_internal.h | 65 +++ drivers/gpu/drm/gud/gud_drm_pipe.c | 426 ++ include/drm/gud_drm.h | 361 10 files changed, 2255 insertions(+) create mode 100644 drivers/gpu/drm/gud/Kconfig create mode 100644 drivers/gpu/drm/gud/Makefile create mode 100644 drivers/gpu/drm/gud/gud_drm_connector.c create mode 100644 drivers/gpu/drm/gud/gud_drm_drv.c create mode 100644 drivers/gpu/drm/gud/gud_drm_internal.h create mode 100644 drivers/gpu/drm/gud/gud_drm_pipe.c create mode 100644 include/drm/gud_drm.h diff --git a/MAINTAINERS b/MAINTAINERS index caeeac9f6b46..b15bf9b2229b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5291,6 +5291,14 @@ S: Maintained F: drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c F: Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.yaml +DRM DRIVER FOR GENERIC USB DISPLAY +M: Noralf Trønnes +S: Maintained +W: https://github.com/notro/gud/wiki +T: git git://anongit.freedesktop.org/drm/drm-misc +F: drivers/gpu/drm/gud/ +F: include/drm/gud_drm.h + DRM DRIVER FOR GRAIN MEDIA GM12U320 PROJECTORS M: Hans de Goede S: Maintained diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 4bffa08f610a..33e63b68a82d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -391,6 +391,8 @@ source "drivers/gpu/drm/mcde/Kconfig" source "drivers/gpu/drm/tidss/Kconfig" +source "drivers/gpu/drm/gud/Kconfig" + # Keep legacy drivers last menuconfig DRM_LEGACY diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 53d8fa170143..41a87763f0e5 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -124,3 +124,4 @@ obj-$(CONFIG_DRM_PANFROST) += panfrost/ obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/ obj-$(CONFIG_DRM_MCDE) += mcde/ obj-$(CONFIG_DRM_TIDSS) += tidss/ +obj-y += gud/ diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig new file mode 100644 index ..767f1c067ed9 --- /dev/null +++ b/drivers/gpu/drm/gud/Kconfig @@ -0,0 +1,14 @@ +# SPDX-License-Identifier: GPL-2.0 + +config DRM_GUD + tristate "Generic USB Display" + depends on DRM && USB + select LZ4_COMPRESS + select DRM_KMS_HELPER + select DRM_GEM_SHMEM_HELPER + select BACKLIGHT_CLASS_DEVICE + help + This is a DRM display driver for Generic USB Displays or display + adapters. + + If M is selected the module will be called gud_drm. diff --git a/drivers/gpu/drm/gud/Makefile b/drivers/gpu/drm/gud/Makefile new file mode 100644 index ..73ed7ef3da94
