mbox series

[v14,0/7] Support Nuvoton NPCM Video Capture/Encode Engine

Message ID 20230828091859.3889817-1-milkfafa@gmail.com
Headers show
Series Support Nuvoton NPCM Video Capture/Encode Engine | expand

Message

Marvin Lin Aug. 28, 2023, 9:18 a.m. UTC
This patch series add DTS node, dt-bindings document and drivers for Video
Capture/Differentiation Engine (VCD) and Encoding Compression Engine (ECE)
present on Nuvoton NPCM SoCs.

As described in the datasheet NPCM750D_DS_Rev_1.0, the VCD can capture a
frame from digital video input and compare two frames in memory, and then
the ECE can compress the frame data into HEXTILE format which is defined
in Remote Framebuffer Protocol (RFC 6143, chapter 7.7.4. Hextile Encoding).

The output of v4l2-compliance:
v4l2-compliance 1.23.0-4996, 64 bits, 64-bit time_t
v4l2-compliance SHA: 9431e4b26b48 2023-02-13 14:51:47

Compliance test for npcm-video device /dev/video0:

Driver Info:
        Driver name      : npcm-video
        Card type        : NPCM Video Engine
        Bus info         : platform:npcm-video
        Driver version   : 6.1.12
        Capabilities     : 0x84200001
                Video Capture
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x04200001
                Video Capture
                Streaming
                Extended Pix Format

Required ioctls:
        test VIDIOC_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/video0 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK
        test VIDIOC_DV_TIMINGS_CAP: OK
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
                warn: v4l2-test-controls.cpp(1139): V4L2_CID_DV_RX_POWER_PRESENT not found for input 0
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 1 Private Controls: 2

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Total for npcm-video device /dev/video0: 45, Succeeded: 45, Failed: 0, Warnings: 1

Changes in v14:
  - Modify the flow of setting resolution and queue setup
  - Correct the control type (TYPE_MENU) of selecting between two modes.
  - Let ECE could be optional (only supports PIX_FMT_RGB565 if ECE is not
    enabled in DT).

Changes in v13:
  - Modify the flow for capturing next frame
  - Modify the behavior of resolution change interrupt
  - Move GFXI dt-bindings document to
    Documentation/devicetree/bindings/soc/nuvoton/nuvoton,gfxi.yaml

Changes in v12:
  - Modify the flow for detecting resolution change and raise
    V4L2_EVENT_SOURCE_CHANGE event.
  - Add V4L2_PIX_FMT_RGB565 format support.

Changes in v11:
  - Replace "u8/u16/u32" with "unsigned int" for generic local variables.
  - Correct subsystem prefixes, drop redundant words in commit subject, and
    add more information in commit message.

Changes in v10:
  - drivers/media/platform/nuvoton/npcm-video.c
    * Let short functions to be inline function.
    * Correct return type of some functions, and properly handle return
      value by callers.
    * Correct the timing of removing rect_list and the flow of FIFO overrun
      case in irq.
    * Adjust line breaks, indentations, and style of variable declarations.

Changes in v9:
  - Change ECE node name to "video-codec".
  - Drop redundant "bindings for" in commit subject of patch 2/7.
  - Refine the format of VCD/ECE dt-binding document.

Changes in v8:
  - Let VCD/ECE to be 2 separate nodes and update dt-binding documents.
  - Move register definitions out to a local header file.
  - Driver refinements (add error handling for memory allocation, remove
    unnecessary condition check and introduce "goto"s to handle similar
    error recovery paths).
  - Correct properties and typo in GFXI dt-binding document.

Changes in v7:
  - Add uapi documents for driver-specific controls.
  - Implement driver-specific controls for switching capture mode and
    getting the count of compressed HEXTILE rectangles.
  - Drop unnecessary "enum_framesizes" and "enum_frameintervals" functions.
  - Include the output of v4l2-compliance in cover letter.

Changes in v6:
  - Support NPCM845 and add compatible "nuvoton,npcm845-video".
  - Correct pixel format to V4L2_PIX_FMT_HEXTILE which is newly added in
    this patch series.

Changes in v5:
  - Simplify function prefix "nuvoton_" to "npcm_".
  - Increase VCD_BUSY_TIMEOUT_US and ECE_POLL_TIMEOUT_US to 300ms to
    prevent polling timeout when ECC is enabled or system is busy.

Changes in v4:
  - Fix compile warning reported by kernel test robot.

Changes in v3:
  - Add video driver entry in MAINTAINERS.
  - Change config name to CONFIG_VIDEO_NPCM_VCD_ECE.
  - Reduce the waiting time after resetting the VCD/ECE module.
  - Correct data types of some variables.

Changes in v2:
  - Add Hextile document and locate with vendor formats.

Marvin Lin (7):
  ARM: dts: nuvoton: Add node for NPCM VCD and ECE engine
  media: dt-bindings: nuvoton: Add NPCM VCD and ECE engine
  dt-bindings: soc: nuvoton: Add NPCM GFXI
  media: v4l: Add HEXTILE compressed format
  media: v4l2-ctrls: Add user control base for Nuvoton NPCM controls
  media: uapi: Add controls for NPCM video driver
  media: nuvoton: Add driver for NPCM video capture and encode engine

 .../bindings/media/nuvoton,npcm-ece.yaml      |   43 +
 .../bindings/media/nuvoton,npcm-vcd.yaml      |   72 +
 .../bindings/soc/nuvoton/nuvoton,gfxi.yaml    |   39 +
 .../userspace-api/media/drivers/index.rst     |    1 +
 .../media/drivers/npcm-video.rst              |   67 +
 .../media/v4l/pixfmt-reserved.rst             |    7 +
 MAINTAINERS                                   |   12 +
 .../dts/nuvoton/nuvoton-common-npcm7xx.dtsi   |   23 +
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/nuvoton/Kconfig        |   15 +
 drivers/media/platform/nuvoton/Makefile       |    2 +
 drivers/media/platform/nuvoton/npcm-regs.h    |  152 ++
 drivers/media/platform/nuvoton/npcm-video.c   | 1840 +++++++++++++++++
 drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
 include/uapi/linux/npcm-video.h               |   41 +
 include/uapi/linux/v4l2-controls.h            |    6 +
 include/uapi/linux/videodev2.h                |    1 +
 18 files changed, 2324 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/nuvoton,npcm-ece.yaml
 create mode 100644 Documentation/devicetree/bindings/media/nuvoton,npcm-vcd.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,gfxi.yaml
 create mode 100644 Documentation/userspace-api/media/drivers/npcm-video.rst
 create mode 100644 drivers/media/platform/nuvoton/Kconfig
 create mode 100644 drivers/media/platform/nuvoton/Makefile
 create mode 100644 drivers/media/platform/nuvoton/npcm-regs.h
 create mode 100644 drivers/media/platform/nuvoton/npcm-video.c
 create mode 100644 include/uapi/linux/npcm-video.h

Comments

Hans Verkuil Aug. 30, 2023, 9:42 a.m. UTC | #1
Hi Marvin,

A few minor change requests, and a question w.r.t. the V4L2_CID_NPCM_RECT_COUNT
control:

On 28/08/2023 11:18, Marvin Lin wrote:
> Add driver for Video Capture/Differentiation Engine (VCD) and Encoding
> Compression Engine (ECE) present on Nuvoton NPCM SoCs. As described in
> the datasheet NPCM750D_DS_Rev_1.0, the VCD can capture a frame from
> digital video input and compare two frames in memory, and then the ECE
> can compress the frame data into HEXTILE format. This driver implements
> V4L2 interfaces and provides user controls to support KVM feature, also
> tested with VNC Viewer ver.6.22.826 and openbmc/obmc-ikvm.
> 
> Signed-off-by: Marvin Lin <milkfafa@gmail.com>
> ---
>  MAINTAINERS                                 |   12 +
>  drivers/media/platform/Kconfig              |    1 +
>  drivers/media/platform/Makefile             |    1 +
>  drivers/media/platform/nuvoton/Kconfig      |   15 +
>  drivers/media/platform/nuvoton/Makefile     |    2 +
>  drivers/media/platform/nuvoton/npcm-regs.h  |  152 ++
>  drivers/media/platform/nuvoton/npcm-video.c | 1840 +++++++++++++++++++
>  7 files changed, 2023 insertions(+)
>  create mode 100644 drivers/media/platform/nuvoton/Kconfig
>  create mode 100644 drivers/media/platform/nuvoton/Makefile
>  create mode 100644 drivers/media/platform/nuvoton/npcm-regs.h
>  create mode 100644 drivers/media/platform/nuvoton/npcm-video.c
> 

<snip>

> +static int npcm_video_querycap(struct file *file, void *fh,
> +			       struct v4l2_capability *cap)
> +{
> +	strscpy(cap->driver, DEVICE_NAME, sizeof(cap->driver));
> +	strscpy(cap->card, "NPCM Video Engine", sizeof(cap->card));
> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", DEVICE_NAME);

You can drop this last line, it is already filled in by the V4L2 core for
platform devices.

> +
> +	return 0;
> +}
> +
> +static int npcm_video_enum_format(struct file *file, void *fh,
> +				  struct v4l2_fmtdesc *f)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +	const struct npcm_fmt *fmt;
> +
> +	if (f->index >= NUM_FORMATS)
> +		return -EINVAL;
> +
> +	fmt = &npcm_fmt_list[f->index];
> +	if (fmt->fourcc == V4L2_PIX_FMT_HEXTILE && !video->ece.enable)
> +		return -EINVAL;
> +
> +	f->pixelformat = fmt->fourcc;
> +	return 0;
> +}
> +
> +static int npcm_video_try_format(struct file *file, void *fh,
> +				 struct v4l2_format *f)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +	const struct npcm_fmt *fmt;
> +
> +	fmt = npcm_video_find_format(f);
> +
> +	/* If format not found or HEXTILE not supported, use RGB565 as default */
> +	if (!fmt || (fmt->fourcc == V4L2_PIX_FMT_HEXTILE && !video->ece.enable))
> +		f->fmt.pix.pixelformat = npcm_fmt_list[0].fourcc;
> +
> +	f->fmt.pix.field = V4L2_FIELD_NONE;
> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> +	f->fmt.pix.quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	f->fmt.pix.width = video->pix_fmt.width;
> +	f->fmt.pix.height = video->pix_fmt.height;
> +	f->fmt.pix.bytesperline = video->bytesperline;
> +	f->fmt.pix.sizeimage = video->pix_fmt.sizeimage;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_get_format(struct file *file, void *fh,
> +				 struct v4l2_format *f)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +
> +	f->fmt.pix = video->pix_fmt;
> +	return 0;
> +}
> +
> +static int npcm_video_set_format(struct file *file, void *fh,
> +				 struct v4l2_format *f)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +	int ret;
> +
> +	ret = npcm_video_try_format(file, fh, f);
> +	if (ret)
> +		return ret;
> +
> +	if (vb2_is_busy(&video->queue)) {
> +		dev_err(video->dev, "%s device busy\n", __func__);
> +		return -EBUSY;
> +	}
> +
> +	video->pix_fmt.pixelformat = f->fmt.pix.pixelformat;
> +	return 0;
> +}
> +
> +static int npcm_video_enum_input(struct file *file, void *fh,
> +				 struct v4l2_input *inp)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +
> +	if (inp->index)
> +		return -EINVAL;
> +
> +	strscpy(inp->name, "Host VGA capture", sizeof(inp->name));
> +	inp->type = V4L2_INPUT_TYPE_CAMERA;
> +	inp->capabilities = V4L2_IN_CAP_DV_TIMINGS;
> +	inp->status = video->v4l2_input_status;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_get_input(struct file *file, void *fh, unsigned int *i)
> +{
> +	*i = 0;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_set_input(struct file *file, void *fh, unsigned int i)
> +{
> +	if (i)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_set_dv_timings(struct file *file, void *fh,
> +				     struct v4l2_dv_timings *timings)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +	int rc;
> +
> +	if (timings->bt.width == video->active_timings.width &&
> +	    timings->bt.height == video->active_timings.height)
> +		return 0;
> +
> +	if (vb2_is_busy(&video->queue)) {
> +		dev_err(video->dev, "%s device busy\n", __func__);
> +		return -EBUSY;
> +	}
> +
> +	rc = npcm_video_set_resolution(video, &timings->bt);
> +	if (rc)
> +		return rc;
> +
> +	timings->type = V4L2_DV_BT_656_1120;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_get_dv_timings(struct file *file, void *fh,
> +				     struct v4l2_dv_timings *timings)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +
> +	timings->type = V4L2_DV_BT_656_1120;
> +	timings->bt = video->active_timings;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_query_dv_timings(struct file *file, void *fh,
> +				       struct v4l2_dv_timings *timings)
> +{
> +	struct npcm_video *video = video_drvdata(file);
> +
> +	npcm_video_detect_resolution(video);
> +	timings->type = V4L2_DV_BT_656_1120;
> +	timings->bt = video->detected_timings;
> +
> +	return video->v4l2_input_status ? -ENOLINK : 0;
> +}
> +
> +static int npcm_video_enum_dv_timings(struct file *file, void *fh,
> +				      struct v4l2_enum_dv_timings *timings)
> +{
> +	return v4l2_enum_dv_timings_cap(timings, &npcm_video_timings_cap,
> +					NULL, NULL);
> +}
> +
> +static int npcm_video_dv_timings_cap(struct file *file, void *fh,
> +				     struct v4l2_dv_timings_cap *cap)
> +{
> +	*cap = npcm_video_timings_cap;
> +
> +	return 0;
> +}
> +
> +static int npcm_video_sub_event(struct v4l2_fh *fh,
> +				const struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_SOURCE_CHANGE:
> +		return v4l2_src_change_event_subscribe(fh, sub);
> +	}
> +
> +	return v4l2_ctrl_subscribe_event(fh, sub);
> +}
> +
> +static const struct v4l2_ioctl_ops npcm_video_ioctls = {
> +	.vidioc_querycap = npcm_video_querycap,
> +
> +	.vidioc_enum_fmt_vid_cap = npcm_video_enum_format,
> +	.vidioc_g_fmt_vid_cap = npcm_video_get_format,
> +	.vidioc_s_fmt_vid_cap = npcm_video_set_format,
> +	.vidioc_try_fmt_vid_cap = npcm_video_try_format,
> +
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_expbuf = vb2_ioctl_expbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
> +
> +	.vidioc_enum_input = npcm_video_enum_input,
> +	.vidioc_g_input = npcm_video_get_input,
> +	.vidioc_s_input = npcm_video_set_input,
> +
> +	.vidioc_s_dv_timings = npcm_video_set_dv_timings,
> +	.vidioc_g_dv_timings = npcm_video_get_dv_timings,
> +	.vidioc_query_dv_timings = npcm_video_query_dv_timings,
> +	.vidioc_enum_dv_timings = npcm_video_enum_dv_timings,
> +	.vidioc_dv_timings_cap = npcm_video_dv_timings_cap,
> +
> +	.vidioc_subscribe_event = npcm_video_sub_event,
> +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +};
> +
> +static int npcm_video_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct npcm_video *video = container_of(ctrl->handler, struct npcm_video,
> +						ctrl_handler);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_NPCM_CAPTURE_MODE:
> +		if (ctrl->val == V4L2_NPCM_CAPTURE_MODE_COMPLETE)
> +			video->ctrl_cmd = VCD_CMD_OPERATION_CAPTURE;
> +		else if (ctrl->val == V4L2_NPCM_CAPTURE_MODE_DIFF)
> +			video->ctrl_cmd = VCD_CMD_OPERATION_COMPARE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int npcm_video_get_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct npcm_video *video = container_of(ctrl->handler, struct npcm_video,
> +						ctrl_handler);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_NPCM_RECT_COUNT:
> +		ctrl->val = video->rect[video->vb_index];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops npcm_video_ctrl_ops = {
> +	.s_ctrl = npcm_video_set_ctrl,
> +	.g_volatile_ctrl = npcm_video_get_volatile_ctrl,
> +};
> +
> +static const char * const npcm_ctrl_capture_mode_menu[] = {
> +	"COMPLETE mode",
> +	"DIFF mode",

Hmm, I would drop the 'mode' bit, since it is already obvious that
these are the modes.

> +	NULL,
> +};
> +
> +static const struct v4l2_ctrl_config npcm_ctrl_capture_mode = {
> +	.ops = &npcm_video_ctrl_ops,
> +	.id = V4L2_CID_NPCM_CAPTURE_MODE,
> +	.name = "NPCM Video Capture Mode",
> +	.type = V4L2_CTRL_TYPE_MENU,
> +	.min = 0,
> +	.max = V4L2_NPCM_CAPTURE_MODE_DIFF,
> +	.def = 0,
> +	.qmenu = npcm_ctrl_capture_mode_menu,
> +};
> +
> +static const struct v4l2_ctrl_config npcm_ctrl_rect_count = {
> +	.ops = &npcm_video_ctrl_ops,
> +	.id = V4L2_CID_NPCM_RECT_COUNT,
> +	.name = "NPCM Compressed Hextile Rectangle Count",
> +	.type = V4L2_CTRL_TYPE_INTEGER,
> +	.flags = V4L2_CTRL_FLAG_VOLATILE,
> +	.min = 0,
> +	.max = (MAX_WIDTH / RECT_W) * (MAX_HEIGHT / RECT_H),
> +	.step = 1,
> +	.def = 0,
> +};

Just to confirm: you decided against using an integer array control?

There is a real danger that if userspace isn't reading this control
quickly enough (i.e. before the next frame arrives at the driver), then
the control's value is that of that next frame instead of the current
frame.

It doesn't feel robust to me.

<snip>

Regards,

	Hans
Marvin Lin Aug. 31, 2023, 12:42 p.m. UTC | #2
Hi Hans,

Thanks for the review.

> > +     snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", DEVICE_NAME);
>
> You can drop this last line, it is already filled in by the V4L2 core for
> platform devices.

> > +static const char * const npcm_ctrl_capture_mode_menu[] = {
> > +     "COMPLETE mode",
> > +     "DIFF mode",
>
> Hmm, I would drop the 'mode' bit, since it is already obvious that
> these are the modes.

OK. Will drop them in the next version.

> > +static const struct v4l2_ctrl_config npcm_ctrl_rect_count = {
> > +     .ops = &npcm_video_ctrl_ops,
> > +     .id = V4L2_CID_NPCM_RECT_COUNT,
> > +     .name = "NPCM Compressed Hextile Rectangle Count",
> > +     .type = V4L2_CTRL_TYPE_INTEGER,
> > +     .flags = V4L2_CTRL_FLAG_VOLATILE,
> > +     .min = 0,
> > +     .max = (MAX_WIDTH / RECT_W) * (MAX_HEIGHT / RECT_H),
> > +     .step = 1,
> > +     .def = 0,
> > +};
>
> Just to confirm: you decided against using an integer array control?
>
> There is a real danger that if userspace isn't reading this control
> quickly enough (i.e. before the next frame arrives at the driver), then
> the control's value is that of that next frame instead of the current
> frame.
>
> It doesn't feel robust to me.

Actually the driver will store the frames and counts for each buffer
index till userspace dequeues them.

Ex. assume that driver has captured 3 frames:
- 1st capture (buffer index = 0):
     video->list[0] => store the list of HEXTILE rects for the 1st frame
     video->rect[0] => store the rect count of video->list[0]
- 2nd capture (buffer index = 1):
     video->list[1] => store the list of HEXTILE rects for the 2nd frame
     video->rect[1] => store the rect count of video->list[1]
- 3rd capture (buffer index = 2):
     video->list[2] => store the list of HEXTILE rects for the 3rd frame
     video->rect[2] => store the rect count of video->list[2]

When userspace dequeues the 1st buffer (video->list[0]), it needs to
know the count of HEXTILE rectangles in the buffer,
so after dequeuing the buffer it will call this control to get the
rect count (video->rect[0]). And when a buffer is dequeued,
npcm_video_buf_finish() will be called, in which the buffer index (in
this example, buffer index = 0) will be stored to video->vb_index.
Then when userspace calls this control, npcm_video_get_volatile_ctrl()
will return the rect count of vb_index = 0.
In this way, I think userspace is always reading the correct control's
value even if userspace is slow.
Does it make sense to you or is there anything I missed?

Regards,
Marvin
Hans Verkuil Aug. 31, 2023, 2:06 p.m. UTC | #3
On 31/08/2023 14:42, Kun-Fa Lin wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
>>> +     snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", DEVICE_NAME);
>>
>> You can drop this last line, it is already filled in by the V4L2 core for
>> platform devices.
> 
>>> +static const char * const npcm_ctrl_capture_mode_menu[] = {
>>> +     "COMPLETE mode",
>>> +     "DIFF mode",
>>
>> Hmm, I would drop the 'mode' bit, since it is already obvious that
>> these are the modes.
> 
> OK. Will drop them in the next version.
> 
>>> +static const struct v4l2_ctrl_config npcm_ctrl_rect_count = {
>>> +     .ops = &npcm_video_ctrl_ops,
>>> +     .id = V4L2_CID_NPCM_RECT_COUNT,
>>> +     .name = "NPCM Compressed Hextile Rectangle Count",
>>> +     .type = V4L2_CTRL_TYPE_INTEGER,
>>> +     .flags = V4L2_CTRL_FLAG_VOLATILE,
>>> +     .min = 0,
>>> +     .max = (MAX_WIDTH / RECT_W) * (MAX_HEIGHT / RECT_H),
>>> +     .step = 1,
>>> +     .def = 0,
>>> +};
>>
>> Just to confirm: you decided against using an integer array control?
>>
>> There is a real danger that if userspace isn't reading this control
>> quickly enough (i.e. before the next frame arrives at the driver), then
>> the control's value is that of that next frame instead of the current
>> frame.
>>
>> It doesn't feel robust to me.
> 
> Actually the driver will store the frames and counts for each buffer
> index till userspace dequeues them.
> 
> Ex. assume that driver has captured 3 frames:
> - 1st capture (buffer index = 0):
>      video->list[0] => store the list of HEXTILE rects for the 1st frame
>      video->rect[0] => store the rect count of video->list[0]
> - 2nd capture (buffer index = 1):
>      video->list[1] => store the list of HEXTILE rects for the 2nd frame
>      video->rect[1] => store the rect count of video->list[1]
> - 3rd capture (buffer index = 2):
>      video->list[2] => store the list of HEXTILE rects for the 3rd frame
>      video->rect[2] => store the rect count of video->list[2]
> 
> When userspace dequeues the 1st buffer (video->list[0]), it needs to
> know the count of HEXTILE rectangles in the buffer,
> so after dequeuing the buffer it will call this control to get the
> rect count (video->rect[0]). And when a buffer is dequeued,
> npcm_video_buf_finish() will be called, in which the buffer index (in
> this example, buffer index = 0) will be stored to video->vb_index.
> Then when userspace calls this control, npcm_video_get_volatile_ctrl()
> will return the rect count of vb_index = 0.
> In this way, I think userspace is always reading the correct control's
> value even if userspace is slow.
> Does it make sense to you or is there anything I missed?

Ah, I don't think I have ever seen anyone use buf_finish in that way!

Very inventive, and perfectly legal. Actually a very nice idea!

So, with that in mind there are still some things that need to change.

First of all, you can drop the 'VOLATILE' flag from the control, instead
just call v4l2_ctrl_s_ctrl() from buf_finish() to update the control.
And in stop_streaming the control value should probably be set to 0.

The use of volatile for a control is a last resort, and in this case it
is not volatile at all.

Secondly, this behavior has to be documented: in buf_finish add a comment
along the lines of: "This callback is called when the buffer is dequeued,
so update this control with the number of rectangles."

And where the control is defined, refer to buf_finish to explain where it
is set.

Finally the user-facing documentation has to be updated (npcm-video.rst)
to explain this behavior.

Regards,

	Hans
Marvin Lin Sept. 4, 2023, 4:49 a.m. UTC | #4
Hi Hans,

Thanks for the reply.

> > When userspace dequeues the 1st buffer (video->list[0]), it needs to
> > know the count of HEXTILE rectangles in the buffer,
> > so after dequeuing the buffer it will call this control to get the
> > rect count (video->rect[0]). And when a buffer is dequeued,
> > npcm_video_buf_finish() will be called, in which the buffer index (in
> > this example, buffer index = 0) will be stored to video->vb_index.
> > Then when userspace calls this control, npcm_video_get_volatile_ctrl()
> > will return the rect count of vb_index = 0.
> > In this way, I think userspace is always reading the correct control's
> > value even if userspace is slow.
> > Does it make sense to you or is there anything I missed?
>
> Ah, I don't think I have ever seen anyone use buf_finish in that way!
>
> Very inventive, and perfectly legal. Actually a very nice idea!
>
> So, with that in mind there are still some things that need to change.
>
> First of all, you can drop the 'VOLATILE' flag from the control, instead
> just call v4l2_ctrl_s_ctrl() from buf_finish() to update the control.
> And in stop_streaming the control value should probably be set to 0.
>
> The use of volatile for a control is a last resort, and in this case it
> is not volatile at all.
>
> Secondly, this behavior has to be documented: in buf_finish add a comment
> along the lines of: "This callback is called when the buffer is dequeued,
> so update this control with the number of rectangles."
>
> And where the control is defined, refer to buf_finish to explain where it
> is set.
>
> Finally the user-facing documentation has to be updated (npcm-video.rst)
> to explain this behavior.

OK. Will drop the VOLATILE flag and update comment/document in the next version.

Regards,
Marvin