Message-ID: <78fa589d-f9b6-41d8-bee5-766d0d1c3b17@wolfvision.net>
Date: Fri, 10 Jan 2025 10:12:29 +0100
From: Michael Riesch <michael.riesch@...fvision.net>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Nicolas Dufresne <nicolas@...fresne.ca>,
 Sebastian Fricke <sebastian.fricke@...labora.com>,
 Alexander Shiyan <eagle.alexander923@...il.com>,
 Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 Mehdi Djait <mehdi.djait@...tlin.com>,
 Gerald Loacker <gerald.loacker@...fvision.net>
Subject: Re: [PATCH v2 4/6] media: rockchip: add a driver for the rockchip
 camera interface (cif)

Hi Sakari,

On 1/9/25 18:06, Sakari Ailus wrote:
> 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()?

Sounds good!

> 
>>
>>>
>>>> +{
>>>> +	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.

Ack.

> 
>>>> +{
>>>> +	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.

Ack.

> 
>>>> +	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.

Ack.

> 
>>>> +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?

Right... Forgot about this case. This means user space needs to
determine the possible frame sizes of each V4L2 device and subdevice in
the pipeline and find a size that matches, right?

> 
>>>> 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@...fvision.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.

Fair enough.

> 
>>
>> 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.
> 

I would suggest the tuple (rkcif/rkcif-/rkcif_) then. This is in
alignment with the Rockchip ISP driver (rkisp1/rkisp1-/rkisp1_).
Unfortunately, the Rockchip RGA differs here (but with the tuple
(rga/rga-/rga_) it uses the same prefix for everything at least).

There seems to be even less alignment when looking at other
drivers/media/platform/ drivers, therefore I can only try to maximize
the alignment within drivers/media/platform/rockchip/.

What do you think?

Regards,
Michael

Powered by blists - more mailing lists