Hi Dave,
On Thu, Jul 21, 2022 at 04:39:08PM +0100, Dave Stevenson wrote:
> On Thu, 21 Jul 2022 at 09:36, Laurent Pinchart wrote:
> > Implement read-only access to crop selection rectangles to expose the
> > analogue crop rectangle. The public (leaked) IMX290 documentation is not
> > very clear on how cropping is implemented and configured exactly, so
> > the margins may not be entirely accurate.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx290.c | 94 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index baf9941c5fbe..0cb11ec1cf0f 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -105,6 +105,53 @@
> >
> > #define IMX290_VMAX_DEFAULT 1125
> >
> > +
> > +/*
> > + * The IMX290 pixel array is organized as follows:
> > + *
> > + * +------------------------------------+
> > + * | Optical Black | } Vertical effective optical black (10)
> > + * +---+------------------------------------+---+
> > + * | | | | } Effective top margin (8)
> > + * | | +----------------------------+ | | \
> > + * | | | | | | |
> > + * | | | | | | |
> > + * | | | | | | |
> > + * | | | Recording Pixel Area | | | | Recommended height (1080)
> > + * | | | | | | |
> > + * | | | | | | |
> > + * | | | | | | |
> > + * | | +----------------------------+ | | /
> > + * | | | | } Effective bottom margin (8)
>
> I have an official datasheet from Sony. "Effective margin for colour
> processing" at the bottom is stated to be 9 lines, not 8. That also
> makes the numbers then tally of total height being 8 + 1080 + 9 =
> 1097.
Will fix in v2. The value in the text and macro is correct, only the
diagram has an issue.
> Otherwise I agree with the numbers you quote here.
>
> My datasheet does go into how window mode is configured, however
> window mode is not being used.
> The register sets present in the driver set the output image size to
> 1920x1080 or 1280x720 of the overall 1945x1097 pixels. They differ
> slightly from the definitions given in the datasheet for the Full HD
> 1080p and HD720p modes which read out 1945x1097 and 1297x725 pixels
> respectively (assuming I've read it correctly). Exactly how those
> extra pixels are cropped off isn't defined, but I'd suspect it was the
> top left portion of the full image.
>
> If you want 100% defined cropping for each mode then that should be
> achievable using window mode.
I'd like that, but one thing at a time :-) I may experiment if I can
find free time.
> > + * +---+------------------------------------+---+
> > + * <-> <-> <--------------------------> <-> <->
> > + * \---- Ignored right margin (4)
> > + * \-------- Effective right margin (9)
> > + * \------------------------- Recommended width (1920)
> > + * \----------------------------------------- Effective left margin (8)
> > + * \--------------------------------------------- Ignored left margin (4)
> > + *
> > + * The optical black lines are output over CSI-2 with a separate data type.
> > + *
> > + * The pixel array is meant to have 1920x1080 usable pixels after image
> > + * processing in an ISP. It has 8 (9) extra active pixels usable for color
> > + * processing in the ISP on the top and left (bottom and right) sides of the
> > + * image. In addition, 4 additional pixels are present on the left and right
> > + * sides of the image, documented as "ignored area".
> > + *
> > + * As far as is understood, all pixels of the pixel array (ignored area, color
> > + * processing margins and recording area) can be output by the sensor.
> > + */
> > +
> > +#define IMX290_PIXEL_ARRAY_WIDTH 1945
> > +#define IMX290_PIXEL_ARRAY_HEIGHT 1097
> > +#define IMX920_PIXEL_ARRAY_MARGIN_LEFT 12
> > +#define IMX920_PIXEL_ARRAY_MARGIN_RIGHT 13
> > +#define IMX920_PIXEL_ARRAY_MARGIN_TOP 8
> > +#define IMX920_PIXEL_ARRAY_MARGIN_BOTTOM 9
> > +#define IMX290_PIXEL_ARRAY_RECORDING_WIDTH 1920
> > +#define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT 1080
> > +
> > static const char * const imx290_supply_name[] = {
> > "vdda",
> > "vddd",
> > @@ -671,6 +718,52 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static int imx290_get_selection(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct imx290 *imx290 = to_imx290(sd);
> > + struct v4l2_mbus_framefmt *format;
> > +
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_CROP: {
> > + format = imx290_get_pad_format(imx290, sd_state, sel->which);
> > +
> > + mutex_lock(&imx290->lock);
> > +
> > + sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
> > + + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
> > + sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT
> > + + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2;
> > + sel->r.width = format->width;
> > + sel->r.height = format->height;
> > +
> > + mutex_unlock(&imx290->lock);
> > + return 0;
> > + }
> > +
> > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + sel->r.top = 0;
> > + sel->r.left = 0;
> > + sel->r.width = IMX290_PIXEL_ARRAY_WIDTH;
> > + sel->r.height = IMX290_PIXEL_ARRAY_HEIGHT;
> > +
> > + return 0;
> > +
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP;
> > + sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT;
> > + sel->r.width = IMX290_PIXEL_ARRAY_RECORDING_WIDTH;
> > + sel->r.height = IMX290_PIXEL_ARRAY_RECORDING_HEIGHT;
> > +
> > + return 0;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state)
> > {
> > @@ -887,6 +980,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
> > .enum_frame_size = imx290_enum_frame_size,
> > .get_fmt = imx290_get_fmt,
> > .set_fmt = imx290_set_fmt,
> > + .get_selection = imx290_get_selection,
> > };
> >
> > static const struct v4l2_subdev_ops imx290_subdev_ops = {
--
Regards,
Laurent Pinchart
