Hi Hans,
Thank you for the review.
On Sat, Apr 27, 2024 at 05:44:48PM +0200, Hans Verkuil wrote:
...
> > +static int queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
> > + unsigned int *num_planes, unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct ipu6_isys_queue *aq = vb2_queue_to_isys_queue(q);
> > + struct ipu6_isys_video *av = ipu6_isys_queue_to_video(aq);
> > + struct device *dev = &av->isys->adev->auxdev.dev;
> > + u32 size = av->pix_fmt.sizeimage;
> > +
> > + /* num_planes == 0: we're being called through VIDIOC_REQBUFS */
> > + if (!*num_planes) {
> > + sizes[0] = size;
> > + } else if (sizes[0] < size) {
> > + dev_err(dev, "%s: queue setup: size %u < %u\n",
> > + av->vdev.name, sizes[0], size);
> > + return -EINVAL;
> > + }
> > +
> > + *num_planes = 1;
> > + alloc_devs[0] = aq->dev;
>
> Don't set this. This is already done by the vb2 core. Setting this is only
> necessary for very weird hardware that uses a different device per plane.
I'll drop this from v7.
> > +static int start_streaming(struct vb2_queue *q, unsigned int count)
> > +{
> > + struct ipu6_isys_queue *aq = vb2_queue_to_isys_queue(q);
> > + struct ipu6_isys_video *av = ipu6_isys_queue_to_video(aq);
> > + struct device *dev = &av->isys->adev->auxdev.dev;
> > + struct ipu6_isys_buffer_list __bl, *bl = NULL;
> > + struct ipu6_isys_stream *stream;
> > + struct media_entity *source_entity = NULL;
> > + int nr_queues, ret;
> > +
> > + dev_dbg(dev, "stream: %s: width %u, height %u, css pixelformat %u\n",
> > + av->vdev.name, av->pix_fmt.width, av->pix_fmt.height,
> > + av->pfmt->css_pixelformat);
> > +
> > + ret = ipu6_isys_setup_video(av, &source_entity, &nr_queues);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to setup video\n");
> > + goto out_return_buffers;
> > + }
> > +
> > + ret = ipu6_isys_link_fmt_validate(aq);
> > + if (ret) {
> > + dev_err(dev,
> > + "%s: link format validation failed (%d)\n",
> > + av->vdev.name, ret);
> > + goto out_pipeline_stop;
> > + }
> > +
> > + ret = ipu6_isys_fw_open(av->isys);
> > + if (ret)
> > + goto out_pipeline_stop;
> > +
> > + stream = av->stream;
> > + mutex_lock(&stream->mutex);
> > + if (!stream->nr_streaming) {
> > + ret = ipu6_isys_video_prepare_stream(av, source_entity,
> > + nr_queues);
> > + if (ret)
> > + goto out_fw_close;
> > + }
> > +
> > + stream->nr_streaming++;
> > + dev_dbg(dev, "queue %u of %u\n", stream->nr_streaming,
> > + stream->nr_queues);
> > +
> > + list_add(&aq->node, &stream->queues);
> > + ipu6_isys_set_csi2_streams_status(av, true);
> > + ipu6_isys_configure_stream_watermark(av, true);
> > + ipu6_isys_update_stream_watermark(av, true);
> > +
> > + if (stream->nr_streaming != stream->nr_queues)
> > + goto out;
> > +
> > + bl = &__bl;
> > + ret = buffer_list_get(stream, bl);
> > + if (ret < 0) {
> > + dev_dbg(dev,
> > + "no buffer available, postponing streamon\n");
>
> You can ensure that start_streaming is only called when there is at least
> one buffer queued by setting q->min_queued_buffers to 1 in ipu6_isys_queue_init().
> That is the recommended method for dealing with such requirements.
Ack.
I'll still leave this as a sanity check and turn it into an error.
>
> > + goto out;
> > + }
> > +
> > + ret = ipu6_isys_stream_start(av, bl, false);
> > + if (ret)
> > + goto out_stream_start;
> > +
> > +out:
> > + mutex_unlock(&stream->mutex);
> > +
> > + return 0;
> > +
> > +out_stream_start:
> > + ipu6_isys_update_stream_watermark(av, false);
> > + list_del(&aq->node);
> > + stream->nr_streaming--;
> > +
> > +out_fw_close:
> > + mutex_unlock(&stream->mutex);
> > + ipu6_isys_fw_close(av->isys);
> > +
> > +out_pipeline_stop:
> > + ipu6_isys_stream_cleanup(av);
> > +
> > +out_return_buffers:
> > + return_buffers(aq, VB2_BUF_STATE_QUEUED);
> > +
> > + return ret;
> > +}
...
> > +static int vidioc_s_fmt_vid_cap(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct ipu6_isys_video *av = video_drvdata(file);
> > +
> > + if (av->aq.vbq.streaming)
>
> This should call vb2_is_busy() instead of checking the streaming flag.
>
> I'm pretty sure v4l2-compliance checks for that, but perhaps the current
> FAILs prevented it from getting that far.
>
> According to the cover letter, those FAILs in v4l2-compliance are because
> of a limitation in v4l2-compliance: that should be fixed so that this
> driver can pass the full v4l2-compliance test. I suspect that not doing that
> causes this issue to be missed.
I'll switch to vb2_is_busy() in v7.
--
Kind regards,
Sakari Ailus
