| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 30 Oct 2023 13:47:17 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Mehdi Djait <mehdi.djait@...tlin.com>, mchehab@...nel.org,
heiko@...ech.de, hverkuil-cisco@...all.nl,
krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
conor+dt@...nel.org
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
alexandre.belloni@...tlin.com, maxime.chevallier@...tlin.com,
paul.kocialkowski@...tlin.com, michael.riesch@...fvision.net
Subject: Re: [PATCH v9 2/3] media: rockchip: Add a driver for Rockchip's
camera interface
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@...tlin.com>
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;
> +}
...
Powered by blists - more mailing lists