| Message ID | 20230521172147.4163085-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] drm/msm/dpu: drop SSPP register dumpers | expand |
On 5/21/2023 10:21 AM, Dmitry Baryshkov wrote: > Drop SSPP-specifig debugfs register dumps in favour of using > debugfs/dri/0/kms or devcoredump. > I did see another series which removes src_blk from the catalog (I am yet to review that one) . Lets assume that one is fine and this change will be going on top of that one right? The concern I have with this change is that although I do agree that we should be in favor of using debugfs/dri/0/kms ( i have used it a few times and it works pretty well ), devcoredump does not have the support to dump sub-blocks . Something which we should add with priority because even with DSC blocks with the separation of enc/ctl blocks we need that like I wrote in one of the responses. So the "len" of the blocks having sub-blocks will be ignored in favor of the len of the sub-blocks. If we remove this without adding that support first, its a loss of debug functionality. Can we retain these blocks and remove dpu_debugfs_create_regset32 in a different way? > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 25 --------------------- > 1 file changed, 25 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > index bfd82c2921af..6c5ebee2f7cd 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > @@ -727,31 +727,6 @@ int _dpu_hw_sspp_init_debugfs(struct dpu_hw_sspp *hw_pipe, struct dpu_kms *kms, > debugfs_create_xul("features", 0600, > debugfs_root, (unsigned long *)&hw_pipe->cap->features); > > - /* add register dump support */ > - dpu_debugfs_create_regset32("src_blk", 0400, > - debugfs_root, > - sblk->src_blk.base + cfg->base, > - sblk->src_blk.len, > - kms); > - > - if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) || > - cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) || > - cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) || > - cfg->features & BIT(DPU_SSPP_SCALER_QSEED4)) > - dpu_debugfs_create_regset32("scaler_blk", 0400, > - debugfs_root, > - sblk->scaler_blk.base + cfg->base, > - sblk->scaler_blk.len, > - kms); > - > - if (cfg->features & BIT(DPU_SSPP_CSC) || > - cfg->features & BIT(DPU_SSPP_CSC_10BIT)) > - dpu_debugfs_create_regset32("csc_blk", 0400, > - debugfs_root, > - sblk->csc_blk.base + cfg->base, > - sblk->csc_blk.len, > - kms); > - > debugfs_create_u32("xin_id", > 0400, > debugfs_root,
On Tue, 23 May 2023 at 23:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 5/21/2023 10:21 AM, Dmitry Baryshkov wrote: > > Drop SSPP-specifig debugfs register dumps in favour of using > > debugfs/dri/0/kms or devcoredump. > > > > I did see another series which removes src_blk from the catalog (I am > yet to review that one) . Lets assume that one is fine and this change > will be going on top of that one right? > > The concern I have with this change is that although I do agree that we > should be in favor of using debugfs/dri/0/kms ( i have used it a few > times and it works pretty well ), devcoredump does not have the support > to dump sub-blocks . Something which we should add with priority because > even with DSC blocks with the separation of enc/ctl blocks we need that > like I wrote in one of the responses. > > So the "len" of the blocks having sub-blocks will be ignored in favor of > the len of the sub-blocks. > > If we remove this without adding that support first, its a loss of debug > functionality. > > Can we retain these blocks and remove dpu_debugfs_create_regset32 in a > different way? Let's add subblocks dumping. This sounds like a good idea. I'll take a look closer to the weekend. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 25 --------------------- > > 1 file changed, 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > index bfd82c2921af..6c5ebee2f7cd 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > @@ -727,31 +727,6 @@ int _dpu_hw_sspp_init_debugfs(struct dpu_hw_sspp *hw_pipe, struct dpu_kms *kms, > > debugfs_create_xul("features", 0600, > > debugfs_root, (unsigned long *)&hw_pipe->cap->features); > > > > - /* add register dump support */ > > - dpu_debugfs_create_regset32("src_blk", 0400, > > - debugfs_root, > > - sblk->src_blk.base + cfg->base, > > - sblk->src_blk.len, > > - kms); > > - > > - if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) || > > - cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) || > > - cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) || > > - cfg->features & BIT(DPU_SSPP_SCALER_QSEED4)) > > - dpu_debugfs_create_regset32("scaler_blk", 0400, > > - debugfs_root, > > - sblk->scaler_blk.base + cfg->base, > > - sblk->scaler_blk.len, > > - kms); > > - > > - if (cfg->features & BIT(DPU_SSPP_CSC) || > > - cfg->features & BIT(DPU_SSPP_CSC_10BIT)) > > - dpu_debugfs_create_regset32("csc_blk", 0400, > > - debugfs_root, > > - sblk->csc_blk.base + cfg->base, > > - sblk->csc_blk.len, > > - kms); > > - > > debugfs_create_u32("xin_id", > > 0400, > > debugfs_root,
On 5/24/2023 2:48 AM, Marijn Suijten wrote: > On 2023-05-23 13:01:13, Abhinav Kumar wrote: >> >> >> On 5/21/2023 10:21 AM, Dmitry Baryshkov wrote: >>> Drop SSPP-specifig debugfs register dumps in favour of using >>> debugfs/dri/0/kms or devcoredump. >>> >> >> I did see another series which removes src_blk from the catalog (I am >> yet to review that one) . Lets assume that one is fine and this change >> will be going on top of that one right? > > It replaces src_blk with directly accessing the blk (non-sub-block) > directly, because they were overlapping anyway. > >> The concern I have with this change is that although I do agree that we >> should be in favor of using debugfs/dri/0/kms ( i have used it a few >> times and it works pretty well ), devcoredump does not have the support >> to dump sub-blocks . Something which we should add with priority because >> even with DSC blocks with the separation of enc/ctl blocks we need that >> like I wrote in one of the responses. >> >> So the "len" of the blocks having sub-blocks will be ignored in favor of >> the len of the sub-blocks. > > The sub-blocks are not always contiguous with their parent block, are > they? It's probably better to print the sub-blocks separately with Yes, not contiguous otherwise we could have just had them in one big range. > clear headers anyway rather than dumping the range parent_blk_base to > max(parent_blk_base+len, parent_blk_base+sblk_base+sblk_len...). > > - Marijn When I meant sub-block support to devcoredump, this is how I visualize them to be printed =========================SSPP xxx ======================= =========================SSPP_CSC =======================(for SSPP_xxx) =========================SSPP_QSEED =====================(for SSPP_xxx) etc OR for DSC ========================DSC_xxx ========================== ========================DSC_CTL ========================== (for DSC_xxx) ========================DSC_ENC ===========================(for DSC_xxx) This is clear enough headers. > >> If we remove this without adding that support first, its a loss of debug >> functionality. >> >> Can we retain these blocks and remove dpu_debugfs_create_regset32 in a >> different way? > > <snip>
On 2023-05-24 12:18:09, Abhinav Kumar wrote: > > > On 5/24/2023 2:48 AM, Marijn Suijten wrote: > > On 2023-05-23 13:01:13, Abhinav Kumar wrote: > >> > >> > >> On 5/21/2023 10:21 AM, Dmitry Baryshkov wrote: > >>> Drop SSPP-specifig debugfs register dumps in favour of using > >>> debugfs/dri/0/kms or devcoredump. > >>> > >> > >> I did see another series which removes src_blk from the catalog (I am > >> yet to review that one) . Lets assume that one is fine and this change > >> will be going on top of that one right? > > > > It replaces src_blk with directly accessing the blk (non-sub-block) > > directly, because they were overlapping anyway. > > > >> The concern I have with this change is that although I do agree that we > >> should be in favor of using debugfs/dri/0/kms ( i have used it a few > >> times and it works pretty well ), devcoredump does not have the support > >> to dump sub-blocks . Something which we should add with priority because > >> even with DSC blocks with the separation of enc/ctl blocks we need that > >> like I wrote in one of the responses. > >> > >> So the "len" of the blocks having sub-blocks will be ignored in favor of > >> the len of the sub-blocks. > > > > The sub-blocks are not always contiguous with their parent block, are > > they? It's probably better to print the sub-blocks separately with > > Yes, not contiguous otherwise we could have just had them in one big range. > > > clear headers anyway rather than dumping the range parent_blk_base to > > max(parent_blk_base+len, parent_blk_base+sblk_base+sblk_len...). > > > > - Marijn > > When I meant sub-block support to devcoredump, this is how I visualize > them to be printed > > =========================SSPP xxx ======================= > =========================SSPP_CSC =======================(for SSPP_xxx) > =========================SSPP_QSEED =====================(for SSPP_xxx) Yeah something along those lines, though I don't really like the `(for SSPP_xxx)` suffix which we should either drop entirely and make the "heading" less of a "heading" ========================= SSPP xxx ======================= ... ------------------------- SSPP_CSC ----------------------- ... ------------------------- SSPP_QSEED --------------------- ... And/or inline the numbers: ========================= SSPP xxx ======================= ... ----------------------- SSPP_xxx_CSC --------------------- ... ---------------------- SSPP_xxx_QSEED -------------------- ... Either works, or any other pattern in the title (e.g `SSPP xxx: CSC`) that clearly tells the blocks and sub-blocks apart. - Marijn
On Tue, 30 May 2023 at 20:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 5/29/2023 2:36 PM, Marijn Suijten wrote: > > On 2023-05-24 12:18:09, Abhinav Kumar wrote: > >> > >> > >> On 5/24/2023 2:48 AM, Marijn Suijten wrote: > >>> On 2023-05-23 13:01:13, Abhinav Kumar wrote: > >>>> > >>>> > >>>> On 5/21/2023 10:21 AM, Dmitry Baryshkov wrote: > >>>>> Drop SSPP-specifig debugfs register dumps in favour of using > >>>>> debugfs/dri/0/kms or devcoredump. > >>>>> > >>>> > >>>> I did see another series which removes src_blk from the catalog (I am > >>>> yet to review that one) . Lets assume that one is fine and this change > >>>> will be going on top of that one right? > >>> > >>> It replaces src_blk with directly accessing the blk (non-sub-block) > >>> directly, because they were overlapping anyway. > >>> > >>>> The concern I have with this change is that although I do agree that we > >>>> should be in favor of using debugfs/dri/0/kms ( i have used it a few > >>>> times and it works pretty well ), devcoredump does not have the support > >>>> to dump sub-blocks . Something which we should add with priority because > >>>> even with DSC blocks with the separation of enc/ctl blocks we need that > >>>> like I wrote in one of the responses. > >>>> > >>>> So the "len" of the blocks having sub-blocks will be ignored in favor of > >>>> the len of the sub-blocks. > >>> > >>> The sub-blocks are not always contiguous with their parent block, are > >>> they? It's probably better to print the sub-blocks separately with > >> > >> Yes, not contiguous otherwise we could have just had them in one big range. > >> > >>> clear headers anyway rather than dumping the range parent_blk_base to > >>> max(parent_blk_base+len, parent_blk_base+sblk_base+sblk_len...). > >>> > >>> - Marijn > >> > >> When I meant sub-block support to devcoredump, this is how I visualize > >> them to be printed > >> > >> =========================SSPP xxx ======================= > >> =========================SSPP_CSC =======================(for SSPP_xxx) > >> =========================SSPP_QSEED =====================(for SSPP_xxx) > > > > Yeah something along those lines, though I don't really like the `(for > > SSPP_xxx)` suffix which we should either drop entirely and make the > > "heading" less of a "heading" > > > > I wrote that "for SSPP_xxx" to explain the idea, ofcourse it wont be > part of the print itself. > > Without that, it matches what you have shared below. > > > > ========================= SSPP xxx ======================= > > ... > > ------------------------- SSPP_CSC ----------------------- > > ... > > ------------------------- SSPP_QSEED --------------------- > > ... > > > > And/or inline the numbers: > > > > ========================= SSPP xxx ======================= > > ... > > ----------------------- SSPP_xxx_CSC --------------------- > > ... > > ---------------------- SSPP_xxx_QSEED -------------------- > > ... I'd prefer this format. It eases grepping. > > > > sure this is also fine with me. > > > Either works, or any other pattern in the title (e.g `SSPP xxx: CSC`) > > that clearly tells the blocks and sub-blocks apart. > > > > - Marijn
On 2023-05-30 23:14:19, Dmitry Baryshkov wrote: > On Tue, 30 May 2023 at 20:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > On 5/29/2023 2:36 PM, Marijn Suijten wrote: > > > On 2023-05-24 12:18:09, Abhinav Kumar wrote: > > >> > > >> > > >> On 5/24/2023 2:48 AM, Marijn Suijten wrote: > > >>> On 2023-05-23 13:01:13, Abhinav Kumar wrote: > > >>>> > > >>>> > > >>>> On 5/21/2023 10:21 AM, Dmitry Baryshkov wrote: > > >>>>> Drop SSPP-specifig debugfs register dumps in favour of using > > >>>>> debugfs/dri/0/kms or devcoredump. > > >>>>> > > >>>> > > >>>> I did see another series which removes src_blk from the catalog (I am > > >>>> yet to review that one) . Lets assume that one is fine and this change > > >>>> will be going on top of that one right? > > >>> > > >>> It replaces src_blk with directly accessing the blk (non-sub-block) > > >>> directly, because they were overlapping anyway. > > >>> > > >>>> The concern I have with this change is that although I do agree that we > > >>>> should be in favor of using debugfs/dri/0/kms ( i have used it a few > > >>>> times and it works pretty well ), devcoredump does not have the support > > >>>> to dump sub-blocks . Something which we should add with priority because > > >>>> even with DSC blocks with the separation of enc/ctl blocks we need that > > >>>> like I wrote in one of the responses. > > >>>> > > >>>> So the "len" of the blocks having sub-blocks will be ignored in favor of > > >>>> the len of the sub-blocks. > > >>> > > >>> The sub-blocks are not always contiguous with their parent block, are > > >>> they? It's probably better to print the sub-blocks separately with > > >> > > >> Yes, not contiguous otherwise we could have just had them in one big range. > > >> > > >>> clear headers anyway rather than dumping the range parent_blk_base to > > >>> max(parent_blk_base+len, parent_blk_base+sblk_base+sblk_len...). > > >>> > > >>> - Marijn > > >> > > >> When I meant sub-block support to devcoredump, this is how I visualize > > >> them to be printed > > >> > > >> =========================SSPP xxx ======================= > > >> =========================SSPP_CSC =======================(for SSPP_xxx) > > >> =========================SSPP_QSEED =====================(for SSPP_xxx) > > > > > > Yeah something along those lines, though I don't really like the `(for > > > SSPP_xxx)` suffix which we should either drop entirely and make the > > > "heading" less of a "heading" > > > > > > > I wrote that "for SSPP_xxx" to explain the idea, ofcourse it wont be > > part of the print itself. > > > > Without that, it matches what you have shared below. > > > > > > > ========================= SSPP xxx ======================= > > > ... > > > ------------------------- SSPP_CSC ----------------------- > > > ... > > > ------------------------- SSPP_QSEED --------------------- > > > ... > > > > > > And/or inline the numbers: > > > > > > ========================= SSPP xxx ======================= > > > ... > > > ----------------------- SSPP_xxx_CSC --------------------- > > > ... > > > ---------------------- SSPP_xxx_QSEED -------------------- > > > ... > > I'd prefer this format. It eases grepping. Cool. And let's also have spaces around the names :) - Marijn > > > > > > > > sure this is also fine with me. > > > > > Either works, or any other pattern in the title (e.g `SSPP xxx: CSC`) > > > that clearly tells the blocks and sub-blocks apart. > > > > > > - Marijn > > > > -- > With best wishes > Dmitry
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c index bfd82c2921af..6c5ebee2f7cd 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c @@ -727,31 +727,6 @@ int _dpu_hw_sspp_init_debugfs(struct dpu_hw_sspp *hw_pipe, struct dpu_kms *kms, debugfs_create_xul("features", 0600, debugfs_root, (unsigned long *)&hw_pipe->cap->features); - /* add register dump support */ - dpu_debugfs_create_regset32("src_blk", 0400, - debugfs_root, - sblk->src_blk.base + cfg->base, - sblk->src_blk.len, - kms); - - if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) || - cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) || - cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) || - cfg->features & BIT(DPU_SSPP_SCALER_QSEED4)) - dpu_debugfs_create_regset32("scaler_blk", 0400, - debugfs_root, - sblk->scaler_blk.base + cfg->base, - sblk->scaler_blk.len, - kms); - - if (cfg->features & BIT(DPU_SSPP_CSC) || - cfg->features & BIT(DPU_SSPP_CSC_10BIT)) - dpu_debugfs_create_regset32("csc_blk", 0400, - debugfs_root, - sblk->csc_blk.base + cfg->base, - sblk->csc_blk.len, - kms); - debugfs_create_u32("xin_id", 0400, debugfs_root,
Drop SSPP-specifig debugfs register dumps in favour of using debugfs/dri/0/kms or devcoredump. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 25 --------------------- 1 file changed, 25 deletions(-)