Messages in this thread | | | Date | Mon, 7 Nov 2022 09:23:41 +0100 | Subject | Re: [PATCH v6 5/5] drivers: media: platform: Add NPCM Video Capture/Encode Engine driver | From | Hans Verkuil <> |
| |
On 07/11/2022 08:20, Kun-Fa Lin wrote: > Hi Hans, > > Thanks for the review. > >> >> These functions are not usually present when capturing from video. You don't >> have a choice w.r.t. resolution and fps, since that's determined by the >> incoming video. I would drop support for this. > > Just to confirm, do you mean `npcm_video_enum_framesizes` and > `npcm_video_enum_frameintervals` functions?
Right.
> > >>> + switch (ctrl->id) { >>> + case V4L2_CID_DETECT_MD_MODE: >>> + if (ctrl->val == V4L2_DETECT_MD_MODE_GLOBAL) >>> + video->ctrl_cmd = VCD_CMD_OPERATION_CAPTURE; >>> + else >>> + video->ctrl_cmd = VCD_CMD_OPERATION_COMPARE; >>> + break; >> >> Incorrect indentation for the 'break'. > > Will correct it. > > >>> + v4l2_ctrl_new_std_menu(&video->ctrl_handler, &npcm_video_ctrl_ops, >>> + V4L2_CID_DETECT_MD_MODE, >>> + V4L2_DETECT_MD_MODE_REGION_GRID, 0, >>> + V4L2_DETECT_MD_MODE_GLOBAL); >> >> Why is this driver using a control designed for motion detection devices? >> That seems odd, and it looks like you are abusing this control to do something >> else. > > The Video Capture/Differentiation (VCD) engine supports two modes: > - COMPLETE (capture the next "complete frame" into memory) > - DIFF (compare the incoming frame with the frame stored in memory, > and updates the "diff frame" in memory) > > The purpose here is to provide a way for application to switch the > COMPLETE/DIFF mode. Since I couldn't find an appropriate ioctl that is > designed for this purpose, so I used VIDIOC_S_CTRL with control values > of V4L2_DETECT_MD_MODE_GLOBAL (for COMPLETE) and > V4L2_DETECT_MD_MODE_REGION_GRID (for DIFF). It would be appreciated if > you could point me in the right direction.
This is very much a driver-specific control. So you have to make your own.
This series is a good example on how to add a custom control:
https://lore.kernel.org/linux-media/20221028023554.928-1-jammy_huang@aspeedtech.com/
Driver-specific controls are fine, as long as they are properly documented.
> > >> When you post v7, please also include the output of v4l2-compliance to the >> cover letter! >> Make sure you compile v4l2-compliance from the v4l-utils git repo, do not >> use a version from a distro, that will be too old. > > OK, I'll try to compile v4l2-compliance and include the output. > > Regards, > Marvin
Regards,
Hans
| |