Re: [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

Mon, 10 Jul 2023 09:18:51 -0700 


On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.

Signed-off-by: Kuogee Hsieh <quic_khs...@quicinc.com>
---
  drivers/gpu/drm/msm/dp/dp_aux.c     |  3 ++

  drivers/gpu/drm/msm/dp/dp_display.c | 75 
+++++++++++++++++++++++++++++--------

  2 files changed, 63 insertions(+), 15 deletions(-)


diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 8e3b677..c592064 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c

@@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

          return -EINVAL;
      }
  +    pm_runtime_get_sync(dp_aux->dev);


Let me quote the function's documentation:

Consider using pm_runtime_resume_and_get() instead of it, especially 
if its return value is checked by the caller, as this is likely to 
result in cleaner code.


pm_runtime_resume_and_get() will call pm_runtime_resume()  every time.


Since aux_transfer is called very frequently, is it just simple to call 
pm_runtiem_get_sync() which will call pm_runtime_reusme() if power 
counter is 0 before increased it.


otherwise it just increase power counter?




So two notes concerning the whole patch:
- error checking is missing
- please use pm_runtime_resume_and_get() instead.


      mutex_lock(&aux->mutex);
      if (!aux->initted) {
          ret = -EIO;

@@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

    exit:
      mutex_unlock(&aux->mutex);
+    pm_runtime_mark_last_busy(dp_aux->dev);
+    pm_runtime_put_autosuspend(dp_aux->dev);
        return ret;
  }

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 76f1395..2c5706a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c

@@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
struct device *master,

          goto end;
      }
  +    pm_runtime_enable(dev);


devm_pm_runtime_enable() removes need for a cleanup.


+    pm_runtime_set_autosuspend_delay(dev, 1000);
+    pm_runtime_use_autosuspend(dev);


Why do you want to use autosuspend here?


+
      return 0;
  end:
      return rc;

@@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, 
struct device *master,

      struct dp_display_private *dp = dev_get_dp_display_private(dev);
      struct msm_drm_private *priv = dev_get_drvdata(master);
  -    /* disable all HPD interrupts */
-    if (dp->core_initialized)

-        dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
false);

+    pm_runtime_dont_use_autosuspend(dev);
+    pm_runtime_disable(dev);
        kthread_stop(dp->ev_tsk);

  @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
dp_display_private *dp)

          dp->dp_display.connector_type, dp->core_initialized,
          dp->phy_initialized);
  -    dp_power_init(dp->power);
-    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
-    dp_aux_init(dp->aux);
-    dp->core_initialized = true;
+    if (!dp->core_initialized) {
+        dp_power_init(dp->power);
+        dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+        dp_aux_init(dp->aux);
+        dp->core_initialized = true;
+    }


Is this relevant to PM runtime? I don't think so.


  }
    static void dp_display_host_deinit(struct dp_display_private *dp)

@@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
dp_display_private *dp)

          dp->dp_display.connector_type, dp->core_initialized,
          dp->phy_initialized);
  -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
-    dp_aux_deinit(dp->aux);
-    dp_power_deinit(dp->power);
-    dp->core_initialized = false;
+    if (dp->core_initialized) {
+        dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+        dp_aux_deinit(dp->aux);
+        dp_power_deinit(dp->power);
+        dp->core_initialized = false;
+    }
  }
    static int dp_display_usbpd_configure_cb(struct device *dev)

@@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
platform_device *pdev)

      dp_display_deinit_sub_modules(dp);
        platform_set_drvdata(pdev, NULL);
+    pm_runtime_put_sync_suspend(&pdev->dev);
+
+    return 0;
+}
+
+static int dp_pm_runtime_suspend(struct device *dev)
+{
+    struct platform_device *pdev = to_platform_device(dev);
+    struct msm_dp *dp_display = platform_get_drvdata(pdev);
+    struct dp_display_private *dp;
+

+    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+
+    dp_display_host_phy_exit(dp);
+    dp_catalog_ctrl_hpd_enable(dp->catalog);


What? NO!


+    dp_display_host_deinit(dp);
+
+    return 0;
+}
+
+static int dp_pm_runtime_resume(struct device *dev)
+{
+    struct platform_device *pdev = to_platform_device(dev);
+    struct msm_dp *dp_display = platform_get_drvdata(pdev);
+    struct dp_display_private *dp;
+

+    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+
+    dp_display_host_init(dp);
+    if (dp_display->is_edp) {
+        dp_catalog_ctrl_hpd_enable(dp->catalog);
+        dp_display_host_phy_init(dp);
+    }
        return 0;
  }
@@ -1409,6 +1449,7 @@ static int dp_pm_suspend(struct device *dev)
  }
    static const struct dev_pm_ops dp_pm_ops = {

+    SET_RUNTIME_PM_OPS(dp_pm_runtime_suspend, dp_pm_runtime_resume, 
NULL)

      .suspend = dp_pm_suspend,
      .resume =  dp_pm_resume,



With the runtime PM in place, can we change suspend/resume to use 
pm_runtime_force_suspend() and pm_runtime_force_resume() ?



Let em try if i can move checking device connection status out of 
dp_pm_resume(). it handles external dp panel plugin/unplug during 
suspend cases.






  };

@@ -1493,10 +1534,6 @@ static int dp_display_get_next_bridge(struct 
msm_dp *dp)

      aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
        if (aux_bus && dp->is_edp) {
-        dp_display_host_init(dp_priv);
-        dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
-        dp_display_host_phy_init(dp_priv);



Are you going to populate the AUX bus (which can cause AUX bus access) 
without waking up the device?



devm_of_dp_aux_populate_ep_devices() ==>  will call 
pm_runtiemget_sync() internally which will call pm_runtime_resume() to 
wake dp driver

-
          /*
           * The code below assumes that the panel will finish probing
           * by the time devm_of_dp_aux_populate_ep_devices() returns.

@@ -1604,6 +1641,7 @@ void dp_bridge_atomic_enable(struct drm_bridge 
*drm_bridge,

          dp_hpd_plug_handle(dp_display, 0);


Nearly the same question. Resume device before accessing registers.


mutex_lock(&dp_display->event_mutex);
+    pm_runtime_get_sync(&dp_display->pdev->dev);
        state = dp_display->hpd_state;
      if (state != ST_DISPLAY_OFF && state != ST_MAINLINK_READY) {

@@ -1684,6 +1722,8 @@ void dp_bridge_atomic_post_disable(struct 
drm_bridge *drm_bridge,

      }
        drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
+
+    pm_runtime_put_sync(&dp_display->pdev->dev);
      mutex_unlock(&dp_display->event_mutex);
  }

  @@ -1723,6 +1763,8 @@ void dp_bridge_hpd_enable(struct drm_bridge 
*bridge)
      struct dp_display_private *dp = container_of(dp_display, struct 
dp_display_private, dp_display);

        mutex_lock(&dp->event_mutex);
+    pm_runtime_get_sync(&dp->pdev->dev);
+
      dp_catalog_ctrl_hpd_enable(dp->catalog);
        /* enable HDP interrupts */

@@ -1744,6 +1786,9 @@ void dp_bridge_hpd_disable(struct drm_bridge 
*bridge)

      dp_catalog_ctrl_hpd_disable(dp->catalog);
        dp_display->internal_hpd = false;
+
+    pm_runtime_mark_last_busy(&dp->pdev->dev);
+    pm_runtime_put_autosuspend(&dp->pdev->dev);
      mutex_unlock(&dp->event_mutex);
  }

























					Reply via email to