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

Thu Feb 28 14:47:43 UTC 2019

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

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.

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

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.

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);

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.

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

Interesting proposal. Something like that could be useful.

thanks,

-- 
heikki