Hi Sakari On Wed, Dec 04, 2024 at 02:15:07PM +0000, Sakari Ailus wrote: > 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. > Fine indeed! > > ------------------------------------------------------------------------------- > > > > 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? > I'm fine, after all there's no "analogue scaling" this can be confused with, right ? > > > > > + 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 >
