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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 04-03-19 16:17, Heikki Krogerus wrote:
> 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.

Ok, I will give the discussed approach a try and then post a new version of
the patchset. I hope to get around to this this weekend (as you know this is a side /
spare-time project for me).

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel