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

[Hans de Goede]

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.

> 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:

> 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.

Regards,

Hans