* [PATCH v3] media: vimc: Implement get/set selection in sink
@ 2019-11-06 1:12 Danilo Figueiredo Rocha
2019-11-06 13:59 ` Helen Koike
0 siblings, 1 reply; 4+ messages in thread
From: Danilo Figueiredo Rocha @ 2019-11-06 1:12 UTC (permalink / raw)
To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media,
linux-kernel, hverkuil-cisco, lkcamp
From: Guilherme Alcarde Gallo <gagallo7@gmail.com>
Add support for the sink pad of 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.
* Add new const struct crop_rect_default to initialize subdev scaler
properly.
* Make changes in sink pad format reflect to 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@gmail.com>
Co-developed-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
Signed-off-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
---
Changes in V3:
* Sort the headers in alphabetical order.
* Change all functions prefix to 'vimc_sca_'.
* Remove useless case.
* Change commit message subject.
Changes in V2:
* Use v4l2_rect_map_inside to clamp the crop rectangle.
* Do the crop rectangle clamping after changing sink format.
* Implement try ioctls for selection with CROP* targets.
* Treat tiny rectangles with area smaller than minimal dimensions of vimc
frames.
* Allow user to get selection when the streaming is on.
* Reuse bound rectangle generation in a static function.
* Disable get_selection for Source pads as we did before with
set_selection.
---
drivers/media/platform/vimc/vimc-scaler.c | 170 ++++++++++++++++++++--
1 file changed, 154 insertions(+), 16 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 2f88a7d9d67b..935bb0e0d0ee 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -6,8 +6,9 @@
*/
#include <linux/moduleparam.h>
-#include <linux/vmalloc.h>
#include <linux/v4l2-mediabus.h>
+#include <linux/vmalloc.h>
+#include <media/v4l2-rect.h>
#include <media/v4l2-subdev.h>
#include "vimc-common.h"
@@ -18,6 +19,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;
@@ -25,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 +38,64 @@ 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 const struct v4l2_rect crop_rect_min = {
+ .width = VIMC_FRAME_MIN_WIDTH,
+ .height = VIMC_FRAME_MIN_HEIGHT,
+ .top = 0,
+ .left = 0,
+};
+
+static struct v4l2_rect
+vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
+{
+ /* Get the crop bounds to clamp the crop rectangle correctly */
+ struct v4l2_rect r = {
+ .left = 0,
+ .top = 0,
+ .width = sink_fmt->width,
+ .height = sink_fmt->height,
+ };
+ return r;
+}
+
+static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
+ const struct v4l2_mbus_framefmt *sink_fmt)
+{
+ const struct v4l2_rect sink_rect =
+ vimc_sca_get_crop_bound_sink(sink_fmt);
+
+ /* Disallow rectangles smaller than the minimal one. */
+ v4l2_rect_set_min_size(r, &crop_rect_min);
+ v4l2_rect_map_inside(r, &sink_rect);
+}
+
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 +154,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 +200,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 +208,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,8 +220,8 @@ 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);
@@ -184,6 +239,80 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
fmt->format.xfer_func, fmt->format.ycbcr_enc);
*sink_fmt = fmt->format;
+
+ /* Do the crop, but respect the current bounds */
+ vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
+ }
+
+ 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);
+ struct v4l2_mbus_framefmt *sink_fmt;
+ struct v4l2_rect *crop_rect;
+
+ if (VIMC_IS_SRC(sel->pad))
+ return -EINVAL;
+
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ 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);
+ }
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ sel->r = *crop_rect;
+ break;
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
+ 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 *crop_rect;
+ struct v4l2_rect sink_rect;
+ struct v4l2_mbus_framefmt *sink_fmt;
+
+ if (VIMC_IS_SRC(sel->pad))
+ return -EINVAL;
+
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ /* Do not change the format while stream is on */
+ if (vsca->src_frame)
+ return -EBUSY;
+
+ crop_rect = &vsca->crop_rect;
+ sink_fmt = &vsca->sink_fmt;
+ } else {
+ crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
+ sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+ }
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ /* Do the crop, but respect the current bounds */
+ sink_rect = vimc_sca_get_crop_bound_sink(sink_fmt);
+ vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
+ *crop_rect = sel->r;
+ break;
+ default:
+ return -EINVAL;
}
return 0;
@@ -195,6 +324,8 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
.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 +344,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 +390,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 +410,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->ved.dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
vsca->sd.name, lin * sca_mult, col * sca_mult, index);
@@ -308,11 +442,12 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
const u8 *const sink_frame)
{
unsigned int i, j;
+ 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);
}
@@ -384,5 +519,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;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] media: vimc: Implement get/set selection in sink
2019-11-06 1:12 [PATCH v3] media: vimc: Implement get/set selection in sink Danilo Figueiredo Rocha
@ 2019-11-06 13:59 ` Helen Koike
2019-11-06 14:58 ` [Lkcamp] " Guilherme Alcarde Gallo
0 siblings, 1 reply; 4+ messages in thread
From: Helen Koike @ 2019-11-06 13:59 UTC (permalink / raw)
To: Danilo Figueiredo Rocha, Shuah Khan, Mauro Carvalho Chehab,
linux-media, linux-kernel, hverkuil-cisco, lkcamp,
Kieran Bingham, Jacopo Mondi, Laurent Pinchart,
Niklas Söderlund, Gabriela Bittencourt, pedro
Hi Danilo and Guilherme,
On 11/5/19 11:12 PM, Danilo Figueiredo Rocha wrote:
> From: Guilherme Alcarde Gallo <gagallo7@gmail.com>
>
> Add support for the sink pad of 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.
>
> * Add new const struct crop_rect_default to initialize subdev scaler
> properly.
> * Make changes in sink pad format reflect to 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@gmail.com>
> Co-developed-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
> Signed-off-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
Thanks for the patch, lgtm, just minor nitpicking comments below.
Also, I'm adding libcamera people in CC, as this might require updating it.
And I'm also adding Gabriela and Pedro in CC, as this might require some
changes in patch "media: vimc: Enable set resolution at the scaler src pad",
which removes the hardcoded scaling factor.
>
> ---
> Changes in V3:
> * Sort the headers in alphabetical order.
> * Change all functions prefix to 'vimc_sca_'.
> * Remove useless case.
> * Change commit message subject.
>
> Changes in V2:
> * Use v4l2_rect_map_inside to clamp the crop rectangle.
> * Do the crop rectangle clamping after changing sink format.
> * Implement try ioctls for selection with CROP* targets.
> * Treat tiny rectangles with area smaller than minimal dimensions of vimc
> frames.
> * Allow user to get selection when the streaming is on.
> * Reuse bound rectangle generation in a static function.
> * Disable get_selection for Source pads as we did before with
> set_selection.
> ---
> drivers/media/platform/vimc/vimc-scaler.c | 170 ++++++++++++++++++++--
> 1 file changed, 154 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 2f88a7d9d67b..935bb0e0d0ee 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -6,8 +6,9 @@
> */
>
> #include <linux/moduleparam.h>
> -#include <linux/vmalloc.h>
> #include <linux/v4l2-mediabus.h>
> +#include <linux/vmalloc.h>
I think you moved this one by accident?
Please, send v4 fixing this.
> +#include <media/v4l2-rect.h>
> #include <media/v4l2-subdev.h>
>
> #include "vimc-common.h"
> @@ -18,6 +19,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;
> @@ -25,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 +38,64 @@ 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 const struct v4l2_rect crop_rect_min = {
> + .width = VIMC_FRAME_MIN_WIDTH,
> + .height = VIMC_FRAME_MIN_HEIGHT,
> + .top = 0,
> + .left = 0,
> +};
> +
> +static struct v4l2_rect
> +vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
> +{
> + /* Get the crop bounds to clamp the crop rectangle correctly */
> + struct v4l2_rect r = {
> + .left = 0,
> + .top = 0,
> + .width = sink_fmt->width,
> + .height = sink_fmt->height,
> + };
> + return r;
> +}
> +
> +static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
> + const struct v4l2_mbus_framefmt *sink_fmt)
> +{
> + const struct v4l2_rect sink_rect =
> + vimc_sca_get_crop_bound_sink(sink_fmt);
> +
> + /* Disallow rectangles smaller than the minimal one. */
> + v4l2_rect_set_min_size(r, &crop_rect_min);
> + v4l2_rect_map_inside(r, &sink_rect);
> +}
> +
> 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 +154,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 +200,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 +208,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,8 +220,8 @@ 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);
> @@ -184,6 +239,80 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> fmt->format.xfer_func, fmt->format.ycbcr_enc);
>
> *sink_fmt = fmt->format;
> +
> + /* Do the crop, but respect the current bounds */
> + vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
> + }
> +
> + 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);
> + struct v4l2_mbus_framefmt *sink_fmt;
> + struct v4l2_rect *crop_rect;
> +
> + if (VIMC_IS_SRC(sel->pad))
> + return -EINVAL;
> +
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + 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);
> + }
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + sel->r = *crop_rect;
> + break;
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
> + 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 *crop_rect;
> + struct v4l2_rect sink_rect;
> + struct v4l2_mbus_framefmt *sink_fmt;
Do you mind declaring this just after vsca? I was trying to put bigger lines first in the declaration order.
> +
> + if (VIMC_IS_SRC(sel->pad))
> + return -EINVAL;
> +
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + /* Do not change the format while stream is on */
> + if (vsca->src_frame)
> + return -EBUSY;
> +
> + crop_rect = &vsca->crop_rect;
> + sink_fmt = &vsca->sink_fmt;
> + } else {
> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> + }
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + /* Do the crop, but respect the current bounds */
> + sink_rect = vimc_sca_get_crop_bound_sink(sink_fmt);
> + vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
> + *crop_rect = sel->r;
> + break;
> + default:
> + return -EINVAL;
> }
>
> return 0;
> @@ -195,6 +324,8 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
> .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 +344,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 +390,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;
Same here.
>
> /* Point to the pixel value in position (lin, col) in the sink frame */
> index = VIMC_FRAME_INDEX(lin, col,
> @@ -278,8 +410,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->ved.dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
> vsca->sd.name, lin * sca_mult, col * sca_mult, index);
> @@ -308,11 +442,12 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
> const u8 *const sink_frame)
> {
> unsigned int i, j;
> + const struct v4l2_rect r = vsca->crop_rect;
Same here.
>
> /* 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);
> }
>
> @@ -384,5 +519,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;
> }
>
With these changes:
Acked-by: Helen Koike <helen.koike@collabora.com>
Thanks
Helen
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Lkcamp] [PATCH v3] media: vimc: Implement get/set selection in sink
2019-11-06 13:59 ` Helen Koike
@ 2019-11-06 14:58 ` Guilherme Alcarde Gallo
2019-11-06 19:01 ` Helen Koike
0 siblings, 1 reply; 4+ messages in thread
From: Guilherme Alcarde Gallo @ 2019-11-06 14:58 UTC (permalink / raw)
To: Helen Koike
Cc: Danilo Figueiredo Rocha, Shuah Khan, Mauro Carvalho Chehab,
linux-media, linux-kernel, hverkuil-cisco, lkcamp,
Kieran Bingham, Jacopo Mondi, Laurent Pinchart,
Niklas Söderlund, Gabriela Bittencourt, pedro
Hi Helen,
Thanks for the prompt review!
Just one comment below.
On Wed, Nov 6, 2019 at 11:01 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Danilo and Guilherme,
>
> On 11/5/19 11:12 PM, Danilo Figueiredo Rocha wrote:
> > From: Guilherme Alcarde Gallo <gagallo7@gmail.com>
> >
> > Add support for the sink pad of 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.
> >
> > * Add new const struct crop_rect_default to initialize subdev scaler
> > properly.
> > * Make changes in sink pad format reflect to 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@gmail.com>
> > Co-developed-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
> > Signed-off-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
>
> Thanks for the patch, lgtm, just minor nitpicking comments below.
>
> Also, I'm adding libcamera people in CC, as this might require updating it.
>
> And I'm also adding Gabriela and Pedro in CC, as this might require some
> changes in patch "media: vimc: Enable set resolution at the scaler src pad",
> which removes the hardcoded scaling factor.
>
> >
> > ---
> > Changes in V3:
> > * Sort the headers in alphabetical order.
> > * Change all functions prefix to 'vimc_sca_'.
> > * Remove useless case.
> > * Change commit message subject.
> >
> > Changes in V2:
> > * Use v4l2_rect_map_inside to clamp the crop rectangle.
> > * Do the crop rectangle clamping after changing sink format.
> > * Implement try ioctls for selection with CROP* targets.
> > * Treat tiny rectangles with area smaller than minimal dimensions of vimc
> > frames.
> > * Allow user to get selection when the streaming is on.
> > * Reuse bound rectangle generation in a static function.
> > * Disable get_selection for Source pads as we did before with
> > set_selection.
> > ---
> > drivers/media/platform/vimc/vimc-scaler.c | 170 ++++++++++++++++++++--
> > 1 file changed, 154 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> > index 2f88a7d9d67b..935bb0e0d0ee 100644
> > --- a/drivers/media/platform/vimc/vimc-scaler.c
> > +++ b/drivers/media/platform/vimc/vimc-scaler.c
> > @@ -6,8 +6,9 @@
> > */
> >
> > #include <linux/moduleparam.h>
> > -#include <linux/vmalloc.h>
> > #include <linux/v4l2-mediabus.h>
> > +#include <linux/vmalloc.h>
>
> I think you moved this one by accident?
> Please, send v4 fixing this.
Actually we sorted the include headers as you requested in the last review.
Is there any rule about how to sort the headers?
>
> > +#include <media/v4l2-rect.h>
> > #include <media/v4l2-subdev.h>
> >
> > #include "vimc-common.h"
> > @@ -18,6 +19,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;
> > @@ -25,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 +38,64 @@ 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 const struct v4l2_rect crop_rect_min = {
> > + .width = VIMC_FRAME_MIN_WIDTH,
> > + .height = VIMC_FRAME_MIN_HEIGHT,
> > + .top = 0,
> > + .left = 0,
> > +};
> > +
> > +static struct v4l2_rect
> > +vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
> > +{
> > + /* Get the crop bounds to clamp the crop rectangle correctly */
> > + struct v4l2_rect r = {
> > + .left = 0,
> > + .top = 0,
> > + .width = sink_fmt->width,
> > + .height = sink_fmt->height,
> > + };
> > + return r;
> > +}
> > +
> > +static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
> > + const struct v4l2_mbus_framefmt *sink_fmt)
> > +{
> > + const struct v4l2_rect sink_rect =
> > + vimc_sca_get_crop_bound_sink(sink_fmt);
> > +
> > + /* Disallow rectangles smaller than the minimal one. */
> > + v4l2_rect_set_min_size(r, &crop_rect_min);
> > + v4l2_rect_map_inside(r, &sink_rect);
> > +}
> > +
> > 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 +154,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 +200,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 +208,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,8 +220,8 @@ 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);
> > @@ -184,6 +239,80 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> > fmt->format.xfer_func, fmt->format.ycbcr_enc);
> >
> > *sink_fmt = fmt->format;
> > +
> > + /* Do the crop, but respect the current bounds */
> > + vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
> > + }
> > +
> > + 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);
> > + struct v4l2_mbus_framefmt *sink_fmt;
> > + struct v4l2_rect *crop_rect;
> > +
> > + if (VIMC_IS_SRC(sel->pad))
> > + return -EINVAL;
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + 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);
> > + }
> > +
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + sel->r = *crop_rect;
> > + break;
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
> > + 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 *crop_rect;
> > + struct v4l2_rect sink_rect;
> > + struct v4l2_mbus_framefmt *sink_fmt;
>
> Do you mind declaring this just after vsca? I was trying to put bigger lines first in the declaration order.
>
> > +
> > + if (VIMC_IS_SRC(sel->pad))
> > + return -EINVAL;
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + /* Do not change the format while stream is on */
> > + if (vsca->src_frame)
> > + return -EBUSY;
> > +
> > + crop_rect = &vsca->crop_rect;
> > + sink_fmt = &vsca->sink_fmt;
> > + } else {
> > + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
> > + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> > + }
> > +
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + /* Do the crop, but respect the current bounds */
> > + sink_rect = vimc_sca_get_crop_bound_sink(sink_fmt);
> > + vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
> > + *crop_rect = sel->r;
> > + break;
> > + default:
> > + return -EINVAL;
> > }
> >
> > return 0;
> > @@ -195,6 +324,8 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
> > .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 +344,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 +390,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;
>
> Same here.
>
> >
> > /* Point to the pixel value in position (lin, col) in the sink frame */
> > index = VIMC_FRAME_INDEX(lin, col,
> > @@ -278,8 +410,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->ved.dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
> > vsca->sd.name, lin * sca_mult, col * sca_mult, index);
> > @@ -308,11 +442,12 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
> > const u8 *const sink_frame)
> > {
> > unsigned int i, j;
> > + const struct v4l2_rect r = vsca->crop_rect;
>
> Same here.
>
> >
> > /* 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);
> > }
> >
> > @@ -384,5 +519,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;
> > }
> >
>
> With these changes:
>
> Acked-by: Helen Koike <helen.koike@collabora.com>
>
> Thanks
> Helen
>
> _______________________________________________
> Lkcamp mailing list
> Lkcamp@lists.libreplanetbr.org
> https://lists.libreplanetbr.org/mailman/listinfo/lkcamp
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Lkcamp] [PATCH v3] media: vimc: Implement get/set selection in sink
2019-11-06 14:58 ` [Lkcamp] " Guilherme Alcarde Gallo
@ 2019-11-06 19:01 ` Helen Koike
0 siblings, 0 replies; 4+ messages in thread
From: Helen Koike @ 2019-11-06 19:01 UTC (permalink / raw)
To: Guilherme Alcarde Gallo
Cc: Danilo Figueiredo Rocha, Shuah Khan, Mauro Carvalho Chehab,
linux-media, linux-kernel, hverkuil-cisco, lkcamp,
Kieran Bingham, Jacopo Mondi, Laurent Pinchart,
Niklas Söderlund, Gabriela Bittencourt, pedro
On 11/6/19 12:58 PM, Guilherme Alcarde Gallo wrote:
> Hi Helen,
> Thanks for the prompt review!
>
> Just one comment below.
>
> On Wed, Nov 6, 2019 at 11:01 AM Helen Koike <helen.koike@collabora.com> wrote:
>>
>> Hi Danilo and Guilherme,
>>
>> On 11/5/19 11:12 PM, Danilo Figueiredo Rocha wrote:
>>> From: Guilherme Alcarde Gallo <gagallo7@gmail.com>
>>>
>>> Add support for the sink pad of 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.
>>>
>>> * Add new const struct crop_rect_default to initialize subdev scaler
>>> properly.
>>> * Make changes in sink pad format reflect to 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@gmail.com>
>>> Co-developed-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
>>> Signed-off-by: Danilo Figueiredo Rocha <drocha.figueiredo@gmail.com>
>>
>> Thanks for the patch, lgtm, just minor nitpicking comments below.
>>
>> Also, I'm adding libcamera people in CC, as this might require updating it.
>>
>> And I'm also adding Gabriela and Pedro in CC, as this might require some
>> changes in patch "media: vimc: Enable set resolution at the scaler src pad",
>> which removes the hardcoded scaling factor.
>>
>>>
>>> ---
>>> Changes in V3:
>>> * Sort the headers in alphabetical order.
>>> * Change all functions prefix to 'vimc_sca_'.
>>> * Remove useless case.
>>> * Change commit message subject.
>>>
>>> Changes in V2:
>>> * Use v4l2_rect_map_inside to clamp the crop rectangle.
>>> * Do the crop rectangle clamping after changing sink format.
>>> * Implement try ioctls for selection with CROP* targets.
>>> * Treat tiny rectangles with area smaller than minimal dimensions of vimc
>>> frames.
>>> * Allow user to get selection when the streaming is on.
>>> * Reuse bound rectangle generation in a static function.
>>> * Disable get_selection for Source pads as we did before with
>>> set_selection.
>>> ---
>>> drivers/media/platform/vimc/vimc-scaler.c | 170 ++++++++++++++++++++--
>>> 1 file changed, 154 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>>> index 2f88a7d9d67b..935bb0e0d0ee 100644
>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>>> @@ -6,8 +6,9 @@
>>> */
>>>
>>> #include <linux/moduleparam.h>
>>> -#include <linux/vmalloc.h>
>>> #include <linux/v4l2-mediabus.h>
>>> +#include <linux/vmalloc.h>
>>
>> I think you moved this one by accident?
>> Please, send v4 fixing this.
> Actually we sorted the include headers as you requested in the last review.
> Is there any rule about how to sort the headers?
Yes, but the order should be
#include <linux/...>
#include <media/v4l2-rect.h>
#include <media/v4l2-subdev.h>
What I meant in previous review is that your patch should include media/v4l2-rect.h
before media/v4l2-subdev.h.
The order between linux/vmalloc.h and linux/v4l2-mediabus.h shouldn't matter for this patch,
as you are not modifying those.
Make sense?
Thanks,
Helen
>>
>>> +#include <media/v4l2-rect.h>
>>> #include <media/v4l2-subdev.h>
>>>
>>> #include "vimc-common.h"
>>> @@ -18,6 +19,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;
>>> @@ -25,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 +38,64 @@ 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 const struct v4l2_rect crop_rect_min = {
>>> + .width = VIMC_FRAME_MIN_WIDTH,
>>> + .height = VIMC_FRAME_MIN_HEIGHT,
>>> + .top = 0,
>>> + .left = 0,
>>> +};
>>> +
>>> +static struct v4l2_rect
>>> +vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
>>> +{
>>> + /* Get the crop bounds to clamp the crop rectangle correctly */
>>> + struct v4l2_rect r = {
>>> + .left = 0,
>>> + .top = 0,
>>> + .width = sink_fmt->width,
>>> + .height = sink_fmt->height,
>>> + };
>>> + return r;
>>> +}
>>> +
>>> +static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
>>> + const struct v4l2_mbus_framefmt *sink_fmt)
>>> +{
>>> + const struct v4l2_rect sink_rect =
>>> + vimc_sca_get_crop_bound_sink(sink_fmt);
>>> +
>>> + /* Disallow rectangles smaller than the minimal one. */
>>> + v4l2_rect_set_min_size(r, &crop_rect_min);
>>> + v4l2_rect_map_inside(r, &sink_rect);
>>> +}
>>> +
>>> 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 +154,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 +200,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 +208,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,8 +220,8 @@ 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);
>>> @@ -184,6 +239,80 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>>> fmt->format.xfer_func, fmt->format.ycbcr_enc);
>>>
>>> *sink_fmt = fmt->format;
>>> +
>>> + /* Do the crop, but respect the current bounds */
>>> + vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
>>> + }
>>> +
>>> + 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);
>>> + struct v4l2_mbus_framefmt *sink_fmt;
>>> + struct v4l2_rect *crop_rect;
>>> +
>>> + if (VIMC_IS_SRC(sel->pad))
>>> + return -EINVAL;
>>> +
>>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>> + 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);
>>> + }
>>> +
>>> + switch (sel->target) {
>>> + case V4L2_SEL_TGT_CROP:
>>> + sel->r = *crop_rect;
>>> + break;
>>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>>> + sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
>>> + 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 *crop_rect;
>>> + struct v4l2_rect sink_rect;
>>> + struct v4l2_mbus_framefmt *sink_fmt;
>>
>> Do you mind declaring this just after vsca? I was trying to put bigger lines first in the declaration order.
>>
>>> +
>>> + if (VIMC_IS_SRC(sel->pad))
>>> + return -EINVAL;
>>> +
>>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>> + /* Do not change the format while stream is on */
>>> + if (vsca->src_frame)
>>> + return -EBUSY;
>>> +
>>> + crop_rect = &vsca->crop_rect;
>>> + sink_fmt = &vsca->sink_fmt;
>>> + } else {
>>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
>>> + }
>>> +
>>> + switch (sel->target) {
>>> + case V4L2_SEL_TGT_CROP:
>>> + /* Do the crop, but respect the current bounds */
>>> + sink_rect = vimc_sca_get_crop_bound_sink(sink_fmt);
>>> + vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
>>> + *crop_rect = sel->r;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> }
>>>
>>> return 0;
>>> @@ -195,6 +324,8 @@ static const struct v4l2_subdev_pad_ops vimc_sca_pad_ops = {
>>> .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 +344,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 +390,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;
>>
>> Same here.
>>
>>>
>>> /* Point to the pixel value in position (lin, col) in the sink frame */
>>> index = VIMC_FRAME_INDEX(lin, col,
>>> @@ -278,8 +410,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->ved.dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
>>> vsca->sd.name, lin * sca_mult, col * sca_mult, index);
>>> @@ -308,11 +442,12 @@ static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
>>> const u8 *const sink_frame)
>>> {
>>> unsigned int i, j;
>>> + const struct v4l2_rect r = vsca->crop_rect;
>>
>> Same here.
>>
>>>
>>> /* 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);
>>> }
>>>
>>> @@ -384,5 +519,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;
>>> }
>>>
>>
>> With these changes:
>>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>
>> Thanks
>> Helen
>>
>> _______________________________________________
>> Lkcamp mailing list
>> Lkcamp@lists.libreplanetbr.org
>> https://lists.libreplanetbr.org/mailman/listinfo/lkcamp
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-11-06 19:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 1:12 [PATCH v3] media: vimc: Implement get/set selection in sink Danilo Figueiredo Rocha
2019-11-06 13:59 ` Helen Koike
2019-11-06 14:58 ` [Lkcamp] " Guilherme Alcarde Gallo
2019-11-06 19:01 ` Helen Koike
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.