| 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
| ||
|
Message-ID: <c39cac68-73ab-4ab0-a701-e92f01c92774@xs4all.nl>
Date: Wed, 3 Nov 2021 15:41:05 +0100
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Linux Media Mailing List <linux-media@...r.kernel.org>
Cc: Tsuchiya Yuto <kitakar@...il.com>,
Hans de Goede <hdegoede@...hat.com>,
Patrik Gfeller <patrik.gfeller@...il.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Kaixu Xia <kaixuxia@...cent.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Yang Li <abaci-bugfix@...ux.alibaba.com>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Alex Dewar <alex.dewar90@...il.com>,
Aline Santana Cordeiro <alinesantanacordeiro@...il.com>,
Arnd Bergmann <arnd@...db.de>, Alan <alan@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, linux-staging@...ts.linux.dev,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: atomisp current issues
On 03/11/2021 14:54, Mauro Carvalho Chehab wrote:
> Hi,
>
> From what I've seen so far, those are the main issues with regards to V4L2 API,
> in order to allow a generic V4L2 application to work with it.
>
> MMAP support
> ============
>
> Despite having some MMAP code on it, the current implementation is broken.
> Fixing it is not trivial, as it would require fixing the HMM support on it,
> which does several tricks.
>
> The best would be to replace it by something simpler. If this is similar
> enough to IPU3, perhaps one idea would be to replace the HMM code on it by
> videodev2 + IPU3 HMM code.
>
> As this is not trivial, I'm postponing such task. If someone has enough
> time, it would be great to have this fixed.
>
> From my side, I opted to add support for USERPTR on camorama:
>
> https://github.com/alessio/camorama
>
> As this is something I wanted to do anyway, and it allowed me to cleanup
> several things in camorama's code.
>
> Support for USERPTR is not autodetected. So, this should be selected
You can autodetect this: the capabilities field returned by VIDIOC_REQBUFS
or VIDIOC_CREATE_BUFS will indicate support for this. This works with any
vb2-based driver.
Just thought I should mention this...
Regards,
Hans
> via a command line parameter. Here (Fedora), I installed the build
> dependencies on my test device with:
>
> $ sudo dnf builddep camorama
> $ sudo dnf install gnome-common # for gnome-autogen.sh
>
> Testing it with atomisp would be:
>
> $ git clone https://github.com/alessio/camorama
> $ cd camorama && ./autogen.sh
> $ make && ./src/camorama -d /dev/video2 --debug -U -D
>
> In time:
> --------
>
> Camorama currently doesn't work due to some other API bugs. See below.
>
>
> VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT issues
> ===============================================
>
> The implementation for VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT are not
> properly implemented. It has several issues.
>
> The main one is related to padding. Based on the comments inside the code,
> the ISP 2 seems to require padding to work, both vertically an horizontally.
>
> Those are controlled by two modprobe parameters: pad_h and pad_w.
> The default for both is 16.
>
> There are some other padding logic inside the function which sets the
> video formats at atomisp_cmd: atomisp_set_fmt(). This function is quite
> complex, being big and it calls several other atomisp kAPIs.
>
> It basically queries the sensor, and then it mounts a pipeline that
> will have the sensor + ISP blocks. Those ISP blocks convert the format
> from Bayer into YUYV or RGB and scale down the resolution in order to
> match the request.
>
> It also adjusts the padding. The padding code there is very complex,
> as it not only uses pad_h/pad_w, having things like:
>
> if (!atomisp_subdev_format_conversion(asd, source_pad)) {
> padding_w = 0;
> padding_h = 0;
> } else if (IS_BYT) {
> padding_w = 12;
> padding_h = 12;
> }
> ...
> atomisp_get_dis_envelop(asd, f->fmt.pix.width, f->fmt.pix.height,
> &dvs_env_w, &dvs_env_h);
> ...
> /*
> * set format info to sensor
> * In continuous mode, resolution is set only if it is higher than
> * existing value. This because preview pipe will be configured after
> * capture pipe and usually has lower resolution than capture pipe.
> */
> if (!asd->continuous_mode->val ||
> isp_sink_fmt->width < (f->fmt.pix.width + padding_w + dvs_env_w) ||
> isp_sink_fmt->height < (f->fmt.pix.height + padding_h +
> dvs_env_h)) {
>
> Due to that, the sensors are set to have those extra 16 columns/lines.
> Those extra data are consumed internally at the driver, so the output
> buffer won't contain those.
>
> Yet, the buffer size to be allocated by userspace on USERPTR mode is not just
> width x height x bpp, but it may need extra space for such pads and/or other
> things.
>
> In practice, when VIDIOC_S_FMT asks for a 1600x1200 resolution, it
> actually sets 1616x1216 at the sensor (at the pad source), but the
> pad sink provides the requested resolution: 1600x1200.
>
> However, the resolution returned by VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT
> is not always the sink resolution. Sometimes, it returns the sensor format.
>
> Fixing it has been challenging.
>
> -
>
> Another related issue is that, when a resolution bigger than the maximum
> resolution is requested, the driver doesn't return 1600x1200, but, instead,
> a smaller one.
>
> On other words, setting to YUYV 1600x1200 gives:
>
> $ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1600,height=1200 -d /dev/video2 --verbose
> VIDIOC_QUERYCAP: ok
> VIDIOC_G_FMT: ok
> VIDIOC_S_FMT: ok
> Format Video Capture:
> Width/Height : 1600/1200
> Pixel Format : 'YUYV' (YUYV 4:2:2)
> Field : None
> Bytes per Line : 3200
> Size Image : 3842048
> Colorspace : Rec. 709
> Transfer Function : Rec. 709
> YCbCr/HSV Encoding: Rec. 709
> Quantization : Default (maps to Limited Range)
> Flags :
>
> Setting to a higher resolution (which is a method that some apps use to test
> for the max resolution, when VIDIOC_ENUM_FRAMESIZES is not available or
> broken) produces:
>
> $ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=10000,height=10000 -d /dev/video2 --verbose
> VIDIOC_QUERYCAP: ok
> VIDIOC_G_FMT: ok
> VIDIOC_S_FMT: ok
> Format Video Capture:
> Width/Height : 1600/900
> Pixel Format : 'YUYV' (YUYV 4:2:2)
> Field : None
> Bytes per Line : 3200
> Size Image : 2883584
> Colorspace : Rec. 709
> Transfer Function : Rec. 709
> YCbCr/HSV Encoding: Rec. 709
> Quantization : Default (maps to Limited Range)
> Flags :
>
> Trying to set to the sensor resolution is even worse, as it returns EINVAL:
>
> $ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1616,height=1216 -d /dev/video2 --verbose
> VIDIOC_QUERYCAP: ok
> VIDIOC_G_FMT: ok
> VIDIOC_S_FMT: failed: Invalid argument
>
> The logs for such case are:
>
> [ 4799.594724] atomisp-isp2 0000:00:03.0: can't create streams
> [ 4799.594757] atomisp-isp2 0000:00:03.0: __get_frame_info 1616x1216 (padded to 0) returned -22
> [ 4799.594781] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22
>
> Video devices
> =============
>
> Currently, 10 video? devices are created:
>
> $ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> /dev/video0: Name : ATOMISP ISP CAPTURE output
> /dev/video1: Name : ATOMISP ISP VIEWFINDER output
> /dev/video2: Name : ATOMISP ISP PREVIEW output
> /dev/video3: Name : ATOMISP ISP VIDEO output
> /dev/video4: Name : ATOMISP ISP ACC
> /dev/video5: Name : ATOMISP ISP MEMORY input
> /dev/video6: Name : ATOMISP ISP CAPTURE output
> /dev/video7: Name : ATOMISP ISP VIEWFINDER output
> /dev/video8: Name : ATOMISP ISP PREVIEW output
> /dev/video9: Name : ATOMISP ISP VIDEO output
> /dev/video10: Name : ATOMISP ISP ACC
>
> That seems to be written to satisfy some Android-based app, but we don't
> really need all of those.
>
> I'm thinking to comment out the part of the code which creates all of those,
> keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> of those.
>
> Comments?
>
> Regards,
> Mauro
>
Powered by blists - more mailing lists