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 4:19 PM, Dmitry Baryshkov wrote:
> On 10/12/2021 01:27, Abhinav Kumar wrote:
> > 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.
>
> I'd add atomic_print_state(), it seems simpler (and more future-proof).
Now, that https://patchwork.freedesktop.org/patch/467031/ has been pushed,
Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > > +
> > > > +    /* 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,
> > > >   };