On Wed, Nov 29, 2023 at 07:01:37PM +0800, Andy Yan wrote:
> 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]<[email protected]>
> > >
> > > /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]<[email protected]>
> > > ---
> > >
> > > (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 ?
Instead of:
static const struct vop_dump_regs rk3588_dump_regs[] = {
{ RK3568_REG_CFG_DONE, "SYS", {0}, 0 },
do:
static const struct vop_dump_regs rk3588_dump_regs[] = {
{
.offset = RK3568_REG_CFG_DONE.
.name = "SYS"
},
...
It's a bit more verbose but better readable for people who don't know
the order of the struct members.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Sascha:
On 11/29/23 20:59, Sascha Hauer wrote:
> On Wed, Nov 29, 2023 at 07:01:37PM +0800, Andy Yan wrote:
>> 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]<[email protected]>
>>>>
>>>> /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]<[email protected]>
>>>> ---
>>>>
>>>> (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 ?
>
> Instead of:
>
> static const struct vop_dump_regs rk3588_dump_regs[] = {
> { RK3568_REG_CFG_DONE, "SYS", {0}, 0 },
>
> do:
>
> static const struct vop_dump_regs rk3588_dump_regs[] = {
> {
> .offset = RK3568_REG_CFG_DONE.
> .name = "SYS"
> },
> ...
>
> It's a bit more verbose but better readable for people who don't know
> the order of the struct members.
Got it, will do.
>
> Sascha
>