On 07/12/2020 15:53, Sakari Ailus wrote: > On Mon, Dec 07, 2020 at 03:42:58PM +0200, Sakari Ailus wrote: >> Hi Hans, >> >> On Mon, Dec 07, 2020 at 02:16:39PM +0100, Hans Verkuil wrote: >>> On 07/12/2020 13:46, Sakari Ailus wrote: >>>> Hi Hans, >>>> >>>> Thanks for the patch. >>>> >>>> On Mon, Dec 07, 2020 at 12:18:55PM +0100, Hans Verkuil wrote: >>>>> This driver does not support composition, only cropping. >>>>> Composition means that the sensor can output e.g. 1920x1080, >>>>> but can compose a cropped 1280x720 image in the middle of the >>>>> 1920x1080 canvas, filling in the unused area with a background >>>>> color. >>>> >>>> That's how this would work on V4L2 video nodes... >>>> >>>>> >>>>> That's not supported at all. So drop the bogus composition support. >>>> >>>> But this is a sub-device driver. On sub-devices the COMPOSE target is used >>>> for configuring scaling, binning and sub-sampling. I don't know about the >>>> capabilities of this particular driver but the code >>>> (__imx274_change_compose function in particular) looks very much such that >>>> it does support binning. >>>> >>> >>> That should be done via set_fmt. There you select the output width and height. >>> >>> So if set_fmt sets 1920x1080, and the crop is 960x540, then you scale (or >>> do binning/sub-sampling). Compose means composing the image into a larger >>> canvas. For this driver the compose rectangle is always equal to the >>> format, so set_selection(COMPOSE) is identical to set_fmt(). >>> >>> If it was real composition, then there would have to be a try_compose as >>> well, just as there is a try_crop. Instead set_selection(COMPOSE) fills in >> >> On sub-devices there's a try context that's file handle specific. I know, but that isn't used here. >> >>> try_fmt. Clearly wrong. >> >> Not more than using set_fmt if you look at the documentation: >> >> <URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/dev-subdev.html#order-of-configuration-and-format-propagation> >> >> In this case it's just on the sink pad, as the sub-device exports no source >> pads. I think there are probably a few such drivers around. Which example are you referring to? 4.5 or 4.6? 4.5 is closest to what a sensor is, except you won't have a sink pad. Here you crop from the sensor image, and to scale it up/down to the source media bus format width and height (if the hardware supports that, of course). There is no composition. If composition was supported, then the crop rectangle would be scaled up/down to the composition rectangle, which would be a rectangle inside the source media bus format. Any pixels outside of that composition rectangle would have to be filled with some background color. A video device would use composition to write to a rectangle inside a memory buffer, a subdev device would use composition in the same way, except it would just generate background pixels for anything outside of the composition rectangle on the source pad. It is very, very rare for sensors or video receivers that support composition. They typically support crop and they may support scaling of some sort, and that's it. This imx274 driver certainly does not support composition. It's plain wrong. And if you look at the code carefully you'll see that the 'composition' just sets the source media bus format. There are (besides imx274) only two sensor drivers that claim to support composition: ccs and s5k5baf. The latter might actually be real since it has an embedded SoC ISP. It is lacking a control to set the background color, though. CCS is a pipeline with scalers, binners, etc., but I am not so sure it actually supports composition: can it compose the image inside a larger image that will go out on a source pad? BTW, how do you test CCS? Do you run v4l2-compliance? I see no support for the CROP/COMPOSE_DEFAULT selection, which suggests to me that v4l2-compliance will likely protest. In order to implement the old VIDIOC_CROPCAP ioctl using the new selection API you need both _BOUNDS and _DEFAULT selection targets. v4l2-compliance checks for that. >> >> Which sub-device drivers configure scaling based on set_fmt? I'm only aware >> of omap3isp which pre-dates the selection API. > > That said, there are a lot of drivers that pick the entire configuration > (cropping (digital and analogue), binning, scaling and sub-samplink) just > based on set_fmt. It's what drivers to as there are no good solutions for > the current API, but for the user it's pretty awful. > No, that's nothing to do with composition. The CCS driver creates a chain of subdevs, one for each step (scaling, binning), and so you can control it precisely. But I am pretty sure that each step will be like the 4.5 example: you receive an image on the sink pad, you can optionally crop that, and the result is scaled up/down to the source media bus format. There is no composition taking place. Or in other words, the composition rectangle always matches the source media bus format. But there is no point exposing that, it would just be confusing to userspace. The only possible use-case that I can see is similar to example 4.6 (except with one source pad, not two): there you scale from the sink crop to the sink compose rectangle, then do a source crop and output that to the source pad. But that only makes sense if you can actually do a source crop in the hardware. If you can't, then it again reduces to just a sink crop and scaling to the source media bus format. In any case, imx274 definitely doesn't do composition, so that should be removed. Regards, Hans
