| Message ID | 7940b65c-b5d8-0167-3262-4282f10e873c@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | imx274: remove composition support, add V4L2_SEL_TGT_CROP_DEFAULT | expand |
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.
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. > 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 sub-device drivers configure scaling based on set_fmt? I'm only aware of omap3isp which pre-dates the selection API.
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. > > > 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 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.
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
On Tue, Dec 08, 2020 at 10:47:29AM +0100, Hans Verkuil wrote: > 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. What exactly are you referring to? The imx274 driver does at least appear to do just that. > > >> > >>> 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 I'm not claiming there is composition, but the COMPOSE targets are also used when the device supports scaling, not composition. This has been so from the very beginning, and cannot (and should not) be changed at this point. > 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. The selection targets are documented in the spec but none has been mandatory. Some dependencies would be reasonable, for instance to require CROP_BOUNDS when CROP is supported. But these are not in the spec now, and v4l2-compliance shouldn't require them unless this is documented so in the spec. > > >> > >> 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. Again, that's not how it works. I also want to stress this is how it has always worked: the sub-device selection support was merged with the CCS driver that was the first user. There's simply no way to re-purpose interfaces now, after a decade of use in the mainline kernel. > 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. -- Kind regards, Sakari Ailus
On 08/12/2020 12:46, Sakari Ailus wrote: > On Tue, Dec 08, 2020 at 10:47:29AM +0100, Hans Verkuil wrote: >> 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. > > What exactly are you referring to? The imx274 driver does at least appear > to do just that. No, it doesn't. Just grep for try_compose, you won't find it. Only try_fmt and try_crop. > >> >>>> >>>>> 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 > > I'm not claiming there is composition, but the COMPOSE targets are also > used when the device supports scaling, not composition. This has been so > from the very beginning, and cannot (and should not) be changed at this > point. No, that's not true. If that were true, then lots of sensor drivers would all support TGT_COMPOSE, but they don't. Without true composition then the scaling is just between the crop rectangle (if any) and the mediabus format. I.e. try_crop and try_fmt. Otherwise it would be very confusing since that would allow userspace to set both the format size and the compose rectangle to different sizes, and which will then be picked? Without true composition one would have to override the other, which makes no sense. > >> 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. > > The selection targets are documented in the spec but none has been > mandatory. Some dependencies would be reasonable, for instance to require > CROP_BOUNDS when CROP is supported. But these are not in the spec now, and > v4l2-compliance shouldn't require them unless this is documented so in the > spec. CROPCAP returns the bound and default rectangles. It can only do that if the selection API can return those rectangles. CROPCAP is required if cropping is supported, see: https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/vidioc-cropcap.html "This ioctl must be implemented for video capture or output devices that support cropping and/or scaling and/or have non-square pixels, and for overlay devices." So this IS in the spec. It is trivial to implement since in almost all cases DEFAULT and BOUNDS are identical. Drivers do not implement cropcap directly anymore, but the cropcap implementation in v4l2-ioctl.c just requests the BOUNDS and DEFAULT selection targets. Video drivers typically just pass that request on to the subdev driver. It is how I found that issue: v4l2-compliance fails on that with the imx274 since it doesn't implement the CROP_DEFAULT target. > >> >>>> >>>> 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. > > Again, that's not how it works. > > I also want to stress this is how it has always worked: the sub-device > selection support was merged with the CCS driver that was the first user. > There's simply no way to re-purpose interfaces now, after a decade of use > in the mainline kernel. So if I set the composition rectangle to 1280x720 and with (top, left) at (128,128), and I set the media bus format to 1920x1080, and then try to validate the pipeline, what will happen? Anyway, I'm talking about the imx274 driver, not the ccs driver. I believe the ccs driver is wrong, but it has been wrong since the beginning, so there is a reason to keep it. In the imx274 this composition support was added in commit 39dd23dc9d4c in July 2018, so if you prefer to keep it to prevent breakage, then that is a valid reason. In that case I'll rework the patch to just add support for CROP_DEFAULT. But it is hopelessly wrong: both SUBDEV_S_FMT and S_SELECTION(COMPOSE) set try_fmt for V4L2_SUBDEV_FORMAT_TRY. I.e., they do the same thing. Basically this is a degenerate case where the composition rectangle must always remain the same as the format width/height, and changing one will change the other. This makes S_SELECTION(COMPOSE) pointless. Worse, it suggests to userspace that the driver supports true composition, when it really doesn't. Regards, Hans > >> 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. >
Hi Hans, On Tue, Dec 08, 2020 at 01:44:59PM +0100, Hans Verkuil wrote: > On 08/12/2020 12:46, Sakari Ailus wrote: > > On Tue, Dec 08, 2020 at 10:47:29AM +0100, Hans Verkuil wrote: > >> 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. > > > > What exactly are you referring to? The imx274 driver does at least appear > > to do just that. > > No, it doesn't. Just grep for try_compose, you won't find it. Only try_fmt and > try_crop. > > >>>>> 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 > > > > I'm not claiming there is composition, but the COMPOSE targets are also > > used when the device supports scaling, not composition. This has been so > > from the very beginning, and cannot (and should not) be changed at this > > point. > > No, that's not true. > > If that were true, then lots of sensor drivers would all support TGT_COMPOSE, but > they don't. Lots of sensor drivers simply don't support any scaling, either because the hardware doesn't, or because the driver doesn't implement it. > Without true composition then the scaling is just between the crop rectangle (if any) > and the mediabus format. I.e. try_crop and try_fmt. > > Otherwise it would be very confusing since that would allow userspace to set both > the format size and the compose rectangle to different sizes, and which will then > be picked? Without true composition one would have to override the other, which > makes no sense. > > >> 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. > > > > The selection targets are documented in the spec but none has been > > mandatory. Some dependencies would be reasonable, for instance to require > > CROP_BOUNDS when CROP is supported. But these are not in the spec now, and > > v4l2-compliance shouldn't require them unless this is documented so in the > > spec. > > CROPCAP returns the bound and default rectangles. It can only do that if the > selection API can return those rectangles. CROPCAP is required if cropping is > supported, see: > > https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/vidioc-cropcap.html > > "This ioctl must be implemented for video capture or output devices that support > cropping and/or scaling and/or have non-square pixels, and for overlay devices." > > So this IS in the spec. It is trivial to implement since in almost all cases > DEFAULT and BOUNDS are identical. Drivers do not implement cropcap directly > anymore, but the cropcap implementation in v4l2-ioctl.c just requests the > BOUNDS and DEFAULT selection targets. Video drivers typically just pass that > request on to the subdev driver. > > It is how I found that issue: v4l2-compliance fails on that with the imx274 > since it doesn't implement the CROP_DEFAULT target. > > >>>> 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. > > > > Again, that's not how it works. > > > > I also want to stress this is how it has always worked: the sub-device > > selection support was merged with the CCS driver that was the first user. > > There's simply no way to re-purpose interfaces now, after a decade of use > > in the mainline kernel. > > So if I set the composition rectangle to 1280x720 and with (top, left) at > (128,128), and I set the media bus format to 1920x1080, and then try to > validate the pipeline, what will happen? The subdev driver will modify the format on the source pad to 1280x720 in the .set_fmt() handler. This is how subdevs operate, they propagate changes internally (but not across links), and update requested selection rectangles and formats based on the configuration of the previous elements of the subdev. > Anyway, I'm talking about the imx274 driver, not the ccs driver. I believe > the ccs driver is wrong, but it has been wrong since the beginning, so there > is a reason to keep it. > > In the imx274 this composition support was added in commit 39dd23dc9d4c in July > 2018, so if you prefer to keep it to prevent breakage, then that is a valid > reason. In that case I'll rework the patch to just add support for CROP_DEFAULT. > But it is hopelessly wrong: both SUBDEV_S_FMT and S_SELECTION(COMPOSE) set > try_fmt for V4L2_SUBDEV_FORMAT_TRY. I.e., they do the same thing. > > Basically this is a degenerate case where the composition rectangle must always > remain the same as the format width/height, and changing one will change the other. > This makes S_SELECTION(COMPOSE) pointless. Worse, it suggests to userspace that > the driver supports true composition, when it really doesn't. Changing the composition rectangle will change the format, but changing the format won't modify the composition rectangle, the change to the format will instead be adjusted based on the composition rectangle. We have, in mainline, different drivers that use different mechanism to configure scaling. Some use the compose rectangle, and some use the output format. If we want to repurpose the compose selection rectangle, it has to be done in the context of an overhaul of the scaling API, taking the whole issue into account (considering all of binning, skipping and scaling), and figuring out how to handle existing userspace without any breakage. Note that this doesn't only apply to sensors, but also to scalers in other components (such as ISPs), so these must be taken into account as well. > >> 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, Laurent Pinchart
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c index 54642d5f2d5b..abb2f8d4895f 100644 --- a/drivers/media/i2c/imx274.c +++ b/drivers/media/i2c/imx274.c @@ -980,11 +980,11 @@ static int imx274_binning_goodness(struct stimx274 *imx274, } /** - * Helper function to change binning and set both compose and format. + * Helper function to change binning and set both crop and format. * * We have two entry points to change binning: set_fmt and - * set_selection(COMPOSE). Both have to compute the new output size - * and set it in both the compose rect and the frame format size. We + * set_selection(CROP). Both have to compute the new output size + * and set it in both the crop rect and the frame format size. We * also need to do the same things after setting cropping to restore * 1:1 binning. * @@ -1003,12 +1003,12 @@ static int imx274_binning_goodness(struct stimx274 *imx274, * @flags: Selection flags from struct v4l2_subdev_selection, or 0 if not * available (when called from set_fmt) */ -static int __imx274_change_compose(struct stimx274 *imx274, - struct v4l2_subdev_pad_config *cfg, - u32 which, - u32 *width, - u32 *height, - u32 flags) +static int __imx274_change_crop_fmt(struct stimx274 *imx274, + struct v4l2_subdev_pad_config *cfg, + u32 which, + u32 *width, + u32 *height, + u32 flags) { struct device *dev = &imx274->client->dev; const struct v4l2_rect *cur_crop; @@ -1099,14 +1099,14 @@ static int imx274_set_fmt(struct v4l2_subdev *sd, mutex_lock(&imx274->lock); - err = __imx274_change_compose(imx274, cfg, format->which, - &fmt->width, &fmt->height, 0); + err = __imx274_change_crop_fmt(imx274, cfg, format->which, + &fmt->width, &fmt->height, 0); if (err) goto out; /* - * __imx274_change_compose already set width and height in the + * __imx274_change_crop_fmt already set width and height in the * applicable format, but we need to keep all other format * values, so do a full copy here */ @@ -1127,14 +1127,12 @@ static int imx274_get_selection(struct v4l2_subdev *sd, struct v4l2_subdev_selection *sel) { struct stimx274 *imx274 = to_imx274(sd); - const struct v4l2_rect *src_crop; - const struct v4l2_mbus_framefmt *src_fmt; - int ret = 0; if (sel->pad != 0) return -EINVAL; - if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) { + if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS || + sel->target == V4L2_SEL_TGT_CROP_DEFAULT) { sel->r.left = 0; sel->r.top = 0; sel->r.width = IMX274_MAX_WIDTH; @@ -1142,57 +1140,42 @@ static int imx274_get_selection(struct v4l2_subdev *sd, return 0; } - if (sel->which == V4L2_SUBDEV_FORMAT_TRY) { - src_crop = &cfg->try_crop; - src_fmt = &cfg->try_fmt; - } else { - src_crop = &imx274->crop; - src_fmt = &imx274->format; - } + if (sel->target != V4L2_SEL_TGT_CROP) + return -EINVAL; mutex_lock(&imx274->lock); - - switch (sel->target) { - case V4L2_SEL_TGT_CROP: - sel->r = *src_crop; - break; - case V4L2_SEL_TGT_COMPOSE_BOUNDS: - sel->r.top = 0; - sel->r.left = 0; - sel->r.width = src_crop->width; - sel->r.height = src_crop->height; - break; - case V4L2_SEL_TGT_COMPOSE: - sel->r.top = 0; - sel->r.left = 0; - sel->r.width = src_fmt->width; - sel->r.height = src_fmt->height; - break; - default: - ret = -EINVAL; - } + if (sel->which == V4L2_SUBDEV_FORMAT_TRY) + sel->r = cfg->try_crop; + else + sel->r = imx274->crop; mutex_unlock(&imx274->lock); - return ret; + return 0; } -static int imx274_set_selection_crop(struct stimx274 *imx274, - struct v4l2_subdev_pad_config *cfg, - struct v4l2_subdev_selection *sel) +static int imx274_set_selection(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_selection *sel) { + struct stimx274 *imx274 = to_imx274(sd); struct v4l2_rect *tgt_crop; struct v4l2_rect new_crop; bool size_changed; - /* * h_step could be 12 or 24 depending on the binning. But we * won't know the binning until we choose the mode later in - * __imx274_change_compose(). Thus let's be safe and use the + * __imx274_change_crop_fmt(). Thus let's be safe and use the * most conservative value in all cases. */ const u32 h_step = 24; + if (sel->pad != 0) + return -EINVAL; + + if (sel->target != V4L2_SEL_TGT_CROP) + return -EINVAL; + new_crop.width = min_t(u32, IMX274_ROUND(sel->r.width, h_step, sel->flags), IMX274_MAX_WIDTH); @@ -1224,56 +1207,20 @@ static int imx274_set_selection_crop(struct stimx274 *imx274, size_changed = (new_crop.width != tgt_crop->width || new_crop.height != tgt_crop->height); - /* __imx274_change_compose needs the new size in *tgt_crop */ + /* __imx274_change_crop_fmt needs the new size in *tgt_crop */ *tgt_crop = new_crop; /* if crop size changed then reset the output image size */ if (size_changed) - __imx274_change_compose(imx274, cfg, sel->which, - &new_crop.width, &new_crop.height, - sel->flags); + __imx274_change_crop_fmt(imx274, cfg, sel->which, + &new_crop.width, &new_crop.height, + sel->flags); mutex_unlock(&imx274->lock); return 0; } -static int imx274_set_selection(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, - struct v4l2_subdev_selection *sel) -{ - struct stimx274 *imx274 = to_imx274(sd); - - if (sel->pad != 0) - return -EINVAL; - - if (sel->target == V4L2_SEL_TGT_CROP) - return imx274_set_selection_crop(imx274, cfg, sel); - - if (sel->target == V4L2_SEL_TGT_COMPOSE) { - int err; - - mutex_lock(&imx274->lock); - err = __imx274_change_compose(imx274, cfg, sel->which, - &sel->r.width, &sel->r.height, - sel->flags); - mutex_unlock(&imx274->lock); - - /* - * __imx274_change_compose already set width and - * height in set->r, we still need to set top-left - */ - if (!err) { - sel->r.top = 0; - sel->r.left = 0; - } - - return err; - } - - return -EINVAL; -} - static int imx274_apply_trimming(struct stimx274 *imx274) { u32 h_start;
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 not supported at all. So drop the bogus composition support. Support for V4L2_SEL_TGT_CROP_DEFAULT was missing in imx274_get_selection, which meant that VIDIOC_CROPCAP couldn't be emulated and that caused a v4l2-compliance failure. Add support for this target to fix this. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- This patch has been in my queue for almost half a year. I thought I had posted it before, but apparently not. Apologies for that. --- drivers/media/i2c/imx274.c | 125 +++++++++++-------------------------- 1 file changed, 36 insertions(+), 89 deletions(-)