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

[Hans de Goede]

Hi,

On 28-02-19 10:15, Heikki Krogerus wrote:
> On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 27-02-19 12:16, Jani Nikula wrote:
>>> On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus at linux.intel.com> wrote:
>>>> One thing that this series does not consider is the DP lane count
>>>> problem. The GPU drivers (i915 in this case) does not know is four,
>>>> two or one DP lanes in use.
>>>
>>> Also, orientation.
>>
>> The orientation should simply be correct with Type-C over DP. The mux /
>> orientation-switch used will take care of (physically) swapping the
>> lanes if the connector is inserted upside-down.
>>
>>>> I guess that is not a critical issue since there is a workaround (I
>>>> think) where the driver basically does trial and error, but ideally we
>>>> should be able to tell i915 also the pin assignment that was
>>>> negotiated with the partner device so it knows the DP lane count.
>>>
>>> Yeah, if the information is there, we'd like to know.
>>
>> So far machines actually using a setup where the kernel does the
>> USB-PD / Type-C negotiation rather then this being handled in firmware
>> in say the Alpine Ridge controller, are very rare.
>>
>> AFAIK in the Alpine Ridge controller case, there is no communication
>> with the i915 driver, the only thing the Alpine Ridge controller does
>> which the everything done in the kernel approach does not, is that
>> it actually has a pin connected to the HDP pin of the displayport in
>> question.  But that just lets the i915 driver know about hotplug-events,
>> not how many lanes are used.
>>
>> Currently I'm aware of only 2 x86 models which actually need the
>> hotplug event propagation from the Type-C subsystem to the i915 driver.
>> Do we really want to come up with a much more complex solution to
>> optimize for this corner case, while the much more common case
>> (Alpine Ridge controller) does not provide this info to the i915 driver?
> 
> The HPD signal is often handled with a GPIO on Intel Platforms. Either
> the PD controller or EC controller, after receiving the Attention
> message, triggers the GPIO. On some systems though that GPIO method
> could not be used, so instead a side channel communication is used:
> the GFX device (so i915 driver) receives a specific custom interrupt.
> 
> Unfortunately it now appears that there may be products coming where
> using the GPIO is not going to be possible, and also side channel
> communication is going to be out of the question. I've started an
> internal discussion inside Intel about the possibility to extend the
> UCSI specification to be able to support that kind of products.
> 
> Alpine Ridge uses TI's Power Delivery controllers. The platforms that
> have Alpine Ridge TBT controller(s) often expose the USB Type-C
> connectors on them to the OS via UCSI, however, it appears the Alpine
> Ridge actually allows direct access to the PD controllers from the OS.
> That would mean we can supply the same information to the GPU drivers
> that we will deliver on CHT also on every platform that uses Alpine
> Ridge.

Ok.

>> To give some idea of the complexity, first of all we need some
>> mechanism to let the kernel know which drm_connector is connected
>> to which Type-C port. For the 2 models I know if which need this, this
>> info is not available and we would need to hardcode it in the kernel
>> based on e.g. DMI matching.
> 
> 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.

>> Then once we have this link, we need to start thinking about probe
>> ordering. Likely we need the typec framework to find the drm_connector,
>> since the typec-partner device is only created when there actually is
>> a DP capable "dongle" connected, making doing it the other way around
>> tricky. Then the typec-partner device needs to get a reference or some
>> such to make sure the drm_connector does not fgo away during the lifetime
>> of the typec-partner device.
> 
> No! We must not link the partner device with anything other than the
> Type-C connector. We link the USB Type-C connector to the DisplayPort,
> and we link the USB Type-C connector to the partner. The Type-C
> connector is the proxy here.

Maybe, but even then we still need one side of the link to have a
reference on the other, having a proxy in between does not change
anything.

>> I would really like to avoid this, so if we want to send more info to
>> the i915 driver, I suggest we create some event struct for this which
>> gets passed to the notifier, this could include a string to
>> describe the DP connector which the Type-C connector is connected to
>> when the mux is set to DP mode, e.g. "i915/DP-1" should be unique
>> or probably better, use the PCI device name, so: "0000:00:02.0/DP-1"
>>
>> Then we still have a loose coupling avoiding lifetime issues, while
>> the receiving drm driver can check which connector the event is for
>> and we can then also include other info such as lane-count and orientation
>> in the event struct.
> 
> 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'm not sure we have checked all the options we have yet. Perhaps
> there is a simpler way of doing this.

As a result of writing this patches I've been thinking that we really
have a need for some sort of in kernel event mechanism, think something
pub/sub ish, a bit like mqtt.

I'm thinking a global event-queue with an API like this:

struct kernel_event {
	enum kernel_event_type type;
	char source_id[32];
	char dest_id[32];
	union data {
		kernel_event_type_foo foo;
		kernel_event_type_bar bar;
	};
}

Where drivers interested in events can then specify that they
only want events of a certain type and optionally also filter
on source / dest id.

Note that setting dest_id would be optional for event generators,
since not all event generators will know this.

Looking at all the extcon and power_supply notifications we
already have going on with the Type-C PD support, all using their
own private notifier solutions, I think something generic like this,
which does not depend on one device getting some sort of reference
on another device, might in the end be better.

This would also avoid a lot of PROBE_DEFER handling in various places,
which in some cases gets rather tricky wrt ordering.

Regards,

Hans