On 9/25/19 8:15 PM, Helen Koike wrote: > +Hans +Shuah > > Hi Guilherme and Danilo, > > Thank you for the patch, please see my comments below. > > On 9/9/19 1:08 AM, Guilherme Alcarde Gallo wrote: >> Add support for the scaler subdevice to respond VIDIOC_G_SELECTION and >> VIDIOC_S_SELECTION ioctls with the following targets: >> V4L2_SEL_TGT_COMPOSE_BOUNDS and V4L2_SEL_TGT_CROP. >> >> * Added new const struct crop_rect_default to initialize subdev scaler >> properly. >> * Make changes in sink pad format reflect the crop rectangle. E.g. >> changing the frame format to a smaller size one can make the former >> crop rectangle selects a non existing frame area. To solve this >> situation the crop rectangle is clamped to the frame boundaries. >> * Clamp crop rectangle respecting the sink bounds during set_selection >> ioctl. >> >> Signed-off-by: Guilherme Alcarde Gallo <gagallo7@xxxxxxxxx> >> Co-developed-by: Danilo Figueiredo Rocha <drocha.figueiredo@xxxxxxxxx> >> Signed-off-by: Danilo Figueiredo Rocha <drocha.figueiredo@xxxxxxxxx> >> >> --- >> >> This patch is based on the monolithic vimc driver from the patchset >> named "Collapse vimc into single monolithic driver" >> https://patchwork.kernel.org/patch/11136201/ >> >> --- >> >> drivers/media/platform/vimc/vimc-scaler.c | 148 +++++++++++++++++++--- >> 1 file changed, 133 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >> index a5a0855ad9cd..b50d11e76a2b 100644 >> --- a/drivers/media/platform/vimc/vimc-scaler.c >> +++ b/drivers/media/platform/vimc/vimc-scaler.c >> @@ -18,6 +18,9 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier"); >> >> #define MAX_ZOOM 8 >> >> +#define VIMC_SCA_FMT_WIDTH_DEFAULT 640 >> +#define VIMC_SCA_FMT_HEIGHT_DEFAULT 480 >> + >> struct vimc_sca_device { >> struct vimc_ent_device ved; >> struct v4l2_subdev sd; >> @@ -26,6 +29,7 @@ struct vimc_sca_device { >> * with the width and hight multiplied by mult >> */ >> struct v4l2_mbus_framefmt sink_fmt; >> + struct v4l2_rect crop_rect; >> /* Values calculated when the stream starts */ >> u8 *src_frame; >> unsigned int src_line_size; >> @@ -33,22 +37,33 @@ struct vimc_sca_device { >> }; >> >> static const struct v4l2_mbus_framefmt sink_fmt_default = { >> - .width = 640, >> - .height = 480, >> + .width = VIMC_SCA_FMT_WIDTH_DEFAULT, >> + .height = VIMC_SCA_FMT_HEIGHT_DEFAULT, >> .code = MEDIA_BUS_FMT_RGB888_1X24, >> .field = V4L2_FIELD_NONE, >> .colorspace = V4L2_COLORSPACE_DEFAULT, >> }; >> >> +static const struct v4l2_rect crop_rect_default = { >> + .width = VIMC_SCA_FMT_WIDTH_DEFAULT, >> + .height = VIMC_SCA_FMT_HEIGHT_DEFAULT, >> + .top = 0, >> + .left = 0, >> +}; >> + >> static int vimc_sca_init_cfg(struct v4l2_subdev *sd, >> struct v4l2_subdev_pad_config *cfg) >> { >> struct v4l2_mbus_framefmt *mf; >> + struct v4l2_rect *r; >> unsigned int i; >> >> mf = v4l2_subdev_get_try_format(sd, cfg, 0); >> *mf = sink_fmt_default; >> >> + r = v4l2_subdev_get_try_crop(sd, cfg, 0); >> + *r = crop_rect_default; >> + >> for (i = 1; i < sd->entity.num_pads; i++) { >> mf = v4l2_subdev_get_try_format(sd, cfg, i); >> *mf = sink_fmt_default; >> @@ -107,16 +122,21 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd, >> struct v4l2_subdev_format *format) >> { >> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd); >> + struct v4l2_rect *crop_rect; >> >> /* Get the current sink format */ >> - format->format = (format->which == V4L2_SUBDEV_FORMAT_TRY) ? >> - *v4l2_subdev_get_try_format(sd, cfg, 0) : >> - vsca->sink_fmt; >> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { >> + format->format = *v4l2_subdev_get_try_format(sd, cfg, 0); >> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0); >> + } else { >> + format->format = vsca->sink_fmt; >> + crop_rect = &vsca->crop_rect; >> + } >> >> /* Scale the frame size for the source pad */ >> if (VIMC_IS_SRC(format->pad)) { >> - format->format.width = vsca->sink_fmt.width * sca_mult; >> - format->format.height = vsca->sink_fmt.height * sca_mult; >> + format->format.width = crop_rect->width * sca_mult; >> + format->format.height = crop_rect->height * sca_mult; >> } >> >> return 0; >> @@ -148,6 +168,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd, >> { >> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd); >> struct v4l2_mbus_framefmt *sink_fmt; >> + struct v4l2_rect *crop_rect; >> >> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { >> /* Do not change the format while stream is on */ >> @@ -155,8 +176,10 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd, >> return -EBUSY; >> >> sink_fmt = &vsca->sink_fmt; >> + crop_rect = &vsca->crop_rect; >> } else { >> sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); >> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0); >> } >> >> /* >> @@ -165,12 +188,20 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd, >> */ >> if (VIMC_IS_SRC(fmt->pad)) { >> fmt->format = *sink_fmt; >> - fmt->format.width = sink_fmt->width * sca_mult; >> - fmt->format.height = sink_fmt->height * sca_mult; >> + fmt->format.width = crop_rect->width * sca_mult; >> + fmt->format.height = crop_rect->height * sca_mult; >> } else { >> /* Set the new format in the sink pad */ >> vimc_sca_adjust_sink_fmt(&fmt->format); >> >> + crop_rect->width = clamp_t(u32, crop_rect->width, >> + VIMC_FRAME_MIN_WIDTH, >> + fmt->format.width) & ~1; >> + >> + crop_rect->height = clamp_t(u32, crop_rect->height, >> + VIMC_FRAME_MIN_HEIGHT, >> + fmt->format.height) & ~1; >> + > > You need to consider top and left of the crop rectangle to make this clamp. > > Lets say you have a 300x300 image and a crop rectangle to top=100,left=100,width=100,height=100. > Then, if you set the image to 150x150, the crop rectangle should adjust to top=100,left=100,width=50,height=50. > > There is also the case if you set the sink image to 50x50, now the crop rectangle is outside the image, > I'm not sure what should happen in this case, maybe you should just reset the crop rectangle to the image format. > > Hans, could you confirm what should be done here? When you want to map an existing crop rectangle to a new, smaller image format, then you try to keep the crop width and height. So in the example above (changing the image to 150x150) the new crop would be top=50,left=50,width=100,height=100. Use the functions in media/v4l2-rect.h for this (esp. v4l2_rect_map_inside). If you'd change the image size to 50x50, then the new crop would automatically become top=0,left=0,width=50,height=50. In other words, this does exactly what you want and always keeps a valid configuration. Regards, Hans > >> dev_dbg(vsca->dev, "%s: sink format update: " >> "old:%dx%d (0x%x, %d, %d, %d, %d) " >> "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vsca->sd.name, >> @@ -189,12 +220,91 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd, >> return 0; >> } >> >> +static int vimc_sca_get_selection(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_selection *sel) >> +{ >> + struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd); >> + >> + if (sel->which == V4L2_SUBDEV_FORMAT_TRY) >> + return -EINVAL; > > I think you could implement TRY already, you just need to use v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_format() > similar to what set/get format do. > >> + >> + if (vsca->src_frame) >> + return -EBUSY; >> + >> + if (VIMC_IS_SRC(sel->pad)) >> + return -EINVAL; >> + >> + switch (sel->target) { >> + case V4L2_SEL_TGT_CROP: >> + sel->r = vsca->crop_rect; >> + break; >> + case V4L2_SEL_TGT_CROP_BOUNDS: >> + sel->r.left = 0; >> + sel->r.top = 0; >> + sel->r.width = vsca->sink_fmt.width; >> + sel->r.height = vsca->sink_fmt.height; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int vimc_sca_set_selection(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_selection *sel) >> +{ >> + struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd); >> + struct v4l2_rect *vsca_crop_rect = &vsca->crop_rect; >> + struct v4l2_subdev_selection bound_sel = *sel; >> + int ret = 0; >> + >> + if (sel->which == V4L2_SUBDEV_FORMAT_TRY) >> + return -EINVAL; > > Same thing here. > >> + >> + /* Do not change the format while stream is on */ >> + if (vsca->src_frame) >> + return -EBUSY; >> + >> + if (VIMC_IS_SRC(sel->pad)) >> + return -EINVAL; >> + >> + switch (sel->target) { >> + case V4L2_SEL_TGT_CROP: >> + /* Get the crop bounds to clamp the crop rectangle correctly */ >> + bound_sel.target = V4L2_SEL_TGT_CROP_BOUNDS; >> + ret = vimc_sca_get_selection(sd, cfg, &bound_sel); >> + if (ret) { >> + pr_err("Error during call to vimc_sca_get_selection."); >> + return ret; >> + } >> + >> + sel->r.width = clamp_t(u32, sel->r.width, VIMC_FRAME_MIN_WIDTH, >> + bound_sel.r.width) & ~1; >> + sel->r.height = clamp_t(u32, sel->r.height, >> + VIMC_FRAME_MIN_HEIGHT, >> + bound_sel.r.height); > > You should also consider top/left to make this clamp. > top and left should also be ajusted if they are set outside of the boundaries. > >> + *vsca_crop_rect = sel->r; >> + break; >> + case V4L2_SEL_TGT_CROP_BOUNDS: >> + return -EINVAL; >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = { >> .init_cfg = vimc_sca_init_cfg, >> .enum_mbus_code = vimc_sca_enum_mbus_code, >> .enum_frame_size = vimc_sca_enum_frame_size, >> .get_fmt = vimc_sca_get_fmt, >> .set_fmt = vimc_sca_set_fmt, >> + .get_selection = vimc_sca_get_selection, >> + .set_selection = vimc_sca_set_selection, >> }; >> >> static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable) >> @@ -213,11 +323,11 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable) >> vsca->bpp = vpix->bpp; >> >> /* Calculate the width in bytes of the src frame */ >> - vsca->src_line_size = vsca->sink_fmt.width * >> + vsca->src_line_size = vsca->crop_rect.width * >> sca_mult * vsca->bpp; >> >> /* Calculate the frame size of the source pad */ >> - frame_size = vsca->src_line_size * vsca->sink_fmt.height * >> + frame_size = vsca->src_line_size * vsca->crop_rect.height * >> sca_mult; >> >> /* Allocate the frame buffer. Use vmalloc to be able to >> @@ -259,11 +369,12 @@ static void vimc_sca_fill_pix(u8 *const ptr, >> } >> >> static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca, >> - const unsigned int lin, const unsigned int col, >> + unsigned int lin, unsigned int col, >> const u8 *const sink_frame) >> { >> unsigned int i, j, index; >> const u8 *pixel; >> + const struct v4l2_rect crop_rect = vsca->crop_rect; >> >> /* Point to the pixel value in position (lin, col) in the sink frame */ >> index = VIMC_FRAME_INDEX(lin, col, >> @@ -278,8 +389,10 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca, >> /* point to the place we are going to put the first pixel >> * in the scaled src frame >> */ >> + lin -= crop_rect.top; >> + col -= crop_rect.left; >> index = VIMC_FRAME_INDEX(lin * sca_mult, col * sca_mult, >> - vsca->sink_fmt.width * sca_mult, vsca->bpp); >> + crop_rect.width * sca_mult, vsca->bpp); >> >> dev_dbg(vsca->dev, "sca: %s: scale_pix src pos %dx%d, index %d\n", >> vsca->sd.name, lin * sca_mult, col * sca_mult, index); >> @@ -309,10 +422,12 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca, >> { >> unsigned int i, j; >> > > Please remove this extra line. > > Thanks :) > Helen > >> + const struct v4l2_rect r = vsca->crop_rect; >> + >> /* Scale each pixel from the original sink frame */ >> /* TODO: implement scale down, only scale up is supported for now */ >> - for (i = 0; i < vsca->sink_fmt.height; i++) >> - for (j = 0; j < vsca->sink_fmt.width; j++) >> + for (i = r.top; i < r.top + r.height; i++) >> + for (j = r.left; j < r.left + r.width; j++) >> vimc_sca_scale_pix(vsca, i, j, sink_frame); >> } >> >> @@ -382,5 +497,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >> /* Initialize the frame format */ >> vsca->sink_fmt = sink_fmt_default; >> >> + /* Initialize the crop selection */ >> + vsca->crop_rect = crop_rect_default; >> + >> return &vsca->ved; >> } >>
