Re: [PATCH 2/2] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Hans de Goede]

Hi Kate,

On 17-Apr-26 10:32, Kate Hsuan wrote:
> Add a new driver for Sony imx471 camera sensor. It is based on
> Jimmy Su <jimmy.su@xxxxxxxxx> implementation and the driver can be found
> in the following URL.
> https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
> 
> This sensor can be found on Lenovo X9-14 and X9-15 laptop and it is a part
> of IPU7 solution. The driver was tested on Lenovo X9-14 and X9-15 laptops.
> 
> Link: https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/imx471.c
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2454119
> Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
<snip>

> diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> new file mode 100644
> index 000000000000..32a105a60731
> --- /dev/null
> +++ b/drivers/media/i2c/imx471.c
> @@ -0,0 +1,1047 @@

<snip>

> +static int imx471_update_flip(struct imx471_data *sensor, u32 value,
> +			      u8 flip_bit)
> +{
> +	int ret;
> +	u64 val = value ? flip_bit : 0;
> +
> +	if (sensor->streaming)
> +		return -EBUSY;

I see no reason why this could not be updated while streaming,
since the h/y offsets get adjusted the bayer pattern stays
the same so changing while streaming should be fine.

> +
> +	/* hflip */
> +	/*
> +	 * Some manufacturers mount the sensor upside-down (rotation == 180).
> +	 * V4L2 sets both h/vflip to 1 for 180-degree rotation, but only the
> +	 * vflip should actually be applied. Skip the initial hflip write to
> +	 * preserve correct orientation.
> +	 */

I was answering your off-list email about this, but now I see that you've
added this workaround here. I believe that this workaround is wrong, so
let me move answer things here instead of off-list:

> I filled in the DMI information in the table and I found v4l2 sets up
> both hflip=1 and vflip=1 when the rotation is 180.

Yes that is correct, note this is actually done by libcamera, in response
to the rotation property reporting 180 degrees rotation after adding the
laptop to the DMI table.

> In my case, I only
> need to set vflip then I can get a correct image.

First of all are you sure that you only need to set vflip? A camera is not
a mirror! If you say raise your right hand in front of the camera then on
the screen you should be seen raising the hand which is on the left for
"the you" looking at the screen because if you were to look at you from
the pov of the camera your right hand is on the left.

The easiest way to check this is to have something with some written text
on it. In a mirror you cannot (easily) read e.g. the text printed on
a T-shirt but with a camera you should be able to read this without
problems.

Also make sure you use qcam to test because qcam does not mirror/hflip.
Some apps hflip the image for you (esp. things like google meet) because
people are so used to seeing themselves in a mirror that they adjust
the view for you. Note e.g. google meet only mirrors your own preview
it sends out an unmirrored image to the people on the call (IIRC).

If after this long mansplaining (sorry) writeup about the difference
between a mirror and a camera you still think you only need vflip,
then that means that either the hflip ot the vflip control of
the sensor is inverted and the driver needs to invert it.

Are we sure the camera module is upside down? Maybe vflip is the one
which we need to invert and the module is not upside-down at all ?

Hmm, looking at other imx sensor drivers, unlike ov sensors where
sometimes hflip is inverted it seems the 2 flip controls are sofar
always straight forward on imx. Although some drivers only implement
vflip and have no hflip at all.

As you mention in the cover letter this is a cleaned up version of:
https://github.com/intel/ipu6-drivers/tree/master/drivers/media/i2c/imx471.c

Note that we've seen issues with mirroring / flipping from various
other drivers originating from Intel, they have not always got this
correct, especially when it comes to mirroring by default (when
the hflip control's value is 0) but also with vflipping by default
when the driver was developed on a laptop which had the module
upside-down, see e.g. :

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/media/i2c/ov02c10.c

where we needed to do quite a few flipping related fixes.

> +	if (flip_bit == IMX471_HFLIP_BIT) {
> +		if (sensor->rotation == 180 && !sensor->hflip_initialized) {
> +			sensor->hflip_initialized = true;
> +			return 0;
> +		}

This looks like you skip writing the hflip on the first start stream,
but what about subsequent streams ?

Also see my next comment below, I think this skipping only once
does point us in the right direction.

> +
> +		cci_update_bits(sensor->regmap, IMX471_REG_ORIENTATION,
> +				flip_bit, val, &ret);
> +
> +		return ret;
> +	}
> +
> +	/* vflip */
> +	cci_update_bits(sensor->regmap, IMX471_REG_ORIENTATION,
> +			flip_bit, val, &ret);
> +	if (ret)
> +		return ret;

Hmm, I wonder if the problem here is you doing 2 subsequent
cci_update_bits(). If the flip control registered is double-buffered
and the new value is latched as the actual value on the start
of the next frame; and this is combined with reading back
reading the active value, not the last written value then
the first time you do this the setting of the hflip bit will
be overwritten by the second cci_update_bits.

I think it would be better to do something similar to what
imx219.c and replace these 2 cci_update_bits() calls with:

        cci_write(imx471->regmap, IMX471_REG_ORIENTATION,
                  imx471->hflip->val | imx471->vflip->val << 1, &ret);

I believe this should work here too.


> +
> +	cci_write(sensor->regmap, IMX471_REG_V_WIN_OFFSET,
> +		  value ? 0xe0 : 0xeb, &ret);
> +	if (ret)
> +		return ret;
> +
> +	cci_update_bits(sensor->regmap, IMX471_REG_H_WIN_OFFSET, 1,
> +			value ? 0x01 : 0x00, &ret);

No need for cci_update_bits() here, the register is always
initialized to 0xc8 so this can just use hardcoded values
like the V_WIN_OFFSET path:

	cci_write(sensor->regmap, IMX471_REG_H_WIN_OFFSET,
		  value ? 0xc9 : 0xc8, &ret);

> +	return ret;

Updating both offsets here is wrong when hflip != vflip, you
should only update V_WIN_OFFSET when changing vflip and
H_WIN_OFFSET when changing hflip.

I suggest dropping this function and instead in set_ctrl()
do this:

	case V4L2_CID_HFLIP:
		cci_write(imx471->regmap, IMX471_REG_ORIENTATION,
			  imx471->hflip->val | imx471->vflip->val << 1, &ret);
		cci_write(sensor->regmap, IMX471_REG_H_WIN_OFFSET,
			  value ? 0xc9 : 0xc8, &ret);
		break;
        case V4L2_CID_VFLIP:
		cci_write(imx471->regmap, IMX471_REG_ORIENTATION,
			  imx471->hflip->val | imx471->vflip->val << 1, &ret);
		cci_write(sensor->regmap, IMX471_REG_V_WIN_OFFSET,
			  value ? 0xe0 : 0xeb, &ret);
		break;

Regards,

Hans