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
