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"
* @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.
+ } 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);
}
