Message-ID: <ZV3CaqNVlnB5O1A0@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Date:   Wed, 22 Nov 2023 09:57:14 +0100
From:   Tommaso Merciai <tomm.merciai@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     martin.hecht@...et.eu, michael.roeder@...et.eu, mhecht73@...il.com,
        linuxfancy@...glegroups.com, laurent.pinchart@...asonboard.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        Gerald Loacker <gerald.loacker@...fvision.net>,
        Bingbu Cao <bingbu.cao@...el.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Nicholas Roth <nicholas@...hemail.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v13 3/3] media: i2c: Add support for alvium camera

Hi Sakary,
Thanks for your quick reply :)

On Wed, Nov 22, 2023 at 08:46:42AM +0000, Sakari Ailus wrote:
> Hi Tommaso,
> 
> On Wed, Nov 22, 2023 at 09:36:21AM +0100, Tommaso Merciai wrote:
> > > > > > > > +static int alvium_get_bcrm_vers(struct alvium_dev *alvium)
> > > > > > > > +{
> > > > > > > > +	struct device *dev = &alvium->i2c_client->dev;
> > > > > > > > +	struct alvium_bcrm_vers *v;
> > > > > > > > +	u64 val;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	ret = alvium_read(alvium, REG_BCRM_VERSION_R, &val, NULL);
> > > > > > > > +	if (ret)
> > > > > > > > +		return ret;
> > > > > > > > +
> > > > > > > > +	v = (struct alvium_bcrm_vers *) &val;
> > > > > > > 
> > > > > > > No space before "&" in type cast, please. The same elsewhere.
> > > > > > > 
> > > > > > > As you cast a single value to a struct, I think the struct field values
> > > > > > > will be swapped on BE systems. You'll need to convert each value
> > > > > > > separately. Same for struct alvium_fw_vers below.
> > > > > > 
> > > > > > What about:
> > > > > > 
> > > > > >  v->minor = le16_to_cpu(v->minor);
> > > > > >  v->major = le16_to_cpu(v->major);
> > > > > > 
> > > > > > here. I posted this solution in some previous v :)
> > > > > 
> > > > > You shouldn't assign it to a field marked little endian. Instead, use
> > > > > le16_to_cpu() when you access the data below.
> > > > > 
> > > > > If you want to access the struct in the driver without using the conversion
> > > > > macros, you should read the data one field at a time (and use u16 instead
> > > > > of __le16 type for the fields).
> > > > 
> > > > It's fine with your suggestion thanks.
> > > > I'm moving to use the following to prints those values:
> > > > 
> > > > 	v = (struct alvium_bcrm_vers *)&val;
> > > > 
> > > > 	dev_info(dev, "bcrm version: %u.%u\n",
> > > > 		 le16_to_cpu(v->minor), le16_to_cpu(v->major));
> > > > 
> > > > thanks for the hint.
> > > 
> > > Sorry, I forgot alvium_read(), via cci_read(), already does endianness
> > > conversion here, from big endian to CPU endianness. Is there a need to do
> > > further conversion here? Noting that le16_to_cpu() does nothing on little
> > > endian systems, is it necessary here?
> > > 
> > > The options here are either changing the struct fields to CPU endianness
> > > and reading them individually or accessing the register as a single 32-bit
> > > value.
> > > 
> > > I'd do the former, it's easier to access them that way in the driver.
> > > 
> > > The same applies to BCRM version below.
> > > 
> > > struct alvium_bcrm_vers {
> > > 	u16 minor;
> > > 	u16 major;
> > > };
> > 
> > Understood, thanks.
> > 
> > Then to resume just compared to v13 I just need to
> > revert the alvium_bcrm_vers/alvium_fw_vers struct
> > to:
> > 
> > struct alvium_bcrm_vers {
> > 	u16 minor;
> > 	u16 major;
> > };
> > 
> > struct alvium_fw_vers {
> > 	u8 special;
> > 	u8 major;
> > 	u16 minor;
> > 	u16 patch;
> > };
> 
> Yes, and accessing the registers one field at a time.

I'm doing that into dev_info or I'm wrong?

	v = (struct alvium_bcrm_vers *)&val;

	dev_info(dev, "bcrm version: %u.%u\n", v->minor, v->major);

same for:

	fw_v = (struct alvium_fw_vers *)&val;

	dev_info(dev, "fw version: %u.%u.%u.%u\n", fw_v->special, fw_v->major,
		 fw_v->minor, fw_v->patch);

Sorry but I want be sure to be aligned with you :)

Thanks & Regards,
Tommaso

> 
> > > > > > > > +static int alvium_init_cfg(struct v4l2_subdev *sd,
> > > > > > > > +			   struct v4l2_subdev_state *state)
> > > > > > > > +{
> > > > > > > > +	struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > > +	struct alvium_mode *mode = &alvium->mode;
> > > > > > > > +	struct v4l2_subdev_format sd_fmt = {
> > > > > > > > +		.which = V4L2_SUBDEV_FORMAT_TRY,
> > > > > > > > +		.format = alvium_csi2_default_fmt,
> > > > > > > > +	};
> > > > > > > > +	struct v4l2_subdev_crop sd_crop = {
> > > > > > > > +		.which = V4L2_SUBDEV_FORMAT_TRY,
> > > > > > > > +		.rect = {
> > > > > > > > +			.left = mode->crop.left,
> > > > > > > > +			.top = mode->crop.top,
> > > > > > > > +			.width = mode->crop.width,
> > > > > > > > +			.height = mode->crop.height,
> > > > > > > > +		},
> > > > > > > > +	};
> > > > > > > > +
> > > > > > > > +	*v4l2_subdev_get_pad_crop(sd, state, 0) = sd_crop.rect;
> > > > > > > > +	*v4l2_subdev_get_pad_format(sd, state, 0) = sd_fmt.format;
> > > > > > > 
> > > > > > > Shouldn't the format have same width and height as crop? What about the
> > > > > > > mbus code?
> > > > 
> > > > Can you clarify to me this comment pls :)
> > > 
> > > The purpose of the init_cfg operation is to initialise the sub-device
> > > state, including format and crop rectangle (if applicable). The width and
> > > height fields of the format are not initialised above, leaving them zeros.
> > 
> > Mmmmm.
> > Why zeros?
> > 
> > .format = alvium_csi2_default_fmt
> > 
> > where:
> > 
> > static const struct v4l2_mbus_framefmt alvium_csi2_default_fmt = {
> > 	.code = MEDIA_BUS_FMT_UYVY8_1X16,
> > 	.width = 640,
> > 	.height = 480,
> > 	.colorspace = V4L2_COLORSPACE_SRGB,
> > 	.ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB),
> > 	.quantization = V4L2_QUANTIZATION_FULL_RANGE,
> > 	.xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB),
> > 	.field = V4L2_FIELD_NONE,
> > };
> 
> Ah, I missed this one. I think this part should be fine then.
> 
> > > That doesn't seem to be a valid configuration, given that the crop
> > > rectangle is initiliased with different values.
> > 
> > Why different values?
> > crop is initialized into subdev_init
> > 
> > static int alvium_subdev_init(struct alvium_dev *alvium)
> > {
> > 	struct i2c_client *client = alvium->i2c_client;
> > 	struct device *dev = &alvium->i2c_client->dev;
> > 	struct v4l2_subdev *sd = &alvium->sd;
> > 	int ret;
> > 
> > 	/* Setup initial frame interval*/
> > 	alvium->frame_interval.numerator = 1;
> > 	alvium->frame_interval.denominator = ALVIUM_DEFAULT_FR_HZ;
> > 	alvium->fr = ALVIUM_DEFAULT_FR_HZ;
> > 
> > 	/* Setup the initial mode */
> > 	alvium->mode.fmt = alvium_csi2_default_fmt;
> > 	alvium->mode.width = alvium_csi2_default_fmt.width;
> > 	alvium->mode.height = alvium_csi2_default_fmt.height;
> > 	alvium->mode.crop.left = alvium->min_offx;
> > 	alvium->mode.crop.top = alvium->min_offy;
> > 	alvium->mode.crop.width = alvium_csi2_default_fmt.width;
> > 	alvium->mode.crop.height = alvium_csi2_default_fmt.height;
> > ...
> > 
> > Then:
> > 
> > static int alvium_init_cfg(struct v4l2_subdev *sd,
> > 			   struct v4l2_subdev_state *state)
> > {
> > 	struct alvium_dev *alvium = sd_to_alvium(sd);
> > 	struct alvium_mode *mode = &alvium->mode;
> > 	struct v4l2_subdev_format sd_fmt = {
> > 		.which = V4L2_SUBDEV_FORMAT_TRY,
> > 		.format = alvium_csi2_default_fmt,
> > 	};
> > 	struct v4l2_subdev_crop sd_crop = {
> > 		.which = V4L2_SUBDEV_FORMAT_TRY,
> > 		.rect = {
> > 			.left = mode->crop.left,
> > 			.top = mode->crop.top,
> > 			.width = mode->crop.width,
> > 			.height = mode->crop.height,
> > 		},
> > 	};
> > ...
> > 
> > Seems that crop and format are using the same init values
> > or I'm wrong?
> > Mmm.. maybe I'm missing something?
> > 
> > Let me know
> > 
> > > 
> > > > 
> > > > > > > 
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int alvium_set_fmt(struct v4l2_subdev *sd,
> > > > > > > > +			  struct v4l2_subdev_state *sd_state,
> > > > > > > > +			  struct v4l2_subdev_format *format)
> > > > > > > > +{
> > > > > > > > +	struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > > +	const struct alvium_pixfmt *alvium_csi2_fmt;
> > > > > > > > +	struct v4l2_mbus_framefmt *fmt;
> > > > > > > > +	struct v4l2_rect *crop;
> > > > > > > > +
> > > > > > > > +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > > > > > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > > > +
> > > > > > > > +	fmt->width = clamp(format->format.width, alvium->img_min_width,
> > > > > > > > +			   alvium->img_max_width);
> > > > > > > > +	fmt->height = clamp(format->format.height, alvium->img_min_height,
> > > > > > > > +			    alvium->img_max_height);
> > > > > > > > +
> > > > > > > > +	/* Adjust left and top to prevent roll over sensor area */
> > > > > > > > +	crop->left = clamp((u32)crop->left, (u32)0,
> > > > > > > > +			   (alvium->img_max_width - fmt->width));
> > > > > > > > +	crop->top = clamp((u32)crop->top, (u32)0,
> > > > > > > > +			  (alvium->img_max_height - fmt->height));
> > > > > > > > +
> > > > > > > > +	/* Set also the crop width and height when set a new fmt */
> > > > > > > > +	crop->width = fmt->width;
> > > > > > > > +	crop->height = fmt->height;
> > > > > > > > +
> > > > > > > > +	alvium_csi2_fmt = alvium_code_to_pixfmt(alvium, format->format.code);
> > > > > > > > +	fmt->code = alvium_csi2_fmt->code;
> > > > > > > > +
> > > > > > > > +	*fmt = format->format;
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int alvium_set_selection(struct v4l2_subdev *sd,
> > > > > > > > +				struct v4l2_subdev_state *sd_state,
> > > > > > > > +				struct v4l2_subdev_selection *sel)
> > > > > > > > +{
> > > > > > > > +	struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > > +	struct v4l2_mbus_framefmt *fmt;
> > > > > > > > +	struct v4l2_rect *crop;
> > > > > > > > +
> > > > > > > > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > > > +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * Alvium can only shift the origin of the img
> > > > > > > > +	 * then we accept only value with the same value of the actual fmt
> > > > > > > > +	 */
> > > > > > > > +	if (sel->r.width != fmt->width)
> > > > > > > > +		sel->r.width = fmt->width;
> > > > > > > > +
> > > > > > > > +	if (sel->r.height != fmt->height)
> > > > > > > > +		sel->r.height = fmt->height;
> > > > > > > > +
> > > > > > > > +	if (sel->target != V4L2_SEL_TGT_CROP)
> > > > > > > > +		return -EINVAL;
> > > > > > > 
> > > > > > > This should be the first thing to test.
> > > > > > 
> > > > > > Oks
> > > > > > 
> > > > > > > 
> > > > > > > > +
> > > > > > > > +	/* alvium don't accept negative crop left/top */
> > > > > > > > +	crop->left = clamp((u32)max(0, sel->r.left), alvium->min_offx,
> > > > > > > > +			   alvium->img_max_width - sel->r.width);
> > > > > > > > +	crop->top = clamp((u32)max(0, sel->r.top), alvium->min_offy,
> > > > > > > > +			  alvium->img_max_height - sel->r.height);
> > > > > > > > +
> > > > > > > > +	sel->r = *crop;
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int alvium_get_selection(struct v4l2_subdev *sd,
> > > > > > > > +				struct v4l2_subdev_state *sd_state,
> > > > > > > > +				struct v4l2_subdev_selection *sel)
> > > > > > > > +{
> > > > > > > > +	struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > > +
> > > > > > > > +	switch (sel->target) {
> > > > > > > > +	/* Current cropping area */
> > > > > > > > +	case V4L2_SEL_TGT_CROP:
> > > > > > > > +		sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > > > +		break;
> > > > > > > > +	/* Cropping bounds */
> > > > > > > > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > > > > > > > +		sel->r.top = 0;
> > > > > > > > +		sel->r.left = 0;
> > > > > > > > +		sel->r.width = alvium->img_max_width;
> > > > > > > > +		sel->r.height = alvium->img_max_height;
> > > > > > > > +		break;
> > > > > > > > +	/* Default cropping area */
> > > > > > > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > > > > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > > > > > +		sel->r.top = alvium->min_offy;
> > > > > > > > +		sel->r.left = alvium->min_offx;
> > > > > > > > +		sel->r.width = alvium->img_max_width;
> > > > > > > > +		sel->r.height = alvium->img_max_height;
> > > > > > > > +		break;
> > > > > > > > +	default:
> > > > > > > > +		return -EINVAL;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int alvium_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > > > +{
> > > > > > > > +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > > > > > +	struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > > +	int val;
> > > > > > > > +
> > > > > > > > +	switch (ctrl->id) {
> > > > > > > > +	case V4L2_CID_GAIN:
> > > > > > > > +		val = alvium_get_gain(alvium);
> > > > > > > > +		if (val < 0)
> > > > > > > > +			return val;
> > > > > > > > +		alvium->ctrls.gain->val = val;
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_EXPOSURE:
> > > > > > > > +		val = alvium_get_exposure(alvium);
> > > > > > > > +		if (val < 0)
> > > > > > > > +			return val;
> > > > > > > > +		alvium->ctrls.exposure->val = val;
> > > > > > > > +		break;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int alvium_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > > > +{
> > > > > > > > +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > > > > > +	struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > > +	struct i2c_client *client = v4l2_get_subdevdata(&alvium->sd);
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * Applying V4L2 control value only happens
> > > > > > > > +	 * when power is up for streaming
> > > > > > > > +	 */
> > > > > > > > +	if (!pm_runtime_get_if_in_use(&client->dev))
> > > > > > > > +		return 0;
> > > > > > > > +
> > > > > > > > +	switch (ctrl->id) {
> > > > > > > > +	case V4L2_CID_AUTOGAIN:
> > > > > > > > +		ret = alvium_set_ctrl_gain(alvium, ctrl->val);
> > > > > > > 
> > > > > > > 		ret = alvium_set_autogain(alvium, ctrl->val);
> > > > > > > 
> > > > > > 
> > > > > > Pls check [1]
> > > > > > 
> > > > > > > Where do you set the manual gain value? What about the manual exposure
> > > > > > > value? Both appear to be missing here.
> > > > > > > 
> > > > > > > How have you tested this?
> > > > > > > 
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_EXPOSURE_AUTO:
> > > > > > > > +		ret = alvium_set_ctrl_exposure(alvium, ctrl->val);
> > > > > > > 
> > > > > > > 		ret = alvium_set_autoexposure(alvium, ctrl->val);
> > > > > > > 
> > > > > > > You're still missing grabbing the manual controls when the corresponding
> > > > > > > automatic control is enabled. I've commented on the same matter previously.
> > > > > > 
> > > > > > Same comment in [1]
> > > > > > 
> > > > > > > 
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_AUTO_WHITE_BALANCE:
> > > > > > > > +		ret = alvium_set_ctrl_white_balance(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_HUE:
> > > > > > > > +		ret = alvium_set_ctrl_hue(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_CONTRAST:
> > > > > > > > +		ret = alvium_set_ctrl_contrast(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_SATURATION:
> > > > > > > > +		ret = alvium_set_ctrl_saturation(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_GAMMA:
> > > > > > > > +		ret = alvium_set_ctrl_gamma(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_SHARPNESS:
> > > > > > > > +		ret = alvium_set_ctrl_sharpness(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_HFLIP:
> > > > > > > > +		ret = alvium_set_ctrl_hflip(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	case V4L2_CID_VFLIP:
> > > > > > > > +		ret = alvium_set_ctrl_vflip(alvium, ctrl->val);
> > > > > > > > +		break;
> > > > > > > > +	default:
> > > > > > > > +		ret = -EINVAL;
> > > > > > > > +		break;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	pm_runtime_put(&client->dev);
> > > > > > > > +
> > > > > > > > +	return ret;
> > > > > > > > +}
> > 
> > Here I plan to switch to:
> > 
> > static int alvium_s_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > 	struct alvium_dev *alvium = sd_to_alvium(sd);
> > 	struct i2c_client *client = v4l2_get_subdevdata(&alvium->sd);
> > 	int ret;
> > 
> > 	/*
> > 	 * Applying V4L2 control value only happens
> > 	 * when power is up for streaming
> > 	 */
> > 	if (!pm_runtime_get_if_in_use(&client->dev))
> > 		return 0;
> > 
> > 	switch (ctrl->id) {
> > 	case V4L2_CID_GAIN:
> > 		ret = alvium_set_ctrl_gain(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_AUTOGAIN:
> > 		ret = alvium_set_ctrl_auto_gain(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_EXPOSURE:
> > 		ret = alvium_set_ctrl_exposure(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_EXPOSURE_AUTO:
> > 		ret = alvium_set_ctrl_auto_exposure(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_RED_BALANCE:
> > 		ret = alvium_set_ctrl_red_balance_ratio(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_BLUE_BALANCE:
> > 		ret = alvium_set_ctrl_blue_balance_ratio(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_AUTO_WHITE_BALANCE:
> > 		ret = alvium_set_ctrl_awb(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_HUE:
> > 		ret = alvium_set_ctrl_hue(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_CONTRAST:
> > 		ret = alvium_set_ctrl_contrast(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_SATURATION:
> > 		ret = alvium_set_ctrl_saturation(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_GAMMA:
> > 		ret = alvium_set_ctrl_gamma(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_SHARPNESS:
> > 		ret = alvium_set_ctrl_sharpness(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_HFLIP:
> > 		ret = alvium_set_ctrl_hflip(alvium, ctrl->val);
> > 		break;
> > 	case V4L2_CID_VFLIP:
> > 		ret = alvium_set_ctrl_vflip(alvium, ctrl->val);
> > 		break;
> > 	default:
> > 		ret = -EINVAL;
> > 		break;
> > 	}
> > 
> > 	pm_runtime_put(&client->dev);
> > 
> > 	return ret;
> > }
> > 
> > Like you suggested.
> > I think is more clean/understandable.
> > Thanks for your comment.
> 
> Ack.
> 
> -- 
> Regards,
> 
> Sakari Ailus

Powered by blists - more mailing lists