Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx