Message ID | 20231130122001.12474-1-andyshrk@163.com |
---|---|
Headers | show |
Series | Add VOP2 support on rk3588 | expand |
Hi Sascha: On 12/5/23 17:15, Sascha Hauer wrote: > On Thu, Nov 30, 2023 at 08:24:49PM +0800, Andy Yan wrote: >> From: Andy Yan <andy.yan@rock-chips.com> >> >> /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 <andy.yan@rock-chips.com> >> >> --- >> >> Changes in v3: >> - put regs dump info in vop2_data >> >> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 268 +++++++++++++++++++ >> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 11 + >> drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 191 +++++++++++++ >> 3 files changed, 470 insertions(+) >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c >> index 6f17cc56501e..56a165c0b130 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; >> >> @@ -238,6 +240,37 @@ struct vop2 { >> >> #define vop2_output_if_is_dpi(x) ((x) == ROCKCHIP_VOP2_EP_RGB0) >> >> + >> +/* >> + * 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) >> @@ -2522,6 +2555,239 @@ 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; > > 'pstate' is dereferenced before its checked being non NULL. Either the > check is unnecessary or should be before the seq_printf() call. I will move it before seq_printf. > >> + >> + 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]; > > Did you really intend to use array index [0] here or should this rather be [i]? > If you intended to use [0] then you could move the initialization out of > the loop. It's should use index[i]. thanks for catching this. > >> + >> + 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, >> + const struct vop2_regs_dump *dump, bool active_only) >> +{ >> + resource_size_t start; >> + const int reg_num = 0x110 / 4; > > You added the number of registers to struct vop2_regs_dump, no need to > hardcode this anymore. Thanks for catching this ,will removed in next version. > >> + u32 val; >> + int i; >> + >> + if (dump->en_mask && active_only) { >> + 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; > > node->info_ent->data is a void pointer, no need to cast explicitly. Thanks for pointing out, will drop the cast in next version. > >> + struct drm_minor *minor = node->minor; >> + struct drm_device *drm_dev = minor->dev; >> + const struct vop2_regs_dump *dump; >> + unsigned int n = vop2->data->regs_dump_size; > > 'n' is used only once, it might be clearer just to use the value where > needed and drop the extra variable. Okay, will do. > >> + unsigned int i; >> + >> + drm_modeset_lock_all(drm_dev); >> + >> + if (vop2->enable_count) { >> + for (i = 0; i < n; i++) { >> + dump = &vop2->data->regs_dump[i]; >> + vop2_regs_print(vop2, s, dump, false); >> + } >> + } else { >> + seq_printf(s, "VOP disabled:\n"); > > There's nothing following after the ':', right? Then this should be > "VOP: disabled\n" or without the colon at all. the colon will be droped in next versin. > >> + } >> + drm_modeset_unlock_all(drm_dev); > > This code is repeated in vop2_active_regs_show() below. Maybe factor > this out to a common function? > Do you mean all the code between drm_modeset_lock_all and drm_modeset_unlock_all ? >> + >> + return 0; >> +} >> + >> +static int vop2_active_regs_show(struct seq_file *s, void *data) >> +{ >> + 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; >> + const struct vop2_regs_dump *dump; >> + unsigned int n = vop2->data->regs_dump_size; >> + unsigned int i; >> + >> + drm_modeset_lock_all(drm_dev); >> + if (vop2->enable_count) { >> + for (i = 0; i < n; i++) { >> + dump = &vop2->data->regs_dump[i]; >> + vop2_regs_print(vop2, s, dump, true); >> + } >> + } else { >> + seq_printf(s, "VOP disabled:\n"); >> + } >> + drm_modeset_unlock_all(drm_dev); >> + >> + return 0; >> +} >> + > > Sascha >
On Wed, Dec 06, 2023 at 06:20:58PM +0800, Andy Yan wrote: > Hi Sascha: > > > > + unsigned int n = vop2->data->regs_dump_size; > > > > 'n' is used only once, it might be clearer just to use the value where > > needed and drop the extra variable. > > Okay, will do. > > > > > + unsigned int i; > > > + > > > + drm_modeset_lock_all(drm_dev); > > > + > > > + if (vop2->enable_count) { > > > + for (i = 0; i < n; i++) { > > > + dump = &vop2->data->regs_dump[i]; > > > + vop2_regs_print(vop2, s, dump, false); > > > + } > > > + } else { > > > + seq_printf(s, "VOP disabled:\n"); > > > > There's nothing following after the ':', right? Then this should be > > "VOP: disabled\n" or without the colon at all. > > the colon will be droped in next versin. > > > > > > + } > > > + drm_modeset_unlock_all(drm_dev); > > > > This code is repeated in vop2_active_regs_show() below. Maybe factor > > this out to a common function? > > > > > Do you mean all the code between drm_modeset_lock_all and drm_modeset_unlock_all ? Including drm_modeset_lock_all() and drm_modeset_unlock_all(), yes. Sascha
Hi Sascha: On 12/6/23 19:20, Sascha Hauer wrote: > On Wed, Dec 06, 2023 at 06:20:58PM +0800, Andy Yan wrote: >> Hi Sascha: >> >>>> + unsigned int n = vop2->data->regs_dump_size; >>> >>> 'n' is used only once, it might be clearer just to use the value where >>> needed and drop the extra variable. >> >> Okay, will do. >>> >>>> + unsigned int i; >>>> + >>>> + drm_modeset_lock_all(drm_dev); >>>> + >>>> + if (vop2->enable_count) { >>>> + for (i = 0; i < n; i++) { >>>> + dump = &vop2->data->regs_dump[i]; >>>> + vop2_regs_print(vop2, s, dump, false); >>>> + } >>>> + } else { >>>> + seq_printf(s, "VOP disabled:\n"); >>> >>> There's nothing following after the ':', right? Then this should be >>> "VOP: disabled\n" or without the colon at all. >> >> the colon will be droped in next versin. >> >>> >>>> + } >>>> + drm_modeset_unlock_all(drm_dev); >>> >>> This code is repeated in vop2_active_regs_show() below. Maybe factor >>> this out to a common function? >>> >> >> >> Do you mean all the code between drm_modeset_lock_all and drm_modeset_unlock_all ? > > Including drm_modeset_lock_all() and drm_modeset_unlock_all(), yes. > Okay, will try in v4. > Sascha >
From: Andy Yan <andy.yan@rock-chips.com> This patch sets aims at enable the VOP2 support on rk3588. Main feature of VOP2 on rk3588: Four video ports: VP0 Max 4096x2160 VP1 Max 4096x2160 VP2 Max 4096x2160 VP3 Max 2048x1080 4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support 4 4K Esmart windows with line RGB/YUV support The current version support all the 8 windows with all the suppported plane format. And we don't have a upstreamed encoder/connector(HDMI/DP) for rk3588 yet, Cristian from collabora is working on adding upstream support for HDMI on rk3588. My current test(1080P/4KP60) is runing with a HDMI driver pick from downstream bsp kernel. A branch based on linux-6.7 rc3 containing all the series and HDMI driver(not compatible with mainline rk3568 hdmi) picked from downstream bsp kernel is available [0]. [0]https://github.com/andyshrk/linux/tree/rk3588-vop2-hdmi-upstream-linux-6.7-rc3-2023-11-30 Changes in v3: - split confg one patch from the vop2 driver patch - put bool variable yuv_overlay next to other bool variable - define macro for RK3568_OVL_CTRL__YUV_MODE - just write RK3568_OVL_CTRL register once in function vop2_setup_layer_mixer - constrain properties in allOf:if:then - some description updates - change the subject as Krzysztof suggested, and add his ACK - add braces for x in macro vop2_output_if_is_yyy(x) - clear the bits of a mask before setting it in rk3588_set_intf_mux - add more comments. - put regs dump info in vop2_data Changes in v2: - fix errors when running 'make DT_CHECKER_FLAGS=-m dt_binding_check' - split form vop driver patch - add rk3588_ prefix for functions which are rk3588 only - make some calculation as fixed value and keep calculation formula as comment - check return value for some cru calculation functions. - check return value for syscon_regmap_lookup_by_phandle - add NV20/NV30 for esmart plane Andy Yan (14): drm/rockchip: move output interface related definition to rockchip_drm_drv.h Revert "drm/rockchip: vop2: Use regcache_sync() to fix suspend/resume" drm/rockchip: vop2: set half_block_en bit in all mode drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode drm/rockchip: vop2: Add write mask for VP config done drm/rockchip: vop2: Set YUV/RGB overlay mode drm/rockchip: vop2: rename grf to sys_grf dt-bindings: soc: rockchip: add rk3588 vop/vo syscon dt-bindings: display: vop2: Add rk3588 support dt-bindings: rockchip,vop2: Add more endpoint definition drm/rockchip: vop2: Add support for rk3588 drm/rockchip: vop2: Add debugfs support dt-bindings: iommu: rockchip: Add Rockchip RK3588 arm64: dts: rockchip: Add vop on rk3588 .../display/rockchip/rockchip-vop2.yaml | 118 ++- .../bindings/iommu/rockchip,iommu.yaml | 1 + .../devicetree/bindings/soc/rockchip/grf.yaml | 2 + arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 96 +++ .../gpu/drm/rockchip/analogix_dp-rockchip.c | 1 - drivers/gpu/drm/rockchip/cdn-dp-core.c | 1 - .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 1 - drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 1 - drivers/gpu/drm/rockchip/inno_hdmi.c | 1 - drivers/gpu/drm/rockchip/rk3066_hdmi.c | 1 - drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 18 + drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 12 +- drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 749 +++++++++++++++++- drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 109 ++- drivers/gpu/drm/rockchip/rockchip_lvds.c | 1 - drivers/gpu/drm/rockchip/rockchip_rgb.c | 1 - drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 412 ++++++++++ include/dt-bindings/soc/rockchip,vop2.h | 4 + 18 files changed, 1452 insertions(+), 77 deletions(-)