Hi Jacopo
On Fri, 2 Jun 2023 at 14:58, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Dave
>
> On Tue, May 30, 2023 at 06:30:00PM +0100, Dave Stevenson wrote:
> > The sensor supports H & V flips, but the controls were READ_ONLY.
> >
> > Note that the Bayer order changes with these flips, therefore
> > they set the V4L2_CTRL_FLAG_MODIFY_LAYOUT property.
>
> Nice!
>
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx258.c | 99 ++++++++++++++++++++++++--------------
> > 1 file changed, 64 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 98b5c1e3abff..cf90ac66e14c 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -83,8 +83,8 @@
> >
> > /* Orientation */
> > #define REG_MIRROR_FLIP_CONTROL 0x0101
> > -#define REG_CONFIG_MIRROR_FLIP 0x03
> > -#define REG_CONFIG_FLIP_TEST_PATTERN 0x02
> > +#define REG_CONFIG_MIRROR_HFLIP 0x01
> > +#define REG_CONFIG_MIRROR_VFLIP 0x02
> >
> > /* IMX258 native and active pixel array size. */
> > #define IMX258_NATIVE_WIDTH 4224U
> > @@ -484,6 +484,23 @@ static const struct imx258_variant_cfg imx258_pdaf_cfg = {
> > .num_regs = ARRAY_SIZE(imx258_pdaf_cfg_regs),
> > };
> >
> > +/*
> > + * The supported formats.
> > + * This table MUST contain 4 entries per format, to cover the various flip
> > + * combinations in the order
> > + * - no flip
> > + * - h flip
> > + * - v flip
> > + * - h&v flips
> > + */
> > +static const u32 codes[] = {
> > + /* 10-bit modes. */
> > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > + MEDIA_BUS_FMT_SGRBG10_1X10,
> > + MEDIA_BUS_FMT_SGBRG10_1X10,
> > + MEDIA_BUS_FMT_SBGGR10_1X10
> > +};
> > +
> > static const char * const imx258_test_pattern_menu[] = {
> > "Disabled",
> > "Solid Colour",
> > @@ -677,6 +694,8 @@ struct imx258 {
> > struct v4l2_ctrl *vblank;
> > struct v4l2_ctrl *hblank;
> > struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *hflip;
> > + struct v4l2_ctrl *vflip;
> > /* Current long exposure factor in use. Set through V4L2_CID_VBLANK */
> > unsigned int long_exp_shift;
> >
> > @@ -780,9 +799,22 @@ static int imx258_write_regs(struct imx258 *imx258,
> > return 0;
> > }
> >
> > +/* Get bayer order based on flip setting. */
> > +static u32 imx258_get_format_code(struct imx258 *imx258)
>
> can struct imx258 be const ?
It can
> > +{
> > + unsigned int i;
> > +
> > + lockdep_assert_held(&imx258->mutex);
> > +
> > + i = (imx258->vflip->val ? 2 : 0) |
> > + (imx258->hflip->val ? 1 : 0);
> > +
> > + return codes[i];
> > +}
>
> An empty line wouldn't hurt
Done
> > /* Open sub-device */
> > static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > {
> > + struct imx258 *imx258 = to_imx258(sd);
> > struct v4l2_mbus_framefmt *try_fmt =
> > v4l2_subdev_get_try_format(sd, fh->state, 0);
> > struct v4l2_rect *try_crop;
> > @@ -790,7 +822,7 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > /* Initialize try_fmt */
> > try_fmt->width = supported_modes[0].width;
> > try_fmt->height = supported_modes[0].height;
> > - try_fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > + try_fmt->code = imx258_get_format_code(imx258);
> > try_fmt->field = V4L2_FIELD_NONE;
> >
> > /* Initialize try_crop */
> > @@ -903,10 +935,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> > ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
> > IMX258_REG_VALUE_16BIT,
> > ctrl->val);
> > - ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> > - IMX258_REG_VALUE_08BIT,
> > - !ctrl->val ? REG_CONFIG_MIRROR_FLIP :
> > - REG_CONFIG_FLIP_TEST_PATTERN);
> > break;
> > case V4L2_CID_WIDE_DYNAMIC_RANGE:
> > if (!ctrl->val) {
> > @@ -928,6 +956,15 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> > ret = imx258_set_frame_length(imx258,
> > imx258->cur_mode->height + ctrl->val);
> > break;
> > + case V4L2_CID_VFLIP:
> > + case V4L2_CID_HFLIP:
> > + ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> > + IMX258_REG_VALUE_08BIT,
> > + (imx258->hflip->val ?
> > + REG_CONFIG_MIRROR_HFLIP : 0) |
> > + (imx258->vflip->val ?
> > + REG_CONFIG_MIRROR_VFLIP : 0));
> > + break;
> > default:
> > dev_info(&client->dev,
> > "ctrl(id:0x%x,val:0x%x) is not handled\n",
> > @@ -949,11 +986,13 @@ static int imx258_enum_mbus_code(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_mbus_code_enum *code)
> > {
> > - /* Only one bayer order(GRBG) is supported */
> > + struct imx258 *imx258 = to_imx258(sd);
> > +
> > + /* Only one bayer format (10 bit) is supported */
> > if (code->index > 0)
> > return -EINVAL;
> >
> > - code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > + code->code = imx258_get_format_code(imx258);
> >
> > return 0;
> > }
> > @@ -962,10 +1001,11 @@ static int imx258_enum_frame_size(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_frame_size_enum *fse)
> > {
> > + struct imx258 *imx258 = to_imx258(sd);
> > if (fse->index >= ARRAY_SIZE(supported_modes))
> > return -EINVAL;
> >
> > - if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
> > + if (fse->code != imx258_get_format_code(imx258))
> > return -EINVAL;
> >
> > fse->min_width = supported_modes[fse->index].width;
> > @@ -976,12 +1016,13 @@ static int imx258_enum_frame_size(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > -static void imx258_update_pad_format(const struct imx258_mode *mode,
> > +static void imx258_update_pad_format(struct imx258 *imx258,
> > + const struct imx258_mode *mode,
>
> Can't you get mode from imx258->cur_mode ?
It's called from imx258_set_pad_format for both SET and TRY, so needs
to be able to work on the new mode to be selected/tried as well as the
current mode.
> > struct v4l2_subdev_format *fmt)
> > {
> > fmt->format.width = mode->width;
> > fmt->format.height = mode->height;
> > - fmt->format.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > + fmt->format.code = imx258_get_format_code(imx258);
> > fmt->format.field = V4L2_FIELD_NONE;
> > }
> >
> > @@ -994,7 +1035,7 @@ static int __imx258_get_pad_format(struct imx258 *imx258,
> > sd_state,
> > fmt->pad);
> > else
> > - imx258_update_pad_format(imx258->cur_mode, fmt);
> > + imx258_update_pad_format(imx258, imx258->cur_mode, fmt);
> >
> > return 0;
> > }
> > @@ -1030,13 +1071,12 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
> >
> > mutex_lock(&imx258->mutex);
> >
> > - /* Only one raw bayer(GBRG) order is supported */
> > - fmt->format.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > + fmt->format.code = imx258_get_format_code(imx258);
> >
> > mode = v4l2_find_nearest_size(supported_modes,
> > ARRAY_SIZE(supported_modes), width, height,
> > fmt->format.width, fmt->format.height);
> > - imx258_update_pad_format(mode, fmt);
> > + imx258_update_pad_format(imx258, mode, fmt);
> > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > *framefmt = fmt->format;
> > @@ -1186,15 +1226,6 @@ static int imx258_start_streaming(struct imx258 *imx258)
> > return ret;
> > }
> >
> > - /* Set Orientation be 180 degree */
> > - ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> > - IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
> > - if (ret) {
> > - dev_err(&client->dev, "%s failed to set orientation\n",
> > - __func__);
> > - return ret;
> > - }
> > -
> > /* Apply customized values from user */
> > ret = __v4l2_ctrl_handler_setup(imx258->sd.ctrl_handler);
> > if (ret)
> > @@ -1383,7 +1414,6 @@ static int imx258_init_controls(struct imx258 *imx258)
> > struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> > const struct imx258_link_freq_config *link_freq_cfgs;
> > struct v4l2_fwnode_device_properties props;
> > - struct v4l2_ctrl *vflip, *hflip;
> > struct v4l2_ctrl_handler *ctrl_hdlr;
> > const struct imx258_link_cfg *link_cfg;
> > s64 vblank_def;
> > @@ -1408,16 +1438,15 @@ 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;
> > + imx258->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 1);
> > + if (imx258->hflip)
> > + imx258->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >
> > - vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > - V4L2_CID_VFLIP, 1, 1, 1, 1);
> > - if (vflip)
> > - vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > + imx258->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 1);
> > + if (imx258->vflip)
> > + imx258->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> if flips are now writable, should they be enabled by default ?
This is the potential debate.
An existing userspace app is using this driver and it renders as
desired. Merge this change and suddenly all their images are upside
down, and that's assuming they haven't hard coded the Bayer order
which has now changed due to correcting the cropping registers.
Some would say that is a regression.
I know you'd discussed it with Sakari, but wasn't totally clear on the
final decision. It's part of the reason why I had this as the final
patch, because then it's easy to drop whilst discussing it.
Dave
> >
> > link_freq_cfgs = &imx258->link_freq_configs[0];
> > link_cfg = link_freq_cfgs[imx258->lane_mode_idx].link_cfg;
> > --
> > 2.25.1
> >
