linux-kernel - Re: [PATCH v6 5/5] drivers: media: platform: Add NPCM Video Capture/Encode Engine driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADnNmFp4r-3+pvHa+_HOxcXAkORadMzgg6fFKbLcgs66a_90gw@mail.gmail.com>
Date:   Mon, 7 Nov 2022 15:20:36 +0800
From:   Kun-Fa Lin <milkfafa@...il.com>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     mchehab@...nel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
        avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
        kwliu@...oton.com, kflin@...oton.com
Subject: Re: [PATCH v6 5/5] drivers: media: platform: Add NPCM Video
 Capture/Encode Engine driver

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?


> > +     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.


> 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