Re: [Freedreno] [PATCH v3] drm/msm/mdp5: stop overriding drvdata

Thu, 27 Oct 2022 12:59:18 -0700 



On 10/27/2022 12:56 PM, Dmitry Baryshkov wrote:

On 27/10/2022 21:30, Abhinav Kumar wrote:



On 10/24/2022 8:26 AM, Dmitry Baryshkov wrote:

The rest of the code expects that master's device drvdata is the
struct msm_drm_private instance. Do not override the mdp5's drvdata.

Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
---
Abhinav, Rob, please pick this for -fixes.

This is an updated version of [1]. Fixed the read_mdp_hw_revision()
function. PM runtime isn't available at the moment, as priv->kms is not
set.

[1] https://patchwork.freedesktop.org/patch/490326/?series=105392&rev=1

Changes since v2:
- Removed the clause checking whether mdp5_enable() has failed (it can
   not fail, noted by Abhinav)

Changes since v1:
- Expanded the patch to also handle the read_mdp_hw_revision() and also
   to move pm enablement to the place where the pm_runtime can actually
   be used.

---
  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 32 +++++++++++++-----------
  1 file changed, 17 insertions(+), 15 deletions(-)


diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c

index b0d21838a134..b46f983f2b46 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c

@@ -203,7 +203,7 @@ static int mdp5_set_split_display(struct msm_kms 
*kms,

                                slave_encoder);
  }
-static void mdp5_destroy(struct platform_device *pdev);
+static void mdp5_destroy(struct mdp5_kms *mdp5_kms);
  static void mdp5_kms_destroy(struct msm_kms *kms)
  {
@@ -223,7 +223,7 @@ static void mdp5_kms_destroy(struct msm_kms *kms)
      }
      mdp_kms_destroy(&mdp5_kms->base);
-    mdp5_destroy(mdp5_kms->pdev);
+    mdp5_destroy(mdp5_kms);
  }
  #ifdef CONFIG_DEBUG_FS

@@ -519,9 +519,10 @@ static void read_mdp_hw_revision(struct mdp5_kms 
*mdp5_kms,

      struct device *dev = &mdp5_kms->pdev->dev;
      u32 version;
-    pm_runtime_get_sync(dev);
+    /* Manually enable the MDP5, as pm runtime isn't usable yet. */
+    mdp5_enable(mdp5_kms);
      version = mdp5_read(mdp5_kms, REG_MDP5_HW_VERSION);
-    pm_runtime_put_sync(dev);
+    mdp5_disable(mdp5_kms);



Please correct me if wrong here, if we bypass the pm to enable the 
clocks explicitly here, are we still guaranteed about GDSC to be ON?



The gdsc is tied to the mdss device, not to mdp5. So the gdsc will be 
enabled, because for mdp5 to probe the mdss device also should be powered.




Ok, thanks for clarifying.

LGTM,

Reviewed-by: Abhinav Kumar <quic_abhin...@quicinc.com>




      *major = FIELD(version, MDP5_HW_VERSION_MAJOR);
      *minor = FIELD(version, MDP5_HW_VERSION_MINOR);
@@ -559,6 +560,8 @@ static int mdp5_kms_init(struct drm_device *dev)
      int irq, i, ret;
      ret = mdp5_init(to_platform_device(dev->dev), dev);
+    if (ret)
+        return ret;
      /* priv->kms would have been populated by the MDP5 driver */
      kms = priv->kms;
@@ -632,9 +635,8 @@ static int mdp5_kms_init(struct drm_device *dev)
      return ret;
  }
-static void mdp5_destroy(struct platform_device *pdev)
+static void mdp5_destroy(struct mdp5_kms *mdp5_kms)
  {
-    struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
      int i;
      if (mdp5_kms->ctlm)

@@ -648,7 +650,7 @@ static void mdp5_destroy(struct platform_device 
*pdev)

          kfree(mdp5_kms->intfs[i]);
      if (mdp5_kms->rpm_enabled)
-        pm_runtime_disable(&pdev->dev);
+        pm_runtime_disable(&mdp5_kms->pdev->dev);
      drm_atomic_private_obj_fini(&mdp5_kms->glob_state);
      drm_modeset_lock_fini(&mdp5_kms->glob_state_lock);

@@ -797,8 +799,6 @@ static int mdp5_init(struct platform_device 
*pdev, struct drm_device *dev)

          goto fail;
      }
-    platform_set_drvdata(pdev, mdp5_kms);
-
      spin_lock_init(&mdp5_kms->resource_lock);
      mdp5_kms->dev = dev;

@@ -839,9 +839,6 @@ static int mdp5_init(struct platform_device 
*pdev, struct drm_device *dev)

       */
      clk_set_rate(mdp5_kms->core_clk, 200000000);
-    pm_runtime_enable(&pdev->dev);
-    mdp5_kms->rpm_enabled = true;
-
      read_mdp_hw_revision(mdp5_kms, &major, &minor);
      mdp5_kms->cfg = mdp5_cfg_init(mdp5_kms, major, minor);

@@ -893,10 +890,13 @@ static int mdp5_init(struct platform_device 
*pdev, struct drm_device *dev)

      /* set uninit-ed kms */
      priv->kms = &mdp5_kms->base.base;
+    pm_runtime_enable(&pdev->dev);
+    mdp5_kms->rpm_enabled = true;
+
      return 0;
  fail:
      if (mdp5_kms)
-        mdp5_destroy(pdev);
+        mdp5_destroy(mdp5_kms);
      return ret;
  }

@@ -953,7 +953,8 @@ static int mdp5_dev_remove(struct platform_device 
*pdev)

  static __maybe_unused int mdp5_runtime_suspend(struct device *dev)
  {
      struct platform_device *pdev = to_platform_device(dev);
-    struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
+    struct msm_drm_private *priv = platform_get_drvdata(pdev);
+    struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
      DBG("");

@@ -963,7 +964,8 @@ static __maybe_unused int 
mdp5_runtime_suspend(struct device *dev)

  static __maybe_unused int mdp5_runtime_resume(struct device *dev)
  {
      struct platform_device *pdev = to_platform_device(dev);
-    struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
+    struct msm_drm_private *priv = platform_get_drvdata(pdev);
+    struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
      DBG("");
























					Reply via email to