Re: [PATCH 22/22] drm: rockchip: Add VOP2 driver

On Montag, 20. Dezember 2021 12:06:30 CET Sascha Hauer wrote:

From: Andy Yan andy.yan@rock-chips.com

The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568. It replaces the VOP unit found in the older Rockchip SoCs.

This driver has been derived from the downstream Rockchip Kernel and heavily modified:

• All nonstandard DRM properties have been removed
• dropped struct vop2_plane_state and pass around less data between functions
• Dropped all DRM_FORMAT_* not known on upstream
• rework register access to get rid of excessively used macros
• Drop all waiting for framesyncs

The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB board. Overlay support is tested with the modetest utility. AFBC support on the cluster windows is tested with weston-simple-dmabuf-egl on weston using the (yet to be upstreamed) panfrost driver support.

Signed-off-by: Sascha Hauer s.hauer@pengutronix.de

Hi Sascha,

quick partial review of the code in-line.

For reference, I debugged locking issues with the kernel lock debug config options and assert_spin_locked in the reg write functions, as well as some manual deduction.

drivers/gpu/drm/rockchip/Kconfig | 6 + drivers/gpu/drm/rockchip/Makefile | 1 + drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 1 + drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 7 +- drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 2 + drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 15 + drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2768 ++++++++++++++++++ drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 480 +++ drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 285 ++ 9 files changed, 3564 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h create mode 100644 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c

diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig index b9b156308460a..4ff0043f0ee70 100644 --- a/drivers/gpu/drm/rockchip/Kconfig +++ b/drivers/gpu/drm/rockchip/Kconfig @@ -28,6 +28,12 @@ config ROCKCHIP_VOP This selects support for the VOP driver. You should enable it on all older SoCs up to RK3399.

+config ROCKCHIP_VOP2

• bool "Rockchip VOP2 driver"
• help

•  This selects support for the VOP2 driver. You should enable it
  

•  on all newer SoCs beginning form RK3568.
  

config ROCKCHIP_ANALOGIX_DP bool "Rockchip specific extensions for Analogix DP driver" depends on ROCKCHIP_VOP diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile index cd6e7bb5ce9c5..29848caef5c21 100644 --- a/drivers/gpu/drm/rockchip/Makefile +++ b/drivers/gpu/drm/rockchip/Makefile @@ -7,6 +7,7 @@ rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \ rockchip_drm_gem.o rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o

+rockchipdrm-$(CONFIG_ROCKCHIP_VOP2) += rockchip_drm_vop2.o rockchip_vop2_reg.o rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o rockchip_vop_reg.o rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 64fa5fd62c01a..2bd9acb265e5a 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -474,6 +474,7 @@ static int __init rockchip_drm_init(void)

num_rockchip_sub_drivers = 0; ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_ROCKCHIP_VOP);

• ADD_ROCKCHIP_SUB_DRIVER(vop2_platform_driver, CONFIG_ROCKCHIP_VOP2); ADD_ROCKCHIP_SUB_DRIVER(rockchip_lvds_driver, CONFIG_ROCKCHIP_LVDS); ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver,

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index aa0909e8edf93..fd6994f21817e 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -18,7 +18,7 @@

#define ROCKCHIP_MAX_FB_BUFFER 3 #define ROCKCHIP_MAX_CONNECTOR 2 -#define ROCKCHIP_MAX_CRTC 2 +#define ROCKCHIP_MAX_CRTC 4

struct drm_device; struct drm_connector; @@ -31,6 +31,9 @@ struct rockchip_crtc_state { int output_bpc; int output_flags; bool enable_afbc;

• uint32_t bus_format;
• u32 bus_flags;
• int color_space;

}; #define to_rockchip_crtc_state(s) \ container_of(s, struct rockchip_crtc_state, base) @@ -65,4 +68,6 @@ extern struct platform_driver rockchip_dp_driver; extern struct platform_driver rockchip_lvds_driver; extern struct platform_driver vop_platform_driver; extern struct platform_driver rk3066_hdmi_driver; +extern struct platform_driver vop2_platform_driver;

#endif /* _ROCKCHIP_DRM_DRV_H_ */ diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 3aa37e177667e..0d2cb4f3922b8 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -134,4 +134,6 @@ void rockchip_drm_mode_config_init(struct drm_device *dev)

dev->mode_config.funcs = &rockchip_drm_mode_config_funcs; dev->mode_config.helper_private = &rockchip_mode_config_helpers;

• dev->mode_config.normalize_zpos = true;

} diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h index 857d97cdc67c6..1e364d7b50e69 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h @@ -54,9 +54,23 @@ struct vop_afbc { struct vop_reg enable; struct vop_reg win_sel; struct vop_reg format;

• struct vop_reg rb_swap;
• struct vop_reg uv_swap;
• struct vop_reg auto_gating_en;
• struct vop_reg block_split_en;
• struct vop_reg pic_vir_width;
• struct vop_reg tile_num; struct vop_reg hreg_block_split;
• struct vop_reg pic_offset; struct vop_reg pic_size;
• struct vop_reg dsp_offset;
• struct vop_reg transform_offset; struct vop_reg hdr_ptr;
• struct vop_reg half_block_en;
• struct vop_reg xmirror;
• struct vop_reg ymirror;
• struct vop_reg rotate_270;
• struct vop_reg rotate_90; struct vop_reg rstn;

};

@@ -410,4 +424,5 @@ static inline int scl_vop_cal_lb_mode(int width, bool is_yuv) }

extern const struct component_ops vop_component_ops;

#endif /* _ROCKCHIP_DRM_VOP_H */ diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c new file mode 100644 index 0000000000000..7d39ba90061d1 --- /dev/null +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c @@ -0,0 +1,2768 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/*

• • Copyright (c) 2020 Rockchip Electronics Co., Ltd.
• • Author: Andy Yan andy.yan@rock-chips.com
• */

+#include <drm/drm.h> +#include <drm/drm_atomic.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_flip_work.h> +#include <drm/drm_plane_helper.h> +#include <drm/drm_writeback.h> +#include <drm/drm_probe_helper.h> +#include <drm/drm_atomic_uapi.h>

+#include <linux/iopoll.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/iopoll.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/pm_runtime.h> +#include <linux/component.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> +#include <linux/delay.h> +#include <linux/swab.h> +#include <linux/bitfield.h> +#include <drm/drm_debugfs.h> +#include <uapi/linux/videodev2.h> +#include <drm/drm_vblank.h> +#include <dt-bindings/soc/rockchip,vop2.h>

Alphabetically sort these includes? Not sure on what the convention is in the DRM subsystem.

+#include "rockchip_drm_drv.h" +#include "rockchip_drm_gem.h" +#include "rockchip_drm_fb.h" +#include "rockchip_drm_vop2.h"

+/*

• • VOP2 architecture
• +----------+ +-------------+ +-----------+
• | Cluster | | Sel 1 from 6| | 1 from 3 |
• | window0 | | Layer0 | | RGB |
• +----------+ +-------------+ +---------------+ +-------------+ +-----------+
• +----------+ +-------------+ |N from 6 layers| | |
• | Cluster | | Sel 1 from 6| | Overlay0 +--->| Video Port0 | +-----------+
• | window1 | | Layer1 | | | | | | 1 from 3 |
• +----------+ +-------------+ +---------------+ +-------------+ | LVDS |
• +----------+ +-------------+ +-----------+
• | Esmart | | Sel 1 from 6|
• | window0 | | Layer2 | +---------------+ +-------------+ +-----------+
• +----------+ +-------------+ |N from 6 Layers| | | +--> | 1 from 3 |
• +----------+ +-------------+ --------> | Overlay1 +--->| Video Port1 | | MIPI |
• | Esmart | | Sel 1 from 6| --------> | | | | +-----------+
• | Window1 | | Layer3 | +---------------+ +-------------+
• +----------+ +-------------+ +-----------+
• +----------+ +-------------+ | 1 from 3 |
• | Smart | | Sel 1 from 6| +---------------+ +-------------+ | HDMI |
• | Window0 | | Layer4 | |N from 6 Layers| | | +-----------+
• +----------+ +-------------+ | Overlay2 +--->| Video Port2 |
• +----------+ +-------------+ | | | | +-----------+
• | Smart | | Sel 1 from 6| +---------------+ +-------------+ | 1 from 3 |
• | Window1 | | Layer5 | | eDP |
• +----------+ +-------------+ +-----------+
• */

+enum vop2_data_format {

• VOP2_FMT_ARGB8888 = 0,
• VOP2_FMT_RGB888,
• VOP2_FMT_RGB565,
• VOP2_FMT_XRGB101010,
• VOP2_FMT_YUV420SP,
• VOP2_FMT_YUV422SP,
• VOP2_FMT_YUV444SP,
• VOP2_FMT_YUYV422 = 8,
• VOP2_FMT_YUYV420,
• VOP2_FMT_VYUY422,
• VOP2_FMT_VYUY420,
• VOP2_FMT_YUV420SP_TILE_8x4 = 0x10,
• VOP2_FMT_YUV420SP_TILE_16x2,
• VOP2_FMT_YUV422SP_TILE_8x4,
• VOP2_FMT_YUV422SP_TILE_16x2,
• VOP2_FMT_YUV420SP_10,
• VOP2_FMT_YUV422SP_10,
• VOP2_FMT_YUV444SP_10,

+};

+enum vop2_afbc_format {

• VOP2_AFBC_FMT_RGB565,
• VOP2_AFBC_FMT_ARGB2101010 = 2,
• VOP2_AFBC_FMT_YUV420_10BIT,
• VOP2_AFBC_FMT_RGB888,
• VOP2_AFBC_FMT_ARGB8888,
• VOP2_AFBC_FMT_YUV420 = 9,
• VOP2_AFBC_FMT_YUV422 = 0xb,
• VOP2_AFBC_FMT_YUV422_10BIT = 0xe,
• VOP2_AFBC_FMT_INVALID = -1,

+};

+union vop2_alpha_ctrl {

• uint32_t val;
• struct {

• /* [0:1] */
  

• uint32_t color_mode:1;
  

• uint32_t alpha_mode:1;
  

• /* [2:3] */
  

• uint32_t blend_mode:2;
  

• uint32_t alpha_cal_mode:1;
  

• /* [5:7] */
  

• uint32_t factor_mode:3;
  

• /* [8:9] */
  

• uint32_t alpha_en:1;
  

• uint32_t src_dst_swap:1;
  

• uint32_t reserved:6;
  

• /* [16:23] */
  

• uint32_t glb_alpha:8;
  

• } bits;

+};

+struct vop2_alpha {

• union vop2_alpha_ctrl src_color_ctrl;
• union vop2_alpha_ctrl dst_color_ctrl;
• union vop2_alpha_ctrl src_alpha_ctrl;
• union vop2_alpha_ctrl dst_alpha_ctrl;

+};

+struct vop2_alpha_config {

• bool src_premulti_en;
• bool dst_premulti_en;
• bool src_pixel_alpha_en;
• bool dst_pixel_alpha_en;
• uint16_t src_glb_alpha_value;
• uint16_t dst_glb_alpha_value;

+};

+struct vop2_win {

• struct vop2 *vop2;
• struct drm_plane base;
• const struct vop2_win_data *data;
• struct regmap_field *reg[VOP2_WIN_MAX_REG];
• /**

• * @win_id: graphic window id, a cluster maybe split into two
  

maybe -> may be

• * graphics windows.
  

• */
  

• uint8_t win_id;
• uint32_t offset;
• uint8_t delay;
• enum drm_plane_type type;

+};

+struct vop2_video_port {

• struct drm_crtc crtc;
• struct vop2 *vop2;
• struct clk *dclk;
• uint8_t id;
• const struct vop2_video_port_regs *regs;
• const struct vop2_video_port_data *data;
• struct completion dsp_hold_completion;
• /**

• * @win_mask: Bitmask of wins attached to the video port;
  

wins -> windows

• */
  

• uint32_t win_mask;
• struct vop2_win *primary_plane;
• struct drm_pending_vblank_event *event;
• int nlayers;

+};

+struct vop2 {

• struct device *dev;
• struct drm_device *drm;
• struct vop2_video_port vps[ROCKCHIP_MAX_CRTC];
• const struct vop2_data *data;
• /* Number of win that registered as plane,

• * maybe less than the total number of hardware
  

• * win.
  

Number of windows that are registered as planes, may be, windows again

• */
  

• uint32_t registered_num_wins;
• void __iomem *regs;
• struct regmap *map;
• struct regmap *grf;
• /* physical map length of vop2 register */
• uint32_t len;
• void __iomem *lut_regs;
• /* one time only one process allowed to config the register */

only one thread at a time is allowed to configure the registers

• spinlock_t reg_lock;
• /* protects crtc enable/disable */
• struct mutex vop2_lock;
• int irq;
• /*

• * Some globle resource are shared between all
  

• * the vidoe ports(crtcs), so we need a ref counter here.
  

Some global resources, video

• */
  

• unsigned int enable_count;
• struct clk *hclk;
• struct clk *aclk;
• /* must put at the end of the struct */

must be put

• struct vop2_win win[];

+};

+static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc) +{

• return container_of(crtc, struct vop2_video_port, crtc);

+}

+static struct vop2_win *to_vop2_win(struct drm_plane *p) +{

• return container_of(p, struct vop2_win, base);

+}

+static void vop2_lock(struct vop2 *vop2) +{

• mutex_lock(&vop2->vop2_lock);

+}

+static void vop2_unlock(struct vop2 *vop2) +{

• mutex_unlock(&vop2->vop2_lock);

+}

+static void vop2_writel(struct vop2 *vop2, uint32_t offset, uint32_t v) +{

• regmap_write(vop2->map, offset, v);

+}

+static void vop2_vp_write(struct vop2_video_port *vp, uint32_t offset,

• 	  uint32_t v)
  

+{

• regmap_write(vp->vop2->map, vp->data->offset + offset, v);

+}

+static uint32_t vop2_readl(struct vop2 *vop2, uint32_t offset) +{

• uint32_t val;
• regmap_read(vop2->map, offset, &val);
• return val;

+}

+static void vop2_win_write(const struct vop2_win *win, unsigned int reg,

• 	   uint32_t v)
  

+{

• regmap_field_write(win->reg[reg], v);

+}

+static bool vop2_cluster_window(const struct vop2_win *win) +{

• return win->data->feature & WIN_FEATURE_CLUSTER;

+}

+static void vop2_cfg_done(struct vop2_video_port *vp) +{

• struct vop2 *vop2 = vp->vop2;
• uint32_t val;
• val = vop2_readl(vop2, RK3568_REG_CFG_DONE);
• val &= 0x7;

GENMASK(2, 0) instead of 0x7, should that be a constant somewhere?

• vop2_writel(vop2, RK3568_REG_CFG_DONE,

•     val | BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
  

+}

+static void vop2_win_disable(struct vop2_win *win) +{

• vop2_win_write(win, VOP2_WIN_ENABLE, 0);
• if (vop2_cluster_window(win))

• vop2_win_write(win, VOP2_WIN_CLUSTER_ENABLE, 0);
  

+}

+static enum vop2_data_format vop2_convert_format(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_XRGB8888:
• case DRM_FORMAT_ARGB8888:
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_ABGR8888:

• return VOP2_FMT_ARGB8888;
  

• case DRM_FORMAT_RGB888:
• case DRM_FORMAT_BGR888:

• return VOP2_FMT_RGB888;
  

• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_BGR565:

• return VOP2_FMT_RGB565;
  

• case DRM_FORMAT_NV12:

• return VOP2_FMT_YUV420SP;
  

• case DRM_FORMAT_NV16:

• return VOP2_FMT_YUV422SP;
  

• case DRM_FORMAT_NV24:

• return VOP2_FMT_YUV444SP;
  

• case DRM_FORMAT_YUYV:
• case DRM_FORMAT_YVYU:

• return VOP2_FMT_VYUY422;
  

• case DRM_FORMAT_VYUY:
• case DRM_FORMAT_UYVY:

• return VOP2_FMT_YUYV422;
  

• default:

• DRM_ERROR("unsupported format[%08x]\n", format);
  

• return -EINVAL;
  

• }

+}

+static enum vop2_afbc_format vop2_convert_afbc_format(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_XRGB8888:
• case DRM_FORMAT_ARGB8888:
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_ABGR8888:

• return VOP2_AFBC_FMT_ARGB8888;
  

• case DRM_FORMAT_RGB888:
• case DRM_FORMAT_BGR888:

• return VOP2_AFBC_FMT_RGB888;
  

• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_BGR565:

• return VOP2_AFBC_FMT_RGB565;
  

• case DRM_FORMAT_NV12:

• return VOP2_AFBC_FMT_YUV420;
  

• case DRM_FORMAT_NV16:

• return VOP2_AFBC_FMT_YUV422;
  

• default:

• return VOP2_AFBC_FMT_INVALID;
  

• }
• return VOP2_AFBC_FMT_INVALID;

+}

+static bool vop2_win_rb_swap(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_ABGR8888:
• case DRM_FORMAT_BGR888:
• case DRM_FORMAT_BGR565:

• return true;
  

• default:

• return false;
  

• }

+}

+static bool vop2_afbc_rb_swap(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_NV24:

• return true;
  

• default:

• return false;
  

• }

+}

+static bool vop2_afbc_uv_swap(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_NV12:
• case DRM_FORMAT_NV16:

• return true;
  

• default:

• return false;
  

• }

+}

+static bool vop2_win_uv_swap(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_NV12:
• case DRM_FORMAT_NV16:
• case DRM_FORMAT_NV24:

• return true;
  

• default:

• return false;
  

• }

+}

+static bool vop2_win_dither_up(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_BGR565:
• case DRM_FORMAT_RGB565:

• return true;
  

• default:

• return false;
  

• }

+}

+static bool vop2_output_uv_swap(uint32_t bus_format, uint32_t output_mode) +{

• /*

• * FIXME:
  

• *
  

• * There is no media type for YUV444 output,
  

• * so when out_mode is AAAA or P888, assume output is YUV444 on
  

• * yuv format.
  

• *
  

• * From H/W testing, YUV444 mode need a rb swap.
  

• */
  

• if (bus_format == MEDIA_BUS_FMT_YVYU8_1X16 ||

•    bus_format == MEDIA_BUS_FMT_VYUY8_1X16 ||
  

•    bus_format == MEDIA_BUS_FMT_YVYU8_2X8 ||
  

•    bus_format == MEDIA_BUS_FMT_VYUY8_2X8 ||
  

•    ((bus_format == MEDIA_BUS_FMT_YUV8_1X24 ||
  

•      bus_format == MEDIA_BUS_FMT_YUV10_1X30) &&
  

•     (output_mode == ROCKCHIP_OUT_MODE_AAAA ||
  

•      output_mode == ROCKCHIP_OUT_MODE_P888)))
  

• return true;
  

• else

• return false;
  

+}

+static bool is_yuv_output(uint32_t bus_format) +{

• switch (bus_format) {
• case MEDIA_BUS_FMT_YUV8_1X24:
• case MEDIA_BUS_FMT_YUV10_1X30:
• case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
• case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
• case MEDIA_BUS_FMT_YUYV8_2X8:
• case MEDIA_BUS_FMT_YVYU8_2X8:
• case MEDIA_BUS_FMT_UYVY8_2X8:
• case MEDIA_BUS_FMT_VYUY8_2X8:
• case MEDIA_BUS_FMT_YUYV8_1X16:
• case MEDIA_BUS_FMT_YVYU8_1X16:
• case MEDIA_BUS_FMT_UYVY8_1X16:
• case MEDIA_BUS_FMT_VYUY8_1X16:

• return true;
  

• default:

• return false;
  

• }

+}

+static bool rockchip_afbc(struct drm_plane *plane, u64 modifier) +{

• int i;
• if (modifier == DRM_FORMAT_MOD_LINEAR)

• return false;
  

• for (i = 0 ; i < plane->modifier_count; i++)

• if (plane->modifiers[i] == modifier)
  

• 	break;
  

• return (i < plane->modifier_count) ? true : false;

+}

+static bool rockchip_vop2_mod_supported(struct drm_plane *plane, uint32_t format, u64 modifier) +{

• struct vop2_win *win = to_vop2_win(plane);
• struct vop2 *vop2 = win->vop2;
• if (modifier == DRM_FORMAT_MOD_INVALID)

• return false;
  

• if (modifier == DRM_FORMAT_MOD_LINEAR)

• return true;
  

• if (!rockchip_afbc(plane, modifier)) {

• drm_err(vop2->drm, "Unsupported format modifier 0x%llx\n", modifier);
  

• return false;
  

• }
• return vop2_convert_afbc_format(format) >= 0;

+}

+static int vop2_afbc_half_block_enable(struct drm_plane_state *pstate) +{

• if ((pstate->rotation & DRM_MODE_ROTATE_270) || (pstate->rotation & DRM_MODE_ROTATE_90))

• return 0;
  

• else

• return 1;
  

+}

+static uint32_t vop2_afbc_transform_offset(struct drm_plane_state *pstate,

• 			   bool afbc_half_block_en)
  

+{

• struct drm_rect *src = &pstate->src;
• struct drm_framebuffer *fb = pstate->fb;
• uint32_t bpp = fb->format->cpp[0] * 8;
• uint32_t vir_width = (fb->pitches[0] << 3) / bpp;
• uint32_t width = drm_rect_width(src) >> 16;
• uint32_t height = drm_rect_height(src) >> 16;
• uint32_t act_xoffset = src->x1 >> 16;
• uint32_t act_yoffset = src->y1 >> 16;
• uint32_t align16_crop = 0;
• uint32_t align64_crop = 0;
• uint32_t height_tmp = 0;
• uint32_t transform_tmp = 0;
• uint8_t transform_xoffset = 0;
• uint8_t transform_yoffset = 0;
• uint8_t top_crop = 0;
• uint8_t top_crop_line_num = 0;
• uint8_t bottom_crop_line_num = 0;
• /* 16 pixel align */
• if (height & 0xf)

• align16_crop = 16 - (height & 0xf);
  

• height_tmp = height + align16_crop;
• /* 64 pixel align */
• if (height_tmp & 0x3f)

• align64_crop = 64 - (height_tmp & 0x3f);
  

• top_crop_line_num = top_crop << 2;
• if (top_crop == 0)

• bottom_crop_line_num = align16_crop + align64_crop;
  

• else if (top_crop == 1)

• bottom_crop_line_num = align16_crop + align64_crop + 12;
  

• else if (top_crop == 2)

• bottom_crop_line_num = align16_crop + align64_crop + 8;
  

top_crop == 0 is always taken from what I can gather, rest is dead code. Is this intentional? It doesn't seem intentional.

• switch (pstate->rotation &

• (DRM_MODE_REFLECT_X |
  

•  DRM_MODE_REFLECT_Y |
  

•  DRM_MODE_ROTATE_90 |
  

•  DRM_MODE_ROTATE_270)) {
  

• case DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y:

• transform_tmp = act_xoffset + width;
  

• transform_xoffset = 16 - (transform_tmp & 0xf);
  

• transform_tmp = bottom_crop_line_num - act_yoffset;
  

• if (afbc_half_block_en)
  

• 	transform_yoffset = transform_tmp & 0x7;
  

• else
  

• 	transform_yoffset = (transform_tmp & 0xf);
  

• break;
  

• case DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_90:

• transform_tmp = bottom_crop_line_num - act_yoffset;
  

• transform_xoffset = transform_tmp & 0xf;
  

• transform_tmp = vir_width - width - act_xoffset;
  

• transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• case DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_270:

• transform_tmp = top_crop_line_num + act_yoffset;
  

• transform_xoffset = transform_tmp & 0xf;
  

• transform_tmp = act_xoffset;
  

• transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• case DRM_MODE_REFLECT_X:

• transform_tmp = act_xoffset + width;
  

• transform_xoffset = 16 - (transform_tmp & 0xf);
  

• transform_tmp = top_crop_line_num + act_yoffset;
  

• if (afbc_half_block_en)
  

• 	transform_yoffset = transform_tmp & 0x7;
  

• else
  

• 	transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• case DRM_MODE_REFLECT_Y:

• transform_tmp = act_xoffset;
  

• transform_xoffset = transform_tmp & 0xf;
  

• transform_tmp = bottom_crop_line_num - act_yoffset;
  

• if (afbc_half_block_en)
  

• 	transform_yoffset = transform_tmp & 0x7;
  

• else
  

• 	transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• case DRM_MODE_ROTATE_90:

• transform_tmp = bottom_crop_line_num - act_yoffset;
  

• transform_xoffset = transform_tmp & 0xf;
  

• transform_tmp = act_xoffset;
  

• transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• case DRM_MODE_ROTATE_270:

• transform_tmp = top_crop_line_num + act_yoffset;
  

• transform_xoffset = transform_tmp & 0xf;
  

• transform_tmp = vir_width - width - act_xoffset;
  

• transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• case 0:

• transform_tmp = act_xoffset;
  

• transform_xoffset = transform_tmp & 0xf;
  

• transform_tmp = top_crop_line_num + act_yoffset;
  

• if (afbc_half_block_en)
  

• 	transform_yoffset = transform_tmp & 0x7;
  

• else
  

• 	transform_yoffset = transform_tmp & 0xf;
  

• break;
  

• }
• return (transform_xoffset & 0xf) | ((transform_yoffset & 0xf) << 16);

+}

0xf, 0x3f, 0x7 might be more clear as GENMASK. if (afbc_half_block_en) branches can be moved out of cases and assign a transform mask variable instead.

+/*

• • A Cluster window has 2048 x 16 line buffer, which can
• • works at 2048 x 16(Full) or 4096 x 8 (Half) mode.
• • for Cluster_lb_mode register:
• • 0: half mode, for plane input width range 2048 ~ 4096
• • 1: half mode, for cluster work at 2 * 2048 plane mode
• • 2: half mode, for rotate_90/270 mode
• */

+static int vop2_get_cluster_lb_mode(struct vop2_win *win, struct drm_plane_state *pstate) +{

• if ((pstate->rotation & DRM_MODE_ROTATE_270) || (pstate->rotation & DRM_MODE_ROTATE_90))

• return 2;
  

• else

• return 0;
  

+}

What about 1?

+/*

• • bli_sd_factor = (src - 1) / (dst - 1) << 12;
• • avg_sd_factor:
• • bli_su_factor:
• • bic_su_factor:
• • = (src - 1) / (dst - 1) << 16;
• • gt2 enable: dst get one line from two line of the src
• • gt4 enable: dst get one line from four line of the src.
• */

+static uint16_t vop2_scale_factor(enum scale_mode mode,

• 		  int32_t filter_mode,
  

• 		  uint32_t src, uint32_t dst)
  

+{

• uint32_t fac;
• int i;
• if (mode == SCALE_NONE)

• return 0;
  

• /*

• * A workaround to avoid zero div.
  

• */
  

• if (dst == 1 || src == 1) {

• dst++;
  

• src++;
  

• }
• if ((mode == SCALE_DOWN) && (filter_mode == VOP2_SCALE_DOWN_BIL)) {

• fac = ((src - 1) << 12) / (dst - 1);
  

• for (i = 0; i < 100; i++) {
  

• 	if (fac * (dst - 1) >> 12 < (src - 1))
  

• 		break;
  

• 	fac -= 1;
  

• 	DRM_DEBUG("down fac cali: src:%d, dst:%d, fac:0x%x\n", src, dst, fac);
  

• }
  

• } else {

• fac = ((src - 1) << 16) / (dst - 1);
  

• for (i = 0; i < 100; i++) {
  

• 	if (fac * (dst - 1) >> 16 < (src - 1))
  

• 		break;
  

• 	fac -= 1;
  

• 	DRM_DEBUG("up fac cali:  src:%d, dst:%d, fac:0x%x\n", src, dst, fac);
  

• }
  

• }
• return fac;

+}

src = 0 or dst = 0 causes an uint underflow here.

+static void vop2_setup_scale(struct vop2 *vop2, const struct vop2_win *win,

• 	     uint32_t src_w, uint32_t src_h, uint32_t dst_w,
  

• 	     uint32_t dst_h, uint32_t pixel_format)
  

+{

• const struct drm_format_info *info;
• uint16_t cbcr_src_w;
• uint16_t cbcr_src_h;
• uint16_t yrgb_hor_scl_mode, yrgb_ver_scl_mode;
• uint16_t cbcr_hor_scl_mode, cbcr_ver_scl_mode;
• uint16_t hscl_filter_mode, vscl_filter_mode;
• uint8_t gt2 = 0;
• uint8_t gt4 = 0;
• uint32_t val;
• info = drm_format_info(pixel_format);
• cbcr_src_w = src_w / info->hsub;
• cbcr_src_h = src_h / info->vsub;
• if (src_h >= (4 * dst_h)) {

• gt4 = 1;
  

• src_h >>= 2;
  

• } else if (src_h >= (2 * dst_h)) {

• gt2 = 1;
  

• src_h >>= 1;
  

• }
• yrgb_hor_scl_mode = scl_get_scl_mode(src_w, dst_w);
• yrgb_ver_scl_mode = scl_get_scl_mode(src_h, dst_h);
• if (yrgb_hor_scl_mode == SCALE_UP)

• hscl_filter_mode = VOP2_SCALE_UP_BIC;
  

• else

• hscl_filter_mode = VOP2_SCALE_DOWN_BIL;
  

• if (yrgb_ver_scl_mode == SCALE_UP)

• vscl_filter_mode = VOP2_SCALE_UP_BIL;
  

• else

• vscl_filter_mode = VOP2_SCALE_DOWN_BIL;
  

• /*

• * RK3568 VOP Esmart/Smart dsp_w should be even pixel
  

• * at scale down mode
  

• */
  

• if (!(win->data->feature & WIN_FEATURE_AFBDC)) {

• if ((yrgb_hor_scl_mode == SCALE_DOWN) && (dst_w & 0x1)) {
  

• 	drm_dbg(vop2->drm, "%s dst_w[%d] should align as 2 pixel\n",
  

• 		win->data->name, dst_w);
  

• 	dst_w += 1;
  

• }
  

• }
• val = vop2_scale_factor(yrgb_hor_scl_mode, hscl_filter_mode,

• 		src_w, dst_w);
  

• vop2_win_write(win, VOP2_WIN_SCALE_YRGB_X, val);
• val = vop2_scale_factor(yrgb_ver_scl_mode, vscl_filter_mode,

• 		src_h, dst_h);
  

• vop2_win_write(win, VOP2_WIN_SCALE_YRGB_Y, val);
• vop2_win_write(win, VOP2_WIN_VSD_YRGB_GT4, gt4);
• vop2_win_write(win, VOP2_WIN_VSD_YRGB_GT2, gt2);
• vop2_win_write(win, VOP2_WIN_YRGB_HOR_SCL_MODE, yrgb_hor_scl_mode);
• vop2_win_write(win, VOP2_WIN_YRGB_VER_SCL_MODE, yrgb_ver_scl_mode);
• if (vop2_cluster_window(win))

• return;
  

• vop2_win_write(win, VOP2_WIN_YRGB_HSCL_FILTER_MODE, hscl_filter_mode);
• vop2_win_write(win, VOP2_WIN_YRGB_VSCL_FILTER_MODE, vscl_filter_mode);
• if (info->is_yuv) {

• gt4 = gt2 = 0;
  

• if (cbcr_src_h >= (4 * dst_h))
  

• 	gt4 = 1;
  

• else if (cbcr_src_h >= (2 * dst_h))
  

• 	gt2 = 1;
  

• if (gt4)
  

• 	cbcr_src_h >>= 2;
  

• else if (gt2)
  

• 	cbcr_src_h >>= 1;
  

• cbcr_hor_scl_mode = scl_get_scl_mode(cbcr_src_w, dst_w);
  

• cbcr_ver_scl_mode = scl_get_scl_mode(cbcr_src_h, dst_h);
  

• val = vop2_scale_factor(cbcr_hor_scl_mode, hscl_filter_mode,
  

• 			cbcr_src_w, dst_w);
  

• vop2_win_write(win, VOP2_WIN_SCALE_CBCR_X, val);
  

• val = vop2_scale_factor(cbcr_ver_scl_mode, vscl_filter_mode,
  

• 			cbcr_src_h, dst_h);
  

• vop2_win_write(win, VOP2_WIN_SCALE_CBCR_Y, val);
  

• vop2_win_write(win, VOP2_WIN_VSD_CBCR_GT4, gt4);
  

• vop2_win_write(win, VOP2_WIN_VSD_CBCR_GT2, gt2);
  

• vop2_win_write(win, VOP2_WIN_CBCR_HOR_SCL_MODE, cbcr_hor_scl_mode);
  

• vop2_win_write(win, VOP2_WIN_CBCR_VER_SCL_MODE, cbcr_ver_scl_mode);
  

• vop2_win_write(win, VOP2_WIN_CBCR_HSCL_FILTER_MODE, hscl_filter_mode);
  

• vop2_win_write(win, VOP2_WIN_CBCR_VSCL_FILTER_MODE, vscl_filter_mode);
  

• }

+}

Double-check that we do have the reg spinlock here, my lock debugging died earlier due to a different lock bug

+static int vop2_convert_csc_mode(int csc_mode) +{

• switch (csc_mode) {
• case V4L2_COLORSPACE_SMPTE170M:
• case V4L2_COLORSPACE_470_SYSTEM_M:
• case V4L2_COLORSPACE_470_SYSTEM_BG:

• return CSC_BT601L;
  

• case V4L2_COLORSPACE_REC709:
• case V4L2_COLORSPACE_SMPTE240M:
• case V4L2_COLORSPACE_DEFAULT:

• return CSC_BT709L;
  

• case V4L2_COLORSPACE_JPEG:

• return CSC_BT601F;
  

• case V4L2_COLORSPACE_BT2020:

• return CSC_BT2020;
  

• default:

• return CSC_BT709L;
  

• }

+}

+/*

• • colorspace path:

• •  Input        Win csc                     Output
    

• • 1. YUV(2020) --> Y2R->2020To709->R2Y --> YUV_OUTPUT(601/709)
• • RGB --> R2Y __/
• • 2. YUV(2020) --> bypasss --> YUV_OUTPUT(2020)
• • RGB --> 709To2020->R2Y __/
• • 3. YUV(2020) --> Y2R->2020To709 --> RGB_OUTPUT(709)
• • RGB --> R2Y __/
• • 4. YUV(601/709)-> Y2R->709To2020->R2Y --> YUV_OUTPUT(2020)
• • RGB --> 709To2020->R2Y __/
• • 5. YUV(601/709)-> bypass --> YUV_OUTPUT(709)
• • RGB --> R2Y __/
• • 6. YUV(601/709)-> bypass --> YUV_OUTPUT(601)
• • RGB --> R2Y(601) __/
• • 7. YUV --> Y2R(709) --> RGB_OUTPUT(709)
• • RGB --> bypass __/
• • 8. RGB --> 709To2020->R2Y --> YUV_OUTPUT(2020)
• • 9. RGB --> R2Y(709) --> YUV_OUTPUT(709)
• • 10. RGB --> R2Y(601) --> YUV_OUTPUT(601)
• • 11. RGB --> bypass --> RGB_OUTPUT(709)
• */

+static void vop2_setup_csc_mode(struct vop2_video_port *vp,

• 		struct vop2_win *win,
  

• 		struct drm_plane_state *pstate)
  

+{

• struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(vp->crtc.state);
• int is_input_yuv = pstate->fb->format->is_yuv;
• int is_output_yuv = is_yuv_output(vcstate->bus_format);
• int input_csc = V4L2_COLORSPACE_DEFAULT;
• int output_csc = vcstate->color_space;
• bool r2y_en, y2r_en;
• int csc_mode;
• if (is_input_yuv && !is_output_yuv) {

• y2r_en = 1;
  

• r2y_en = 0;
  

They're bools, use true/false.

• csc_mode = vop2_convert_csc_mode(input_csc);
  

• } else if (!is_input_yuv && is_output_yuv) {

• y2r_en = 0;
  

• r2y_en = 1;
  

ditto

• csc_mode = vop2_convert_csc_mode(output_csc);
  

• } else {

• y2r_en = 0;
  

• r2y_en = 0;
  

ditto

• csc_mode = 0;
  

• }
• vop2_win_write(win, VOP2_WIN_Y2R_EN, y2r_en);
• vop2_win_write(win, VOP2_WIN_R2Y_EN, r2y_en);
• vop2_win_write(win, VOP2_WIN_CSC_MODE, csc_mode);

+}

+static void vop2_crtc_enable_irq(struct vop2_video_port *vp, uint32_t irq) +{

• struct vop2 *vop2 = vp->vop2;
• vop2_writel(vop2, RK3568_VP_INT_CLR(vp->id), irq << 16 | irq);
• vop2_writel(vop2, RK3568_VP_INT_EN(vp->id), irq << 16 | irq);

+}

+static void vop2_crtc_disable_irq(struct vop2_video_port *vp, uint32_t irq) +{

• struct vop2 *vop2 = vp->vop2;
• vop2_writel(vop2, RK3568_VP_INT_EN(vp->id), irq << 16);

+}

+static int vop2_core_clks_prepare_enable(struct vop2 *vop2) +{

• int ret;
• ret = clk_prepare_enable(vop2->hclk);
• if (ret < 0) {

• drm_err(vop2->drm, "failed to enable hclk - %d\n", ret);
  

• return ret;
  

• }
• ret = clk_prepare_enable(vop2->aclk);
• if (ret < 0) {

• drm_err(vop2->drm, "failed to enable aclk - %d\n", ret);
  

• goto err;
  

• }
• return 0;

+err:

• clk_disable_unprepare(vop2->hclk);
• return ret;

+}

+static void vop2_enable(struct vop2 *vop2) +{

• int ret;
• uint32_t v;
• ret = pm_runtime_get_sync(vop2->dev);
• if (ret < 0) {

• drm_err(vop2->drm, "failed to get pm runtime: %d\n", ret);
  

• return;
  

• }
• ret = vop2_core_clks_prepare_enable(vop2);
• if (ret) {

• pm_runtime_put_sync(vop2->dev);
  

• return;
  

• }
• if (vop2->data->soc_id == 3566)

• vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
  

• vop2_writel(vop2, RK3568_REG_CFG_DONE, RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
• /*

• * Disable auto gating, this is a workaround to
  

• * avoid display image shift when a window enabled.
  

• */
  

• v = vop2_readl(vop2, RK3568_SYS_AUTO_GATING_CTRL);
• v &= ~RK3568_SYS_AUTO_GATING_CTRL__AUTO_GATING_EN;
• vop2_writel(vop2, RK3568_SYS_AUTO_GATING_CTRL, v);
• vop2_writel(vop2, RK3568_SYS0_INT_CLR,

•     VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
  

• vop2_writel(vop2, RK3568_SYS0_INT_EN,

•     VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
  

• vop2_writel(vop2, RK3568_SYS1_INT_CLR,

•     VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
  

• vop2_writel(vop2, RK3568_SYS1_INT_EN,

•     VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
  

+}

Does reg writes but doesn't have the spinlock.

[...] +static void vop2_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) +{

• struct vop2_video_port *vp = to_vop2_video_port(crtc);
• struct vop2 *vop2 = vp->vop2;
• const struct vop2_data *vop2_data = vop2->data;
• const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
• struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
• struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
• struct drm_display_mode *mode = &crtc->state->adjusted_mode;
• unsigned long clock = mode->crtc_clock * 1000;
• uint16_t hsync_len = mode->crtc_hsync_end - mode->crtc_hsync_start;
• uint16_t hdisplay = mode->crtc_hdisplay;
• uint16_t htotal = mode->crtc_htotal;
• uint16_t hact_st = mode->crtc_htotal - mode->crtc_hsync_start;
• uint16_t hact_end = hact_st + hdisplay;
• uint16_t vdisplay = mode->crtc_vdisplay;
• uint16_t vtotal = mode->crtc_vtotal;
• uint16_t vsync_len = mode->crtc_vsync_end - mode->crtc_vsync_start;
• uint16_t vact_st = mode->crtc_vtotal - mode->crtc_vsync_start;
• uint16_t vact_end = vact_st + vdisplay;
• uint8_t out_mode;
• uint32_t dsp_ctrl = 0;
• int act_end;
• uint32_t val, polflags;
• int ret;
• struct drm_encoder *encoder;
• drm_dbg(vop2->drm, "Update mode to %dx%d%s%d, type: %d for vp%d\n",

• hdisplay, vdisplay, mode->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "p",
  

• drm_mode_vrefresh(mode), vcstate->output_type, vp->id);
  

• vop2_lock(vop2);
• ret = clk_prepare_enable(vp->dclk);
• if (ret < 0) {

• drm_err(vop2->drm, "failed to enable dclk for video port%d - %d\n",
  

• 	      vp->id, ret);
  

• return;
  

vop2_lock is never unlocked in this branch

• }
• if (!vop2->enable_count)

• vop2_enable(vop2);
  

• vop2->enable_count++;
• vop2_crtc_enable_irq(vp, VP_INT_POST_BUF_EMPTY);
• polflags = 0;
• if (vcstate->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)

• polflags |= POLFLAG_DCLK_INV;
  

• if (mode->flags & DRM_MODE_FLAG_PHSYNC)

• polflags |= BIT(HSYNC_POSITIVE);
  

• if (mode->flags & DRM_MODE_FLAG_PVSYNC)

• polflags |= BIT(VSYNC_POSITIVE);
  

• drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {

• struct device_node *node, *parent;
  

• parent = of_get_parent(encoder->port);
  

• for_each_endpoint_of_node(parent, node) {
  

• 	struct device_node *crtc_port = of_graph_get_remote_port(node);
  

• 	struct device_node *epn;
  

• 	struct of_endpoint endpoint;
  

• 	if (crtc->port != crtc_port) {
  

• 		of_node_put(crtc_port);
  

• 		continue;
  

• 	}
  

• 	of_node_put(crtc_port);
  

• 	epn = of_graph_get_remote_endpoint(node);
  

• 	of_graph_parse_endpoint(epn, &endpoint);
  

• 	of_node_put(epn);
  

• 	drm_dbg(vop2->drm, "vp%d is connected to %s, id %d\n",
  

• 			   vp->id, encoder->name, endpoint.id);
  

• 	rk3568_set_intf_mux(vp, endpoint.id, polflags);
  

• }
  

• of_node_put(parent);
  

• }
• if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&

•     !(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
  

• out_mode = ROCKCHIP_OUT_MODE_P888;
  

• else

• out_mode = vcstate->output_mode;
  

• dsp_ctrl |= FIELD_PREP(RK3568_VP_DSP_CTRL__OUT_MODE, out_mode);
• if (vop2_output_uv_swap(vcstate->bus_format, vcstate->output_mode))

• dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_RB_SWAP;
  

• if (is_yuv_output(vcstate->bus_format))

• dsp_ctrl |= RK3568_VP_DSP_CTRL__POST_DSP_OUT_R2Y;
  

• vop2_dither_setup(crtc, &dsp_ctrl);
• vop2_vp_write(vp, RK3568_VP_DSP_HTOTAL_HS_END, (htotal << 16) | hsync_len);
• val = hact_st << 16;
• val |= hact_end;
• vop2_vp_write(vp, RK3568_VP_DSP_HACT_ST_END, val);
• val = vact_st << 16;
• val |= vact_end;
• vop2_vp_write(vp, RK3568_VP_DSP_VACT_ST_END, val);
• if (mode->flags & DRM_MODE_FLAG_INTERLACE) {

• uint16_t vact_st_f1 = vtotal + vact_st + 1;
  

• uint16_t vact_end_f1 = vact_st_f1 + vdisplay;
  

• val = vact_st_f1 << 16 | vact_end_f1;
  

• vop2_vp_write(vp, RK3568_VP_DSP_VACT_ST_END_F1, val);
  

• val = vtotal << 16 | (vtotal + vsync_len);
  

• vop2_vp_write(vp, RK3568_VP_DSP_VS_ST_END_F1, val);
  

• dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_INTERLACE;
  

• dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_FILED_POL;
  

• dsp_ctrl |= RK3568_VP_DSP_CTRL__P2I_EN;
  

• vtotal += vtotal + 1;
  

• act_end = vact_end_f1;
  

• } else {

• act_end = vact_end;
  

• }
• vop2_writel(vop2, RK3568_VP_LINE_FLAG(vp->id),

•     (act_end - us_to_vertical_line(mode, 0)) << 16 | act_end);
  

• vop2_vp_write(vp, RK3568_VP_DSP_VTOTAL_VS_END, vtotal << 16 | vsync_len);
• if (mode->flags & DRM_MODE_FLAG_DBLCLK) {

• dsp_ctrl |= RK3568_VP_DSP_CTRL__CORE_DCLK_DIV;
  

• clock *= 2;
  

• }
• vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
• clk_set_rate(vp->dclk, clock);
• vop2_post_config(crtc);
• vop2_cfg_done(vp);
• vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
• drm_crtc_vblank_on(crtc);
• vop2_unlock(vop2);

+}

Whole function does reg writes without the spinlock, caught by assert_spin_locked.

[...] +static irqreturn_t vop2_isr(int irq, void *data) +{

• struct vop2 *vop2 = data;
• const struct vop2_data *vop2_data = vop2->data;
• uint32_t axi_irqs[VOP2_SYS_AXI_BUS_NUM];
• int ret = IRQ_NONE;
• int i;
• /*

• * The irq is shared with the iommu. If the runtime-pm state of the
  

• * vop2-device is disabled the irq has to be targeted at the iommu.
  

• */
  

• if (!pm_runtime_get_if_in_use(vop2->dev))

• return IRQ_NONE;
  

• for (i = 0; i < vop2_data->nr_vps; i++) {

• struct vop2_video_port *vp = &vop2->vps[i];
  

• struct drm_crtc *crtc = &vp->crtc;
  

• uint32_t irqs;
  

• irqs = vop2_readl(vop2, RK3568_VP_INT_STATUS(vp->id));
  

• vop2_writel(vop2, RK3568_VP_INT_CLR(vp->id), irqs << 16 | irqs);
  

• if (irqs & VP_INT_DSP_HOLD_VALID) {
  

• 	complete(&vp->dsp_hold_completion);
  

• 	ret = IRQ_HANDLED;
  

• }
  

• if (irqs & VP_INT_FS_FIELD) {
  

• 	unsigned long flags;
  

• 	drm_crtc_handle_vblank(crtc);
  

• 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
  

• 	if (vp->event) {
  

• 		uint32_t val = vop2_readl(vop2, RK3568_REG_CFG_DONE);
  

• 		if (!(val & BIT(vp->id))) {
  

• 			drm_crtc_send_vblank_event(crtc, vp->event);
  

• 			vp->event = NULL;
  

• 			drm_crtc_vblank_put(crtc);
  

• 		}
  

• 	}
  

• 	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
  

• 	ret = IRQ_HANDLED;
  

• }
  

• if (irqs & VP_INT_POST_BUF_EMPTY) {
  

• 	drm_err_ratelimited(vop2->drm,
  

• 			    "POST_BUF_EMPTY irq err at vp%d\n",
  

• 			    vp->id);
  

• 	ret = IRQ_HANDLED;
  

• }
  

• }
• axi_irqs[0] = vop2_readl(vop2, RK3568_SYS0_INT_STATUS);
• vop2_writel(vop2, RK3568_SYS0_INT_CLR, axi_irqs[0] << 16 | axi_irqs[0]);
• axi_irqs[1] = vop2_readl(vop2, RK3568_SYS1_INT_STATUS);
• vop2_writel(vop2, RK3568_SYS1_INT_CLR, axi_irqs[1] << 16 | axi_irqs[1]);
• for (i = 0; i < ARRAY_SIZE(axi_irqs); i++) {

• if (axi_irqs[i] & VOP2_INT_BUS_ERRPR) {
  

• 	drm_err_ratelimited(vop2->drm, "BUS_ERROR irq err\n");
  

• 	ret = IRQ_HANDLED;
  

• }
  

• }
• pm_runtime_put(vop2->dev);
• return ret;

+}

Does reg writes, does the ISR need the reg spinlock here? I'm not sure.

In general there appear to be a lot of issues with the register lock, so either I'm misunderstanding what it's supposed to protect or the code is very thread unsafe.

Regards, Nicolas Frattaroli