Re: [Freedreno] [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

2023-03-01 Thread Sankeerth Billakanti (QUIC) 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>[..]
>> +static int dp_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->dp_display.is_edp) {
>> +dp_display_host_phy_init(dp);
>> +} else {
>> +dp_catalog_hpd_config_intr(dp->catalog,
>> +DP_DP_HPD_PLUG_INT_MASK |
>> +DP_DP_HPD_UNPLUG_INT_MASK,
>> +true);
>
>I believe this is backwards.
>
>Only in the event that there's no "downstream" HPD handler should we use
>the internal HPD. This is signalled by the DRM framework by a call to
>dp_bridge_hpd_enable(). So we should use that to enable/disable the
>internal HPD handler.
>
>When this happens, we have a reason for keeping power on; i.e. call
>pm_runtime_get(). Once we have power/clocking, we'd call
>dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().
>
>
>In the case that the internal HPD handling is not use,
>dp_bridge_hpd_enable() will not be called, instead once the downstream hpd
>handler switches state dp_bridge_hpd_notify() will be invoked.
>
>In this case, we need the DP controller to be powered/clocked between
>connector_status_connected and connector_status_disconnected.
>
>
>I believe this should allow the DP controller(s) to stay powered down in the
>case where we have external HPD handling (e.g. USB Type-C or gpio-based
>dp-connector).
>
>Regards,
>Bjorn

I agree with the approach. I am moving my dev to msm-next. Will make the 
changes according to the HPD handling and repost

Thank you,
Sankeerth

Re: [Freedreno] [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

2023-02-27 Thread Bjorn Andersson 
On Thu, Feb 23, 2023 at 07:26:35PM +0530, Sankeerth Billakanti wrote:
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
[..]
> +static int dp_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->dp_display.is_edp) {
> + dp_display_host_phy_init(dp);
> + } else {
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);

I believe this is backwards.

Only in the event that there's no "downstream" HPD handler should we use
the internal HPD. This is signalled by the DRM framework by a call to
dp_bridge_hpd_enable(). So we should use that to enable/disable the
internal HPD handler.

When this happens, we have a reason for keeping power on; i.e. call
pm_runtime_get(). Once we have power/clocking, we'd call
dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().


In the case that the internal HPD handling is not use,
dp_bridge_hpd_enable() will not be called, instead once the downstream
hpd handler switches state dp_bridge_hpd_notify() will be invoked.

In this case, we need the DP controller to be powered/clocked between
connector_status_connected and connector_status_disconnected.


I believe this should allow the DP controller(s) to stay powered down in
the case where we have external HPD handling (e.g. USB Type-C or
gpio-based dp-connector).

Regards,
Bjorn

[Freedreno] [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

2023-02-23 Thread Sankeerth Billakanti 
The current DP driver directly enables or disables the necessary control
resources based on code flow. This could disable a required resource that
is needed in a different usecase. It can also lead to excessive voting of
a resource and may increase power consumption.

The pm_runtime framework can solve this problem in DP driver by monitoring
the resource enable disable calls. This change will enable support for the
pm_runtime resume and suspend operations for DP driver.

Signed-off-by: Sankeerth Billakanti 
---
 drivers/gpu/drm/msm/dp/dp_aux.c |   6 ++
 drivers/gpu/drm/msm/dp/dp_display.c | 121 
 drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
 3 files changed, 76 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 5da95dfdeede..45026827bf7a 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -338,6 +338,7 @@ static ssize_t dp_aux_transfer_init(struct drm_dp_aux 
*dp_aux,
return -EINVAL;
}
 
+   pm_runtime_get_sync(dp_aux->dev);
mutex_lock(&aux->mutex);
if (!aux->initted) {
ret = -EIO;
@@ -418,6 +419,8 @@ static ssize_t dp_aux_transfer_init(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;
 }
@@ -454,6 +457,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
return -EINVAL;
}
 
+   pm_runtime_get_sync(dp_aux->dev);
mutex_lock(&aux->mutex);
if (!aux->initted) {
ret = -EIO;
@@ -527,6 +531,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 a5dcef040b74..a23e79e43100 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -307,8 +307,10 @@ static void dp_display_unbind(struct device *dev, struct 
device *master,
struct msm_drm_private *priv = dev_get_drvdata(master);
 
/* disable all HPD interrupts */
-   if (dp->core_initialized)
+   if (dp->core_initialized) {
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
false);
+   pm_runtime_put_sync(dev);
+   }
 
kthread_stop(dp->ev_tsk);
 
@@ -1083,26 +1085,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, 
struct msm_dp *dp)
mutex_unlock(&dp_display->event_mutex);
 }
 
-static void dp_display_config_hpd(struct dp_display_private *dp)
-{
-
-   dp_display_host_init(dp);
-   dp_catalog_ctrl_hpd_config(dp->catalog);
-
-   /* Enable plug and unplug interrupts only if requested */
-   if (dp->dp_display.internal_hpd)
-   dp_catalog_hpd_config_intr(dp->catalog,
-   DP_DP_HPD_PLUG_INT_MASK |
-   DP_DP_HPD_UNPLUG_INT_MASK,
-   true);
-
-   /* Enable interrupt first time
-* we are leaving dp clocks on during disconnect
-* and never disable interrupt
-*/
-   enable_irq(dp->irq);
-}
-
 static int hpd_event_thread(void *data)
 {
struct dp_display_private *dp_priv;
@@ -1163,9 +1145,6 @@ static int hpd_event_thread(void *data)
spin_unlock_irqrestore(&dp_priv->event_lock, flag);
 
switch (todo->event_id) {
-   case EV_HPD_INIT_SETUP:
-   dp_display_config_hpd(dp_priv);
-   break;
case EV_HPD_PLUG_INT:
dp_hpd_plug_handle(dp_priv, todo->data);
break;
@@ -1337,16 +1316,12 @@ static int dp_display_probe(struct platform_device 
*pdev)
 
platform_set_drvdata(pdev, &dp->dp_display);
 
-   if (dp->dp_display.is_edp) {
-   dp_display_host_init(dp);
-   dp_display_host_phy_init(dp);
-   dp_catalog_ctrl_hpd_config(dp->catalog);
+   pm_runtime_enable(&pdev->dev);
+   pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+   pm_runtime_use_autosuspend(&pdev->dev);
 
+   if (dp->dp_display.is_edp) {
rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
-
-   dp_display_host_phy_exit(dp);
-   dp_display_host_deinit(dp);
-
if (rc) {
DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
goto error;
@@ -1367,6 +1342,8 @@ static int dp_display_remove(struct platform_device *pdev)
 {
struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
 
+   pm_runtime_dont_use_autosuspend(&pdev->dev);
+   pm_runtime_disable(&pdev->dev);