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 ?
However given the "three return values" problem, we might want to keep
_try_run() separate ?
>
> 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
> > > > > > > > >
> > > > > > > > >
