On 11/16/2022 12:19 AM, Dmitry Baryshkov wrote:
On 16/11/2022 10:50, Abhinav Kumar wrote:On 11/4/2022 6:03 AM, 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> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>I am confused with both the ordering and the split of this patch.You have defined DPU_MDP_PERIPH_0_REMOVED in the catalog header file in this patch but used it in the next.But you also have code in this patch which relies on setting of this bit. So if this patch is taken without the next, it will still crash.It will not crash if this patch is taken without the next one. Without the next patch the DPU driver will not match and bind against the qcom,sm8450-dpu device.
Ah okay, I just now saw that you have the compatible change also in the next patch.
So, the ordering is quite logical from my point of view: - add support for all the features required for the device - add the device compat string & catalog entryRather, you should combine the define part of this patch to the next patch in the series https://patchwork.freedesktop.org/patch/510114/?series=108883&rev=3 , then move that one in front of this patch.No. This way we'll have a state (after adding the next patch) when the sm8450 support is enabled, but the top-hole is not handled, leading to a crash.
What if you split the compatible to a separate patch like what SM8350 did. https://patchwork.freedesktop.org/patch/511659/?series=110924&rev=1 So, we have hw catalog changes ---> snapshot fix ---> add the compatible. That will make both of us happy?
So that its much more coherent that you defined DPU_MDP_PERIPH_0_REMOVED both in the catalog header and used it in the catalog.c file and the in the next change you used the caps to avoid touching that register.I'd say it's rather strange way. When I see a define/feature addition, I'd prefer to seethe implementation too.Regarding the TOP hole itself, I need one day to investigate this. I am waiting for permissions to the documentation.If i cannot get access by the time you have re-ordered this, I will ack this once the reorder is done within a day.For the reference: [1][1] https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/commit/f9ff8af5b640147f3651c23551c60f81f62874b1--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 11 +++++++++-- 2 files changed, 10 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.hindex 38aa38ab1568..4730f8268f2a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -92,6 +92,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.cindex f3660cd14f4f..95d8765c1c53 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_kmsm_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 (dpu_kms->hw_mdp->caps->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"); + } 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); }
