Re: [PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sascha:



On 11/29/23 16:52, Sascha Hauer wrote:
On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
    Hi Sascha:

    thanks for you review.

    On 11/27/23 18:13, Sascha Hauer wrote:

  On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:

  From: Andy Yan [1]<andy.yan@xxxxxxxxxxxxxx>

  /sys/kernel/debug/dri/vop2/summary:  dump vop display state
  /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
  /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
  activated modules

  Signed-off-by: Andy Yan [2]<andy.yan@xxxxxxxxxxxxxx>
  ---

  (no changes since v1)

   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++++++++++++++++++
   1 file changed, 399 insertions(+)

  diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
  index 9eecbe1f71f9..4a2342209c15 100644
  --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
  +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
  @@ -27,6 +27,7 @@
   #include <drm/drm_debugfs.h>
   #include <drm/drm_flip_work.h>
   #include <drm/drm_framebuffer.h>
  +#include <drm/drm_gem_framebuffer_helper.h>
   #include <drm/drm_probe_helper.h>
   #include <drm/drm_vblank.h>

  @@ -187,6 +188,7 @@ struct vop2 {
           */
          u32 registered_num_wins;

  +       struct resource *res;
          void __iomem *regs;
          struct regmap *map;

  @@ -228,6 +230,44 @@ struct vop2 {
   #define vop2_output_if_is_lvds(x)      (x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
   #define vop2_output_if_is_dpi(x)       (x == ROCKCHIP_VOP2_EP_RGB0)

  +struct vop2_regs_dump {
  +       const char *name;
  +       u32 base;
  +       u32 en_reg;
  +       u32 en_val;
  +       u32 en_mask;
  +};
  +
  +/*
  + * bus-format types.
  + */
  +struct drm_bus_format_enum_list {
  +       int type;
  +       const char *name;
  +};
  +
  +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
  +       { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
  +       { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
  +       { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
  +       { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
  +       { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
  +       { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
  +       { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
  +       { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
  +       { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
  +       { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
  +       { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
  +       { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
  +       { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
  +       { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
  +       { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
  +       { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
  +       { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
  +       { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
  +};
  +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
  +
   static const struct regmap_config vop2_regmap_config;

   static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
  @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
          .atomic_disable = vop2_crtc_atomic_disable,
   };

  +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
  +{
  +       struct drm_connector_list_iter conn_iter;
  +       struct drm_connector *connector;
  +
  +       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
  +       drm_for_each_connector_iter(connector, &conn_iter) {
  +               if (crtc->state->connector_mask & drm_connector_mask(connector))
  +                       seq_printf(s, "    Connector: %s\n", connector->name);
  +
  +       }
  +       drm_connector_list_iter_end(&conn_iter);
  +}
  +
  +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
  +{
  +       struct vop2_win *win = to_vop2_win(plane);
  +       struct drm_plane_state *pstate = plane->state;
  +       struct drm_rect *src, *dst;
  +       struct drm_framebuffer *fb;
  +       struct drm_gem_object *obj;
  +       struct rockchip_gem_object *rk_obj;
  +       bool xmirror;
  +       bool ymirror;
  +       bool rotate_270;
  +       bool rotate_90;
  +       dma_addr_t fb_addr;
  +       int i;
  +
  +       seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
  +       if (!pstate || !pstate->fb)
  +               return 0;
  +
  +       fb = pstate->fb;
  +       src = &pstate->src;
  +       dst = &pstate->dst;
  +       xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
  +       ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
  +       rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
  +       rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
  +
  +       seq_printf(s, "\twin_id: %d\n", win->win_id);
  +
  +       seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
  +                  &fb->format->format,
  +                  drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
  +                  pstate->alpha >> 8);
  +       seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
  +                  xmirror, ymirror, rotate_90, rotate_270);
  +       seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
  +       seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
  +                  src->y1 >> 16, drm_rect_width(src) >> 16,
  +                  drm_rect_height(src) >> 16);
  +       seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
  +                  drm_rect_width(dst), drm_rect_height(dst));
  +
  +       for (i = 0; i < fb->format->num_planes; i++) {
  +               obj = fb->obj[0];
  +               rk_obj = to_rockchip_obj(obj);
  +               fb_addr = rk_obj->dma_addr + fb->offsets[0];
  +
  +               seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
  +                          i, &fb_addr, fb->pitches[i], fb->offsets[i]);
  +       }
  +
  +       return 0;
  +}
  +
  +static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
  +{
  +       struct vop2_video_port *vp = to_vop2_video_port(crtc);
  +       struct drm_crtc_state *cstate = crtc->state;
  +       struct rockchip_crtc_state *vcstate;
  +       struct drm_display_mode *mode;
  +       struct drm_plane *plane;
  +       bool interlaced;
  +
  +       seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
  +                  "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
  +
  +       if (!cstate || !cstate->active)
  +               return 0;
  +
  +       mode = &crtc->state->adjusted_mode;
  +       vcstate = to_rockchip_crtc_state(cstate);
  +       interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
  +
  +       vop2_dump_connector_on_crtc(crtc, s);
  +       seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
  +                   drm_get_bus_format_name(vcstate->bus_format));
  +       seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
  +       seq_printf(s, " color_space[%d]\n", vcstate->color_space);
  +       seq_printf(s, "    Display mode: %dx%d%s%d\n",
  +                   mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
  +                   drm_mode_vrefresh(mode));
  +       seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
  +                   mode->clock, mode->crtc_clock, mode->type, mode->flags);
  +       seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
  +                   mode->hsync_end, mode->htotal);
  +       seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
  +                   mode->vsync_end, mode->vtotal);
  +
  +       drm_atomic_crtc_for_each_plane(plane, crtc) {
  +               vop2_plane_state_dump(s, plane);
  +       }
  +
  +       return 0;
  +}
  +
  +static int vop2_summary_show(struct seq_file *s, void *data)
  +{
  +       struct drm_info_node *node = s->private;
  +       struct drm_minor *minor = node->minor;
  +       struct drm_device *drm_dev = minor->dev;
  +       struct drm_crtc *crtc;
  +
  +       drm_modeset_lock_all(drm_dev);
  +       drm_for_each_crtc(crtc, drm_dev) {
  +               vop2_crtc_state_dump(crtc, s);
  +       }
  +       drm_modeset_unlock_all(drm_dev);
  +
  +       return 0;
  +}
  +
  +static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
  +{
  +       resource_size_t start;
  +       const int reg_num = 0x110 / 4;

  If I'm not mistaken this prints a register space of 0x110 bytes.
  Shouldn't it be 0x100 bytes instead?

  Also, are all these register spaces really have the same size? Does it
  make sense to add the size to struct vop2_regs_dump?

    In fact, most used registers of the most blocks are not more than 100, but
    for Cluster windows,

    there is a CLUSTER_CTRL register sting at 0x100.

    I think i should add the size to struct vop2_regs_dump.


  +       u32 val;
  +       int i;
  +
  +       if (dump->en_mask) {
  +               val = vop2_readl(vop2, dump->base + dump->en_reg);
  +               if ((val & dump->en_mask) != dump->en_val)
  +                       return;
  +       }
  +       seq_printf(s, "\n%s:\n", dump->name);
  +
  +       start = vop2->res->start + dump->base;
  +       for (i = 0; i < reg_num;) {
  +               seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
  +                          vop2_readl(vop2, dump->base + (4 * i)),
  +                          vop2_readl(vop2, dump->base + (4 * (i + 1))),
  +                          vop2_readl(vop2, dump->base + (4 * (i + 2))),
  +                          vop2_readl(vop2, dump->base + (4 * (i + 3))));
  +               i += 4;
  +       }
  +
  +}
  +
  +static int vop2_regs_show(struct seq_file *s, void *arg)
  +{
  +       struct drm_info_node *node = s->private;
  +       struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
  +       struct drm_minor *minor = node->minor;
  +       struct drm_device *drm_dev = minor->dev;
  +
  +       struct vop2_regs_dump dump;
  +
  +       drm_modeset_lock_all(drm_dev);
  +
  +       if (vop2->enable_count) {
  +               dump.en_mask = 0;
  +
  +               dump.name = "SYS";
  +               dump.base = RK3568_REG_CFG_DONE;
  +               vop2_regs_print(vop2, s, &dump);

  Can you create a statically initialized array of struct vop2_regs_dump
  and iterate over it?
  You would need an additional present_in_soc_xy flag in struct
  vop2_regs_dump, but other than that I don't see a problem and the result
  might look better.

  For the windows it might also be an option to iterate over
  vop2->data->win instead. This array already contains the register base
  addresses and window names.

    In fact, we have a dump_regs  arrar in vop2_data per soc in our bsp
    kernel[0],

    do you like something like that?

    [0]
    [3]https://github.com/armbian/linux-rockchip/blob/rk-5.10-rkr6/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c#L3684

This looks good from a first glance. I would suggest using C99
initializers though.


Thanks for your reply, but am not quiet claer about the C99 initializers, would you plase make it more specific,
or give some example ?



  Not sure if we really need an additional debugfs entry to print only the
  active entities, but if we do then we could avoid a bit of code
  duplication by adding creating a common register dump function called
  from vop2_regs_show() and vop2_active_regs_show() which takes an
  additional ignore_disabled argument.

    As the whole vop2 registers block is very large, so some times only dump

    active modules make we dig bugs easier.

    It seems that if we  "initialized array of struct vop2_regs_dump" as you
    said befor, we can avoid

    some duplication code here?

Yes.

Sascha





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux