Le 30/10/2023 à 13:25, Mehdi Djait a écrit :
Introduce a video node-centric driver for the Rockchip CIF
This controller supports multiple interfaces, but for now only
the BT.656 interface could be tested, hence it's the only one
that's supported in the first version of this driver.
This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
but for now it's only been tested on the PX30.
Most of this driver was written following the BSP driver from rockchip,
removing the parts that either didn't fit correctly the guidelines, or
that couldn't be tested.
This basic version doesn't support cropping nor scaling, and is only
designed with one SDTV video decoder being attached to it a any time.
This version uses the "pingpong" mode of the controller, which is a
double-buffering mechanism.
Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
Hi,
a few nit and a real question at the end.
+static int cif_init_buffers(struct cif_stream *stream)
+{
+ struct cif_device *cif_dev = stream->cifdev;
+ unsigned long lock_flags = 0;
Nit: no need to init
+
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+
+ stream->buffs[0] = cif_get_buffer(stream);
+ stream->buffs[1] = cif_get_buffer(stream);
+
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+
+ if (!(stream->buffs[0]) || !(stream->buffs[1]))
+ return -EINVAL;
+
+ stream->drop_frame = false;
+
+ cif_write(cif_dev, CIF_FRM0_ADDR_Y,
+ stream->buffs[0]->buff_addr[CIF_PLANE_Y]);
+ cif_write(cif_dev, CIF_FRM0_ADDR_UV,
+ stream->buffs[0]->buff_addr[CIF_PLANE_UV]);
+
+ cif_write(cif_dev, CIF_FRM1_ADDR_Y,
+ stream->buffs[1]->buff_addr[CIF_PLANE_Y]);
+ cif_write(cif_dev, CIF_FRM1_ADDR_UV,
+ stream->buffs[1]->buff_addr[CIF_PLANE_UV]);
+
+ return 0;
+}
+
+static void cif_assign_new_buffer_pingpong(struct cif_stream *stream)
+{
+ struct cif_device *cif_dev = stream->cifdev;
+ struct cif_buffer *buffer = NULL;
+ u32 frm_addr_y, frm_addr_uv;
+ unsigned long lock_flags = 0;
Nit: no need to init
+
+ stream->drop_frame = false;
+
+ /* Set up an empty buffer for the next frame. */
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+
+ buffer = cif_get_buffer(stream);
+
+ /*
+ * In Pinpong mode:
+ * After one frame0 captured, CIF will start to capture the next frame1
+ * automatically.
+ *
+ * If there is no buffer:
+ * 1. Make the next frame0 write to the buffer of frame1.
+ *
+ * 2. Drop the frame1: Don't return it to user-space, as it will be
+ * overwritten by the next frame0.
+ */
+ if (!buffer) {
+ stream->drop_frame = true;
+ buffer = stream->buffs[1 - stream->frame_phase];
+ }
+
+ stream->buffs[stream->frame_phase] = buffer;
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+
+ frm_addr_y = stream->frame_phase ? CIF_FRM1_ADDR_Y : CIF_FRM0_ADDR_Y;
+ frm_addr_uv = stream->frame_phase ? CIF_FRM1_ADDR_UV : CIF_FRM0_ADDR_UV;
+
+ cif_write(cif_dev, frm_addr_y, buffer->buff_addr[CIF_PLANE_Y]);
+ cif_write(cif_dev, frm_addr_uv, buffer->buff_addr[CIF_PLANE_UV]);
+}
...
+static void cif_buf_queue(struct vb2_buffer *vb)
+{
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct cif_buffer *cifbuf = to_cif_buffer(vbuf);
+ struct vb2_queue *queue = vb->vb2_queue;
+ struct cif_stream *stream = queue->drv_priv;
+ struct v4l2_pix_format *pix = &stream->pix;
+ unsigned long lock_flags = 0;
Nit: no need to init
+ int i;
+
+ struct cif_output_fmt *fmt = stream->cif_fmt_out;
+
+ memset(cifbuf->buff_addr, 0, sizeof(cifbuf->buff_addr));
+
+ cifbuf->buff_addr[0] = vb2_dma_contig_plane_dma_addr(vb, 0);
+
+ for (i = 0; i < fmt->cplanes - 1; i++)
+ cifbuf->buff_addr[i + 1] = cifbuf->buff_addr[i] +
+ pix->bytesperline * pix->height;
+
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+ list_add_tail(&cifbuf->queue, &stream->buf_head);
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+}
+
+static void cif_return_all_buffers(struct cif_stream *stream,
+ enum vb2_buffer_state state)
+{
+ struct cif_buffer *buf;
+ unsigned long lock_flags = 0;
Nit: no need to init
+
+ if (stream->buffs[0]) {
+ vb2_buffer_done(&stream->buffs[0]->vb.vb2_buf, state);
+ stream->buffs[0] = NULL;
+ }
+
+ if (stream->buffs[1]) {
+ if (!stream->drop_frame)
+ vb2_buffer_done(&stream->buffs[1]->vb.vb2_buf, state);
+
+ stream->buffs[1] = NULL;
+ }
+
+ while (!list_empty(&stream->buf_head)) {
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+
+ buf = cif_get_buffer(stream);
+ vb2_buffer_done(&buf->vb.vb2_buf, state);
+
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+ }
+}
...
+static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_connection *asd)
+{
+ struct cif_device *cif_dev = container_of(notifier,
+ struct cif_device, notifier);
+
Nit: no need for an extra blank line
+ int pad;
+
+ cif_dev->remote.sd = subdev;
+ pad = media_entity_get_fwnode_pad(&subdev->entity, subdev->fwnode,
+ MEDIA_PAD_FL_SOURCE);
+ if (pad < 0)
+ return pad;
+
+ cif_dev->remote.pad = pad;
+
+ return 0;
+}
...
+static int cif_subdev_notifier(struct cif_device *cif_dev)
+{
+ struct v4l2_async_notifier *ntf = &cif_dev->notifier;
+ struct device *dev = cif_dev->dev;
+ struct v4l2_async_connection *asd;
+ struct v4l2_fwnode_endpoint vep = {
+ .bus_type = V4L2_MBUS_PARALLEL,
+ };
+ struct fwnode_handle *ep;
+ int ret;
+
+ v4l2_async_nf_init(ntf, &cif_dev->v4l2_dev);
+
+ ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
+ FWNODE_GRAPH_ENDPOINT_NEXT);
+ if (!ep)
+ return -EINVAL;
+
+ ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+ if (ret)
+ return ret;
+
+ asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
+ struct v4l2_async_connection);
+ if (IS_ERR(asd)) {
+ ret = PTR_ERR(asd);
+ return ret;
Nit: return PTR_ERR(asd);
+ }
+
+ ntf->ops = &subdev_notifier_ops;
+
+ fwnode_handle_put(ep);
+
+ ret = v4l2_async_nf_register(ntf);
+ return ret;
Nit: return v4l2_async_nf_register(ntf);
+}
...
+static int cif_plat_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct v4l2_device *v4l2_dev;
+ struct cif_device *cif_dev;
+ int ret, irq;
+
+ cif_dev = devm_kzalloc(dev, sizeof(*cif_dev), GFP_KERNEL);
+ if (!cif_dev)
+ return -ENOMEM;
+
+ cif_dev->match_data = of_device_get_match_data(dev);
+ if (!cif_dev->match_data)
+ return -ENODEV;
+
+ platform_set_drvdata(pdev, cif_dev);
+ cif_dev->dev = dev;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = devm_request_irq(dev, irq, cif_irq_pingpong, 0,
+ dev_driver_string(dev), dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "request irq failed\n");
+
+ cif_dev->irq = irq;
+
+ ret = cif_get_resource(pdev, cif_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_clk_bulk_get(dev, cif_dev->match_data->clks_num,
+ cif_dev->match_data->clks);
+ if (ret)
+ return ret;
+
+ cif_dev->cif_rst = devm_reset_control_array_get(dev, false, false);
+ if (IS_ERR(cif_dev->cif_rst))
+ return PTR_ERR(cif_dev->cif_rst);
+
+ /* Initialize the stream. */
+ cif_stream_init(cif_dev);
+ strscpy(cif_dev->media_dev.model, "cif",
+ sizeof(cif_dev->media_dev.model));
+ cif_dev->media_dev.dev = &pdev->dev;
+ v4l2_dev = &cif_dev->v4l2_dev;
+ v4l2_dev->mdev = &cif_dev->media_dev;
+ strscpy(v4l2_dev->name, "rockchip-cif", sizeof(v4l2_dev->name));
+
+ ret = v4l2_device_register(cif_dev->dev, &cif_dev->v4l2_dev);
+ if (ret < 0)
+ return ret;
+
+ media_device_init(&cif_dev->media_dev);
+
+ ret = media_device_register(&cif_dev->media_dev);
+ if (ret < 0)
+ goto err_unreg_v4l2_dev;
+
+ /* Create & register platform subdev. */
+ ret = cif_register_stream_vdev(cif_dev);
+ if (ret < 0)
+ goto err_unreg_media_dev;
+
+ ret = cif_subdev_notifier(cif_dev);
+ if (ret < 0) {
+ v4l2_err(&cif_dev->v4l2_dev,
+ "Failed to register subdev notifier(%d)\n", ret);
+ cif_unregister_stream_vdev(cif_dev);
+ goto err_unreg_media_dev;
Should there be another label with cif_unregister_stream_vdev(cif_dev);
if an error occurs here?
CJ
+ }
+
+ cif_set_default_format(cif_dev);
+ pm_runtime_enable(&pdev->dev);
+
+ return 0;
+
+err_unreg_media_dev:
+ media_device_unregister(&cif_dev->media_dev);
+err_unreg_v4l2_dev:
+ v4l2_device_unregister(&cif_dev->v4l2_dev);
+ return ret;
+}
...
