| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <412f68a3-e3cc-f26e-2e3d-59727e5c48d8@linaro.org>
Date: Sat, 24 Jun 2023 15:07:51 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>,
Ryan McCann <quic_rmccann@...cinc.com>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Cc: Rob Clark <robdclark@...omium.org>, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, quic_jesszhan@...cinc.com
Subject: Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers
of sub blocks
On 24/06/2023 03:09, Abhinav Kumar wrote:
>
>
> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>> On 23/06/2023 02:48, Ryan McCann wrote:
>>> Currently, the device core dump mechanism does not dump registers of sub
>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>> function to dump hardware blocks that contain sub blocks.
>>>
>>> Signed-off-by: Ryan McCann <quic_rmccann@...cinc.com>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194
>>> +++++++++++++++++++++++++++-----
>>> 1 file changed, 168 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index aa8499de1b9f..9b1b1c382269 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms
>>> *kms)
>>> return 0;
>>> }
>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state
>>> *disp_state,
>>> + void __iomem *mmio, void *blk,
>>> + enum dpu_hw_blk_type blk_type)
>>
>> No. Such multiplexers add no value to the code. Please inline it.
>>
>> Not to mention that this patch is hard to review. You both move
>> existing code and add new features. If it were to go, it should have
>> been split into two patches: one introducing the multiplexer and
>> another one adding subblocks.
>>
>
> Ok. we can split this into:
>
> 1) adding the multiplexer
> 2) adding sub-blk parsing support inside the multiplexer
I'd say, drop the multiplexer completely. It adds no value here. It is
only used from dpu_kms_mdp_snapshot(). If the code there was complex
enough, it would have made sense to _split_ the function. But even in
such case there would be no point in having multiplexer. We do not
enumerate block by type.
>
>>> +{
>>> + u32 base;
>>> +
>>> + switch (blk_type) {
>>> + case DPU_HW_BLK_TOP:
>>> + {
>>> + struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>> +
>>> + if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>> + msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>> + mmio + top->base, "top");
>>> + msm_disp_snapshot_add_block(disp_state, top->len -
>>> MDP_PERIPH_TOP0_END,
>>> + mmio + top->base + MDP_PERIPH_TOP0_END,
>>> + "top_2");
>>> + } else {
>>> + msm_disp_snapshot_add_block(disp_state, top->len, mmio +
>>> top->base, "top");
>>> + }
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_LM:
>>> + {
>>> + struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, mixer->len, mmio +
>>> mixer->base, "%s",
>>> + mixer->name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_CTL:
>>> + {
>>> + struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, ctl->len, mmio +
>>> ctl->base, "%s",
>>> + ctl->name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_INTF:
>>> + {
>>> + struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, intf->len, mmio +
>>> intf->base, "%s",
>>> + intf->name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_WB:
>>> + {
>>> + struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, wb->len, mmio +
>>> wb->base, "%s",
>>> + wb->name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_SSPP:
>>> + {
>>> + struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>> + const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>> +
>>> + base = sspp_block->base;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, sspp_block->len,
>>> mmio + base, "%s",
>>> + sspp_block->name);
>>> +
>>> + if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>> + sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>> + sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>> + msm_disp_snapshot_add_block(disp_state,
>>> sblk->scaler_blk.len,
>>> + mmio + base + sblk->scaler_blk.base,
>>> "%s_%s",
>>> + sspp_block->name, sblk->scaler_blk.name);
>>
>> Actually, it would be better to:
>> - drop name from all sblk instances (and use known string instead of
>> the sblk name here)
>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>
>
> No, I dont agree. If we drop the names from the sub_blk in the catalog,
> we will end up using "sub_blk_name" string here in the code to indicate
> which blk that is in the dump.
>
> If we add more sub_blks in the catalog in the future we need to keep
> changing the code over here. Thats not how it should be.
>
> Leaving the names in the catalog ensures that this code wont change and
> only catalog changes when we add a new sub_blk either for an existing or
> new chipset.
>
> catalog is indicating the new blk, and dumping code just prints it.
>
> with your approach, dumping code will or can keep changing with chipsets
> or sub_blks. Thats not how it should be.
Well, we do not enumerate sub-blocks in any way, they are not indexed.
So even with sblk->blk.name in place, adding new sub-block would require
adding new code here. That's why I wrote that the calling code knows
which sub-block it refers to.
Let me extract the relevant code (skipping all the conditions for now):
msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, "%s",
sspp_block->name);
if (have_scaler)
msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
mmio + base + sblk->scaler_blk.base, "%s_%s",
sspp_block->name, sblk->scaler_blk.name);
if (have_csc)
msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
mmio + base + sblk->csc_blk.base, "%s_%s",
sspp_block->name, sblk->csc_blk.name);
Consider adding new sub-block, "baz". We would still require manual
addition of the following code:
msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
mmio + base + sblk->baz_blk.base, "%s_%s",
sspp_block->name, sblk->baz_blk.name);
Compare this with:
msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
mmio + base + sblk->baz_blk.base, "%s_baz",
sspp_block->name);
Moreover, if we follow the style of dpu_kms_mdp_snapshot() (which
doesn't use name), it should be:
msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
mmio + base + sblk->baz_blk.base, "sspp%d_baz", idx);
>
>>> +
>>> + if (sspp_block->features & BIT(DPU_SSPP_CSC) ||
>>> sspp_block->features
>>> + & BIT(DPU_SSPP_CSC_10BIT))
>>
>> A very bad use of indentation. In future please split logically rather
>> than just filling the line up to the line width.
>>
>>> + msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>>> + mmio + base + sblk->csc_blk.base, "%s_%s",
>>> + sspp_block->name, sblk->csc_blk.name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_DSPP:
>>> + {
>>> + struct dpu_dspp_cfg *dspp_block = (struct dpu_dspp_cfg *)blk;
>>> +
>>> + base = dspp_block->base;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, dspp_block->len,
>>> mmio + base, "%s",
>>> + dspp_block->name);
>>> +
>>> + if (dspp_block->features & BIT(DPU_DSPP_PCC))
>>> + msm_disp_snapshot_add_block(disp_state,
>>> dspp_block->sblk->pcc.len,
>>> + mmio + base + dspp_block->sblk->pcc.base,
>>> + "%s_%s", dspp_block->name,
>>> + dspp_block->sblk->pcc.name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_PINGPONG:
>>> + {
>>> + struct dpu_pingpong_cfg *pingpong_block = (struct
>>> dpu_pingpong_cfg *)blk;
>>> + const struct dpu_pingpong_sub_blks *sblk =
>>> pingpong_block->sblk;
>>> +
>>> + base = pingpong_block->base;
>>> +
>>> + msm_disp_snapshot_add_block(disp_state, pingpong_block->len,
>>> mmio + base, "%s",
>>> + pingpong_block->name);
>>> +
>>> + if (pingpong_block->features & BIT(DPU_PINGPONG_TE2))
>>> + msm_disp_snapshot_add_block(disp_state, sblk->te2.len,
>>> + mmio + base + sblk->te2.base, "%s_%s",
>>> + pingpong_block->name, sblk->te2.name);
>>> +
>>> + if (pingpong_block->features & BIT(DPU_PINGPONG_DITHER))
>>> + msm_disp_snapshot_add_block(disp_state, sblk->dither.len,
>>> + mmio + base + sblk->dither.base, "%s_%s",
>>> + pingpong_block->name, sblk->dither.name);
>>> + break;
>>> + }
>>> + case DPU_HW_BLK_DSC:
>>> + {
>>> + struct dpu_dsc_cfg *dsc_block = (struct dpu_dsc_cfg *)blk;
>>> +
>>> + base = dsc_block->base;
>>> +
>>> + if (dsc_block->features & BIT(DPU_DSC_HW_REV_1_2)) {
>>> + struct dpu_dsc_blk enc = dsc_block->sblk->enc;
>>> + struct dpu_dsc_blk ctl = dsc_block->sblk->ctl;
>>> +
>>> + /* For now, pass in a length of 0 because the DSC_BLK
>>> register space
>>> + * overlaps with the sblks' register space.
>>> + *
>>> + * TODO: Pass in a length of 0 t0 DSC_BLK_1_2 in the HW
>>> catalog where
>>> + * applicable.
>>
>> Nice catch, thank you. We should fix that.
>>
>
> Yes and we would have fixed that ourself if you wanted that with this
> series as another patch.
This is not relevant to this series, so it should be fixed in a separate
series.
--
With best wishes
Dmitry
Powered by blists - more mailing lists