Hi Michael,
On Thu, Jan 09, 2025 at 02:05:54PM +0100, Michael Riesch wrote:
> Hi Sakari,
>
> Thanks a lot for your review!
You're welcome!
>
> On 1/9/25 11:07, Sakari Ailus wrote:
> > Hi Michael,
> >
> > Thanks for the update.
> >
> > On Tue, Dec 17, 2024 at 04:55:16PM +0100, Michael Riesch wrote:
...
> >> +static int cif_subdev_notifier_register(struct cif_device *cif_dev, int index)
> >
> > This function name is misleading. It does not register a notifier, it
> > prepares one though.
>
> cif_subdev_notifier_add(..) ?
Sounds good.
> >> +{
> >> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> >> + struct v4l2_fwnode_endpoint *vep;
> >> + struct cif_remote *remote;
> >> + struct device *dev = cif_dev->dev;
> >> + struct fwnode_handle *ep;
> >> + int ret;
> >> +
> >> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), index, 0,
> >> + FWNODE_GRAPH_ENDPOINT_NEXT);
> >> + if (!ep)
> >> + return -ENODEV;
> >> +
> >> + if (index == 0) {
> >
> > index seems to be always 0.
>
> Right now, yes. The soon-to-be-added MIPI paths shall bring different
> indices into play.
Ack.
>
> >
> >> + vep = &cif_dev->dvp.vep;
> >> +
> >> + vep->bus_type = V4L2_MBUS_UNKNOWN;
> >> + ret = v4l2_fwnode_endpoint_parse(ep, vep);
> >> + if (ret)
> >> + goto complete;
> >> +
> >> + if (vep->bus_type != V4L2_MBUS_BT656 &&
> >> + vep->bus_type != V4L2_MBUS_PARALLEL) {
> >> + v4l2_err(&cif_dev->v4l2_dev, "unsupported bus type\n");
> >> + goto complete;
> >> + }
> >> +
> >> + remote = v4l2_async_nf_add_fwnode_remote(ntf, ep,
> >> + struct cif_remote);
> >> + if (IS_ERR(remote)) {
> >> + ret = PTR_ERR(remote);
> >> + goto complete;
> >> + }
> >> +
> >> + cif_dev->dvp.stream.remote = remote;
> >> + remote->stream = &cif_dev->dvp.stream;
> >> + } else {
> >> + ret = -ENODEV;
> >> + goto complete;
> >> + }
> >> +
> >> +complete:
> >> + fwnode_handle_put(ep);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void cif_subdev_notifier_unregister(struct cif_device *cif_dev,
> >> + int index)
> >
> > This seems to be redundant.
>
> Ack.
>
> >
> >> +{
> >> +}
> >> +
> >> +static int cif_entities_register(struct cif_device *cif_dev)
> >
> > This function appears to be misnamed.
>
> Hm. The function registers the different entities of this hardware, such
> as the DVP. However, I am not emotionally attached to this name and any
> suggestions are welcome :-)
Ok, fair enough: one of the things it does is to register the video device
but it does a whole lot of other stuff unrelated to it. How about just
cif_register()?
>
> >
> >> +{
> >> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> >> + struct device *dev = cif_dev->dev;
> >> + int ret;
> >> +
> >> + v4l2_async_nf_init(ntf, &cif_dev->v4l2_dev);
> >> + ntf->ops = &cif_subdev_notifier_ops;
> >> +
> >> + if (cif_dev->match_data->dvp) {
> >> + ret = cif_subdev_notifier_register(cif_dev, 0);
> >> + if (ret) {
> >> + dev_err(dev,
> >> + "failed to register notifier for dvp: %d\n",
> >> + ret);
> >> + goto err;
> >> + }
> >> +
> >> + ret = cif_dvp_register(cif_dev);
> >> + if (ret) {
> >> + dev_err(dev, "failed to register dvp: %d\n", ret);
> >> + goto err_dvp_notifier_unregister;
> >> + }
> >> + }
> >> +
> >> + ret = v4l2_async_nf_register(ntf);
> >> + if (ret)
> >> + goto err_notifier_cleanup;
> >> +
> >> + return 0;
> >> +
> >> +err_notifier_cleanup:
> >> + if (cif_dev->match_data->dvp)
> >> + cif_dvp_unregister(cif_dev);
> >> +err_dvp_notifier_unregister:
> >> + if (cif_dev->match_data->dvp)
> >> + cif_subdev_notifier_unregister(cif_dev, 0);
> >> + v4l2_async_nf_cleanup(ntf);
> >> +err:
> >> + return ret;
> >> +}
> >> +
> >> +static void cif_entities_unregister(struct cif_device *cif_dev)
And similarly here.
> >> +{
> >> + v4l2_async_nf_unregister(&cif_dev->notifier);
> >> + v4l2_async_nf_cleanup(&cif_dev->notifier);
> >> +
> >> + if (cif_dev->match_data->dvp) {
> >> + cif_subdev_notifier_unregister(cif_dev, 0);
> >> + cif_dvp_unregister(cif_dev);
> >> + }
> >> +}
> >> +
> >> +static irqreturn_t cif_isr(int irq, void *ctx)
> >> +{
> >> + irqreturn_t ret = IRQ_NONE;
> >> +
> >> + if (cif_dvp_isr(irq, ctx) == IRQ_HANDLED)
> >> + ret = IRQ_HANDLED;
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int cif_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *dev = &pdev->dev;
> >> + struct cif_device *cif_dev;
> >> + u32 cif_clk_delaynum = 0;
> >> + int ret, irq, i;
> >> +
> >> + 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;
> >> +
> >> + dev_set_drvdata(dev, cif_dev);
> >> + cif_dev->dev = dev;
> >> +
> >> + cif_dev->base_addr = devm_platform_ioremap_resource(pdev, 0);
> >> + if (IS_ERR(cif_dev->base_addr))
> >> + return PTR_ERR(cif_dev->base_addr);
> >> +
> >> + irq = platform_get_irq(pdev, 0);
> >> + if (irq < 0)
> >> + return irq;
> >> +
> >> + ret = devm_request_irq(dev, irq, cif_isr, IRQF_SHARED,
> >> + dev_driver_string(dev), dev);
> >> + if (ret)
> >> + return dev_err_probe(dev, ret, "failed to request irq\n");
> >> + cif_dev->irq = irq;
> >
> > Where do you need the irq field?
>
> Hm. Seems to be a leftover. Should we check irq == cif_dev->irq in the
> ISR or is this unnecessary?
You've got the device context already, I don't think you need to check the
irq.
> >> + cif_dev->media_dev.dev = dev;
> >> + strscpy(cif_dev->media_dev.model, CIF_DRIVER_NAME,
> >> + sizeof(cif_dev->media_dev.model));
> >> + media_device_init(&cif_dev->media_dev);
> >> +
> >> + cif_dev->v4l2_dev.mdev = &cif_dev->media_dev;
> >> + cif_dev->v4l2_dev.notify = cif_notify;
> >> + strscpy(cif_dev->v4l2_dev.name, CIF_DRIVER_NAME,
> >> + sizeof(cif_dev->v4l2_dev.name));
> >
> > Do you need to set the name? v4l2_device_register() assigns a default one.
>
> I guess we can use the default, then.
>
> We do need to set the model of the media device, though, right?
Correct.
> >> +static int cif_stream_start_streaming(struct vb2_queue *queue,
> >> + unsigned int count)
> >> +{
> >> + struct cif_stream *stream = queue->drv_priv;
> >> + struct cif_device *cif_dev = stream->cif_dev;
> >> + struct v4l2_subdev *sd = stream->remote->sd;
> >> + int ret;
> >> +
> >> + stream->frame_idx = 0;
> >> + stream->frame_phase = 0;
> >> +
> >> + ret = video_device_pipeline_start(&stream->vdev, &stream->pipeline);
> >> + if (ret) {
> >> + dev_err(cif_dev->dev, "failed to start pipeline %d\n", ret);
> >> + goto err_out;
> >> + }
> >> +
> >> + ret = pm_runtime_resume_and_get(cif_dev->dev);
> >> + if (ret < 0) {
> >> + dev_err(cif_dev->dev, "failed to get runtime pm, %d\n", ret);
> >> + goto err_pipeline_stop;
> >> + }
> >> +
> >> + ret = cif_stream_init_buffers(stream);
> >> + if (ret)
> >> + goto err_runtime_put;
> >> +
> >> + if (stream->start_streaming) {
> >> + ret = stream->start_streaming(stream);
> >> + if (ret < 0)
> >> + goto err_runtime_put;
> >> + }
> >> +
> >> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> >
> > Could you use v4l2_subdev_enable_streams() instead for this?
> >
> > See e.g. drivers/media/pci/intel/ipu6 for an example.
>
> This should be pretty much a 1:1 replacement I guess? But with support
> for streams?
Yes, and I'd prefer to get rid of the s_stream() op altogether. That may
take a long time though.
> >> +static int cif_stream_enum_framesizes(struct file *file, void *fh,
> >> + struct v4l2_frmsizeenum *fsize)
> >> +{
> >> + struct cif_stream *stream = video_drvdata(file);
> >> + struct v4l2_subdev_frame_size_enum fse = {
> >> + .index = fsize->index,
> >> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >> + };
> >> + struct v4l2_subdev *sd = stream->remote->sd;
> >> + const struct cif_output_fmt *fmt;
> >> + int ret;
> >> +
> >> + fmt = cif_stream_find_output_fmt(stream, fsize->pixel_format);
> >> + if (!fmt)
> >> + return -EINVAL;
> >> +
> >> + fse.code = fmt->mbus_code;
> >> +
> >> + ret = v4l2_subdev_call(sd, pad, enum_frame_size, NULL, &fse);
> >
> > You shouldn't get this information from the sensor driver but just convey
> > what this device supports.
>
> OK, but what does this device support? In principle, there is a
> continuous range of frame sizes up to a certain maximum size. But in
> practice, it depends on the subdevice as there is no scaler in the DVP
> path. Thus, every frame size but the one that the subdevice delivers
> will result in a broken stream?
Could you use CIF_MAX_WIDTH and CIF_MAX_HEIGHT?
>
> If this does not return the only possible frame size, is this callback
> useful at all?
In order not to configure an output size for the sensor that can't be
received by this block?
> >> diff --git a/drivers/media/platform/rockchip/cif/cif-stream.h b/drivers/media/platform/rockchip/cif/cif-stream.h
> >> new file mode 100644
> >> index 000000000000..5bfa260eda83
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rockchip/cif/cif-stream.h
> >> @@ -0,0 +1,24 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Rockchip Camera Interface (CIF) Driver
> >> + *
> >> + * Copyright (C) 2024 Michael Riesch <michael.riesch@xxxxxxxxxxxxxx>
> >> + */
> >> +
> >> +#ifndef _CIF_STREAM_H
> >> +#define _CIF_STREAM_H
> >> +
> >> +#include "cif-common.h"
> >> +
> >> +struct cif_stream_config {
> >> + const char *name;
> >> +};
> >> +
> >> +void cif_stream_pingpong(struct cif_stream *stream);
> >> +
> >> +int cif_stream_register(struct cif_device *cif_dev, struct cif_stream *stream,
> >> + const struct cif_stream_config *config);
> >> +
> >> +void cif_stream_unregister(struct cif_stream *stream);
> >
> > There are other CIFs out there. I think it'd be good to use either some
> > Rockchip specific prefix (rk maybe?) or a namespace.
>
> Are there? In drivers/media or in general?
I may recall something that may be after all related to this.
Either way, "cif" is a non-specific short string and it's entirely
conceivable it'll easily conflict with something else.
>
> And would that apply to all the method and struct names in this driver
> or to the driver as well (location, file names)?
>
> The name has been discussed several times during the 13 iterations of
> the PX30 VIP series and I believe it changed from (cif//rkcif_) in
> downstream -> (vip//vip_) in Maximes work -> (cif/cif-/cif_) in Mehdis
> work, where the tuple is (driver directory/filename prefix/function and
> structs prefix).
>
> Hence I am a bit reluctant to change the naming convention yet again.
> That said, I'll be prepared to change it JUST ONE MORE TIME to any tuple
> you suggest -- but this really should be the end of the name bikeshedding.
I don't care about the internal naming but the global symbols. Using a
namespace is another option.
--
Kind regards,
Sakari Ailus
