Hi Bingbu,
On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@xxxxxxxxx wrote:
> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> Register v4l2 video device and setup the vb2 queue to
> support basic video capture. Video streaming callback
> will trigger the input system driver to construct a
> input system stream configuration for firmware based on
> data type and stream ID and then queue buffers to firmware
> to do capture.
>
> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> ---
...
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
...
> +static int video_release(struct file *file)
> +{
> + return vb2_fop_release(file);
> +}
...
> +/*
> + * Do everything that's needed to initialise things related to video
> + * buffer queue, video node, and the related media entity. The caller
> + * is expected to assign isys field and set the name of the video
> + * device.
> + */
> +int ipu6_isys_video_init(struct ipu6_isys_video *av)
> +{
> + struct v4l2_format format = {
> + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> + .fmt.pix_mp = {
> + .width = 1920,
> + .height = 1080,
> + },
> + };
> + int ret;
> +
> + mutex_init(&av->mutex);
> + av->vdev.device_caps = V4L2_CAP_STREAMING | V4L2_CAP_IO_MC |
> + V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> + av->vdev.vfl_dir = VFL_DIR_RX;
> +
> + ret = ipu6_isys_queue_init(&av->aq);
> + if (ret)
> + goto out_free_watermark;
> +
> + av->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> + ret = media_entity_pads_init(&av->vdev.entity, 1, &av->pad);
> + if (ret)
> + goto out_ipu6_isys_queue_cleanup;
> +
> + av->vdev.entity.ops = &entity_ops;
> + av->vdev.release = video_device_release_empty;
> + av->vdev.fops = &isys_fops;
> + av->vdev.v4l2_dev = &av->isys->v4l2_dev;
> + if (!av->vdev.ioctl_ops)
> + av->vdev.ioctl_ops = &ioctl_ops_mplane;
> + av->vdev.queue = &av->aq.vbq;
> + av->vdev.lock = &av->mutex;
> +
> + ipu6_isys_video_try_fmt_vid_mplane(av, &format.fmt.pix_mp);
> + av->mpix = format.fmt.pix_mp;
> +
> + set_bit(V4L2_FL_USES_V4L2_FH, &av->vdev.flags);
> + video_set_drvdata(&av->vdev, av);
> +
> + ret = video_register_device(&av->vdev, VFL_TYPE_VIDEO, -1);
> + if (ret)
> + goto out_media_entity_cleanup;
> +
> + return ret;
> +
> +out_media_entity_cleanup:
> + video_unregister_device(&av->vdev);
> + media_entity_cleanup(&av->vdev.entity);
> +
> +out_ipu6_isys_queue_cleanup:
> + ipu6_isys_queue_cleanup(&av->aq);
> +
> +out_free_watermark:
> + mutex_destroy(&av->mutex);
> +
> + return ret;
> +}
> +
> +void ipu6_isys_video_cleanup(struct ipu6_isys_video *av)
> +{
> + video_unregister_device(&av->vdev);
> + media_entity_cleanup(&av->vdev.entity);
> + mutex_destroy(&av->mutex);
> + ipu6_isys_queue_cleanup(&av->aq);
> +}
I believe you should use vb2_video_unregister_device here instead of
video_unregister_device in both ipu6_isys_video_init and
ipu6_isys_video_cleanup, and remove the call to
ipu6_isys_queue_cleanup.
Commit f729ef5796d8 from Hans Verkuil that added
vb2_video_unregister_device states: "If a driver calls
(_)vb2_fop_release(), then such a driver should also call
vb2_video_unregister_device() instead of video_unregister_device().
This helper will call vb2_queue_release() if a filehandle is marked as
owner of the queue. This ensures that at unregister time any streaming
is cancelled and all buffers are returned to userspace."
In my testing, using IPU4, this prevents some crashes when unbinding
the PCI device while streaming.
/Andreas
