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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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;
  }



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux