2019-12-29 20:52:49

by Pedro Terra Delboni

Subject: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

Modify the scaler subdevice to accept setting the resolution of the source
pad (previously the source resolution would always be 3 times the sink for
both dimensions). Now any resolution can be set at src (even smaller ones)
and the sink video will be scaled to match it.

Test example: With the vimc module up (using the default vimc topology)
media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
v4l2-ctl -d /dev/video2 -v width=300,height=700
v4l2-ctl -d /dev/video0 -v pixelformat=BA81
v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
--stream-to=test.raw
ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
-video_size "300x700" test.raw

Co-developed-by: Gabriela Bittencourt <[email protected]>
Signed-off-by: Gabriela Bittencourt <[email protected]>
Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
Signed-off-by: Pedro "pirate" Terra <[email protected]>

---

Changes in V4:
* Rebased with media/master
* Scaling is now compatible with crop
* Updated test example at the commit message
* Add vimc prefix to the pad enumeration

Changes in V3:
* Corrections suggested by Hans:
- Default scaling factor is now 1 (we removed the define and
set the source format equals the sink).
- Removed SCA_COUNT (enum that represents the number of pads)
as there always 2
- Swapped the per byte pixel copy to memcpy.
* Corrections suggested by Dafna:
- Removed from the documentation the old scaler parameter which
isn't necessary anymore.
* Added a thank you note at the end of the email

Changes in V2:
* Patch was not sent to media list mail for some reason (even though it
was on the Cc list), trying again.
* Updating documentation.

Hello!
This code is the result of friends getting together with too much
coffee, sugar and beer trying to get started with some kernel coding.
Please, don't go easy on us! s2

Running
/usr/local/bin/v4l2-compliance -m /dev/media0
Gave the following result:
v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
---
Documentation/media/v4l-drivers/vimc.rst | 21 +-
drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
2 files changed, 87 insertions(+), 182 deletions(-)

diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
index 8f5d7f8d83bb..af04ebbd4fa1 100644
--- a/Documentation/media/v4l-drivers/vimc.rst
+++ b/Documentation/media/v4l-drivers/vimc.rst
@@ -61,9 +61,11 @@ vimc-debayer:
* 1 Pad source

vimc-scaler:
- Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
- 1920x1440 image. (this value can be configured, see at
- `Module options`_).
+ Re-size the image to meet the source pad resolution. E.g.: if the sync pad
+is configured to 360x480 and the source to 1280x720, the image will be stretched
+to fit the source resolution. Works for any resolution within the vimc
+limitations (even shrinking the image if necessary).
+
Exposes:

* 1 Pad sink
@@ -76,19 +78,6 @@ vimc-capture:
* 1 Pad sink
* 1 Pad source

-
-Module options
---------------
-
-Vimc has a module parameter to configure the driver.
-
-* ``sca_mult=<unsigned int>``
-
- Image size multiplier factor to be used to multiply both width and
- height, so the image size will be ``sca_mult^2`` bigger than the
- original one. Currently, only supports scaling up (the default value
- is 3).
-
Source code documentation
-------------------------

diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index e2e551bc3ded..785009b7ac9e 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -6,6 +6,7 @@
*/

#include <linux/moduleparam.h>
+#include <linux/string.h>
#include <linux/vmalloc.h>
#include <linux/v4l2-mediabus.h>
#include <media/v4l2-rect.h>
@@ -13,11 +14,11 @@

#include "vimc-common.h"

-static unsigned int sca_mult = 3;
-module_param(sca_mult, uint, 0000);
-MODULE_PARM_DESC(sca_mult, " the image size multiplier");
-
-#define MAX_ZOOM 8
+/* Pad identifier */
+enum vimc_sca_pad {
+ VIMC_SCA_SINK = 0,
+ VIMC_SCA_SRC = 1,
+};

#define VIMC_SCA_FMT_WIDTH_DEFAULT 640
#define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
@@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
struct vimc_sca_device {
struct vimc_ent_device ved;
struct v4l2_subdev sd;
- /* NOTE: the source fmt is the same as the sink
- * with the width and hight multiplied by mult
- */
- struct v4l2_mbus_framefmt sink_fmt;
struct v4l2_rect crop_rect;
+ /* Frame format for both sink and src pad */
+ struct v4l2_mbus_framefmt fmt[2];
/* Values calculated when the stream starts */
u8 *src_frame;
- unsigned int src_line_size;
unsigned int bpp;
struct media_pad pads[2];
};
@@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
struct v4l2_rect *r;
unsigned int i;

- mf = v4l2_subdev_get_try_format(sd, cfg, 0);
+ mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
*mf = sink_fmt_default;

- r = v4l2_subdev_get_try_crop(sd, cfg, 0);
+ r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
*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;
- mf->width = mf->width * sca_mult;
- mf->height = mf->height * sca_mult;
}

return 0;
@@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,

fse->min_width = VIMC_FRAME_MIN_WIDTH;
fse->min_height = VIMC_FRAME_MIN_HEIGHT;
-
- if (VIMC_IS_SINK(fse->pad)) {
- fse->max_width = VIMC_FRAME_MAX_WIDTH;
- fse->max_height = VIMC_FRAME_MAX_HEIGHT;
- } else {
- fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
- fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
- }
+ fse->max_width = VIMC_FRAME_MAX_WIDTH;
+ fse->max_height = VIMC_FRAME_MAX_HEIGHT;

return 0;
}
@@ -154,95 +144,73 @@ 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 */
- 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 = crop_rect->width * sca_mult;
- format->format.height = crop_rect->height * sca_mult;
- }
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY)
+ format->format = *v4l2_subdev_get_try_format(sd, cfg,
+ format->pad);
+ else
+ format->format = vsca->fmt[format->pad];

return 0;
}

-static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
+static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
{
- const struct vimc_pix_map *vpix;
+ if (pad == VIMC_SCA_SINK) {
+ const struct vimc_pix_map *vpix;

- /* Only accept code in the pix map table in non bayer format */
- vpix = vimc_pix_map_by_code(fmt->code);
- if (!vpix || vpix->bayer)
- fmt->code = sink_fmt_default.code;
+ /* Only accept code in the pix map table in non bayer format */
+ vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
+ if (!vpix || vpix->bayer)
+ fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
+ if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
+ fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;

- fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
+ vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
+ }
+
+ fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
VIMC_FRAME_MAX_WIDTH) & ~1;
- fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
+ fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
VIMC_FRAME_MAX_HEIGHT) & ~1;

- if (fmt->field == V4L2_FIELD_ANY)
- fmt->field = sink_fmt_default.field;
-
- vimc_colorimetry_clamp(fmt);
+ /* Assure src pad attributes besides dimensions are the same as sink */
+ fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
+ fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
+ fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
}

static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
- struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_format *format)
{
struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
- struct v4l2_mbus_framefmt *sink_fmt;
+ struct v4l2_mbus_framefmt *fmt[2];
struct v4l2_rect *crop_rect;

- if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
/* Do not change the format while stream is on */
if (vsca->src_frame)
return -EBUSY;

- sink_fmt = &vsca->sink_fmt;
+ fmt[VIMC_SCA_SINK] = &vsca->fmt[VIMC_SCA_SINK];
+ fmt[VIMC_SCA_SRC] = &vsca->fmt[VIMC_SCA_SRC];
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);
+ fmt[VIMC_SCA_SINK] = v4l2_subdev_get_try_format(sd, cfg,
+ VIMC_SCA_SINK);
+ fmt[VIMC_SCA_SRC] = v4l2_subdev_get_try_format(sd, cfg,
+ VIMC_SCA_SRC);
+ crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
}

- /*
- * Do not change the format of the source pad,
- * it is propagated from the sink
- */
- if (VIMC_IS_SRC(fmt->pad)) {
- fmt->format = *sink_fmt;
- 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);
-
- dev_dbg(vsca->ved.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,
- /* old */
- sink_fmt->width, sink_fmt->height, sink_fmt->code,
- sink_fmt->colorspace, sink_fmt->quantization,
- sink_fmt->xfer_func, sink_fmt->ycbcr_enc,
- /* new */
- fmt->format.width, fmt->format.height, fmt->format.code,
- fmt->format.colorspace, fmt->format.quantization,
- fmt->format.xfer_func, fmt->format.ycbcr_enc);
-
- *sink_fmt = fmt->format;
+ *fmt[format->pad] = format->format;
+ vimc_sca_adjust_fmt(fmt, format->pad);
+ format->format = *fmt[format->pad];

+ if (format->pad == VIMC_SCA_SINK)
/* Do the crop, but respect the current bounds */
- vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
- }
+ vimc_sca_adjust_sink_crop(crop_rect, fmt[VIMC_SCA_SINK]);

return 0;
}
@@ -259,11 +227,11 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
return -EINVAL;

if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
- sink_fmt = &vsca->sink_fmt;
+ sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
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);
+ sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
+ crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
}

switch (sel->target) {
@@ -297,10 +265,10 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
return -EBUSY;

crop_rect = &vsca->crop_rect;
- sink_fmt = &vsca->sink_fmt;
+ sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
} else {
- crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
- sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+ crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
+ sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
}

switch (sel->target) {
@@ -338,16 +306,12 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
return 0;

/* Save the bytes per pixel of the sink */
- vpix = vimc_pix_map_by_code(vsca->sink_fmt.code);
+ vpix = vimc_pix_map_by_code(vsca->fmt[VIMC_SCA_SINK].code);
vsca->bpp = vpix->bpp;

- /* Calculate the width in bytes of the src frame */
- 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->crop_rect.height *
- sca_mult;
+ frame_size = vsca->fmt[VIMC_SCA_SRC].width
+ * vsca->fmt[VIMC_SCA_SRC].height * vsca->bpp;

/* Allocate the frame buffer. Use vmalloc to be able to
* allocate a large amount of memory
@@ -376,77 +340,28 @@ static const struct v4l2_subdev_ops vimc_sca_ops = {
.video = &vimc_sca_video_ops,
};

-static void vimc_sca_fill_pix(u8 *const ptr,
- const u8 *const pixel,
- const unsigned int bpp)
-{
- unsigned int i;
-
- /* copy the pixel to the pointer */
- for (i = 0; i < bpp; i++)
- ptr[i] = pixel[i];
-}
-
-static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
- unsigned int lin, unsigned int col,
- const u8 *const sink_frame)
-{
- const struct v4l2_rect crop_rect = vsca->crop_rect;
- unsigned int i, j, index;
- const u8 *pixel;
-
- /* Point to the pixel value in position (lin, col) in the sink frame */
- index = VIMC_FRAME_INDEX(lin, col,
- vsca->sink_fmt.width,
- vsca->bpp);
- pixel = &sink_frame[index];
-
- dev_dbg(vsca->ved.dev,
- "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
- vsca->sd.name, lin, col, index);
-
- /* 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,
- 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);
-
- /* Repeat this pixel mult times */
- for (i = 0; i < sca_mult; i++) {
- /* Iterate through each beginning of a
- * pixel repetition in a line
- */
- for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
- dev_dbg(vsca->ved.dev,
- "sca: %s: sca: scale_pix src pos %d\n",
- vsca->sd.name, index + j);
-
- /* copy the pixel to the position index + j */
- vimc_sca_fill_pix(&vsca->src_frame[index + j],
- pixel, vsca->bpp);
- }
-
- /* move the index to the next line */
- index += vsca->src_line_size;
- }
-}
-
static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
const u8 *const sink_frame)
{
- const struct v4l2_rect r = vsca->crop_rect;
- unsigned int i, j;
-
- /* Scale each pixel from the original sink frame */
- /* TODO: implement scale down, only scale up is supported for now */
- 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);
+ const struct v4l2_mbus_framefmt *src_fmt = &vsca->fmt[VIMC_SCA_SRC];
+ const struct v4l2_rect *r = &vsca->crop_rect;
+ unsigned int s_width = vsca->fmt[VIMC_SCA_SINK].width;
+ unsigned int lin, col, index;
+ u8 *walker = vsca->src_frame;
+
+ /* Set each pixel at the src_frame to its sink_frame equivalent */
+ for (lin = 0; lin < src_fmt->height; lin++) {
+ for (col = 0; col < src_fmt->width; col++) {
+ unsigned int s_lin, s_col;
+
+ s_lin = (lin * r->height) / src_fmt->height + r->top;
+ s_col = (col * r->width) / src_fmt->width + r->left;
+ index = VIMC_FRAME_INDEX(s_lin, s_col, s_width,
+ vsca->bpp);
+ memcpy(walker, &sink_frame[index], vsca->bpp);
+ walker += vsca->bpp;
+ }
+ }
}

static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
@@ -498,8 +413,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
return NULL;

/* Initialize ved and sd */
- vsca->pads[0].flags = MEDIA_PAD_FL_SINK;
- vsca->pads[1].flags = MEDIA_PAD_FL_SOURCE;
+ vsca->pads[VIMC_SCA_SINK].flags = MEDIA_PAD_FL_SINK;
+ vsca->pads[VIMC_SCA_SRC].flags = MEDIA_PAD_FL_SOURCE;

ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
vcfg_name,
@@ -515,7 +430,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
vsca->ved.dev = &vimc->pdev.dev;

/* Initialize the frame format */
- vsca->sink_fmt = sink_fmt_default;
+ vsca->fmt[VIMC_SCA_SINK] = sink_fmt_default;
+ vsca->fmt[VIMC_SCA_SRC] = sink_fmt_default;

/* Initialize the crop selection */
vsca->crop_rect = crop_rect_default;
--
2.23.0

2019-12-30 14:00:45

by Helen Koike

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

Hi,

Thanks for the patch, just minor comments below.

On 12/29/19 5:42 PM, Pedro Terra wrote:
> Modify the scaler subdevice to accept setting the resolution of the source
> pad (previously the source resolution would always be 3 times the sink for
> both dimensions). Now any resolution can be set at src (even smaller ones)
> and the sink video will be scaled to match it.
>
> Test example: With the vimc module up (using the default vimc topology)
> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> v4l2-ctl -d /dev/video2 -v width=300,height=700
> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
> --stream-to=test.raw
> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> -video_size "300x700" test.raw
>
> Co-developed-by: Gabriela Bittencourt <[email protected]>
> Signed-off-by: Gabriela Bittencourt <[email protected]>
> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
> Signed-off-by: Pedro "pirate" Terra <[email protected]>
>
> ---
>
> Changes in V4:
> * Rebased with media/master
> * Scaling is now compatible with crop
> * Updated test example at the commit message
> * Add vimc prefix to the pad enumeration
>
> Changes in V3:
> * Corrections suggested by Hans:
> - Default scaling factor is now 1 (we removed the define and
> set the source format equals the sink).
> - Removed SCA_COUNT (enum that represents the number of pads)
> as there always 2
> - Swapped the per byte pixel copy to memcpy.
> * Corrections suggested by Dafna:
> - Removed from the documentation the old scaler parameter which
> isn't necessary anymore.
> * Added a thank you note at the end of the email
>
> Changes in V2:
> * Patch was not sent to media list mail for some reason (even though it
> was on the Cc list), trying again.
> * Updating documentation.
>
> Hello!
> This code is the result of friends getting together with too much
> coffee, sugar and beer trying to get started with some kernel coding.
> Please, don't go easy on us! s2
>
> Running
> /usr/local/bin/v4l2-compliance -m /dev/media0
> Gave the following result:
> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
> ---
> Documentation/media/v4l-drivers/vimc.rst | 21 +-
> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
> 2 files changed, 87 insertions(+), 182 deletions(-)
>
> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
> index 8f5d7f8d83bb..af04ebbd4fa1 100644
> --- a/Documentation/media/v4l-drivers/vimc.rst
> +++ b/Documentation/media/v4l-drivers/vimc.rst
> @@ -61,9 +61,11 @@ vimc-debayer:
> * 1 Pad source
>
> vimc-scaler:
> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
> - 1920x1440 image. (this value can be configured, see at
> - `Module options`_).
> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
> +is configured to 360x480 and the source to 1280x720, the image will be stretched
> +to fit the source resolution. Works for any resolution within the vimc
> +limitations (even shrinking the image if necessary).
> +
> Exposes:
>
> * 1 Pad sink
> @@ -76,19 +78,6 @@ vimc-capture:
> * 1 Pad sink
> * 1 Pad source
>
> -
> -Module options
> ---------------
> -
> -Vimc has a module parameter to configure the driver.
> -
> -* ``sca_mult=<unsigned int>``
> -
> - Image size multiplier factor to be used to multiply both width and
> - height, so the image size will be ``sca_mult^2`` bigger than the
> - original one. Currently, only supports scaling up (the default value
> - is 3).
> -
> Source code documentation
> -------------------------
>
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index e2e551bc3ded..785009b7ac9e 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/moduleparam.h>
> +#include <linux/string.h>
> #include <linux/vmalloc.h>
> #include <linux/v4l2-mediabus.h>
> #include <media/v4l2-rect.h>
> @@ -13,11 +14,11 @@
>
> #include "vimc-common.h"
>
> -static unsigned int sca_mult = 3;
> -module_param(sca_mult, uint, 0000);
> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> -
> -#define MAX_ZOOM 8
> +/* Pad identifier */
> +enum vimc_sca_pad {
> + VIMC_SCA_SINK = 0,
> + VIMC_SCA_SRC = 1,
> +};
>
> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> struct vimc_sca_device {
> struct vimc_ent_device ved;
> struct v4l2_subdev sd;
> - /* NOTE: the source fmt is the same as the sink
> - * with the width and hight multiplied by mult
> - */
> - struct v4l2_mbus_framefmt sink_fmt;
> struct v4l2_rect crop_rect;
> + /* Frame format for both sink and src pad */
> + struct v4l2_mbus_framefmt fmt[2];
> /* Values calculated when the stream starts */
> u8 *src_frame;
> - unsigned int src_line_size;
> unsigned int bpp;
> struct media_pad pads[2];
> };
> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> struct v4l2_rect *r;
> unsigned int i;
>
> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> *mf = sink_fmt_default;
>
> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> *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;
> - mf->width = mf->width * sca_mult;
> - mf->height = mf->height * sca_mult;
> }
>
> return 0;
> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>
> fse->min_width = VIMC_FRAME_MIN_WIDTH;
> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
> -
> - if (VIMC_IS_SINK(fse->pad)) {
> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> - } else {
> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
> - }
> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>
> return 0;
> }
> @@ -154,95 +144,73 @@ 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 */
> - 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 = crop_rect->width * sca_mult;
> - format->format.height = crop_rect->height * sca_mult;
> - }
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
> + format->pad);
> + else
> + format->format = vsca->fmt[format->pad];
>
> return 0;
> }
>
> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
> {
> - const struct vimc_pix_map *vpix;
> + if (pad == VIMC_SCA_SINK) {
> + const struct vimc_pix_map *vpix;
>
> - /* Only accept code in the pix map table in non bayer format */
> - vpix = vimc_pix_map_by_code(fmt->code);
> - if (!vpix || vpix->bayer)
> - fmt->code = sink_fmt_default.code;
> + /* Only accept code in the pix map table in non bayer format */
> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
> + if (!vpix || vpix->bayer)
> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
>
> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
> + }
> +
> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
> VIMC_FRAME_MAX_WIDTH) & ~1;

Could you fix the alignment here?
For some reason checkpatch doesn't catch this :(

> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
> VIMC_FRAME_MAX_HEIGHT) & ~1;

Also here.

>
> - if (fmt->field == V4L2_FIELD_ANY)
> - fmt->field = sink_fmt_default.field;
> -
> - vimc_colorimetry_clamp(fmt);
> + /* Assure src pad attributes besides dimensions are the same as sink */
> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;

Ideally we should propagate all the other fields to src. Maybe save width and height to
a tmp var, assing the whole sink fmt to src, and restore width and height.



With these changes:
Acked-by: Helen Koike <[email protected]>

Thanks
Helen


> }
>
> static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_pad_config *cfg,
> - struct v4l2_subdev_format *fmt)
> + struct v4l2_subdev_format *format)
> {
> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> - struct v4l2_mbus_framefmt *sink_fmt;
> + struct v4l2_mbus_framefmt *fmt[2];
> struct v4l2_rect *crop_rect;
>
> - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> /* Do not change the format while stream is on */
> if (vsca->src_frame)
> return -EBUSY;
>
> - sink_fmt = &vsca->sink_fmt;
> + fmt[VIMC_SCA_SINK] = &vsca->fmt[VIMC_SCA_SINK];
> + fmt[VIMC_SCA_SRC] = &vsca->fmt[VIMC_SCA_SRC];
> 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);
> + fmt[VIMC_SCA_SINK] = v4l2_subdev_get_try_format(sd, cfg,
> + VIMC_SCA_SINK);
> + fmt[VIMC_SCA_SRC] = v4l2_subdev_get_try_format(sd, cfg,
> + VIMC_SCA_SRC);
> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> }
>
> - /*
> - * Do not change the format of the source pad,
> - * it is propagated from the sink
> - */
> - if (VIMC_IS_SRC(fmt->pad)) {
> - fmt->format = *sink_fmt;
> - 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);
> -
> - dev_dbg(vsca->ved.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,
> - /* old */
> - sink_fmt->width, sink_fmt->height, sink_fmt->code,
> - sink_fmt->colorspace, sink_fmt->quantization,
> - sink_fmt->xfer_func, sink_fmt->ycbcr_enc,
> - /* new */
> - fmt->format.width, fmt->format.height, fmt->format.code,
> - fmt->format.colorspace, fmt->format.quantization,
> - fmt->format.xfer_func, fmt->format.ycbcr_enc);
> -
> - *sink_fmt = fmt->format;
> + *fmt[format->pad] = format->format;
> + vimc_sca_adjust_fmt(fmt, format->pad);
> + format->format = *fmt[format->pad];
>
> + if (format->pad == VIMC_SCA_SINK)
> /* Do the crop, but respect the current bounds */
> - vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
> - }
> + vimc_sca_adjust_sink_crop(crop_rect, fmt[VIMC_SCA_SINK]);
>
> return 0;
> }
> @@ -259,11 +227,11 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
> return -EINVAL;
>
> if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> - sink_fmt = &vsca->sink_fmt;
> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
> 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);
> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> }
>
> switch (sel->target) {
> @@ -297,10 +265,10 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
> return -EBUSY;
>
> crop_rect = &vsca->crop_rect;
> - sink_fmt = &vsca->sink_fmt;
> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
> } else {
> - crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
> - sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> }
>
> switch (sel->target) {
> @@ -338,16 +306,12 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
> return 0;
>
> /* Save the bytes per pixel of the sink */
> - vpix = vimc_pix_map_by_code(vsca->sink_fmt.code);
> + vpix = vimc_pix_map_by_code(vsca->fmt[VIMC_SCA_SINK].code);
> vsca->bpp = vpix->bpp;
>
> - /* Calculate the width in bytes of the src frame */
> - 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->crop_rect.height *
> - sca_mult;
> + frame_size = vsca->fmt[VIMC_SCA_SRC].width
> + * vsca->fmt[VIMC_SCA_SRC].height * vsca->bpp;
>
> /* Allocate the frame buffer. Use vmalloc to be able to
> * allocate a large amount of memory
> @@ -376,77 +340,28 @@ static const struct v4l2_subdev_ops vimc_sca_ops = {
> .video = &vimc_sca_video_ops,
> };
>
> -static void vimc_sca_fill_pix(u8 *const ptr,
> - const u8 *const pixel,
> - const unsigned int bpp)
> -{
> - unsigned int i;
> -
> - /* copy the pixel to the pointer */
> - for (i = 0; i < bpp; i++)
> - ptr[i] = pixel[i];
> -}
> -
> -static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> - unsigned int lin, unsigned int col,
> - const u8 *const sink_frame)
> -{
> - const struct v4l2_rect crop_rect = vsca->crop_rect;
> - unsigned int i, j, index;
> - const u8 *pixel;
> -
> - /* Point to the pixel value in position (lin, col) in the sink frame */
> - index = VIMC_FRAME_INDEX(lin, col,
> - vsca->sink_fmt.width,
> - vsca->bpp);
> - pixel = &sink_frame[index];
> -
> - dev_dbg(vsca->ved.dev,
> - "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
> - vsca->sd.name, lin, col, index);
> -
> - /* 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,
> - 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);
> -
> - /* Repeat this pixel mult times */
> - for (i = 0; i < sca_mult; i++) {
> - /* Iterate through each beginning of a
> - * pixel repetition in a line
> - */
> - for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
> - dev_dbg(vsca->ved.dev,
> - "sca: %s: sca: scale_pix src pos %d\n",
> - vsca->sd.name, index + j);
> -
> - /* copy the pixel to the position index + j */
> - vimc_sca_fill_pix(&vsca->src_frame[index + j],
> - pixel, vsca->bpp);
> - }
> -
> - /* move the index to the next line */
> - index += vsca->src_line_size;
> - }
> -}
> -
> static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
> const u8 *const sink_frame)
> {
> - const struct v4l2_rect r = vsca->crop_rect;
> - unsigned int i, j;
> -
> - /* Scale each pixel from the original sink frame */
> - /* TODO: implement scale down, only scale up is supported for now */
> - 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);
> + const struct v4l2_mbus_framefmt *src_fmt = &vsca->fmt[VIMC_SCA_SRC];
> + const struct v4l2_rect *r = &vsca->crop_rect;
> + unsigned int s_width = vsca->fmt[VIMC_SCA_SINK].width;
> + unsigned int lin, col, index;
> + u8 *walker = vsca->src_frame;
> +
> + /* Set each pixel at the src_frame to its sink_frame equivalent */
> + for (lin = 0; lin < src_fmt->height; lin++) {
> + for (col = 0; col < src_fmt->width; col++) {
> + unsigned int s_lin, s_col;
> +
> + s_lin = (lin * r->height) / src_fmt->height + r->top;
> + s_col = (col * r->width) / src_fmt->width + r->left;
> + index = VIMC_FRAME_INDEX(s_lin, s_col, s_width,
> + vsca->bpp);
> + memcpy(walker, &sink_frame[index], vsca->bpp);
> + walker += vsca->bpp;
> + }
> + }
> }
>
> static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> @@ -498,8 +413,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> return NULL;
>
> /* Initialize ved and sd */
> - vsca->pads[0].flags = MEDIA_PAD_FL_SINK;
> - vsca->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> + vsca->pads[VIMC_SCA_SINK].flags = MEDIA_PAD_FL_SINK;
> + vsca->pads[VIMC_SCA_SRC].flags = MEDIA_PAD_FL_SOURCE;
>
> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
> vcfg_name,
> @@ -515,7 +430,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> vsca->ved.dev = &vimc->pdev.dev;
>
> /* Initialize the frame format */
> - vsca->sink_fmt = sink_fmt_default;
> + vsca->fmt[VIMC_SCA_SINK] = sink_fmt_default;
> + vsca->fmt[VIMC_SCA_SRC] = sink_fmt_default;
>
> /* Initialize the crop selection */
> vsca->crop_rect = crop_rect_default;
>

2020-01-01 10:12:04

by Dafna Hirschfeld

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

Hi,


On 30.12.19 14:59, Helen Koike wrote:
> Hi,
>
> Thanks for the patch, just minor comments below.
>
> On 12/29/19 5:42 PM, Pedro Terra wrote:
>> Modify the scaler subdevice to accept setting the resolution of the source
>> pad (previously the source resolution would always be 3 times the sink for
>> both dimensions). Now any resolution can be set at src (even smaller ones)
>> and the sink video will be scaled to match it.
>>
>> Test example: With the vimc module up (using the default vimc topology)
>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
>> v4l2-ctl -d /dev/video2 -v width=300,height=700
>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
>> --stream-to=test.raw
>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
>> -video_size "300x700" test.raw
>>
>> Co-developed-by: Gabriela Bittencourt <[email protected]>
>> Signed-off-by: Gabriela Bittencourt <[email protected]>
>> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
>> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
>> Signed-off-by: Pedro "pirate" Terra <[email protected]>
>>
>> ---
>>
>> Changes in V4:
>> * Rebased with media/master
>> * Scaling is now compatible with crop
>> * Updated test example at the commit message
>> * Add vimc prefix to the pad enumeration
>>
>> Changes in V3:
>> * Corrections suggested by Hans:
>> - Default scaling factor is now 1 (we removed the define and
>> set the source format equals the sink).
>> - Removed SCA_COUNT (enum that represents the number of pads)
>> as there always 2
>> - Swapped the per byte pixel copy to memcpy.
>> * Corrections suggested by Dafna:
>> - Removed from the documentation the old scaler parameter which
>> isn't necessary anymore.
>> * Added a thank you note at the end of the email
>>
>> Changes in V2:
>> * Patch was not sent to media list mail for some reason (even though it
>> was on the Cc list), trying again.
>> * Updating documentation.
>>
>> Hello!
>> This code is the result of friends getting together with too much
>> coffee, sugar and beer trying to get started with some kernel coding.
>> Please, don't go easy on us! s2
>>
>> Running
>> /usr/local/bin/v4l2-compliance -m /dev/media0
>> Gave the following result:
>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
>> ---
>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
>> 2 files changed, 87 insertions(+), 182 deletions(-)
>>
>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
>> --- a/Documentation/media/v4l-drivers/vimc.rst
>> +++ b/Documentation/media/v4l-drivers/vimc.rst
>> @@ -61,9 +61,11 @@ vimc-debayer:
>> * 1 Pad source
>>
>> vimc-scaler:
>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
>> - 1920x1440 image. (this value can be configured, see at
>> - `Module options`_).
>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
>> +to fit the source resolution. Works for any resolution within the vimc
>> +limitations (even shrinking the image if necessary).
>> +
>> Exposes:
>>
>> * 1 Pad sink
>> @@ -76,19 +78,6 @@ vimc-capture:
>> * 1 Pad sink
>> * 1 Pad source
>>
>> -
>> -Module options
>> ---------------
>> -
>> -Vimc has a module parameter to configure the driver.
>> -
>> -* ``sca_mult=<unsigned int>``
>> -
>> - Image size multiplier factor to be used to multiply both width and
>> - height, so the image size will be ``sca_mult^2`` bigger than the
>> - original one. Currently, only supports scaling up (the default value
>> - is 3).
>> -
>> Source code documentation
>> -------------------------
>>
>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>> index e2e551bc3ded..785009b7ac9e 100644
>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>> @@ -6,6 +6,7 @@
>> */
>>
>> #include <linux/moduleparam.h>
>> +#include <linux/string.h>
>> #include <linux/vmalloc.h>
>> #include <linux/v4l2-mediabus.h>
>> #include <media/v4l2-rect.h>
>> @@ -13,11 +14,11 @@
>>
>> #include "vimc-common.h"
>>
>> -static unsigned int sca_mult = 3;
>> -module_param(sca_mult, uint, 0000);
>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>> -
>> -#define MAX_ZOOM 8
>> +/* Pad identifier */
>> +enum vimc_sca_pad {
>> + VIMC_SCA_SINK = 0,
>> + VIMC_SCA_SRC = 1,
>> +};
>>
>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>> struct vimc_sca_device {
>> struct vimc_ent_device ved;
>> struct v4l2_subdev sd;
>> - /* NOTE: the source fmt is the same as the sink
>> - * with the width and hight multiplied by mult
>> - */
>> - struct v4l2_mbus_framefmt sink_fmt;
>> struct v4l2_rect crop_rect;
>> + /* Frame format for both sink and src pad */
>> + struct v4l2_mbus_framefmt fmt[2];
>> /* Values calculated when the stream starts */
>> u8 *src_frame;
>> - unsigned int src_line_size;
>> unsigned int bpp;
>> struct media_pad pads[2];
>> };
>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>> struct v4l2_rect *r;
>> unsigned int i;
>>
>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>> *mf = sink_fmt_default;
>>
>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>> *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;
>> - mf->width = mf->width * sca_mult;
>> - mf->height = mf->height * sca_mult;
>> }
>>
>> return 0;
>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>>
>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
>> -
>> - if (VIMC_IS_SINK(fse->pad)) {
>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>> - } else {
>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
>> - }
>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>
>> return 0;
>> }
>> @@ -154,95 +144,73 @@ 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 */
>> - 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 = crop_rect->width * sca_mult;
>> - format->format.height = crop_rect->height * sca_mult;
>> - }
>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
>> + format->pad);
>> + else
>> + format->format = vsca->fmt[format->pad];
>>
>> return 0;
>> }
>>
>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
>> {
>> - const struct vimc_pix_map *vpix;
>> + if (pad == VIMC_SCA_SINK) {
>> + const struct vimc_pix_map *vpix;
>>
>> - /* Only accept code in the pix map table in non bayer format */
>> - vpix = vimc_pix_map_by_code(fmt->code);
>> - if (!vpix || vpix->bayer)
>> - fmt->code = sink_fmt_default.code;
>> + /* Only accept code in the pix map table in non bayer format */
>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
>> + if (!vpix || vpix->bayer)
>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
>>
>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
>> + }
>> +
>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
>> VIMC_FRAME_MAX_WIDTH) & ~1;
>
> Could you fix the alignment here?
> For some reason checkpatch doesn't catch this :(
>
>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
>> VIMC_FRAME_MAX_HEIGHT) & ~1;
>
> Also here.
>
>>
>> - if (fmt->field == V4L2_FIELD_ANY)
>> - fmt->field = sink_fmt_default.field;
>> -
>> - vimc_colorimetry_clamp(fmt);
>> + /* Assure src pad attributes besides dimensions are the same as sink */
>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
>
> Ideally we should propagate all the other fields to src. Maybe save width and height to
> a tmp var, assing the whole sink fmt to src, and restore width and height.
>
Acctually according to the subdevices documentation, when changing the
sink format, the width and height of the src format should reset to the
same values:

""
- Sub-devices that scale frames using variable scaling factors should
reset the scale factors to default values when sink pads formats are
modified. If the 1:1 scaling ratio is supported, this means that
source pads formats should be reset to the sink pads formats.
""
>
>
> With these changes:
> Acked-by: Helen Koike <[email protected]>
>
> Thanks
> Helen
>
>
>> }
>>
>> static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>> struct v4l2_subdev_pad_config *cfg,
>> - struct v4l2_subdev_format *fmt)
>> + struct v4l2_subdev_format *format)
>> {
>> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
>> - struct v4l2_mbus_framefmt *sink_fmt;
>> + struct v4l2_mbus_framefmt *fmt[2];
>> struct v4l2_rect *crop_rect;
>>
>> - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>> /* Do not change the format while stream is on */
>> if (vsca->src_frame)
>> return -EBUSY;
>>
>> - sink_fmt = &vsca->sink_fmt;
>> + fmt[VIMC_SCA_SINK] = &vsca->fmt[VIMC_SCA_SINK];
>> + fmt[VIMC_SCA_SRC] = &vsca->fmt[VIMC_SCA_SRC];
>> 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);
>> + fmt[VIMC_SCA_SINK] = v4l2_subdev_get_try_format(sd, cfg,
>> + VIMC_SCA_SINK);
>> + fmt[VIMC_SCA_SRC] = v4l2_subdev_get_try_format(sd, cfg,
>> + VIMC_SCA_SRC);
>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>> }
>>
>> - /*
>> - * Do not change the format of the source pad,
>> - * it is propagated from the sink
>> - */
>> - if (VIMC_IS_SRC(fmt->pad)) {
>> - fmt->format = *sink_fmt;
>> - 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);
>> -
>> - dev_dbg(vsca->ved.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,
>> - /* old */
>> - sink_fmt->width, sink_fmt->height, sink_fmt->code,
>> - sink_fmt->colorspace, sink_fmt->quantization,
>> - sink_fmt->xfer_func, sink_fmt->ycbcr_enc,
>> - /* new */
>> - fmt->format.width, fmt->format.height, fmt->format.code,
>> - fmt->format.colorspace, fmt->format.quantization,
>> - fmt->format.xfer_func, fmt->format.ycbcr_enc);
>> -
>> - *sink_fmt = fmt->format;
>> + *fmt[format->pad] = format->format;
>> + vimc_sca_adjust_fmt(fmt, format->pad);
>> + format->format = *fmt[format->pad];
>>
>> + if (format->pad == VIMC_SCA_SINK)
>> /* Do the crop, but respect the current bounds */
>> - vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
>> - }
>> + vimc_sca_adjust_sink_crop(crop_rect, fmt[VIMC_SCA_SINK]);
>>
>> return 0;
>> }
>> @@ -259,11 +227,11 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
>> return -EINVAL;
>>
>> if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>> - sink_fmt = &vsca->sink_fmt;
>> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
>> 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);
>> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>> }
>>
>> switch (sel->target) {
>> @@ -297,10 +265,10 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
>> return -EBUSY;
>>
>> crop_rect = &vsca->crop_rect;
>> - sink_fmt = &vsca->sink_fmt;
>> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
>> } else {
>> - crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
>> - sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>> }
>>
>> switch (sel->target) {
>> @@ -338,16 +306,12 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>> return 0;
>>
>> /* Save the bytes per pixel of the sink */
>> - vpix = vimc_pix_map_by_code(vsca->sink_fmt.code);
>> + vpix = vimc_pix_map_by_code(vsca->fmt[VIMC_SCA_SINK].code);
>> vsca->bpp = vpix->bpp;
>>
>> - /* Calculate the width in bytes of the src frame */
>> - 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->crop_rect.height *
>> - sca_mult;
>> + frame_size = vsca->fmt[VIMC_SCA_SRC].width
>> + * vsca->fmt[VIMC_SCA_SRC].height * vsca->bpp;
>>
>> /* Allocate the frame buffer. Use vmalloc to be able to
>> * allocate a large amount of memory
>> @@ -376,77 +340,28 @@ static const struct v4l2_subdev_ops vimc_sca_ops = {
>> .video = &vimc_sca_video_ops,
>> };
>>
>> -static void vimc_sca_fill_pix(u8 *const ptr,
>> - const u8 *const pixel,
>> - const unsigned int bpp)
>> -{
>> - unsigned int i;
>> -
>> - /* copy the pixel to the pointer */
>> - for (i = 0; i < bpp; i++)
>> - ptr[i] = pixel[i];
>> -}
>> -
>> -static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
>> - unsigned int lin, unsigned int col,
>> - const u8 *const sink_frame)
>> -{
>> - const struct v4l2_rect crop_rect = vsca->crop_rect;
>> - unsigned int i, j, index;
>> - const u8 *pixel;
>> -
>> - /* Point to the pixel value in position (lin, col) in the sink frame */
>> - index = VIMC_FRAME_INDEX(lin, col,
>> - vsca->sink_fmt.width,
>> - vsca->bpp);
>> - pixel = &sink_frame[index];
>> -
>> - dev_dbg(vsca->ved.dev,
>> - "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
>> - vsca->sd.name, lin, col, index);
>> -
>> - /* 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,
>> - 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);
>> -
>> - /* Repeat this pixel mult times */
>> - for (i = 0; i < sca_mult; i++) {
>> - /* Iterate through each beginning of a
>> - * pixel repetition in a line
>> - */
>> - for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
>> - dev_dbg(vsca->ved.dev,
>> - "sca: %s: sca: scale_pix src pos %d\n",
>> - vsca->sd.name, index + j);
>> -
>> - /* copy the pixel to the position index + j */
>> - vimc_sca_fill_pix(&vsca->src_frame[index + j],
>> - pixel, vsca->bpp);
>> - }
>> -
>> - /* move the index to the next line */
>> - index += vsca->src_line_size;
>> - }
>> -}
>> -
>> static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
>> const u8 *const sink_frame)
>> {
>> - const struct v4l2_rect r = vsca->crop_rect;
>> - unsigned int i, j;
>> -
>> - /* Scale each pixel from the original sink frame */
>> - /* TODO: implement scale down, only scale up is supported for now */
>> - 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);
>> + const struct v4l2_mbus_framefmt *src_fmt = &vsca->fmt[VIMC_SCA_SRC];
>> + const struct v4l2_rect *r = &vsca->crop_rect;
>> + unsigned int s_width = vsca->fmt[VIMC_SCA_SINK].width;
better replace 's_width' with 'snk_width' (s can also indicate 'source')

>> + unsigned int lin, col, index;
maybe replace 'lin' and 'col' with 'src_lin' 'src_col' and move 'index'
to inside the loop
>> + u8 *walker = vsca->src_frame;
>> +
>> + /* Set each pixel at the src_frame to its sink_frame equivalent */
>> + for (lin = 0; lin < src_fmt->height; lin++) {
>> + for (col = 0; col < src_fmt->width; col++) {
>> + unsigned int s_lin, s_col;
the 's_' prefix is confusing, better replace with 'snk_'
>> +
>> + s_lin = (lin * r->height) / src_fmt->height + r->top;
>> + s_col = (col * r->width) / src_fmt->width + r->left;
>> + index = VIMC_FRAME_INDEX(s_lin, s_col, s_width,
>> + vsca->bpp);
>> + memcpy(walker, &sink_frame[index], vsca->bpp);
>> + walker += vsca->bpp;
>> + }
>> + }
>> }
>>
>> static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>> @@ -498,8 +413,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> return NULL;
>>
>> /* Initialize ved and sd */
>> - vsca->pads[0].flags = MEDIA_PAD_FL_SINK;
>> - vsca->pads[1].flags = MEDIA_PAD_FL_SOURCE;
>> + vsca->pads[VIMC_SCA_SINK].flags = MEDIA_PAD_FL_SINK;
>> + vsca->pads[VIMC_SCA_SRC].flags = MEDIA_PAD_FL_SOURCE;
>>
>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
>> vcfg_name,
>> @@ -515,7 +430,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> vsca->ved.dev = &vimc->pdev.dev;
>>
>> /* Initialize the frame format */
>> - vsca->sink_fmt = sink_fmt_default;
>> + vsca->fmt[VIMC_SCA_SINK] = sink_fmt_default;
>> + vsca->fmt[VIMC_SCA_SRC] = sink_fmt_default;
>>
>> /* Initialize the crop selection */
>> vsca->crop_rect = crop_rect_default;
>>

Thanks,
Dafna

2020-01-10 17:23:52

by Pedro Terra Delboni

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

Hello!

On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
<[email protected]> wrote:
>
> Hi,
>
>
> On 30.12.19 14:59, Helen Koike wrote:
> > Hi,
> >
> > Thanks for the patch, just minor comments below.
> >
> > On 12/29/19 5:42 PM, Pedro Terra wrote:
> >> Modify the scaler subdevice to accept setting the resolution of the source
> >> pad (previously the source resolution would always be 3 times the sink for
> >> both dimensions). Now any resolution can be set at src (even smaller ones)
> >> and the sink video will be scaled to match it.
> >>
> >> Test example: With the vimc module up (using the default vimc topology)
> >> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> >> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> >> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> >> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
> >> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> >> v4l2-ctl -d /dev/video2 -v width=300,height=700
> >> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
> >> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
> >> --stream-to=test.raw
> >> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> >> -video_size "300x700" test.raw
> >>
> >> Co-developed-by: Gabriela Bittencourt <[email protected]>
> >> Signed-off-by: Gabriela Bittencourt <[email protected]>
> >> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
> >> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
> >> Signed-off-by: Pedro "pirate" Terra <[email protected]>
> >>
> >> ---
> >>
> >> Changes in V4:
> >> * Rebased with media/master
> >> * Scaling is now compatible with crop
> >> * Updated test example at the commit message
> >> * Add vimc prefix to the pad enumeration
> >>
> >> Changes in V3:
> >> * Corrections suggested by Hans:
> >> - Default scaling factor is now 1 (we removed the define and
> >> set the source format equals the sink).
> >> - Removed SCA_COUNT (enum that represents the number of pads)
> >> as there always 2
> >> - Swapped the per byte pixel copy to memcpy.
> >> * Corrections suggested by Dafna:
> >> - Removed from the documentation the old scaler parameter which
> >> isn't necessary anymore.
> >> * Added a thank you note at the end of the email
> >>
> >> Changes in V2:
> >> * Patch was not sent to media list mail for some reason (even though it
> >> was on the Cc list), trying again.
> >> * Updating documentation.
> >>
> >> Hello!
> >> This code is the result of friends getting together with too much
> >> coffee, sugar and beer trying to get started with some kernel coding.
> >> Please, don't go easy on us! s2
> >>
> >> Running
> >> /usr/local/bin/v4l2-compliance -m /dev/media0
> >> Gave the following result:
> >> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
> >> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
> >> ---
> >> Documentation/media/v4l-drivers/vimc.rst | 21 +-
> >> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
> >> 2 files changed, 87 insertions(+), 182 deletions(-)
> >>
> >> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
> >> index 8f5d7f8d83bb..af04ebbd4fa1 100644
> >> --- a/Documentation/media/v4l-drivers/vimc.rst
> >> +++ b/Documentation/media/v4l-drivers/vimc.rst
> >> @@ -61,9 +61,11 @@ vimc-debayer:
> >> * 1 Pad source
> >>
> >> vimc-scaler:
> >> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
> >> - 1920x1440 image. (this value can be configured, see at
> >> - `Module options`_).
> >> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
> >> +is configured to 360x480 and the source to 1280x720, the image will be stretched
> >> +to fit the source resolution. Works for any resolution within the vimc
> >> +limitations (even shrinking the image if necessary).
> >> +
> >> Exposes:
> >>
> >> * 1 Pad sink
> >> @@ -76,19 +78,6 @@ vimc-capture:
> >> * 1 Pad sink
> >> * 1 Pad source
> >>
> >> -
> >> -Module options
> >> ---------------
> >> -
> >> -Vimc has a module parameter to configure the driver.
> >> -
> >> -* ``sca_mult=<unsigned int>``
> >> -
> >> - Image size multiplier factor to be used to multiply both width and
> >> - height, so the image size will be ``sca_mult^2`` bigger than the
> >> - original one. Currently, only supports scaling up (the default value
> >> - is 3).
> >> -
> >> Source code documentation
> >> -------------------------
> >>
> >> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> >> index e2e551bc3ded..785009b7ac9e 100644
> >> --- a/drivers/media/platform/vimc/vimc-scaler.c
> >> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> >> @@ -6,6 +6,7 @@
> >> */
> >>
> >> #include <linux/moduleparam.h>
> >> +#include <linux/string.h>
> >> #include <linux/vmalloc.h>
> >> #include <linux/v4l2-mediabus.h>
> >> #include <media/v4l2-rect.h>
> >> @@ -13,11 +14,11 @@
> >>
> >> #include "vimc-common.h"
> >>
> >> -static unsigned int sca_mult = 3;
> >> -module_param(sca_mult, uint, 0000);
> >> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >> -
> >> -#define MAX_ZOOM 8
> >> +/* Pad identifier */
> >> +enum vimc_sca_pad {
> >> + VIMC_SCA_SINK = 0,
> >> + VIMC_SCA_SRC = 1,
> >> +};
> >>
> >> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
> >> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
> >> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >> struct vimc_sca_device {
> >> struct vimc_ent_device ved;
> >> struct v4l2_subdev sd;
> >> - /* NOTE: the source fmt is the same as the sink
> >> - * with the width and hight multiplied by mult
> >> - */
> >> - struct v4l2_mbus_framefmt sink_fmt;
> >> struct v4l2_rect crop_rect;
> >> + /* Frame format for both sink and src pad */
> >> + struct v4l2_mbus_framefmt fmt[2];
> >> /* Values calculated when the stream starts */
> >> u8 *src_frame;
> >> - unsigned int src_line_size;
> >> unsigned int bpp;
> >> struct media_pad pads[2];
> >> };
> >> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> >> struct v4l2_rect *r;
> >> unsigned int i;
> >>
> >> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> >> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> >> *mf = sink_fmt_default;
> >>
> >> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
> >> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >> *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;
> >> - mf->width = mf->width * sca_mult;
> >> - mf->height = mf->height * sca_mult;
> >> }
> >>
> >> return 0;
> >> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
> >>
> >> fse->min_width = VIMC_FRAME_MIN_WIDTH;
> >> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
> >> -
> >> - if (VIMC_IS_SINK(fse->pad)) {
> >> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >> - } else {
> >> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
> >> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
> >> - }
> >> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>
> >> return 0;
> >> }
> >> @@ -154,95 +144,73 @@ 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 */
> >> - 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 = crop_rect->width * sca_mult;
> >> - format->format.height = crop_rect->height * sca_mult;
> >> - }
> >> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> >> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
> >> + format->pad);
> >> + else
> >> + format->format = vsca->fmt[format->pad];
> >>
> >> return 0;
> >> }
> >>
> >> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> >> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
> >> {
> >> - const struct vimc_pix_map *vpix;
> >> + if (pad == VIMC_SCA_SINK) {
> >> + const struct vimc_pix_map *vpix;
> >>
> >> - /* Only accept code in the pix map table in non bayer format */
> >> - vpix = vimc_pix_map_by_code(fmt->code);
> >> - if (!vpix || vpix->bayer)
> >> - fmt->code = sink_fmt_default.code;
> >> + /* Only accept code in the pix map table in non bayer format */
> >> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
> >> + if (!vpix || vpix->bayer)
> >> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
> >> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
> >> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
> >>
> >> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> >> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
> >> + }
> >> +
> >> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
> >> VIMC_FRAME_MAX_WIDTH) & ~1;
> >
> > Could you fix the alignment here?
> > For some reason checkpatch doesn't catch this :(
> >
> >> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
> >> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
> >> VIMC_FRAME_MAX_HEIGHT) & ~1;
> >
> > Also here.
> >
> >>
> >> - if (fmt->field == V4L2_FIELD_ANY)
> >> - fmt->field = sink_fmt_default.field;
> >> -
> >> - vimc_colorimetry_clamp(fmt);
> >> + /* Assure src pad attributes besides dimensions are the same as sink */
> >> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
> >> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
> >> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
> >
> > Ideally we should propagate all the other fields to src. Maybe save width and height to
> > a tmp var, assing the whole sink fmt to src, and restore width and height.
> >
> Acctually according to the subdevices documentation, when changing the
> sink format, the width and height of the src format should reset to the
> same values:
>
> ""
> - Sub-devices that scale frames using variable scaling factors should
> reset the scale factors to default values when sink pads formats are
> modified. If the 1:1 scaling ratio is supported, this means that
> source pads formats should be reset to the sink pads formats.
> ""

I have a small question: Should I worry about the crop? I believe that
in the current
implementation setting the sink does not necessarily reset the crop zone.
Should we reset to the sink resolution or to the one determined by the crop?
With that said, the way we implemented the scaller, setting a crop
does not affect the
source resolution (it retains the sink dimensions), should we change this too?
> >
> >
> > With these changes:
> > Acked-by: Helen Koike <[email protected]>
> >
> > Thanks
> > Helen
> >
> >
> >> }
> >>
> >> static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> >> struct v4l2_subdev_pad_config *cfg,
> >> - struct v4l2_subdev_format *fmt)
> >> + struct v4l2_subdev_format *format)
> >> {
> >> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> >> - struct v4l2_mbus_framefmt *sink_fmt;
> >> + struct v4l2_mbus_framefmt *fmt[2];
> >> struct v4l2_rect *crop_rect;
> >>
> >> - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >> /* Do not change the format while stream is on */
> >> if (vsca->src_frame)
> >> return -EBUSY;
> >>
> >> - sink_fmt = &vsca->sink_fmt;
> >> + fmt[VIMC_SCA_SINK] = &vsca->fmt[VIMC_SCA_SINK];
> >> + fmt[VIMC_SCA_SRC] = &vsca->fmt[VIMC_SCA_SRC];
> >> 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);
> >> + fmt[VIMC_SCA_SINK] = v4l2_subdev_get_try_format(sd, cfg,
> >> + VIMC_SCA_SINK);
> >> + fmt[VIMC_SCA_SRC] = v4l2_subdev_get_try_format(sd, cfg,
> >> + VIMC_SCA_SRC);
> >> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >> }
> >>
> >> - /*
> >> - * Do not change the format of the source pad,
> >> - * it is propagated from the sink
> >> - */
> >> - if (VIMC_IS_SRC(fmt->pad)) {
> >> - fmt->format = *sink_fmt;
> >> - 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);
> >> -
> >> - dev_dbg(vsca->ved.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,
> >> - /* old */
> >> - sink_fmt->width, sink_fmt->height, sink_fmt->code,
> >> - sink_fmt->colorspace, sink_fmt->quantization,
> >> - sink_fmt->xfer_func, sink_fmt->ycbcr_enc,
> >> - /* new */
> >> - fmt->format.width, fmt->format.height, fmt->format.code,
> >> - fmt->format.colorspace, fmt->format.quantization,
> >> - fmt->format.xfer_func, fmt->format.ycbcr_enc);
> >> -
> >> - *sink_fmt = fmt->format;
> >> + *fmt[format->pad] = format->format;
> >> + vimc_sca_adjust_fmt(fmt, format->pad);
> >> + format->format = *fmt[format->pad];
> >>
> >> + if (format->pad == VIMC_SCA_SINK)
> >> /* Do the crop, but respect the current bounds */
> >> - vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
> >> - }
> >> + vimc_sca_adjust_sink_crop(crop_rect, fmt[VIMC_SCA_SINK]);
> >>
> >> return 0;
> >> }
> >> @@ -259,11 +227,11 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
> >> return -EINVAL;
> >>
> >> if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >> - sink_fmt = &vsca->sink_fmt;
> >> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
> >> 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);
> >> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> >> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >> }
> >>
> >> switch (sel->target) {
> >> @@ -297,10 +265,10 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
> >> return -EBUSY;
> >>
> >> crop_rect = &vsca->crop_rect;
> >> - sink_fmt = &vsca->sink_fmt;
> >> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
> >> } else {
> >> - crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
> >> - sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> >> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> >> }
> >>
> >> switch (sel->target) {
> >> @@ -338,16 +306,12 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
> >> return 0;
> >>
> >> /* Save the bytes per pixel of the sink */
> >> - vpix = vimc_pix_map_by_code(vsca->sink_fmt.code);
> >> + vpix = vimc_pix_map_by_code(vsca->fmt[VIMC_SCA_SINK].code);
> >> vsca->bpp = vpix->bpp;
> >>
> >> - /* Calculate the width in bytes of the src frame */
> >> - 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->crop_rect.height *
> >> - sca_mult;
> >> + frame_size = vsca->fmt[VIMC_SCA_SRC].width
> >> + * vsca->fmt[VIMC_SCA_SRC].height * vsca->bpp;
> >>
> >> /* Allocate the frame buffer. Use vmalloc to be able to
> >> * allocate a large amount of memory
> >> @@ -376,77 +340,28 @@ static const struct v4l2_subdev_ops vimc_sca_ops = {
> >> .video = &vimc_sca_video_ops,
> >> };
> >>
> >> -static void vimc_sca_fill_pix(u8 *const ptr,
> >> - const u8 *const pixel,
> >> - const unsigned int bpp)
> >> -{
> >> - unsigned int i;
> >> -
> >> - /* copy the pixel to the pointer */
> >> - for (i = 0; i < bpp; i++)
> >> - ptr[i] = pixel[i];
> >> -}
> >> -
> >> -static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> >> - unsigned int lin, unsigned int col,
> >> - const u8 *const sink_frame)
> >> -{
> >> - const struct v4l2_rect crop_rect = vsca->crop_rect;
> >> - unsigned int i, j, index;
> >> - const u8 *pixel;
> >> -
> >> - /* Point to the pixel value in position (lin, col) in the sink frame */
> >> - index = VIMC_FRAME_INDEX(lin, col,
> >> - vsca->sink_fmt.width,
> >> - vsca->bpp);
> >> - pixel = &sink_frame[index];
> >> -
> >> - dev_dbg(vsca->ved.dev,
> >> - "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
> >> - vsca->sd.name, lin, col, index);
> >> -
> >> - /* 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,
> >> - 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);
> >> -
> >> - /* Repeat this pixel mult times */
> >> - for (i = 0; i < sca_mult; i++) {
> >> - /* Iterate through each beginning of a
> >> - * pixel repetition in a line
> >> - */
> >> - for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
> >> - dev_dbg(vsca->ved.dev,
> >> - "sca: %s: sca: scale_pix src pos %d\n",
> >> - vsca->sd.name, index + j);
> >> -
> >> - /* copy the pixel to the position index + j */
> >> - vimc_sca_fill_pix(&vsca->src_frame[index + j],
> >> - pixel, vsca->bpp);
> >> - }
> >> -
> >> - /* move the index to the next line */
> >> - index += vsca->src_line_size;
> >> - }
> >> -}
> >> -
> >> static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
> >> const u8 *const sink_frame)
> >> {
> >> - const struct v4l2_rect r = vsca->crop_rect;
> >> - unsigned int i, j;
> >> -
> >> - /* Scale each pixel from the original sink frame */
> >> - /* TODO: implement scale down, only scale up is supported for now */
> >> - 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);
> >> + const struct v4l2_mbus_framefmt *src_fmt = &vsca->fmt[VIMC_SCA_SRC];
> >> + const struct v4l2_rect *r = &vsca->crop_rect;
> >> + unsigned int s_width = vsca->fmt[VIMC_SCA_SINK].width;
> better replace 's_width' with 'snk_width' (s can also indicate 'source')
>
> >> + unsigned int lin, col, index;
> maybe replace 'lin' and 'col' with 'src_lin' 'src_col' and move 'index'
> to inside the loop
> >> + u8 *walker = vsca->src_frame;
> >> +
> >> + /* Set each pixel at the src_frame to its sink_frame equivalent */
> >> + for (lin = 0; lin < src_fmt->height; lin++) {
> >> + for (col = 0; col < src_fmt->width; col++) {
> >> + unsigned int s_lin, s_col;
> the 's_' prefix is confusing, better replace with 'snk_'
> >> +
> >> + s_lin = (lin * r->height) / src_fmt->height + r->top;
> >> + s_col = (col * r->width) / src_fmt->width + r->left;
> >> + index = VIMC_FRAME_INDEX(s_lin, s_col, s_width,
> >> + vsca->bpp);
> >> + memcpy(walker, &sink_frame[index], vsca->bpp);
> >> + walker += vsca->bpp;
> >> + }
> >> + }
> >> }
> >>
> >> static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> >> @@ -498,8 +413,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> >> return NULL;
> >>
> >> /* Initialize ved and sd */
> >> - vsca->pads[0].flags = MEDIA_PAD_FL_SINK;
> >> - vsca->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> >> + vsca->pads[VIMC_SCA_SINK].flags = MEDIA_PAD_FL_SINK;
> >> + vsca->pads[VIMC_SCA_SRC].flags = MEDIA_PAD_FL_SOURCE;
> >>
> >> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
> >> vcfg_name,
> >> @@ -515,7 +430,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> >> vsca->ved.dev = &vimc->pdev.dev;
> >>
> >> /* Initialize the frame format */
> >> - vsca->sink_fmt = sink_fmt_default;
> >> + vsca->fmt[VIMC_SCA_SINK] = sink_fmt_default;
> >> + vsca->fmt[VIMC_SCA_SRC] = sink_fmt_default;
> >>
> >> /* Initialize the crop selection */
> >> vsca->crop_rect = crop_rect_default;
> >>
>
> Thanks,
> Dafna

Thank you!

2020-01-10 17:28:51

by Helen Koike

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
> Hello!
>
> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
> <[email protected]> wrote:
>>
>> Hi,
>>
>>
>> On 30.12.19 14:59, Helen Koike wrote:
>>> Hi,
>>>
>>> Thanks for the patch, just minor comments below.
>>>
>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
>>>> Modify the scaler subdevice to accept setting the resolution of the source
>>>> pad (previously the source resolution would always be 3 times the sink for
>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
>>>> and the sink video will be scaled to match it.
>>>>
>>>> Test example: With the vimc module up (using the default vimc topology)
>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
>>>> --stream-to=test.raw
>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
>>>> -video_size "300x700" test.raw
>>>>
>>>> Co-developed-by: Gabriela Bittencourt <[email protected]>
>>>> Signed-off-by: Gabriela Bittencourt <[email protected]>
>>>> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
>>>> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
>>>> Signed-off-by: Pedro "pirate" Terra <[email protected]>
>>>>
>>>> ---
>>>>
>>>> Changes in V4:
>>>> * Rebased with media/master
>>>> * Scaling is now compatible with crop
>>>> * Updated test example at the commit message
>>>> * Add vimc prefix to the pad enumeration
>>>>
>>>> Changes in V3:
>>>> * Corrections suggested by Hans:
>>>> - Default scaling factor is now 1 (we removed the define and
>>>> set the source format equals the sink).
>>>> - Removed SCA_COUNT (enum that represents the number of pads)
>>>> as there always 2
>>>> - Swapped the per byte pixel copy to memcpy.
>>>> * Corrections suggested by Dafna:
>>>> - Removed from the documentation the old scaler parameter which
>>>> isn't necessary anymore.
>>>> * Added a thank you note at the end of the email
>>>>
>>>> Changes in V2:
>>>> * Patch was not sent to media list mail for some reason (even though it
>>>> was on the Cc list), trying again.
>>>> * Updating documentation.
>>>>
>>>> Hello!
>>>> This code is the result of friends getting together with too much
>>>> coffee, sugar and beer trying to get started with some kernel coding.
>>>> Please, don't go easy on us! s2
>>>>
>>>> Running
>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
>>>> Gave the following result:
>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
>>>> ---
>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
>>>>
>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
>>>> @@ -61,9 +61,11 @@ vimc-debayer:
>>>> * 1 Pad source
>>>>
>>>> vimc-scaler:
>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
>>>> - 1920x1440 image. (this value can be configured, see at
>>>> - `Module options`_).
>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
>>>> +to fit the source resolution. Works for any resolution within the vimc
>>>> +limitations (even shrinking the image if necessary).
>>>> +
>>>> Exposes:
>>>>
>>>> * 1 Pad sink
>>>> @@ -76,19 +78,6 @@ vimc-capture:
>>>> * 1 Pad sink
>>>> * 1 Pad source
>>>>
>>>> -
>>>> -Module options
>>>> ---------------
>>>> -
>>>> -Vimc has a module parameter to configure the driver.
>>>> -
>>>> -* ``sca_mult=<unsigned int>``
>>>> -
>>>> - Image size multiplier factor to be used to multiply both width and
>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
>>>> - original one. Currently, only supports scaling up (the default value
>>>> - is 3).
>>>> -
>>>> Source code documentation
>>>> -------------------------
>>>>
>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>>>> index e2e551bc3ded..785009b7ac9e 100644
>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>>>> @@ -6,6 +6,7 @@
>>>> */
>>>>
>>>> #include <linux/moduleparam.h>
>>>> +#include <linux/string.h>
>>>> #include <linux/vmalloc.h>
>>>> #include <linux/v4l2-mediabus.h>
>>>> #include <media/v4l2-rect.h>
>>>> @@ -13,11 +14,11 @@
>>>>
>>>> #include "vimc-common.h"
>>>>
>>>> -static unsigned int sca_mult = 3;
>>>> -module_param(sca_mult, uint, 0000);
>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>> -
>>>> -#define MAX_ZOOM 8
>>>> +/* Pad identifier */
>>>> +enum vimc_sca_pad {
>>>> + VIMC_SCA_SINK = 0,
>>>> + VIMC_SCA_SRC = 1,
>>>> +};
>>>>
>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>> struct vimc_sca_device {
>>>> struct vimc_ent_device ved;
>>>> struct v4l2_subdev sd;
>>>> - /* NOTE: the source fmt is the same as the sink
>>>> - * with the width and hight multiplied by mult
>>>> - */
>>>> - struct v4l2_mbus_framefmt sink_fmt;
>>>> struct v4l2_rect crop_rect;
>>>> + /* Frame format for both sink and src pad */
>>>> + struct v4l2_mbus_framefmt fmt[2];
>>>> /* Values calculated when the stream starts */
>>>> u8 *src_frame;
>>>> - unsigned int src_line_size;
>>>> unsigned int bpp;
>>>> struct media_pad pads[2];
>>>> };
>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>>>> struct v4l2_rect *r;
>>>> unsigned int i;
>>>>
>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>>>> *mf = sink_fmt_default;
>>>>
>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>> *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;
>>>> - mf->width = mf->width * sca_mult;
>>>> - mf->height = mf->height * sca_mult;
>>>> }
>>>>
>>>> return 0;
>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>>>>
>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
>>>> -
>>>> - if (VIMC_IS_SINK(fse->pad)) {
>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>> - } else {
>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
>>>> - }
>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>
>>>> return 0;
>>>> }
>>>> @@ -154,95 +144,73 @@ 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 */
>>>> - 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 = crop_rect->width * sca_mult;
>>>> - format->format.height = crop_rect->height * sca_mult;
>>>> - }
>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
>>>> + format->pad);
>>>> + else
>>>> + format->format = vsca->fmt[format->pad];
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
>>>> {
>>>> - const struct vimc_pix_map *vpix;
>>>> + if (pad == VIMC_SCA_SINK) {
>>>> + const struct vimc_pix_map *vpix;
>>>>
>>>> - /* Only accept code in the pix map table in non bayer format */
>>>> - vpix = vimc_pix_map_by_code(fmt->code);
>>>> - if (!vpix || vpix->bayer)
>>>> - fmt->code = sink_fmt_default.code;
>>>> + /* Only accept code in the pix map table in non bayer format */
>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
>>>> + if (!vpix || vpix->bayer)
>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
>>>>
>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
>>>> + }
>>>> +
>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
>>>
>>> Could you fix the alignment here?
>>> For some reason checkpatch doesn't catch this :(
>>>
>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
>>>
>>> Also here.
>>>
>>>>
>>>> - if (fmt->field == V4L2_FIELD_ANY)
>>>> - fmt->field = sink_fmt_default.field;
>>>> -
>>>> - vimc_colorimetry_clamp(fmt);
>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
>>>
>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
>>>
>> Acctually according to the subdevices documentation, when changing the
>> sink format, the width and height of the src format should reset to the
>> same values:
>>
>> ""
>> - Sub-devices that scale frames using variable scaling factors should
>> reset the scale factors to default values when sink pads formats are
>> modified. If the 1:1 scaling ratio is supported, this means that
>> source pads formats should be reset to the sink pads formats.
>> ""
>
> I have a small question: Should I worry about the crop? I believe that
> in the current
> implementation setting the sink does not necessarily reset the crop zone.
> Should we reset to the sink resolution or to the one determined by the crop?
> With that said, the way we implemented the scaller, setting a crop
> does not affect the
> source resolution (it retains the sink dimensions), should we change this too?

From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
have crop in the sink, then the source pad should have dimentions of the crop.

At least that is my understanding, and the docs should be updated to make it more
clear.

I would like to hear other people's opinions on this.

Helen

>>>
>>>
>>> With these changes:
>>> Acked-by: Helen Koike <[email protected]>
>>>
>>> Thanks
>>> Helen
>>>
>>>
>>>> }
>>>>
>>>> static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>>>> struct v4l2_subdev_pad_config *cfg,
>>>> - struct v4l2_subdev_format *fmt)
>>>> + struct v4l2_subdev_format *format)
>>>> {
>>>> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
>>>> - struct v4l2_mbus_framefmt *sink_fmt;
>>>> + struct v4l2_mbus_framefmt *fmt[2];
>>>> struct v4l2_rect *crop_rect;
>>>>
>>>> - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>>> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>>> /* Do not change the format while stream is on */
>>>> if (vsca->src_frame)
>>>> return -EBUSY;
>>>>
>>>> - sink_fmt = &vsca->sink_fmt;
>>>> + fmt[VIMC_SCA_SINK] = &vsca->fmt[VIMC_SCA_SINK];
>>>> + fmt[VIMC_SCA_SRC] = &vsca->fmt[VIMC_SCA_SRC];
>>>> 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);
>>>> + fmt[VIMC_SCA_SINK] = v4l2_subdev_get_try_format(sd, cfg,
>>>> + VIMC_SCA_SINK);
>>>> + fmt[VIMC_SCA_SRC] = v4l2_subdev_get_try_format(sd, cfg,
>>>> + VIMC_SCA_SRC);
>>>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>> }
>>>>
>>>> - /*
>>>> - * Do not change the format of the source pad,
>>>> - * it is propagated from the sink
>>>> - */
>>>> - if (VIMC_IS_SRC(fmt->pad)) {
>>>> - fmt->format = *sink_fmt;
>>>> - 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);
>>>> -
>>>> - dev_dbg(vsca->ved.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,
>>>> - /* old */
>>>> - sink_fmt->width, sink_fmt->height, sink_fmt->code,
>>>> - sink_fmt->colorspace, sink_fmt->quantization,
>>>> - sink_fmt->xfer_func, sink_fmt->ycbcr_enc,
>>>> - /* new */
>>>> - fmt->format.width, fmt->format.height, fmt->format.code,
>>>> - fmt->format.colorspace, fmt->format.quantization,
>>>> - fmt->format.xfer_func, fmt->format.ycbcr_enc);
>>>> -
>>>> - *sink_fmt = fmt->format;
>>>> + *fmt[format->pad] = format->format;
>>>> + vimc_sca_adjust_fmt(fmt, format->pad);
>>>> + format->format = *fmt[format->pad];
>>>>
>>>> + if (format->pad == VIMC_SCA_SINK)
>>>> /* Do the crop, but respect the current bounds */
>>>> - vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
>>>> - }
>>>> + vimc_sca_adjust_sink_crop(crop_rect, fmt[VIMC_SCA_SINK]);
>>>>
>>>> return 0;
>>>> }
>>>> @@ -259,11 +227,11 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
>>>> return -EINVAL;
>>>>
>>>> if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>>> - sink_fmt = &vsca->sink_fmt;
>>>> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
>>>> 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);
>>>> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>>>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>> }
>>>>
>>>> switch (sel->target) {
>>>> @@ -297,10 +265,10 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
>>>> return -EBUSY;
>>>>
>>>> crop_rect = &vsca->crop_rect;
>>>> - sink_fmt = &vsca->sink_fmt;
>>>> + sink_fmt = &vsca->fmt[VIMC_SCA_SINK];
>>>> } else {
>>>> - crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>>> - sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
>>>> + crop_rect = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>> + sink_fmt = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>>>> }
>>>>
>>>> switch (sel->target) {
>>>> @@ -338,16 +306,12 @@ static int vimc_sca_s_stream(struct v4l2_subdev *sd, int enable)
>>>> return 0;
>>>>
>>>> /* Save the bytes per pixel of the sink */
>>>> - vpix = vimc_pix_map_by_code(vsca->sink_fmt.code);
>>>> + vpix = vimc_pix_map_by_code(vsca->fmt[VIMC_SCA_SINK].code);
>>>> vsca->bpp = vpix->bpp;
>>>>
>>>> - /* Calculate the width in bytes of the src frame */
>>>> - 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->crop_rect.height *
>>>> - sca_mult;
>>>> + frame_size = vsca->fmt[VIMC_SCA_SRC].width
>>>> + * vsca->fmt[VIMC_SCA_SRC].height * vsca->bpp;
>>>>
>>>> /* Allocate the frame buffer. Use vmalloc to be able to
>>>> * allocate a large amount of memory
>>>> @@ -376,77 +340,28 @@ static const struct v4l2_subdev_ops vimc_sca_ops = {
>>>> .video = &vimc_sca_video_ops,
>>>> };
>>>>
>>>> -static void vimc_sca_fill_pix(u8 *const ptr,
>>>> - const u8 *const pixel,
>>>> - const unsigned int bpp)
>>>> -{
>>>> - unsigned int i;
>>>> -
>>>> - /* copy the pixel to the pointer */
>>>> - for (i = 0; i < bpp; i++)
>>>> - ptr[i] = pixel[i];
>>>> -}
>>>> -
>>>> -static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
>>>> - unsigned int lin, unsigned int col,
>>>> - const u8 *const sink_frame)
>>>> -{
>>>> - const struct v4l2_rect crop_rect = vsca->crop_rect;
>>>> - unsigned int i, j, index;
>>>> - const u8 *pixel;
>>>> -
>>>> - /* Point to the pixel value in position (lin, col) in the sink frame */
>>>> - index = VIMC_FRAME_INDEX(lin, col,
>>>> - vsca->sink_fmt.width,
>>>> - vsca->bpp);
>>>> - pixel = &sink_frame[index];
>>>> -
>>>> - dev_dbg(vsca->ved.dev,
>>>> - "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
>>>> - vsca->sd.name, lin, col, index);
>>>> -
>>>> - /* 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,
>>>> - 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);
>>>> -
>>>> - /* Repeat this pixel mult times */
>>>> - for (i = 0; i < sca_mult; i++) {
>>>> - /* Iterate through each beginning of a
>>>> - * pixel repetition in a line
>>>> - */
>>>> - for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
>>>> - dev_dbg(vsca->ved.dev,
>>>> - "sca: %s: sca: scale_pix src pos %d\n",
>>>> - vsca->sd.name, index + j);
>>>> -
>>>> - /* copy the pixel to the position index + j */
>>>> - vimc_sca_fill_pix(&vsca->src_frame[index + j],
>>>> - pixel, vsca->bpp);
>>>> - }
>>>> -
>>>> - /* move the index to the next line */
>>>> - index += vsca->src_line_size;
>>>> - }
>>>> -}
>>>> -
>>>> static void vimc_sca_fill_src_frame(const struct vimc_sca_device *const vsca,
>>>> const u8 *const sink_frame)
>>>> {
>>>> - const struct v4l2_rect r = vsca->crop_rect;
>>>> - unsigned int i, j;
>>>> -
>>>> - /* Scale each pixel from the original sink frame */
>>>> - /* TODO: implement scale down, only scale up is supported for now */
>>>> - 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);
>>>> + const struct v4l2_mbus_framefmt *src_fmt = &vsca->fmt[VIMC_SCA_SRC];
>>>> + const struct v4l2_rect *r = &vsca->crop_rect;
>>>> + unsigned int s_width = vsca->fmt[VIMC_SCA_SINK].width;
>> better replace 's_width' with 'snk_width' (s can also indicate 'source')
>>
>>>> + unsigned int lin, col, index;
>> maybe replace 'lin' and 'col' with 'src_lin' 'src_col' and move 'index'
>> to inside the loop
>>>> + u8 *walker = vsca->src_frame;
>>>> +
>>>> + /* Set each pixel at the src_frame to its sink_frame equivalent */
>>>> + for (lin = 0; lin < src_fmt->height; lin++) {
>>>> + for (col = 0; col < src_fmt->width; col++) {
>>>> + unsigned int s_lin, s_col;
>> the 's_' prefix is confusing, better replace with 'snk_'
>>>> +
>>>> + s_lin = (lin * r->height) / src_fmt->height + r->top;
>>>> + s_col = (col * r->width) / src_fmt->width + r->left;
>>>> + index = VIMC_FRAME_INDEX(s_lin, s_col, s_width,
>>>> + vsca->bpp);
>>>> + memcpy(walker, &sink_frame[index], vsca->bpp);
>>>> + walker += vsca->bpp;
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>>>> @@ -498,8 +413,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>>>> return NULL;
>>>>
>>>> /* Initialize ved and sd */
>>>> - vsca->pads[0].flags = MEDIA_PAD_FL_SINK;
>>>> - vsca->pads[1].flags = MEDIA_PAD_FL_SOURCE;
>>>> + vsca->pads[VIMC_SCA_SINK].flags = MEDIA_PAD_FL_SINK;
>>>> + vsca->pads[VIMC_SCA_SRC].flags = MEDIA_PAD_FL_SOURCE;
>>>>
>>>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
>>>> vcfg_name,
>>>> @@ -515,7 +430,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>>>> vsca->ved.dev = &vimc->pdev.dev;
>>>>
>>>> /* Initialize the frame format */
>>>> - vsca->sink_fmt = sink_fmt_default;
>>>> + vsca->fmt[VIMC_SCA_SINK] = sink_fmt_default;
>>>> + vsca->fmt[VIMC_SCA_SRC] = sink_fmt_default;
>>>>
>>>> /* Initialize the crop selection */
>>>> vsca->crop_rect = crop_rect_default;
>>>>
>>
>> Thanks,
>> Dafna
>
> Thank you!
>

2020-01-15 14:52:23

by Hans Verkuil

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

On 1/10/20 6:26 PM, Helen Koike wrote:
>
>
> On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
>> Hello!
>>
>> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
>> <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 30.12.19 14:59, Helen Koike wrote:
>>>> Hi,
>>>>
>>>> Thanks for the patch, just minor comments below.
>>>>
>>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
>>>>> Modify the scaler subdevice to accept setting the resolution of the source
>>>>> pad (previously the source resolution would always be 3 times the sink for
>>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
>>>>> and the sink video will be scaled to match it.
>>>>>
>>>>> Test example: With the vimc module up (using the default vimc topology)
>>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
>>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
>>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
>>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
>>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
>>>>> --stream-to=test.raw
>>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
>>>>> -video_size "300x700" test.raw
>>>>>
>>>>> Co-developed-by: Gabriela Bittencourt <[email protected]>
>>>>> Signed-off-by: Gabriela Bittencourt <[email protected]>
>>>>> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
>>>>> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
>>>>> Signed-off-by: Pedro "pirate" Terra <[email protected]>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in V4:
>>>>> * Rebased with media/master
>>>>> * Scaling is now compatible with crop
>>>>> * Updated test example at the commit message
>>>>> * Add vimc prefix to the pad enumeration
>>>>>
>>>>> Changes in V3:
>>>>> * Corrections suggested by Hans:
>>>>> - Default scaling factor is now 1 (we removed the define and
>>>>> set the source format equals the sink).
>>>>> - Removed SCA_COUNT (enum that represents the number of pads)
>>>>> as there always 2
>>>>> - Swapped the per byte pixel copy to memcpy.
>>>>> * Corrections suggested by Dafna:
>>>>> - Removed from the documentation the old scaler parameter which
>>>>> isn't necessary anymore.
>>>>> * Added a thank you note at the end of the email
>>>>>
>>>>> Changes in V2:
>>>>> * Patch was not sent to media list mail for some reason (even though it
>>>>> was on the Cc list), trying again.
>>>>> * Updating documentation.
>>>>>
>>>>> Hello!
>>>>> This code is the result of friends getting together with too much
>>>>> coffee, sugar and beer trying to get started with some kernel coding.
>>>>> Please, don't go easy on us! s2
>>>>>
>>>>> Running
>>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
>>>>> Gave the following result:
>>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
>>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
>>>>> ---
>>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
>>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
>>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
>>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
>>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
>>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
>>>>> @@ -61,9 +61,11 @@ vimc-debayer:
>>>>> * 1 Pad source
>>>>>
>>>>> vimc-scaler:
>>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
>>>>> - 1920x1440 image. (this value can be configured, see at
>>>>> - `Module options`_).
>>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
>>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
>>>>> +to fit the source resolution. Works for any resolution within the vimc
>>>>> +limitations (even shrinking the image if necessary).
>>>>> +
>>>>> Exposes:
>>>>>
>>>>> * 1 Pad sink
>>>>> @@ -76,19 +78,6 @@ vimc-capture:
>>>>> * 1 Pad sink
>>>>> * 1 Pad source
>>>>>
>>>>> -
>>>>> -Module options
>>>>> ---------------
>>>>> -
>>>>> -Vimc has a module parameter to configure the driver.
>>>>> -
>>>>> -* ``sca_mult=<unsigned int>``
>>>>> -
>>>>> - Image size multiplier factor to be used to multiply both width and
>>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
>>>>> - original one. Currently, only supports scaling up (the default value
>>>>> - is 3).
>>>>> -
>>>>> Source code documentation
>>>>> -------------------------
>>>>>
>>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>>>>> index e2e551bc3ded..785009b7ac9e 100644
>>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>>>>> @@ -6,6 +6,7 @@
>>>>> */
>>>>>
>>>>> #include <linux/moduleparam.h>
>>>>> +#include <linux/string.h>
>>>>> #include <linux/vmalloc.h>
>>>>> #include <linux/v4l2-mediabus.h>
>>>>> #include <media/v4l2-rect.h>
>>>>> @@ -13,11 +14,11 @@
>>>>>
>>>>> #include "vimc-common.h"
>>>>>
>>>>> -static unsigned int sca_mult = 3;
>>>>> -module_param(sca_mult, uint, 0000);
>>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>>> -
>>>>> -#define MAX_ZOOM 8
>>>>> +/* Pad identifier */
>>>>> +enum vimc_sca_pad {
>>>>> + VIMC_SCA_SINK = 0,
>>>>> + VIMC_SCA_SRC = 1,
>>>>> +};
>>>>>
>>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
>>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
>>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>>> struct vimc_sca_device {
>>>>> struct vimc_ent_device ved;
>>>>> struct v4l2_subdev sd;
>>>>> - /* NOTE: the source fmt is the same as the sink
>>>>> - * with the width and hight multiplied by mult
>>>>> - */
>>>>> - struct v4l2_mbus_framefmt sink_fmt;
>>>>> struct v4l2_rect crop_rect;
>>>>> + /* Frame format for both sink and src pad */
>>>>> + struct v4l2_mbus_framefmt fmt[2];
>>>>> /* Values calculated when the stream starts */
>>>>> u8 *src_frame;
>>>>> - unsigned int src_line_size;
>>>>> unsigned int bpp;
>>>>> struct media_pad pads[2];
>>>>> };
>>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>>>>> struct v4l2_rect *r;
>>>>> unsigned int i;
>>>>>
>>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>>>>> *mf = sink_fmt_default;
>>>>>
>>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>>> *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;
>>>>> - mf->width = mf->width * sca_mult;
>>>>> - mf->height = mf->height * sca_mult;
>>>>> }
>>>>>
>>>>> return 0;
>>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>>>>>
>>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
>>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
>>>>> -
>>>>> - if (VIMC_IS_SINK(fse->pad)) {
>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>> - } else {
>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
>>>>> - }
>>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -154,95 +144,73 @@ 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 */
>>>>> - 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 = crop_rect->width * sca_mult;
>>>>> - format->format.height = crop_rect->height * sca_mult;
>>>>> - }
>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
>>>>> + format->pad);
>>>>> + else
>>>>> + format->format = vsca->fmt[format->pad];
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
>>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
>>>>> {
>>>>> - const struct vimc_pix_map *vpix;
>>>>> + if (pad == VIMC_SCA_SINK) {
>>>>> + const struct vimc_pix_map *vpix;
>>>>>
>>>>> - /* Only accept code in the pix map table in non bayer format */
>>>>> - vpix = vimc_pix_map_by_code(fmt->code);
>>>>> - if (!vpix || vpix->bayer)
>>>>> - fmt->code = sink_fmt_default.code;
>>>>> + /* Only accept code in the pix map table in non bayer format */
>>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
>>>>> + if (!vpix || vpix->bayer)
>>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
>>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
>>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
>>>>>
>>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
>>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
>>>>> + }
>>>>> +
>>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
>>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
>>>>
>>>> Could you fix the alignment here?
>>>> For some reason checkpatch doesn't catch this :(
>>>>
>>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
>>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
>>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
>>>>
>>>> Also here.
>>>>
>>>>>
>>>>> - if (fmt->field == V4L2_FIELD_ANY)
>>>>> - fmt->field = sink_fmt_default.field;
>>>>> -
>>>>> - vimc_colorimetry_clamp(fmt);
>>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
>>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
>>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
>>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
>>>>
>>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
>>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
>>>>
>>> Acctually according to the subdevices documentation, when changing the
>>> sink format, the width and height of the src format should reset to the
>>> same values:
>>>
>>> ""
>>> - Sub-devices that scale frames using variable scaling factors should
>>> reset the scale factors to default values when sink pads formats are
>>> modified. If the 1:1 scaling ratio is supported, this means that
>>> source pads formats should be reset to the sink pads formats.
>>> ""
>>
>> I have a small question: Should I worry about the crop? I believe that
>> in the current
>> implementation setting the sink does not necessarily reset the crop zone.
>> Should we reset to the sink resolution or to the one determined by the crop?
>> With that said, the way we implemented the scaller, setting a crop
>> does not affect the
>> source resolution (it retains the sink dimensions), should we change this too?
>
> From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
> have crop in the sink, then the source pad should have dimentions of the crop.

Correct.

>
> At least that is my understanding, and the docs should be updated to make it more
> clear.

Patches are welcome :-)

>
> I would like to hear other people's opinions on this.

I agree with you, this is my understanding as well.

Regards,

Hans

2020-01-17 21:59:27

by Pedro Terra Delboni

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

Sorry to keep bumping this thread, but while doing some testing I got
confused with the following:
The documentation mentions that the scaler should be reset whenever
the sink format is set.
Does this mean that I should reset it independently if the sink set
changes the dimensions?
For example: if I change the pixels from RGB to another format, should
that also reset the source dimensions?

Another thing that got me confused was:
At by the functions names, setting the crop dimensions is not
considered changing the pad format, it's considered setting a
selection.
Should I also update the set_selection function to propagate the
changes to the source pad?
Otherwise, by changing the crop (without changing the sink format)
will cause the scaling to behave in the same way it would if we didn't
propagate the sink properties to the source.

So:
Should I check if the set_fmt is actually changing the sink dimensions
in order to propagate them to the source?
Should I also propagate the dimensions when setting the sink crop?

Sorry for the long email!
Thanks for the attention that you've all been giving us so far!

On Wed, Jan 15, 2020 at 11:51 AM Hans Verkuil <[email protected]> wrote:
>
> On 1/10/20 6:26 PM, Helen Koike wrote:
> >
> >
> > On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
> >> Hello!
> >>
> >> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
> >> <[email protected]> wrote:
> >>>
> >>> Hi,
> >>>
> >>>
> >>> On 30.12.19 14:59, Helen Koike wrote:
> >>>> Hi,
> >>>>
> >>>> Thanks for the patch, just minor comments below.
> >>>>
> >>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
> >>>>> Modify the scaler subdevice to accept setting the resolution of the source
> >>>>> pad (previously the source resolution would always be 3 times the sink for
> >>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
> >>>>> and the sink video will be scaled to match it.
> >>>>>
> >>>>> Test example: With the vimc module up (using the default vimc topology)
> >>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> >>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> >>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> >>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
> >>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> >>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
> >>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
> >>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
> >>>>> --stream-to=test.raw
> >>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> >>>>> -video_size "300x700" test.raw
> >>>>>
> >>>>> Co-developed-by: Gabriela Bittencourt <[email protected]>
> >>>>> Signed-off-by: Gabriela Bittencourt <[email protected]>
> >>>>> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
> >>>>> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
> >>>>> Signed-off-by: Pedro "pirate" Terra <[email protected]>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Changes in V4:
> >>>>> * Rebased with media/master
> >>>>> * Scaling is now compatible with crop
> >>>>> * Updated test example at the commit message
> >>>>> * Add vimc prefix to the pad enumeration
> >>>>>
> >>>>> Changes in V3:
> >>>>> * Corrections suggested by Hans:
> >>>>> - Default scaling factor is now 1 (we removed the define and
> >>>>> set the source format equals the sink).
> >>>>> - Removed SCA_COUNT (enum that represents the number of pads)
> >>>>> as there always 2
> >>>>> - Swapped the per byte pixel copy to memcpy.
> >>>>> * Corrections suggested by Dafna:
> >>>>> - Removed from the documentation the old scaler parameter which
> >>>>> isn't necessary anymore.
> >>>>> * Added a thank you note at the end of the email
> >>>>>
> >>>>> Changes in V2:
> >>>>> * Patch was not sent to media list mail for some reason (even though it
> >>>>> was on the Cc list), trying again.
> >>>>> * Updating documentation.
> >>>>>
> >>>>> Hello!
> >>>>> This code is the result of friends getting together with too much
> >>>>> coffee, sugar and beer trying to get started with some kernel coding.
> >>>>> Please, don't go easy on us! s2
> >>>>>
> >>>>> Running
> >>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
> >>>>> Gave the following result:
> >>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
> >>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
> >>>>> ---
> >>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
> >>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
> >>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
> >>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
> >>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
> >>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
> >>>>> @@ -61,9 +61,11 @@ vimc-debayer:
> >>>>> * 1 Pad source
> >>>>>
> >>>>> vimc-scaler:
> >>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
> >>>>> - 1920x1440 image. (this value can be configured, see at
> >>>>> - `Module options`_).
> >>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
> >>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
> >>>>> +to fit the source resolution. Works for any resolution within the vimc
> >>>>> +limitations (even shrinking the image if necessary).
> >>>>> +
> >>>>> Exposes:
> >>>>>
> >>>>> * 1 Pad sink
> >>>>> @@ -76,19 +78,6 @@ vimc-capture:
> >>>>> * 1 Pad sink
> >>>>> * 1 Pad source
> >>>>>
> >>>>> -
> >>>>> -Module options
> >>>>> ---------------
> >>>>> -
> >>>>> -Vimc has a module parameter to configure the driver.
> >>>>> -
> >>>>> -* ``sca_mult=<unsigned int>``
> >>>>> -
> >>>>> - Image size multiplier factor to be used to multiply both width and
> >>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
> >>>>> - original one. Currently, only supports scaling up (the default value
> >>>>> - is 3).
> >>>>> -
> >>>>> Source code documentation
> >>>>> -------------------------
> >>>>>
> >>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> >>>>> index e2e551bc3ded..785009b7ac9e 100644
> >>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
> >>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> >>>>> @@ -6,6 +6,7 @@
> >>>>> */
> >>>>>
> >>>>> #include <linux/moduleparam.h>
> >>>>> +#include <linux/string.h>
> >>>>> #include <linux/vmalloc.h>
> >>>>> #include <linux/v4l2-mediabus.h>
> >>>>> #include <media/v4l2-rect.h>
> >>>>> @@ -13,11 +14,11 @@
> >>>>>
> >>>>> #include "vimc-common.h"
> >>>>>
> >>>>> -static unsigned int sca_mult = 3;
> >>>>> -module_param(sca_mult, uint, 0000);
> >>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >>>>> -
> >>>>> -#define MAX_ZOOM 8
> >>>>> +/* Pad identifier */
> >>>>> +enum vimc_sca_pad {
> >>>>> + VIMC_SCA_SINK = 0,
> >>>>> + VIMC_SCA_SRC = 1,
> >>>>> +};
> >>>>>
> >>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
> >>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
> >>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >>>>> struct vimc_sca_device {
> >>>>> struct vimc_ent_device ved;
> >>>>> struct v4l2_subdev sd;
> >>>>> - /* NOTE: the source fmt is the same as the sink
> >>>>> - * with the width and hight multiplied by mult
> >>>>> - */
> >>>>> - struct v4l2_mbus_framefmt sink_fmt;
> >>>>> struct v4l2_rect crop_rect;
> >>>>> + /* Frame format for both sink and src pad */
> >>>>> + struct v4l2_mbus_framefmt fmt[2];
> >>>>> /* Values calculated when the stream starts */
> >>>>> u8 *src_frame;
> >>>>> - unsigned int src_line_size;
> >>>>> unsigned int bpp;
> >>>>> struct media_pad pads[2];
> >>>>> };
> >>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> >>>>> struct v4l2_rect *r;
> >>>>> unsigned int i;
> >>>>>
> >>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> >>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> >>>>> *mf = sink_fmt_default;
> >>>>>
> >>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
> >>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >>>>> *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;
> >>>>> - mf->width = mf->width * sca_mult;
> >>>>> - mf->height = mf->height * sca_mult;
> >>>>> }
> >>>>>
> >>>>> return 0;
> >>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
> >>>>>
> >>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
> >>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
> >>>>> -
> >>>>> - if (VIMC_IS_SINK(fse->pad)) {
> >>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>>>> - } else {
> >>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
> >>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
> >>>>> - }
> >>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>>>>
> >>>>> return 0;
> >>>>> }
> >>>>> @@ -154,95 +144,73 @@ 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 */
> >>>>> - 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 = crop_rect->width * sca_mult;
> >>>>> - format->format.height = crop_rect->height * sca_mult;
> >>>>> - }
> >>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> >>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
> >>>>> + format->pad);
> >>>>> + else
> >>>>> + format->format = vsca->fmt[format->pad];
> >>>>>
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> >>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
> >>>>> {
> >>>>> - const struct vimc_pix_map *vpix;
> >>>>> + if (pad == VIMC_SCA_SINK) {
> >>>>> + const struct vimc_pix_map *vpix;
> >>>>>
> >>>>> - /* Only accept code in the pix map table in non bayer format */
> >>>>> - vpix = vimc_pix_map_by_code(fmt->code);
> >>>>> - if (!vpix || vpix->bayer)
> >>>>> - fmt->code = sink_fmt_default.code;
> >>>>> + /* Only accept code in the pix map table in non bayer format */
> >>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
> >>>>> + if (!vpix || vpix->bayer)
> >>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
> >>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
> >>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
> >>>>>
> >>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> >>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
> >>>>> + }
> >>>>> +
> >>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
> >>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
> >>>>
> >>>> Could you fix the alignment here?
> >>>> For some reason checkpatch doesn't catch this :(
> >>>>
> >>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
> >>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
> >>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
> >>>>
> >>>> Also here.
> >>>>
> >>>>>
> >>>>> - if (fmt->field == V4L2_FIELD_ANY)
> >>>>> - fmt->field = sink_fmt_default.field;
> >>>>> -
> >>>>> - vimc_colorimetry_clamp(fmt);
> >>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
> >>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
> >>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
> >>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
> >>>>
> >>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
> >>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
> >>>>
> >>> Acctually according to the subdevices documentation, when changing the
> >>> sink format, the width and height of the src format should reset to the
> >>> same values:
> >>>
> >>> ""
> >>> - Sub-devices that scale frames using variable scaling factors should
> >>> reset the scale factors to default values when sink pads formats are
> >>> modified. If the 1:1 scaling ratio is supported, this means that
> >>> source pads formats should be reset to the sink pads formats.
> >>> ""
> >>
> >> I have a small question: Should I worry about the crop? I believe that
> >> in the current
> >> implementation setting the sink does not necessarily reset the crop zone.
> >> Should we reset to the sink resolution or to the one determined by the crop?
> >> With that said, the way we implemented the scaller, setting a crop
> >> does not affect the
> >> source resolution (it retains the sink dimensions), should we change this too?
> >
> > From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
> > have crop in the sink, then the source pad should have dimentions of the crop.
>
> Correct.
>
> >
> > At least that is my understanding, and the docs should be updated to make it more
> > clear.
>
> Patches are welcome :-)
>
> >
> > I would like to hear other people's opinions on this.
>
> I agree with you, this is my understanding as well.
>
> Regards,
>
> Hans

2020-01-21 13:44:57

by Helen Koike

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

On 1/17/20 7:56 PM, Pedro Terra Delboni wrote:
> Sorry to keep bumping this thread, but while doing some testing I got
> confused with the following:
> The documentation mentions that the scaler should be reset whenever
> the sink format is set.
> Does this mean that I should reset it independently if the sink set
> changes the dimensions?
> For example: if I change the pixels from RGB to another format, should
> that also reset the source dimensions?

Confusing indeed.

My understanding is:
If you make any changes in the sink pad that would affect the scaling
ratio, then you should adjust the source pad to make a scaling ration
1:1.
So, if you only change the pixel format, it shouldn't reset the scaling
ratio.

>
> Another thing that got me confused was:
> At by the functions names, setting the crop dimensions is not
> considered changing the pad format, it's considered setting a
> selection.
> Should I also update the set_selection function to propagate the
> changes to the source pad?

Following my understanding above, yes.

> Otherwise, by changing the crop (without changing the sink format)
> will cause the scaling to behave in the same way it would if we didn't
> propagate the sink properties to the source.
>
> So:
> Should I check if the set_fmt is actually changing the sink dimensions
> in order to propagate them to the source?

Following my understanding above, yes.

> Should I also propagate the dimensions when setting the sink crop?

Following my understanding above, yes.

I wonder now how other drivers are doing this.

At least, I'm sure the driver rkisp1 is not doing the right thing
and needs to be corrected.

Thanks
Helen

>
> Sorry for the long email!
> Thanks for the attention that you've all been giving us so far!
>
> On Wed, Jan 15, 2020 at 11:51 AM Hans Verkuil <[email protected]> wrote:
>>
>> On 1/10/20 6:26 PM, Helen Koike wrote:
>>>
>>>
>>> On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
>>>> Hello!
>>>>
>>>> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
>>>> <[email protected]> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 30.12.19 14:59, Helen Koike wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Thanks for the patch, just minor comments below.
>>>>>>
>>>>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
>>>>>>> Modify the scaler subdevice to accept setting the resolution of the source
>>>>>>> pad (previously the source resolution would always be 3 times the sink for
>>>>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
>>>>>>> and the sink video will be scaled to match it.
>>>>>>>
>>>>>>> Test example: With the vimc module up (using the default vimc topology)
>>>>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>>>>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>>>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
>>>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
>>>>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
>>>>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
>>>>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
>>>>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
>>>>>>> --stream-to=test.raw
>>>>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
>>>>>>> -video_size "300x700" test.raw
>>>>>>>
>>>>>>> Co-developed-by: Gabriela Bittencourt <[email protected]>
>>>>>>> Signed-off-by: Gabriela Bittencourt <[email protected]>
>>>>>>> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
>>>>>>> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
>>>>>>> Signed-off-by: Pedro "pirate" Terra <[email protected]>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in V4:
>>>>>>> * Rebased with media/master
>>>>>>> * Scaling is now compatible with crop
>>>>>>> * Updated test example at the commit message
>>>>>>> * Add vimc prefix to the pad enumeration
>>>>>>>
>>>>>>> Changes in V3:
>>>>>>> * Corrections suggested by Hans:
>>>>>>> - Default scaling factor is now 1 (we removed the define and
>>>>>>> set the source format equals the sink).
>>>>>>> - Removed SCA_COUNT (enum that represents the number of pads)
>>>>>>> as there always 2
>>>>>>> - Swapped the per byte pixel copy to memcpy.
>>>>>>> * Corrections suggested by Dafna:
>>>>>>> - Removed from the documentation the old scaler parameter which
>>>>>>> isn't necessary anymore.
>>>>>>> * Added a thank you note at the end of the email
>>>>>>>
>>>>>>> Changes in V2:
>>>>>>> * Patch was not sent to media list mail for some reason (even though it
>>>>>>> was on the Cc list), trying again.
>>>>>>> * Updating documentation.
>>>>>>>
>>>>>>> Hello!
>>>>>>> This code is the result of friends getting together with too much
>>>>>>> coffee, sugar and beer trying to get started with some kernel coding.
>>>>>>> Please, don't go easy on us! s2
>>>>>>>
>>>>>>> Running
>>>>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
>>>>>>> Gave the following result:
>>>>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
>>>>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
>>>>>>> ---
>>>>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
>>>>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
>>>>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
>>>>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
>>>>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
>>>>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
>>>>>>> @@ -61,9 +61,11 @@ vimc-debayer:
>>>>>>> * 1 Pad source
>>>>>>>
>>>>>>> vimc-scaler:
>>>>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
>>>>>>> - 1920x1440 image. (this value can be configured, see at
>>>>>>> - `Module options`_).
>>>>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
>>>>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
>>>>>>> +to fit the source resolution. Works for any resolution within the vimc
>>>>>>> +limitations (even shrinking the image if necessary).
>>>>>>> +
>>>>>>> Exposes:
>>>>>>>
>>>>>>> * 1 Pad sink
>>>>>>> @@ -76,19 +78,6 @@ vimc-capture:
>>>>>>> * 1 Pad sink
>>>>>>> * 1 Pad source
>>>>>>>
>>>>>>> -
>>>>>>> -Module options
>>>>>>> ---------------
>>>>>>> -
>>>>>>> -Vimc has a module parameter to configure the driver.
>>>>>>> -
>>>>>>> -* ``sca_mult=<unsigned int>``
>>>>>>> -
>>>>>>> - Image size multiplier factor to be used to multiply both width and
>>>>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
>>>>>>> - original one. Currently, only supports scaling up (the default value
>>>>>>> - is 3).
>>>>>>> -
>>>>>>> Source code documentation
>>>>>>> -------------------------
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>>>>>>> index e2e551bc3ded..785009b7ac9e 100644
>>>>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>>>>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>>>>>>> @@ -6,6 +6,7 @@
>>>>>>> */
>>>>>>>
>>>>>>> #include <linux/moduleparam.h>
>>>>>>> +#include <linux/string.h>
>>>>>>> #include <linux/vmalloc.h>
>>>>>>> #include <linux/v4l2-mediabus.h>
>>>>>>> #include <media/v4l2-rect.h>
>>>>>>> @@ -13,11 +14,11 @@
>>>>>>>
>>>>>>> #include "vimc-common.h"
>>>>>>>
>>>>>>> -static unsigned int sca_mult = 3;
>>>>>>> -module_param(sca_mult, uint, 0000);
>>>>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>>>>> -
>>>>>>> -#define MAX_ZOOM 8
>>>>>>> +/* Pad identifier */
>>>>>>> +enum vimc_sca_pad {
>>>>>>> + VIMC_SCA_SINK = 0,
>>>>>>> + VIMC_SCA_SRC = 1,
>>>>>>> +};
>>>>>>>
>>>>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
>>>>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
>>>>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>>>>> struct vimc_sca_device {
>>>>>>> struct vimc_ent_device ved;
>>>>>>> struct v4l2_subdev sd;
>>>>>>> - /* NOTE: the source fmt is the same as the sink
>>>>>>> - * with the width and hight multiplied by mult
>>>>>>> - */
>>>>>>> - struct v4l2_mbus_framefmt sink_fmt;
>>>>>>> struct v4l2_rect crop_rect;
>>>>>>> + /* Frame format for both sink and src pad */
>>>>>>> + struct v4l2_mbus_framefmt fmt[2];
>>>>>>> /* Values calculated when the stream starts */
>>>>>>> u8 *src_frame;
>>>>>>> - unsigned int src_line_size;
>>>>>>> unsigned int bpp;
>>>>>>> struct media_pad pads[2];
>>>>>>> };
>>>>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>>>>>>> struct v4l2_rect *r;
>>>>>>> unsigned int i;
>>>>>>>
>>>>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>>>>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>>>>>>> *mf = sink_fmt_default;
>>>>>>>
>>>>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>>>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>>>>> *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;
>>>>>>> - mf->width = mf->width * sca_mult;
>>>>>>> - mf->height = mf->height * sca_mult;
>>>>>>> }
>>>>>>>
>>>>>>> return 0;
>>>>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>>>>>>>
>>>>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
>>>>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
>>>>>>> -
>>>>>>> - if (VIMC_IS_SINK(fse->pad)) {
>>>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>>>> - } else {
>>>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
>>>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
>>>>>>> - }
>>>>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>> @@ -154,95 +144,73 @@ 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 */
>>>>>>> - 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 = crop_rect->width * sca_mult;
>>>>>>> - format->format.height = crop_rect->height * sca_mult;
>>>>>>> - }
>>>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>>>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
>>>>>>> + format->pad);
>>>>>>> + else
>>>>>>> + format->format = vsca->fmt[format->pad];
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
>>>>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
>>>>>>> {
>>>>>>> - const struct vimc_pix_map *vpix;
>>>>>>> + if (pad == VIMC_SCA_SINK) {
>>>>>>> + const struct vimc_pix_map *vpix;
>>>>>>>
>>>>>>> - /* Only accept code in the pix map table in non bayer format */
>>>>>>> - vpix = vimc_pix_map_by_code(fmt->code);
>>>>>>> - if (!vpix || vpix->bayer)
>>>>>>> - fmt->code = sink_fmt_default.code;
>>>>>>> + /* Only accept code in the pix map table in non bayer format */
>>>>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
>>>>>>> + if (!vpix || vpix->bayer)
>>>>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
>>>>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
>>>>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
>>>>>>>
>>>>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
>>>>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
>>>>>>> + }
>>>>>>> +
>>>>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
>>>>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
>>>>>>
>>>>>> Could you fix the alignment here?
>>>>>> For some reason checkpatch doesn't catch this :(
>>>>>>
>>>>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
>>>>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
>>>>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
>>>>>>
>>>>>> Also here.
>>>>>>
>>>>>>>
>>>>>>> - if (fmt->field == V4L2_FIELD_ANY)
>>>>>>> - fmt->field = sink_fmt_default.field;
>>>>>>> -
>>>>>>> - vimc_colorimetry_clamp(fmt);
>>>>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
>>>>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
>>>>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
>>>>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
>>>>>>
>>>>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
>>>>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
>>>>>>
>>>>> Acctually according to the subdevices documentation, when changing the
>>>>> sink format, the width and height of the src format should reset to the
>>>>> same values:
>>>>>
>>>>> ""
>>>>> - Sub-devices that scale frames using variable scaling factors should
>>>>> reset the scale factors to default values when sink pads formats are
>>>>> modified. If the 1:1 scaling ratio is supported, this means that
>>>>> source pads formats should be reset to the sink pads formats.
>>>>> ""
>>>>
>>>> I have a small question: Should I worry about the crop? I believe that
>>>> in the current
>>>> implementation setting the sink does not necessarily reset the crop zone.
>>>> Should we reset to the sink resolution or to the one determined by the crop?
>>>> With that said, the way we implemented the scaller, setting a crop
>>>> does not affect the
>>>> source resolution (it retains the sink dimensions), should we change this too?
>>>
>>> From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
>>> have crop in the sink, then the source pad should have dimentions of the crop.
>>
>> Correct.
>>
>>>
>>> At least that is my understanding, and the docs should be updated to make it more
>>> clear.
>>
>> Patches are welcome :-)
>>
>>>
>>> I would like to hear other people's opinions on this.
>>
>> I agree with you, this is my understanding as well.
>>
>> Regards,
>>
>> Hans

2021-07-30 13:57:30

by Laurent Pinchart

Subject: Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad

Hello,

Reviving an old thread.

On Tue, Jan 21, 2020 at 10:42:10AM -0300, Helen Koike wrote:
> On 1/17/20 7:56 PM, Pedro Terra Delboni wrote:
> > Sorry to keep bumping this thread, but while doing some testing I got
> > confused with the following:
> > The documentation mentions that the scaler should be reset whenever
> > the sink format is set.
> > Does this mean that I should reset it independently if the sink set
> > changes the dimensions?
> > For example: if I change the pixels from RGB to another format, should
> > that also reset the source dimensions?
>
> Confusing indeed.
>
> My understanding is:
> If you make any changes in the sink pad that would affect the scaling
> ratio, then you should adjust the source pad to make a scaling ration
> 1:1.
> So, if you only change the pixel format, it shouldn't reset the scaling
> ratio.

It's not clearly described in the documentation, but I would have done
the opposite, resetting the scaler to 1:1 every time S_FMT is called on
the sink pad, regardless of what is being changed.

> > Another thing that got me confused was:
> > At by the functions names, setting the crop dimensions is not
> > considered changing the pad format, it's considered setting a selection.
> > Should I also update the set_selection function to propagate the
> > changes to the source pad?
>
> Following my understanding above, yes.

I disagree here too :-) The crop rectangle on the source pad is used to
control digital zoom, during streaming. This requires the output size to
stay the same when the crop rectangle size changes.

Pedro, would you consider submitting a v5 ? Or should I take over ?

> > Otherwise, by changing the crop (without changing the sink format)
> > will cause the scaling to behave in the same way it would if we didn't
> > propagate the sink properties to the source.
> >
> > So:
> > Should I check if the set_fmt is actually changing the sink dimensions
> > in order to propagate them to the source?
>
> Following my understanding above, yes.
>
> > Should I also propagate the dimensions when setting the sink crop?
>
> Following my understanding above, yes.
>
> I wonder now how other drivers are doing this.
>
> At least, I'm sure the driver rkisp1 is not doing the right thing
> and needs to be corrected.
>
> > Sorry for the long email!
> > Thanks for the attention that you've all been giving us so far!
> >
> > On Wed, Jan 15, 2020 at 11:51 AM Hans Verkuil <[email protected]> wrote:
> >>
> >> On 1/10/20 6:26 PM, Helen Koike wrote:
> >>>
> >>>
> >>> On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
> >>>> Hello!
> >>>>
> >>>> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
> >>>> <[email protected]> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>>
> >>>>> On 30.12.19 14:59, Helen Koike wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> Thanks for the patch, just minor comments below.
> >>>>>>
> >>>>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
> >>>>>>> Modify the scaler subdevice to accept setting the resolution of the source
> >>>>>>> pad (previously the source resolution would always be 3 times the sink for
> >>>>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
> >>>>>>> and the sink video will be scaled to match it.
> >>>>>>>
> >>>>>>> Test example: With the vimc module up (using the default vimc topology)
> >>>>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> >>>>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
> >>>>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
> >>>>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
> >>>>>>> --stream-to=test.raw
> >>>>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> >>>>>>> -video_size "300x700" test.raw
> >>>>>>>
> >>>>>>> Co-developed-by: Gabriela Bittencourt <[email protected]>
> >>>>>>> Signed-off-by: Gabriela Bittencourt <[email protected]>
> >>>>>>> Co-developed-by: Gabriel Francisco Mandaji <[email protected]>
> >>>>>>> Signed-off-by: Gabriel Francisco Mandaji <[email protected]>
> >>>>>>> Signed-off-by: Pedro "pirate" Terra <[email protected]>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Changes in V4:
> >>>>>>> * Rebased with media/master
> >>>>>>> * Scaling is now compatible with crop
> >>>>>>> * Updated test example at the commit message
> >>>>>>> * Add vimc prefix to the pad enumeration
> >>>>>>>
> >>>>>>> Changes in V3:
> >>>>>>> * Corrections suggested by Hans:
> >>>>>>> - Default scaling factor is now 1 (we removed the define and
> >>>>>>> set the source format equals the sink).
> >>>>>>> - Removed SCA_COUNT (enum that represents the number of pads)
> >>>>>>> as there always 2
> >>>>>>> - Swapped the per byte pixel copy to memcpy.
> >>>>>>> * Corrections suggested by Dafna:
> >>>>>>> - Removed from the documentation the old scaler parameter which
> >>>>>>> isn't necessary anymore.
> >>>>>>> * Added a thank you note at the end of the email
> >>>>>>>
> >>>>>>> Changes in V2:
> >>>>>>> * Patch was not sent to media list mail for some reason (even though it
> >>>>>>> was on the Cc list), trying again.
> >>>>>>> * Updating documentation.
> >>>>>>>
> >>>>>>> Hello!
> >>>>>>> This code is the result of friends getting together with too much
> >>>>>>> coffee, sugar and beer trying to get started with some kernel coding.
> >>>>>>> Please, don't go easy on us! s2
> >>>>>>>
> >>>>>>> Running
> >>>>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
> >>>>>>> Gave the following result:
> >>>>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
> >>>>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
> >>>>>>> ---
> >>>>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
> >>>>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
> >>>>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
> >>>>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
> >>>>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
> >>>>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
> >>>>>>> @@ -61,9 +61,11 @@ vimc-debayer:
> >>>>>>> * 1 Pad source
> >>>>>>>
> >>>>>>> vimc-scaler:
> >>>>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
> >>>>>>> - 1920x1440 image. (this value can be configured, see at
> >>>>>>> - `Module options`_).
> >>>>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
> >>>>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
> >>>>>>> +to fit the source resolution. Works for any resolution within the vimc
> >>>>>>> +limitations (even shrinking the image if necessary).
> >>>>>>> +
> >>>>>>> Exposes:
> >>>>>>>
> >>>>>>> * 1 Pad sink
> >>>>>>> @@ -76,19 +78,6 @@ vimc-capture:
> >>>>>>> * 1 Pad sink
> >>>>>>> * 1 Pad source
> >>>>>>>
> >>>>>>> -
> >>>>>>> -Module options
> >>>>>>> ---------------
> >>>>>>> -
> >>>>>>> -Vimc has a module parameter to configure the driver.
> >>>>>>> -
> >>>>>>> -* ``sca_mult=<unsigned int>``
> >>>>>>> -
> >>>>>>> - Image size multiplier factor to be used to multiply both width and
> >>>>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
> >>>>>>> - original one. Currently, only supports scaling up (the default value
> >>>>>>> - is 3).
> >>>>>>> -
> >>>>>>> Source code documentation
> >>>>>>> -------------------------
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> >>>>>>> index e2e551bc3ded..785009b7ac9e 100644
> >>>>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
> >>>>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> >>>>>>> @@ -6,6 +6,7 @@
> >>>>>>> */
> >>>>>>>
> >>>>>>> #include <linux/moduleparam.h>
> >>>>>>> +#include <linux/string.h>
> >>>>>>> #include <linux/vmalloc.h>
> >>>>>>> #include <linux/v4l2-mediabus.h>
> >>>>>>> #include <media/v4l2-rect.h>
> >>>>>>> @@ -13,11 +14,11 @@
> >>>>>>>
> >>>>>>> #include "vimc-common.h"
> >>>>>>>
> >>>>>>> -static unsigned int sca_mult = 3;
> >>>>>>> -module_param(sca_mult, uint, 0000);
> >>>>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >>>>>>> -
> >>>>>>> -#define MAX_ZOOM 8
> >>>>>>> +/* Pad identifier */
> >>>>>>> +enum vimc_sca_pad {
> >>>>>>> + VIMC_SCA_SINK = 0,
> >>>>>>> + VIMC_SCA_SRC = 1,
> >>>>>>> +};
> >>>>>>>
> >>>>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
> >>>>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
> >>>>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >>>>>>> struct vimc_sca_device {
> >>>>>>> struct vimc_ent_device ved;
> >>>>>>> struct v4l2_subdev sd;
> >>>>>>> - /* NOTE: the source fmt is the same as the sink
> >>>>>>> - * with the width and hight multiplied by mult
> >>>>>>> - */
> >>>>>>> - struct v4l2_mbus_framefmt sink_fmt;
> >>>>>>> struct v4l2_rect crop_rect;
> >>>>>>> + /* Frame format for both sink and src pad */
> >>>>>>> + struct v4l2_mbus_framefmt fmt[2];
> >>>>>>> /* Values calculated when the stream starts */
> >>>>>>> u8 *src_frame;
> >>>>>>> - unsigned int src_line_size;
> >>>>>>> unsigned int bpp;
> >>>>>>> struct media_pad pads[2];
> >>>>>>> };
> >>>>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> >>>>>>> struct v4l2_rect *r;
> >>>>>>> unsigned int i;
> >>>>>>>
> >>>>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> >>>>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> >>>>>>> *mf = sink_fmt_default;
> >>>>>>>
> >>>>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
> >>>>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >>>>>>> *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;
> >>>>>>> - mf->width = mf->width * sca_mult;
> >>>>>>> - mf->height = mf->height * sca_mult;
> >>>>>>> }
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
> >>>>>>>
> >>>>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
> >>>>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
> >>>>>>> -
> >>>>>>> - if (VIMC_IS_SINK(fse->pad)) {
> >>>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >>>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>>>>>> - } else {
> >>>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
> >>>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
> >>>>>>> - }
> >>>>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >>>>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>> @@ -154,95 +144,73 @@ 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 */
> >>>>>>> - 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 = crop_rect->width * sca_mult;
> >>>>>>> - format->format.height = crop_rect->height * sca_mult;
> >>>>>>> - }
> >>>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> >>>>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
> >>>>>>> + format->pad);
> >>>>>>> + else
> >>>>>>> + format->format = vsca->fmt[format->pad];
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>>
> >>>>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> >>>>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
> >>>>>>> {
> >>>>>>> - const struct vimc_pix_map *vpix;
> >>>>>>> + if (pad == VIMC_SCA_SINK) {
> >>>>>>> + const struct vimc_pix_map *vpix;
> >>>>>>>
> >>>>>>> - /* Only accept code in the pix map table in non bayer format */
> >>>>>>> - vpix = vimc_pix_map_by_code(fmt->code);
> >>>>>>> - if (!vpix || vpix->bayer)
> >>>>>>> - fmt->code = sink_fmt_default.code;
> >>>>>>> + /* Only accept code in the pix map table in non bayer format */
> >>>>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
> >>>>>>> + if (!vpix || vpix->bayer)
> >>>>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
> >>>>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
> >>>>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
> >>>>>>>
> >>>>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> >>>>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
> >>>>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
> >>>>>>
> >>>>>> Could you fix the alignment here?
> >>>>>> For some reason checkpatch doesn't catch this :(
> >>>>>>
> >>>>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
> >>>>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
> >>>>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
> >>>>>>
> >>>>>> Also here.
> >>>>>>
> >>>>>>>
> >>>>>>> - if (fmt->field == V4L2_FIELD_ANY)
> >>>>>>> - fmt->field = sink_fmt_default.field;
> >>>>>>> -
> >>>>>>> - vimc_colorimetry_clamp(fmt);
> >>>>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
> >>>>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
> >>>>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
> >>>>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
> >>>>>>
> >>>>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
> >>>>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
> >>>>>>
> >>>>> Acctually according to the subdevices documentation, when changing the
> >>>>> sink format, the width and height of the src format should reset to the
> >>>>> same values:
> >>>>>
> >>>>> ""
> >>>>> - Sub-devices that scale frames using variable scaling factors should
> >>>>> reset the scale factors to default values when sink pads formats are
> >>>>> modified. If the 1:1 scaling ratio is supported, this means that
> >>>>> source pads formats should be reset to the sink pads formats.
> >>>>> ""
> >>>>
> >>>> I have a small question: Should I worry about the crop? I believe that
> >>>> in the current
> >>>> implementation setting the sink does not necessarily reset the crop zone.
> >>>> Should we reset to the sink resolution or to the one determined by the crop?
> >>>> With that said, the way we implemented the scaller, setting a crop
> >>>> does not affect the
> >>>> source resolution (it retains the sink dimensions), should we change this too?
> >>>
> >>> From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
> >>> have crop in the sink, then the source pad should have dimentions of the crop.
> >>
> >> Correct.
> >>
> >>>
> >>> At least that is my understanding, and the docs should be updated to make it more
> >>> clear.
> >>
> >> Patches are welcome :-)
> >>
> >>>
> >>> I would like to hear other people's opinions on this.
> >>
> >> I agree with you, this is my understanding as well.

--
Regards,

Laurent Pinchart