Re: [PATCH v7 4/6] media: starfive: camss: Add video driver

[Jack Zhu]

On 2023/8/2 2:47, Laurent Pinchart wrote:
> Hi Jack,
> 
> On Tue, Aug 01, 2023 at 02:23:07PM +0800, Jack Zhu wrote:
>> On 2023/7/27 16:49, Hans Verkuil wrote:
>> > On 19/06/2023 13:28, Jack Zhu wrote:
>> >> Add video driver for StarFive Camera Subsystem.
>> >> 
>> >> Signed-off-by: Jack Zhu <jack.zhu@xxxxxxxxxxxxxxxx>
>> >> ---
>> >>  .../media/platform/starfive/camss/Makefile    |   4 +-
>> >>  .../media/platform/starfive/camss/stf_video.c | 724 ++++++++++++++++++
>> >>  .../media/platform/starfive/camss/stf_video.h |  92 +++
>> >>  3 files changed, 819 insertions(+), 1 deletion(-)
>> >>  create mode 100644 drivers/media/platform/starfive/camss/stf_video.c
>> >>  create mode 100644 drivers/media/platform/starfive/camss/stf_video.h
> 
> [snip]
> 
>> >> diff --git a/drivers/media/platform/starfive/camss/stf_video.c b/drivers/media/platform/starfive/camss/stf_video.c
>> >> new file mode 100644
>> >> index 000000000000..2e6472fe51c6
>> >> --- /dev/null
>> >> +++ b/drivers/media/platform/starfive/camss/stf_video.c
>> >> @@ -0,0 +1,724 @@
> 
> [snip]
> 
>> >> +int stf_video_register(struct stfcamss_video *video,
>> >> +		       struct v4l2_device *v4l2_dev, const char *name)
>> >> +{
>> >> +	struct video_device *vdev;
>> >> +	struct vb2_queue *q;
>> >> +	struct media_pad *pad = &video->pad;
>> >> +	int ret;
>> >> +
>> >> +	vdev = &video->vdev;
>> >> +
>> >> +	mutex_init(&video->q_lock);
>> >> +
>> >> +	q = &video->vb2_q;
>> >> +	q->drv_priv = video;
>> >> +	q->mem_ops = &vb2_dma_contig_memops;
>> >> +	q->ops = &stf_video_vb2_q_ops;
>> >> +	q->type = video->type;
>> >> +	q->io_modes = VB2_DMABUF | VB2_MMAP | VB2_READ;
>> > 
>> > VB2_READ support does not generally make sense for uncompressed video since
>> > read() always requires a memcpy, and that makes it very inefficient.
>> > 
>> > It doesn't hurt though, so it is up to you whether or not you want this.
>> 
>> Yes, we would like to retain this feature to meet some possible special needs.
> 
> The issue with enabling READ support in drivers is that it encourages
> applications to do the wrong thing. If you want to keep it, I'd like to
> know what use cases you envision would strongly require it.
> 

OK, I'll drop it. Thank you!

>> >> +	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> >> +	q->buf_struct_size = sizeof(struct stfcamss_buffer);
>> >> +	q->dev = video->stfcamss->dev;
>> >> +	q->lock = &video->q_lock;
>> >> +	q->min_buffers_needed = STFCAMSS_MIN_BUFFERS;
>> >> +	ret = vb2_queue_init(q);
>> >> +	if (ret < 0) {
>> >> +		dev_err(video->stfcamss->dev,
>> >> +			"Failed to init vb2 queue: %d\n", ret);
>> >> +		goto err_vb2_init;
>> >> +	}
>> >> +
>> >> +	pad->flags = MEDIA_PAD_FL_SINK;
>> >> +	ret = media_entity_pads_init(&vdev->entity, 1, pad);
>> >> +	if (ret < 0) {
>> >> +		dev_err(video->stfcamss->dev,
>> >> +			"Failed to init video entity: %d\n", ret);
>> >> +		goto err_vb2_init;
>> >> +	}
>> >> +
>> >> +	mutex_init(&video->lock);
>> >> +
>> >> +	if (video->id == STF_V_LINE_WR) {
>> >> +		video->formats = formats_pix_wr;
>> >> +		video->nformats = ARRAY_SIZE(formats_pix_wr);
>> >> +		video->bpl_alignment = 8;
>> >> +	} else {
>> >> +		video->formats = formats_pix_isp;
>> >> +		video->nformats = ARRAY_SIZE(formats_pix_isp);
>> >> +		video->bpl_alignment = 1;
>> >> +	}
>> >> +
>> >> +	ret = stf_video_init_format(video);
>> >> +	if (ret < 0) {
>> >> +		dev_err(video->stfcamss->dev,
>> >> +			"Failed to init format: %d\n", ret);
>> >> +		goto err_vid_init_format;
>> >> +	}
>> >> +
>> >> +	vdev->fops = &stf_vid_fops;
>> >> +	vdev->ioctl_ops = &stf_vid_ioctl_ops;
>> >> +	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
>> >> +	vdev->vfl_dir = VFL_DIR_RX;
>> >> +	vdev->device_caps |= V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
>> >> +	vdev->release = stf_video_release;
>> >> +	vdev->v4l2_dev = v4l2_dev;
>> >> +	vdev->queue = &video->vb2_q;
>> >> +	vdev->lock = &video->lock;
>> >> +	strscpy(vdev->name, name, sizeof(vdev->name));
>> >> +
>> >> +	ret = video_register_device(vdev, VFL_TYPE_VIDEO, video->id);
>> >> +	if (ret < 0) {
>> >> +		dev_err(video->stfcamss->dev,
>> >> +			"Failed to register video device: %d\n", ret);
>> >> +		goto err_vid_reg;
>> >> +	}
>> >> +
>> >> +	video_set_drvdata(vdev, video);
>> >> +	return 0;
>> >> +
>> >> +err_vid_reg:
>> >> +err_vid_init_format:
>> >> +	media_entity_cleanup(&vdev->entity);
>> >> +	mutex_destroy(&video->lock);
>> >> +err_vb2_init:
>> >> +	mutex_destroy(&video->q_lock);
>> >> +	return ret;
>> >> +}
> 
> [snip]
>