Re: [PATCH v5 08/10] drm/msm/dpu: add support for MDP_TOP blackhole

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

 



On 25/11/2022 08:01, Abhinav Kumar wrote:


On 11/23/2022 1:04 PM, Dmitry Baryshkov wrote:
On sm8450 a register block was removed from MDP TOP. Accessing it during
snapshotting results in NoC errors / immediate reboot. Skip accessing
these registers during snapshot.

Tested-by: Vinod Koul <vkoul@xxxxxxxxxx>
Reviewed-by: Vinod Koul <vkoul@xxxxxxxxxx>
Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  3 +++
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 11 +++++++++--
  2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 38aa38ab1568..8da4c5ba6dc3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -82,6 +82,8 @@ enum {
   * @DPU_MDP_UBWC_1_0,      This chipsets supports Universal Bandwidth
   *                         compression initial revision
   * @DPU_MDP_UBWC_1_5,      Universal Bandwidth compression version 1.5
+ * @DPU_MDP_PERIPH_0_REMOVED Indicates that access to periph top0 block results
+ *               in a failure
shouldnt this be that "indicates that the top register block is not contiguous and the two sub-blocks are separated by an offset"

Not so sure. Your suggestion is closer to the dynamic case, where all the sizes are dynamic in catalog. Since the patch uses fixed offsets, I'd mention periph0 instead (like the downstream does).

   * @DPU_MDP_MAX            Maximum value
   */
@@ -92,6 +94,7 @@ enum {
      DPU_MDP_UBWC_1_0,
      DPU_MDP_UBWC_1_5,
      DPU_MDP_AUDIO_SELECT,
+    DPU_MDP_PERIPH_0_REMOVED,
      DPU_MDP_MAX
  };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index f3660cd14f4f..4ac14de55139 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -927,8 +927,15 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
          msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
                  dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
-    msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
-            dpu_kms->mmio + cat->mdp[0].base, "top");
+    if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
+        msm_disp_snapshot_add_block(disp_state, 0x380,
+                dpu_kms->mmio + cat->mdp[0].base, "top");
+        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len - 0x3a8,
+                dpu_kms->mmio + cat->mdp[0].base + 0x3a8, "top_2");

I recall one of the comments from konrad that this should come from the catalog rather than a hard-coded offset which you wanted to keep it for a later time. I am fine with that.

But instead of a hard-coded offset, do you want to have a macro so that atleast we know what the value means and can fix it in the future? Otherwise it would end up being one of those numbers which someone later on wouldnt understand where it comes from and what it means.

Yes, I macro makes sense to me. I'll fix in v6.


+    } else {
+        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
+                dpu_kms->mmio + cat->mdp[0].base, "top");
+    }
      pm_runtime_put_sync(&dpu_kms->pdev->dev);
  }

--
With best wishes
Dmitry




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux