| Message ID | 20221123210403.3593366-9-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm: add support for SM8450 | expand |
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@kernel.org> > Reviewed-by: Vinod Koul <vkoul@kernel.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > 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); > }
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@kernel.org> >> Reviewed-by: Vinod Koul <vkoul@kernel.org> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> 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); >> }
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 * @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"); + } 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); }