Hi Mauro, On Wed, Nov 03, 2021 at 01:54:18PM +0000, 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 We should *really* phase out USERPTR support. Worst case you may support DMABUF only if MMAP is problematic, but I don't really see why it could be easy to map an imported buffer and difficult to map a buffer allocated by the driver. videobuf2 should be used. > 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 > 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. Why is that ? Being able to capture multiple streams in different resolutions is important for lots of applications, the viewfinder resolution is often different than the video streaming and/or still capture resolution. Scaling after capture is often expensive (and there are memory bandwidth and power constraints to take into account too). A single-stream device may be better than nothing, but it's time to move to the 21st century. -- Regards, Laurent Pinchart
