[PATCH 0/3] Adjust behavior for HPD sensor

[PATCH 0/3] Adjust behavior for HPD sensor

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Some platforms include a human presence detection (HPD) sensor. When
enabled and a user is detected a wake event will be emitted from the
sensor fusion hub that software can react to.

Example use cases are "wake from suspend on approach" or to "lock
when leaving".

This is currently enabled by default on supported systems, but users
can't control it. This essentially means that wake on approach is
enabled which is a really surprising behavior to users that don't
expect it.

Instead of defaulting to enabled add a sysfs knob that users can
use to enable the feature if desirable and set it to disabled by
default.

Cc: Pratap Nirujogi <pratap.nirujogi@amd.com>
Cc: Anson Tsao  <anson.tsao@amd.com>

Mario Limonciello (3):
  HID: amd_sfh: Allow configuring whether HPD is enabled or disabled
  HID: amd_sfh: Default to HPD disabled
  HID: amd_sfh: Don't show wrong status for amd_sfh_hpd_info()

 .../ABI/testing/sysfs-driver-amd-sfh          | 13 +++++
 drivers/hid/amd-sfh-hid/amd_sfh_common.h      |  1 +
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        | 58 +++++++++++++++++++
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 50 +++++++++++++++-
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h |  3 +
 .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c    |  2 +-
 6 files changed, 124 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-amd-sfh

-- 
2.43.0

[PATCH 1/3] HID: amd_sfh: Allow configuring whether HPD is enabled or disabled

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Human presence detection (HPD) sensor uses a camera to determine when
user is physically in front of the machine.  This might not be a
desirable behavior because it can (for example) cause the machine to
wake on approach.

Add a new sysfs file "hpd" that will control whether this sensor is
enabled. Use the value of this sysfs file to turn off HPD and prevent
it from re-enabling after resume from suspend.

Cc: Pratap Nirujogi <pratap.nirujogi@amd.com>
Tested-by: Anson Tsao <anson.tsao@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 .../ABI/testing/sysfs-driver-amd-sfh          | 13 +++++
 drivers/hid/amd-sfh-hid/amd_sfh_common.h      |  1 +
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        | 58 +++++++++++++++++++
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 49 +++++++++++++++-
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h |  3 +
 5 files changed, 122 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-amd-sfh

diff --git a/Documentation/ABI/testing/sysfs-driver-amd-sfh b/Documentation/ABI/testing/sysfs-driver-amd-sfh
new file mode 100644
index 0000000000000..c053126a83bb3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-amd-sfh
@@ -0,0 +1,13 @@
+What:		/sys/bus/pci/drivers/pcie_mp2_amd/*/hpd
+Date:		April 2025
+Contact:	mario.limonciello@amd.com
+Description:
+		Human presence detection (HPD) enable/disable.
+		When HPD is enabled, the device will be able to detect the
+		presence of a human and will send an interrupt that can be
+		used to wake the system from a low power state.
+		When HPD is disabled, the device will not be able to detect
+		the presence of a human.
+
+		Access: Read/Write
+		Valid values: enabled/disabled
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
index 799b8686a88a4..f44a3bb2fbd4f 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
@@ -42,6 +42,7 @@ struct amd_mp2_sensor_info {
 
 struct sfh_dev_status {
 	bool is_hpd_present;
+	bool is_hpd_enabled;
 	bool is_als_present;
 	bool is_sra_present;
 };
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 48cfd0c582414..1c1fd63330c93 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -18,6 +18,7 @@
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/string_choices.h>
 
 #include "amd_sfh_pcie.h"
 #include "sfh1_1/amd_sfh_init.h"
@@ -330,6 +331,57 @@ static const struct dmi_system_id dmi_nodevs[] = {
 	{ }
 };
 
+static ssize_t hpd_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct amd_mp2_dev *mp2 = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%s\n", str_enabled_disabled(mp2->dev_en.is_hpd_enabled));
+}
+
+static ssize_t hpd_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	struct amd_mp2_dev *mp2 = dev_get_drvdata(dev);
+	bool enabled;
+	int ret;
+
+	ret = kstrtobool(buf, &enabled);
+	if (ret)
+		return ret;
+
+	mp2->sfh1_1_ops->toggle_hpd(mp2, enabled);
+
+	return count;
+}
+static DEVICE_ATTR_RW(hpd);
+
+static umode_t sfh_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct amd_mp2_dev *mp2 = dev_get_drvdata(dev);
+
+	if (!mp2->sfh1_1_ops || !mp2->dev_en.is_hpd_present)
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute *sfh_attrs[] = {
+	&dev_attr_hpd.attr,
+	NULL,
+};
+
+static struct attribute_group sfh_attr_group = {
+	.attrs = sfh_attrs,
+	.is_visible = sfh_attr_is_visible,
+};
+
+static const struct attribute_group *amd_sfh_groups[] = {
+	&sfh_attr_group,
+	NULL,
+};
+
 static void sfh1_1_init_work(struct work_struct *work)
 {
 	struct amd_mp2_dev *mp2 = container_of(work, struct amd_mp2_dev, work);
@@ -341,6 +393,11 @@ static void sfh1_1_init_work(struct work_struct *work)
 
 	amd_sfh_clear_intr(mp2);
 	mp2->init_done = 1;
+
+	rc = sysfs_update_group(&mp2->pdev->dev.kobj, &sfh_attr_group);
+	if (rc)
+		dev_warn(&mp2->pdev->dev, "failed to update sysfs group\n");
+
 }
 
 static void sfh_init_work(struct work_struct *work)
@@ -487,6 +544,7 @@ static struct pci_driver amd_mp2_pci_driver = {
 	.driver.pm	= &amd_mp2_pm_ops,
 	.shutdown	= amd_sfh_shutdown,
 	.remove		= amd_sfh_remove,
+	.dev_groups	= amd_sfh_groups,
 };
 module_pci_driver(amd_mp2_pci_driver);
 
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index e9929c4aa72eb..fc9c297d0db7f 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -212,6 +212,7 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
 			switch (cl_data->sensor_idx[i]) {
 			case HPD_IDX:
 				privdata->dev_en.is_hpd_present = true;
+				privdata->dev_en.is_hpd_enabled = true;
 				break;
 			case ALS_IDX:
 				privdata->dev_en.is_als_present = true;
@@ -255,6 +256,10 @@ static void amd_sfh_resume(struct amd_mp2_dev *mp2)
 	}
 
 	for (i = 0; i < cl_data->num_hid_devices; i++) {
+		/* leave HPD alone; policy is controlled by sysfs */
+		if (cl_data->sensor_idx[i] == HPD_IDX)
+			continue;
+
 		if (cl_data->sensor_sts[i] == SENSOR_DISABLED) {
 			info.sensor_idx = cl_data->sensor_idx[i];
 			mp2->mp2_ops->start(mp2, info);
@@ -285,8 +290,10 @@ static void amd_sfh_suspend(struct amd_mp2_dev *mp2)
 	}
 
 	for (i = 0; i < cl_data->num_hid_devices; i++) {
-		if (cl_data->sensor_idx[i] != HPD_IDX &&
-		    cl_data->sensor_sts[i] == SENSOR_ENABLED) {
+		/* leave HPD alone; policy is controlled by sysfs */
+		if (cl_data->sensor_idx[i] == HPD_IDX)
+			continue;
+		if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
 			mp2->mp2_ops->stop(mp2, cl_data->sensor_idx[i]);
 			status = amd_sfh_wait_for_response
 					(mp2, cl_data->sensor_idx[i], DISABLE_SENSOR);
@@ -304,6 +311,44 @@ static void amd_sfh_suspend(struct amd_mp2_dev *mp2)
 	amd_sfh_clear_intr(mp2);
 }
 
+void amd_sfh_toggle_hpd(struct amd_mp2_dev *mp2, bool enabled)
+{
+	struct amdtp_cl_data *cl_data = mp2->cl_data;
+	struct amd_mp2_sensor_info info;
+	int i, status;
+
+	if (mp2->dev_en.is_hpd_enabled == enabled)
+		return;
+
+	for (i = 0; i < cl_data->num_hid_devices; i++) {
+		if (cl_data->sensor_idx[i] != HPD_IDX)
+			continue;
+		info.sensor_idx = cl_data->sensor_idx[i];
+		if (enabled) {
+			mp2->mp2_ops->start(mp2, info);
+			status = amd_sfh_wait_for_response
+					(mp2, cl_data->sensor_idx[i], ENABLE_SENSOR);
+			if (status == 0)
+				status = SENSOR_ENABLED;
+			if (status == SENSOR_ENABLED)
+				cl_data->sensor_sts[i] = SENSOR_ENABLED;
+		} else {
+			mp2->mp2_ops->stop(mp2, cl_data->sensor_idx[i]);
+			status = amd_sfh_wait_for_response
+					(mp2, cl_data->sensor_idx[i], DISABLE_SENSOR);
+			if (status == 0)
+				status = SENSOR_DISABLED;
+			if (status != SENSOR_ENABLED)
+				cl_data->sensor_sts[i] = SENSOR_DISABLED;
+		}
+		dev_dbg(&mp2->pdev->dev, "toggle sid 0x%x (%s) status 0x%x\n",
+			cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]),
+			cl_data->sensor_sts[i]);
+		break;
+	}
+	mp2->dev_en.is_hpd_enabled = enabled;
+}
+
 static void amd_mp2_pci_remove(void *privdata)
 {
 	struct amd_mp2_dev *mp2 = privdata;
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h
index 21c44990bbeba..797d206641c69 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h
@@ -15,12 +15,15 @@
 
 struct amd_sfh1_1_ops {
 	int (*init)(struct amd_mp2_dev *mp2);
+	void (*toggle_hpd)(struct amd_mp2_dev *mp2, bool enable);
 };
 
 int amd_sfh1_1_init(struct amd_mp2_dev *mp2);
+void amd_sfh_toggle_hpd(struct amd_mp2_dev *mp2, bool enabled);
 
 static const struct amd_sfh1_1_ops __maybe_unused sfh1_1_ops = {
 	.init = amd_sfh1_1_init,
+	.toggle_hpd = amd_sfh_toggle_hpd,
 };
 
 #endif
-- 
2.43.0

[PATCH 2/3] HID: amd_sfh: Default to HPD disabled

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Unless you know to look for it, HPD is a surprising behavior; particularly
because it can wake the system from suspend. It also has implications for
power consumption because sensors are left enabled.

After the sensors have been probed (and HPD is found present), explicitly
turn off HPD by default. Userspace can manually turn it on if desirable.

Cc: Pratap Nirujogi <pratap.nirujogi@amd.com>
Tested-by: Anson Tsao <anson.tsao@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index fc9c297d0db7f..25f0ebfcbd5f5 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -213,6 +213,7 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
 			case HPD_IDX:
 				privdata->dev_en.is_hpd_present = true;
 				privdata->dev_en.is_hpd_enabled = true;
+				amd_sfh_toggle_hpd(privdata, false);
 				break;
 			case ALS_IDX:
 				privdata->dev_en.is_als_present = true;
-- 
2.43.0

[PATCH 3/3] HID: amd_sfh: Don't show wrong status for amd_sfh_hpd_info()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When HPD is present but has been disabled, avoid reporting HPD status
to PMF.

Cc: Pratap Nirujogi <pratap.nirujogi@amd.com>
Tested-by: Anson Tsao <anson.tsao@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
index ffb98b4c36cbd..837d59e7a6610 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
@@ -129,7 +129,7 @@ static int amd_sfh_hpd_info(u8 *user_present)
 	if (!user_present)
 		return -EINVAL;
 
-	if (!emp2 || !emp2->dev_en.is_hpd_present)
+	if (!emp2 || !emp2->dev_en.is_hpd_present || !emp2->dev_en.is_hpd_enabled)
 		return -ENODEV;
 
 	hpdstatus.val = readl(emp2->mmio + amd_get_c2p_val(emp2, 4));
-- 
2.43.0

Re: [PATCH 0/3] Adjust behavior for HPD sensor

[Jiri Kosina]

On Fri, 28 Feb 2025, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Some platforms include a human presence detection (HPD) sensor. When
> enabled and a user is detected a wake event will be emitted from the
> sensor fusion hub that software can react to.
> 
> Example use cases are "wake from suspend on approach" or to "lock
> when leaving".
> 
> This is currently enabled by default on supported systems, but users
> can't control it. This essentially means that wake on approach is
> enabled which is a really surprising behavior to users that don't
> expect it.
> 
> Instead of defaulting to enabled add a sysfs knob that users can
> use to enable the feature if desirable and set it to disabled by
> default.
> 
> Cc: Pratap Nirujogi <pratap.nirujogi@amd.com>
> Cc: Anson Tsao  <anson.tsao@amd.com>
> 
> Mario Limonciello (3):
>   HID: amd_sfh: Allow configuring whether HPD is enabled or disabled
>   HID: amd_sfh: Default to HPD disabled
>   HID: amd_sfh: Don't show wrong status for amd_sfh_hpd_info()
> 
>  .../ABI/testing/sysfs-driver-amd-sfh          | 13 +++++
>  drivers/hid/amd-sfh-hid/amd_sfh_common.h      |  1 +
>  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c        | 58 +++++++++++++++++++
>  drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 50 +++++++++++++++-
>  drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.h |  3 +
>  .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c    |  2 +-
>  6 files changed, 124 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-amd-sfh

Now applied to hid.git#for-6.14/amd_sfh. Thanks,

-- 
Jiri Kosina
SUSE Labs