Re: [PATCH v2 4/6] media: rockchip: add a driver for the rockchip camera interface (cif)

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@wolfvision.net>
> >> + */
> >> +
> >> +#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