| 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: <4db7ea27-4e87-b02b-aac8-9e1c1242dc59@linaro.org>
Date: Sat, 24 Jun 2023 15:14:04 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jessica Zhang <quic_jesszhan@...cinc.com>,
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
Subject: Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers
of sub blocks
On 24/06/2023 04:23, Jessica Zhang wrote:
>
>
> On 6/23/2023 5:09 PM, 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
>>
>>>> +{
>>>> + 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)
>
> Hey Dmitry,
>
> FWIW, I second Abhinav's points about the sblk names. For example, if in
> the future we want to add a "_rot" suffix specifically to the
> VIG_SBLK_ROT.scaler name, it would be easier to just make that change in
> the HW catalog.
But why? The scaler is the same qseed3 scaler. We do not dump features,
they are constant for the platform in question.
>
>>> - Use sblk->foo_blk.len to check if it should be printed or not.
> From my understanding, your suggestion is to replace the feature flag
> checks with a sblk.len > 0 check.
>
> I don't think that would be good because it wouldn't be correct to
> assume that the sblk will always be present. For example, for
> DPU_HW_BLK_DSC, the sblks will only be present for DSC_BLK_1_2.
I don't consider sub-block as being always present. But if it present,
it has non-zero length. If its length is zero, we have nothing to dump
for it.
> In addition, it is possible for sblks, like pp_sblk_te.te2, to have a
> len of 0. While the register space of that specific sblk will not be
> printed, I'd prefer the devcore dump to reflect what is present within
> the HW catalog so that the user knows which pingpong blks have the TE2
> sblk.
I'd consider this as dumping the feature instead of dumping the
registers. If you think it is necessary to ease decoding of the dump,
consider adding block.features to the dump instead.
>
> Thanks,
>
> Jessica Zhang
>
>>>
>>
>> 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.
>>
--
With best wishes
Dmitry
Powered by blists - more mailing lists