Re: [PATCH v7 2/2] drm/msm/dp: do not initialize phy until plugin interrupt received

2021-12-21 Thread Kuogee Hsieh 



On 12/14/2021 5:50 PM, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-12-09 13:35:07)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 0766752..cfbc5e4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -83,6 +83,7 @@ struct dp_display_private {

 /* state variables */
 bool core_initialized;
+   bool phy_initialized;
 bool hpd_irq_on;
 bool audio_supported;

@@ -371,21 +372,46 @@ static int dp_display_process_hpd_high(struct 
dp_display_private *dp)
 return rc;
  }

-static void dp_display_host_init(struct dp_display_private *dp, int reset)
+static void dp_display_host_phy_init(struct dp_display_private *dp)
  {
-   bool flip = false;
+   DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
+   dp->core_initialized, dp->phy_initialized);

+   if (!dp->phy_initialized) {

Is this check only here because we don't know when this function is
getting called? I see in the DP case we get here from
dp_display_usbpd_configure_cb() but in the eDP case we get here from
dp_display_host_init() and presumably again from
dp_display_usbpd_configure_cb() called by dp_hpd_plug_handle().

If at all possible, I'd prefer to not have another tracking variable and
call dp_display_host_phy_init() from the same place regardless of DP or
eDP. Doing that would make it symmetric, per the commit text.


Agree, but  dp->phy_initialized variable used to track some other 
conditions.


For example, phy required to be re inited if dpcd read failed due to 
cable unplugged.


other dpcd read failed will not required to re inited.

Therefore I think dp->phy_initialized variable is still required.


+   dp_ctrl_phy_init(dp->ctrl);
+   dp->phy_initialized = true;
+   }
+}
+
+static void dp_display_host_phy_exit(struct dp_display_private *dp)
+{
+   DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
+   dp->core_initialized, dp->phy_initialized);
+
+   if (dp->phy_initialized) {
+   dp_ctrl_phy_exit(dp->ctrl);
+   dp->phy_initialized = false;
+   }
+}
+
+static void dp_display_host_init(struct dp_display_private *dp)
+{
 DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized);
 if (dp->core_initialized) {
 DRM_DEBUG_DP("DP core already initialized\n");
 return;
 }

-   if (dp->usbpd->orientation == ORIENTATION_CC2)
-   flip = true;
+   dp_power_init(dp->power, false);
+   dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+
+   /*
+* eDP is the embedded primary display and has its own phy
+* initialize phy immediately

Can we get some more details here? Why is it better to initialize the
phy here vs. when HPD goes high on the panel? The comment says what the
code is doing but it isn't telling us why that's OK.


Will do.




+*/
+   if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP)
+   dp_display_host_phy_init(dp);

-   dp_power_init(dp->power, flip);
-   dp_ctrl_host_init(dp->ctrl, flip, reset);
 dp_aux_init(dp->aux);
 dp->core_initialized = true;
  }

Re: [PATCH v7 2/2] drm/msm/dp: do not initialize phy until plugin interrupt received

2021-12-14 Thread Stephen Boyd 
Quoting Kuogee Hsieh (2021-12-09 13:35:07)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 0766752..cfbc5e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -83,6 +83,7 @@ struct dp_display_private {
>
> /* state variables */
> bool core_initialized;
> +   bool phy_initialized;
> bool hpd_irq_on;
> bool audio_supported;
>
> @@ -371,21 +372,46 @@ static int dp_display_process_hpd_high(struct 
> dp_display_private *dp)
> return rc;
>  }
>
> -static void dp_display_host_init(struct dp_display_private *dp, int reset)
> +static void dp_display_host_phy_init(struct dp_display_private *dp)
>  {
> -   bool flip = false;
> +   DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
> +   dp->core_initialized, dp->phy_initialized);
>
> +   if (!dp->phy_initialized) {

Is this check only here because we don't know when this function is
getting called? I see in the DP case we get here from
dp_display_usbpd_configure_cb() but in the eDP case we get here from
dp_display_host_init() and presumably again from
dp_display_usbpd_configure_cb() called by dp_hpd_plug_handle().

If at all possible, I'd prefer to not have another tracking variable and
call dp_display_host_phy_init() from the same place regardless of DP or
eDP. Doing that would make it symmetric, per the commit text.

> +   dp_ctrl_phy_init(dp->ctrl);
> +   dp->phy_initialized = true;
> +   }
> +}
> +
> +static void dp_display_host_phy_exit(struct dp_display_private *dp)
> +{
> +   DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
> +   dp->core_initialized, dp->phy_initialized);
> +
> +   if (dp->phy_initialized) {
> +   dp_ctrl_phy_exit(dp->ctrl);
> +   dp->phy_initialized = false;
> +   }
> +}
> +
> +static void dp_display_host_init(struct dp_display_private *dp)
> +{
> DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized);
> if (dp->core_initialized) {
> DRM_DEBUG_DP("DP core already initialized\n");
> return;
> }
>
> -   if (dp->usbpd->orientation == ORIENTATION_CC2)
> -   flip = true;
> +   dp_power_init(dp->power, false);
> +   dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> +
> +   /*
> +* eDP is the embedded primary display and has its own phy
> +* initialize phy immediately

Can we get some more details here? Why is it better to initialize the
phy here vs. when HPD goes high on the panel? The comment says what the
code is doing but it isn't telling us why that's OK.

> +*/
> +   if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP)
> +   dp_display_host_phy_init(dp);
>
> -   dp_power_init(dp->power, flip);
> -   dp_ctrl_host_init(dp->ctrl, flip, reset);
> dp_aux_init(dp->aux);
> dp->core_initialized = true;
>  }