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 <gagal...@gmail.com>
>> Co-developed-by: Danilo Figueiredo Rocha <drocha.figueir...@gmail.com>
>> Signed-off-by: Danilo Figueiredo Rocha <drocha.figueir...@gmail.com>
>>
>> ---
>>
>> 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;
>> }
>>
