Re: [PATCH 2/3] media: imx258: Register H/V flip controls

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Tue, Jan 17, 2023 at 11:28:39AM +0100, Jacopo Mondi wrote:
> On Tue, Jan 17, 2023 at 10:17:13AM +0000, Sakari Ailus wrote:
> > On Tue, Jan 17, 2023 at 11:06:02AM +0100, Jacopo Mondi wrote:
> > > Register controls for V4L2_CID_HFLIP and V4L2_CID_VFLIP.
> > >
> > > The controls are registered as read-only and enabled by default, as the
> > > driver embeds a 180 degrees rotation in its programming sequences and
> > > only supports that mode of operations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx258.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > index 3b560865b657..2e0a4ea76589 100644
> > > --- a/drivers/media/i2c/imx258.c
> > > +++ b/drivers/media/i2c/imx258.c
> > > @@ -1151,6 +1151,7 @@ static int imx258_init_controls(struct imx258 *imx258)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> > >  	struct v4l2_fwnode_device_properties props;
> > >  	struct v4l2_ctrl_handler *ctrl_hdlr;
> > > +	struct v4l2_ctrl *vflip, *hflip;
> > >  	s64 vblank_def;
> > >  	s64 vblank_min;
> > >  	s64 pixel_rate_min;
> > > @@ -1158,7 +1159,7 @@ static int imx258_init_controls(struct imx258 *imx258)
> > >  	int ret;
> > >
> > >  	ctrl_hdlr = &imx258->ctrl_handler;
> > > -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > @@ -1174,6 +1175,17 @@ static int imx258_init_controls(struct imx258 *imx258)
> > >  	if (imx258->link_freq)
> > >  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > +	/* The driver only supports one bayer order and flips by default. */
> > > +	hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > > +				  V4L2_CID_HFLIP, 1, 1, 1, 1);
> > > +	if (hflip)
> > > +		hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > +	vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > > +				  V4L2_CID_VFLIP, 1, 1, 1, 1);
> >
> > This defaults the controls to 1, which suggests the image is upside down.
> >
> > The rotation property has been used to tell the driver the sensor is upside
> 
> Well the rotation property should tell userspace how the camera is
> mounted, not to the driver how to compensate it ? There are several
> reasons not to automatically compensate in the driver, mostly the fact
> that the desired rotation to have the images in the "right"
> orientation might require a transpose (as in example for 90 degrees or
> 270 degrees mounting rotations) which sensors cannot actually do.
> 
> I would rather inform userspace of the mounting rotation and the
> sensor capabilities and have them decide how to compensate it.
> 
> > down, and this has also had the effect of reversing flip contron values so
> > if they're disabled, the image is upright.
> 
> Ok, but why should the driver only accept a mounting rotation of 180
> degrees ? If your sensor is mounted on a mobile device (as the
> PinephonPro where this sensor is used) the phyisical mounting rotation
> is actually 270 degrees.
> 
> Userspace will see that flips are not writable, the rotation to
> compensate is 270 degrees, and will handle that as it like the most.

This sounds good to me. The alternative would be to avoid exposing the
flip controls to userspace, and add 180 to the rotation reported in DT
before passing it to userspace. I don't think that's the right option,
drivers shouldn't try to hide this.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > See e.g. the CCS driver.
> >
> > I'd do the same here.
> >
> > > +	if (vflip)
> > > +		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > >  	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> > >  	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> > >  	/* By default, PIXEL_RATE is read only */

-- 
Regards,

Laurent Pinchart