Hi Jacopo, Thank you for the review. On Wed, Dec 04, 2024 at 12:25:11PM +0100, Jacopo Mondi wrote: > Hi Sakari > thanks for the update > > On Fri, Nov 29, 2024 at 11:51:38AM +0200, Sakari Ailus wrote: > > Document scaling and post-scaler digital crop operations for the common > > raw sensor model. > > Signedg-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > --- > > .../media/v4l/subdev-config-model.rst | 19 +++++++++++++++---- > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/subdev-config-model.rst b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > > index 4ddf98e3143c..1ae20800f34b 100644 > > --- a/Documentation/userspace-api/media/v4l/subdev-config-model.rst > > +++ b/Documentation/userspace-api/media/v4l/subdev-config-model.rst > > @@ -119,9 +119,13 @@ The digital crop operation takes place after binning and sub-sampling. It is > > configured by setting the ``V4L2_SEL_TGT_CROP`` rectangle on (pad, stream) pair > > 0/0. The resulting image size is further output by the sensor. > > I think this last line "The resulting image size is further output by > the sensor." doesn't apply anymore now that we have [digital crop + > digital scaling + post-scaling crop]. Indeed. I'll address this in v4. > > > > > +The digital scaling operation is performed after the digital crop. It is > > +configured by setting the ``V4L2_SEL_TGT_COMPOSE`` rectangle on (pad, stream) pair > > +0/0, relative to the digital crop. > > + > > "to the digital crop rectangle." maybe ? Yes. > > > > The sensor's output mbus code is configured by setting the format on the (pad, > > -stream) pair 0/0. When setting the format, always use the same width and height > > -as for the digital crop setting. > > +stream) pair 0/0. The width and height fields are used to configure post-scaler > > +digital crop, affecting the right side and the bottom of the frame. > > > > I would introduce the (optional) presence of an additional post-scaler > digital crop step explicitly, also I get this is kind of rare feature, > isn't it ? > > ------------------------------------------------------------------------------- > The digital scaling operation is performed after the digital crop. It > is configured by setting the ``V4L2_SEL_TGT_COMPOSE`` rectangle on > (pad, stream) pair 0/0, relative to the digital crop rectangle. > > The sensor's output mbus code is configured by setting the format on the (pad, > stream) pair 0/0. An optional post-scaler crop step might be performed by > specifying a width and height smaller than the digital scaling > rectangle. If post-scaler cropping is not supported the format's sizes > will always match the compose rectangle sizes applied on the same 0/0 > (pad, stream) pair. Right, even if the post-scaler crop is optional, the format is always there but unmodifiable (directly). I'd refer to driver support instead of somewhat nebulous "might be performed". How about: The sensor's output mbus code is configured by setting the format on the (pad, stream) pair 0/0. The width and height fields are used to configure post-scaler digital crop if supported by the driver, affecting the right and bottom edges of the frame. If post-scaler digital crop is not supported, the width and height fields of the format will match the compose rectangle sizes applied on the same 0/0 (pad, stream) pair. > ------------------------------------------------------------------------------- > > I didn't get when we spoke about it where the "affecting the right > side and bottom of the frame" comes from when talking about > post-scaler cropping. > > > > Drivers may only support some of even none of these configurations, in which > > case they do not expose the corresponding selection rectangles. If any selection > > @@ -179,12 +183,19 @@ Also refer to :ref:`Selection targets <v4l2-selection-targets-table>`. > > - X > > - Digital crop. This rectangle is relative to the ``V4L2_SEL_TGT_COMPOSE`` > > rectangle on (pad, stream) pair 1/0. > > + * - 0/0 > > + - ``V4L2_SEL_TGT_COMPOSE`` > > + - \- > > + - X > > + - Scaling. This rectangle is relative to the ``V4L2_SEL_TGT_CROP`` > > "Digital scaling" ? or is not necessary ? We don't mention it in the context of sub-sampling either. Digital and analogue variants are separately documented where they exist, I think this should be fine. What do you think? > > > + rectangle on (pad, stream) pair 0/0. > > * - 0/0 > > - Format > > - X > > - X > > - - Image data source format. Always assign the width and height fields of > > - the format to the same values than for the ``V4L2_SEL_TGT_CROP`` > > + - Image data source format and post-scaler crop. The width and height > > + fields of the format, used to configure post-scaler crop on the right > > + and bottom edges of the image, are related to the ``V4L2_SEL_TGT_COMPOSE`` > > rectangle on (pad, stream) pair 0/0. The media bus code reflects the > > pixel data output of the sensor. > > * - 0/1 -- Kind regards, Sakari Ailus
