Hi All,
Here is a new attempt to make DP over Type-C work on devices where the Type-C controller does not drive the HPD pin on the GPU, but instead we need to forward HPD events from the Type-C controller to the DRM driver.
For anyone interested here are the old (2019!) patches for this:
https://patchwork.freedesktop.org/patch/288491/ https://patchwork.freedesktop.org/patch/288493/ https://patchwork.freedesktop.org/patch/288495/
Last time I posted this the biggest change requested was for more info to be included in the event send to the DRM-subsystem, specifically sending the following info was requested:
1. Which DP connector on the GPU the event is for 2. How many lanes are available 3. Connector orientation
This series is basically an entirely new approach, which no longer uses the notifier framework at all. Instead the Type-C code looksup a connector based on a fwnode (this was suggested by Heikki Krogerus) and then calls a new oob_hotplug_event drm_connector_func directly on the connector, passing the requested info as argument.
This series not only touches drm subsys files but it also touches drivers/usb/typec/altmodes/typec_displayport.c, that file usually does not see a whole lot of changes. So I believe it would be best to just merge the entire series through drm-misc, Assuming we can get an ack from Greg for merging the typec_displayport.c changes this way.
Regards,
Hans
Hans de Goede (8): drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector drm/connector: Add a fwnode pointer to drm_connector and register with ACPI drm/connector: Add drm_connector_find_by_fwnode() function drm/connector: Add support for out-of-band hotplug notification drm/i915/dp: Add support for out-of-bound hotplug events usb: typec: altmodes/displayport: Make dp_altmode_notify() more generic usb: typec: altmodes/displayport: Notify drm subsys of hotplug events platform/x86/intel_cht_int33fe: Correct "displayport" fwnode reference
Heikki Krogerus (1): drm/i915: Associate ACPI connector nodes with connector entries
drivers/gpu/drm/drm_connector.c | 20 +++ drivers/gpu/drm/drm_sysfs.c | 129 ++++++++++++++++-- drivers/gpu/drm/i915/display/intel_acpi.c | 40 ++++++ drivers/gpu/drm/i915/display/intel_acpi.h | 3 + drivers/gpu/drm/i915/display/intel_display.c | 1 + drivers/gpu/drm/i915/display/intel_dp.c | 13 ++ .../platform/x86/intel_cht_int33fe_typec.c | 4 +- drivers/usb/typec/altmodes/displayport.c | 78 ++++++++--- include/drm/drm_connector.h | 36 +++++ 9 files changed, 292 insertions(+), 32 deletions(-)
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/drm_sysfs.c | 54 +++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index f0336c804639..c344c6d5e738 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -50,6 +50,10 @@ static struct device_type drm_sysfs_device_minor = { .name = "drm_minor" };
+static struct device_type drm_sysfs_device_connector = { + .name = "drm_connector", +}; + struct class *drm_class;
static char *drm_devnode(struct device *dev, umode_t *mode) @@ -271,30 +275,64 @@ static const struct attribute_group *connector_dev_groups[] = { NULL };
+static void drm_sysfs_connector_release(struct device *dev) +{ + struct drm_connector *connector = to_drm_connector(dev); + + drm_connector_put(connector); + kfree(dev); +} + int drm_sysfs_connector_add(struct drm_connector *connector) { struct drm_device *dev = connector->dev; + struct device *kdev; + int r;
if (connector->kdev) return 0;
- connector->kdev = - device_create_with_groups(drm_class, dev->primary->kdev, 0, - connector, connector_dev_groups, - "card%d-%s", dev->primary->index, - connector->name); + kdev = kzalloc(sizeof(*kdev), GFP_KERNEL); + if (!kdev) + return -ENOMEM; + + device_initialize(kdev); + kdev->class = drm_class; + kdev->type = &drm_sysfs_device_connector; + kdev->parent = dev->primary->kdev; + kdev->groups = connector_dev_groups; + kdev->release = drm_sysfs_connector_release; + dev_set_drvdata(kdev, connector); + + r = dev_set_name(kdev, "card%d-%s", dev->primary->index, connector->name); + if (r) + goto err_free; + DRM_DEBUG("adding "%s" to sysfs\n", connector->name);
- if (IS_ERR(connector->kdev)) { - DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev)); - return PTR_ERR(connector->kdev); + r = device_add(kdev); + if (r) { + DRM_ERROR("failed to register connector device: %d\n", r); + goto err_free; }
+ /* + * Ensure the connector object does not get free-ed if userspace still has + * references open to the device through e.g. the connector sysfs-attributes. + */ + drm_connector_get(connector); + + connector->kdev = kdev; + if (connector->ddc) return sysfs_create_link(&connector->kdev->kobj, &connector->ddc->dev.kobj, "ddc"); return 0; + +err_free: + put_device(kdev); + return r; }
void drm_sysfs_connector_remove(struct drm_connector *connector)
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Also I'd assume this creates a loop since the connector holds a reference on the kdev? -Daniel
drivers/gpu/drm/drm_sysfs.c | 54 +++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index f0336c804639..c344c6d5e738 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -50,6 +50,10 @@ static struct device_type drm_sysfs_device_minor = { .name = "drm_minor" };
+static struct device_type drm_sysfs_device_connector = {
- .name = "drm_connector",
+};
struct class *drm_class;
static char *drm_devnode(struct device *dev, umode_t *mode) @@ -271,30 +275,64 @@ static const struct attribute_group *connector_dev_groups[] = { NULL };
+static void drm_sysfs_connector_release(struct device *dev) +{
- struct drm_connector *connector = to_drm_connector(dev);
- drm_connector_put(connector);
- kfree(dev);
+}
int drm_sysfs_connector_add(struct drm_connector *connector) { struct drm_device *dev = connector->dev;
struct device *kdev;
int r;
if (connector->kdev) return 0;
- connector->kdev =
device_create_with_groups(drm_class, dev->primary->kdev, 0,connector, connector_dev_groups,"card%d-%s", dev->primary->index,connector->name);
- kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
- if (!kdev)
return -ENOMEM;- device_initialize(kdev);
- kdev->class = drm_class;
- kdev->type = &drm_sysfs_device_connector;
- kdev->parent = dev->primary->kdev;
- kdev->groups = connector_dev_groups;
- kdev->release = drm_sysfs_connector_release;
- dev_set_drvdata(kdev, connector);
- r = dev_set_name(kdev, "card%d-%s", dev->primary->index, connector->name);
- if (r)
goto err_free;- DRM_DEBUG("adding "%s" to sysfs\n", connector->name);
- if (IS_ERR(connector->kdev)) {
DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev));return PTR_ERR(connector->kdev);
r = device_add(kdev);
if (r) {
DRM_ERROR("failed to register connector device: %d\n", r);goto err_free;}
/*
* Ensure the connector object does not get free-ed if userspace still has* references open to the device through e.g. the connector sysfs-attributes.*/drm_connector_get(connector);
connector->kdev = kdev;
if (connector->ddc) return sysfs_create_link(&connector->kdev->kobj, &connector->ddc->dev.kobj, "ddc"); return 0;
+err_free:
- put_device(kdev);
- return r;
}
void drm_sysfs_connector_remove(struct drm_connector *connector)
2.31.1
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one. -Daniel
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is still needed but for a different reason:
""" We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """
This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that.
Regards,
Hans
On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is still needed but for a different reason:
""" We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """
This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that.
Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function.
Lookup tables holding full references tends to lead to all kinds of bad side effects. -Daniel
Hi,
On 4/29/21 9:09 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is still needed but for a different reason:
""" We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """
This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that.
Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function.
I'm afraid that things are a bit more complicated here. The idea here is that we have a subsystem outside of the DRM subsystem which received a hotplug event for a drm-connector. The only info which this subsystem has is a reference on the fwnode level (either through device-tree or to platform-code instantiating software-fwnode-s + links for this).
So in order to deliver the hotplug event to the connector we need to lookup the connector by fwnode.
I've chosen to implement this by iterating over all drm_class devices with a dev_type of drm_connector using class_dev_iter_init() and friends. This makes sure that we either get a reference to the device, or that we skip the device if it is being deleted.
But this just gives us a reference to the connector->kdev, not to the connector itself. A pointer to the connector itself is stored as drvdata inside the device, but without taking a reference as this patch does, there is no guarantee that that pointer does not point to possibly free-ed mem.
We could set drvdata to 0 from drm_sysfs_connector_remove() Before calling device_unregister(connector->kdev) and then do something like this inside drm_connector_find_by_fwnode():
/* * Lock the device to ensure we either see the drvdata == NULL * set by drm_sysfs_connector_remove(); or we block the removal * from continuing until we are done with the device. */ device_lock(dev); connector = dev_get_drvdata(dev); if (connector && connector->fwnode == fwnode) { drm_connector_get(connector); found = connector; } device_unlock(dev);
With the device_lock() synchronizing against the device_lock() in device_unregister(connector->kdev). So that we either see drvdata == NULL if we race with unregistering; or we get a reference on the drm_connector obj before its ref-count can drop to 0.
There might be places though where we call code take the device_lock while holding a lock necessary for the drm_connector_get() , so this approach might lead to an AB BA deadlock. As such I think my original approach is better (also see below).
Lookup tables holding full references tends to lead to all kinds of bad side effects.
The proposed reference is not part of a lookup list, it is a reference from the kdev on the drm_connector object which gets dropped as soon as the kdev's refcount hits 0, which normally happens directly after drm_connector_unregister() has run.
In many other places in the kernel problems like this are solved by embedding the device struct inside the containing data struct (so the drm_connector struct) and using the device_struct's refcounting for all refcounting and using the device struct's release callback as the release callback for the entire object.
That is not doable here since the drm_object code has its own refcounting going on. What this patch is in essence doing is simulating having only 1 refcount, by making sure the main-object release callback does not get run until the drm_objects' refcount and the device's refcount have both reached 0 (by keeping the drm_object's refcount at a minimum of 1 as long as there are references to the device).
Regards,
Hans
On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 4/29/21 9:09 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: > Userspace could hold open a reference to the connector->kdev device, > through e.g. holding a sysfs-atrtribute open after > drm_sysfs_connector_remove() has been called. In this case the connector > could be free-ed while the connector->kdev device's drvdata is still > pointing to it. > > Give drm_connector devices there own device type, which allows > us to specify our own release function and make drm_sysfs_connector_add() > take a reference on the connector object, and have the new release > function put the reference when the device is released. > > Giving drm_connector devices there own device type, will also allow > checking if a device is a drm_connector device with a > "if (device->type == &drm_sysfs_device_connector)" check. > > Note that the setting of the name member of the device_type struct will > cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector > as extra info. So this extends the uevent part of the userspace API. > > Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is still needed but for a different reason:
""" We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """
This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that.
Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function.
I'm afraid that things are a bit more complicated here. The idea here is that we have a subsystem outside of the DRM subsystem which received a hotplug event for a drm-connector. The only info which this subsystem has is a reference on the fwnode level (either through device-tree or to platform-code instantiating software-fwnode-s + links for this).
So in order to deliver the hotplug event to the connector we need to lookup the connector by fwnode.
I've chosen to implement this by iterating over all drm_class devices with a dev_type of drm_connector using class_dev_iter_init() and friends. This makes sure that we either get a reference to the device, or that we skip the device if it is being deleted.
But this just gives us a reference to the connector->kdev, not to the connector itself. A pointer to the connector itself is stored as drvdata inside the device, but without taking a reference as this patch does, there is no guarantee that that pointer does not point to possibly free-ed mem.
We could set drvdata to 0 from drm_sysfs_connector_remove() Before calling device_unregister(connector->kdev) and then do something like this inside drm_connector_find_by_fwnode():
/*
- Lock the device to ensure we either see the drvdata == NULL
- set by drm_sysfs_connector_remove(); or we block the removal
- from continuing until we are done with the device.
*/ device_lock(dev); connector = dev_get_drvdata(dev); if (connector && connector->fwnode == fwnode) { drm_connector_get(connector); found = connector; } device_unlock(dev);
Yes this is what I mean. Except not a drm_connector_get, but a kref_get_unless_zero. The connector might already be on it's way out, but the drvdata not yet cleared.
With the device_lock() synchronizing against the device_lock() in device_unregister(connector->kdev). So that we either see drvdata == NULL if we race with unregistering; or we get a reference on the drm_connector obj before its ref-count can drop to 0.
The trouble is that most connectors aren't full drivers on their kdev. So this isn't the right lock. We need another lock which protects the drvdata pointer appropriately for drm connectors.
There might be places though where we call code take the device_lock while holding a lock necessary for the drm_connector_get() , so this approach might lead to an AB BA deadlock. As such I think my original approach is better (also see below).
Lookup tables holding full references tends to lead to all kinds of bad side effects.
The proposed reference is not part of a lookup list, it is a reference from the kdev on the drm_connector object which gets dropped as soon as the kdev's refcount hits 0, which normally happens directly after drm_connector_unregister() has run.
Yeah but the way you use it is for lookup purposes. What we're implementing is the "get me the drm_connector for this fwnode" functionality, and that _is_ a lookup. How its implemented is an internal detail really, and somehow using full references for lookup functionality isn't great.
I'm also not sure why we have to use the kdev stuff here. For other random objects we need to look up we're building that functionality on that object. It means you need to keep another list_head around for that lookup, but that's really not a big cost. E.g. drm_bridge/panel work like that.
In many other places in the kernel problems like this are solved by embedding the device struct inside the containing data struct (so the drm_connector struct) and using the device_struct's refcounting for all refcounting and using the device struct's release callback as the release callback for the entire object.
That is not doable here since the drm_object code has its own refcounting going on. What this patch is in essence doing is simulating having only 1 refcount, by making sure the main-object release callback does not get run until the drm_objects' refcount and the device's refcount have both reached 0 (by keeping the drm_object's refcount at a minimum of 1 as long as there are references to the device).
Uh yeah that sounds bad. If you really need the full refcount we should really only have one. -Daniel
Hi,
On 4/30/21 1:38 PM, Daniel Vetter wrote:
On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 4/29/21 9:09 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: > On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: >> Userspace could hold open a reference to the connector->kdev device, >> through e.g. holding a sysfs-atrtribute open after >> drm_sysfs_connector_remove() has been called. In this case the connector >> could be free-ed while the connector->kdev device's drvdata is still >> pointing to it. >> >> Give drm_connector devices there own device type, which allows >> us to specify our own release function and make drm_sysfs_connector_add() >> take a reference on the connector object, and have the new release >> function put the reference when the device is released. >> >> Giving drm_connector devices there own device type, will also allow >> checking if a device is a drm_connector device with a >> "if (device->type == &drm_sysfs_device_connector)" check. >> >> Note that the setting of the name member of the device_type struct will >> cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector >> as extra info. So this extends the uevent part of the userspace API. >> >> Signed-off-by: Hans de Goede hdegoede@redhat.com > > Are you sure? I thought sysfs is supposed to flush out any pending > operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is still needed but for a different reason:
""" We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """
This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that.
Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function.
I'm afraid that things are a bit more complicated here. The idea here is that we have a subsystem outside of the DRM subsystem which received a hotplug event for a drm-connector. The only info which this subsystem has is a reference on the fwnode level (either through device-tree or to platform-code instantiating software-fwnode-s + links for this).
So in order to deliver the hotplug event to the connector we need to lookup the connector by fwnode.
I've chosen to implement this by iterating over all drm_class devices with a dev_type of drm_connector using class_dev_iter_init() and friends. This makes sure that we either get a reference to the device, or that we skip the device if it is being deleted.
But this just gives us a reference to the connector->kdev, not to the connector itself. A pointer to the connector itself is stored as drvdata inside the device, but without taking a reference as this patch does, there is no guarantee that that pointer does not point to possibly free-ed mem.
We could set drvdata to 0 from drm_sysfs_connector_remove() Before calling device_unregister(connector->kdev) and then do something like this inside drm_connector_find_by_fwnode():
/*
- Lock the device to ensure we either see the drvdata == NULL
- set by drm_sysfs_connector_remove(); or we block the removal
- from continuing until we are done with the device.
*/ device_lock(dev); connector = dev_get_drvdata(dev); if (connector && connector->fwnode == fwnode) { drm_connector_get(connector); found = connector; } device_unlock(dev);
Yes this is what I mean. Except not a drm_connector_get, but a kref_get_unless_zero. The connector might already be on it's way out, but the drvdata not yet cleared.
The function we race with is drm_sysfs_connector_remove() and either:
1. The lookup wins the race in which case drm_sysfs_connector_remove() can only complete after the drm_connector_get(); and the connector kref won't drop to 0 before drm_sysfs_connector_remove() completes; or 2. drm_sysfs_connector_remove() wins the race in which case drvdata will be 0.
So using kref_get_unless_zero here will not make a difference and requires poking inside the drm_connector internals.
Note I will probably go with your suggestion below, so whether or not to use kref_get_unless_zero here is likely no longer relevant.
With the device_lock() synchronizing against the device_lock() in device_unregister(connector->kdev). So that we either see drvdata == NULL if we race with unregistering; or we get a reference on the drm_connector obj before its ref-count can drop to 0.
The trouble is that most connectors aren't full drivers on their kdev. So this isn't the right lock. We need another lock which protects the drvdata pointer appropriately for drm connectors.
There might be places though where we call code take the device_lock while holding a lock necessary for the drm_connector_get() , so this approach might lead to an AB BA deadlock. As such I think my original approach is better (also see below).
Lookup tables holding full references tends to lead to all kinds of bad side effects.
The proposed reference is not part of a lookup list, it is a reference from the kdev on the drm_connector object which gets dropped as soon as the kdev's refcount hits 0, which normally happens directly after drm_connector_unregister() has run.
Yeah but the way you use it is for lookup purposes. What we're implementing is the "get me the drm_connector for this fwnode" functionality, and that _is_ a lookup.
Ack.
How its implemented is an internal detail really, and somehow using full references for lookup functionality isn't great.
Ok, note that the caller of this only needs the reference for a short while, what the caller does is:
connector = drm_connector_find_by_fwnode(dp->connector_fwnode); if (connector) { drm_connector_oob_hotplug_event(connector, &data); drm_connector_put(connector); }
As a result of out discussion I have been thinking about enforcing this short-lifetime of the reference by changing:
void drm_connector_oob_hotplug_event(struct drm_connector *connector, struct drm_connector_oob_hotplug_event_data *data);
to:
void drm_connector_oob_hotplug_event(struct fwnode_handle connector_fwnode, struct drm_connector_oob_hotplug_event_data *data);
And making that do the lookup (+ almost immediate put) internally, making the connector-lookup a purely drm-subsys internal thing and enforcing code outside of the drm-subsys not holding a long-time reference to the connector this way.
Please let me know if you prefer the variant where the connector lookup details are hidden from the callers ?
Then I can change this for for v2 of this patch/series.
I'm also not sure why we have to use the kdev stuff here. For other random objects we need to look up we're building that functionality on that object. It means you need to keep another list_head around for that lookup, but that's really not a big cost. E.g. drm_bridge/panel work like that.
Using class_for_each_dev seemed like a good way to iterate over all the connectors. But given the discussion this has caused, just adding a new static list + mutex for this to drivers/gpu/drm/drm_connector.c sounds like it might be a better approach indeed.
So shall I change thing over to this approach for v2 of this patch/series?
Regards,
Hans
On Fri, Apr 30, 2021 at 3:32 PM Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 4/30/21 1:38 PM, Daniel Vetter wrote:
On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 4/29/21 9:09 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote: > On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: >> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: >>> Userspace could hold open a reference to the connector->kdev device, >>> through e.g. holding a sysfs-atrtribute open after >>> drm_sysfs_connector_remove() has been called. In this case the connector >>> could be free-ed while the connector->kdev device's drvdata is still >>> pointing to it. >>> >>> Give drm_connector devices there own device type, which allows >>> us to specify our own release function and make drm_sysfs_connector_add() >>> take a reference on the connector object, and have the new release >>> function put the reference when the device is released. >>> >>> Giving drm_connector devices there own device type, will also allow >>> checking if a device is a drm_connector device with a >>> "if (device->type == &drm_sysfs_device_connector)" check. >>> >>> Note that the setting of the name member of the device_type struct will >>> cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector >>> as extra info. So this extends the uevent part of the userspace API. >>> >>> Signed-off-by: Hans de Goede hdegoede@redhat.com >> >> Are you sure? I thought sysfs is supposed to flush out any pending >> operations (they complete fast) and handle open fd internally? > > Yes, it "should" :)
Thanks for confirming my vague memories :-)
Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is still needed but for a different reason:
""" We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """
This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that.
Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function.
I'm afraid that things are a bit more complicated here. The idea here is that we have a subsystem outside of the DRM subsystem which received a hotplug event for a drm-connector. The only info which this subsystem has is a reference on the fwnode level (either through device-tree or to platform-code instantiating software-fwnode-s + links for this).
So in order to deliver the hotplug event to the connector we need to lookup the connector by fwnode.
I've chosen to implement this by iterating over all drm_class devices with a dev_type of drm_connector using class_dev_iter_init() and friends. This makes sure that we either get a reference to the device, or that we skip the device if it is being deleted.
But this just gives us a reference to the connector->kdev, not to the connector itself. A pointer to the connector itself is stored as drvdata inside the device, but without taking a reference as this patch does, there is no guarantee that that pointer does not point to possibly free-ed mem.
We could set drvdata to 0 from drm_sysfs_connector_remove() Before calling device_unregister(connector->kdev) and then do something like this inside drm_connector_find_by_fwnode():
/*
- Lock the device to ensure we either see the drvdata == NULL
- set by drm_sysfs_connector_remove(); or we block the removal
- from continuing until we are done with the device.
*/ device_lock(dev); connector = dev_get_drvdata(dev); if (connector && connector->fwnode == fwnode) { drm_connector_get(connector); found = connector; } device_unlock(dev);
Yes this is what I mean. Except not a drm_connector_get, but a kref_get_unless_zero. The connector might already be on it's way out, but the drvdata not yet cleared.
The function we race with is drm_sysfs_connector_remove() and either:
- The lookup wins the race in which case drm_sysfs_connector_remove() can only complete after the drm_connector_get(); and the connector kref won't drop to 0 before drm_sysfs_connector_remove() completes; or
- drm_sysfs_connector_remove() wins the race in which case drvdata will be 0.
So using kref_get_unless_zero here will not make a difference and requires poking inside the drm_connector internals.
Note I will probably go with your suggestion below, so whether or not to use kref_get_unless_zero here is likely no longer relevant.
Ah I missed that nuance, it's what I get for not reading the patchset :-/ I was assuming that this is a lookup function which races rather freely. The way you explain it's wired up here it's clear that we remove the lookup entry as part of hotunplug, not as part of final drm_connector cleanup.
So in that special case the additional refcount due to the lookup entry isn't a problem, but it's still better to aim for something where we only deal with a single refcount for drm_connector.
With the device_lock() synchronizing against the device_lock() in device_unregister(connector->kdev). So that we either see drvdata == NULL if we race with unregistering; or we get a reference on the drm_connector obj before its ref-count can drop to 0.
The trouble is that most connectors aren't full drivers on their kdev. So this isn't the right lock. We need another lock which protects the drvdata pointer appropriately for drm connectors.
There might be places though where we call code take the device_lock while holding a lock necessary for the drm_connector_get() , so this approach might lead to an AB BA deadlock. As such I think my original approach is better (also see below).
Lookup tables holding full references tends to lead to all kinds of bad side effects.
The proposed reference is not part of a lookup list, it is a reference from the kdev on the drm_connector object which gets dropped as soon as the kdev's refcount hits 0, which normally happens directly after drm_connector_unregister() has run.
Yeah but the way you use it is for lookup purposes. What we're implementing is the "get me the drm_connector for this fwnode" functionality, and that _is_ a lookup.
Ack.
How its implemented is an internal detail really, and somehow using full references for lookup functionality isn't great.
Ok, note that the caller of this only needs the reference for a short while, what the caller does is:
connector = drm_connector_find_by_fwnode(dp->connector_fwnode); if (connector) { drm_connector_oob_hotplug_event(connector, &data); drm_connector_put(connector); }As a result of out discussion I have been thinking about enforcing this short-lifetime of the reference by changing:
void drm_connector_oob_hotplug_event(struct drm_connector *connector, struct drm_connector_oob_hotplug_event_data *data);
to:
void drm_connector_oob_hotplug_event(struct fwnode_handle connector_fwnode, struct drm_connector_oob_hotplug_event_data *data);
And making that do the lookup (+ almost immediate put) internally, making the connector-lookup a purely drm-subsys internal thing and enforcing code outside of the drm-subsys not holding a long-time reference to the connector this way.
Please let me know if you prefer the variant where the connector lookup details are hidden from the callers ?
Yeah I think that's a very nice idea. The kref_get_unless_zero is for when the lookup entry is really decoupled from the lifetime of the object, and you want to be able to get a long term access. If we can outright hide the refcounting, even better.
Also this preps us for a world where v4l would also want to get these oob hotplug event, so that's a nice bonus. It's just the function name that would need to loose the drm_ prefix for that case maybe.
Then I can change this for for v2 of this patch/series.
I'm also not sure why we have to use the kdev stuff here. For other random objects we need to look up we're building that functionality on that object. It means you need to keep another list_head around for that lookup, but that's really not a big cost. E.g. drm_bridge/panel work like that.
Using class_for_each_dev seemed like a good way to iterate over all the connectors. But given the discussion this has caused, just adding a new static list + mutex for this to drivers/gpu/drm/drm_connector.c sounds like it might be a better approach indeed.
Yeah I think kobject lifetimes are complex enough already that if there is no need to tie ourselves to the driver model lifetime rules, our own notifier list with our own locking is better.
Also before Greg rolls in: This imo only applies for notifiers like we're talking about here. When we want actual references to drivers/devices and all that, then it's better to deal with all the kobject model and use it correctly. Because a lot of thought has been put into handling all the corner cases there. E.g. drm_bridge is hitting a lot of the problems which the driver model has solutions for, similarly component.c is also a bit awkward.
Anyway, we still need the refcount dance because calling the oob hotplug with the lock held could deadlock with hotunplug processing and removal of the drm_connector, which needs the same lock. So it's still tricky.
So shall I change thing over to this approach for v2 of this patch/series?
Yeah I think we've explored all the options and found something that fits neatly now for v2. -Daniel
p.s.
On 4/30/21 1:38 PM, Daniel Vetter wrote:
Offtopic:
I'm also not sure why we have to use the kdev stuff here. For other random objects we need to look up we're building that functionality on that object. It means you need to keep another list_head around for that lookup, but that's really not a big cost. E.g. drm_bridge/panel work like that.
So I took a peek at the bridge/panel code and that actually seems to have an issue with removal vs lookup. It is not even just a race, it seems a lookup does not take a reference and there is nothing stopping a user from doing an unbind or rmmod causing the panel to be removed while other code which got a pointer to the panel through of_drm_find_panel() will not be prepared to deal with that pointer all of a sudden no longer being valid.
Now this would be a case of the user shooting his-self in the foot (where as connectors can actually dynamically disappear under normal circumstances), but ideally we really should do better here.
Is there a TODO list somewhere for issues like this ? Or shall I submit a patch adding a FIXME comment, or is this considered not worth the trouble of fixing it?
Regards,
Hans
Hi,
On 4/29/21 1:40 PM, Daniel Vetter wrote:
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it.
Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == &drm_sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Are you sure? I thought sysfs is supposed to flush out any pending operations (they complete fast) and handle open fd internally?
So I did some digging in fs/kernfs and it looks like you right, once the file has been removed from sysfs any accesses through an open fd will fail with -ENODEV, interesting I did not know this.
We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer.
But I obviously need to rewrite the commit message of this commit as it currently is completely wrong.
Maybe I should even squash this into the commit adding drm_connector_find_by_fwnode() ?
Note sure about that though I personally think this is best kept as a new preparation patch but with a new commit msg.
Also I'd assume this creates a loop since the connector holds a reference on the kdev?
So I was wondering the same thing when working on this code and I checked. the reference on the kdev is dropped from: drm_connector_unregister() -> drm_sysfs_connector_remove() and then happens independent of the reference count on the connector-drm-obj dropping to 0.
So what this change does is it keeps a reference to the drm_connector obj as long as someone is keeping a reference to the connnector->kdev device around after drm_connector_unregister() but as soon as that kdev device ref is dropped, so will the drm_connector's obj reference.
I also tested this using a dock with DP MST, which dynamically adds/removes connectors on plug-in / plug-out of the dock-cable and I added a printk to the new drm_sysfs_connector_release() this adds and that printk triggered pretty much immediately on unplug as expected, releasing the extra drm_connector obj as soon as drm_connector_unregister() got called.
Regards,
Hans
-Daniel
drivers/gpu/drm/drm_sysfs.c | 54 +++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index f0336c804639..c344c6d5e738 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -50,6 +50,10 @@ static struct device_type drm_sysfs_device_minor = { .name = "drm_minor" };
+static struct device_type drm_sysfs_device_connector = {
- .name = "drm_connector",
+};
struct class *drm_class;
static char *drm_devnode(struct device *dev, umode_t *mode) @@ -271,30 +275,64 @@ static const struct attribute_group *connector_dev_groups[] = { NULL };
+static void drm_sysfs_connector_release(struct device *dev) +{
- struct drm_connector *connector = to_drm_connector(dev);
- drm_connector_put(connector);
- kfree(dev);
+}
int drm_sysfs_connector_add(struct drm_connector *connector) { struct drm_device *dev = connector->dev;
struct device *kdev;
int r;
if (connector->kdev) return 0;
- connector->kdev =
device_create_with_groups(drm_class, dev->primary->kdev, 0,connector, connector_dev_groups,"card%d-%s", dev->primary->index,connector->name);
- kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
- if (!kdev)
return -ENOMEM;- device_initialize(kdev);
- kdev->class = drm_class;
- kdev->type = &drm_sysfs_device_connector;
- kdev->parent = dev->primary->kdev;
- kdev->groups = connector_dev_groups;
- kdev->release = drm_sysfs_connector_release;
- dev_set_drvdata(kdev, connector);
- r = dev_set_name(kdev, "card%d-%s", dev->primary->index, connector->name);
- if (r)
goto err_free;- DRM_DEBUG("adding "%s" to sysfs\n", connector->name);
- if (IS_ERR(connector->kdev)) {
DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev));return PTR_ERR(connector->kdev);
r = device_add(kdev);
if (r) {
DRM_ERROR("failed to register connector device: %d\n", r);goto err_free;}
/*
* Ensure the connector object does not get free-ed if userspace still has* references open to the device through e.g. the connector sysfs-attributes.*/drm_connector_get(connector);
connector->kdev = kdev;
if (connector->ddc) return sysfs_create_link(&connector->kdev->kobj, &connector->ddc->dev.kobj, "ddc"); return 0;
+err_free:
- put_device(kdev);
- return r;
}
void drm_sysfs_connector_remove(struct drm_connector *connector)
2.31.1
Add a fwnode pointer to struct drm_connector and register an acpi_bus_type for the connectors with the ACPI subsystem (when CONFIG_ACPI is enabled).
The adding of the fwnode pointer allows drivers to associate a fwnode that represents a connector with that connector.
When the new fwnode pointer points to an ACPI-companion, then the new acpi_bus_type will cause the ACPI subsys to bind the device instantiated for the connector with the fwnode by calling acpi_bind_one(). This will result in a firmware_node symlink under /sys/class/card#-<connecter-name>/ which helps to verify that the fwnode-s and connectors are properly matched.
Co-authored-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/drm_sysfs.c | 37 +++++++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 2 ++ 2 files changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index c344c6d5e738..c1d3e2b843b7 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -10,6 +10,7 @@ * Copyright (c) 2003-2004 IBM Corp. */
+#include <linux/acpi.h> #include <linux/device.h> #include <linux/err.h> #include <linux/export.h> @@ -56,6 +57,39 @@ static struct device_type drm_sysfs_device_connector = {
struct class *drm_class;
+#ifdef CONFIG_ACPI +static bool drm_connector_acpi_bus_match(struct device *dev) +{ + return dev->type == &drm_sysfs_device_connector; +} + +static struct acpi_device *drm_connector_acpi_find_companion(struct device *dev) +{ + struct drm_connector *connector = to_drm_connector(dev); + + return to_acpi_device_node(connector->fwnode); +} + +static struct acpi_bus_type drm_connector_acpi_bus = { + .name = "drm_connector", + .match = drm_connector_acpi_bus_match, + .find_companion = drm_connector_acpi_find_companion, +}; + +static void drm_sysfs_acpi_register(void) +{ + register_acpi_bus_type(&drm_connector_acpi_bus); +} + +static void drm_sysfs_acpi_unregister(void) +{ + unregister_acpi_bus_type(&drm_connector_acpi_bus); +} +#else +static void drm_sysfs_acpi_register(void) { } +static void drm_sysfs_acpi_unregister(void) { } +#endif + static char *drm_devnode(struct device *dev, umode_t *mode) { return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev)); @@ -89,6 +123,8 @@ int drm_sysfs_init(void) }
drm_class->devnode = drm_devnode; + + drm_sysfs_acpi_register(); return 0; }
@@ -101,6 +137,7 @@ void drm_sysfs_destroy(void) { if (IS_ERR_OR_NULL(drm_class)) return; + drm_sysfs_acpi_unregister(); class_remove_file(drm_class, &class_attr_version.attr); class_destroy(drm_class); drm_class = NULL; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 0261801af62c..d20bfd7576ed 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1254,6 +1254,8 @@ struct drm_connector { struct device *kdev; /** @attr: sysfs attributes */ struct device_attribute *attr; + /** @fwnode: associated fwnode supplied by platform firmware */ + struct fwnode_handle *fwnode;
/** * @head:
Add a function which allows other subsystems to find a connector based on a fwnode.
This will be used by the USB-Type-C code to be able to find the DP connector to which to send hotplug events received by a Type-C port in DP-alternate-mode.
Note this function is defined in drivers/gpu/drm/drm_sysfs.c because it needs access to the drm_sysfs_device_connector device_type struct.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/drm_sysfs.c | 38 +++++++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 1 + 2 files changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index c1d3e2b843b7..ca71c46fad78 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -520,3 +520,41 @@ void drm_class_device_unregister(struct device *dev) return device_unregister(dev); } EXPORT_SYMBOL_GPL(drm_class_device_unregister); + +/** + * drm_connector_find_by_fwnode - Find a connector based on the associated fwnode + * @fwnode: fwnode for which to find the matching drm_connector + * + * This functions looks up a drm_connector based on its associated fwnode. When + * a connector is found a reference to the connector is returned. The caller must + * call drm_connector_put() to release this reference when it is done with the + * connector. + * + * Returns: A reference to the found connector or NULL if no matching connector was found + */ +struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode) +{ + struct drm_connector *connector, *found = NULL; + struct class_dev_iter iter; + struct device *dev; + + if (!fwnode) + return NULL; + + class_dev_iter_init(&iter, drm_class, NULL, &drm_sysfs_device_connector); + while ((dev = class_dev_iter_next(&iter))) { + connector = to_drm_connector(dev); + + if ((connector->fwnode == fwnode) || + (connector->fwnode && connector->fwnode->secondary == fwnode)) { + drm_connector_get(connector); + found = connector; + break; + } + + } + class_dev_iter_exit(&iter); + + return found; +} +EXPORT_SYMBOL(drm_connector_find_by_fwnode); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index d20bfd7576ed..1e5d154d1f4a 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1696,6 +1696,7 @@ drm_connector_is_unregistered(struct drm_connector *connector) DRM_CONNECTOR_UNREGISTERED; }
+struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode); const char *drm_get_connector_type_name(unsigned int connector_type); const char *drm_get_connector_status_name(enum drm_connector_status status); const char *drm_get_subpixel_order_name(enum subpixel_order order);
Add a new drm_connector_oob_hotplug_event() function and oob_hotplug_event drm_connector_funcs member.
On some hardware a hotplug event notification may come from outside the display driver / device. An example of this is some USB Type-C setups where the hardware muxes the DisplayPort data and aux-lines but does not pass the altmode HPD status bit to the GPU's DP HPD pin.
In cases like this the new drm_connector_oob_hotplug_event() function can be used to report these out-of-band events after obtaining a drm_connector reference through calling drm_connector_find_by_fwnode().
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/drm_connector.c | 20 ++++++++++++++++++++ include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 87c68563e6c3..7d3adc41c11a 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2676,6 +2676,26 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, return ret; }
+/** + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to connector + * @connector: connector to report the event on + * @data: data related to the event + * + * On some hardware a hotplug event notification may come from outside the display + * driver / device. An example of this is some USB Type-C setups where the hardware + * muxes the DisplayPort data and aux-lines but does not pass the altmode HPD + * status bit to the GPU's DP HPD pin. + * + * This function can be used to report these out-of-band events after obtaining + * a drm_connector reference through calling drm_connector_find_by_fwnode(). + */ +void drm_connector_oob_hotplug_event(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data) +{ + if (connector->funcs->oob_hotplug_event) + connector->funcs->oob_hotplug_event(connector, data); +} +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
/** * DOC: Tile group diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 1e5d154d1f4a..6a10f8890809 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -27,6 +27,7 @@ #include <linux/llist.h> #include <linux/ctype.h> #include <linux/hdmi.h> +#include <linux/usb/typec.h> /* For enum typec_orientation */ #include <drm/drm_mode_object.h> #include <drm/drm_util.h>
@@ -649,6 +650,27 @@ struct drm_connector_tv_margins { unsigned int top; };
+/** + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data + * + * Contains data about out-of-band hotplug events, signalled through + * drm_connector_oob_hotplug_event(). + */ +struct drm_connector_oob_hotplug_event_data { + /** + * @connected: New connected status for the connector. + */ + bool connected; + /** + * @dp_lanes: Number of available displayport lanes, 0 if unknown. + */ + int dp_lanes; + /** + * @orientation: Connector orientation. + */ + enum typec_orientation orientation; +}; + /** * struct drm_tv_connector_state - TV connector related states * @subconnector: selected subconnector @@ -1110,6 +1132,15 @@ struct drm_connector_funcs { */ void (*atomic_print_state)(struct drm_printer *p, const struct drm_connector_state *state); + + /** + * @oob_hotplug_event: + * + * This will get called when a hotplug-event for a drm-connector + * has been received from a source outside the display driver / device. + */ + void (*oob_hotplug_event)(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data); };
/** @@ -1697,6 +1728,8 @@ drm_connector_is_unregistered(struct drm_connector *connector) }
struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode); +void drm_connector_oob_hotplug_event(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data); const char *drm_get_connector_type_name(unsigned int connector_type); const char *drm_get_connector_status_name(enum drm_connector_status status); const char *drm_get_subpixel_order_name(enum subpixel_order order);
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
+/**
- struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
- Contains data about out-of-band hotplug events, signalled through
- drm_connector_oob_hotplug_event().
- */
+struct drm_connector_oob_hotplug_event_data {
- /**
* @connected: New connected status for the connector.*/- bool connected;
- /**
* @dp_lanes: Number of available displayport lanes, 0 if unknown.*/- int dp_lanes;
- /**
* @orientation: Connector orientation.*/- enum typec_orientation orientation;
+};
I don't think the orientation is relevant. It will always be "normal" from DP PoW after muxing, no?
I'm also not sure those deatils are enough in the long run. Based on what I've understood from our graphics team guys, for example knowing if multi-function is preferred may be important in some cases.
+Imre.
All of that, and more, is already available in the Configuration VDO Status VDO that the we have negotiated with the DP partner. Both those VDOs are part of struct typec_displayport_data. I think we should simply supply that structure to the DRM code instead of picking those details out of it...
/**
- struct drm_tv_connector_state - TV connector related states
- @subconnector: selected subconnector
@@ -1110,6 +1132,15 @@ struct drm_connector_funcs { */ void (*atomic_print_state)(struct drm_printer *p, const struct drm_connector_state *state);
- /**
* @oob_hotplug_event:** This will get called when a hotplug-event for a drm-connector* has been received from a source outside the display driver / device.*/- void (*oob_hotplug_event)(struct drm_connector *connector,
struct drm_connector_oob_hotplug_event_data *data);
So I would not try to generalise this like that. This callback should be USB Type-C DP altmode specific:
void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_displayport_data *data);
Or like this if the orientation can really be reversed after muxing:
void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_altmode *altmode, struct typec_displayport_data *data);
You can now check the orientation separately with typec_altmode_get_orientation() if necessary.
thanks,
Hi,
On 5/3/21 10:00 AM, Heikki Krogerus wrote:
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
+/**
- struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
- Contains data about out-of-band hotplug events, signalled through
- drm_connector_oob_hotplug_event().
- */
+struct drm_connector_oob_hotplug_event_data {
- /**
* @connected: New connected status for the connector.*/- bool connected;
- /**
* @dp_lanes: Number of available displayport lanes, 0 if unknown.*/- int dp_lanes;
- /**
* @orientation: Connector orientation.*/- enum typec_orientation orientation;
+};
I don't think the orientation is relevant. It will always be "normal" from DP PoW after muxing, no?
That is what I thought to, but during the discussion of my previous attempt at this one of the i915 devs mentioned that in some cases the muxes manage to swap the lane order when the connector upside-down and at least the Intel GPUs can correct for this on the GPU side, so they asked for this info to be included.
I'm also not sure those deatils are enough in the long run. Based on what I've understood from our graphics team guys, for example knowing if multi-function is preferred may be important in some cases.
The current data being passed is just intended as a starting point, this is purely a kernel internal API so we can easily add more data to the struct. As I mentioned in the cover-letter the current oob_hotplug handler which the i915 patch adds to the i915 driver does not actually do anything with the data. ATM it is purely there to demonstrate that the ability to pass relevant data is there now (which was an issue with the previous attempt). I believe the current code is fine as a PoC of "pass event data" once GPU drivers actually start doing something with the data we can extend or outright replace it without issues.
+Imre.
All of that, and more, is already available in the Configuration VDO Status VDO that the we have negotiated with the DP partner. Both those VDOs are part of struct typec_displayport_data. I think we should simply supply that structure to the DRM code instead of picking those details out of it...
I'm not sure I like the idea of passing the raw VDO, but if the DRM folks think that would be useful we can certainly add it.
Regards,
Hans
/**
- struct drm_tv_connector_state - TV connector related states
- @subconnector: selected subconnector
@@ -1110,6 +1132,15 @@ struct drm_connector_funcs { */ void (*atomic_print_state)(struct drm_printer *p, const struct drm_connector_state *state);
- /**
* @oob_hotplug_event:** This will get called when a hotplug-event for a drm-connector* has been received from a source outside the display driver / device.*/- void (*oob_hotplug_event)(struct drm_connector *connector,
struct drm_connector_oob_hotplug_event_data *data);So I would not try to generalise this like that. This callback should be USB Type-C DP altmode specific:
void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_displayport_data *data);
Or like this if the orientation can really be reversed after muxing:
void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_altmode *altmode, struct typec_displayport_data *data);
You can now check the orientation separately with typec_altmode_get_orientation() if necessary.
thanks,
On Mon, May 03, 2021 at 04:35:29PM +0200, Hans de Goede wrote:
Hi,
On 5/3/21 10:00 AM, Heikki Krogerus wrote:
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
+/**
- struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
- Contains data about out-of-band hotplug events, signalled through
- drm_connector_oob_hotplug_event().
- */
+struct drm_connector_oob_hotplug_event_data {
- /**
* @connected: New connected status for the connector.*/- bool connected;
- /**
* @dp_lanes: Number of available displayport lanes, 0 if unknown.*/- int dp_lanes;
- /**
* @orientation: Connector orientation.*/- enum typec_orientation orientation;
+};
I don't think the orientation is relevant. It will always be "normal" from DP PoW after muxing, no?
That is what I thought to, but during the discussion of my previous attempt at this one of the i915 devs mentioned that in some cases the muxes manage to swap the lane order when the connector upside-down and at least the Intel GPUs can correct for this on the GPU side, so they asked for this info to be included.
I'm also not sure those deatils are enough in the long run. Based on what I've understood from our graphics team guys, for example knowing if multi-function is preferred may be important in some cases.
The current data being passed is just intended as a starting point, this is purely a kernel internal API so we can easily add more data to the struct. As I mentioned in the cover-letter the current oob_hotplug handler which the i915 patch adds to the i915 driver does not actually do anything with the data. ATM it is purely there to demonstrate that the ability to pass relevant data is there now (which was an issue with the previous attempt). I believe the current code is fine as a PoC of "pass event data" once GPU drivers actually start doing something with the data we can extend or outright replace it without issues.
Ah, if there is nothing using that information yet, then just don't pass it at all for now. As you said, it's kernel internal API, we can change it later if needed.
All of that, and more, is already available in the Configuration VDO Status VDO that the we have negotiated with the DP partner. Both those VDOs are part of struct typec_displayport_data. I think we should simply supply that structure to the DRM code instead of picking those details out of it...
I'm not sure I like the idea of passing the raw VDO, but if the DRM folks think that would be useful we can certainly add it.
Why are you against passing all the data that we have? What is the benefit in picking only certain details out of an object that has a standard format, and constructing a customised object for those details instead?
thanks,
Hi,
On 5/4/21 4:52 PM, Heikki Krogerus wrote:
On Mon, May 03, 2021 at 04:35:29PM +0200, Hans de Goede wrote:
Hi,
On 5/3/21 10:00 AM, Heikki Krogerus wrote:
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
+/**
- struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
- Contains data about out-of-band hotplug events, signalled through
- drm_connector_oob_hotplug_event().
- */
+struct drm_connector_oob_hotplug_event_data {
- /**
* @connected: New connected status for the connector.*/- bool connected;
- /**
* @dp_lanes: Number of available displayport lanes, 0 if unknown.*/- int dp_lanes;
- /**
* @orientation: Connector orientation.*/- enum typec_orientation orientation;
+};
I don't think the orientation is relevant. It will always be "normal" from DP PoW after muxing, no?
That is what I thought to, but during the discussion of my previous attempt at this one of the i915 devs mentioned that in some cases the muxes manage to swap the lane order when the connector upside-down and at least the Intel GPUs can correct for this on the GPU side, so they asked for this info to be included.
I'm also not sure those deatils are enough in the long run. Based on what I've understood from our graphics team guys, for example knowing if multi-function is preferred may be important in some cases.
The current data being passed is just intended as a starting point, this is purely a kernel internal API so we can easily add more data to the struct. As I mentioned in the cover-letter the current oob_hotplug handler which the i915 patch adds to the i915 driver does not actually do anything with the data. ATM it is purely there to demonstrate that the ability to pass relevant data is there now (which was an issue with the previous attempt). I believe the current code is fine as a PoC of "pass event data" once GPU drivers actually start doing something with the data we can extend or outright replace it without issues.
Ah, if there is nothing using that information yet, then just don't pass it at all for now. As you said, it's kernel internal API, we can change it later if needed.
All of that, and more, is already available in the Configuration VDO Status VDO that the we have negotiated with the DP partner. Both those VDOs are part of struct typec_displayport_data. I think we should simply supply that structure to the DRM code instead of picking those details out of it...
I'm not sure I like the idea of passing the raw VDO, but if the DRM folks think that would be useful we can certainly add it.
Why are you against passing all the data that we have? What is the benefit in picking only certain details out of an object that has a standard format, and constructing a customised object for those details instead?
The VDO is Type-C specific and the drm_connector_oob_hotplug_event() is intended to be a generic API.
There are other OOB event sources, e.g. the drivers/apci/acpi_video.c code receives hotplug events for connectors on powered-down GPUs on dual/hybrid GPU laptops. ATM the GPU drivers register an ACPI notifier to catch these; and there are no immediate plans to change this, but this does illustrate how OOB hotplug notification is not just a Type-C thing, where as the VDO and its format very much are Type-C things.
Regards,
Hans
On Mon, May 03, 2021 at 11:00:20AM +0300, Heikki Krogerus wrote:
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
+/**
- struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
- Contains data about out-of-band hotplug events, signalled through
- drm_connector_oob_hotplug_event().
- */
+struct drm_connector_oob_hotplug_event_data {
- /**
* @connected: New connected status for the connector.*/- bool connected;
- /**
* @dp_lanes: Number of available displayport lanes, 0 if unknown.*/- int dp_lanes;
- /**
* @orientation: Connector orientation.*/- enum typec_orientation orientation;
+};
I don't think the orientation is relevant. It will always be "normal" from DP PoW after muxing, no?
I'm also not sure those deatils are enough in the long run. Based on what I've understood from our graphics team guys, for example knowing if multi-function is preferred may be important in some cases.
Combo PHY ports - which is what this patchset is adding the notification for - can only reverse the lane assignment. TypeC PHY ports (on ICL+) have a more C-type aware mux in the SoC (FIA) as well, so in theory we could have a system based on such platforms with an external mux only switching between the USB, DP, USB+DP (MFD) modes, but leaving the plug orientation specific muxing up to the FIA. The graphics driver is not involved in programming the FIA though, it's done by a firmware component, so I don't think this configuration needs to get passed.
Yes, the driver needs to know if the PD controller configured the sink in the MFD mode (DP+USB) or in the DP-only mode. For that the number of lanes assigned to DP is enough.
+Imre.
All of that, and more, is already available in the Configuration VDO Status VDO that the we have negotiated with the DP partner. Both those VDOs are part of struct typec_displayport_data. I think we should simply supply that structure to the DRM code instead of picking those details out of it...
/**
- struct drm_tv_connector_state - TV connector related states
- @subconnector: selected subconnector
@@ -1110,6 +1132,15 @@ struct drm_connector_funcs { */ void (*atomic_print_state)(struct drm_printer *p, const struct drm_connector_state *state);
- /**
* @oob_hotplug_event:** This will get called when a hotplug-event for a drm-connector* has been received from a source outside the display driver / device.*/- void (*oob_hotplug_event)(struct drm_connector *connector,
struct drm_connector_oob_hotplug_event_data *data);So I would not try to generalise this like that. This callback should be USB Type-C DP altmode specific:
void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_displayport_data *data);
Or like this if the orientation can really be reversed after muxing:
void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_altmode *altmode, struct typec_displayport_data *data);
You can now check the orientation separately with typec_altmode_get_orientation() if necessary.
thanks,
-- heikki
From: Heikki Krogerus heikki.krogerus@linux.intel.com
On Intel platforms we know that the ACPI connector device node order will follow the order the driver (i915) decides. The decision is made using the custom Intel ACPI OpRegion (intel_opregion.c), though the driver does not actually know that the values it sends to ACPI there are used for associating a device node for the connectors, and assigning address for them.
In reality that custom Intel ACPI OpRegion actually violates ACPI specification (we supply dynamic information to objects that are defined static, for example _ADR), however, it makes assigning correct connector node for a connector entry straightforward (it's one-on-one mapping).
Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com [hdegoede@redhat.com: Move intel_acpi_assign_connector_fwnodes() to intel_acpi.c] Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/display/intel_acpi.c | 40 ++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_acpi.h | 3 ++ drivers/gpu/drm/i915/display/intel_display.c | 1 + 3 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c index 833d0c1be4f1..9f266dfda7dd 100644 --- a/drivers/gpu/drm/i915/display/intel_acpi.c +++ b/drivers/gpu/drm/i915/display/intel_acpi.c @@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv) } drm_connector_list_iter_end(&conn_iter); } + +/* NOTE: The connector order must be final before this is called. */ +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) +{ + struct drm_connector_list_iter conn_iter; + struct drm_device *drm_dev = &i915->drm; + struct device *kdev = &drm_dev->pdev->dev; + struct fwnode_handle *fwnode = NULL; + struct drm_connector *connector; + struct acpi_device *adev; + + drm_connector_list_iter_begin(drm_dev, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + /* Always getting the next, even when the last was not used. */ + fwnode = device_get_next_child_node(kdev, fwnode); + if (!fwnode) + break; + + switch (connector->connector_type) { + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + case DRM_MODE_CONNECTOR_DSI: + /* + * Integrated displays have a specific address 0x1f on + * most Intel platforms, but not on all of them. + */ + adev = acpi_find_child_device(ACPI_COMPANION(kdev), + 0x1f, 0); + if (adev) { + connector->fwnode = acpi_fwnode_handle(adev); + break; + } + fallthrough; + default: + connector->fwnode = fwnode; + break; + } + } + drm_connector_list_iter_end(&conn_iter); +} diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h index e8b068661d22..d2435691f4b5 100644 --- a/drivers/gpu/drm/i915/display/intel_acpi.h +++ b/drivers/gpu/drm/i915/display/intel_acpi.h @@ -12,11 +12,14 @@ struct drm_i915_private; void intel_register_dsm_handler(void); void intel_unregister_dsm_handler(void); void intel_acpi_device_id_update(struct drm_i915_private *i915); +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915); #else static inline void intel_register_dsm_handler(void) { return; } static inline void intel_unregister_dsm_handler(void) { return; } static inline void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; } +static inline +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { return; } #endif /* CONFIG_ACPI */
#endif /* __INTEL_ACPI_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 828ef4c5625f..87cad549632c 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915)
drm_modeset_lock_all(dev); intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); + intel_acpi_assign_connector_fwnodes(i915); drm_modeset_unlock_all(dev);
for_each_intel_crtc(dev, crtc) {
On some Cherry Trail devices, DisplayPort over Type-C is supported through a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this case does the PD/alt-mode negotiation itself, rather then everything being handled in firmware.
So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch to DP mode and sets the mux accordingly. In this setup the HPD pin is not connected, so the i915 driver needs to respond to a software event and scan the DP port for changes manually.
This commit adds support for this. Together with the recent addition of DP alt-mode support to the Type-C subsystem this makes DP over Type-C work on these devices.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 64a73b3ced94..1029720cc945 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5846,6 +5846,18 @@ static int intel_dp_connector_atomic_check(struct drm_connector *conn, return intel_modeset_synced_crtcs(state, conn); }
+static void intel_dp_oob_hotplug_event(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data) +{ + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); + struct drm_i915_private *i915 = to_i915(connector->dev); + + spin_lock_irq(&i915->irq_lock); + i915->hotplug.event_bits |= BIT(encoder->hpd_pin); + spin_unlock_irq(&i915->irq_lock); + queue_delayed_work(system_wq, &i915->hotplug.hotplug_work, 0); +} + static const struct drm_connector_funcs intel_dp_connector_funcs = { .force = intel_dp_force, .fill_modes = drm_helper_probe_single_connector_modes, @@ -5856,6 +5868,7 @@ static const struct drm_connector_funcs intel_dp_connector_funcs = { .destroy = intel_connector_destroy, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, .atomic_duplicate_state = intel_digital_connector_duplicate_state, + .oob_hotplug_event = intel_dp_oob_hotplug_event, };
static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = {
Make dp_altmode_notify() handle the dp->data.conf == 0 case too, rather then having separate code-paths for this in various places which call it.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/typec/altmodes/displayport.c | 35 +++++++++--------------- 1 file changed, 13 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c index b7f094435b00..aa669b9cf70e 100644 --- a/drivers/usb/typec/altmodes/displayport.c +++ b/drivers/usb/typec/altmodes/displayport.c @@ -66,10 +66,17 @@ struct dp_altmode {
static int dp_altmode_notify(struct dp_altmode *dp) { - u8 state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf)); + unsigned long conf; + u8 state; + + if (dp->data.conf) { + state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf)); + conf = TYPEC_MODAL_STATE(state); + } else { + conf = TYPEC_STATE_USB; + }
- return typec_altmode_notify(dp->alt, TYPEC_MODAL_STATE(state), - &dp->data); + return typec_altmode_notify(dp->alt, conf, &dp->data); }
static int dp_altmode_configure(struct dp_altmode *dp, u8 con) @@ -137,21 +144,10 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
static int dp_altmode_configured(struct dp_altmode *dp) { - int ret; - sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration"); - - if (!dp->data.conf) - return typec_altmode_notify(dp->alt, TYPEC_STATE_USB, - &dp->data); - - ret = dp_altmode_notify(dp); - if (ret) - return ret; - sysfs_notify(&dp->alt->dev.kobj, "displayport", "pin_assignment");
- return 0; + return dp_altmode_notify(dp); }
static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf) @@ -172,13 +168,8 @@ static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf) }
ret = typec_altmode_vdm(dp->alt, header, &conf, 2); - if (ret) { - if (DP_CONF_GET_PIN_ASSIGN(dp->data.conf)) - dp_altmode_notify(dp); - else - typec_altmode_notify(dp->alt, TYPEC_STATE_USB, - &dp->data); - } + if (ret) + dp_altmode_notify(dp);
return ret; }
Use the new drm_connector_find_by_fwnode() and drm_connector_oob_hotplug_event() functions to let drm/kms drivers know about DisplayPort over Type-C hotplug events.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/typec/altmodes/displayport.c | 45 +++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c index aa669b9cf70e..c95ccd2a965c 100644 --- a/drivers/usb/typec/altmodes/displayport.c +++ b/drivers/usb/typec/altmodes/displayport.c @@ -11,8 +11,10 @@ #include <linux/delay.h> #include <linux/mutex.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/usb/pd_vdo.h> #include <linux/usb/typec_dp.h> +#include <drm/drm_connector.h> #include "displayport.h"
#define DP_HEADER(_dp, ver, cmd) (VDO((_dp)->alt->svid, 1, ver, cmd) \ @@ -62,12 +64,35 @@ struct dp_altmode { struct work_struct work; struct typec_altmode *alt; const struct typec_altmode *port; + struct fwnode_handle *connector_fwnode; };
+static void dp_altmode_notify_connector(struct dp_altmode *dp) +{ + struct drm_connector_oob_hotplug_event_data data = { }; + u8 pin_assign = DP_CONF_GET_PIN_ASSIGN(dp->data.conf); + struct drm_connector *connector; + + data.connected = dp->data.status & DP_STATUS_HPD_STATE; + data.orientation = typec_altmode_get_orientation(dp->alt); + + if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK) + data.dp_lanes = 4; + else if (pin_assign & DP_PIN_ASSIGN_MULTI_FUNC_MASK) + data.dp_lanes = 2; + + connector = drm_connector_find_by_fwnode(dp->connector_fwnode); + if (connector) { + drm_connector_oob_hotplug_event(connector, &data); + drm_connector_put(connector); + } +} + static int dp_altmode_notify(struct dp_altmode *dp) { unsigned long conf; u8 state; + int ret;
if (dp->data.conf) { state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf)); @@ -76,7 +101,12 @@ static int dp_altmode_notify(struct dp_altmode *dp) conf = TYPEC_STATE_USB; }
- return typec_altmode_notify(dp->alt, conf, &dp->data); + ret = typec_altmode_notify(dp->alt, conf, &dp->data); + if (ret) + return ret; + + dp_altmode_notify_connector(dp); + return 0; }
static int dp_altmode_configure(struct dp_altmode *dp, u8 con) @@ -512,6 +542,7 @@ static const struct attribute_group dp_altmode_group = { int dp_altmode_probe(struct typec_altmode *alt) { const struct typec_altmode *port = typec_altmode_get_partner(alt); + struct fwnode_handle *fwnode; struct dp_altmode *dp; int ret;
@@ -540,6 +571,11 @@ int dp_altmode_probe(struct typec_altmode *alt) alt->desc = "DisplayPort"; alt->ops = &dp_altmode_ops;
+ fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */ + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0); + if (IS_ERR(dp->connector_fwnode)) + dp->connector_fwnode = NULL; + typec_altmode_set_drvdata(alt, dp);
dp->state = DP_STATE_ENTER; @@ -555,6 +591,13 @@ void dp_altmode_remove(struct typec_altmode *alt)
sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group); cancel_work_sync(&dp->work); + + if (dp->connector_fwnode) { + dp->data.conf = 0; + dp->data.status = 0; + dp_altmode_notify_connector(dp); + fwnode_handle_put(dp->connector_fwnode); + } } EXPORT_SYMBOL_GPL(dp_altmode_remove);
The Type-C connector on these devices is connected to DP-2 not DP-1, so the reference must be to the DD04 child-node of the GPU, rather then the DD02 child-node.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/platform/x86/intel_cht_int33fe_typec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe_typec.c b/drivers/platform/x86/intel_cht_int33fe_typec.c index b61bad9cc8d2..d59544167430 100644 --- a/drivers/platform/x86/intel_cht_int33fe_typec.c +++ b/drivers/platform/x86/intel_cht_int33fe_typec.c @@ -168,8 +168,8 @@ static int cht_int33fe_setup_dp(struct cht_int33fe_data *data) return -ENODEV; }
- /* Then the DP child device node */ - data->dp = device_get_named_child_node(&pdev->dev, "DD02"); + /* Then the DP-2 child device node */ + data->dp = device_get_named_child_node(&pdev->dev, "DD04"); pci_dev_put(pdev); if (!data->dp) return -ENODEV;
Hi,
On 4/28/21 11:52 PM, Hans de Goede wrote:
Hi All,
Here is a new attempt to make DP over Type-C work on devices where the Type-C controller does not drive the HPD pin on the GPU, but instead we need to forward HPD events from the Type-C controller to the DRM driver.
For anyone interested here are the old (2019!) patches for this:
https://patchwork.freedesktop.org/patch/288491/ https://patchwork.freedesktop.org/patch/288493/ https://patchwork.freedesktop.org/patch/288495/
Last time I posted this the biggest change requested was for more info to be included in the event send to the DRM-subsystem, specifically sending the following info was requested:
- Which DP connector on the GPU the event is for
- How many lanes are available
- Connector orientation
This series is basically an entirely new approach, which no longer uses the notifier framework at all. Instead the Type-C code looksup a connector based on a fwnode (this was suggested by Heikki Krogerus) and then calls a new oob_hotplug_event drm_connector_func directly on the connector, passing the requested info as argument.
p.s.
Info such as the orientation and the number of dp-lanes is now passed to the drm_connector_oob_hotplug_event() function as requested in the review of the old code, but nothing is done with it for now. Using this info falls well outside of my knowledge of the i915 driver so this is left to a follow-up patch (I will be available to test patches for this).
Regards,
Hans
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Greg Kroah-Hartman -
Hans de Goede -
Heikki Krogerus -
Imre Deak