| Message ID | 20240220-rk3568-vicap-v6-0-d2f5fbee1551@collabora.com |
|---|---|
| Headers | show |
| Series | media: rockchip: add a driver for the rockchip camera interface | expand |
Hi Laurent, On 5/2/25 15:31, Laurent Pinchart wrote: > Hi Michael, > > Thank you for the patch. > > On Wed, Apr 30, 2025 at 11:15:56AM +0200, Michael Riesch via B4 Relay wrote: >> From: Michael Riesch <michael.riesch@wolfvision.net> >> >> The Rockchip RK3568 MIPI CSI-2 Receiver is a CSI-2 bridge with one >> input port and one output port. It receives the data with the help >> of an external MIPI PHY (C-PHY or D-PHY) and passes it to the >> Rockchip RK3568 Video Capture (VICAP) block. >> >> Add a V4L2 subdevice driver for this unit. >> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> >> Signed-off-by: Michael Riesch <michael.riesch@collabora.com> >> --- >> drivers/media/platform/rockchip/rkcif/Makefile | 3 + >> .../rockchip/rkcif/rkcif-mipi-csi-receiver.c | 731 +++++++++++++++++++++ >> 2 files changed, 734 insertions(+) >> >> diff --git a/drivers/media/platform/rockchip/rkcif/Makefile b/drivers/media/platform/rockchip/rkcif/Makefile >> index 818424972c7b..a5c18a45c213 100644 >> --- a/drivers/media/platform/rockchip/rkcif/Makefile >> +++ b/drivers/media/platform/rockchip/rkcif/Makefile >> @@ -5,3 +5,6 @@ rockchip-cif-objs += rkcif-dev.o \ >> rkcif-capture-mipi.o \ >> rkcif-interface.o \ >> rkcif-stream.o >> + >> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip-mipi-csi.o >> +rockchip-mipi-csi-objs += rkcif-mipi-csi-receiver.o >> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c >> new file mode 100644 >> index 000000000000..81489f70490f >> --- /dev/null >> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c >> @@ -0,0 +1,731 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Rockchip MIPI CSI-2 Receiver Driver >> + * >> + * Copyright (C) 2019 Rockchip Electronics Co., Ltd. >> + * Copyright (C) 2025 Michael Riesch <michael.riesch@wolfvision.net> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_graph.h> >> +#include <linux/of_platform.h> >> +#include <linux/phy/phy.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/reset.h> >> + >> +#include <media/mipi-csi2.h> >> +#include <media/v4l2-ctrls.h> >> +#include <media/v4l2-fwnode.h> >> +#include <media/v4l2-subdev.h> >> + >> +#define CSI2HOST_N_LANES 0x04 >> +#define CSI2HOST_CSI2_RESETN 0x10 >> +#define CSI2HOST_PHY_STATE 0x14 >> +#define CSI2HOST_ERR1 0x20 >> +#define CSI2HOST_ERR2 0x24 >> +#define CSI2HOST_MSK1 0x28 >> +#define CSI2HOST_MSK2 0x2c >> +#define CSI2HOST_CONTROL 0x40 > > I'm trying to get to the bottom of the CSI-2 RX integration questions > for the RK3568. Some of the registers here seem to the CSI2RX_1C00 block > as documented starting on page 1059 of the RK3568 TRM (revision 1.1), > but they're not an exact match. The control register, in particular, > doesn't match at all. Where is this CSI-2 receiver documented in the TRM > ? That would be in Chapter 27 of the RK3568 TRM: "MIPI CSI HOST" The registers are at 0xfdfb0000 "CSI_RX_CTRL1", cf. Chapter 1 "System Overview". Naturally, this is quite confusing, as there is a "CSI2RX" whose registers are embedded in the ISP register map. "MIPI CSI HOST" and "CSI2RX" seem to be different IP cores but perform the same job in principle. CSI2RX seems to have additional features related to writing raw data into memory and to HDR, though. Hope that helps! Michael > >> + >> +#define SW_CPHY_EN(x) ((x) << 0) >> +#define SW_DSI_EN(x) ((x) << 4) >> +#define SW_DATATYPE_FS(x) ((x) << 8) >> +#define SW_DATATYPE_FE(x) ((x) << 14) >> +#define SW_DATATYPE_LS(x) ((x) << 20) >> +#define SW_DATATYPE_LE(x) ((x) << 26) >> + >> +#define RKCIF_CSI_CLKS_MAX 1 >> + >> +enum { >> + RKCIF_CSI_PAD_SINK, >> + RKCIF_CSI_PAD_SRC, >> + RKCIF_CSI_PAD_MAX, >> +}; >> + >> +struct rkcif_csi_format { >> + u32 code; >> + u8 depth; >> + u8 csi_dt; >> +}; >> + >> +struct rkcif_csi_device { >> + struct device *dev; >> + >> + void __iomem *base_addr; >> + struct clk_bulk_data *clks; >> + unsigned int clks_num; >> + struct phy *phy; >> + struct reset_control *reset; >> + >> + const struct rkcif_csi_format *formats; >> + unsigned int formats_num; >> + >> + struct media_pad pads[RKCIF_CSI_PAD_MAX]; >> + struct v4l2_async_notifier notifier; >> + struct v4l2_fwnode_endpoint vep; >> + struct v4l2_subdev sd; >> + >> + struct v4l2_subdev *source_sd; >> + u32 source_pad; >> +}; >> + >> +static const struct v4l2_mbus_framefmt default_format = { >> + .width = 3840, >> + .height = 2160, >> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, >> + .field = V4L2_FIELD_NONE, >> + .colorspace = V4L2_COLORSPACE_RAW, >> + .ycbcr_enc = V4L2_YCBCR_ENC_601, >> + .quantization = V4L2_QUANTIZATION_FULL_RANGE, >> + .xfer_func = V4L2_XFER_FUNC_NONE, >> +}; >> + >> +static const struct rkcif_csi_format formats[] = { >> + /* YUV formats */ >> + { >> + .code = MEDIA_BUS_FMT_YUYV8_1X16, >> + .depth = 16, >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_UYVY8_1X16, >> + .depth = 16, >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_YVYU8_1X16, >> + .depth = 16, >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_VYUY8_1X16, >> + .depth = 16, >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >> + }, >> + /* RGB formats */ >> + { >> + .code = MEDIA_BUS_FMT_RGB888_1X24, >> + .depth = 24, >> + .csi_dt = MIPI_CSI2_DT_RGB888, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_BGR888_1X24, >> + .depth = 24, >> + .csi_dt = MIPI_CSI2_DT_RGB888, >> + }, >> + /* Bayer formats */ >> + { >> + .code = MEDIA_BUS_FMT_SBGGR8_1X8, >> + .depth = 8, >> + .csi_dt = MIPI_CSI2_DT_RAW8, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SGBRG8_1X8, >> + .depth = 8, >> + .csi_dt = MIPI_CSI2_DT_RAW8, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SGRBG8_1X8, >> + .depth = 8, >> + .csi_dt = MIPI_CSI2_DT_RAW8, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SRGGB8_1X8, >> + .depth = 8, >> + .csi_dt = MIPI_CSI2_DT_RAW8, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SBGGR10_1X10, >> + .depth = 10, >> + .csi_dt = MIPI_CSI2_DT_RAW10, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SGBRG10_1X10, >> + .depth = 10, >> + .csi_dt = MIPI_CSI2_DT_RAW10, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SGRBG10_1X10, >> + .depth = 10, >> + .csi_dt = MIPI_CSI2_DT_RAW10, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, >> + .depth = 10, >> + .csi_dt = MIPI_CSI2_DT_RAW10, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SBGGR12_1X12, >> + .depth = 12, >> + .csi_dt = MIPI_CSI2_DT_RAW12, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SGBRG12_1X12, >> + .depth = 12, >> + .csi_dt = MIPI_CSI2_DT_RAW12, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SGRBG12_1X12, >> + .depth = 12, >> + .csi_dt = MIPI_CSI2_DT_RAW12, >> + }, >> + { >> + .code = MEDIA_BUS_FMT_SRGGB12_1X12, >> + .depth = 12, >> + .csi_dt = MIPI_CSI2_DT_RAW12, >> + }, >> +}; >> + >> +static inline struct rkcif_csi_device *to_rkcif_csi(struct v4l2_subdev *sd) >> +{ >> + return container_of(sd, struct rkcif_csi_device, sd); >> +} >> + >> +static inline __maybe_unused void >> +rkcif_csi_write(struct rkcif_csi_device *csi_dev, unsigned int addr, u32 val) >> +{ >> + writel(val, csi_dev->base_addr + addr); >> +} >> + >> +static inline __maybe_unused u32 >> +rkcif_csi_read(struct rkcif_csi_device *csi_dev, unsigned int addr) >> +{ >> + return readl(csi_dev->base_addr + addr); >> +} >> + >> +static const struct rkcif_csi_format * >> +rkcif_csi_find_format(struct rkcif_csi_device *csi_dev, u32 mbus_code) >> +{ >> + const struct rkcif_csi_format *format; >> + >> + WARN_ON(csi_dev->formats_num == 0); >> + >> + for (int i = 0; i < csi_dev->formats_num; i++) { >> + format = &csi_dev->formats[i]; >> + if (format->code == mbus_code) >> + return format; >> + } >> + >> + return NULL; >> +} >> + >> +static int rkcif_csi_start(struct rkcif_csi_device *csi_dev) >> +{ >> + enum v4l2_mbus_type bus_type = csi_dev->vep.bus_type; >> + union phy_configure_opts opts; >> + s64 link_freq; >> + u32 lanes = csi_dev->vep.bus.mipi_csi2.num_data_lanes; >> + u32 control = 0; >> + >> + if (lanes < 1 || lanes > 4) >> + return -EINVAL; >> + >> + /* set mult and div to 0, thus completely rely on V4L2_CID_LINK_FREQ */ >> + link_freq = v4l2_get_link_freq(csi_dev->source_sd->ctrl_handler, 0, 0); >> + if (link_freq <= 0) >> + return -EINVAL; >> + >> + if (bus_type == V4L2_MBUS_CSI2_DPHY) { >> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; >> + >> + phy_mipi_dphy_get_default_config_for_hsclk(link_freq * 2, lanes, >> + cfg); >> + phy_set_mode(csi_dev->phy, PHY_MODE_MIPI_DPHY); >> + phy_configure(csi_dev->phy, &opts); >> + >> + control |= SW_CPHY_EN(0); >> + >> + } else if (bus_type == V4L2_MBUS_CSI2_CPHY) { >> + control |= SW_CPHY_EN(1); >> + >> + /* TODO: implement CPHY configuration */ >> + } else { >> + return -EINVAL; >> + } >> + >> + control |= SW_DATATYPE_FS(0x00) | SW_DATATYPE_FE(0x01) | >> + SW_DATATYPE_LS(0x02) | SW_DATATYPE_LE(0x03); >> + >> + rkcif_csi_write(csi_dev, CSI2HOST_N_LANES, lanes - 1); >> + rkcif_csi_write(csi_dev, CSI2HOST_CONTROL, control); >> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 1); >> + >> + phy_power_on(csi_dev->phy); >> + >> + return 0; >> +} >> + >> +static void rkcif_csi_stop(struct rkcif_csi_device *csi_dev) >> +{ >> + phy_power_off(csi_dev->phy); >> + >> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 0); >> + rkcif_csi_write(csi_dev, CSI2HOST_MSK1, ~0); >> + rkcif_csi_write(csi_dev, CSI2HOST_MSK2, ~0); >> +} >> + >> +static const struct media_entity_operations rkcif_csi_media_ops = { >> + .link_validate = v4l2_subdev_link_validate, >> +}; >> + >> +static int rkcif_csi_enum_mbus_code(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *sd_state, >> + struct v4l2_subdev_mbus_code_enum *code) >> +{ >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >> + >> + if (code->pad == RKCIF_CSI_PAD_SRC) { >> + const struct v4l2_mbus_framefmt *sink_fmt; >> + >> + if (code->index) >> + return -EINVAL; >> + >> + sink_fmt = v4l2_subdev_state_get_format(sd_state, >> + RKCIF_CSI_PAD_SINK); >> + code->code = sink_fmt->code; >> + >> + return 0; >> + } else if (code->pad == RKCIF_CSI_PAD_SINK) { >> + if (code->index > csi_dev->formats_num) >> + return -EINVAL; >> + >> + code->code = csi_dev->formats[code->index].code; >> + return 0; >> + } >> + >> + return -EINVAL; >> +} >> + >> +static int rkcif_csi_set_fmt(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, >> + struct v4l2_subdev_format *format) >> +{ >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >> + const struct rkcif_csi_format *fmt; >> + struct v4l2_mbus_framefmt *sink, *src; >> + >> + /* the format on the source pad always matches the sink pad */ >> + if (format->pad == RKCIF_CSI_PAD_SRC) >> + return v4l2_subdev_get_fmt(sd, state, format); >> + >> + sink = v4l2_subdev_state_get_format(state, format->pad, format->stream); >> + if (!sink) >> + return -EINVAL; >> + >> + fmt = rkcif_csi_find_format(csi_dev, format->format.code); >> + if (fmt) >> + *sink = format->format; >> + else >> + *sink = default_format; >> + >> + /* propagate the format to the source pad */ >> + src = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, >> + format->stream); >> + if (!src) >> + return -EINVAL; >> + >> + *src = *sink; >> + >> + return 0; >> +} >> + >> +static int rkcif_csi_set_routing(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, >> + enum v4l2_subdev_format_whence which, >> + struct v4l2_subdev_krouting *routing) >> +{ >> + int ret; >> + >> + ret = v4l2_subdev_routing_validate(sd, routing, >> + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1); >> + if (ret) >> + return ret; >> + >> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, >> + &default_format); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int rkcif_csi_enable_streams(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u64 streams_mask) >> +{ >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >> + struct v4l2_subdev *remote_sd; >> + struct media_pad *sink_pad, *remote_pad; >> + struct device *dev = csi_dev->dev; >> + u64 mask; >> + int ret; >> + >> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; >> + remote_pad = media_pad_remote_pad_first(sink_pad); >> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); >> + >> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, >> + RKCIF_CSI_PAD_SRC, >> + &streams_mask); >> + >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret) >> + goto err; >> + >> + ret = rkcif_csi_start(csi_dev); >> + if (ret) { >> + dev_err(dev, "failed to enable CSI hardware\n"); >> + goto err_pm_runtime_put; >> + } >> + >> + ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index, mask); >> + if (ret) >> + goto err_csi_stop; >> + >> + return 0; >> + >> +err_csi_stop: >> + rkcif_csi_stop(csi_dev); >> +err_pm_runtime_put: >> + pm_runtime_put_sync(dev); >> +err: >> + return ret; >> +} >> + >> +static int rkcif_csi_disable_streams(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u64 streams_mask) >> +{ >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >> + struct v4l2_subdev *remote_sd; >> + struct media_pad *sink_pad, *remote_pad; >> + struct device *dev = csi_dev->dev; >> + u64 mask; >> + int ret; >> + >> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; >> + remote_pad = media_pad_remote_pad_first(sink_pad); >> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); >> + >> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, >> + RKCIF_CSI_PAD_SRC, >> + &streams_mask); >> + >> + ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask); >> + >> + rkcif_csi_stop(csi_dev); >> + >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> + >> + return ret; >> +} >> + >> +static const struct v4l2_subdev_pad_ops rkcif_csi_pad_ops = { >> + .enum_mbus_code = rkcif_csi_enum_mbus_code, >> + .get_fmt = v4l2_subdev_get_fmt, >> + .set_fmt = rkcif_csi_set_fmt, >> + .set_routing = rkcif_csi_set_routing, >> + .enable_streams = rkcif_csi_enable_streams, >> + .disable_streams = rkcif_csi_disable_streams, >> +}; >> + >> +static const struct v4l2_subdev_ops rkcif_csi_ops = { >> + .pad = &rkcif_csi_pad_ops, >> +}; >> + >> +static int rkcif_csi_init_state(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state) >> +{ >> + struct v4l2_subdev_route routes[] = { >> + { >> + .sink_pad = RKCIF_CSI_PAD_SINK, >> + .sink_stream = 0, >> + .source_pad = RKCIF_CSI_PAD_SRC, >> + .source_stream = 0, >> + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, >> + }, >> + }; >> + struct v4l2_subdev_krouting routing = { >> + .len_routes = ARRAY_SIZE(routes), >> + .num_routes = ARRAY_SIZE(routes), >> + .routes = routes, >> + }; >> + int ret; >> + >> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing, >> + &default_format); >> + >> + return ret; >> +} >> + >> +static const struct v4l2_subdev_internal_ops rkcif_csi_internal_ops = { >> + .init_state = rkcif_csi_init_state, >> +}; >> + >> +static int rkcif_csi_notifier_bound(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *sd, >> + struct v4l2_async_connection *asd) >> +{ >> + struct rkcif_csi_device *csi_dev = >> + container_of(notifier, struct rkcif_csi_device, notifier); >> + int source_pad; >> + >> + source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >> + MEDIA_PAD_FL_SOURCE); >> + if (source_pad < 0) { >> + dev_err(csi_dev->dev, "failed to find source pad for %s\n", >> + sd->name); >> + return source_pad; >> + } >> + >> + csi_dev->source_sd = sd; >> + csi_dev->source_pad = source_pad; >> + >> + return media_create_pad_link(&sd->entity, source_pad, >> + &csi_dev->sd.entity, RKCIF_CSI_PAD_SINK, >> + MEDIA_LNK_FL_ENABLED); >> +} >> + >> +static const struct v4l2_async_notifier_operations rkcif_csi_notifier_ops = { >> + .bound = rkcif_csi_notifier_bound, >> +}; >> + >> +static int rkcif_csi_register_notifier(struct rkcif_csi_device *csi_dev) >> +{ >> + struct v4l2_async_connection *asd; >> + struct v4l2_async_notifier *ntf = &csi_dev->notifier; >> + struct v4l2_fwnode_endpoint *vep = &csi_dev->vep; >> + struct v4l2_subdev *sd = &csi_dev->sd; >> + struct device *dev = csi_dev->dev; >> + struct fwnode_handle *ep; >> + int ret = 0; >> + >> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); >> + if (!ep) >> + return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n"); >> + >> + vep->bus_type = V4L2_MBUS_UNKNOWN; >> + ret = v4l2_fwnode_endpoint_parse(ep, vep); >> + if (ret) { >> + ret = dev_err_probe(dev, ret, "failed to parse endpoint\n"); >> + goto out; >> + } >> + >> + if (vep->bus_type != V4L2_MBUS_CSI2_DPHY && >> + vep->bus_type != V4L2_MBUS_CSI2_CPHY) { >> + ret = dev_err_probe(dev, -EINVAL, >> + "invalid bus type of endpoint\n"); >> + goto out; >> + } >> + >> + v4l2_async_subdev_nf_init(ntf, sd); >> + ntf->ops = &rkcif_csi_notifier_ops; >> + >> + asd = v4l2_async_nf_add_fwnode_remote(ntf, ep, >> + struct v4l2_async_connection); >> + if (IS_ERR(asd)) { >> + ret = PTR_ERR(asd); >> + goto err_nf_cleanup; >> + } >> + >> + ret = v4l2_async_nf_register(ntf); >> + if (ret) { >> + ret = dev_err_probe(dev, ret, "failed to register notifier\n"); >> + goto err_nf_cleanup; >> + } >> + >> + goto out; >> + >> +err_nf_cleanup: >> + v4l2_async_nf_cleanup(ntf); >> +out: >> + fwnode_handle_put(ep); >> + return ret; >> +} >> + >> +static int rkcif_csi_register(struct rkcif_csi_device *csi_dev) >> +{ >> + struct media_pad *pads = csi_dev->pads; >> + struct v4l2_subdev *sd = &csi_dev->sd; >> + int ret; >> + >> + ret = rkcif_csi_register_notifier(csi_dev); >> + if (ret) >> + goto err; >> + >> + v4l2_subdev_init(sd, &rkcif_csi_ops); >> + sd->dev = csi_dev->dev; >> + sd->entity.ops = &rkcif_csi_media_ops; >> + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; >> + sd->internal_ops = &rkcif_csi_internal_ops; >> + sd->owner = THIS_MODULE; >> + snprintf(sd->name, sizeof(sd->name), "rockchip-mipi-csi %s", >> + dev_name(csi_dev->dev)); >> + >> + pads[RKCIF_CSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK | >> + MEDIA_PAD_FL_MUST_CONNECT; >> + pads[RKCIF_CSI_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE; >> + ret = media_entity_pads_init(&sd->entity, RKCIF_CSI_PAD_MAX, pads); >> + if (ret) >> + goto err_notifier_unregister; >> + >> + ret = v4l2_subdev_init_finalize(sd); >> + if (ret) >> + goto err_entity_cleanup; >> + >> + ret = v4l2_async_register_subdev(sd); >> + if (ret) { >> + dev_err(sd->dev, "failed to register CSI subdev\n"); >> + goto err_subdev_cleanup; >> + } >> + >> + return 0; >> + >> +err_subdev_cleanup: >> + v4l2_subdev_cleanup(sd); >> +err_entity_cleanup: >> + media_entity_cleanup(&sd->entity); >> +err_notifier_unregister: >> + v4l2_async_nf_unregister(&csi_dev->notifier); >> + v4l2_async_nf_cleanup(&csi_dev->notifier); >> +err: >> + return ret; >> +} >> + >> +static void rkcif_csi_unregister(struct rkcif_csi_device *csi_dev) >> +{ >> + struct v4l2_subdev *sd = &csi_dev->sd; >> + >> + v4l2_async_unregister_subdev(sd); >> + v4l2_subdev_cleanup(sd); >> + media_entity_cleanup(&sd->entity); >> + v4l2_async_nf_unregister(&csi_dev->notifier); >> + v4l2_async_nf_cleanup(&csi_dev->notifier); >> +} >> + >> +static const struct of_device_id rkcif_csi_of_match[] = { >> + { >> + .compatible = "rockchip,rk3568-mipi-csi", >> + }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, rkcif_csi_of_match); >> + >> +static int rkcif_csi_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct rkcif_csi_device *csi_dev; >> + int ret; >> + >> + csi_dev = devm_kzalloc(dev, sizeof(*csi_dev), GFP_KERNEL); >> + if (!csi_dev) >> + return -ENOMEM; >> + csi_dev->dev = dev; >> + dev_set_drvdata(dev, csi_dev); >> + >> + csi_dev->base_addr = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(csi_dev->base_addr)) >> + return PTR_ERR(csi_dev->base_addr); >> + >> + ret = devm_clk_bulk_get_all(dev, &csi_dev->clks); >> + if (ret != RKCIF_CSI_CLKS_MAX) >> + return dev_err_probe(dev, -ENODEV, "failed to get clocks\n"); >> + csi_dev->clks_num = ret; >> + >> + csi_dev->phy = devm_phy_get(dev, NULL); >> + if (IS_ERR(csi_dev->phy)) >> + return dev_err_probe(dev, PTR_ERR(csi_dev->phy), >> + "failed to get MIPI CSI PHY\n"); >> + >> + csi_dev->reset = devm_reset_control_array_get_exclusive(dev); >> + if (IS_ERR(csi_dev->reset)) >> + return dev_err_probe(dev, PTR_ERR(csi_dev->reset), >> + "failed to get reset\n"); >> + >> + csi_dev->formats = formats; >> + csi_dev->formats_num = ARRAY_SIZE(formats); >> + >> + pm_runtime_enable(dev); >> + >> + ret = phy_init(csi_dev->phy); >> + if (ret) { >> + ret = dev_err_probe(dev, ret, >> + "failed to initialize MIPI CSI PHY\n"); >> + goto err_pm_runtime_disable; >> + } >> + >> + ret = rkcif_csi_register(csi_dev); >> + if (ret) >> + goto err_phy_exit; >> + >> + return 0; >> + >> +err_phy_exit: >> + phy_exit(csi_dev->phy); >> +err_pm_runtime_disable: >> + pm_runtime_disable(dev); >> + return ret; >> +} >> + >> +static void rkcif_csi_remove(struct platform_device *pdev) >> +{ >> + struct rkcif_csi_device *csi_dev = platform_get_drvdata(pdev); >> + struct device *dev = &pdev->dev; >> + >> + rkcif_csi_unregister(csi_dev); >> + phy_exit(csi_dev->phy); >> + pm_runtime_disable(dev); >> +} >> + >> +static int rkcif_csi_runtime_suspend(struct device *dev) >> +{ >> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); >> + >> + clk_bulk_disable_unprepare(csi_dev->clks_num, csi_dev->clks); >> + >> + return 0; >> +} >> + >> +static int rkcif_csi_runtime_resume(struct device *dev) >> +{ >> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); >> + int ret; >> + >> + reset_control_assert(csi_dev->reset); >> + udelay(5); >> + reset_control_deassert(csi_dev->reset); >> + >> + ret = clk_bulk_prepare_enable(csi_dev->clks_num, csi_dev->clks); >> + if (ret) { >> + dev_err(dev, "failed to enable clocks\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops rkcif_csi_pm_ops = { >> + .runtime_suspend = rkcif_csi_runtime_suspend, >> + .runtime_resume = rkcif_csi_runtime_resume, >> +}; >> + >> +static struct platform_driver rkcif_csi_drv = { >> + .driver = { >> + .name = "rockchip-mipi-csi", >> + .of_match_table = rkcif_csi_of_match, >> + .pm = &rkcif_csi_pm_ops, >> + }, >> + .probe = rkcif_csi_probe, >> + .remove = rkcif_csi_remove, >> +}; >> +module_platform_driver(rkcif_csi_drv); >> + >> +MODULE_DESCRIPTION("Rockchip MIPI CSI-2 Receiver platform driver"); >> +MODULE_LICENSE("GPL"); >
On Fri, May 02, 2025 at 04:19:59PM +0200, Michael Riesch wrote: > On 5/2/25 15:31, Laurent Pinchart wrote: > > On Wed, Apr 30, 2025 at 11:15:56AM +0200, Michael Riesch via B4 Relay wrote: > >> From: Michael Riesch <michael.riesch@wolfvision.net> > >> > >> The Rockchip RK3568 MIPI CSI-2 Receiver is a CSI-2 bridge with one > >> input port and one output port. It receives the data with the help > >> of an external MIPI PHY (C-PHY or D-PHY) and passes it to the > >> Rockchip RK3568 Video Capture (VICAP) block. > >> > >> Add a V4L2 subdevice driver for this unit. > >> > >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> > >> Signed-off-by: Michael Riesch <michael.riesch@collabora.com> > >> --- > >> drivers/media/platform/rockchip/rkcif/Makefile | 3 + > >> .../rockchip/rkcif/rkcif-mipi-csi-receiver.c | 731 +++++++++++++++++++++ > >> 2 files changed, 734 insertions(+) > >> > >> diff --git a/drivers/media/platform/rockchip/rkcif/Makefile b/drivers/media/platform/rockchip/rkcif/Makefile > >> index 818424972c7b..a5c18a45c213 100644 > >> --- a/drivers/media/platform/rockchip/rkcif/Makefile > >> +++ b/drivers/media/platform/rockchip/rkcif/Makefile > >> @@ -5,3 +5,6 @@ rockchip-cif-objs += rkcif-dev.o \ > >> rkcif-capture-mipi.o \ > >> rkcif-interface.o \ > >> rkcif-stream.o > >> + > >> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip-mipi-csi.o > >> +rockchip-mipi-csi-objs += rkcif-mipi-csi-receiver.o > >> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c > >> new file mode 100644 > >> index 000000000000..81489f70490f > >> --- /dev/null > >> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c > >> @@ -0,0 +1,731 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Rockchip MIPI CSI-2 Receiver Driver > >> + * > >> + * Copyright (C) 2019 Rockchip Electronics Co., Ltd. > >> + * Copyright (C) 2025 Michael Riesch <michael.riesch@wolfvision.net> > >> + */ > >> + > >> +#include <linux/clk.h> > >> +#include <linux/delay.h> > >> +#include <linux/io.h> > >> +#include <linux/module.h> > >> +#include <linux/of.h> > >> +#include <linux/of_graph.h> > >> +#include <linux/of_platform.h> > >> +#include <linux/phy/phy.h> > >> +#include <linux/platform_device.h> > >> +#include <linux/pm_runtime.h> > >> +#include <linux/reset.h> > >> + > >> +#include <media/mipi-csi2.h> > >> +#include <media/v4l2-ctrls.h> > >> +#include <media/v4l2-fwnode.h> > >> +#include <media/v4l2-subdev.h> > >> + > >> +#define CSI2HOST_N_LANES 0x04 > >> +#define CSI2HOST_CSI2_RESETN 0x10 > >> +#define CSI2HOST_PHY_STATE 0x14 > >> +#define CSI2HOST_ERR1 0x20 > >> +#define CSI2HOST_ERR2 0x24 > >> +#define CSI2HOST_MSK1 0x28 > >> +#define CSI2HOST_MSK2 0x2c > >> +#define CSI2HOST_CONTROL 0x40 > > > > I'm trying to get to the bottom of the CSI-2 RX integration questions > > for the RK3568. Some of the registers here seem to the CSI2RX_1C00 block > > as documented starting on page 1059 of the RK3568 TRM (revision 1.1), > > but they're not an exact match. The control register, in particular, > > doesn't match at all. Where is this CSI-2 receiver documented in the TRM > > ? > > That would be in Chapter 27 of the RK3568 TRM: "MIPI CSI HOST" > > The registers are at 0xfdfb0000 "CSI_RX_CTRL1", cf. Chapter 1 "System > Overview". > > Naturally, this is quite confusing, as there is a "CSI2RX" whose > registers are embedded in the ISP register map. > > "MIPI CSI HOST" and "CSI2RX" seem to be different IP cores but perform > the same job in principle. CSI2RX seems to have additional features > related to writing raw data into memory and to HDR, though. > > Hope that helps! It does, thank you. I wonder how I missed that. The IP seems very similar to other CSI-2 receivers already supported upstream, see drivers/media/platform/raspberrypi/rp1-cfe/dphy.c, drivers/media/platform/renesas/rcar-csi2.c and drivers/staging/media/imx/imx6-mipi-csi2.c. Those drivers combine support for the CSI-2 RX and D-PHY, explaining some of the differences (and in the case of the rcar-csi2 driver, I think it also combines support for different CSI-2 RX in a single driver). Is there something we could do to avoid adding a 4th driver for the same IP core family (from Synopsys or Cadence I assume) ? It could possibly be done on top of this series to avoid delaying VICAP support, but I'd really like to get this sorted out. > >> + > >> +#define SW_CPHY_EN(x) ((x) << 0) > >> +#define SW_DSI_EN(x) ((x) << 4) > >> +#define SW_DATATYPE_FS(x) ((x) << 8) > >> +#define SW_DATATYPE_FE(x) ((x) << 14) > >> +#define SW_DATATYPE_LS(x) ((x) << 20) > >> +#define SW_DATATYPE_LE(x) ((x) << 26) > >> + > >> +#define RKCIF_CSI_CLKS_MAX 1 > >> + > >> +enum { > >> + RKCIF_CSI_PAD_SINK, > >> + RKCIF_CSI_PAD_SRC, > >> + RKCIF_CSI_PAD_MAX, > >> +}; > >> + > >> +struct rkcif_csi_format { > >> + u32 code; > >> + u8 depth; > >> + u8 csi_dt; > >> +}; > >> + > >> +struct rkcif_csi_device { > >> + struct device *dev; > >> + > >> + void __iomem *base_addr; > >> + struct clk_bulk_data *clks; > >> + unsigned int clks_num; > >> + struct phy *phy; > >> + struct reset_control *reset; > >> + > >> + const struct rkcif_csi_format *formats; > >> + unsigned int formats_num; > >> + > >> + struct media_pad pads[RKCIF_CSI_PAD_MAX]; > >> + struct v4l2_async_notifier notifier; > >> + struct v4l2_fwnode_endpoint vep; > >> + struct v4l2_subdev sd; > >> + > >> + struct v4l2_subdev *source_sd; > >> + u32 source_pad; > >> +}; > >> + > >> +static const struct v4l2_mbus_framefmt default_format = { > >> + .width = 3840, > >> + .height = 2160, > >> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, > >> + .field = V4L2_FIELD_NONE, > >> + .colorspace = V4L2_COLORSPACE_RAW, > >> + .ycbcr_enc = V4L2_YCBCR_ENC_601, > >> + .quantization = V4L2_QUANTIZATION_FULL_RANGE, > >> + .xfer_func = V4L2_XFER_FUNC_NONE, > >> +}; > >> + > >> +static const struct rkcif_csi_format formats[] = { > >> + /* YUV formats */ > >> + { > >> + .code = MEDIA_BUS_FMT_YUYV8_1X16, > >> + .depth = 16, > >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_UYVY8_1X16, > >> + .depth = 16, > >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_YVYU8_1X16, > >> + .depth = 16, > >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_VYUY8_1X16, > >> + .depth = 16, > >> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >> + }, > >> + /* RGB formats */ > >> + { > >> + .code = MEDIA_BUS_FMT_RGB888_1X24, > >> + .depth = 24, > >> + .csi_dt = MIPI_CSI2_DT_RGB888, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_BGR888_1X24, > >> + .depth = 24, > >> + .csi_dt = MIPI_CSI2_DT_RGB888, > >> + }, > >> + /* Bayer formats */ > >> + { > >> + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > >> + .depth = 8, > >> + .csi_dt = MIPI_CSI2_DT_RAW8, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SGBRG8_1X8, > >> + .depth = 8, > >> + .csi_dt = MIPI_CSI2_DT_RAW8, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SGRBG8_1X8, > >> + .depth = 8, > >> + .csi_dt = MIPI_CSI2_DT_RAW8, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SRGGB8_1X8, > >> + .depth = 8, > >> + .csi_dt = MIPI_CSI2_DT_RAW8, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SBGGR10_1X10, > >> + .depth = 10, > >> + .csi_dt = MIPI_CSI2_DT_RAW10, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SGBRG10_1X10, > >> + .depth = 10, > >> + .csi_dt = MIPI_CSI2_DT_RAW10, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SGRBG10_1X10, > >> + .depth = 10, > >> + .csi_dt = MIPI_CSI2_DT_RAW10, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, > >> + .depth = 10, > >> + .csi_dt = MIPI_CSI2_DT_RAW10, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SBGGR12_1X12, > >> + .depth = 12, > >> + .csi_dt = MIPI_CSI2_DT_RAW12, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SGBRG12_1X12, > >> + .depth = 12, > >> + .csi_dt = MIPI_CSI2_DT_RAW12, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SGRBG12_1X12, > >> + .depth = 12, > >> + .csi_dt = MIPI_CSI2_DT_RAW12, > >> + }, > >> + { > >> + .code = MEDIA_BUS_FMT_SRGGB12_1X12, > >> + .depth = 12, > >> + .csi_dt = MIPI_CSI2_DT_RAW12, > >> + }, > >> +}; > >> + > >> +static inline struct rkcif_csi_device *to_rkcif_csi(struct v4l2_subdev *sd) > >> +{ > >> + return container_of(sd, struct rkcif_csi_device, sd); > >> +} > >> + > >> +static inline __maybe_unused void > >> +rkcif_csi_write(struct rkcif_csi_device *csi_dev, unsigned int addr, u32 val) > >> +{ > >> + writel(val, csi_dev->base_addr + addr); > >> +} > >> + > >> +static inline __maybe_unused u32 > >> +rkcif_csi_read(struct rkcif_csi_device *csi_dev, unsigned int addr) > >> +{ > >> + return readl(csi_dev->base_addr + addr); > >> +} > >> + > >> +static const struct rkcif_csi_format * > >> +rkcif_csi_find_format(struct rkcif_csi_device *csi_dev, u32 mbus_code) > >> +{ > >> + const struct rkcif_csi_format *format; > >> + > >> + WARN_ON(csi_dev->formats_num == 0); > >> + > >> + for (int i = 0; i < csi_dev->formats_num; i++) { > >> + format = &csi_dev->formats[i]; > >> + if (format->code == mbus_code) > >> + return format; > >> + } > >> + > >> + return NULL; > >> +} > >> + > >> +static int rkcif_csi_start(struct rkcif_csi_device *csi_dev) > >> +{ > >> + enum v4l2_mbus_type bus_type = csi_dev->vep.bus_type; > >> + union phy_configure_opts opts; > >> + s64 link_freq; > >> + u32 lanes = csi_dev->vep.bus.mipi_csi2.num_data_lanes; > >> + u32 control = 0; > >> + > >> + if (lanes < 1 || lanes > 4) > >> + return -EINVAL; > >> + > >> + /* set mult and div to 0, thus completely rely on V4L2_CID_LINK_FREQ */ > >> + link_freq = v4l2_get_link_freq(csi_dev->source_sd->ctrl_handler, 0, 0); > >> + if (link_freq <= 0) > >> + return -EINVAL; > >> + > >> + if (bus_type == V4L2_MBUS_CSI2_DPHY) { > >> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > >> + > >> + phy_mipi_dphy_get_default_config_for_hsclk(link_freq * 2, lanes, > >> + cfg); > >> + phy_set_mode(csi_dev->phy, PHY_MODE_MIPI_DPHY); > >> + phy_configure(csi_dev->phy, &opts); > >> + > >> + control |= SW_CPHY_EN(0); > >> + > >> + } else if (bus_type == V4L2_MBUS_CSI2_CPHY) { > >> + control |= SW_CPHY_EN(1); > >> + > >> + /* TODO: implement CPHY configuration */ > >> + } else { > >> + return -EINVAL; > >> + } > >> + > >> + control |= SW_DATATYPE_FS(0x00) | SW_DATATYPE_FE(0x01) | > >> + SW_DATATYPE_LS(0x02) | SW_DATATYPE_LE(0x03); > >> + > >> + rkcif_csi_write(csi_dev, CSI2HOST_N_LANES, lanes - 1); > >> + rkcif_csi_write(csi_dev, CSI2HOST_CONTROL, control); > >> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 1); > >> + > >> + phy_power_on(csi_dev->phy); > >> + > >> + return 0; > >> +} > >> + > >> +static void rkcif_csi_stop(struct rkcif_csi_device *csi_dev) > >> +{ > >> + phy_power_off(csi_dev->phy); > >> + > >> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 0); > >> + rkcif_csi_write(csi_dev, CSI2HOST_MSK1, ~0); > >> + rkcif_csi_write(csi_dev, CSI2HOST_MSK2, ~0); > >> +} > >> + > >> +static const struct media_entity_operations rkcif_csi_media_ops = { > >> + .link_validate = v4l2_subdev_link_validate, > >> +}; > >> + > >> +static int rkcif_csi_enum_mbus_code(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_state *sd_state, > >> + struct v4l2_subdev_mbus_code_enum *code) > >> +{ > >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >> + > >> + if (code->pad == RKCIF_CSI_PAD_SRC) { > >> + const struct v4l2_mbus_framefmt *sink_fmt; > >> + > >> + if (code->index) > >> + return -EINVAL; > >> + > >> + sink_fmt = v4l2_subdev_state_get_format(sd_state, > >> + RKCIF_CSI_PAD_SINK); > >> + code->code = sink_fmt->code; > >> + > >> + return 0; > >> + } else if (code->pad == RKCIF_CSI_PAD_SINK) { > >> + if (code->index > csi_dev->formats_num) > >> + return -EINVAL; > >> + > >> + code->code = csi_dev->formats[code->index].code; > >> + return 0; > >> + } > >> + > >> + return -EINVAL; > >> +} > >> + > >> +static int rkcif_csi_set_fmt(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_state *state, > >> + struct v4l2_subdev_format *format) > >> +{ > >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >> + const struct rkcif_csi_format *fmt; > >> + struct v4l2_mbus_framefmt *sink, *src; > >> + > >> + /* the format on the source pad always matches the sink pad */ > >> + if (format->pad == RKCIF_CSI_PAD_SRC) > >> + return v4l2_subdev_get_fmt(sd, state, format); > >> + > >> + sink = v4l2_subdev_state_get_format(state, format->pad, format->stream); > >> + if (!sink) > >> + return -EINVAL; > >> + > >> + fmt = rkcif_csi_find_format(csi_dev, format->format.code); > >> + if (fmt) > >> + *sink = format->format; > >> + else > >> + *sink = default_format; > >> + > >> + /* propagate the format to the source pad */ > >> + src = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, > >> + format->stream); > >> + if (!src) > >> + return -EINVAL; > >> + > >> + *src = *sink; > >> + > >> + return 0; > >> +} > >> + > >> +static int rkcif_csi_set_routing(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_state *state, > >> + enum v4l2_subdev_format_whence which, > >> + struct v4l2_subdev_krouting *routing) > >> +{ > >> + int ret; > >> + > >> + ret = v4l2_subdev_routing_validate(sd, routing, > >> + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1); > >> + if (ret) > >> + return ret; > >> + > >> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, > >> + &default_format); > >> + if (ret) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +static int rkcif_csi_enable_streams(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_state *state, u32 pad, > >> + u64 streams_mask) > >> +{ > >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >> + struct v4l2_subdev *remote_sd; > >> + struct media_pad *sink_pad, *remote_pad; > >> + struct device *dev = csi_dev->dev; > >> + u64 mask; > >> + int ret; > >> + > >> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; > >> + remote_pad = media_pad_remote_pad_first(sink_pad); > >> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); > >> + > >> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, > >> + RKCIF_CSI_PAD_SRC, > >> + &streams_mask); > >> + > >> + ret = pm_runtime_resume_and_get(dev); > >> + if (ret) > >> + goto err; > >> + > >> + ret = rkcif_csi_start(csi_dev); > >> + if (ret) { > >> + dev_err(dev, "failed to enable CSI hardware\n"); > >> + goto err_pm_runtime_put; > >> + } > >> + > >> + ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index, mask); > >> + if (ret) > >> + goto err_csi_stop; > >> + > >> + return 0; > >> + > >> +err_csi_stop: > >> + rkcif_csi_stop(csi_dev); > >> +err_pm_runtime_put: > >> + pm_runtime_put_sync(dev); > >> +err: > >> + return ret; > >> +} > >> + > >> +static int rkcif_csi_disable_streams(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_state *state, u32 pad, > >> + u64 streams_mask) > >> +{ > >> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >> + struct v4l2_subdev *remote_sd; > >> + struct media_pad *sink_pad, *remote_pad; > >> + struct device *dev = csi_dev->dev; > >> + u64 mask; > >> + int ret; > >> + > >> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; > >> + remote_pad = media_pad_remote_pad_first(sink_pad); > >> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); > >> + > >> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, > >> + RKCIF_CSI_PAD_SRC, > >> + &streams_mask); > >> + > >> + ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask); > >> + > >> + rkcif_csi_stop(csi_dev); > >> + > >> + pm_runtime_mark_last_busy(dev); > >> + pm_runtime_put_autosuspend(dev); > >> + > >> + return ret; > >> +} > >> + > >> +static const struct v4l2_subdev_pad_ops rkcif_csi_pad_ops = { > >> + .enum_mbus_code = rkcif_csi_enum_mbus_code, > >> + .get_fmt = v4l2_subdev_get_fmt, > >> + .set_fmt = rkcif_csi_set_fmt, > >> + .set_routing = rkcif_csi_set_routing, > >> + .enable_streams = rkcif_csi_enable_streams, > >> + .disable_streams = rkcif_csi_disable_streams, > >> +}; > >> + > >> +static const struct v4l2_subdev_ops rkcif_csi_ops = { > >> + .pad = &rkcif_csi_pad_ops, > >> +}; > >> + > >> +static int rkcif_csi_init_state(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_state *state) > >> +{ > >> + struct v4l2_subdev_route routes[] = { > >> + { > >> + .sink_pad = RKCIF_CSI_PAD_SINK, > >> + .sink_stream = 0, > >> + .source_pad = RKCIF_CSI_PAD_SRC, > >> + .source_stream = 0, > >> + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > >> + }, > >> + }; > >> + struct v4l2_subdev_krouting routing = { > >> + .len_routes = ARRAY_SIZE(routes), > >> + .num_routes = ARRAY_SIZE(routes), > >> + .routes = routes, > >> + }; > >> + int ret; > >> + > >> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing, > >> + &default_format); > >> + > >> + return ret; > >> +} > >> + > >> +static const struct v4l2_subdev_internal_ops rkcif_csi_internal_ops = { > >> + .init_state = rkcif_csi_init_state, > >> +}; > >> + > >> +static int rkcif_csi_notifier_bound(struct v4l2_async_notifier *notifier, > >> + struct v4l2_subdev *sd, > >> + struct v4l2_async_connection *asd) > >> +{ > >> + struct rkcif_csi_device *csi_dev = > >> + container_of(notifier, struct rkcif_csi_device, notifier); > >> + int source_pad; > >> + > >> + source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, > >> + MEDIA_PAD_FL_SOURCE); > >> + if (source_pad < 0) { > >> + dev_err(csi_dev->dev, "failed to find source pad for %s\n", > >> + sd->name); > >> + return source_pad; > >> + } > >> + > >> + csi_dev->source_sd = sd; > >> + csi_dev->source_pad = source_pad; > >> + > >> + return media_create_pad_link(&sd->entity, source_pad, > >> + &csi_dev->sd.entity, RKCIF_CSI_PAD_SINK, > >> + MEDIA_LNK_FL_ENABLED); > >> +} > >> + > >> +static const struct v4l2_async_notifier_operations rkcif_csi_notifier_ops = { > >> + .bound = rkcif_csi_notifier_bound, > >> +}; > >> + > >> +static int rkcif_csi_register_notifier(struct rkcif_csi_device *csi_dev) > >> +{ > >> + struct v4l2_async_connection *asd; > >> + struct v4l2_async_notifier *ntf = &csi_dev->notifier; > >> + struct v4l2_fwnode_endpoint *vep = &csi_dev->vep; > >> + struct v4l2_subdev *sd = &csi_dev->sd; > >> + struct device *dev = csi_dev->dev; > >> + struct fwnode_handle *ep; > >> + int ret = 0; > >> + > >> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > >> + if (!ep) > >> + return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n"); > >> + > >> + vep->bus_type = V4L2_MBUS_UNKNOWN; > >> + ret = v4l2_fwnode_endpoint_parse(ep, vep); > >> + if (ret) { > >> + ret = dev_err_probe(dev, ret, "failed to parse endpoint\n"); > >> + goto out; > >> + } > >> + > >> + if (vep->bus_type != V4L2_MBUS_CSI2_DPHY && > >> + vep->bus_type != V4L2_MBUS_CSI2_CPHY) { > >> + ret = dev_err_probe(dev, -EINVAL, > >> + "invalid bus type of endpoint\n"); > >> + goto out; > >> + } > >> + > >> + v4l2_async_subdev_nf_init(ntf, sd); > >> + ntf->ops = &rkcif_csi_notifier_ops; > >> + > >> + asd = v4l2_async_nf_add_fwnode_remote(ntf, ep, > >> + struct v4l2_async_connection); > >> + if (IS_ERR(asd)) { > >> + ret = PTR_ERR(asd); > >> + goto err_nf_cleanup; > >> + } > >> + > >> + ret = v4l2_async_nf_register(ntf); > >> + if (ret) { > >> + ret = dev_err_probe(dev, ret, "failed to register notifier\n"); > >> + goto err_nf_cleanup; > >> + } > >> + > >> + goto out; > >> + > >> +err_nf_cleanup: > >> + v4l2_async_nf_cleanup(ntf); > >> +out: > >> + fwnode_handle_put(ep); > >> + return ret; > >> +} > >> + > >> +static int rkcif_csi_register(struct rkcif_csi_device *csi_dev) > >> +{ > >> + struct media_pad *pads = csi_dev->pads; > >> + struct v4l2_subdev *sd = &csi_dev->sd; > >> + int ret; > >> + > >> + ret = rkcif_csi_register_notifier(csi_dev); > >> + if (ret) > >> + goto err; > >> + > >> + v4l2_subdev_init(sd, &rkcif_csi_ops); > >> + sd->dev = csi_dev->dev; > >> + sd->entity.ops = &rkcif_csi_media_ops; > >> + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > >> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > >> + sd->internal_ops = &rkcif_csi_internal_ops; > >> + sd->owner = THIS_MODULE; > >> + snprintf(sd->name, sizeof(sd->name), "rockchip-mipi-csi %s", > >> + dev_name(csi_dev->dev)); > >> + > >> + pads[RKCIF_CSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK | > >> + MEDIA_PAD_FL_MUST_CONNECT; > >> + pads[RKCIF_CSI_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE; > >> + ret = media_entity_pads_init(&sd->entity, RKCIF_CSI_PAD_MAX, pads); > >> + if (ret) > >> + goto err_notifier_unregister; > >> + > >> + ret = v4l2_subdev_init_finalize(sd); > >> + if (ret) > >> + goto err_entity_cleanup; > >> + > >> + ret = v4l2_async_register_subdev(sd); > >> + if (ret) { > >> + dev_err(sd->dev, "failed to register CSI subdev\n"); > >> + goto err_subdev_cleanup; > >> + } > >> + > >> + return 0; > >> + > >> +err_subdev_cleanup: > >> + v4l2_subdev_cleanup(sd); > >> +err_entity_cleanup: > >> + media_entity_cleanup(&sd->entity); > >> +err_notifier_unregister: > >> + v4l2_async_nf_unregister(&csi_dev->notifier); > >> + v4l2_async_nf_cleanup(&csi_dev->notifier); > >> +err: > >> + return ret; > >> +} > >> + > >> +static void rkcif_csi_unregister(struct rkcif_csi_device *csi_dev) > >> +{ > >> + struct v4l2_subdev *sd = &csi_dev->sd; > >> + > >> + v4l2_async_unregister_subdev(sd); > >> + v4l2_subdev_cleanup(sd); > >> + media_entity_cleanup(&sd->entity); > >> + v4l2_async_nf_unregister(&csi_dev->notifier); > >> + v4l2_async_nf_cleanup(&csi_dev->notifier); > >> +} > >> + > >> +static const struct of_device_id rkcif_csi_of_match[] = { > >> + { > >> + .compatible = "rockchip,rk3568-mipi-csi", > >> + }, > >> + {} > >> +}; > >> +MODULE_DEVICE_TABLE(of, rkcif_csi_of_match); > >> + > >> +static int rkcif_csi_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct rkcif_csi_device *csi_dev; > >> + int ret; > >> + > >> + csi_dev = devm_kzalloc(dev, sizeof(*csi_dev), GFP_KERNEL); > >> + if (!csi_dev) > >> + return -ENOMEM; > >> + csi_dev->dev = dev; > >> + dev_set_drvdata(dev, csi_dev); > >> + > >> + csi_dev->base_addr = devm_platform_ioremap_resource(pdev, 0); > >> + if (IS_ERR(csi_dev->base_addr)) > >> + return PTR_ERR(csi_dev->base_addr); > >> + > >> + ret = devm_clk_bulk_get_all(dev, &csi_dev->clks); > >> + if (ret != RKCIF_CSI_CLKS_MAX) > >> + return dev_err_probe(dev, -ENODEV, "failed to get clocks\n"); > >> + csi_dev->clks_num = ret; > >> + > >> + csi_dev->phy = devm_phy_get(dev, NULL); > >> + if (IS_ERR(csi_dev->phy)) > >> + return dev_err_probe(dev, PTR_ERR(csi_dev->phy), > >> + "failed to get MIPI CSI PHY\n"); > >> + > >> + csi_dev->reset = devm_reset_control_array_get_exclusive(dev); > >> + if (IS_ERR(csi_dev->reset)) > >> + return dev_err_probe(dev, PTR_ERR(csi_dev->reset), > >> + "failed to get reset\n"); > >> + > >> + csi_dev->formats = formats; > >> + csi_dev->formats_num = ARRAY_SIZE(formats); > >> + > >> + pm_runtime_enable(dev); > >> + > >> + ret = phy_init(csi_dev->phy); > >> + if (ret) { > >> + ret = dev_err_probe(dev, ret, > >> + "failed to initialize MIPI CSI PHY\n"); > >> + goto err_pm_runtime_disable; > >> + } > >> + > >> + ret = rkcif_csi_register(csi_dev); > >> + if (ret) > >> + goto err_phy_exit; > >> + > >> + return 0; > >> + > >> +err_phy_exit: > >> + phy_exit(csi_dev->phy); > >> +err_pm_runtime_disable: > >> + pm_runtime_disable(dev); > >> + return ret; > >> +} > >> + > >> +static void rkcif_csi_remove(struct platform_device *pdev) > >> +{ > >> + struct rkcif_csi_device *csi_dev = platform_get_drvdata(pdev); > >> + struct device *dev = &pdev->dev; > >> + > >> + rkcif_csi_unregister(csi_dev); > >> + phy_exit(csi_dev->phy); > >> + pm_runtime_disable(dev); > >> +} > >> + > >> +static int rkcif_csi_runtime_suspend(struct device *dev) > >> +{ > >> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); > >> + > >> + clk_bulk_disable_unprepare(csi_dev->clks_num, csi_dev->clks); > >> + > >> + return 0; > >> +} > >> + > >> +static int rkcif_csi_runtime_resume(struct device *dev) > >> +{ > >> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); > >> + int ret; > >> + > >> + reset_control_assert(csi_dev->reset); > >> + udelay(5); > >> + reset_control_deassert(csi_dev->reset); > >> + > >> + ret = clk_bulk_prepare_enable(csi_dev->clks_num, csi_dev->clks); > >> + if (ret) { > >> + dev_err(dev, "failed to enable clocks\n"); > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static const struct dev_pm_ops rkcif_csi_pm_ops = { > >> + .runtime_suspend = rkcif_csi_runtime_suspend, > >> + .runtime_resume = rkcif_csi_runtime_resume, > >> +}; > >> + > >> +static struct platform_driver rkcif_csi_drv = { > >> + .driver = { > >> + .name = "rockchip-mipi-csi", > >> + .of_match_table = rkcif_csi_of_match, > >> + .pm = &rkcif_csi_pm_ops, > >> + }, > >> + .probe = rkcif_csi_probe, > >> + .remove = rkcif_csi_remove, > >> +}; > >> +module_platform_driver(rkcif_csi_drv); > >> + > >> +MODULE_DESCRIPTION("Rockchip MIPI CSI-2 Receiver platform driver"); > >> +MODULE_LICENSE("GPL");
Hi Michael, Thank you for the patch! Is it possible to sent the v4l2-compliance output in the next version ? On Wed, Apr 30, 2025 at 11:15:55AM +0200, Michael Riesch via B4 Relay wrote: > From: Michael Riesch <michael.riesch@wolfvision.net> > SNIP > +irqreturn_t rkcif_dvp_isr(int irq, void *ctx) > +{ > + struct device *dev = ctx; > + struct rkcif_device *rkcif = dev_get_drvdata(dev); > + struct rkcif_stream *stream; > + u32 intstat, lastline, lastpix, cif_frmst; > + irqreturn_t ret = IRQ_NONE; > + > + if (!rkcif->match_data->dvp) > + return ret; > + > + intstat = cif_dvp_read(rkcif, RKCIF_DVP_INTSTAT); > + cif_frmst = cif_dvp_read(rkcif, RKCIF_DVP_FRAME_STATUS); > + lastline = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_LINE)); > + lastpix = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_PIX)); > + > + if (intstat & RKCIF_INTSTAT_FRAME_END) { > + cif_dvp_write(rkcif, RKCIF_DVP_INTSTAT, > + RKCIF_INTSTAT_FRAME_END_CLR | > + RKCIF_INTSTAT_LINE_END_CLR); > + > + stream = &rkcif->interfaces[RKCIF_DVP].streams[RKCIF_ID0]; > + > + if (stream->stopping) { > + cif_dvp_stop_streaming(stream); > + wake_up(&stream->wq_stopped); > + return IRQ_HANDLED; > + } > + > + if (lastline != stream->pix.height) { > + v4l2_err(&rkcif->v4l2_dev, > + "bad frame, irq:%#x frmst:%#x size:%dx%d\n", > + intstat, cif_frmst, lastpix, lastline); > + > + cif_dvp_reset_stream(rkcif); > + } > + > + rkcif_stream_pingpong(stream); > + > + ret = IRQ_HANDLED; just return IRQ_HANDLED like above ? > + } > + > + return ret; > +} > + > +int rkcif_dvp_register(struct rkcif_device *rkcif) > +{ > + struct rkcif_interface *interface; > + int ret, i; > + > + if (!rkcif->match_data->dvp) > + return 0; > + > + interface = &rkcif->interfaces[RKCIF_DVP]; > + interface->index = RKCIF_DVP; > + interface->type = RKCIF_IF_DVP; > + interface->in_fmts = rkcif->match_data->dvp->in_fmts; > + interface->in_fmts_num = rkcif->match_data->dvp->in_fmts_num; > + interface->set_crop = rkcif_dvp_set_crop; > + ret = rkcif_interface_register(rkcif, interface); > + if (ret) > + return 0; | +-> Copy-paste error ? > + > + if (rkcif->match_data->dvp->setup) > + rkcif->match_data->dvp->setup(rkcif); > + > + interface->streams_num = rkcif->match_data->dvp->has_ids ? 4 : 1; > + for (i = 0; i < interface->streams_num; i++) { > + struct rkcif_stream *stream = &interface->streams[i]; > + > + stream->id = i; > + stream->interface = interface; > + stream->out_fmts = rkcif->match_data->dvp->out_fmts; > + stream->out_fmts_num = rkcif->match_data->dvp->out_fmts_num; > + stream->queue_buffer = cif_dvp_queue_buffer; > + stream->start_streaming = cif_dvp_start_streaming; > + stream->stop_streaming = cif_dvp_stop_streaming; > + > + ret = rkcif_stream_register(rkcif, stream); > + if (ret) > + goto err_streams_unregister; > + } > + return 0; > + > +err_streams_unregister: > + for (; i >= 0; i--) > + rkcif_stream_unregister(&interface->streams[i]); > + rkcif_interface_unregister(interface); > + > + return ret; > +} > + SNIP > +static inline struct rkcif_buffer *to_rkcif_buffer(struct vb2_v4l2_buffer *vb) > +{ > + return container_of(vb, struct rkcif_buffer, vb); > +} > + > +static inline struct rkcif_stream *to_rkcif_stream(struct video_device *vdev) > +{ > + return container_of(vdev, struct rkcif_stream, vdev); > +} > + > +static struct rkcif_buffer *rkcif_stream_pop_buffer(struct rkcif_stream *stream) > +{ > + struct rkcif_buffer *buffer = NULL; > + unsigned long lock_flags; > + > + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); guard(spinlock_irqsave)(&stream->driver_queue_lock) will simplify this function. > + > + if (list_empty(&stream->driver_queue)) > + goto err_empty; > + > + buffer = list_first_entry(&stream->driver_queue, struct rkcif_buffer, > + queue); > + list_del(&buffer->queue); > + > +err_empty: > + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); > + return buffer; > +} > + > +static void rkcif_stream_push_buffer(struct rkcif_stream *stream, > + struct rkcif_buffer *buffer) > +{ > + unsigned long lock_flags; > + > + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); > + list_add_tail(&buffer->queue, &stream->driver_queue); > + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); > +} > + > +static inline void rkcif_stream_return_buffer(struct rkcif_buffer *buffer, > + enum vb2_buffer_state state) > +{ > + struct vb2_v4l2_buffer *vb = &buffer->vb; > + > + vb2_buffer_done(&vb->vb2_buf, state); > +} > + > +static void rkcif_stream_complete_buffer(struct rkcif_stream *stream, > + struct rkcif_buffer *buffer) > +{ > + struct vb2_v4l2_buffer *vb = &buffer->vb; > + > + vb->vb2_buf.timestamp = ktime_get_ns(); > + vb->sequence = stream->frame_idx; > + vb2_buffer_done(&vb->vb2_buf, VB2_BUF_STATE_DONE); > + stream->frame_idx++; > +} > + > +void rkcif_stream_pingpong(struct rkcif_stream *stream) > +{ > + struct rkcif_buffer *buffer; > + > + buffer = stream->buffers[stream->frame_phase]; > + if (!buffer->is_dummy) > + rkcif_stream_complete_buffer(stream, buffer); You can actually keep this frame dropping mechanism without using the dummy buffer. You can set a drop flag to TRUE: keep overwriting the buffer you already have without returning it to user-space until you can get another buffer, set the flag again to FALSE and resume returning the buffers to user-space. > + > + buffer = rkcif_stream_pop_buffer(stream); > + if (buffer) { > + stream->buffers[stream->frame_phase] = buffer; > + stream->buffers[stream->frame_phase]->is_dummy = false; > + } else { > + stream->buffers[stream->frame_phase] = &stream->dummy.buffer; > + stream->buffers[stream->frame_phase]->is_dummy = true; > + dev_warn(stream->rkcif->dev, > + "no buffer available, frame will be dropped\n"); This warning can quickly flood the kernel logs if the user-space is too slow in enqueuing the buffers. > + } > + > + if (stream->queue_buffer) > + stream->queue_buffer(stream, stream->frame_phase); is this if statement really needed ? > + > + stream->frame_phase = 1 - stream->frame_phase; > +} > + > +static int rkcif_stream_init_buffers(struct rkcif_stream *stream) > +{ > + struct v4l2_pix_format_mplane *pix = &stream->pix; > + int i; > + > + stream->buffers[0] = rkcif_stream_pop_buffer(stream); > + if (!stream->buffers[0]) > + goto err_buff_0; > + > + stream->buffers[1] = rkcif_stream_pop_buffer(stream); > + if (!stream->buffers[1]) > + goto err_buff_1; > + > + if (stream->queue_buffer) { > + stream->queue_buffer(stream, 0); > + stream->queue_buffer(stream, 1); > + } > + > + stream->dummy.size = pix->num_planes * pix->plane_fmt[0].sizeimage; > + stream->dummy.vaddr = > + dma_alloc_attrs(stream->rkcif->dev, stream->dummy.size, > + &stream->dummy.buffer.buff_addr[0], GFP_KERNEL, > + DMA_ATTR_NO_KERNEL_MAPPING); > + if (!stream->dummy.vaddr) > + goto err_dummy; > + > + for (i = 1; i < pix->num_planes; i++) > + stream->dummy.buffer.buff_addr[i] = > + stream->dummy.buffer.buff_addr[i - 1] + > + pix->plane_fmt[i - 1].bytesperline * pix->height; > + > + return 0; > + > +err_dummy: > + rkcif_stream_return_buffer(stream->buffers[1], VB2_BUF_STATE_QUEUED); > + stream->buffers[1] = NULL; > + > +err_buff_1: > + rkcif_stream_return_buffer(stream->buffers[0], VB2_BUF_STATE_QUEUED); > + stream->buffers[0] = NULL; > +err_buff_0: > + return -EINVAL; > +} > + SNIP > +static int rkcif_stream_init_vb2_queue(struct vb2_queue *q, > + struct rkcif_stream *stream) > +{ > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > + q->io_modes = VB2_MMAP | VB2_DMABUF; > + q->drv_priv = stream; > + q->ops = &rkcif_stream_vb2_ops; > + q->mem_ops = &vb2_dma_contig_memops; > + q->buf_struct_size = sizeof(struct rkcif_buffer); > + q->min_queued_buffers = CIF_REQ_BUFS_MIN; If I recall correctly min_queued_buffers should be the strict minimum number of buffers you need to start streaming. So in this case it should be 3 = 2 pingpong buffers + 1 dummy buffer. VIDIOC_REQBUFS will allocate min_queued_buffers + 1 and user-space will probably allocate even more anyway. > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > + q->lock = &stream->vlock; > + q->dev = stream->rkcif->dev; > + > + return vb2_queue_init(q); > +} -- Kind Regards Mehdi Djait
Hi Laurent, On 5/2/25 16:35, Laurent Pinchart wrote: > On Fri, May 02, 2025 at 04:19:59PM +0200, Michael Riesch wrote: >> On 5/2/25 15:31, Laurent Pinchart wrote: >>> On Wed, Apr 30, 2025 at 11:15:56AM +0200, Michael Riesch via B4 Relay wrote: >>>> From: Michael Riesch <michael.riesch@wolfvision.net> >>>> >>>> The Rockchip RK3568 MIPI CSI-2 Receiver is a CSI-2 bridge with one >>>> input port and one output port. It receives the data with the help >>>> of an external MIPI PHY (C-PHY or D-PHY) and passes it to the >>>> Rockchip RK3568 Video Capture (VICAP) block. >>>> >>>> Add a V4L2 subdevice driver for this unit. >>>> >>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> >>>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com> >>>> --- >>>> drivers/media/platform/rockchip/rkcif/Makefile | 3 + >>>> .../rockchip/rkcif/rkcif-mipi-csi-receiver.c | 731 +++++++++++++++++++++ >>>> 2 files changed, 734 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/rockchip/rkcif/Makefile b/drivers/media/platform/rockchip/rkcif/Makefile >>>> index 818424972c7b..a5c18a45c213 100644 >>>> --- a/drivers/media/platform/rockchip/rkcif/Makefile >>>> +++ b/drivers/media/platform/rockchip/rkcif/Makefile >>>> @@ -5,3 +5,6 @@ rockchip-cif-objs += rkcif-dev.o \ >>>> rkcif-capture-mipi.o \ >>>> rkcif-interface.o \ >>>> rkcif-stream.o >>>> + >>>> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip-mipi-csi.o >>>> +rockchip-mipi-csi-objs += rkcif-mipi-csi-receiver.o >>>> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c >>>> new file mode 100644 >>>> index 000000000000..81489f70490f >>>> --- /dev/null >>>> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c >>>> @@ -0,0 +1,731 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * Rockchip MIPI CSI-2 Receiver Driver >>>> + * >>>> + * Copyright (C) 2019 Rockchip Electronics Co., Ltd. >>>> + * Copyright (C) 2025 Michael Riesch <michael.riesch@wolfvision.net> >>>> + */ >>>> + >>>> +#include <linux/clk.h> >>>> +#include <linux/delay.h> >>>> +#include <linux/io.h> >>>> +#include <linux/module.h> >>>> +#include <linux/of.h> >>>> +#include <linux/of_graph.h> >>>> +#include <linux/of_platform.h> >>>> +#include <linux/phy/phy.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/pm_runtime.h> >>>> +#include <linux/reset.h> >>>> + >>>> +#include <media/mipi-csi2.h> >>>> +#include <media/v4l2-ctrls.h> >>>> +#include <media/v4l2-fwnode.h> >>>> +#include <media/v4l2-subdev.h> >>>> + >>>> +#define CSI2HOST_N_LANES 0x04 >>>> +#define CSI2HOST_CSI2_RESETN 0x10 >>>> +#define CSI2HOST_PHY_STATE 0x14 >>>> +#define CSI2HOST_ERR1 0x20 >>>> +#define CSI2HOST_ERR2 0x24 >>>> +#define CSI2HOST_MSK1 0x28 >>>> +#define CSI2HOST_MSK2 0x2c >>>> +#define CSI2HOST_CONTROL 0x40 >>> >>> I'm trying to get to the bottom of the CSI-2 RX integration questions >>> for the RK3568. Some of the registers here seem to the CSI2RX_1C00 block >>> as documented starting on page 1059 of the RK3568 TRM (revision 1.1), >>> but they're not an exact match. The control register, in particular, >>> doesn't match at all. Where is this CSI-2 receiver documented in the TRM >>> ? >> >> That would be in Chapter 27 of the RK3568 TRM: "MIPI CSI HOST" >> >> The registers are at 0xfdfb0000 "CSI_RX_CTRL1", cf. Chapter 1 "System >> Overview". >> >> Naturally, this is quite confusing, as there is a "CSI2RX" whose >> registers are embedded in the ISP register map. >> >> "MIPI CSI HOST" and "CSI2RX" seem to be different IP cores but perform >> the same job in principle. CSI2RX seems to have additional features >> related to writing raw data into memory and to HDR, though. >> >> Hope that helps! > > It does, thank you. I wonder how I missed that. > > The IP seems very similar to other CSI-2 receivers already supported > upstream, see drivers/media/platform/raspberrypi/rp1-cfe/dphy.c, > drivers/media/platform/renesas/rcar-csi2.c and > drivers/staging/media/imx/imx6-mipi-csi2.c. Those drivers combine > support for the CSI-2 RX and D-PHY, explaining some of the differences > (and in the case of the rcar-csi2 driver, I think it also combines > support for different CSI-2 RX in a single driver). Before I started writing the driver under discussion, I did scan the existing drivers, but could not see any similarities to rcar-csi2 and rp1-cfe/dphy.c (and I still don't see them), respectively (but maybe those two are the same). Unfortunately, I forgot to look into staging. Hm. Indeed it resembles the imx6-mipi-csi2.c driver, at least judging by the register layout. According to Kever, while a Synopsys IP core was used in older SoCs, the newer SoCs feature an IP core developed by Rockchip. Probably the register map was recycled but slightly modified. > Is there something we could do to avoid adding a 4th driver for the same > IP core family (from Synopsys or Cadence I assume) ? It could possibly > be done on top of this series to avoid delaying VICAP support, but I'd > really like to get this sorted out. Based on my current level of knowledge I wouldn't be comfortable with placing this driver in media/platform/synopsys/something. If you all advise me to do so, I will comply, though. I am not too keen on digging out a quite dated staging driver and basing the current efforts on that one. We could, however, move this driver to media/platform/rockchip/mipi-csi-receiver/ (name to be bikeshedded, open to suggestions). It is a separate kernel module already anyway, and maybe this way it is more visible to the next person developing a driver for a similar IP core. Sound reasonable? Best regards, Michael > >>>> + >>>> +#define SW_CPHY_EN(x) ((x) << 0) >>>> +#define SW_DSI_EN(x) ((x) << 4) >>>> +#define SW_DATATYPE_FS(x) ((x) << 8) >>>> +#define SW_DATATYPE_FE(x) ((x) << 14) >>>> +#define SW_DATATYPE_LS(x) ((x) << 20) >>>> +#define SW_DATATYPE_LE(x) ((x) << 26) >>>> + >>>> +#define RKCIF_CSI_CLKS_MAX 1 >>>> + >>>> +enum { >>>> + RKCIF_CSI_PAD_SINK, >>>> + RKCIF_CSI_PAD_SRC, >>>> + RKCIF_CSI_PAD_MAX, >>>> +}; >>>> + >>>> +struct rkcif_csi_format { >>>> + u32 code; >>>> + u8 depth; >>>> + u8 csi_dt; >>>> +}; >>>> + >>>> +struct rkcif_csi_device { >>>> + struct device *dev; >>>> + >>>> + void __iomem *base_addr; >>>> + struct clk_bulk_data *clks; >>>> + unsigned int clks_num; >>>> + struct phy *phy; >>>> + struct reset_control *reset; >>>> + >>>> + const struct rkcif_csi_format *formats; >>>> + unsigned int formats_num; >>>> + >>>> + struct media_pad pads[RKCIF_CSI_PAD_MAX]; >>>> + struct v4l2_async_notifier notifier; >>>> + struct v4l2_fwnode_endpoint vep; >>>> + struct v4l2_subdev sd; >>>> + >>>> + struct v4l2_subdev *source_sd; >>>> + u32 source_pad; >>>> +}; >>>> + >>>> +static const struct v4l2_mbus_framefmt default_format = { >>>> + .width = 3840, >>>> + .height = 2160, >>>> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, >>>> + .field = V4L2_FIELD_NONE, >>>> + .colorspace = V4L2_COLORSPACE_RAW, >>>> + .ycbcr_enc = V4L2_YCBCR_ENC_601, >>>> + .quantization = V4L2_QUANTIZATION_FULL_RANGE, >>>> + .xfer_func = V4L2_XFER_FUNC_NONE, >>>> +}; >>>> + >>>> +static const struct rkcif_csi_format formats[] = { >>>> + /* YUV formats */ >>>> + { >>>> + .code = MEDIA_BUS_FMT_YUYV8_1X16, >>>> + .depth = 16, >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_UYVY8_1X16, >>>> + .depth = 16, >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_YVYU8_1X16, >>>> + .depth = 16, >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_VYUY8_1X16, >>>> + .depth = 16, >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, >>>> + }, >>>> + /* RGB formats */ >>>> + { >>>> + .code = MEDIA_BUS_FMT_RGB888_1X24, >>>> + .depth = 24, >>>> + .csi_dt = MIPI_CSI2_DT_RGB888, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_BGR888_1X24, >>>> + .depth = 24, >>>> + .csi_dt = MIPI_CSI2_DT_RGB888, >>>> + }, >>>> + /* Bayer formats */ >>>> + { >>>> + .code = MEDIA_BUS_FMT_SBGGR8_1X8, >>>> + .depth = 8, >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SGBRG8_1X8, >>>> + .depth = 8, >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SGRBG8_1X8, >>>> + .depth = 8, >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SRGGB8_1X8, >>>> + .depth = 8, >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SBGGR10_1X10, >>>> + .depth = 10, >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SGBRG10_1X10, >>>> + .depth = 10, >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SGRBG10_1X10, >>>> + .depth = 10, >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, >>>> + .depth = 10, >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SBGGR12_1X12, >>>> + .depth = 12, >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SGBRG12_1X12, >>>> + .depth = 12, >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SGRBG12_1X12, >>>> + .depth = 12, >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_SRGGB12_1X12, >>>> + .depth = 12, >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, >>>> + }, >>>> +}; >>>> + >>>> +static inline struct rkcif_csi_device *to_rkcif_csi(struct v4l2_subdev *sd) >>>> +{ >>>> + return container_of(sd, struct rkcif_csi_device, sd); >>>> +} >>>> + >>>> +static inline __maybe_unused void >>>> +rkcif_csi_write(struct rkcif_csi_device *csi_dev, unsigned int addr, u32 val) >>>> +{ >>>> + writel(val, csi_dev->base_addr + addr); >>>> +} >>>> + >>>> +static inline __maybe_unused u32 >>>> +rkcif_csi_read(struct rkcif_csi_device *csi_dev, unsigned int addr) >>>> +{ >>>> + return readl(csi_dev->base_addr + addr); >>>> +} >>>> + >>>> +static const struct rkcif_csi_format * >>>> +rkcif_csi_find_format(struct rkcif_csi_device *csi_dev, u32 mbus_code) >>>> +{ >>>> + const struct rkcif_csi_format *format; >>>> + >>>> + WARN_ON(csi_dev->formats_num == 0); >>>> + >>>> + for (int i = 0; i < csi_dev->formats_num; i++) { >>>> + format = &csi_dev->formats[i]; >>>> + if (format->code == mbus_code) >>>> + return format; >>>> + } >>>> + >>>> + return NULL; >>>> +} >>>> + >>>> +static int rkcif_csi_start(struct rkcif_csi_device *csi_dev) >>>> +{ >>>> + enum v4l2_mbus_type bus_type = csi_dev->vep.bus_type; >>>> + union phy_configure_opts opts; >>>> + s64 link_freq; >>>> + u32 lanes = csi_dev->vep.bus.mipi_csi2.num_data_lanes; >>>> + u32 control = 0; >>>> + >>>> + if (lanes < 1 || lanes > 4) >>>> + return -EINVAL; >>>> + >>>> + /* set mult and div to 0, thus completely rely on V4L2_CID_LINK_FREQ */ >>>> + link_freq = v4l2_get_link_freq(csi_dev->source_sd->ctrl_handler, 0, 0); >>>> + if (link_freq <= 0) >>>> + return -EINVAL; >>>> + >>>> + if (bus_type == V4L2_MBUS_CSI2_DPHY) { >>>> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; >>>> + >>>> + phy_mipi_dphy_get_default_config_for_hsclk(link_freq * 2, lanes, >>>> + cfg); >>>> + phy_set_mode(csi_dev->phy, PHY_MODE_MIPI_DPHY); >>>> + phy_configure(csi_dev->phy, &opts); >>>> + >>>> + control |= SW_CPHY_EN(0); >>>> + >>>> + } else if (bus_type == V4L2_MBUS_CSI2_CPHY) { >>>> + control |= SW_CPHY_EN(1); >>>> + >>>> + /* TODO: implement CPHY configuration */ >>>> + } else { >>>> + return -EINVAL; >>>> + } >>>> + >>>> + control |= SW_DATATYPE_FS(0x00) | SW_DATATYPE_FE(0x01) | >>>> + SW_DATATYPE_LS(0x02) | SW_DATATYPE_LE(0x03); >>>> + >>>> + rkcif_csi_write(csi_dev, CSI2HOST_N_LANES, lanes - 1); >>>> + rkcif_csi_write(csi_dev, CSI2HOST_CONTROL, control); >>>> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 1); >>>> + >>>> + phy_power_on(csi_dev->phy); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void rkcif_csi_stop(struct rkcif_csi_device *csi_dev) >>>> +{ >>>> + phy_power_off(csi_dev->phy); >>>> + >>>> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 0); >>>> + rkcif_csi_write(csi_dev, CSI2HOST_MSK1, ~0); >>>> + rkcif_csi_write(csi_dev, CSI2HOST_MSK2, ~0); >>>> +} >>>> + >>>> +static const struct media_entity_operations rkcif_csi_media_ops = { >>>> + .link_validate = v4l2_subdev_link_validate, >>>> +}; >>>> + >>>> +static int rkcif_csi_enum_mbus_code(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_mbus_code_enum *code) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >>>> + >>>> + if (code->pad == RKCIF_CSI_PAD_SRC) { >>>> + const struct v4l2_mbus_framefmt *sink_fmt; >>>> + >>>> + if (code->index) >>>> + return -EINVAL; >>>> + >>>> + sink_fmt = v4l2_subdev_state_get_format(sd_state, >>>> + RKCIF_CSI_PAD_SINK); >>>> + code->code = sink_fmt->code; >>>> + >>>> + return 0; >>>> + } else if (code->pad == RKCIF_CSI_PAD_SINK) { >>>> + if (code->index > csi_dev->formats_num) >>>> + return -EINVAL; >>>> + >>>> + code->code = csi_dev->formats[code->index].code; >>>> + return 0; >>>> + } >>>> + >>>> + return -EINVAL; >>>> +} >>>> + >>>> +static int rkcif_csi_set_fmt(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *state, >>>> + struct v4l2_subdev_format *format) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >>>> + const struct rkcif_csi_format *fmt; >>>> + struct v4l2_mbus_framefmt *sink, *src; >>>> + >>>> + /* the format on the source pad always matches the sink pad */ >>>> + if (format->pad == RKCIF_CSI_PAD_SRC) >>>> + return v4l2_subdev_get_fmt(sd, state, format); >>>> + >>>> + sink = v4l2_subdev_state_get_format(state, format->pad, format->stream); >>>> + if (!sink) >>>> + return -EINVAL; >>>> + >>>> + fmt = rkcif_csi_find_format(csi_dev, format->format.code); >>>> + if (fmt) >>>> + *sink = format->format; >>>> + else >>>> + *sink = default_format; >>>> + >>>> + /* propagate the format to the source pad */ >>>> + src = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, >>>> + format->stream); >>>> + if (!src) >>>> + return -EINVAL; >>>> + >>>> + *src = *sink; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int rkcif_csi_set_routing(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *state, >>>> + enum v4l2_subdev_format_whence which, >>>> + struct v4l2_subdev_krouting *routing) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = v4l2_subdev_routing_validate(sd, routing, >>>> + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, >>>> + &default_format); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int rkcif_csi_enable_streams(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *state, u32 pad, >>>> + u64 streams_mask) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >>>> + struct v4l2_subdev *remote_sd; >>>> + struct media_pad *sink_pad, *remote_pad; >>>> + struct device *dev = csi_dev->dev; >>>> + u64 mask; >>>> + int ret; >>>> + >>>> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; >>>> + remote_pad = media_pad_remote_pad_first(sink_pad); >>>> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); >>>> + >>>> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, >>>> + RKCIF_CSI_PAD_SRC, >>>> + &streams_mask); >>>> + >>>> + ret = pm_runtime_resume_and_get(dev); >>>> + if (ret) >>>> + goto err; >>>> + >>>> + ret = rkcif_csi_start(csi_dev); >>>> + if (ret) { >>>> + dev_err(dev, "failed to enable CSI hardware\n"); >>>> + goto err_pm_runtime_put; >>>> + } >>>> + >>>> + ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index, mask); >>>> + if (ret) >>>> + goto err_csi_stop; >>>> + >>>> + return 0; >>>> + >>>> +err_csi_stop: >>>> + rkcif_csi_stop(csi_dev); >>>> +err_pm_runtime_put: >>>> + pm_runtime_put_sync(dev); >>>> +err: >>>> + return ret; >>>> +} >>>> + >>>> +static int rkcif_csi_disable_streams(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *state, u32 pad, >>>> + u64 streams_mask) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); >>>> + struct v4l2_subdev *remote_sd; >>>> + struct media_pad *sink_pad, *remote_pad; >>>> + struct device *dev = csi_dev->dev; >>>> + u64 mask; >>>> + int ret; >>>> + >>>> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; >>>> + remote_pad = media_pad_remote_pad_first(sink_pad); >>>> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); >>>> + >>>> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, >>>> + RKCIF_CSI_PAD_SRC, >>>> + &streams_mask); >>>> + >>>> + ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask); >>>> + >>>> + rkcif_csi_stop(csi_dev); >>>> + >>>> + pm_runtime_mark_last_busy(dev); >>>> + pm_runtime_put_autosuspend(dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct v4l2_subdev_pad_ops rkcif_csi_pad_ops = { >>>> + .enum_mbus_code = rkcif_csi_enum_mbus_code, >>>> + .get_fmt = v4l2_subdev_get_fmt, >>>> + .set_fmt = rkcif_csi_set_fmt, >>>> + .set_routing = rkcif_csi_set_routing, >>>> + .enable_streams = rkcif_csi_enable_streams, >>>> + .disable_streams = rkcif_csi_disable_streams, >>>> +}; >>>> + >>>> +static const struct v4l2_subdev_ops rkcif_csi_ops = { >>>> + .pad = &rkcif_csi_pad_ops, >>>> +}; >>>> + >>>> +static int rkcif_csi_init_state(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_state *state) >>>> +{ >>>> + struct v4l2_subdev_route routes[] = { >>>> + { >>>> + .sink_pad = RKCIF_CSI_PAD_SINK, >>>> + .sink_stream = 0, >>>> + .source_pad = RKCIF_CSI_PAD_SRC, >>>> + .source_stream = 0, >>>> + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, >>>> + }, >>>> + }; >>>> + struct v4l2_subdev_krouting routing = { >>>> + .len_routes = ARRAY_SIZE(routes), >>>> + .num_routes = ARRAY_SIZE(routes), >>>> + .routes = routes, >>>> + }; >>>> + int ret; >>>> + >>>> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing, >>>> + &default_format); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct v4l2_subdev_internal_ops rkcif_csi_internal_ops = { >>>> + .init_state = rkcif_csi_init_state, >>>> +}; >>>> + >>>> +static int rkcif_csi_notifier_bound(struct v4l2_async_notifier *notifier, >>>> + struct v4l2_subdev *sd, >>>> + struct v4l2_async_connection *asd) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = >>>> + container_of(notifier, struct rkcif_csi_device, notifier); >>>> + int source_pad; >>>> + >>>> + source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >>>> + MEDIA_PAD_FL_SOURCE); >>>> + if (source_pad < 0) { >>>> + dev_err(csi_dev->dev, "failed to find source pad for %s\n", >>>> + sd->name); >>>> + return source_pad; >>>> + } >>>> + >>>> + csi_dev->source_sd = sd; >>>> + csi_dev->source_pad = source_pad; >>>> + >>>> + return media_create_pad_link(&sd->entity, source_pad, >>>> + &csi_dev->sd.entity, RKCIF_CSI_PAD_SINK, >>>> + MEDIA_LNK_FL_ENABLED); >>>> +} >>>> + >>>> +static const struct v4l2_async_notifier_operations rkcif_csi_notifier_ops = { >>>> + .bound = rkcif_csi_notifier_bound, >>>> +}; >>>> + >>>> +static int rkcif_csi_register_notifier(struct rkcif_csi_device *csi_dev) >>>> +{ >>>> + struct v4l2_async_connection *asd; >>>> + struct v4l2_async_notifier *ntf = &csi_dev->notifier; >>>> + struct v4l2_fwnode_endpoint *vep = &csi_dev->vep; >>>> + struct v4l2_subdev *sd = &csi_dev->sd; >>>> + struct device *dev = csi_dev->dev; >>>> + struct fwnode_handle *ep; >>>> + int ret = 0; >>>> + >>>> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); >>>> + if (!ep) >>>> + return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n"); >>>> + >>>> + vep->bus_type = V4L2_MBUS_UNKNOWN; >>>> + ret = v4l2_fwnode_endpoint_parse(ep, vep); >>>> + if (ret) { >>>> + ret = dev_err_probe(dev, ret, "failed to parse endpoint\n"); >>>> + goto out; >>>> + } >>>> + >>>> + if (vep->bus_type != V4L2_MBUS_CSI2_DPHY && >>>> + vep->bus_type != V4L2_MBUS_CSI2_CPHY) { >>>> + ret = dev_err_probe(dev, -EINVAL, >>>> + "invalid bus type of endpoint\n"); >>>> + goto out; >>>> + } >>>> + >>>> + v4l2_async_subdev_nf_init(ntf, sd); >>>> + ntf->ops = &rkcif_csi_notifier_ops; >>>> + >>>> + asd = v4l2_async_nf_add_fwnode_remote(ntf, ep, >>>> + struct v4l2_async_connection); >>>> + if (IS_ERR(asd)) { >>>> + ret = PTR_ERR(asd); >>>> + goto err_nf_cleanup; >>>> + } >>>> + >>>> + ret = v4l2_async_nf_register(ntf); >>>> + if (ret) { >>>> + ret = dev_err_probe(dev, ret, "failed to register notifier\n"); >>>> + goto err_nf_cleanup; >>>> + } >>>> + >>>> + goto out; >>>> + >>>> +err_nf_cleanup: >>>> + v4l2_async_nf_cleanup(ntf); >>>> +out: >>>> + fwnode_handle_put(ep); >>>> + return ret; >>>> +} >>>> + >>>> +static int rkcif_csi_register(struct rkcif_csi_device *csi_dev) >>>> +{ >>>> + struct media_pad *pads = csi_dev->pads; >>>> + struct v4l2_subdev *sd = &csi_dev->sd; >>>> + int ret; >>>> + >>>> + ret = rkcif_csi_register_notifier(csi_dev); >>>> + if (ret) >>>> + goto err; >>>> + >>>> + v4l2_subdev_init(sd, &rkcif_csi_ops); >>>> + sd->dev = csi_dev->dev; >>>> + sd->entity.ops = &rkcif_csi_media_ops; >>>> + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >>>> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; >>>> + sd->internal_ops = &rkcif_csi_internal_ops; >>>> + sd->owner = THIS_MODULE; >>>> + snprintf(sd->name, sizeof(sd->name), "rockchip-mipi-csi %s", >>>> + dev_name(csi_dev->dev)); >>>> + >>>> + pads[RKCIF_CSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK | >>>> + MEDIA_PAD_FL_MUST_CONNECT; >>>> + pads[RKCIF_CSI_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE; >>>> + ret = media_entity_pads_init(&sd->entity, RKCIF_CSI_PAD_MAX, pads); >>>> + if (ret) >>>> + goto err_notifier_unregister; >>>> + >>>> + ret = v4l2_subdev_init_finalize(sd); >>>> + if (ret) >>>> + goto err_entity_cleanup; >>>> + >>>> + ret = v4l2_async_register_subdev(sd); >>>> + if (ret) { >>>> + dev_err(sd->dev, "failed to register CSI subdev\n"); >>>> + goto err_subdev_cleanup; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +err_subdev_cleanup: >>>> + v4l2_subdev_cleanup(sd); >>>> +err_entity_cleanup: >>>> + media_entity_cleanup(&sd->entity); >>>> +err_notifier_unregister: >>>> + v4l2_async_nf_unregister(&csi_dev->notifier); >>>> + v4l2_async_nf_cleanup(&csi_dev->notifier); >>>> +err: >>>> + return ret; >>>> +} >>>> + >>>> +static void rkcif_csi_unregister(struct rkcif_csi_device *csi_dev) >>>> +{ >>>> + struct v4l2_subdev *sd = &csi_dev->sd; >>>> + >>>> + v4l2_async_unregister_subdev(sd); >>>> + v4l2_subdev_cleanup(sd); >>>> + media_entity_cleanup(&sd->entity); >>>> + v4l2_async_nf_unregister(&csi_dev->notifier); >>>> + v4l2_async_nf_cleanup(&csi_dev->notifier); >>>> +} >>>> + >>>> +static const struct of_device_id rkcif_csi_of_match[] = { >>>> + { >>>> + .compatible = "rockchip,rk3568-mipi-csi", >>>> + }, >>>> + {} >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, rkcif_csi_of_match); >>>> + >>>> +static int rkcif_csi_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct rkcif_csi_device *csi_dev; >>>> + int ret; >>>> + >>>> + csi_dev = devm_kzalloc(dev, sizeof(*csi_dev), GFP_KERNEL); >>>> + if (!csi_dev) >>>> + return -ENOMEM; >>>> + csi_dev->dev = dev; >>>> + dev_set_drvdata(dev, csi_dev); >>>> + >>>> + csi_dev->base_addr = devm_platform_ioremap_resource(pdev, 0); >>>> + if (IS_ERR(csi_dev->base_addr)) >>>> + return PTR_ERR(csi_dev->base_addr); >>>> + >>>> + ret = devm_clk_bulk_get_all(dev, &csi_dev->clks); >>>> + if (ret != RKCIF_CSI_CLKS_MAX) >>>> + return dev_err_probe(dev, -ENODEV, "failed to get clocks\n"); >>>> + csi_dev->clks_num = ret; >>>> + >>>> + csi_dev->phy = devm_phy_get(dev, NULL); >>>> + if (IS_ERR(csi_dev->phy)) >>>> + return dev_err_probe(dev, PTR_ERR(csi_dev->phy), >>>> + "failed to get MIPI CSI PHY\n"); >>>> + >>>> + csi_dev->reset = devm_reset_control_array_get_exclusive(dev); >>>> + if (IS_ERR(csi_dev->reset)) >>>> + return dev_err_probe(dev, PTR_ERR(csi_dev->reset), >>>> + "failed to get reset\n"); >>>> + >>>> + csi_dev->formats = formats; >>>> + csi_dev->formats_num = ARRAY_SIZE(formats); >>>> + >>>> + pm_runtime_enable(dev); >>>> + >>>> + ret = phy_init(csi_dev->phy); >>>> + if (ret) { >>>> + ret = dev_err_probe(dev, ret, >>>> + "failed to initialize MIPI CSI PHY\n"); >>>> + goto err_pm_runtime_disable; >>>> + } >>>> + >>>> + ret = rkcif_csi_register(csi_dev); >>>> + if (ret) >>>> + goto err_phy_exit; >>>> + >>>> + return 0; >>>> + >>>> +err_phy_exit: >>>> + phy_exit(csi_dev->phy); >>>> +err_pm_runtime_disable: >>>> + pm_runtime_disable(dev); >>>> + return ret; >>>> +} >>>> + >>>> +static void rkcif_csi_remove(struct platform_device *pdev) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = platform_get_drvdata(pdev); >>>> + struct device *dev = &pdev->dev; >>>> + >>>> + rkcif_csi_unregister(csi_dev); >>>> + phy_exit(csi_dev->phy); >>>> + pm_runtime_disable(dev); >>>> +} >>>> + >>>> +static int rkcif_csi_runtime_suspend(struct device *dev) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); >>>> + >>>> + clk_bulk_disable_unprepare(csi_dev->clks_num, csi_dev->clks); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int rkcif_csi_runtime_resume(struct device *dev) >>>> +{ >>>> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + reset_control_assert(csi_dev->reset); >>>> + udelay(5); >>>> + reset_control_deassert(csi_dev->reset); >>>> + >>>> + ret = clk_bulk_prepare_enable(csi_dev->clks_num, csi_dev->clks); >>>> + if (ret) { >>>> + dev_err(dev, "failed to enable clocks\n"); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct dev_pm_ops rkcif_csi_pm_ops = { >>>> + .runtime_suspend = rkcif_csi_runtime_suspend, >>>> + .runtime_resume = rkcif_csi_runtime_resume, >>>> +}; >>>> + >>>> +static struct platform_driver rkcif_csi_drv = { >>>> + .driver = { >>>> + .name = "rockchip-mipi-csi", >>>> + .of_match_table = rkcif_csi_of_match, >>>> + .pm = &rkcif_csi_pm_ops, >>>> + }, >>>> + .probe = rkcif_csi_probe, >>>> + .remove = rkcif_csi_remove, >>>> +}; >>>> +module_platform_driver(rkcif_csi_drv); >>>> + >>>> +MODULE_DESCRIPTION("Rockchip MIPI CSI-2 Receiver platform driver"); >>>> +MODULE_LICENSE("GPL"); >
Hi Mehdi, Thanks for your review! On 5/6/25 12:37, Mehdi Djait wrote: > Hi Michael, > > Thank you for the patch! > > Is it possible to sent the v4l2-compliance output in the next version ? > > On Wed, Apr 30, 2025 at 11:15:55AM +0200, Michael Riesch via B4 Relay wrote: >> From: Michael Riesch <michael.riesch@wolfvision.net> >> > > SNIP > >> +irqreturn_t rkcif_dvp_isr(int irq, void *ctx) >> +{ >> + struct device *dev = ctx; >> + struct rkcif_device *rkcif = dev_get_drvdata(dev); >> + struct rkcif_stream *stream; >> + u32 intstat, lastline, lastpix, cif_frmst; >> + irqreturn_t ret = IRQ_NONE; >> + >> + if (!rkcif->match_data->dvp) >> + return ret; >> + >> + intstat = cif_dvp_read(rkcif, RKCIF_DVP_INTSTAT); >> + cif_frmst = cif_dvp_read(rkcif, RKCIF_DVP_FRAME_STATUS); >> + lastline = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_LINE)); >> + lastpix = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_PIX)); >> + >> + if (intstat & RKCIF_INTSTAT_FRAME_END) { >> + cif_dvp_write(rkcif, RKCIF_DVP_INTSTAT, >> + RKCIF_INTSTAT_FRAME_END_CLR | >> + RKCIF_INTSTAT_LINE_END_CLR); >> + >> + stream = &rkcif->interfaces[RKCIF_DVP].streams[RKCIF_ID0]; >> + >> + if (stream->stopping) { >> + cif_dvp_stop_streaming(stream); >> + wake_up(&stream->wq_stopped); >> + return IRQ_HANDLED; >> + } >> + >> + if (lastline != stream->pix.height) { >> + v4l2_err(&rkcif->v4l2_dev, >> + "bad frame, irq:%#x frmst:%#x size:%dx%d\n", >> + intstat, cif_frmst, lastpix, lastline); >> + >> + cif_dvp_reset_stream(rkcif); >> + } >> + >> + rkcif_stream_pingpong(stream); >> + >> + ret = IRQ_HANDLED; > > just return IRQ_HANDLED like above ? I think I'll go along Bryan's suggestion to make it more consistent. > >> + } >> + >> + return ret; >> +} >> + >> +int rkcif_dvp_register(struct rkcif_device *rkcif) >> +{ >> + struct rkcif_interface *interface; >> + int ret, i; >> + >> + if (!rkcif->match_data->dvp) >> + return 0; >> + >> + interface = &rkcif->interfaces[RKCIF_DVP]; >> + interface->index = RKCIF_DVP; >> + interface->type = RKCIF_IF_DVP; >> + interface->in_fmts = rkcif->match_data->dvp->in_fmts; >> + interface->in_fmts_num = rkcif->match_data->dvp->in_fmts_num; >> + interface->set_crop = rkcif_dvp_set_crop; >> + ret = rkcif_interface_register(rkcif, interface); >> + if (ret) >> + return 0; > | > +-> Copy-paste error ? Hm. It's not a mistake. But maybe it is a bit misleading. The point here is that if something fails with registering the DVP, the driver may continue to register other entities, such as the MIPI capture thing. I'll have another look over this mechanism and will try to make it more comprehensible. > >> + >> + if (rkcif->match_data->dvp->setup) >> + rkcif->match_data->dvp->setup(rkcif); >> + >> + interface->streams_num = rkcif->match_data->dvp->has_ids ? 4 : 1; >> + for (i = 0; i < interface->streams_num; i++) { >> + struct rkcif_stream *stream = &interface->streams[i]; >> + >> + stream->id = i; >> + stream->interface = interface; >> + stream->out_fmts = rkcif->match_data->dvp->out_fmts; >> + stream->out_fmts_num = rkcif->match_data->dvp->out_fmts_num; >> + stream->queue_buffer = cif_dvp_queue_buffer; >> + stream->start_streaming = cif_dvp_start_streaming; >> + stream->stop_streaming = cif_dvp_stop_streaming; >> + >> + ret = rkcif_stream_register(rkcif, stream); >> + if (ret) >> + goto err_streams_unregister; >> + } >> + return 0; >> + >> +err_streams_unregister: >> + for (; i >= 0; i--) >> + rkcif_stream_unregister(&interface->streams[i]); >> + rkcif_interface_unregister(interface); >> + >> + return ret; >> +} >> + > > SNIP > >> +static inline struct rkcif_buffer *to_rkcif_buffer(struct vb2_v4l2_buffer *vb) >> +{ >> + return container_of(vb, struct rkcif_buffer, vb); >> +} >> + >> +static inline struct rkcif_stream *to_rkcif_stream(struct video_device *vdev) >> +{ >> + return container_of(vdev, struct rkcif_stream, vdev); >> +} >> + >> +static struct rkcif_buffer *rkcif_stream_pop_buffer(struct rkcif_stream *stream) >> +{ >> + struct rkcif_buffer *buffer = NULL; >> + unsigned long lock_flags; >> + >> + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); > > guard(spinlock_irqsave)(&stream->driver_queue_lock) will simplify this function. I'll guard up these methods in v7. > >> + >> + if (list_empty(&stream->driver_queue)) >> + goto err_empty; >> + >> + buffer = list_first_entry(&stream->driver_queue, struct rkcif_buffer, >> + queue); >> + list_del(&buffer->queue); >> + >> +err_empty: >> + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); >> + return buffer; >> +} >> + >> +static void rkcif_stream_push_buffer(struct rkcif_stream *stream, >> + struct rkcif_buffer *buffer) >> +{ >> + unsigned long lock_flags; >> + >> + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); >> + list_add_tail(&buffer->queue, &stream->driver_queue); >> + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); >> +} >> + >> +static inline void rkcif_stream_return_buffer(struct rkcif_buffer *buffer, >> + enum vb2_buffer_state state) >> +{ >> + struct vb2_v4l2_buffer *vb = &buffer->vb; >> + >> + vb2_buffer_done(&vb->vb2_buf, state); >> +} >> + >> +static void rkcif_stream_complete_buffer(struct rkcif_stream *stream, >> + struct rkcif_buffer *buffer) >> +{ >> + struct vb2_v4l2_buffer *vb = &buffer->vb; >> + >> + vb->vb2_buf.timestamp = ktime_get_ns(); >> + vb->sequence = stream->frame_idx; >> + vb2_buffer_done(&vb->vb2_buf, VB2_BUF_STATE_DONE); >> + stream->frame_idx++; >> +} >> + >> +void rkcif_stream_pingpong(struct rkcif_stream *stream) >> +{ >> + struct rkcif_buffer *buffer; >> + >> + buffer = stream->buffers[stream->frame_phase]; >> + if (!buffer->is_dummy) >> + rkcif_stream_complete_buffer(stream, buffer); > > You can actually keep this frame dropping mechanism without using the > dummy buffer. > > You can set a drop flag to TRUE: keep overwriting the buffer you already have > without returning it to user-space until you can get another buffer, set > the flag again to FALSE and resume returning the buffers to user-space. The approach you describe is what the downstream driver does and I am not really happy with it. A perfectly fine frame is sacrificed in a buffer starvation situation. The approach in the patch series at hand follows the example in the rkisp1 driver, which should be a good reference. >> + >> + buffer = rkcif_stream_pop_buffer(stream); >> + if (buffer) { >> + stream->buffers[stream->frame_phase] = buffer; >> + stream->buffers[stream->frame_phase]->is_dummy = false; >> + } else { >> + stream->buffers[stream->frame_phase] = &stream->dummy.buffer; >> + stream->buffers[stream->frame_phase]->is_dummy = true; >> + dev_warn(stream->rkcif->dev, >> + "no buffer available, frame will be dropped\n"); > > This warning can quickly flood the kernel logs if the user-space is too slow in > enqueuing the buffers. True. dev_warn_ratelimited(...)? > >> + } >> + >> + if (stream->queue_buffer) >> + stream->queue_buffer(stream, stream->frame_phase); > > is this if statement really needed ? I find it good practice to check the callbacks before calling them. But this is a matter of taste, of course. > >> + >> + stream->frame_phase = 1 - stream->frame_phase; >> +} >> + >> +static int rkcif_stream_init_buffers(struct rkcif_stream *stream) >> +{ >> + struct v4l2_pix_format_mplane *pix = &stream->pix; >> + int i; >> + >> + stream->buffers[0] = rkcif_stream_pop_buffer(stream); >> + if (!stream->buffers[0]) >> + goto err_buff_0; >> + >> + stream->buffers[1] = rkcif_stream_pop_buffer(stream); >> + if (!stream->buffers[1]) >> + goto err_buff_1; >> + >> + if (stream->queue_buffer) { >> + stream->queue_buffer(stream, 0); >> + stream->queue_buffer(stream, 1); >> + } >> + >> + stream->dummy.size = pix->num_planes * pix->plane_fmt[0].sizeimage; >> + stream->dummy.vaddr = >> + dma_alloc_attrs(stream->rkcif->dev, stream->dummy.size, >> + &stream->dummy.buffer.buff_addr[0], GFP_KERNEL, >> + DMA_ATTR_NO_KERNEL_MAPPING); >> + if (!stream->dummy.vaddr) >> + goto err_dummy; >> + >> + for (i = 1; i < pix->num_planes; i++) >> + stream->dummy.buffer.buff_addr[i] = >> + stream->dummy.buffer.buff_addr[i - 1] + >> + pix->plane_fmt[i - 1].bytesperline * pix->height; >> + >> + return 0; >> + >> +err_dummy: >> + rkcif_stream_return_buffer(stream->buffers[1], VB2_BUF_STATE_QUEUED); >> + stream->buffers[1] = NULL; >> + >> +err_buff_1: >> + rkcif_stream_return_buffer(stream->buffers[0], VB2_BUF_STATE_QUEUED); >> + stream->buffers[0] = NULL; >> +err_buff_0: >> + return -EINVAL; >> +} >> + > > SNIP > >> +static int rkcif_stream_init_vb2_queue(struct vb2_queue *q, >> + struct rkcif_stream *stream) >> +{ >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> + q->io_modes = VB2_MMAP | VB2_DMABUF; >> + q->drv_priv = stream; >> + q->ops = &rkcif_stream_vb2_ops; >> + q->mem_ops = &vb2_dma_contig_memops; >> + q->buf_struct_size = sizeof(struct rkcif_buffer); >> + q->min_queued_buffers = CIF_REQ_BUFS_MIN; > > If I recall correctly min_queued_buffers should be the strict minimum > number of buffers you need to start streaming. So in this case it should > be 3 = 2 pingpong buffers + 1 dummy buffer. The dummy buffer is allocated separately and does not need to be accounted for. Two pingpong buffers is what the hardware can queue, but in practice, to start (and, above all, keep on) streaming you'll need more. > VIDIOC_REQBUFS will allocate min_queued_buffers + 1 and user-space will > probably allocate even more anyway. Is that so? I found that user space relies too much on this minimum buffer count and experienced several buffer starvation situations because kernel AND user space were to cheap in terms of buffer count. Maybe 8 is too many, but in practice four buffers are required at least for a decent 2160p stream (one ready for DMA write, one ongoing DMA write, one stable for processing (maybe DRM scanout or whatever the application is), one spare). I am open to suggestions but please keep real life situations in mind and move away from theoretical stand-alone-capture-hw setups. Thanks and best regards, Michael > >> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> + q->lock = &stream->vlock; >> + q->dev = stream->rkcif->dev; >> + >> + return vb2_queue_init(q); >> +} > > -- > Kind Regards > Mehdi Djait
On Tue, May 06, 2025 at 08:39:31PM +0200, Michael Riesch wrote: > On 5/2/25 16:35, Laurent Pinchart wrote: > > On Fri, May 02, 2025 at 04:19:59PM +0200, Michael Riesch wrote: > >> On 5/2/25 15:31, Laurent Pinchart wrote: > >>> On Wed, Apr 30, 2025 at 11:15:56AM +0200, Michael Riesch via B4 Relay wrote: > >>>> From: Michael Riesch <michael.riesch@wolfvision.net> > >>>> > >>>> The Rockchip RK3568 MIPI CSI-2 Receiver is a CSI-2 bridge with one > >>>> input port and one output port. It receives the data with the help > >>>> of an external MIPI PHY (C-PHY or D-PHY) and passes it to the > >>>> Rockchip RK3568 Video Capture (VICAP) block. > >>>> > >>>> Add a V4L2 subdevice driver for this unit. > >>>> > >>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> > >>>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com> > >>>> --- > >>>> drivers/media/platform/rockchip/rkcif/Makefile | 3 + > >>>> .../rockchip/rkcif/rkcif-mipi-csi-receiver.c | 731 +++++++++++++++++++++ > >>>> 2 files changed, 734 insertions(+) > >>>> > >>>> diff --git a/drivers/media/platform/rockchip/rkcif/Makefile b/drivers/media/platform/rockchip/rkcif/Makefile > >>>> index 818424972c7b..a5c18a45c213 100644 > >>>> --- a/drivers/media/platform/rockchip/rkcif/Makefile > >>>> +++ b/drivers/media/platform/rockchip/rkcif/Makefile > >>>> @@ -5,3 +5,6 @@ rockchip-cif-objs += rkcif-dev.o \ > >>>> rkcif-capture-mipi.o \ > >>>> rkcif-interface.o \ > >>>> rkcif-stream.o > >>>> + > >>>> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip-mipi-csi.o > >>>> +rockchip-mipi-csi-objs += rkcif-mipi-csi-receiver.o > >>>> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c > >>>> new file mode 100644 > >>>> index 000000000000..81489f70490f > >>>> --- /dev/null > >>>> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-receiver.c > >>>> @@ -0,0 +1,731 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * Rockchip MIPI CSI-2 Receiver Driver > >>>> + * > >>>> + * Copyright (C) 2019 Rockchip Electronics Co., Ltd. > >>>> + * Copyright (C) 2025 Michael Riesch <michael.riesch@wolfvision.net> > >>>> + */ > >>>> + > >>>> +#include <linux/clk.h> > >>>> +#include <linux/delay.h> > >>>> +#include <linux/io.h> > >>>> +#include <linux/module.h> > >>>> +#include <linux/of.h> > >>>> +#include <linux/of_graph.h> > >>>> +#include <linux/of_platform.h> > >>>> +#include <linux/phy/phy.h> > >>>> +#include <linux/platform_device.h> > >>>> +#include <linux/pm_runtime.h> > >>>> +#include <linux/reset.h> > >>>> + > >>>> +#include <media/mipi-csi2.h> > >>>> +#include <media/v4l2-ctrls.h> > >>>> +#include <media/v4l2-fwnode.h> > >>>> +#include <media/v4l2-subdev.h> > >>>> + > >>>> +#define CSI2HOST_N_LANES 0x04 > >>>> +#define CSI2HOST_CSI2_RESETN 0x10 > >>>> +#define CSI2HOST_PHY_STATE 0x14 > >>>> +#define CSI2HOST_ERR1 0x20 > >>>> +#define CSI2HOST_ERR2 0x24 > >>>> +#define CSI2HOST_MSK1 0x28 > >>>> +#define CSI2HOST_MSK2 0x2c > >>>> +#define CSI2HOST_CONTROL 0x40 > >>> > >>> I'm trying to get to the bottom of the CSI-2 RX integration questions > >>> for the RK3568. Some of the registers here seem to the CSI2RX_1C00 block > >>> as documented starting on page 1059 of the RK3568 TRM (revision 1.1), > >>> but they're not an exact match. The control register, in particular, > >>> doesn't match at all. Where is this CSI-2 receiver documented in the TRM > >>> ? > >> > >> That would be in Chapter 27 of the RK3568 TRM: "MIPI CSI HOST" > >> > >> The registers are at 0xfdfb0000 "CSI_RX_CTRL1", cf. Chapter 1 "System > >> Overview". > >> > >> Naturally, this is quite confusing, as there is a "CSI2RX" whose > >> registers are embedded in the ISP register map. > >> > >> "MIPI CSI HOST" and "CSI2RX" seem to be different IP cores but perform > >> the same job in principle. CSI2RX seems to have additional features > >> related to writing raw data into memory and to HDR, though. > >> > >> Hope that helps! > > > > It does, thank you. I wonder how I missed that. > > > > The IP seems very similar to other CSI-2 receivers already supported > > upstream, see drivers/media/platform/raspberrypi/rp1-cfe/dphy.c, > > drivers/media/platform/renesas/rcar-csi2.c and > > drivers/staging/media/imx/imx6-mipi-csi2.c. Those drivers combine > > support for the CSI-2 RX and D-PHY, explaining some of the differences > > (and in the case of the rcar-csi2 driver, I think it also combines > > support for different CSI-2 RX in a single driver). > > Before I started writing the driver under discussion, I did scan the > existing drivers, but could not see any similarities to rcar-csi2 and > rp1-cfe/dphy.c (and I still don't see them), respectively (but maybe > those two are the same). > Unfortunately, I forgot to look into staging. Hm. Indeed it resembles > the imx6-mipi-csi2.c driver, at least judging by the register layout. > > According to Kever, while a Synopsys IP core was used in older SoCs, the > newer SoCs feature an IP core developed by Rockchip. Probably the > register map was recycled but slightly modified. Let's keep this driver Rockchip-specific then. Thanks for checking. > > Is there something we could do to avoid adding a 4th driver for the same > > IP core family (from Synopsys or Cadence I assume) ? It could possibly > > be done on top of this series to avoid delaying VICAP support, but I'd > > really like to get this sorted out. > > Based on my current level of knowledge I wouldn't be comfortable with > placing this driver in media/platform/synopsys/something. If you all > advise me to do so, I will comply, though. > > I am not too keen on digging out a quite dated staging driver and basing > the current efforts on that one. > > We could, however, move this driver to > media/platform/rockchip/mipi-csi-receiver/ (name to be bikeshedded, open > to suggestions). It is a separate kernel module already anyway, and > maybe this way it is more visible to the next person developing a driver > for a similar IP core. Sound reasonable? Sounds good to me. > >>>> + > >>>> +#define SW_CPHY_EN(x) ((x) << 0) > >>>> +#define SW_DSI_EN(x) ((x) << 4) > >>>> +#define SW_DATATYPE_FS(x) ((x) << 8) > >>>> +#define SW_DATATYPE_FE(x) ((x) << 14) > >>>> +#define SW_DATATYPE_LS(x) ((x) << 20) > >>>> +#define SW_DATATYPE_LE(x) ((x) << 26) > >>>> + > >>>> +#define RKCIF_CSI_CLKS_MAX 1 > >>>> + > >>>> +enum { > >>>> + RKCIF_CSI_PAD_SINK, > >>>> + RKCIF_CSI_PAD_SRC, > >>>> + RKCIF_CSI_PAD_MAX, > >>>> +}; > >>>> + > >>>> +struct rkcif_csi_format { > >>>> + u32 code; > >>>> + u8 depth; > >>>> + u8 csi_dt; > >>>> +}; > >>>> + > >>>> +struct rkcif_csi_device { > >>>> + struct device *dev; > >>>> + > >>>> + void __iomem *base_addr; > >>>> + struct clk_bulk_data *clks; > >>>> + unsigned int clks_num; > >>>> + struct phy *phy; > >>>> + struct reset_control *reset; > >>>> + > >>>> + const struct rkcif_csi_format *formats; > >>>> + unsigned int formats_num; > >>>> + > >>>> + struct media_pad pads[RKCIF_CSI_PAD_MAX]; > >>>> + struct v4l2_async_notifier notifier; > >>>> + struct v4l2_fwnode_endpoint vep; > >>>> + struct v4l2_subdev sd; > >>>> + > >>>> + struct v4l2_subdev *source_sd; > >>>> + u32 source_pad; > >>>> +}; > >>>> + > >>>> +static const struct v4l2_mbus_framefmt default_format = { > >>>> + .width = 3840, > >>>> + .height = 2160, > >>>> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, > >>>> + .field = V4L2_FIELD_NONE, > >>>> + .colorspace = V4L2_COLORSPACE_RAW, > >>>> + .ycbcr_enc = V4L2_YCBCR_ENC_601, > >>>> + .quantization = V4L2_QUANTIZATION_FULL_RANGE, > >>>> + .xfer_func = V4L2_XFER_FUNC_NONE, > >>>> +}; > >>>> + > >>>> +static const struct rkcif_csi_format formats[] = { > >>>> + /* YUV formats */ > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_YUYV8_1X16, > >>>> + .depth = 16, > >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_UYVY8_1X16, > >>>> + .depth = 16, > >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_YVYU8_1X16, > >>>> + .depth = 16, > >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_VYUY8_1X16, > >>>> + .depth = 16, > >>>> + .csi_dt = MIPI_CSI2_DT_YUV422_8B, > >>>> + }, > >>>> + /* RGB formats */ > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_RGB888_1X24, > >>>> + .depth = 24, > >>>> + .csi_dt = MIPI_CSI2_DT_RGB888, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_BGR888_1X24, > >>>> + .depth = 24, > >>>> + .csi_dt = MIPI_CSI2_DT_RGB888, > >>>> + }, > >>>> + /* Bayer formats */ > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > >>>> + .depth = 8, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SGBRG8_1X8, > >>>> + .depth = 8, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SGRBG8_1X8, > >>>> + .depth = 8, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SRGGB8_1X8, > >>>> + .depth = 8, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW8, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SBGGR10_1X10, > >>>> + .depth = 10, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SGBRG10_1X10, > >>>> + .depth = 10, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SGRBG10_1X10, > >>>> + .depth = 10, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SRGGB10_1X10, > >>>> + .depth = 10, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW10, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SBGGR12_1X12, > >>>> + .depth = 12, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SGBRG12_1X12, > >>>> + .depth = 12, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SGRBG12_1X12, > >>>> + .depth = 12, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, > >>>> + }, > >>>> + { > >>>> + .code = MEDIA_BUS_FMT_SRGGB12_1X12, > >>>> + .depth = 12, > >>>> + .csi_dt = MIPI_CSI2_DT_RAW12, > >>>> + }, > >>>> +}; > >>>> + > >>>> +static inline struct rkcif_csi_device *to_rkcif_csi(struct v4l2_subdev *sd) > >>>> +{ > >>>> + return container_of(sd, struct rkcif_csi_device, sd); > >>>> +} > >>>> + > >>>> +static inline __maybe_unused void > >>>> +rkcif_csi_write(struct rkcif_csi_device *csi_dev, unsigned int addr, u32 val) > >>>> +{ > >>>> + writel(val, csi_dev->base_addr + addr); > >>>> +} > >>>> + > >>>> +static inline __maybe_unused u32 > >>>> +rkcif_csi_read(struct rkcif_csi_device *csi_dev, unsigned int addr) > >>>> +{ > >>>> + return readl(csi_dev->base_addr + addr); > >>>> +} > >>>> + > >>>> +static const struct rkcif_csi_format * > >>>> +rkcif_csi_find_format(struct rkcif_csi_device *csi_dev, u32 mbus_code) > >>>> +{ > >>>> + const struct rkcif_csi_format *format; > >>>> + > >>>> + WARN_ON(csi_dev->formats_num == 0); > >>>> + > >>>> + for (int i = 0; i < csi_dev->formats_num; i++) { > >>>> + format = &csi_dev->formats[i]; > >>>> + if (format->code == mbus_code) > >>>> + return format; > >>>> + } > >>>> + > >>>> + return NULL; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_start(struct rkcif_csi_device *csi_dev) > >>>> +{ > >>>> + enum v4l2_mbus_type bus_type = csi_dev->vep.bus_type; > >>>> + union phy_configure_opts opts; > >>>> + s64 link_freq; > >>>> + u32 lanes = csi_dev->vep.bus.mipi_csi2.num_data_lanes; > >>>> + u32 control = 0; > >>>> + > >>>> + if (lanes < 1 || lanes > 4) > >>>> + return -EINVAL; > >>>> + > >>>> + /* set mult and div to 0, thus completely rely on V4L2_CID_LINK_FREQ */ > >>>> + link_freq = v4l2_get_link_freq(csi_dev->source_sd->ctrl_handler, 0, 0); > >>>> + if (link_freq <= 0) > >>>> + return -EINVAL; > >>>> + > >>>> + if (bus_type == V4L2_MBUS_CSI2_DPHY) { > >>>> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > >>>> + > >>>> + phy_mipi_dphy_get_default_config_for_hsclk(link_freq * 2, lanes, > >>>> + cfg); > >>>> + phy_set_mode(csi_dev->phy, PHY_MODE_MIPI_DPHY); > >>>> + phy_configure(csi_dev->phy, &opts); > >>>> + > >>>> + control |= SW_CPHY_EN(0); > >>>> + > >>>> + } else if (bus_type == V4L2_MBUS_CSI2_CPHY) { > >>>> + control |= SW_CPHY_EN(1); > >>>> + > >>>> + /* TODO: implement CPHY configuration */ > >>>> + } else { > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + control |= SW_DATATYPE_FS(0x00) | SW_DATATYPE_FE(0x01) | > >>>> + SW_DATATYPE_LS(0x02) | SW_DATATYPE_LE(0x03); > >>>> + > >>>> + rkcif_csi_write(csi_dev, CSI2HOST_N_LANES, lanes - 1); > >>>> + rkcif_csi_write(csi_dev, CSI2HOST_CONTROL, control); > >>>> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 1); > >>>> + > >>>> + phy_power_on(csi_dev->phy); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static void rkcif_csi_stop(struct rkcif_csi_device *csi_dev) > >>>> +{ > >>>> + phy_power_off(csi_dev->phy); > >>>> + > >>>> + rkcif_csi_write(csi_dev, CSI2HOST_CSI2_RESETN, 0); > >>>> + rkcif_csi_write(csi_dev, CSI2HOST_MSK1, ~0); > >>>> + rkcif_csi_write(csi_dev, CSI2HOST_MSK2, ~0); > >>>> +} > >>>> + > >>>> +static const struct media_entity_operations rkcif_csi_media_ops = { > >>>> + .link_validate = v4l2_subdev_link_validate, > >>>> +}; > >>>> + > >>>> +static int rkcif_csi_enum_mbus_code(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_state *sd_state, > >>>> + struct v4l2_subdev_mbus_code_enum *code) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >>>> + > >>>> + if (code->pad == RKCIF_CSI_PAD_SRC) { > >>>> + const struct v4l2_mbus_framefmt *sink_fmt; > >>>> + > >>>> + if (code->index) > >>>> + return -EINVAL; > >>>> + > >>>> + sink_fmt = v4l2_subdev_state_get_format(sd_state, > >>>> + RKCIF_CSI_PAD_SINK); > >>>> + code->code = sink_fmt->code; > >>>> + > >>>> + return 0; > >>>> + } else if (code->pad == RKCIF_CSI_PAD_SINK) { > >>>> + if (code->index > csi_dev->formats_num) > >>>> + return -EINVAL; > >>>> + > >>>> + code->code = csi_dev->formats[code->index].code; > >>>> + return 0; > >>>> + } > >>>> + > >>>> + return -EINVAL; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_set_fmt(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_state *state, > >>>> + struct v4l2_subdev_format *format) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >>>> + const struct rkcif_csi_format *fmt; > >>>> + struct v4l2_mbus_framefmt *sink, *src; > >>>> + > >>>> + /* the format on the source pad always matches the sink pad */ > >>>> + if (format->pad == RKCIF_CSI_PAD_SRC) > >>>> + return v4l2_subdev_get_fmt(sd, state, format); > >>>> + > >>>> + sink = v4l2_subdev_state_get_format(state, format->pad, format->stream); > >>>> + if (!sink) > >>>> + return -EINVAL; > >>>> + > >>>> + fmt = rkcif_csi_find_format(csi_dev, format->format.code); > >>>> + if (fmt) > >>>> + *sink = format->format; > >>>> + else > >>>> + *sink = default_format; > >>>> + > >>>> + /* propagate the format to the source pad */ > >>>> + src = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, > >>>> + format->stream); > >>>> + if (!src) > >>>> + return -EINVAL; > >>>> + > >>>> + *src = *sink; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_set_routing(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_state *state, > >>>> + enum v4l2_subdev_format_whence which, > >>>> + struct v4l2_subdev_krouting *routing) > >>>> +{ > >>>> + int ret; > >>>> + > >>>> + ret = v4l2_subdev_routing_validate(sd, routing, > >>>> + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, > >>>> + &default_format); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_enable_streams(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_state *state, u32 pad, > >>>> + u64 streams_mask) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >>>> + struct v4l2_subdev *remote_sd; > >>>> + struct media_pad *sink_pad, *remote_pad; > >>>> + struct device *dev = csi_dev->dev; > >>>> + u64 mask; > >>>> + int ret; > >>>> + > >>>> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; > >>>> + remote_pad = media_pad_remote_pad_first(sink_pad); > >>>> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); > >>>> + > >>>> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, > >>>> + RKCIF_CSI_PAD_SRC, > >>>> + &streams_mask); > >>>> + > >>>> + ret = pm_runtime_resume_and_get(dev); > >>>> + if (ret) > >>>> + goto err; > >>>> + > >>>> + ret = rkcif_csi_start(csi_dev); > >>>> + if (ret) { > >>>> + dev_err(dev, "failed to enable CSI hardware\n"); > >>>> + goto err_pm_runtime_put; > >>>> + } > >>>> + > >>>> + ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index, mask); > >>>> + if (ret) > >>>> + goto err_csi_stop; > >>>> + > >>>> + return 0; > >>>> + > >>>> +err_csi_stop: > >>>> + rkcif_csi_stop(csi_dev); > >>>> +err_pm_runtime_put: > >>>> + pm_runtime_put_sync(dev); > >>>> +err: > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_disable_streams(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_state *state, u32 pad, > >>>> + u64 streams_mask) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = to_rkcif_csi(sd); > >>>> + struct v4l2_subdev *remote_sd; > >>>> + struct media_pad *sink_pad, *remote_pad; > >>>> + struct device *dev = csi_dev->dev; > >>>> + u64 mask; > >>>> + int ret; > >>>> + > >>>> + sink_pad = &sd->entity.pads[RKCIF_CSI_PAD_SINK]; > >>>> + remote_pad = media_pad_remote_pad_first(sink_pad); > >>>> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity); > >>>> + > >>>> + mask = v4l2_subdev_state_xlate_streams(state, RKCIF_CSI_PAD_SINK, > >>>> + RKCIF_CSI_PAD_SRC, > >>>> + &streams_mask); > >>>> + > >>>> + ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask); > >>>> + > >>>> + rkcif_csi_stop(csi_dev); > >>>> + > >>>> + pm_runtime_mark_last_busy(dev); > >>>> + pm_runtime_put_autosuspend(dev); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static const struct v4l2_subdev_pad_ops rkcif_csi_pad_ops = { > >>>> + .enum_mbus_code = rkcif_csi_enum_mbus_code, > >>>> + .get_fmt = v4l2_subdev_get_fmt, > >>>> + .set_fmt = rkcif_csi_set_fmt, > >>>> + .set_routing = rkcif_csi_set_routing, > >>>> + .enable_streams = rkcif_csi_enable_streams, > >>>> + .disable_streams = rkcif_csi_disable_streams, > >>>> +}; > >>>> + > >>>> +static const struct v4l2_subdev_ops rkcif_csi_ops = { > >>>> + .pad = &rkcif_csi_pad_ops, > >>>> +}; > >>>> + > >>>> +static int rkcif_csi_init_state(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_state *state) > >>>> +{ > >>>> + struct v4l2_subdev_route routes[] = { > >>>> + { > >>>> + .sink_pad = RKCIF_CSI_PAD_SINK, > >>>> + .sink_stream = 0, > >>>> + .source_pad = RKCIF_CSI_PAD_SRC, > >>>> + .source_stream = 0, > >>>> + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > >>>> + }, > >>>> + }; > >>>> + struct v4l2_subdev_krouting routing = { > >>>> + .len_routes = ARRAY_SIZE(routes), > >>>> + .num_routes = ARRAY_SIZE(routes), > >>>> + .routes = routes, > >>>> + }; > >>>> + int ret; > >>>> + > >>>> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing, > >>>> + &default_format); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static const struct v4l2_subdev_internal_ops rkcif_csi_internal_ops = { > >>>> + .init_state = rkcif_csi_init_state, > >>>> +}; > >>>> + > >>>> +static int rkcif_csi_notifier_bound(struct v4l2_async_notifier *notifier, > >>>> + struct v4l2_subdev *sd, > >>>> + struct v4l2_async_connection *asd) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = > >>>> + container_of(notifier, struct rkcif_csi_device, notifier); > >>>> + int source_pad; > >>>> + > >>>> + source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, > >>>> + MEDIA_PAD_FL_SOURCE); > >>>> + if (source_pad < 0) { > >>>> + dev_err(csi_dev->dev, "failed to find source pad for %s\n", > >>>> + sd->name); > >>>> + return source_pad; > >>>> + } > >>>> + > >>>> + csi_dev->source_sd = sd; > >>>> + csi_dev->source_pad = source_pad; > >>>> + > >>>> + return media_create_pad_link(&sd->entity, source_pad, > >>>> + &csi_dev->sd.entity, RKCIF_CSI_PAD_SINK, > >>>> + MEDIA_LNK_FL_ENABLED); > >>>> +} > >>>> + > >>>> +static const struct v4l2_async_notifier_operations rkcif_csi_notifier_ops = { > >>>> + .bound = rkcif_csi_notifier_bound, > >>>> +}; > >>>> + > >>>> +static int rkcif_csi_register_notifier(struct rkcif_csi_device *csi_dev) > >>>> +{ > >>>> + struct v4l2_async_connection *asd; > >>>> + struct v4l2_async_notifier *ntf = &csi_dev->notifier; > >>>> + struct v4l2_fwnode_endpoint *vep = &csi_dev->vep; > >>>> + struct v4l2_subdev *sd = &csi_dev->sd; > >>>> + struct device *dev = csi_dev->dev; > >>>> + struct fwnode_handle *ep; > >>>> + int ret = 0; > >>>> + > >>>> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > >>>> + if (!ep) > >>>> + return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n"); > >>>> + > >>>> + vep->bus_type = V4L2_MBUS_UNKNOWN; > >>>> + ret = v4l2_fwnode_endpoint_parse(ep, vep); > >>>> + if (ret) { > >>>> + ret = dev_err_probe(dev, ret, "failed to parse endpoint\n"); > >>>> + goto out; > >>>> + } > >>>> + > >>>> + if (vep->bus_type != V4L2_MBUS_CSI2_DPHY && > >>>> + vep->bus_type != V4L2_MBUS_CSI2_CPHY) { > >>>> + ret = dev_err_probe(dev, -EINVAL, > >>>> + "invalid bus type of endpoint\n"); > >>>> + goto out; > >>>> + } > >>>> + > >>>> + v4l2_async_subdev_nf_init(ntf, sd); > >>>> + ntf->ops = &rkcif_csi_notifier_ops; > >>>> + > >>>> + asd = v4l2_async_nf_add_fwnode_remote(ntf, ep, > >>>> + struct v4l2_async_connection); > >>>> + if (IS_ERR(asd)) { > >>>> + ret = PTR_ERR(asd); > >>>> + goto err_nf_cleanup; > >>>> + } > >>>> + > >>>> + ret = v4l2_async_nf_register(ntf); > >>>> + if (ret) { > >>>> + ret = dev_err_probe(dev, ret, "failed to register notifier\n"); > >>>> + goto err_nf_cleanup; > >>>> + } > >>>> + > >>>> + goto out; > >>>> + > >>>> +err_nf_cleanup: > >>>> + v4l2_async_nf_cleanup(ntf); > >>>> +out: > >>>> + fwnode_handle_put(ep); > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_register(struct rkcif_csi_device *csi_dev) > >>>> +{ > >>>> + struct media_pad *pads = csi_dev->pads; > >>>> + struct v4l2_subdev *sd = &csi_dev->sd; > >>>> + int ret; > >>>> + > >>>> + ret = rkcif_csi_register_notifier(csi_dev); > >>>> + if (ret) > >>>> + goto err; > >>>> + > >>>> + v4l2_subdev_init(sd, &rkcif_csi_ops); > >>>> + sd->dev = csi_dev->dev; > >>>> + sd->entity.ops = &rkcif_csi_media_ops; > >>>> + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > >>>> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > >>>> + sd->internal_ops = &rkcif_csi_internal_ops; > >>>> + sd->owner = THIS_MODULE; > >>>> + snprintf(sd->name, sizeof(sd->name), "rockchip-mipi-csi %s", > >>>> + dev_name(csi_dev->dev)); > >>>> + > >>>> + pads[RKCIF_CSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK | > >>>> + MEDIA_PAD_FL_MUST_CONNECT; > >>>> + pads[RKCIF_CSI_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE; > >>>> + ret = media_entity_pads_init(&sd->entity, RKCIF_CSI_PAD_MAX, pads); > >>>> + if (ret) > >>>> + goto err_notifier_unregister; > >>>> + > >>>> + ret = v4l2_subdev_init_finalize(sd); > >>>> + if (ret) > >>>> + goto err_entity_cleanup; > >>>> + > >>>> + ret = v4l2_async_register_subdev(sd); > >>>> + if (ret) { > >>>> + dev_err(sd->dev, "failed to register CSI subdev\n"); > >>>> + goto err_subdev_cleanup; > >>>> + } > >>>> + > >>>> + return 0; > >>>> + > >>>> +err_subdev_cleanup: > >>>> + v4l2_subdev_cleanup(sd); > >>>> +err_entity_cleanup: > >>>> + media_entity_cleanup(&sd->entity); > >>>> +err_notifier_unregister: > >>>> + v4l2_async_nf_unregister(&csi_dev->notifier); > >>>> + v4l2_async_nf_cleanup(&csi_dev->notifier); > >>>> +err: > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static void rkcif_csi_unregister(struct rkcif_csi_device *csi_dev) > >>>> +{ > >>>> + struct v4l2_subdev *sd = &csi_dev->sd; > >>>> + > >>>> + v4l2_async_unregister_subdev(sd); > >>>> + v4l2_subdev_cleanup(sd); > >>>> + media_entity_cleanup(&sd->entity); > >>>> + v4l2_async_nf_unregister(&csi_dev->notifier); > >>>> + v4l2_async_nf_cleanup(&csi_dev->notifier); > >>>> +} > >>>> + > >>>> +static const struct of_device_id rkcif_csi_of_match[] = { > >>>> + { > >>>> + .compatible = "rockchip,rk3568-mipi-csi", > >>>> + }, > >>>> + {} > >>>> +}; > >>>> +MODULE_DEVICE_TABLE(of, rkcif_csi_of_match); > >>>> + > >>>> +static int rkcif_csi_probe(struct platform_device *pdev) > >>>> +{ > >>>> + struct device *dev = &pdev->dev; > >>>> + struct rkcif_csi_device *csi_dev; > >>>> + int ret; > >>>> + > >>>> + csi_dev = devm_kzalloc(dev, sizeof(*csi_dev), GFP_KERNEL); > >>>> + if (!csi_dev) > >>>> + return -ENOMEM; > >>>> + csi_dev->dev = dev; > >>>> + dev_set_drvdata(dev, csi_dev); > >>>> + > >>>> + csi_dev->base_addr = devm_platform_ioremap_resource(pdev, 0); > >>>> + if (IS_ERR(csi_dev->base_addr)) > >>>> + return PTR_ERR(csi_dev->base_addr); > >>>> + > >>>> + ret = devm_clk_bulk_get_all(dev, &csi_dev->clks); > >>>> + if (ret != RKCIF_CSI_CLKS_MAX) > >>>> + return dev_err_probe(dev, -ENODEV, "failed to get clocks\n"); > >>>> + csi_dev->clks_num = ret; > >>>> + > >>>> + csi_dev->phy = devm_phy_get(dev, NULL); > >>>> + if (IS_ERR(csi_dev->phy)) > >>>> + return dev_err_probe(dev, PTR_ERR(csi_dev->phy), > >>>> + "failed to get MIPI CSI PHY\n"); > >>>> + > >>>> + csi_dev->reset = devm_reset_control_array_get_exclusive(dev); > >>>> + if (IS_ERR(csi_dev->reset)) > >>>> + return dev_err_probe(dev, PTR_ERR(csi_dev->reset), > >>>> + "failed to get reset\n"); > >>>> + > >>>> + csi_dev->formats = formats; > >>>> + csi_dev->formats_num = ARRAY_SIZE(formats); > >>>> + > >>>> + pm_runtime_enable(dev); > >>>> + > >>>> + ret = phy_init(csi_dev->phy); > >>>> + if (ret) { > >>>> + ret = dev_err_probe(dev, ret, > >>>> + "failed to initialize MIPI CSI PHY\n"); > >>>> + goto err_pm_runtime_disable; > >>>> + } > >>>> + > >>>> + ret = rkcif_csi_register(csi_dev); > >>>> + if (ret) > >>>> + goto err_phy_exit; > >>>> + > >>>> + return 0; > >>>> + > >>>> +err_phy_exit: > >>>> + phy_exit(csi_dev->phy); > >>>> +err_pm_runtime_disable: > >>>> + pm_runtime_disable(dev); > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static void rkcif_csi_remove(struct platform_device *pdev) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = platform_get_drvdata(pdev); > >>>> + struct device *dev = &pdev->dev; > >>>> + > >>>> + rkcif_csi_unregister(csi_dev); > >>>> + phy_exit(csi_dev->phy); > >>>> + pm_runtime_disable(dev); > >>>> +} > >>>> + > >>>> +static int rkcif_csi_runtime_suspend(struct device *dev) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); > >>>> + > >>>> + clk_bulk_disable_unprepare(csi_dev->clks_num, csi_dev->clks); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int rkcif_csi_runtime_resume(struct device *dev) > >>>> +{ > >>>> + struct rkcif_csi_device *csi_dev = dev_get_drvdata(dev); > >>>> + int ret; > >>>> + > >>>> + reset_control_assert(csi_dev->reset); > >>>> + udelay(5); > >>>> + reset_control_deassert(csi_dev->reset); > >>>> + > >>>> + ret = clk_bulk_prepare_enable(csi_dev->clks_num, csi_dev->clks); > >>>> + if (ret) { > >>>> + dev_err(dev, "failed to enable clocks\n"); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static const struct dev_pm_ops rkcif_csi_pm_ops = { > >>>> + .runtime_suspend = rkcif_csi_runtime_suspend, > >>>> + .runtime_resume = rkcif_csi_runtime_resume, > >>>> +}; > >>>> + > >>>> +static struct platform_driver rkcif_csi_drv = { > >>>> + .driver = { > >>>> + .name = "rockchip-mipi-csi", > >>>> + .of_match_table = rkcif_csi_of_match, > >>>> + .pm = &rkcif_csi_pm_ops, > >>>> + }, > >>>> + .probe = rkcif_csi_probe, > >>>> + .remove = rkcif_csi_remove, > >>>> +}; > >>>> +module_platform_driver(rkcif_csi_drv); > >>>> + > >>>> +MODULE_DESCRIPTION("Rockchip MIPI CSI-2 Receiver platform driver"); > >>>> +MODULE_LICENSE("GPL");
Hi Michael, On Tue, May 06, 2025 at 10:32:59PM +0200, Michael Riesch wrote: > Hi Mehdi, > > Thanks for your review! > > On 5/6/25 12:37, Mehdi Djait wrote: > > Hi Michael, > > > > Thank you for the patch! > > > > Is it possible to sent the v4l2-compliance output in the next version ? > > > > On Wed, Apr 30, 2025 at 11:15:55AM +0200, Michael Riesch via B4 Relay wrote: > >> From: Michael Riesch <michael.riesch@wolfvision.net> > >> > > > > SNIP > > > >> +irqreturn_t rkcif_dvp_isr(int irq, void *ctx) > >> +{ > >> + struct device *dev = ctx; > >> + struct rkcif_device *rkcif = dev_get_drvdata(dev); > >> + struct rkcif_stream *stream; > >> + u32 intstat, lastline, lastpix, cif_frmst; > >> + irqreturn_t ret = IRQ_NONE; > >> + > >> + if (!rkcif->match_data->dvp) > >> + return ret; > >> + > >> + intstat = cif_dvp_read(rkcif, RKCIF_DVP_INTSTAT); > >> + cif_frmst = cif_dvp_read(rkcif, RKCIF_DVP_FRAME_STATUS); > >> + lastline = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_LINE)); > >> + lastpix = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_PIX)); > >> + > >> + if (intstat & RKCIF_INTSTAT_FRAME_END) { > >> + cif_dvp_write(rkcif, RKCIF_DVP_INTSTAT, > >> + RKCIF_INTSTAT_FRAME_END_CLR | > >> + RKCIF_INTSTAT_LINE_END_CLR); > >> + > >> + stream = &rkcif->interfaces[RKCIF_DVP].streams[RKCIF_ID0]; > >> + > >> + if (stream->stopping) { > >> + cif_dvp_stop_streaming(stream); > >> + wake_up(&stream->wq_stopped); > >> + return IRQ_HANDLED; > >> + } > >> + > >> + if (lastline != stream->pix.height) { > >> + v4l2_err(&rkcif->v4l2_dev, > >> + "bad frame, irq:%#x frmst:%#x size:%dx%d\n", > >> + intstat, cif_frmst, lastpix, lastline); > >> + > >> + cif_dvp_reset_stream(rkcif); > >> + } > >> + > >> + rkcif_stream_pingpong(stream); > >> + > >> + ret = IRQ_HANDLED; > > > > just return IRQ_HANDLED like above ? > > I think I'll go along Bryan's suggestion to make it more consistent. > > > > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +int rkcif_dvp_register(struct rkcif_device *rkcif) > >> +{ > >> + struct rkcif_interface *interface; > >> + int ret, i; > >> + > >> + if (!rkcif->match_data->dvp) > >> + return 0; > >> + > >> + interface = &rkcif->interfaces[RKCIF_DVP]; > >> + interface->index = RKCIF_DVP; > >> + interface->type = RKCIF_IF_DVP; > >> + interface->in_fmts = rkcif->match_data->dvp->in_fmts; > >> + interface->in_fmts_num = rkcif->match_data->dvp->in_fmts_num; > >> + interface->set_crop = rkcif_dvp_set_crop; > >> + ret = rkcif_interface_register(rkcif, interface); > >> + if (ret) > >> + return 0; > > | > > +-> Copy-paste error ? > > Hm. It's not a mistake. But maybe it is a bit misleading. > > The point here is that if something fails with registering the DVP, the > driver may continue to register other entities, such as the MIPI capture > thing. what if you want to register the DVP interface and it fails ? Maybe two separate function for rkcif_{dvp,mipi}_interface_register(), call one of them based on match_data and verify the ret code --> fail if non-zero ? > > I'll have another look over this mechanism and will try to make it more > comprehensible. > > > > >> + > >> + if (rkcif->match_data->dvp->setup) > >> + rkcif->match_data->dvp->setup(rkcif); > >> + > >> + interface->streams_num = rkcif->match_data->dvp->has_ids ? 4 : 1; > >> + for (i = 0; i < interface->streams_num; i++) { > >> + struct rkcif_stream *stream = &interface->streams[i]; > >> + > >> + stream->id = i; > >> + stream->interface = interface; > >> + stream->out_fmts = rkcif->match_data->dvp->out_fmts; > >> + stream->out_fmts_num = rkcif->match_data->dvp->out_fmts_num; > >> + stream->queue_buffer = cif_dvp_queue_buffer; > >> + stream->start_streaming = cif_dvp_start_streaming; > >> + stream->stop_streaming = cif_dvp_stop_streaming; > >> + > >> + ret = rkcif_stream_register(rkcif, stream); > >> + if (ret) > >> + goto err_streams_unregister; > >> + } > >> + return 0; > >> + > >> +err_streams_unregister: > >> + for (; i >= 0; i--) > >> + rkcif_stream_unregister(&interface->streams[i]); > >> + rkcif_interface_unregister(interface); > >> + > >> + return ret; > >> +} > >> + > > > > SNIP > > > >> +static inline struct rkcif_buffer *to_rkcif_buffer(struct vb2_v4l2_buffer *vb) > >> +{ > >> + return container_of(vb, struct rkcif_buffer, vb); > >> +} > >> + > >> +static inline struct rkcif_stream *to_rkcif_stream(struct video_device *vdev) > >> +{ > >> + return container_of(vdev, struct rkcif_stream, vdev); > >> +} > >> + > >> +static struct rkcif_buffer *rkcif_stream_pop_buffer(struct rkcif_stream *stream) > >> +{ > >> + struct rkcif_buffer *buffer = NULL; > >> + unsigned long lock_flags; > >> + > >> + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); > > > > guard(spinlock_irqsave)(&stream->driver_queue_lock) will simplify this function. > > I'll guard up these methods in v7. > > > > >> + > >> + if (list_empty(&stream->driver_queue)) > >> + goto err_empty; > >> + > >> + buffer = list_first_entry(&stream->driver_queue, struct rkcif_buffer, > >> + queue); > >> + list_del(&buffer->queue); > >> + > >> +err_empty: > >> + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); > >> + return buffer; > >> +} > >> + > >> +static void rkcif_stream_push_buffer(struct rkcif_stream *stream, > >> + struct rkcif_buffer *buffer) > >> +{ > >> + unsigned long lock_flags; > >> + > >> + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); > >> + list_add_tail(&buffer->queue, &stream->driver_queue); > >> + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); > >> +} > >> + > >> +static inline void rkcif_stream_return_buffer(struct rkcif_buffer *buffer, > >> + enum vb2_buffer_state state) > >> +{ > >> + struct vb2_v4l2_buffer *vb = &buffer->vb; > >> + > >> + vb2_buffer_done(&vb->vb2_buf, state); > >> +} > >> + > >> +static void rkcif_stream_complete_buffer(struct rkcif_stream *stream, > >> + struct rkcif_buffer *buffer) > >> +{ > >> + struct vb2_v4l2_buffer *vb = &buffer->vb; > >> + > >> + vb->vb2_buf.timestamp = ktime_get_ns(); > >> + vb->sequence = stream->frame_idx; > >> + vb2_buffer_done(&vb->vb2_buf, VB2_BUF_STATE_DONE); > >> + stream->frame_idx++; > >> +} > >> + > >> +void rkcif_stream_pingpong(struct rkcif_stream *stream) > >> +{ > >> + struct rkcif_buffer *buffer; > >> + > >> + buffer = stream->buffers[stream->frame_phase]; > >> + if (!buffer->is_dummy) > >> + rkcif_stream_complete_buffer(stream, buffer); > > > > You can actually keep this frame dropping mechanism without using the > > dummy buffer. > > > > You can set a drop flag to TRUE: keep overwriting the buffer you already have > > without returning it to user-space until you can get another buffer, set > > the flag again to FALSE and resume returning the buffers to user-space. > > The approach you describe is what the downstream driver does and I am > not really happy with it. A perfectly fine frame is sacrificed in a > buffer starvation situation. Oh I thought the downstream driver does it with the dummy buffer. > > The approach in the patch series at hand follows the example in the > rkisp1 driver, which should be a good reference. Ack. > > >> + > >> + buffer = rkcif_stream_pop_buffer(stream); > >> + if (buffer) { > >> + stream->buffers[stream->frame_phase] = buffer; > >> + stream->buffers[stream->frame_phase]->is_dummy = false; > >> + } else { > >> + stream->buffers[stream->frame_phase] = &stream->dummy.buffer; > >> + stream->buffers[stream->frame_phase]->is_dummy = true; > >> + dev_warn(stream->rkcif->dev, > >> + "no buffer available, frame will be dropped\n"); > > > > This warning can quickly flood the kernel logs if the user-space is too slow in > > enqueuing the buffers. > > True. dev_warn_ratelimited(...)? > Does frame dropping deserve a warning ? If you don't think so, maybe a debug or info ? > > > >> + } > >> + > >> + if (stream->queue_buffer) > >> + stream->queue_buffer(stream, stream->frame_phase); > > > > is this if statement really needed ? > > I find it good practice to check the callbacks before calling them. But > this is a matter of taste, of course. > > > > >> + > >> + stream->frame_phase = 1 - stream->frame_phase; > >> +} > >> + > >> +static int rkcif_stream_init_buffers(struct rkcif_stream *stream) > >> +{ > >> + struct v4l2_pix_format_mplane *pix = &stream->pix; > >> + int i; > >> + > >> + stream->buffers[0] = rkcif_stream_pop_buffer(stream); > >> + if (!stream->buffers[0]) > >> + goto err_buff_0; > >> + > >> + stream->buffers[1] = rkcif_stream_pop_buffer(stream); > >> + if (!stream->buffers[1]) > >> + goto err_buff_1; > >> + > >> + if (stream->queue_buffer) { > >> + stream->queue_buffer(stream, 0); > >> + stream->queue_buffer(stream, 1); > >> + } > >> + > >> + stream->dummy.size = pix->num_planes * pix->plane_fmt[0].sizeimage; > >> + stream->dummy.vaddr = > >> + dma_alloc_attrs(stream->rkcif->dev, stream->dummy.size, > >> + &stream->dummy.buffer.buff_addr[0], GFP_KERNEL, > >> + DMA_ATTR_NO_KERNEL_MAPPING); > >> + if (!stream->dummy.vaddr) > >> + goto err_dummy; > >> + > >> + for (i = 1; i < pix->num_planes; i++) > >> + stream->dummy.buffer.buff_addr[i] = > >> + stream->dummy.buffer.buff_addr[i - 1] + > >> + pix->plane_fmt[i - 1].bytesperline * pix->height; > >> + > >> + return 0; > >> + > >> +err_dummy: > >> + rkcif_stream_return_buffer(stream->buffers[1], VB2_BUF_STATE_QUEUED); > >> + stream->buffers[1] = NULL; > >> + > >> +err_buff_1: > >> + rkcif_stream_return_buffer(stream->buffers[0], VB2_BUF_STATE_QUEUED); > >> + stream->buffers[0] = NULL; > >> +err_buff_0: > >> + return -EINVAL; > >> +} > >> + > > > > SNIP > > > >> +static int rkcif_stream_init_vb2_queue(struct vb2_queue *q, > >> + struct rkcif_stream *stream) > >> +{ > >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > >> + q->io_modes = VB2_MMAP | VB2_DMABUF; > >> + q->drv_priv = stream; > >> + q->ops = &rkcif_stream_vb2_ops; > >> + q->mem_ops = &vb2_dma_contig_memops; > >> + q->buf_struct_size = sizeof(struct rkcif_buffer); > >> + q->min_queued_buffers = CIF_REQ_BUFS_MIN; > > > > If I recall correctly min_queued_buffers should be the strict minimum > > number of buffers you need to start streaming. So in this case it should > > be 3 = 2 pingpong buffers + 1 dummy buffer. > > The dummy buffer is allocated separately and does not need to be > accounted for. > > Two pingpong buffers is what the hardware can queue, but in practice, to > start (and, above all, keep on) streaming you'll need more. > > > VIDIOC_REQBUFS will allocate min_queued_buffers + 1 and user-space will > > probably allocate even more anyway. > > Is that so? I found that user space relies too much on this minimum > buffer count and experienced several buffer starvation situations > because kernel AND user space were to cheap in terms of buffer count. > Maybe 8 is too many, but in practice four buffers are required at least > for a decent 2160p stream (one ready for DMA write, one ongoing DMA > write, one stable for processing (maybe DRM scanout or whatever the > application is), one spare). > > I am open to suggestions but please keep real life situations in mind > and move away from theoretical stand-alone-capture-hw setups. so the documentation says: -------------------------------------------------------------------------- min_queued_buffers is used when a DMA engine cannot be started unless at least this number of buffers have been queued into the driver. -------------------------------------------------------------------------- and: -------------------------------------------------------------------------- VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 buffers will be allocated. -------------------------------------------------------------------------- I also found theses patches: https://lore.kernel.org/linux-media/20231211133251.150999-1-benjamin.gaignard@collabora.com/ https://lore.kernel.org/all/20241007124225.63463-1-jacopo.mondi@ideasonboard.com/ If I understood correctly there is a difference between: - the minimal number of buffers to be allocated with VIDIOC_REQBUFS - the minimal number of buffers to make it possible to start streaming what you are setting is the latter, which means you need 8 buffers to even start streaming which should not be the case for rkcif, which should only need two (of course when using pingpong) what you mentioned with the minimum number of buffers for a decent stream seems to point more towards @min_reqbufs_allocation: -------------------------------------------------------------------------- Drivers can set this if there has to be a certain number of buffers available for the hardware to work effectively. -------------------------------------------------------------------------- of course this being said I am not the expert here so feel free to ask @Laurent @Hans -- Kind Regards Mehdi Djait
Hi Mehdi, On 5/7/25 11:36, Mehdi Djait wrote: > Hi Michael, > > On Tue, May 06, 2025 at 10:32:59PM +0200, Michael Riesch wrote: >> Hi Mehdi, >> >> Thanks for your review! >> >> On 5/6/25 12:37, Mehdi Djait wrote: >>> Hi Michael, >>> >>> Thank you for the patch! >>> >>> Is it possible to sent the v4l2-compliance output in the next version ? Missed that remark. Yes, I'll take care to send the output (maybe as reply to v7, though). >>> >>> On Wed, Apr 30, 2025 at 11:15:55AM +0200, Michael Riesch via B4 Relay wrote: >>>> From: Michael Riesch <michael.riesch@wolfvision.net> >>>> >>> >>> SNIP >>> >>>> +irqreturn_t rkcif_dvp_isr(int irq, void *ctx) >>>> +{ >>>> + struct device *dev = ctx; >>>> + struct rkcif_device *rkcif = dev_get_drvdata(dev); >>>> + struct rkcif_stream *stream; >>>> + u32 intstat, lastline, lastpix, cif_frmst; >>>> + irqreturn_t ret = IRQ_NONE; >>>> + >>>> + if (!rkcif->match_data->dvp) >>>> + return ret; >>>> + >>>> + intstat = cif_dvp_read(rkcif, RKCIF_DVP_INTSTAT); >>>> + cif_frmst = cif_dvp_read(rkcif, RKCIF_DVP_FRAME_STATUS); >>>> + lastline = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_LINE)); >>>> + lastpix = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_PIX)); >>>> + >>>> + if (intstat & RKCIF_INTSTAT_FRAME_END) { >>>> + cif_dvp_write(rkcif, RKCIF_DVP_INTSTAT, >>>> + RKCIF_INTSTAT_FRAME_END_CLR | >>>> + RKCIF_INTSTAT_LINE_END_CLR); >>>> + >>>> + stream = &rkcif->interfaces[RKCIF_DVP].streams[RKCIF_ID0]; >>>> + >>>> + if (stream->stopping) { >>>> + cif_dvp_stop_streaming(stream); >>>> + wake_up(&stream->wq_stopped); >>>> + return IRQ_HANDLED; >>>> + } >>>> + >>>> + if (lastline != stream->pix.height) { >>>> + v4l2_err(&rkcif->v4l2_dev, >>>> + "bad frame, irq:%#x frmst:%#x size:%dx%d\n", >>>> + intstat, cif_frmst, lastpix, lastline); >>>> + >>>> + cif_dvp_reset_stream(rkcif); >>>> + } >>>> + >>>> + rkcif_stream_pingpong(stream); >>>> + >>>> + ret = IRQ_HANDLED; >>> >>> just return IRQ_HANDLED like above ? >> >> I think I'll go along Bryan's suggestion to make it more consistent. >> >>> >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +int rkcif_dvp_register(struct rkcif_device *rkcif) >>>> +{ >>>> + struct rkcif_interface *interface; >>>> + int ret, i; >>>> + >>>> + if (!rkcif->match_data->dvp) >>>> + return 0; >>>> + >>>> + interface = &rkcif->interfaces[RKCIF_DVP]; >>>> + interface->index = RKCIF_DVP; >>>> + interface->type = RKCIF_IF_DVP; >>>> + interface->in_fmts = rkcif->match_data->dvp->in_fmts; >>>> + interface->in_fmts_num = rkcif->match_data->dvp->in_fmts_num; >>>> + interface->set_crop = rkcif_dvp_set_crop; >>>> + ret = rkcif_interface_register(rkcif, interface); >>>> + if (ret) >>>> + return 0; >>> | >>> +-> Copy-paste error ? >> >> Hm. It's not a mistake. But maybe it is a bit misleading. >> >> The point here is that if something fails with registering the DVP, the >> driver may continue to register other entities, such as the MIPI capture >> thing. > > what if you want to register the DVP interface and it fails ? Maybe two > separate function for rkcif_{dvp,mipi}_interface_register(), call one of > them based on match_data and verify the ret code --> fail if non-zero ? Seems I prepared everything in rkcif-dev.c, but failed to complete it in rkcif_{dvp,mipi}_capture :-/ rkcif_register() in rkcif-dev.c tolerates -ENODEV, so if e.g. DVP is not available on a board, the function will proceed to call rkcif_mipi_register. So we should return ret; here. Sounds reasonable? > >> >> I'll have another look over this mechanism and will try to make it more >> comprehensible. >> >>> >>>> + >>>> + if (rkcif->match_data->dvp->setup) >>>> + rkcif->match_data->dvp->setup(rkcif); >>>> + >>>> + interface->streams_num = rkcif->match_data->dvp->has_ids ? 4 : 1; >>>> + for (i = 0; i < interface->streams_num; i++) { >>>> + struct rkcif_stream *stream = &interface->streams[i]; >>>> + >>>> + stream->id = i; >>>> + stream->interface = interface; >>>> + stream->out_fmts = rkcif->match_data->dvp->out_fmts; >>>> + stream->out_fmts_num = rkcif->match_data->dvp->out_fmts_num; >>>> + stream->queue_buffer = cif_dvp_queue_buffer; >>>> + stream->start_streaming = cif_dvp_start_streaming; >>>> + stream->stop_streaming = cif_dvp_stop_streaming; >>>> + >>>> + ret = rkcif_stream_register(rkcif, stream); >>>> + if (ret) >>>> + goto err_streams_unregister; >>>> + } >>>> + return 0; >>>> + >>>> +err_streams_unregister: >>>> + for (; i >= 0; i--) >>>> + rkcif_stream_unregister(&interface->streams[i]); >>>> + rkcif_interface_unregister(interface); >>>> + >>>> + return ret; >>>> +} >>>> + >>> >>> SNIP >>> >>>> +static inline struct rkcif_buffer *to_rkcif_buffer(struct vb2_v4l2_buffer *vb) >>>> +{ >>>> + return container_of(vb, struct rkcif_buffer, vb); >>>> +} >>>> + >>>> +static inline struct rkcif_stream *to_rkcif_stream(struct video_device *vdev) >>>> +{ >>>> + return container_of(vdev, struct rkcif_stream, vdev); >>>> +} >>>> + >>>> +static struct rkcif_buffer *rkcif_stream_pop_buffer(struct rkcif_stream *stream) >>>> +{ >>>> + struct rkcif_buffer *buffer = NULL; >>>> + unsigned long lock_flags; >>>> + >>>> + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); >>> >>> guard(spinlock_irqsave)(&stream->driver_queue_lock) will simplify this function. >> >> I'll guard up these methods in v7. >> >>> >>>> + >>>> + if (list_empty(&stream->driver_queue)) >>>> + goto err_empty; >>>> + >>>> + buffer = list_first_entry(&stream->driver_queue, struct rkcif_buffer, >>>> + queue); >>>> + list_del(&buffer->queue); >>>> + >>>> +err_empty: >>>> + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); >>>> + return buffer; >>>> +} >>>> + >>>> +static void rkcif_stream_push_buffer(struct rkcif_stream *stream, >>>> + struct rkcif_buffer *buffer) >>>> +{ >>>> + unsigned long lock_flags; >>>> + >>>> + spin_lock_irqsave(&stream->driver_queue_lock, lock_flags); >>>> + list_add_tail(&buffer->queue, &stream->driver_queue); >>>> + spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags); >>>> +} >>>> + >>>> +static inline void rkcif_stream_return_buffer(struct rkcif_buffer *buffer, >>>> + enum vb2_buffer_state state) >>>> +{ >>>> + struct vb2_v4l2_buffer *vb = &buffer->vb; >>>> + >>>> + vb2_buffer_done(&vb->vb2_buf, state); >>>> +} >>>> + >>>> +static void rkcif_stream_complete_buffer(struct rkcif_stream *stream, >>>> + struct rkcif_buffer *buffer) >>>> +{ >>>> + struct vb2_v4l2_buffer *vb = &buffer->vb; >>>> + >>>> + vb->vb2_buf.timestamp = ktime_get_ns(); >>>> + vb->sequence = stream->frame_idx; >>>> + vb2_buffer_done(&vb->vb2_buf, VB2_BUF_STATE_DONE); >>>> + stream->frame_idx++; >>>> +} >>>> + >>>> +void rkcif_stream_pingpong(struct rkcif_stream *stream) >>>> +{ >>>> + struct rkcif_buffer *buffer; >>>> + >>>> + buffer = stream->buffers[stream->frame_phase]; >>>> + if (!buffer->is_dummy) >>>> + rkcif_stream_complete_buffer(stream, buffer); >>> >>> You can actually keep this frame dropping mechanism without using the >>> dummy buffer. >>> >>> You can set a drop flag to TRUE: keep overwriting the buffer you already have >>> without returning it to user-space until you can get another buffer, set >>> the flag again to FALSE and resume returning the buffers to user-space. >> >> The approach you describe is what the downstream driver does and I am >> not really happy with it. A perfectly fine frame is sacrificed in a >> buffer starvation situation. > > Oh I thought the downstream driver does it with the dummy buffer. > >> >> The approach in the patch series at hand follows the example in the >> rkisp1 driver, which should be a good reference. > > Ack. Just FWIW: after some discussions off-list I am not sure anymore that the dummy buffer approach is a good idea. However, maybe we can defer this -- this is something that can be changed anytime once the initial driver is mainline. > >> >>>> + >>>> + buffer = rkcif_stream_pop_buffer(stream); >>>> + if (buffer) { >>>> + stream->buffers[stream->frame_phase] = buffer; >>>> + stream->buffers[stream->frame_phase]->is_dummy = false; >>>> + } else { >>>> + stream->buffers[stream->frame_phase] = &stream->dummy.buffer; >>>> + stream->buffers[stream->frame_phase]->is_dummy = true; >>>> + dev_warn(stream->rkcif->dev, >>>> + "no buffer available, frame will be dropped\n"); >>> >>> This warning can quickly flood the kernel logs if the user-space is too slow in >>> enqueuing the buffers. >> >> True. dev_warn_ratelimited(...)? >> > > Does frame dropping deserve a warning ? If you don't think so, maybe a > debug or info ? _dbg sounds reasonable for that. > >>> >>>> + } >>>> + >>>> + if (stream->queue_buffer) >>>> + stream->queue_buffer(stream, stream->frame_phase); >>> >>> is this if statement really needed ? >> >> I find it good practice to check the callbacks before calling them. But >> this is a matter of taste, of course. >> >>> >>>> + >>>> + stream->frame_phase = 1 - stream->frame_phase; >>>> +} >>>> + >>>> +static int rkcif_stream_init_buffers(struct rkcif_stream *stream) >>>> +{ >>>> + struct v4l2_pix_format_mplane *pix = &stream->pix; >>>> + int i; >>>> + >>>> + stream->buffers[0] = rkcif_stream_pop_buffer(stream); >>>> + if (!stream->buffers[0]) >>>> + goto err_buff_0; >>>> + >>>> + stream->buffers[1] = rkcif_stream_pop_buffer(stream); >>>> + if (!stream->buffers[1]) >>>> + goto err_buff_1; >>>> + >>>> + if (stream->queue_buffer) { >>>> + stream->queue_buffer(stream, 0); >>>> + stream->queue_buffer(stream, 1); >>>> + } >>>> + >>>> + stream->dummy.size = pix->num_planes * pix->plane_fmt[0].sizeimage; >>>> + stream->dummy.vaddr = >>>> + dma_alloc_attrs(stream->rkcif->dev, stream->dummy.size, >>>> + &stream->dummy.buffer.buff_addr[0], GFP_KERNEL, >>>> + DMA_ATTR_NO_KERNEL_MAPPING); >>>> + if (!stream->dummy.vaddr) >>>> + goto err_dummy; >>>> + >>>> + for (i = 1; i < pix->num_planes; i++) >>>> + stream->dummy.buffer.buff_addr[i] = >>>> + stream->dummy.buffer.buff_addr[i - 1] + >>>> + pix->plane_fmt[i - 1].bytesperline * pix->height; >>>> + >>>> + return 0; >>>> + >>>> +err_dummy: >>>> + rkcif_stream_return_buffer(stream->buffers[1], VB2_BUF_STATE_QUEUED); >>>> + stream->buffers[1] = NULL; >>>> + >>>> +err_buff_1: >>>> + rkcif_stream_return_buffer(stream->buffers[0], VB2_BUF_STATE_QUEUED); >>>> + stream->buffers[0] = NULL; >>>> +err_buff_0: >>>> + return -EINVAL; >>>> +} >>>> + >>> >>> SNIP >>> >>>> +static int rkcif_stream_init_vb2_queue(struct vb2_queue *q, >>>> + struct rkcif_stream *stream) >>>> +{ >>>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >>>> + q->io_modes = VB2_MMAP | VB2_DMABUF; >>>> + q->drv_priv = stream; >>>> + q->ops = &rkcif_stream_vb2_ops; >>>> + q->mem_ops = &vb2_dma_contig_memops; >>>> + q->buf_struct_size = sizeof(struct rkcif_buffer); >>>> + q->min_queued_buffers = CIF_REQ_BUFS_MIN; >>> >>> If I recall correctly min_queued_buffers should be the strict minimum >>> number of buffers you need to start streaming. So in this case it should >>> be 3 = 2 pingpong buffers + 1 dummy buffer. >> >> The dummy buffer is allocated separately and does not need to be >> accounted for. >> >> Two pingpong buffers is what the hardware can queue, but in practice, to >> start (and, above all, keep on) streaming you'll need more. >> >>> VIDIOC_REQBUFS will allocate min_queued_buffers + 1 and user-space will >>> probably allocate even more anyway. >> >> Is that so? I found that user space relies too much on this minimum >> buffer count and experienced several buffer starvation situations >> because kernel AND user space were to cheap in terms of buffer count. >> Maybe 8 is too many, but in practice four buffers are required at least >> for a decent 2160p stream (one ready for DMA write, one ongoing DMA >> write, one stable for processing (maybe DRM scanout or whatever the >> application is), one spare). >> >> I am open to suggestions but please keep real life situations in mind >> and move away from theoretical stand-alone-capture-hw setups. > > so the documentation says: > -------------------------------------------------------------------------- > min_queued_buffers is used when a DMA engine cannot be started unless at > least this number of buffers have been queued into the driver. > -------------------------------------------------------------------------- > > and: > -------------------------------------------------------------------------- > VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 > buffers will be allocated. > -------------------------------------------------------------------------- > > I also found theses patches: > https://lore.kernel.org/linux-media/20231211133251.150999-1-benjamin.gaignard@collabora.com/ > https://lore.kernel.org/all/20241007124225.63463-1-jacopo.mondi@ideasonboard.com/ > > If I understood correctly there is a difference between: > > - the minimal number of buffers to be allocated with VIDIOC_REQBUFS > - the minimal number of buffers to make it possible to start streaming > > what you are setting is the latter, which means you need 8 buffers to > even start streaming which should not be the case for rkcif, which > should only need two (of course when using pingpong) > > what you mentioned with the minimum number of buffers for a decent stream seems > to point more towards @min_reqbufs_allocation: > -------------------------------------------------------------------------- > Drivers can set this if there has to be a certain number of > buffers available for the hardware to work effectively. > -------------------------------------------------------------------------- > > of course this being said I am not the expert here so feel free to ask > @Laurent @Hans Thanks a lot for digging out all this info. In particular, the pointer to Jacopo's change to the rkisp1 is interesting. I feel kind of stupid now because I stumbled over this exact issue when I tried to capture a single frame with the rkisp1 driver. So in general we should let user space decide, as user space knows best about the exact application (one-shot capture, stream capture, stream capture with extended postprocessing = deeper pipeline, ...). And following Jacopo's reasoning for the rkisp1 we should set the value to 1 here, as we also have a dummy buffer approach. Best regards, Michael > > -- > Kind Regards > Mehdi Djait