drm/nouveau: Fix DP AUX RPM issues

PATCH In review Lyude Paul 2 participants 5 messages Nov 17, 2018 – Nov 26, 2018

Subsystems (3)

DRM drivers DRM misc nouveau

lore

Share & tools

lore

Lyude Paul

author maintainer

2018-11-17 01:50 UTC #0 lore raw

cover drm/nouveau: Fix DP AUX RPM issues

Here's some fixes for the less important DP AUX issues I mentioned a while back.

Lyude Paul (2):
drm/dp: Add ->pre/post_transfer() hooks for drm_dp_aux drm/nouveau: Grab an rpm reference before/after DP AUX transactions

 drivers/gpu/drm/drm_dp_helper.c             |  5 ++
 drivers/gpu/drm/nouveau/nouveau_connector.c | 36 ++++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c       | 12 ++-
 drivers/gpu/drm/nouveau/nouveau_drv.h       |  8 ++
 include/drm/drm_dp_helper.h                 | 91 +++++++++++++++++++++
 5 files changed, 151 insertions(+), 1 deletion(-)

Signature · 1 line

2.19.1

Lyude Paul

author maintainer

2018-11-17 01:50 UTC #1 lore raw

PATCH 1/2 drm/dp: Add ->pre/post_transfer() hooks for drm_dp_aux

Many DRM drivers unfortunately need to be able to access the DP AUX channel during their suspend/resume callbacks. This leads to an annoying
catch-22: drivers which try to ensure that the DP AUX channel is initialized and ready may need to runtime-resume the device housing the channel, which would lead to a deadlock between runtime power management and drm_dp_aux->hw_mutex.

So: add a simple set of optional hooks that drivers can implement in order to perform such setup before hw_mutex is locked, then clean up afterwards. We additionally add the drm_dp_aux_get() and drm_dp_aux_put() functions so that users of the AUX channel that need to prepare the AUX channel ahead of time to avoid other kinds of locking version can do so. We'll need this if we ever want to have a universal dp_mst_status debugfs node, since dumping the MST topology without having the AUX channel prepared beforehand would lead to lock inversion.

Signed-off-by Lyude Paul <[email protected]>

---
 drivers/gpu/drm/drm_dp_helper.c |  5 ++
 include/drm/drm_dp_helper.h     | 91 +++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +5 -0

index 6d483487f2b4..fb1912a2f246 100644
@@ -224,6 +224,10 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 	msg.buffer = buffer;
 	msg.size = size;
 
+	ret = drm_dp_aux_get(aux);
+	if (ret)
+		return ret;
+
 	mutex_lock(&aux->hw_mutex);
 
 	/*

@@ -265,6 +269,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 
 unlock:
 	mutex_unlock(&aux->hw_mutex);
+	drm_dp_aux_put(aux);
 	return ret;
 }

include/drm/drm_dp_helper.h +91 -0

103 lines not on this page · raw message

Signature · 1 line

2.19.1

Lyude Paul

author maintainer

2018-11-17 01:50 UTC #2 lore raw

PATCH 2/2 drm/nouveau: Grab an rpm reference before/after DP AUX transactions

In reply to Lyude Paul

Now that we have ->pre_transfer() and ->post_transfer() for DP AUX channel devices, we can implement these hooks in order to ensure that the GPU is actually woken up before AUX transactions happen. This fixes /dev/drm_dp_aux* not working while the GPU is suspended, along with some more rare issues where the GPU might runtime-suspend if the time between two DP AUX channel transactions ends up being longer then the runtime suspend delay (sometimes observed on KASAN kernels where everything is slow).

Additionally, we add tracking for the current task that's running our runtime suspend/resume callbacks. We need this in order to avoid trying to grab a runtime power reference when nouveau uses the DP AUX channel for MST suspend/resume in it's runtime susped/resume callbacks.

Signed-off-by Lyude Paul <[email protected]>

---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 36 +++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c       | 12 ++++++-
 drivers/gpu/drm/nouveau/nouveau_drv.h       |  8 +++++
 3 files changed, 55 insertions(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_connector.c +36 -0

index fd80661dff92..d2e9752f2f91 100644
@@ -1171,6 +1171,38 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
 	return NVIF_NOTIFY_KEEP;
 }
 
+static int
+nouveau_connector_aux_pre_xfer(struct drm_dp_aux *obj)
+{
+	struct nouveau_connector *nv_connector =
+		container_of(obj, typeof(*nv_connector), aux);
+	struct nouveau_drm *drm = nouveau_drm(nv_connector->base.dev);
+	int ret;
+
+	if (nouveau_is_rpm_worker(drm))
+		return 0;
+
+	ret = pm_runtime_get_sync(drm->dev->dev);
+	if (ret < 0 && ret != -EAGAIN)
+		return ret;
+
+	return 0;
+}
+
+static void
+nouveau_connector_aux_post_xfer(struct drm_dp_aux *obj)
+{
+	struct nouveau_connector *nv_connector =
+		container_of(obj, typeof(*nv_connector), aux);
+	struct nouveau_drm *drm = nouveau_drm(nv_connector->base.dev);
+
+	if (nouveau_is_rpm_worker(drm))
+		return;
+
+	pm_runtime_mark_last_busy(drm->dev->dev);
+	pm_runtime_put_autosuspend(drm->dev->dev);
+}
+
 static ssize_t
 nouveau_connector_aux_xfer(struct drm_dp_aux *obj, struct drm_dp_aux_msg *msg)
 {

@@ -1341,6 +1373,10 @@ nouveau_connector_create(struct drm_device *dev, int index)
 	case DRM_MODE_CONNECTOR_DisplayPort:
 	case DRM_MODE_CONNECTOR_eDP:
 		nv_connector->aux.dev = dev->dev;
+		nv_connector->aux.pre_transfer =
+			nouveau_connector_aux_pre_xfer;
+		nv_connector->aux.post_transfer =
+			nouveau_connector_aux_post_xfer;
 		nv_connector->aux.transfer = nouveau_connector_aux_xfer;
 		ret = drm_dp_aux_register(&nv_connector->aux);
 		if (ret) {

drivers/gpu/drm/nouveau/nouveau_drm.c +11 -1

index 2b2baf6e0e0d..4323e9e61c2e 100644
@@ -859,6 +859,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	int ret;
 
 	if (!nouveau_pmops_runtime()) {

@@ -866,6 +867,8 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 		return -EBUSY;
 	}
 
+	drm->rpm_task = current;
+
 	nouveau_switcheroo_optimus_dsm();
 	ret = nouveau_do_suspend(drm_dev, true);
 	pci_save_state(pdev);

@@ -873,6 +876,8 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 	pci_ignore_hotplug(pdev);
 	pci_set_power_state(pdev, PCI_D3cold);
 	drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
+
+	drm->rpm_task = NULL;
 	return ret;
 }

@@ -881,6 +886,7 @@ nouveau_pmops_runtime_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	struct nvif_device *device = &nouveau_drm(drm_dev)->client.device;
 	int ret;

@@ -889,11 +895,13 @@ nouveau_pmops_runtime_resume(struct device *dev)
 		return -EBUSY;
 	}
 
+	drm->rpm_task = current;
+
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
 	ret = pci_enable_device(pdev);
 	if (ret)
-		return ret;
+		goto out;
 	pci_set_master(pdev);
 
 	ret = nouveau_do_resume(drm_dev, true);

@@ -905,6 +913,8 @@ nouveau_pmops_runtime_resume(struct device *dev)
 	/* Monitors may have been connected / disconnected during suspend */
 	schedule_work(&nouveau_drm(drm_dev)->hpd_work);
 
+out:
+	drm->rpm_task = NULL;
 	return ret;
 }

drivers/gpu/drm/nouveau/nouveau_drv.h +8 -0

index 0b2191fa96f7..e8d4203ddfb4 100644
@@ -212,6 +212,8 @@ struct nouveau_drm {
 	bool have_disp_power_ref;
 
 	struct dev_pm_domain vga_pm_domain;
+
+	struct task_struct *rpm_task;
 };
 
 static inline struct nouveau_drm *

@@ -231,6 +233,12 @@ int nouveau_pmops_suspend(struct device *);
 int nouveau_pmops_resume(struct device *);
 bool nouveau_pmops_runtime(void);
 
+static inline bool
+nouveau_is_rpm_worker(struct nouveau_drm *drm)
+{
+	return drm->rpm_task == current;
+}
+
 #include <nvkm/core/tegra.h>
 
 struct drm_device *

Signature · 1 line

2.19.1

Karol Herbst

2018-11-24 15:47 UTC #3 lore raw

Re: [Nouveau] [PATCH 2/2] drm/nouveau: Grab an rpm reference before/after DP AUX transactions

why the nouveau_is_rpm_worker stuff?

Quoted 167 lines · Lyude Paul

Signed-off-by Lyude Paul <[email protected]>

---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 36 +++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c       | 12 ++++++-
 drivers/gpu/drm/nouveau/nouveau_drv.h       |  8 +++++
 3 files changed, 55 insertions(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_connector.c +36 -0

index fd80661dff92..d2e9752f2f91 100644
@@ -1171,6 +1171,38 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
        return NVIF_NOTIFY_KEEP;
 }
 
+static int
+nouveau_connector_aux_pre_xfer(struct drm_dp_aux *obj)
+{
+       struct nouveau_connector *nv_connector =
+               container_of(obj, typeof(*nv_connector), aux);
+       struct nouveau_drm *drm = nouveau_drm(nv_connector->base.dev);
+       int ret;
+
+       if (nouveau_is_rpm_worker(drm))
+               return 0;
+
+       ret = pm_runtime_get_sync(drm->dev->dev);
+       if (ret < 0 && ret != -EAGAIN)
+               return ret;
+
+       return 0;
+}
+
+static void
+nouveau_connector_aux_post_xfer(struct drm_dp_aux *obj)
+{
+       struct nouveau_connector *nv_connector =
+               container_of(obj, typeof(*nv_connector), aux);
+       struct nouveau_drm *drm = nouveau_drm(nv_connector->base.dev);
+
+       if (nouveau_is_rpm_worker(drm))
+               return;
+
+       pm_runtime_mark_last_busy(drm->dev->dev);
+       pm_runtime_put_autosuspend(drm->dev->dev);
+}
+
 static ssize_t
 nouveau_connector_aux_xfer(struct drm_dp_aux *obj, struct drm_dp_aux_msg *msg)
 {

@@ -1341,6 +1373,10 @@ nouveau_connector_create(struct drm_device *dev, int index)
        case DRM_MODE_CONNECTOR_DisplayPort:
        case DRM_MODE_CONNECTOR_eDP:
                nv_connector->aux.dev = dev->dev;
+               nv_connector->aux.pre_transfer =
+                       nouveau_connector_aux_pre_xfer;
+               nv_connector->aux.post_transfer =
+                       nouveau_connector_aux_post_xfer;
                nv_connector->aux.transfer = nouveau_connector_aux_xfer;
                ret = drm_dp_aux_register(&nv_connector->aux);
                if (ret) {

drivers/gpu/drm/nouveau/nouveau_drm.c +11 -1

index 2b2baf6e0e0d..4323e9e61c2e 100644
@@ -859,6 +859,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 {
        struct pci_dev *pdev = to_pci_dev(dev);
        struct drm_device *drm_dev = pci_get_drvdata(pdev);
+       struct nouveau_drm *drm = nouveau_drm(drm_dev);
        int ret;
 
        if (!nouveau_pmops_runtime()) {

@@ -866,6 +867,8 @@ nouveau_pmops_runtime_suspend(struct device *dev)
                return -EBUSY;
        }
 
+       drm->rpm_task = current;
+
        nouveau_switcheroo_optimus_dsm();
        ret = nouveau_do_suspend(drm_dev, true);
        pci_save_state(pdev);

@@ -873,6 +876,8 @@ nouveau_pmops_runtime_suspend(struct device *dev)
        pci_ignore_hotplug(pdev);
        pci_set_power_state(pdev, PCI_D3cold);
        drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
+
+       drm->rpm_task = NULL;
        return ret;
 }

@@ -881,6 +886,7 @@ nouveau_pmops_runtime_resume(struct device *dev)
 {
        struct pci_dev *pdev = to_pci_dev(dev);
        struct drm_device *drm_dev = pci_get_drvdata(pdev);
+       struct nouveau_drm *drm = nouveau_drm(drm_dev);
        struct nvif_device *device = &nouveau_drm(drm_dev)->client.device;
        int ret;

@@ -889,11 +895,13 @@ nouveau_pmops_runtime_resume(struct device *dev)
                return -EBUSY;
        }
 
+       drm->rpm_task = current;
+
        pci_set_power_state(pdev, PCI_D0);
        pci_restore_state(pdev);
        ret = pci_enable_device(pdev);
        if (ret)
-               return ret;
+               goto out;
        pci_set_master(pdev);
 
        ret = nouveau_do_resume(drm_dev, true);

@@ -905,6 +913,8 @@ nouveau_pmops_runtime_resume(struct device *dev)
        /* Monitors may have been connected / disconnected during suspend */
        schedule_work(&nouveau_drm(drm_dev)->hpd_work);
 
+out:
+       drm->rpm_task = NULL;
        return ret;
 }

drivers/gpu/drm/nouveau/nouveau_drv.h +8 -1

index 0b2191fa96f7..e8d4203ddfb4 100644
@@ -212,6 +212,8 @@ struct nouveau_drm {
        bool have_disp_power_ref;
 
        struct dev_pm_domain vga_pm_domain;
+
+       struct task_struct *rpm_task;
 };
 
 static inline struct nouveau_drm *

@@ -231,6 +233,12 @@ int nouveau_pmops_suspend(struct device *);
 int nouveau_pmops_resume(struct device *);
 bool nouveau_pmops_runtime(void);
 
+static inline bool
+nouveau_is_rpm_worker(struct nouveau_drm *drm)
+{
+       return drm->rpm_task == current;
+}
+
 #include <nvkm/core/tegra.h>
 
 struct drm_device *
--

2.19.1

Quoted 4 lines

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Lyude Paul

author maintainer

2018-11-26 20:59 UTC #4 lore raw

Re: [Nouveau] [PATCH 2/2] drm/nouveau: Grab an rpm reference before/after DP AUX transactions

Quoted 1 line · Karol Herbst

why the nouveau_is_rpm_worker stuff?

To prevent us from trying to grab a runtime PM reference in the runtime suspend/resume codepath without preventing us from using the aux channel in those code paths, since drm_dp_mst_topology_mgr_suspend() and drm_dp_mst_topology_mgr_resume() both need to be able to use the aux channel. Without that those functions will try to grab a runtime pm ref while runtime resume then deadlock.

Quoted 178 lines · Lyude Paul

Quoted 177 lines

Signed-off-by Lyude Paul <[email protected]>

---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 36 +++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c       | 12 ++++++-
 drivers/gpu/drm/nouveau/nouveau_drv.h       |  8 +++++
 3 files changed, 55 insertions(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_connector.c +32 -0

index fd80661dff92..d2e9752f2f91 100644
@@ -1171,6 +1171,38 @@ nouveau_connector_hotplug(struct nvif_notify
 *notify)
        return NVIF_NOTIFY_KEEP;
 }
 
+static int
+nouveau_connector_aux_pre_xfer(struct drm_dp_aux *obj)
+{
+       struct nouveau_connector *nv_connector =
+               container_of(obj, typeof(*nv_connector), aux);
+       struct nouveau_drm *drm = nouveau_drm(nv_connector->base.dev);
+       int ret;
+
+       if (nouveau_is_rpm_worker(drm))
+               return 0;
+
+       ret = pm_runtime_get_sync(drm->dev->dev);
+       if (ret < 0 && ret != -EAGAIN)
+               return ret;
+
+       return 0;
+}
+
+static void
+nouveau_connector_aux_post_xfer(struct drm_dp_aux *obj)
+{
+       struct nouveau_connector *nv_connector =
+               container_of(obj, typeof(*nv_connector), aux);
+       struct nouveau_drm *drm = nouveau_drm(nv_connector->base.dev);
+
+       if (nouveau_is_rpm_worker(drm))
+               return;
+
+       pm_runtime_mark_last_busy(drm->dev->dev);
+       pm_runtime_put_autosuspend(drm->dev->dev);
+}
+
 static ssize_t
 nouveau_connector_aux_xfer(struct drm_dp_aux *obj, struct drm_dp_aux_msg

*msg)
{

drivers/gpu/drm/nouveau/nouveau_connector.c +4 -0

@@ -1341,6 +1373,10 @@ nouveau_connector_create(struct drm_device *dev,
 int index)
        case DRM_MODE_CONNECTOR_DisplayPort:
        case DRM_MODE_CONNECTOR_eDP:
                nv_connector->aux.dev = dev->dev;
+               nv_connector->aux.pre_transfer =
+                       nouveau_connector_aux_pre_xfer;
+               nv_connector->aux.post_transfer =
+                       nouveau_connector_aux_post_xfer;
                nv_connector->aux.transfer = nouveau_connector_aux_xfer;
                ret = drm_dp_aux_register(&nv_connector->aux);

                if (ret) {

drivers/gpu/drm/nouveau/nouveau_drm.c +11 -1

index 2b2baf6e0e0d..4323e9e61c2e 100644
@@ -859,6 +859,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 {
        struct pci_dev *pdev = to_pci_dev(dev);
        struct drm_device *drm_dev = pci_get_drvdata(pdev);
+       struct nouveau_drm *drm = nouveau_drm(drm_dev);
        int ret;
 
        if (!nouveau_pmops_runtime()) {

@@ -866,6 +867,8 @@ nouveau_pmops_runtime_suspend(struct device *dev)
                return -EBUSY;
        }
 
+       drm->rpm_task = current;
+
        nouveau_switcheroo_optimus_dsm();
        ret = nouveau_do_suspend(drm_dev, true);
        pci_save_state(pdev);

@@ -873,6 +876,8 @@ nouveau_pmops_runtime_suspend(struct device *dev)
        pci_ignore_hotplug(pdev);
        pci_set_power_state(pdev, PCI_D3cold);
        drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
+
+       drm->rpm_task = NULL;
        return ret;
 }

@@ -881,6 +886,7 @@ nouveau_pmops_runtime_resume(struct device *dev)
 {
        struct pci_dev *pdev = to_pci_dev(dev);
        struct drm_device *drm_dev = pci_get_drvdata(pdev);
+       struct nouveau_drm *drm = nouveau_drm(drm_dev);
        struct nvif_device *device = &nouveau_drm(drm_dev)->client.device;
        int ret;

@@ -889,11 +895,13 @@ nouveau_pmops_runtime_resume(struct device *dev)
                return -EBUSY;
        }
 
+       drm->rpm_task = current;
+
        pci_set_power_state(pdev, PCI_D0);
        pci_restore_state(pdev);
        ret = pci_enable_device(pdev);
        if (ret)
-               return ret;
+               goto out;
        pci_set_master(pdev);
 
        ret = nouveau_do_resume(drm_dev, true);

@@ -905,6 +913,8 @@ nouveau_pmops_runtime_resume(struct device *dev)
        /* Monitors may have been connected / disconnected during suspend
 */
        schedule_work(&nouveau_drm(drm_dev)->hpd_work);
 
+out:
+       drm->rpm_task = NULL;
        return ret;
 }

drivers/gpu/drm/nouveau/nouveau_drv.h +8 -1

index 0b2191fa96f7..e8d4203ddfb4 100644
@@ -212,6 +212,8 @@ struct nouveau_drm {
        bool have_disp_power_ref;
 
        struct dev_pm_domain vga_pm_domain;
+
+       struct task_struct *rpm_task;
 };
 
 static inline struct nouveau_drm *

@@ -231,6 +233,12 @@ int nouveau_pmops_suspend(struct device *);
 int nouveau_pmops_resume(struct device *);
 bool nouveau_pmops_runtime(void);
 
+static inline bool
+nouveau_is_rpm_worker(struct nouveau_drm *drm)
+{
+       return drm->rpm_task == current;
+}
+
 #include <nvkm/core/tegra.h>
 
 struct drm_device *
--

2.19.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Signature · 7 lines

Cheers,
	Lyude Paul

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau