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

[Jack Zhu]

Hi Laurent,

Thank you for your comments.

On 2023/7/27 23:25, Laurent Pinchart wrote:
> Hi Jack,
> 
> Thank you for the patch.
> 
> On Mon, Jun 19, 2023 at 07:28:36PM +0800, 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
>> 
>> diff --git a/drivers/media/platform/starfive/camss/Makefile b/drivers/media/platform/starfive/camss/Makefile
>> index d56ddd078a71..eb457917a914 100644
>> --- a/drivers/media/platform/starfive/camss/Makefile
>> +++ b/drivers/media/platform/starfive/camss/Makefile
>> @@ -3,6 +3,8 @@
>>  # Makefile for StarFive Camera Subsystem driver
>>  #
>>  
>> -starfive-camss-objs += stf_camss.o
>> +starfive-camss-objs += \
>> +		stf_camss.o \
>> +		stf_video.o
>>  
>>  obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o
>> 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 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * stf_video.c
>> + *
>> + * StarFive Camera Subsystem - V4L2 device node
>> + *
>> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/pm_runtime.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-event.h>
>> +#include <media/v4l2-mc.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +
>> +#include "stf_camss.h"
>> +#include "stf_video.h"
>> +
>> +static const struct stfcamss_format_info formats_pix_wr[] = {
>> +	{
>> +		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
>> +		.pixelformat = V4L2_PIX_FMT_SRGGB10,
>> +		.planes = 1,
>> +		.vsub = { 1 },
>> +		.bpp = 10,
>> +	},
>> +	{
>> +		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>> +		.pixelformat = V4L2_PIX_FMT_SGRBG10,
>> +		.planes = 1,
>> +		.vsub = { 1 },
>> +		.bpp = 10,
>> +	},
>> +	{
>> +		.code = MEDIA_BUS_FMT_SGBRG10_1X10,
>> +		.pixelformat = V4L2_PIX_FMT_SGBRG10,
>> +		.planes = 1,
>> +		.vsub = { 1 },
>> +		.bpp = 10,
>> +	},
>> +	{
>> +		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
>> +		.pixelformat = V4L2_PIX_FMT_SBGGR10,
>> +		.planes = 1,
>> +		.vsub = { 1 },
>> +		.bpp = 10,
>> +	},
>> +};
>> +
>> +static const struct stfcamss_format_info formats_pix_isp[] = {
>> +	{
>> +		.code = MEDIA_BUS_FMT_Y12_1X12,
>> +		.pixelformat = V4L2_PIX_FMT_NV12,
>> +		.planes = 2,
>> +		.vsub = { 1, 2 },
>> +		.bpp = 8,
>> +	},
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Helper functions
>> + */
>> +
>> +static int video_find_format(u32 code, u32 pixelformat,
>> +			     struct stfcamss_video *video)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < video->nformats; ++i) {
>> +		if (video->formats[i].code == code &&
>> +		    video->formats[i].pixelformat == pixelformat)
>> +			return i;
>> +	}
>> +
>> +	for (i = 0; i < video->nformats; ++i)
>> +		if (video->formats[i].code == code)
>> +			return i;
>> +
>> +	for (i = 0; i < video->nformats; ++i)
>> +		if (video->formats[i].pixelformat == pixelformat)
>> +			return i;
>> +
> 
> This looks weird, I don't think it does what you expect below. I think
> you can drop the function, and instead use video_get_pfmt_by_mcode() to
> convert the mbus code to a pixel format, and compare it to the active
> pixel format in video_check_format().
> 
>> +	return -EINVAL;
>> +}
>> +
>> +static int __video_try_fmt(struct stfcamss_video *video, struct v4l2_format *f)
>> +{
>> +	struct v4l2_pix_format *pix;
>> +	const struct stfcamss_format_info *fi;
>> +	u32 width, height;
>> +	u32 bpl;
>> +	unsigned int i;
>> +
>> +	pix = &f->fmt.pix;
> 
> You can initialize pix when declaring it.
> 
>> +
>> +	for (i = 0; i < video->nformats; i++)
>> +		if (pix->pixelformat == video->formats[i].pixelformat)
>> +			break;
>> +
> 
> 	for (i = 0; i < video->nformats; i++) {
> 		if (pix->pixelformat == video->formats[i].pixelformat)
> 			break;
> 	}
> 
> But a helper function that looks up a format by pixelformat, similar to
> video_get_pfmt_by_mcode(), would be useful. I think I would make all
> those helpers return a const struct stfcamss_format_info pointer instead
> of an index.
> 
>> +	if (i == video->nformats)
>> +		i = 0; /* default format */
>> +
>> +	fi = &video->formats[i];
>> +	width = pix->width;
>> +	height = pix->height;
>> +
>> +	memset(pix, 0, sizeof(*pix));
>> +
>> +	pix->pixelformat = fi->pixelformat;
>> +	pix->width = clamp_t(u32, width, STFCAMSS_FRAME_MIN_WIDTH,
>> +			     STFCAMSS_FRAME_MAX_WIDTH);
>> +	pix->height = clamp_t(u32, height, STFCAMSS_FRAME_MIN_HEIGHT,
>> +			      STFCAMSS_FRAME_MAX_HEIGHT);
>> +	bpl = pix->width * fi->bpp / 8;
>> +	bpl = ALIGN(bpl, video->bpl_alignment);
>> +	pix->bytesperline = bpl;
> 
> Does the hardware support configuring the stride ?
> 

The hardware does not support.

>> +
>> +	for (i = 0; i < fi->planes; ++i)
>> +		pix->sizeimage += bpl * pix->height / fi->vsub[i];
>> +
>> +	pix->field = V4L2_FIELD_NONE;
>> +	pix->colorspace = V4L2_COLORSPACE_SRGB;
>> +	pix->flags = 0;
>> +	pix->ycbcr_enc =
>> +		V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
>> +	pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
>> +							  pix->colorspace,
>> +							  pix->ycbcr_enc);
>> +	pix->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pix->colorspace);
> 
> This doesn't seem right for the processed output.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int stf_video_init_format(struct stfcamss_video *video)
>> +{
>> +	int ret;
>> +	struct v4l2_format format = {
>> +		.type = video->type,
>> +		.fmt.pix = {
>> +			.width = 1920,
>> +			.height = 1080,
>> +			.pixelformat = V4L2_PIX_FMT_RGB565,
> 
> That format doesn't seem supported, let's pick V4L2_PIX_FMT_NV12.
> 
>> +		},
>> +	};
>> +
>> +	ret = __video_try_fmt(video, &format);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	video->active_fmt = format;
>> +
>> +	return 0;
>> +}
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Video queue operations
>> + */
>> +
>> +static int video_queue_setup(struct vb2_queue *q,
>> +			     unsigned int *num_buffers,
>> +			     unsigned int *num_planes,
>> +			     unsigned int sizes[],
>> +			     struct device *alloc_devs[])
>> +{
>> +	struct stfcamss_video *video = vb2_get_drv_priv(q);
>> +	const struct v4l2_pix_format *format = &video->active_fmt.fmt.pix;
>> +
>> +	if (*num_planes) {
>> +		if (*num_planes != 1)
>> +			return -EINVAL;
>> +
>> +		if (sizes[0] < format->sizeimage)
>> +			return -EINVAL;
>> +	}
>> +
>> +	*num_planes = 1;
>> +	sizes[0] = format->sizeimage;
>> +	if (!sizes[0])
>> +		dev_err(video->stfcamss->dev,
>> +			"%s: error size is zero!!!\n", __func__);
> 
> Shouldn't you return an error ? Also, use dev_dbg(), printing an error
> message based on a condition that can easily be triggered by
> unpriviledge userspace opens the door to applications flooding the
> kernel log.
> 
>> +
>> +	dev_dbg(video->stfcamss->dev, "planes = %d, size = %d\n",
>> +		*num_planes, sizes[0]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int video_buf_init(struct vb2_buffer *vb)
>> +{
>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> +	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct stfcamss_buffer *buffer =
>> +		container_of(vbuf, struct stfcamss_buffer, vb);
> 
> A static inline to_stfcamss_buffer() function that wraps the
> container_of() would be nice. You can use it below too.
> 
>> +	const struct v4l2_pix_format *fmt = &video->active_fmt.fmt.pix;
>> +	dma_addr_t *paddr;
>> +
>> +	paddr = vb2_plane_cookie(vb, 0);
>> +	buffer->addr[0] = *paddr;
>> +
>> +	if (fmt->pixelformat == V4L2_PIX_FMT_NV12 ||
>> +	    fmt->pixelformat == V4L2_PIX_FMT_NV21 ||
>> +	    fmt->pixelformat == V4L2_PIX_FMT_NV16 ||
>> +	    fmt->pixelformat == V4L2_PIX_FMT_NV61)
> 
> Only V4L2_PIX_FMT_NV12 is listed in formats_pix_isp. Does the hardware
> support the other formats ? If so, it would be nice to support them
> already.
> 
>> +		buffer->addr[1] =
>> +			buffer->addr[0] + fmt->bytesperline * fmt->height;
> 
> As the hardware supports non-contiguous planes, you should use the
> MPLANE API (V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) and support the NV*M
> formats in addition to the NV* formats.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int video_buf_prepare(struct vb2_buffer *vb)
>> +{
>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> +	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> +	const struct v4l2_pix_format *fmt = &video->active_fmt.fmt.pix;
>> +
>> +	if (fmt->sizeimage > vb2_plane_size(vb, 0)) {
>> +		dev_err(video->stfcamss->dev,
> 
> dev_dbg() here too.
> 
>> +			"sizeimage = %d, plane size = %d\n",
>> +			fmt->sizeimage, (unsigned int)vb2_plane_size(vb, 0));
> 
> Both are unsigned, use %u instead of %d.
> 
>> +		return -EINVAL;
>> +	}
>> +	vb2_set_plane_payload(vb, 0, fmt->sizeimage);
>> +
>> +	vbuf->field = V4L2_FIELD_NONE;
>> +
>> +	return 0;
>> +}
>> +
>> +static void video_buf_queue(struct vb2_buffer *vb)
>> +{
>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> +	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct stfcamss_buffer *buffer =
>> +		container_of(vbuf, struct stfcamss_buffer, vb);
>> +
>> +	video->ops->queue_buffer(video, buffer);
>> +}
>> +
>> +/*
>> + * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
>> + * @mbus: v4l2_mbus_framefmt format (input)
>> + * @pix: v4l2_pix_format_mplane format (output)
>> + * @f: a pointer to formats array element to be used for the conversion
>> + * @alignment: bytesperline alignment value
>> + *
>> + * Fill the output pix structure with information from the input mbus format.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static int video_mbus_to_pix(const struct v4l2_mbus_framefmt *mbus,
>> +			     struct v4l2_pix_format *pix,
>> +			     const struct stfcamss_format_info *f,
>> +			     unsigned int alignment)
>> +{
>> +	u32 bytesperline;
>> +	unsigned int i;
>> +
>> +	memset(pix, 0, sizeof(*pix));
>> +	v4l2_fill_pix_format(pix, mbus);
>> +	pix->pixelformat = f->pixelformat;
>> +	bytesperline = pix->width * f->bpp / 8;
>> +	bytesperline = ALIGN(bytesperline, alignment);
>> +	pix->bytesperline = bytesperline;
>> +
>> +	for (i = 0; i < f->planes; ++i)
>> +		pix->sizeimage += bytesperline * pix->height / f->vsub[i];
> 
> This function is used for validation of the format only, the
> bytesperline and sizeimage values are never used. You can simplify the
> driver by dropping the function and comparing the width, height and
> field of the subdev and video device from the v4l2_mbus_framefmt and
> v4l2_pix_format respectively in video_check_format().
> video_get_subdev_format() will then take a v4l2_mbus_framefmt pointer,
> not a v4l2_pix_format.
> 
> The format match check still needs conversion of the 
> 
> To check the format, you need to convert the mbus code from the subdev
> to a pixel format using the 
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct v4l2_subdev *video_remote_subdev(struct stfcamss_video *video,
>> +					       u32 *pad)
>> +{
>> +	struct media_pad *remote;
>> +
>> +	remote = media_pad_remote_pad_first(&video->pad);
>> +
>> +	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
>> +		return NULL;
>> +
>> +	if (pad)
>> +		*pad = remote->index;
>> +
>> +	return media_entity_to_v4l2_subdev(remote->entity);
> 
> As the connected subdev is always the same (the CSI-2 RX for the raw
> capture video device and the ISP for the processed capture video
> device), I would store a pointer to the connected subdev in the
> stfcamss_video structure at registration time. You can pass the pointer
> to the stf_video_register() function.
> 

As the hardware also supports the dvp interface, I think the current
function implementation should be flexible and easy to expand later.

>> +}
>> +
>> +static int video_get_subdev_format(struct stfcamss_video *video,
>> +				   struct v4l2_format *format)
>> +{
>> +	struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix;
>> +	struct v4l2_subdev_format fmt;
>> +	struct v4l2_subdev *subdev;
>> +	u32 pixelformat;
>> +	u32 pad;
>> +	int ret;
>> +
>> +	subdev = video_remote_subdev(video, &pad);
>> +	if (!subdev)
>> +		return -EPIPE;
>> +
>> +	fmt.pad = pad;
>> +	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>> +
>> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> 
> Use v4l2_subdev_call_state_active() to support the subdev state API.
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	pixelformat = pix->pixelformat;
>> +	ret = video_find_format(fmt.format.code, pixelformat, video);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	format->type = video->type;
>> +
>> +	return video_mbus_to_pix(&fmt.format, &format->fmt.pix,
>> +				 &video->formats[ret], video->bpl_alignment);
>> +}
>> +

Regards,

Jack Zhu