Hi Luca, On Tue, Apr 24, 2018 at 04:30:38PM +0200, Luca Ceresoli wrote: > 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. Ack. I'll apply 1--6 then. > > 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@xxxxxxxxxxxxxxxx> > >> > >> --- > >> 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? Hmm. The driver supports sub-device uAPI. Which bridge (or ISP) driver are you using the sensor with? > > 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? The COMPOSE target is also used for configuring scaling: <URL:https://hverkuil.home.xs4all.nl/spec/uapi/v4l/dev-subdev.html#selections-cropping-scaling-and-composition> And binning is effectively scaling. > > > 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, -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx
