[PATCH 0/9] drm + usb-type-c: Add support for out-of-band hotplug notification

[PATCH 0/9] drm + usb-type-c: Add support for out-of-band hotplug notification

[Hans de Goede]

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

-- 
2.31.1

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

[PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Hans de Goede]

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

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

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Greg Kroah-Hartman]

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

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

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Hans de Goede]

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


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

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Hans de Goede]

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

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

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Hans de Goede]

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

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

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Daniel Vetter]

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

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Hans de Goede]

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

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

Re: [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

[Hans de Goede]

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

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

[PATCH 2/9] drm/connector: Add a fwnode pointer to drm_connector and register with ACPI

[Hans de Goede]

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

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

[PATCH 3/9] drm/connector: Add drm_connector_find_by_fwnode() function

[Hans de Goede]

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

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

[PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification

[Hans de Goede]

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

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

Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification

[Heikki Krogerus]

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,

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

Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification

[Hans de Goede]

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

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

Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification

[Heikki Krogerus]

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,

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

Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification

[Hans de Goede]

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

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

Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification

[Imre Deak]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede]

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) {
-- 
2.31.1

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

[PATCH 6/9] drm/i915/dp: Add support for out-of-bound hotplug events

[Hans de Goede]

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 = {
-- 
2.31.1

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

[PATCH 7/9] usb: typec: altmodes/displayport: Make dp_altmode_notify() more generic

[Hans de Goede]

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;
 }
-- 
2.31.1

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

[PATCH 8/9] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[Hans de Goede]

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

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

[PATCH 9/9] platform/x86/intel_cht_int33fe: Correct "displayport" fwnode reference

[Hans de Goede]

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

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

Re: [PATCH 0/9] drm + usb-type-c: Add support for out-of-band hotplug notification

[Hans de Goede]

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

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel