On Montag, 20. Dezember 2021 12:06:30 CET Sascha Hauer wrote:
> From: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> ---
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@xxxxxxxxxxxxxx>
> + */
> +#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
