Date:   Tue, 24 Apr 2018 16:30:38 +0200
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Sakari Ailus <sakari.ailus@....fi>
Cc:     linux-media@...r.kernel.org, Leon Luo <leonl@...pardimaging.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/13] media: imx274: add SELECTION support for
 cropping

Hi Sakari,

On 24/04/2018 11:59, Sakari Ailus wrote:
> Hi Luca,
> 
> Thank you for the patchset.
> 
> Some comments below... what I propose is that I apply the rest of the
> patches and then the comments to this one could be addressed separately.
> Would that work for you?

Yes, but I suggest you only apply patches 1-6. I noticed a warning in
patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.

I'll wait for the outcome of the discussion below before sending v3.
Tell me if you prefer me to do it earlier.

> On Tue, Apr 24, 2018 at 10:24:18AM +0200, Luca Ceresoli wrote:
>> Currently this driver does not support cropping. The supported modes
>> are the following, all capturing the entire area:
>>
>>  - 3840x2160, 1:1 binning (native sensor resolution)
>>  - 1920x1080, 2:1 binning
>>  - 1280x720,  3:1 binning
>>
>> The set_fmt callback chooses among these 3 configurations the one that
>> matches the requested format.
>>
>> Adding support to VIDIOC_SUBDEV_G_SELECTION and
>> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
>> which now chooses the most appropriate binning based on the ratio
>> between the previously-set cropping size and the format size being
>> requested.
>>
>> Note that the names in enum imx274_mode change from being
>> resolution-based to being binning-based. Without cropping, the two
>> naming are equivalent. With cropping, the resolution could be
>> different, e.g. using 2:1 binning mode to crop 1200x960 and output a
>> 600x480 format. Using binning in the names avoids anny
>> misunderstanding.
>>
>> Signed-off-by: Luca Ceresoli <luca@...aceresoli.net>
>>
>> ---
>> Changed v1 -> v2:
>>  - add "media: " prefix to commit message
>> ---
>>  drivers/media/i2c/imx274.c | 266 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 192 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index b6c54712f2aa..ceb5b3e498c6 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -19,6 +19,7 @@
>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#include <linux/kernel.h>
>>  #include <linux/clk.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio.h>
>> @@ -74,7 +75,7 @@
>>   */
>>  #define IMX274_MIN_EXPOSURE_TIME		(4 * 260 / 72)
>>  
>> -#define IMX274_DEFAULT_MODE			IMX274_MODE_3840X2160
>> +#define IMX274_DEFAULT_MODE			IMX274_MODE_BINNING_OFF
>>  #define IMX274_MAX_WIDTH			(3840)
>>  #define IMX274_MAX_HEIGHT			(2160)
>>  #define IMX274_MAX_FRAME_RATE			(120)
>> @@ -111,6 +112,20 @@
>>  #define IMX274_SHR_REG_LSB			0x300C /* SHR */
>>  #define IMX274_SVR_REG_MSB			0x300F /* SVR */
>>  #define IMX274_SVR_REG_LSB			0x300E /* SVR */
>> +#define IMX274_HTRIM_EN_REG			0x3037
>> +#define IMX274_HTRIM_START_REG_LSB		0x3038
>> +#define IMX274_HTRIM_START_REG_MSB		0x3039
>> +#define IMX274_HTRIM_END_REG_LSB		0x303A
>> +#define IMX274_HTRIM_END_REG_MSB		0x303B
>> +#define IMX274_VWIDCUTEN_REG			0x30DD
>> +#define IMX274_VWIDCUT_REG_LSB			0x30DE
>> +#define IMX274_VWIDCUT_REG_MSB			0x30DF
>> +#define IMX274_VWINPOS_REG_LSB			0x30E0
>> +#define IMX274_VWINPOS_REG_MSB			0x30E1
>> +#define IMX274_WRITE_VSIZE_REG_LSB		0x3130
>> +#define IMX274_WRITE_VSIZE_REG_MSB		0x3131
>> +#define IMX274_Y_OUT_SIZE_REG_LSB		0x3132
>> +#define IMX274_Y_OUT_SIZE_REG_MSB		0x3133
>>  #define IMX274_VMAX_REG_1			0x30FA /* VMAX, MSB */
>>  #define IMX274_VMAX_REG_2			0x30F9 /* VMAX */
>>  #define IMX274_VMAX_REG_3			0x30F8 /* VMAX, LSB */
>> @@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config = {
>>  };
>>  
>>  enum imx274_mode {
>> -	IMX274_MODE_3840X2160,
>> -	IMX274_MODE_1920X1080,
>> -	IMX274_MODE_1280X720,
>> +	IMX274_MODE_BINNING_OFF,
>> +	IMX274_MODE_BINNING_2_1,
>> +	IMX274_MODE_BINNING_3_1,
>>  };
>>  
>>  /*
>> @@ -215,31 +230,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] = {
>>  	{0x3004, 0x01},
>>  	{0x3005, 0x01},
>>  	{0x3006, 0x00},
>> -	{0x3007, 0x02},
>> +	{0x3007, 0xa2},
>>  
>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>  
>>  	{0x306B, 0x05},
>>  	{0x30E2, 0x01},
>> -	{0x30F6, 0x07}, /* HMAX, 263 */
>> -	{0x30F7, 0x01}, /* HMAX */
>> -
>> -	{0x30dd, 0x01}, /* crop to 2160 */
>> -	{0x30de, 0x06},
>> -	{0x30df, 0x00},
>> -	{0x30e0, 0x12},
>> -	{0x30e1, 0x00},
>> -	{0x3037, 0x01}, /* to crop to 3840 */
>> -	{0x3038, 0x0c},
>> -	{0x3039, 0x00},
>> -	{0x303a, 0x0c},
>> -	{0x303b, 0x0f},
>>  
>>  	{0x30EE, 0x01},
>> -	{0x3130, 0x86},
>> -	{0x3131, 0x08},
>> -	{0x3132, 0x7E},
>> -	{0x3133, 0x08},
>>  	{0x3342, 0x0A},
>>  	{0x3343, 0x00},
>>  	{0x3344, 0x16},
>> @@ -273,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>>  	{0x3004, 0x02},
>>  	{0x3005, 0x21},
>>  	{0x3006, 0x00},
>> -	{0x3007, 0x11},
>> +	{0x3007, 0xb1},
>>  
>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>  
>> @@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>>  	{0x30F6, 0x04}, /* HMAX, 260 */
>>  	{0x30F7, 0x01}, /* HMAX */
>>  
>> -	{0x30dd, 0x01}, /* to crop to 1920x1080 */
>> -	{0x30de, 0x05},
>> -	{0x30df, 0x00},
>> -	{0x30e0, 0x04},
>> -	{0x30e1, 0x00},
>> -	{0x3037, 0x01},
>> -	{0x3038, 0x0c},
>> -	{0x3039, 0x00},
>> -	{0x303a, 0x0c},
>> -	{0x303b, 0x0f},
>> -
>>  	{0x30EE, 0x01},
>> -	{0x3130, 0x4E},
>> -	{0x3131, 0x04},
>> -	{0x3132, 0x46},
>> -	{0x3133, 0x04},
>>  	{0x3342, 0x0A},
>>  	{0x3343, 0x00},
>>  	{0x3344, 0x1A},
>> @@ -331,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>>  	{0x3004, 0x03},
>>  	{0x3005, 0x31},
>>  	{0x3006, 0x00},
>> -	{0x3007, 0x09},
>> +	{0x3007, 0xa9},
>>  
>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>  
>> @@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>>  	{0x30F6, 0x04}, /* HMAX, 260 */
>>  	{0x30F7, 0x01}, /* HMAX */
>>  
>> -	{0x30DD, 0x01},
>> -	{0x30DE, 0x07},
>> -	{0x30DF, 0x00},
>> -	{0x40E0, 0x04},
>> -	{0x30E1, 0x00},
>> -	{0x3030, 0xD4},
>> -	{0x3031, 0x02},
>> -	{0x3032, 0xD0},
>> -	{0x3033, 0x02},
>> -
>>  	{0x30EE, 0x01},
>> -	{0x3130, 0xE2},
>> -	{0x3131, 0x02},
>> -	{0x3132, 0xDE},
>> -	{0x3133, 0x02},
>>  	{0x3342, 0x0A},
>>  	{0x3343, 0x00},
>>  	{0x3344, 0x1B},
>> @@ -561,6 +530,7 @@ struct stimx274 {
>>  	struct imx274_ctrls ctrls;
>>  	struct v4l2_mbus_framefmt format;
>>  	struct v4l2_fract frame_interval;
>> +	struct v4l2_rect crop;
>>  	struct regmap *regmap;
>>  	struct gpio_desc *reset_gpio;
>>  	struct mutex lock; /* mutex lock for operations */
>> @@ -901,36 +871,30 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>>  {
>>  	struct v4l2_mbus_framefmt *fmt = &format->format;
>>  	struct stimx274 *imx274 = to_imx274(sd);
>> -	struct i2c_client *client = imx274->client;
>> -	int index;
>> +	struct device *dev = &imx274->client->dev;
>> +	unsigned int ratio; /* Binning ratio */
>>  
>> -	dev_dbg(&client->dev,
>> -		"%s: width = %d height = %d code = %d\n",
>> -		__func__, fmt->width, fmt->height, fmt->code);
>> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
>> +		__func__, fmt->width, fmt->height,
>> +		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
>>  
>>  	mutex_lock(&imx274->lock);
>>  
>> -	for (index = 0; index < ARRAY_SIZE(imx274_formats); index++) {
>> -		if (imx274_formats[index].size.width == fmt->width &&
>> -		    imx274_formats[index].size.height == fmt->height)
>> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
>> +	for (ratio = 3; ratio > 1; ratio--)
>> +		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
>> +		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
>>  			break;
>> -	}
>> -
>> -	if (index >= ARRAY_SIZE(imx274_formats)) {
>> -		/* default to first format */
>> -		index = 0;
>> -	}
>>  
>> -	imx274->mode = &imx274_formats[index];
>> +	imx274->mode = &imx274_formats[ratio - 1];
>>  
>> -	if (fmt->width > IMX274_MAX_WIDTH)
>> -		fmt->width = IMX274_MAX_WIDTH;
>> -	if (fmt->height > IMX274_MAX_HEIGHT)
>> -		fmt->height = IMX274_MAX_HEIGHT;
>> -	fmt->width = fmt->width & (~IMX274_MASK_LSB_2_BITS);
>> -	fmt->height = fmt->height & (~IMX274_MASK_LSB_2_BITS);
>> +	fmt->width = imx274->crop.width / ratio;
>> +	fmt->height = imx274->crop.height / ratio;
>>  	fmt->field = V4L2_FIELD_NONE;
>>  
>> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
>> +		__func__, fmt->width, fmt->height, ratio);
>> +
>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>  		cfg->try_fmt = *fmt;
>>  	else
>> @@ -940,6 +904,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>>  	return 0;
>>  }
>>  
>> +static int imx274_get_selection(struct v4l2_subdev *sd,
>> +				struct v4l2_subdev_pad_config *cfg,
>> +				struct v4l2_subdev_selection *sel)
>> +{
>> +	struct stimx274 *imx274 = to_imx274(sd);
>> +
>> +	if (sel->pad != 0)
>> +		return -EINVAL;
>> +
>> +	switch (sel->target) {
>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>> +		sel->r.left = 0;
>> +		sel->r.top = 0;
>> +		sel->r.width = IMX274_MAX_WIDTH;
>> +		sel->r.height = IMX274_MAX_HEIGHT;
>> +		return 0;
>> +	case V4L2_SEL_TGT_CROP:
>> +		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>> +			mutex_lock(&imx274->lock);
>> +			sel->r = imx274->crop;
>> +			mutex_unlock(&imx274->lock);
>> +		} else {
>> +			sel->r = cfg->try_crop;
>> +		}
>> +		return 0;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int imx274_set_selection(struct v4l2_subdev *sd,
>> +				struct v4l2_subdev_pad_config *cfg,
>> +				struct v4l2_subdev_selection *sel)
>> +{
>> +	struct stimx274 *imx274 = to_imx274(sd);
>> +	struct device *dev = &imx274->client->dev;
>> +	struct v4l2_mbus_framefmt *tgt_fmt;
>> +	struct v4l2_rect *tgt_crop;
>> +	struct v4l2_rect new_crop;
>> +
> 
> A lot of sensor drivers use the format IOCTLs to configure the output size
> of the sensor and that's what it must be: the get_fmt() pad op has to
> return the format of the image produced by the sensor. The size set by the
> set_fmt() also must be obtainable using get_fmt().
> 
> What I propose is to move also the binning configuration to use selections,
> i.e. the COMPOSE selection target. The NATIVE_SIZE target is the sensor's
> native size, the largest that can be accessed in its pixel array.
> 
> The size in format related IOCTLs is thus derived from the NATIVE_SIZE,
> CROP and COMPOSE configuration, and is no longer settable directly.

I'm OK with making any improvement, but I'm afraid I'm not understanding
what you mean. Do you mean we should stop responding to the VIDIOC_S_FMT
ioctl, aka v4l2_subdev_pad_ops.set_selection? Wouldn't this break
existing applications?

Also the meaning of COMPOSE in the context of a sensor subdev is unclear
to me. For a video node (which is typically paired with a DMA) it makes
sense: the DMA can write a sub-area of the destination memory buffer.
But the sensor subdev is the first element of a possibly long processing
chain, and as such it only produces a stream. The sensor does not know
what a "memory buffer" is.

What's wrong with my understanding?

> Btw. the crop here, is that taking place after binning as it would seem
> like? Then, I presume it is digital cropping, and the CROP rectangle is
> related to the COMPOSE rectangle (binning configuration).

Logically speaking, cropping here happens before binning, i.e. the crop
rectangle always refers to NATIVE_SIZE. The sensor just skips lines and
columns outside of that rectangle and optionally bins what's inside.

Examples of how it is currently implemented:
- native 3840x2160, crop 3840x2160, fmt 3840x2160 = don't crop,  1:1 bin
- native 3840x2160, crop 1920x1080, fmt 1920x1080 = crop to 50%, 1:1 bin
- native 3840x2160, crop 3840x2160, fmt 1920x1080 = don't crop,  2:1 bin
- native 3840x2160, crop 1920x1080, fmt  960x 540 = crop to 50%, 2:1 bin

Can you provide similar examples of how it would work when setting CROP
+ COMPOSE, and without setting the format?

Regards,
-- 
Luca

Powered by blists - more mailing lists