Re: [PATCH v2 2/3] media: platform: Add Renesas Input Video Control block driver

[Dan Scally <dan.scally@ideasonboard.com>]

Hi Jacopo

On 01/07/2025 14:31, Jacopo Mondi wrote:
> Hi Dan
>
> On Tue, Jul 01, 2025 at 02:21:09PM +0100, Dan Scally wrote:
>> Hi Jacopo
>>
>> On 01/07/2025 14:17, Jacopo Mondi wrote:
>>> Hi Dan
>>>
>>> On Tue, Jul 01, 2025 at 02:01:25PM +0100, Dan Scally wrote:
>>>> Hi Jacopo
>>>>
>>>> On 01/07/2025 13:58, Jacopo Mondi wrote:
>>>>> Hi Dan
>>>>>
>>>>> On Tue, Jul 01, 2025 at 12:27:41PM +0100, Dan Scally wrote:
>>>>>> Hi Jacopo
>>>>>>
>>>>> [snip]
>>>>>
>>>>>>>>>> +static bool rzv2h_ivc_pipeline_ready(struct media_pipeline *pipe)
>>>>>>>>>> +{
>>>>>>>>>> +	struct media_pipeline_entity_iter iter;
>>>>>>>>>> +	unsigned int n_video_devices = 0;
>>>>>>>>>> +	struct media_entity *entity;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	ret = media_pipeline_entity_iter_init(pipe, &iter);
>>>>>>>>>> +	if (ret)
>>>>>>>>>> +		return ret;
>>>>>>>>>> +
>>>>>>>>>> +	media_pipeline_for_each_entity(pipe, &iter, entity) {
>>>>>>>>>> +		if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
>>>>>>>>>> +			n_video_devices++;
>>>>>>>>>> +	}
>>>>>>>>> This counts the ISP video devices as well, right ?
>>>>>>>> That's right
>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +	media_pipeline_entity_iter_cleanup(&iter);
>>>>>>>>>> +
>>>>>>>>>> +	return n_video_devices == pipe->start_count;
>>>>>>>>> So this checks that all other video devices have started when this one
>>>>>>>>> is started as well. What if we start the IVC first and the ISP later?
>>>>>>>> Doesn't matter which order; nothing happens until they're all started and
>>>>>>>> then the .pipeline_started() callbacks for the entities run.
>>>>>>> Ah sure, thanks, I got it wrong.
>>>>>>>
>>>>>>> I see that all drivers in the series (IVC and mali) that use the
>>>>>>> media_pipeline_started() helper have to implement a function similar
>>>>>>> in spirit to rzv2h_ivc_pipeline_ready(). Can't the framework do that ?
>>>>>>> So that drivers can call media_pipeline_started() [*] unconditionally
>>>>>>> and use its return value to find out if the pipeline has actually
>>>>>>> started or not ?
>>>>>> The steer I got from Laurent and Sakari was that code from mc shouldn't be
>>>>>> checking for MEDIA_ENTITY_TYPE_VIDEO_DEVICE, but perhaps we could have a
>>>>>> V4L2 helper that does that instead?
>>>>> Do you mean this check ?
>>>>>
>>>>> 	media_pipeline_for_each_entity(pipe, &iter, entity) {
>>>>> 		if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
>>>>> 			n_video_devices++;
>>>>> 	}
>>>> Yes
>>>>
>>>>> We have wrappers like
>>>>>
>>>>> __must_check int video_device_pipeline_start(struct video_device *vdev,
>>>>> 					     struct media_pipeline *pipe)
>>>>>
>>>>> already, so this might become something like
>>>>>
>>>>> int video_device_try_run_pipeline(vdev)
>>>>> {
>>>>>            pipe = video_device_pipeline(&ivc->vdev.dev);
>>>>>
>>>>>          	ret = media_pipeline_entity_iter_init(pipe, &iter);
>>>>> 	if (ret)
>>>>> 		return ret;
>>>>>
>>>>> 	media_pipeline_for_each_entity(pipe, &iter, entity) {
>>>>> 		if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
>>>>> 			n_video_devices++;
>>>>> 	}
>>>>>
>>>>> 	media_pipeline_entity_iter_cleanup(&iter);
>>>>>
>>>>> 	return (n_video_devices == pipe->start_count) ?
>>>>> 		media_pipeline_started(pipe) : -ENODEV;
>>>>> }
>>>>>
>>>>> The drivers then should become something like:
>>>>>
>>>>> static int rzv2h_ivc_start_streaming(struct vb2_queue *q, unsigned int count)
>>>>> {
>>>>> 	struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>>>>> 	struct media_pipeline *pipe;
>>>>> 	int ret;
>>>>>
>>>>> 	ret = pm_runtime_resume_and_get(ivc->dev);
>>>>> 	if (ret)
>>>>> 		goto err_return_buffers;
>>>>>
>>>>> 	ret = video_device_pipeline_alloc_start(&ivc->vdev.dev);
>>>>> 	if (ret) {
>>>>> 		dev_err(ivc->dev, "failed to start media pipeline\n");
>>>>> 		goto err_pm_runtime_put;
>>>>> 	}
>>>>>
>>>>> 	rzv2h_ivc_format_configure(ivc);
>>>>>
>>>>> 	ivc->buffers.sequence = 0;
>>>>> 	ivc->vvalid_ifp = 0;
>>>>>
>>>>>            if (!video_device_try_run_pipeline(ivc->dev))
>>>>> 		media_jobs_run_jobs(ivc->sched);
>>>>>
>>>>> 	return 0;
>>>>>
>>>>> err_pm_runtime_put:
>>>>> 	pm_runtime_put(ivc->dev);
>>>>> err_return_buffers:
>>>>> 	rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_QUEUED);
>>>>>
>>>>> 	return ret;
>>>>> }
>>>>>
>>>>> Removing a bit of boilerplate in all drivers using
>>>>> media_pipeline_started()/stopped() ?
>>>> Yes something like video_device_pipeline_start() is what I was thinking,
>>>> what you've done there looks spot on to me!
>>> You know, looking at what [video|media]_pipeline_[alloc]_start() does,
>>> it could even be renamed to pipeline_validate() and your new functions
>>> named 'start' :)
>>>
>>> Anyway, just for the sake of discussion, one could even create a
>>>
>>> [video|media]_pipeline_[alloc]_run() function that bundles together
>>> the existing _start() functions and the newly proposed _try_run().
>>>
>>> The driver would be an even more compact
>>>
>>>    static int rzv2h_ivc_start_streaming(struct vb2_queue *q, unsigned int count)
>>>    {
>>>    	struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>>>    	struct media_pipeline *pipe;
>>>    	int ret;
>>>
>>>    	ret = pm_runtime_resume_and_get(ivc->dev);
>>>    	if (ret)
>>>    		goto err_return_buffers;
>>>
>>>    	rzv2h_ivc_format_configure(ivc);
>>>
>>>    	ivc->buffers.sequence = 0;
>>>    	ivc->vvalid_ifp = 0;
>>>
>>>    	ret = video_device_pipeline_alloc_run(&ivc->vdev.dev);
>>>    	if (ret) {
>>>    		dev_err(ivc->dev, "failed to start media pipeline\n");
>>>    		goto err_pm_runtime_put;
>>>    	}
>>>
>>> 	media_jobs_run_jobs(ivc->sched);
>> Wouldn't this still need to be conditional? We'd need
> Yeah my thinking was that if (ret) we don't get here
>
>> video_device_pipeline_alloc_run() to be able to return something that
>> indicated:
>>
>>
>> 1. Failure
>>
>> 2. Success, but the pipeline isn't ready to start
>>
>> 3. Success and the pipeline is ready to start
>>
>>
>> right?
> But yes, you have 3 possible return states, something that might make
> the API cumbersome to design
>
>>> 	return 0;
>>> }
>>>
>>> Do you see a use case for first starting the pipeline then trying to
>>> run it in a separate step ?
>>
>> Not off the top of my head...they're both operations that ought to happen in
>> .start_streaming()...but do you propose replacing the existing
>> video_device_pipeline_start() with a bundled function, or just adding a new
>> function and retaining the old one?
>>
> Not replacing them no, otherwise all drivers should be ported to use
> pipeline_started(), right ?
Yes
> However given the "three return values" problem, we might want to keep
> _try_run() separate ?


I'll have a think and post something...I'll probably bundle this and the pipeline_started() / 
pipeline_stopped() entity ops patch

>> Dan
>>
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_start_streaming(struct vb2_queue *q, unsigned int count)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>>>>>>>>>> +	struct media_pipeline *pipe;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	ret = pm_runtime_resume_and_get(ivc->dev);
>>>>>>>>>> +	if (ret)
>>>>>>>>>> +		goto err_return_buffers;
>>>>>>>>>> +
>>>>>>>>>> +	ret = video_device_pipeline_alloc_start(&ivc->vdev.dev);
>>>>>>>>>> +	if (ret) {
>>>>>>>>>> +		dev_err(ivc->dev, "failed to start media pipeline\n");
>>>>>>>>>> +		goto err_pm_runtime_put;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	rzv2h_ivc_format_configure(ivc);
>>>>>>>>>> +
>>>>>>>>>> +	ivc->buffers.sequence = 0;
>>>>>>>>>> +
>>>>>>>>>> +	spin_lock(&ivc->spinlock);
>>>>>>>>>> +	ivc->vvalid_ifp = 0;
>>>>>>>>>> +	spin_unlock(&ivc->spinlock);
>>>>>>>>> scoped_guard() maybe, and I wonder if you need this if you initialize
>>>>>>>>> the variable before resume_and_get
>>>>>>>> It was just to guarantee that it's in a known state, but I can probably drop it
>>>>>>> I don't contest resetting it to 0, I'm just pointing out you can do
>>>>>>> that earlier and avoid locking ?
>>>>>> Ah! Yes certainly true.
>>>>>>>>>> +
>>>>>>>>>> +	pipe = video_device_pipeline(&ivc->vdev.dev);
>>>>>>>>>> +	if (rzv2h_ivc_pipeline_ready(pipe)) {
>>>>>>>>>> +		ret = media_pipeline_started(pipe);
>>>>>>>>>> +		if (ret)
>>>>>>>>>> +			goto err_stop_pipeline;
>>>>>>>>>> +
>>>>>>>>>> +		media_jobs_run_jobs(ivc->sched);
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	return 0;
>>>>>>>>>> +
>>>>>>>>>> +err_stop_pipeline:
>>>>>>>>>> +	video_device_pipeline_stop(&ivc->vdev.dev);
>>>>>>>>>> +err_pm_runtime_put:
>>>>>>>>>> +	pm_runtime_put(ivc->dev);
>>>>>>>>>> +err_return_buffers:
>>>>>>>>>> +	rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_QUEUED);
>>>>>>>>>> +
>>>>>>>>>> +	return ret;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rzv2h_ivc_stop_streaming(struct vb2_queue *q)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>>>>>>>>>> +	struct media_pipeline *pipe;
>>>>>>>>>> +
>>>>>>>>>> +	pipe = video_device_pipeline(&ivc->vdev.dev);
>>>>>>>>>> +	if (rzv2h_ivc_pipeline_ready(pipe)) {
>>>>>>>>>> +		media_pipeline_stopped(pipe);
>>>>>>>>>> +		media_jobs_cancel_jobs(ivc->sched);
>>>>>>>>>> +	}
>>>>>>>>> I suspect I already asked about this, but this returns true only if
>>>>>>>>> all video devices have started right ?
>>>>>>>> Right
>>>>>>>>>       So what if ISP is stopped first
>>>>>>>>> then IVC ?
>>>>>>>> It doesn't matter which gets stopped first, it's just to make sure we run
>>>>>>>> media_pipeline_stopped() and media_jobs_cancel_jobs() whenever the _first_
>>>>>>>> video device is stopped
>>>>>>>>>> +
>>>>>>>>>> +	rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_ERROR);
>>>>>>>>>> +	video_device_pipeline_stop(&ivc->vdev.dev);
>>>>>>>>>> +	pm_runtime_mark_last_busy(ivc->dev);
>>>>>>>>>> +	pm_runtime_put_autosuspend(ivc->dev);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct vb2_ops rzv2h_ivc_vb2_ops = {
>>>>>>>>>> +	.queue_setup		= &rzv2h_ivc_queue_setup,
>>>>>>>>>> +	.buf_queue		= &rzv2h_ivc_buf_queue,
>>>>>>>>>> +	.wait_prepare		= vb2_ops_wait_prepare,
>>>>>>>>>> +	.wait_finish		= vb2_ops_wait_finish,
>>>>>>>>>> +	.start_streaming	= &rzv2h_ivc_start_streaming,
>>>>>>>>>> +	.stop_streaming		= &rzv2h_ivc_stop_streaming,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static const struct rzv2h_ivc_format *
>>>>>>>>>> +rzv2h_ivc_format_from_pixelformat(u32 fourcc)
>>>>>>>>>> +{
>>>>>>>>>> +	unsigned int i;
>>>>>>>>> nit: Could live inside the for loop
>>>>>>>> Ack
>>>>>>>>>> +
>>>>>>>>>> +	for (i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++)
>>>>>>>>>> +		if (fourcc == rzv2h_ivc_formats[i].fourcc)
>>>>>>>>>> +			return &rzv2h_ivc_formats[i];
>>>>>>>>>> +
>>>>>>>>>> +	return &rzv2h_ivc_formats[0];
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_enum_fmt_vid_out(struct file *file, void *fh,
>>>>>>>>>> +				      struct v4l2_fmtdesc *f)
>>>>>>>>>> +{
>>>>>>>>>> +	if (f->index >= ARRAY_SIZE(rzv2h_ivc_formats))
>>>>>>>>>> +		return -EINVAL;
>>>>>>>>>> +
>>>>>>>>>> +	f->pixelformat = rzv2h_ivc_formats[f->index].fourcc;
>>>>>>>>>> +	return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_g_fmt_vid_out(struct file *file, void *fh,
>>>>>>>>>> +				   struct v4l2_format *f)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = video_drvdata(file);
>>>>>>>>>> +
>>>>>>>>>> +	f->fmt.pix = ivc->format.pix;
>>>>>>>>>> +
>>>>>>>>>> +	return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rzv2h_ivc_try_fmt(struct v4l2_pix_format *pix,
>>>>>>>>>> +			      const struct rzv2h_ivc_format *fmt)
>>>>>>>>>> +{
>>>>>>>>>> +	pix->pixelformat = fmt->fourcc;
>>>>>>>>>> +
>>>>>>>>>> +	pix->width = clamp(pix->width, RZV2H_IVC_MIN_WIDTH,
>>>>>>>>>> +			   RZV2H_IVC_MAX_WIDTH);
>>>>>>>>>> +	pix->height = clamp(pix->height, RZV2H_IVC_MIN_HEIGHT,
>>>>>>>>>> +			    RZV2H_IVC_MAX_HEIGHT);
>>>>>>>>>> +
>>>>>>>>>> +	pix->field = V4L2_FIELD_NONE;
>>>>>>>>>> +	pix->colorspace = V4L2_COLORSPACE_RAW;
>>>>>>>>>> +	pix->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>>>>>>>> +	pix->quantization = V4L2_QUANTIZATION_DEFAULT;
>>>>>>>>> Same as per the subdevice use explicit values, or at least the
>>>>>>>>> DEFAULT() macros
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rzv2h_ivc_set_format(struct rzv2h_ivc *ivc,
>>>>>>>>>> +				 struct v4l2_pix_format *pix)
>>>>>>>>>> +{
>>>>>>>>>> +	const struct rzv2h_ivc_format *fmt;
>>>>>>>>>> +
>>>>>>>>>> +	fmt = rzv2h_ivc_format_from_pixelformat(pix->pixelformat);
>>>>>>>>>> +
>>>>>>>>>> +	rzv2h_ivc_try_fmt(pix, fmt);
>>>>>>>>>> +	ivc->format.pix = *pix;
>>>>>>>>>> +	ivc->format.fmt = fmt;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_s_fmt_vid_out(struct file *file, void *fh,
>>>>>>>>>> +				   struct v4l2_format *f)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = video_drvdata(file);
>>>>>>>>>> +	struct v4l2_pix_format *pix = &f->fmt.pix;
>>>>>>>>>> +
>>>>>>>>>> +	if (vb2_is_busy(&ivc->vdev.vb2q))
>>>>>>>>>> +		return -EBUSY;
>>>>>>>>>> +
>>>>>>>>>> +	rzv2h_ivc_set_format(ivc, pix);
>>>>>>>>>> +
>>>>>>>>>> +	return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_try_fmt_vid_out(struct file *file, void *fh,
>>>>>>>>>> +				     struct v4l2_format *f)
>>>>>>>>>> +{
>>>>>>>>>> +	const struct rzv2h_ivc_format *fmt;
>>>>>>>>>> +
>>>>>>>>>> +	fmt = rzv2h_ivc_format_from_pixelformat(f->fmt.pix.pixelformat);
>>>>>>>>>> +
>>>>>>>>>> +	rzv2h_ivc_try_fmt(&f->fmt.pix, fmt);
>>>>>>>>> nit: maybe remove the previous empty line and add one before return ?
>>>>>>>>>
>>>>>>>>>> +	return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_querycap(struct file *file, void *fh,
>>>>>>>>>> +			      struct v4l2_capability *cap)
>>>>>>>>>> +{
>>>>>>>>>> +	strscpy(cap->driver, "rzv2h-ivc", sizeof(cap->driver));
>>>>>>>>>> +	strscpy(cap->card, "Renesas Input Video Control", sizeof(cap->card));
>>>>>>>>>> +
>>>>>>>>>> +	return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct v4l2_ioctl_ops rzv2h_ivc_v4l2_ioctl_ops = {
>>>>>>>>>> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
>>>>>>>>>> +	.vidioc_querybuf = vb2_ioctl_querybuf,
>>>>>>>>>> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
>>>>>>>>>> +	.vidioc_qbuf = vb2_ioctl_qbuf,
>>>>>>>>>> +	.vidioc_expbuf = vb2_ioctl_expbuf,
>>>>>>>>>> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
>>>>>>>>>> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>>>>>>>>>> +	.vidioc_streamon = vb2_ioctl_streamon,
>>>>>>>>>> +	.vidioc_streamoff = vb2_ioctl_streamoff,
>>>>>>>>>> +	.vidioc_enum_fmt_vid_out = rzv2h_ivc_enum_fmt_vid_out,
>>>>>>>>>> +	.vidioc_g_fmt_vid_out = rzv2h_ivc_g_fmt_vid_out,
>>>>>>>>>> +	.vidioc_s_fmt_vid_out = rzv2h_ivc_s_fmt_vid_out,
>>>>>>>>>> +	.vidioc_try_fmt_vid_out = rzv2h_ivc_try_fmt_vid_out,
>>>>>>>>>> +	.vidioc_querycap = rzv2h_ivc_querycap,
>>>>>>>>>> +	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>>>>>>>>>> +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static const struct v4l2_file_operations rzv2h_ivc_v4l2_fops = {
>>>>>>>>>> +	.owner = THIS_MODULE,
>>>>>>>>>> +	.unlocked_ioctl = video_ioctl2,
>>>>>>>>>> +	.open = v4l2_fh_open,
>>>>>>>>>> +	.release = vb2_fop_release,
>>>>>>>>>> +	.poll = vb2_fop_poll,
>>>>>>>>>> +	.mmap = vb2_fop_mmap,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static bool rzv2h_ivc_job_ready(void *data)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = data;
>>>>>>>>>> +
>>>>>>>>>> +	guard(spinlock)(&ivc->buffers.lock);
>>>>>>>>>> +
>>>>>>>>>> +	if (list_empty(&ivc->buffers.pending))
>>>>>>>>>> +		return false;
>>>>>>>>>> +
>>>>>>>>>> +	return true;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rzv2h_ivc_job_queue(void *data)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = data;
>>>>>>>>>> +	struct rzv2h_ivc_buf *buf;
>>>>>>>>>> +
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * We need to move an entry from the pending queue to the input queue
>>>>>>>>>> +	 * here. We know that there is one, or .check_dep() would not have
>>>>>>>>>> +	 * allowed us to get this far. The entry needs to be removed or the same
>>>>>>>>>> +	 * check would allow a new job to be queued for the exact same buffer.
>>>>>>>>>> +	 */
>>>>>>>>>> +	guard(spinlock)(&ivc->buffers.lock);
>>>>>>>>>> +	buf = list_first_entry(&ivc->buffers.pending,
>>>>>>>>>> +			       struct rzv2h_ivc_buf, queue);
>>>>>>>>>> +	list_move_tail(&buf->queue, &ivc->buffers.queue);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rzv2h_ivc_job_abort(void *data)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = data;
>>>>>>>>>> +	struct rzv2h_ivc_buf *buf;
>>>>>>>>>> +
>>>>>>>>>> +	guard(spinlock)(&ivc->buffers.lock);
>>>>>>>>>> +	buf = list_first_entry(&ivc->buffers.queue,
>>>>>>>>>> +			       struct rzv2h_ivc_buf, queue);
>>>>>>>>>> +
>>>>>>>>>> +	if (buf)
>>>>>>>>>> +		list_move(&buf->queue, &ivc->buffers.pending);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int rzv2h_ivc_job_add_steps(struct media_job *job, void *data)
>>>>>>>>>> +{
>>>>>>>>>> +	struct rzv2h_ivc *ivc = data;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	ret = media_jobs_add_job_step(job, rzv2h_ivc_set_next_buffer, ivc,
>>>>>>>>>> +				      MEDIA_JOBS_FL_STEP_ANYWHERE, 0);
>>>>>>>>>> +	if (ret)
>>>>>>>>>> +		return ret;
>>>>>>>>>> +
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * This stage will be the second to last one to run - the ISP driver may
>>>>>>>>>> +	 * have some post-frame processing to do.
>>>>>>>>>> +	 */
>>>>>>>>>> +	return media_jobs_add_job_step(job, rzv2h_ivc_transfer_buffer, ivc,
>>>>>>>>>> +				       MEDIA_JOBS_FL_STEP_FROM_BACK, 1);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static struct media_job_contributor_ops rzv2h_ivc_media_job_ops = {
>>>>>>>>>> +	.add_steps	= rzv2h_ivc_job_add_steps,
>>>>>>>>>> +	.ready		= rzv2h_ivc_job_ready,
>>>>>>>>>> +	.queue		= rzv2h_ivc_job_queue,
>>>>>>>>>> +	.abort		= rzv2h_ivc_job_abort
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +int rzv2h_initialise_video_dev_and_queue(struct rzv2h_ivc *ivc,
>>>>>>>>>> +					 struct v4l2_device *v4l2_dev)
>>>>>>>>>> +{
>>>>>>>>>> +	struct v4l2_pix_format pix = { };
>>>>>>>>>> +	struct video_device *vdev;
>>>>>>>>>> +	struct vb2_queue *vb2q;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	spin_lock_init(&ivc->buffers.lock);
>>>>>>>>>> +	INIT_LIST_HEAD(&ivc->buffers.queue);
>>>>>>>>>> +	INIT_LIST_HEAD(&ivc->buffers.pending);
>>>>>>>>>> +	init_waitqueue_head(&ivc->buffers.wq);
>>>>>>>>>> +
>>>>>>>>>> +	/* Initialise vb2 queue */
>>>>>>>>>> +	vb2q = &ivc->vdev.vb2q;
>>>>>>>>>> +	vb2q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
>>>>>>>>> it's my understandin that MPLANE API is usually preferred also for devices
>>>>>>>>> that only support single planar formats
>>>>>>>> Oh ok - thanks, I'll make the switch
>>>>>>>>>> +	vb2q->io_modes = VB2_MMAP | VB2_DMABUF;
>>>>>>>>>> +	vb2q->drv_priv = ivc;
>>>>>>>>>> +	vb2q->mem_ops = &vb2_dma_contig_memops;
>>>>>>>>>> +	vb2q->ops = &rzv2h_ivc_vb2_ops;
>>>>>>>>>> +	vb2q->buf_struct_size = sizeof(struct rzv2h_ivc_buf);
>>>>>>>>>> +	vb2q->min_queued_buffers = 0;
>>>>>>>>> You can spare this, or keep it if you want it explicit
>>>>>>>> I'll probably keep it
>>>>>>>>>> +	vb2q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>>>>>>>>>> +	vb2q->lock = &ivc->lock;
>>>>>>>>>> +	vb2q->dev = ivc->dev;
>>>>>>>>>> +
>>>>>>>>>> +	ret = vb2_queue_init(vb2q);
>>>>>>>>>> +	if (ret)
>>>>>>>>>> +		return dev_err_probe(ivc->dev, ret, "vb2 queue init failed\n");
>>>>>>>>>> +
>>>>>>>>>> +	/* Initialise Video Device */
>>>>>>>>>> +	vdev = &ivc->vdev.dev;
>>>>>>>>>> +	strscpy(vdev->name, "rzv2h-ivc", sizeof(vdev->name));
>>>>>>>>>> +	vdev->release = video_device_release_empty;
>>>>>>>>>> +	vdev->fops = &rzv2h_ivc_v4l2_fops;
>>>>>>>>>> +	vdev->ioctl_ops = &rzv2h_ivc_v4l2_ioctl_ops;
>>>>>>>>>> +	vdev->lock = &ivc->lock;
>>>>>>>>>> +	vdev->v4l2_dev = v4l2_dev;
>>>>>>>>>> +	vdev->queue = vb2q;
>>>>>>>>>> +	vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
>>>>>>>>>> +	vdev->vfl_dir = VFL_DIR_TX;
>>>>>>>>>> +	video_set_drvdata(vdev, ivc);
>>>>>>>>>> +
>>>>>>>>>> +	pix.pixelformat = V4L2_PIX_FMT_SRGGB16;
>>>>>>>>>> +	pix.width = RZV2H_IVC_DEFAULT_WIDTH;
>>>>>>>>>> +	pix.height = RZV2H_IVC_DEFAULT_HEIGHT;
>>>>>>>>>> +	rzv2h_ivc_set_format(ivc, &pix);
>>>>>>>>>> +
>>>>>>>>>> +	ivc->vdev.pad.flags = MEDIA_PAD_FL_SOURCE;
>>>>>>>>>> +	ivc->vdev.dev.entity.ops = &rzv2h_ivc_media_ops;
>>>>>>>>>> +	ret = media_entity_pads_init(&ivc->vdev.dev.entity, 1, &ivc->vdev.pad);
>>>>>>>>>> +	if (ret)
>>>>>>>>>> +		goto err_release_vb2q;
>>>>>>>>>> +
>>>>>>>>>> +	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>>>>>>>>>> +	if (ret) {
>>>>>>>>>> +		dev_err(ivc->dev, "failed to register IVC video device\n");
>>>>>>>>>> +		goto err_cleanup_vdev_entity;
>>>>>>>>>> +	}
>>>>>>>>> What is the path that registers the subdevice devnode to userspace ?
>>>>>>>>> IOW I was expecting to see v4l2_device_register_subdev_nodes()
>>>>>>>>> somewhere
>>>>>>>> That's in the ISP driver - the IVC's subdevice connects through the V4L2
>>>>>>>> asynchronous API to the ISP's notifier, and the notifier's .complete()
>>>>>>>> callback runs v4l2_device_register_subdev_nodes()
>>>>>>>>
>>>>>>> Ack, sure, thanks for clarifying it!
>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +	ret = media_create_pad_link(&vdev->entity, 0, &ivc->subdev.sd.entity,
>>>>>>>>>> +				    RZV2H_IVC_SUBDEV_SINK_PAD,
>>>>>>>>>> +				    MEDIA_LNK_FL_ENABLED |
>>>>>>>>>> +				    MEDIA_LNK_FL_IMMUTABLE);
>>>>>>>>>> +	if (ret) {
>>>>>>>>>> +		dev_err(ivc->dev, "failed to create media link\n");
>>>>>>>>>> +		goto err_unregister_vdev;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	ivc->sched = media_jobs_get_scheduler(vdev->entity.graph_obj.mdev);
>>>>>>>>>> +	if (IS_ERR(ivc->sched)) {
>>>>>>>>>> +		ret = PTR_ERR(ivc->sched);
>>>>>>>>>> +		goto err_remove_link;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	ret = media_jobs_register_job_contributor(ivc->sched,
>>>>>>>>>> +						  &rzv2h_ivc_media_job_ops, ivc,
>>>>>>>>>> +						  MEDIA_JOB_TYPE_PIPELINE_PULSE);
>>>>>>>>>> +	if (ret)
>>>>>>>>>> +		goto err_put_media_job_scheduler;
>>>>>>>>>> +
>>>>>>>>>> +	return 0;
>>>>>>>>>> +
>>>>>>>>>> +err_put_media_job_scheduler:
>>>>>>>>>> +	media_jobs_put_scheduler(ivc->sched);
>>>>>>>>>> +err_remove_link:
>>>>>>>>>> +	media_entity_remove_links(&vdev->entity);
>>>>>>>>>> +err_unregister_vdev:
>>>>>>>>>> +	video_unregister_device(vdev);
>>>>>>>>>> +err_cleanup_vdev_entity:
>>>>>>>>>> +	media_entity_cleanup(&vdev->entity);
>>>>>>>>>> +err_release_vb2q:
>>>>>>>>>> +	vb2_queue_release(vb2q);
>>>>>>>>>> +
>>>>>>>>>> +	return ret;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc)
>>>>>>>>>> +{
>>>>>>>>>> +	struct video_device *vdev = &ivc->vdev.dev;
>>>>>>>>>> +	struct vb2_queue *vb2q = &ivc->vdev.vb2q;
>>>>>>>>>> +
>>>>>>>>>> +	if (!ivc->sched)
>>>>>>>>>> +		return;
>>>>>>>>>> +
>>>>>>>>>> +	media_jobs_put_scheduler(ivc->sched);
>>>>>>>>>> +	vb2_video_unregister_device(vdev);
>>>>>>>>>> +	media_entity_cleanup(&vdev->entity);
>>>>>>>>>> +	vb2_queue_release(vb2q);
>>>>>>>>> Shouldn't you get here also in case of !ivc->sched ?
>>>>>>>> This driver (at least in this version) should always have a sched pointer,
>>>>>>>> so this was just a convenient way to check if initialisation finished before
>>>>>>>> trying to deinit anything...it'll probably change though.
>>>>>>> I see, a comment to explain that might be enough!
>>>>>> Sure - I'll add one.
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>> Thanks
>>>>>>>       j
>>>>>>>
>>>>>>>
>>>>>>>>>> +}
>>>>>>>>>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 0000000000000000000000000000000000000000..d2e310ce868125772d97259619b9369ccbcefe3d
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
>>>>>>>>>> @@ -0,0 +1,133 @@
>>>>>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>>>>>>> +/*
>>>>>>>>>> + * Renesas RZ/V2H Input Video Control Block driver
>>>>>>>>>> + *
>>>>>>>>>> + * Copyright (C) 2025 Ideas on Board Oy
>>>>>>>>>> + */
>>>>>>>>>> +
>>>>>>>>>> +#include <linux/clk.h>
>>>>>>>>>> +#include <linux/list.h>
>>>>>>>>>> +#include <linux/mutex.h>
>>>>>>>>>> +#include <linux/reset.h>
>>>>>>>>>> +#include <linux/spinlock.h>
>>>>>>>>>> +#include <linux/types.h>
>>>>>>>>>> +#include <linux/videodev2.h>
>>>>>>>>>> +#include <linux/wait.h>
>>>>>>>>>> +
>>>>>>>>>> +#include <media/media-entity.h>
>>>>>>>>>> +#include <media/v4l2-device.h>
>>>>>>>>>> +#include <media/v4l2-subdev.h>
>>>>>>>>>> +#include <media/videobuf2-core.h>
>>>>>>>>>> +#include <media/videobuf2-v4l2.h>
>>>>>>>>>> +
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_PLNUM			0x0000
>>>>>>>>>> +#define RZV2H_IVC_ONE_EXPOSURE				0x00
>>>>>>>>>> +#define RZV2H_IVC_TWO_EXPOSURE				0x01
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_PXFMT			0x0004
>>>>>>>>>> +#define RZV2H_IVC_INPUT_FMT_MIPI			(0 << 16)
>>>>>>>>>> +#define RZV2H_IVC_INPUT_FMT_CRU_PACKED			(1 << 16)
>>>>>>>>>> +#define RZV2H_IVC_PXFMT_DTYPE				GENMASK(7, 0)
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_SADDL_P0			0x0010
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_SADDH_P0			0x0014
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_SADDL_P1			0x0018
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_SADDH_P1			0x001c
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_HSIZE			0x0020
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_VSIZE			0x0024
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_BLANK			0x0028
>>>>>>>>>> +#define RZV2H_IVC_VBLANK(x)				((x) << 16)
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_STRD			0x0030
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_ISSU			0x0040
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_ERACT			0x0048
>>>>>>>>>> +#define RZV2H_IVC_REG_FM_CONTEXT			0x0100
>>>>>>>>>> +#define RZV2H_IVC_SOFTWARE_CFG				0x00
>>>>>>>>>> +#define RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG		BIT(0)
>>>>>>>>>> +#define RZV2H_IVC_MULTI_CONTEXT_SW_HW_CFG		BIT(1)
>>>>>>>>>> +#define RZV2H_IVC_REG_FM_MCON				0x0104
>>>>>>>>>> +#define RZV2H_IVC_REG_FM_FRCON				0x0108
>>>>>>>>>> +#define RZV2H_IVC_REG_FM_STOP				0x010c
>>>>>>>>>> +#define RZV2H_IVC_REG_FM_INT_EN				0x0120
>>>>>>>>>> +#define RZV2H_IVC_VVAL_IFPE				BIT(0)
>>>>>>>>>> +#define RZV2H_IVC_REG_FM_INT_STA			0x0124
>>>>>>>>>> +#define RZV2H_IVC_REG_AXIRX_FIFOCAP0			0x0208
>>>>>>>>>> +#define RZV2H_IVC_REG_CORE_CAPCON			0x020c
>>>>>>>>>> +#define RZV2H_IVC_REG_CORE_FIFOCAP0			0x0228
>>>>>>>>>> +#define RZV2H_IVC_REG_CORE_FIFOCAP1			0x022c
>>>>>>>>>> +
>>>>>>>>>> +#define RZV2H_IVC_MIN_WIDTH				640
>>>>>>>>>> +#define RZV2H_IVC_MAX_WIDTH				4096
>>>>>>>>>> +#define RZV2H_IVC_MIN_HEIGHT				480
>>>>>>>>>> +#define RZV2H_IVC_MAX_HEIGHT				4096
>>>>>>>>>> +#define RZV2H_IVC_DEFAULT_WIDTH				1920
>>>>>>>>>> +#define RZV2H_IVC_DEFAULT_HEIGHT			1080
>>>>>>>>>> +
>>>>>>>>>> +#define RZV2H_IVC_NUM_CLOCKS				3
>>>>>>>>>> +#define RZV2H_IVC_NUM_RESETS				3
>>>>>>>>>> +
>>>>>>>>>> +struct device;
>>>>>>>>>> +
>>>>>>>>>> +enum rzv2h_ivc_subdev_pads {
>>>>>>>>>> +	RZV2H_IVC_SUBDEV_SINK_PAD,
>>>>>>>>>> +	RZV2H_IVC_SUBDEV_SOURCE_PAD,
>>>>>>>>>> +	RZV2H_IVC_NUM_SUBDEV_PADS
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +struct rzv2h_ivc_format {
>>>>>>>>>> +	u32 fourcc;
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * The CRU packed pixel formats are bayer-order agnostic, so each could
>>>>>>>>>> +	 * support any one of the 4 possible media bus formats.
>>>>>>>>>> +	 */
>>>>>>>>>> +	u32 mbus_codes[4];
>>>>>>>>>> +	u8 dtype;
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +struct rzv2h_ivc {
>>>>>>>>>> +	struct device *dev;
>>>>>>>>>> +	void __iomem *base;
>>>>>>>>>> +	struct clk_bulk_data clks[RZV2H_IVC_NUM_CLOCKS];
>>>>>>>>>> +	struct reset_control_bulk_data resets[RZV2H_IVC_NUM_RESETS];
>>>>>>>>>> +	int irqnum;
>>>>>>>>>> +	u8 vvalid_ifp;
>>>>>>>>>> +
>>>>>>>>>> +	struct {
>>>>>>>>>> +		struct video_device dev;
>>>>>>>>>> +		struct vb2_queue vb2q;
>>>>>>>>>> +		struct media_pad pad;
>>>>>>>>>> +	} vdev;
>>>>>>>>>> +
>>>>>>>>>> +	struct {
>>>>>>>>>> +		struct v4l2_subdev sd;
>>>>>>>>>> +		struct media_pad pads[RZV2H_IVC_NUM_SUBDEV_PADS];
>>>>>>>>>> +	} subdev;
>>>>>>>>>> +
>>>>>>>>>> +	struct {
>>>>>>>>>> +		/* Spinlock to guard buffer queue */
>>>>>>>>>> +		spinlock_t lock;
>>>>>>>>>> +		wait_queue_head_t wq;
>>>>>>>>>> +		struct list_head queue;
>>>>>>>>>> +		struct list_head pending;
>>>>>>>>>> +		struct rzv2h_ivc_buf *curr;
>>>>>>>>>> +		unsigned int sequence;
>>>>>>>>>> +	} buffers;
>>>>>>>>>> +
>>>>>>>>>> +	struct media_job_scheduler *sched;
>>>>>>>>>> +
>>>>>>>>>> +	struct {
>>>>>>>>>> +		struct v4l2_pix_format pix;
>>>>>>>>>> +		const struct rzv2h_ivc_format *fmt;
>>>>>>>>>> +	} format;
>>>>>>>>>> +
>>>>>>>>>> +	/* Mutex to provide to vb2 */
>>>>>>>>>> +	struct mutex lock;
>>>>>>>>>> +	/* Lock to protect the interrupt counter */
>>>>>>>>>> +	spinlock_t spinlock;
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +int rzv2h_initialise_video_dev_and_queue(struct rzv2h_ivc *ivc,
>>>>>>>>>> +					 struct v4l2_device *v4l2_dev);
>>>>>>>>>> +void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc);
>>>>>>>>>> +int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc);
>>>>>>>>>> +void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc);
>>>>>>>>>> +void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val);
>>>>>>>>>> +void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
>>>>>>>>>> +			   u32 mask, u32 val);
>>>>>>>>>>
>>>>>>>>> As agreed, I didn't review the job scheduler part but only the IVC
>>>>>>>>> specific bits. A few nits here and there and next version should be
>>>>>>>>> good to go!
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>        j
>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> 2.34.1
>>>>>>>>>>
>>>>>>>>>>