Hi Hans, On Thu, Feb 28, 2019 at 05:54:21PM +0100, Hans de Goede wrote: > Hi Heikki, > > On 28-02-19 15:47, Heikki Krogerus wrote: > > Hi Hans, > > > > On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 28-02-19 10:15, Heikki Krogerus wrote: > > <snip> > > > > > I've been thinking about this... Do we actually need to link the > > > > correct drm_connector to the Type-C connector? Perhaps we can make > > > > this work by just linking the GFX device to the Type-C connector. > > > > > > What use is it to the kms driver if it gets an event there is a DP > > > hotplug with x lanes and orientation foo, if we are not telling it > > > on which DP port it is ? kms devices already have multiple DP ports > > > and more then one could be hooked-up to type-c connectors. > > > > I was looking at this series. You walk trough every DP port in the > > system when the DP alt mode driver broadcasts the event, but maybe > > that's different. Never mind. > > Right, my "simple / naive" solution simply tells the kms driver to > check all DP ports for connection state changes, similar to how > running "xrandr" under Xorg causes the kms driver to re-check the > connection status of all ports. Actually running xrandr under Xorg > after plugging in the cable, is how I did my initial DP over Type-C > testing on the GPD win. > > But once we start passing extra-info, I believe the kms driver needs > to know to which connector that info belongs. > > <snip> > > > > > Well, I don't think we can deny the GPU driver (in this case) the > > > > information that we have and that is relevant to it, just because it > > > > seems difficult to deliver that information to the right location. > > > > > > Right, but this does not require a tight-coupling. My original > > > proposal can do this if we pass a data struct with an identifier > > > for the DP port for which the event is to the notifier. I suggest using > > > a string for identifier, something like: "0000:00:02.0/DP-1" this > > > event struct could then also contain all the info we want to pass. > > > > I do agree that we should not tie the objects (device entries) > > representing these components in kernel together, but I assume that we > > agree now that the connection between the two - the USB Type-C > > connector and the DisplayPort - must be described somewhere, either in > > firmware or build-in? So I guess we are talking implementation details > > here, right? > > Right. > > > If that is the case... > > > > That string identifier you proposed would basically provide the > > details about the connection, so if we know those details, we might as > > well use "normal" ways to describe the connection and just extract > > them in runtime in the function that our DP alt mode driver calls. I > > think the connection has to be defined in i915 on CHT in any case. > > Interesting, I think the connection should be described in the fwnode > for the fusb302 device for the CHT/GPD win case. Specifically I think > this fits well as a property of the dp altmode. OK, you are correct. I was stupidly still thinking about the driver loading order, but the order does not matter. > > The DP alt mode driver should definitely not need to pass anything > > else to the notifier other than handle to itself (actually, handle > > straight to the port device would be better) as an identifier. The > > notifier function needs to be the one that determines the actual > > connection using that handle. Even if the target DP is described using > > a string like you propose, then that string has to come from > > somewhere, most likely from a device property. The notifier function > > can just as well extract it, we don't need to pass it separately. > > > > Here's my suggestion for function prototype: > > > > int drm_typec_dp_notification(struct device *typec_port_dev, > > struct typec_displayport_data *data); > > How about instead of the port_dev we pass in the altmode object and > we have a method to get the fwnode for the altmode? Then the > drm_typec_dp_notification() function can get info from that fwnode > to implement the connection finding you describe below: We can pass the altmode object, np, but let's not decide which fwnode we'll ultimately use. I'm still leaning towards the connector node. > > So that function finds the connection between typec_port_dev and the > > correct DP in runtime, possibly by letting i915 (or what ever GPU > > driver) to do that. Once the function is done, it decrements any ref > > counts that it incremented before returning. > > > > That struct typec_displayport_data has all the information we have - > > the current pin assignment from the Configure VDO, HPD IRQ from the > > last Status Update, etc. - so it needs to be passed as payload to the > > notifier. > > Ack. > > So I believe that this discussion ties into the discussion from the: > "[PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes" > > Mail thread. As discussed there I agree that adding a usb_connector > child fwnode to the fwnode for the fusb302 to describe things like > sink- and source-pdos is a good idea. > > Our last few mails were discussing describing supported alt-modes on > the connector by adding altmode child-nodes to the usb_connector node. > > I think it is best to continue that discussion here, as the 2 discussions > tie into one another. > > So my last proposal in that thread was adding the following to: > > Documentation/devicetree/bindings/connector/usb-connector.txt: > > """ > Optionally an "usb-c-connector" can have child nodes, describing > supported alt-modes. > > Required properties for usb-c-connector altmode child-nodes: > compatible: "usb-type-c-altmode" > svid: integer, Standard or Vendor ID for the altmode (u16 stored in an u32) property and an u32 > vdo: integer, Vendor Data Object, VDO describing the altmode capabilies, SVID specific > """ > > Since we now want to have the kernel know which DP connector belongs to > the usb_connector being in DP altmode I suggest additionally adding > the following: > > """ > Optional properties for DisplayPort (svid==0xff01) altmode child-nodes. > > linux,dp-connector String in the form of "device-name/connector-name" describing the > DisplayPort connector on the GPU which is used when the usb-c-connector > is in DisplayPort altmode, e.g. "0000:00:02.0/DP-1" > """ > > This to me feels like it is the most logical place to store the connection info, > at least for the CHT/GPD win case. For other cases we may very-well need something > different. Since on the CHT/GPD win case both the producer and consumer of this > property will be in kernel, I think it is best to just go with this for now. > If we then later get a different solution for other cases and that solution turns > out to be generic enough that it will also work on the GPD win we can always move > the GPD win (and pocket) over to the new solution. Just like we are moving it > over to the usb_connector fwnode now. I don't have a problem with your proposal of using a string like that at this point, but don't document it. I want to at least see if it's possible to use real reference instead of a string. I'm also still not sure should that be placed under the altmode node or should go under the connector node. So please don't add it to the usb-connector.txt at this point, even as an optional property. thanks, -- heikki
