Hi Sakari, thanks for your feedback, see below my replies. On 26/04/2018 10:13, Sakari Ailus wrote: > 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. Thanks. >> 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 ^ My typo here, it should be v4l2_subdev_pad_ops.set_fmt. >> existing applications? > > Hmm. The driver supports sub-device uAPI. Which bridge (or ISP) driver are > you using the sensor with? In my current setup I have an AXI-stream chain made of Xilinx IPs: imx274, demosaic, color correction matrix, rgb2yuv, framebuffer writer (=DMA). >> 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> Thanks for the link, that was useful. I'm now convinced COMPOSE makes sense on the _sink_ pads of a subdev. The last example with 2 sinks and 2 sources was particularly enlightening to me. The logical abstraction is that there's a "compose buffer" inside the subdev where 1+ sinks are composed, then 1+ source pads pick a sub-portion (crop) from this buffer and send it out. Of course the "compose buffer" might not exist physically, but the transfer function is the same. But I still fail to understand COMPOSE on a _source_ pad. I understand COMPOSE as: there's a "large" rectangle, but we write only a sub-rectangle of it, leaving the surrounding area unmodified. But in the stream between a sensor and the following subdev (e.g. demosaic) there is only a flow of pixels, no notion of "surrounding unmodified pixels". And no notione of "buffer", be it physical or not. Right? This understanding of mine seems supported by the 5-item list in the page you linked, where the parameters to set are listed as: 1. Sink pad format 2. Sink pad actual crop selection 3. Sink pad actual compose selection 4. Source pad actual crop selection 5. Source pad format And there is no "Source pad actual compose selection". But clearly there is still something I don't understand. Any further reading suggestion, or an existing sensor driver to look at, would be welcome. > And binning is effectively scaling. Sure. >>> 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, Thanks, -- Luca
