Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 12/9/2021 2:18 PM, Abhinav Kumar wrote:
> On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
> > We are preparing to change DPU plane implementation. Move SSPP debugfs
> > code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++++++++++++++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  4 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 82 +++------------------
> >   4 files changed, 84 insertions(+), 70 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 d77eb7da5daf..ae3cf2e4d7d9 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > @@ -8,6 +8,8 @@
> >   #include "dpu_hw_sspp.h"
> >   #include "dpu_kms.h"
> > +#include <drm/drm_file.h>
> > +
> >   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
> >   /* DPU_SSPP_SRC */
> > @@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
> >           c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
> >   }
> > +#ifdef CONFIG_DEBUG_FS
> > +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
> > dpu_kms *kms, struct dentry *entry)
> > +{
> > +    const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
> > +    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
> > +    struct dentry *debugfs_root;
> > +    char sspp_name[32];
> > +
> > +    snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
> > +
> > +    /* create overall sub-directory for the pipe */
> > +    debugfs_root =
> > +        debugfs_create_dir(sspp_name, entry);
>
>
> I would like to take a different approach to this. Let me know what you 
> think.
>
> Let the directory names still be the drm plane names as someone who 
> would first get the DRM state and then try to lookup the register values 
> of that plane would not know where to look now.
>
> Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an 
> extra entry which tells the sspp_index.
>
> This will also establish the plane to SSPP mapping.
>
> Now when planes go virtual in the future, this will be helpful even more
> so that we can know the plane to SSPP mapping.

OR i like rob's suggestion of implementing the atomic_print_state 
callback which will printout the drm plane to SSPP mapping along with 
this change so that when we look at DRM state, we also know the plane
to SSPP mapping and look in the right SSPP's dir.
> > +
> > +    /* don't error check these */
> > +    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,
> > +            (u32 *) &cfg->xin_id);
> > +    debugfs_create_u32("clk_ctrl",
> > +            0400,
> > +            debugfs_root,
> > +            (u32 *) &cfg->clk_ctrl);
> > +    debugfs_create_x32("creq_vblank",
> > +            0600,
> > +            debugfs_root,
> > +            (u32 *) &sblk->creq_vblank);
> > +    debugfs_create_x32("danger_vblank",
> > +            0600,
> > +            debugfs_root,
> > +            (u32 *) &sblk->danger_vblank);
> > +
> > +    return 0;
> > +}
> > +#endif
> > +
> > +
> >   static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
> >           void __iomem *addr,
> >           struct dpu_mdss_cfg *catalog,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > index e8939d7387cb..cef281687bab 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > @@ -381,6 +381,7 @@ struct dpu_hw_pipe {
> >       struct dpu_hw_sspp_ops ops;
> >   };
> > +struct dpu_kms;
> >   /**
> >    * dpu_hw_sspp_init - initializes the sspp hw driver object.
> >    * Should be called once before accessing every pipe.
> > @@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp 
> > idx,
> >    */
> >   void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
> > +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
> > *debugfs_root);
> > +int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct 
> > dpu_kms *kms, struct dentry *entry);
> > +
> >   #endif /*_DPU_HW_SSPP_H */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 7e7a619769a8..de9efe6dcf7c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -281,6 +281,7 @@ static int dpu_kms_debugfs_init(struct msm_kms 
> > *kms, struct drm_minor *minor)
> >       dpu_debugfs_danger_init(dpu_kms, entry);
> >       dpu_debugfs_vbif_init(dpu_kms, entry);
> >       dpu_debugfs_core_irq_init(dpu_kms, entry);
> > +    dpu_debugfs_sspp_init(dpu_kms, entry);
> >       for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
> >           if (priv->dp[i])
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index ef66af696a40..cc7a7eb84fdd 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -13,7 +13,6 @@
> >   #include <drm/drm_atomic.h>
> >   #include <drm/drm_atomic_uapi.h>
> >   #include <drm/drm_damage_helper.h>
> > -#include <drm/drm_file.h>
> >   #include <drm/drm_gem_atomic_helper.h>
> >   #include "msm_drv.h"
> > @@ -1356,78 +1355,22 @@ void dpu_plane_danger_signal_ctrl(struct 
> > drm_plane *plane, bool enable)
> >       pm_runtime_put_sync(&dpu_kms->pdev->dev);
> >   }
> > -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
> > +/* SSPP live inside dpu_plane private data only. Enumerate them here. */
> > +void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry 
> > *debugfs_root)
> >   {
> > -    struct dpu_plane *pdpu = to_dpu_plane(plane);
> > -    struct dpu_kms *kms = _dpu_plane_get_kms(plane);
> > -    const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
> > -    const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
> > -    struct dentry *debugfs_root;
> > -
> > -    /* create overall sub-directory for the pipe */
> > -    debugfs_root =
> > -        debugfs_create_dir(plane->name,
> > -                plane->dev->primary->debugfs_root);
> > -
> > -    /* don't error check these */
> > -    debugfs_create_xul("features", 0600,
> > -            debugfs_root, (unsigned long 
> > *)&pdpu->pipe_hw->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,
> > -            (u32 *) &cfg->xin_id);
> > -    debugfs_create_u32("clk_ctrl",
> > -            0400,
> > -            debugfs_root,
> > -            (u32 *) &cfg->clk_ctrl);
> > -    debugfs_create_x32("creq_vblank",
> > -            0600,
> > -            debugfs_root,
> > -            (u32 *) &sblk->creq_vblank);
> > -    debugfs_create_x32("danger_vblank",
> > -            0600,
> > -            debugfs_root,
> > -            (u32 *) &sblk->danger_vblank);
> > +    struct drm_plane *plane;
> > +    struct dentry *entry = debugfs_create_dir("sspp", debugfs_root);
> > -    return 0;
> > -}
> > -#else
> > -static int _dpu_plane_init_debugfs(struct drm_plane *plane)
> > -{
> > -    return 0;
> > -}
> > -#endif
> > +    if (IS_ERR(entry))
> > +        return;
> > -static int dpu_plane_late_register(struct drm_plane *plane)
> > -{
> > -    return _dpu_plane_init_debugfs(plane);
> > +    drm_for_each_plane(plane, dpu_kms->dev) {
> > +        struct dpu_plane *pdpu = to_dpu_plane(plane);
> > +
> > +        _dpu_hw_sspp_init_debugfs(pdpu->pipe_hw, dpu_kms, entry);
> > +    }
> >   }
> > +#endif
> >   static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
> >           uint32_t format, uint64_t modifier)
> > @@ -1453,7 +1396,6 @@ static const struct drm_plane_funcs 
> > dpu_plane_funcs = {
> >           .reset = dpu_plane_reset,
> >           .atomic_duplicate_state = dpu_plane_duplicate_state,
> >           .atomic_destroy_state = dpu_plane_destroy_state,
> > -        .late_register = dpu_plane_late_register,
> >           .format_mod_supported = dpu_plane_format_mod_supported,
> >   };