linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7] media: vimc: Enable set resolution at the scaler src pad
@ 2021-08-31 17:48 Pedro Terra
  2021-08-31 21:52 ` Laurent Pinchart
  2021-09-13  8:28 ` Hans Verkuil
  0 siblings, 2 replies; 5+ messages in thread
From: Pedro Terra @ 2021-08-31 17:48 UTC (permalink / raw)
  To: pedro, hverkuil-cisco, dafna.hirschfeld, mchehab, skhan,
	linux-media, linux-kernel, gabrielabittencourt00, gfmandaji,
	laurent.pinchart

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 platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d platform:vimc -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
media-ctl -d platform:vimc -V '"Scaler":0[crop:(100,50)/400x150]'
media-ctl -d platform:vimc -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=300,height=700
v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
v4l2-ctl --stream-mmap --stream-count=10 -z platform:vimc -d "RGB/YUV Capture" \
	-stream-to=test.raw

The result will be a cropped stream that can be checked with the command
ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
	-video_size "300x700" test.raw

Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
Signed-off-by: Pedro "pirate" Terra <pirate@terraco.de>

---

Changes in V7:
* Corrections proposed by Laurent:
	- Fix commit message
	- Fix .rst identation
	- Properly initialize both pads in a loop
	- Placed vimc_sca_get_fmt() before set_fmt to keep
		the order they're set in v4l2_subdev_pad_ops structure.
	- Renamed variables to avoid ambiguity.

Changes in V6:
* Corrections proposed by Laurent:
	- Corrected commit example to become file independent.
	- Cleaned unnecessary code inserted at vimc_sca_init_cfg
	- s/__u32/u32/
	- Refactored vimc_sca_(set/get)_fmt and vimc_sca_(get/set)_selection
		as suggested to make it more readable.
	- Corrected code alignment.
	- Cleaned and optimized vimc_sca_fill_src_frame

Changes in V5:
* Fixed code aliment mistake
* Renamed some variables to make the code more readable
* Propagate sink pad formatting to src resetting the 1:1 scaling ratio.
	(the crop is also reset when this is done).

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.

Running
/usr/local/bin/v4l2-compliance -m /dev/media0
Gave the following result:
v4l2-compliance SHA: c86aab9cc7f1 2021-07-28 11:52:45
Grand Total for vimc device /dev/media0: 473, Succeeded: 473, Failed: 0, Warnings: 0
---
 Documentation/admin-guide/media/vimc.rst      |  20 +-
 drivers/media/test-drivers/vimc/vimc-scaler.c | 366 +++++++-----------
 2 files changed, 148 insertions(+), 238 deletions(-)

diff --git a/Documentation/admin-guide/media/vimc.rst b/Documentation/admin-guide/media/vimc.rst
index 211cc8972410..4e5793db6651 100644
--- a/Documentation/admin-guide/media/vimc.rst
+++ b/Documentation/admin-guide/media/vimc.rst
@@ -61,9 +61,10 @@ 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
@@ -75,16 +76,3 @@ 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).
diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index 06880dd0b6ac..820b8f5b502f 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/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 vic_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,19 +26,16 @@ 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];
 };
 
-static const struct v4l2_mbus_framefmt sink_fmt_default = {
+static const struct v4l2_mbus_framefmt fmt_default = {
 	.width = VIMC_SCA_FMT_WIDTH_DEFAULT,
 	.height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
 	.code = MEDIA_BUS_FMT_RGB888_1X24,
@@ -72,17 +70,6 @@ vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
 	return r;
 }
 
-static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
-				      const struct v4l2_mbus_framefmt *sink_fmt)
-{
-	const struct v4l2_rect sink_rect =
-		vimc_sca_get_crop_bound_sink(sink_fmt);
-
-	/* Disallow rectangles smaller than the minimal one. */
-	v4l2_rect_set_min_size(r, &crop_rect_min);
-	v4l2_rect_map_inside(r, &sink_rect);
-}
-
 static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state)
 {
@@ -90,19 +77,14 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
 	struct v4l2_rect *r;
 	unsigned int i;
 
-	mf = v4l2_subdev_get_try_format(sd, sd_state, 0);
-	*mf = sink_fmt_default;
-
-	r = v4l2_subdev_get_try_crop(sd, sd_state, 0);
-	*r = crop_rect_default;
-
-	for (i = 1; i < sd->entity.num_pads; i++) {
+	for (i = 0; i < sd->entity.num_pads; i++) {
 		mf = v4l2_subdev_get_try_format(sd, sd_state, i);
-		*mf = sink_fmt_default;
-		mf->width = mf->width * sca_mult;
-		mf->height = mf->height * sca_mult;
+		*mf = fmt_default;
 	}
 
+	r = v4l2_subdev_get_try_crop(sd, sd_state, VIMC_SCA_SINK);
+	*r = crop_rect_default;
+
 	return 0;
 }
 
@@ -144,112 +126,108 @@ 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;
 }
 
+static struct v4l2_mbus_framefmt *
+vimc_sca_pad_format(struct vimc_sca_device *vsca,
+		    struct v4l2_subdev_state *sd_state, u32 pad,
+		    enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_format(&vsca->sd, sd_state, pad);
+	else
+		return &vsca->fmt[pad];
+}
+
+static struct v4l2_rect *
+vimc_sca_pad_crop(struct vimc_sca_device *vsca,
+		  struct v4l2_subdev_state *sd_state,
+		  enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_crop(&vsca->sd, sd_state,
+						VIMC_SCA_SINK);
+	else
+		return &vsca->crop_rect;
+}
+
 static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *sd_state,
 			    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, sd_state, 0);
-		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 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;
-	}
 
+	format->format = *vimc_sca_pad_format(vsca, sd_state, format->pad,
+					      format->which);
 	return 0;
 }
 
-static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
-{
-	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;
-
-	fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
-			     VIMC_FRAME_MAX_WIDTH) & ~1;
-	fmt->height = clamp_t(u32, fmt->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);
-}
-
 static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *sd_state,
-			    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_rect *crop_rect;
+	struct v4l2_mbus_framefmt *fmt;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		/* Do not change the format while stream is on */
-		if (vsca->src_frame)
-			return -EBUSY;
+	/* Do not change the active format while stream is on */
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsca->src_frame)
+		return -EBUSY;
 
-		sink_fmt = &vsca->sink_fmt;
-		crop_rect = &vsca->crop_rect;
-	} else {
-		sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
-		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
+	fmt = vimc_sca_pad_format(vsca, sd_state, format->pad, format->which);
+
+	/*
+	 * The media bus code and colorspace can only be changed on the sink
+	 * pad, the source pad only follows.
+	 */
+	if (format->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(format->format.code);
+		if (vpix && !vpix->bayer)
+			fmt->code = format->format.code;
+		else
+			fmt->code = fmt_default.code;
+
+		/* Clamp the colorspace to valid values. */
+		fmt->colorspace = format->format.colorspace;
+		fmt->ycbcr_enc = format->format.ycbcr_enc;
+		fmt->quantization = format->format.quantization;
+		fmt->xfer_func = format->format.xfer_func;
+		vimc_colorimetry_clamp(fmt);
 	}
 
+	/* Clamp and align the width and height */
+	fmt->width = clamp_t(u32, format->format.width, VIMC_FRAME_MIN_WIDTH,
+			     VIMC_FRAME_MAX_WIDTH) & ~1;
+	fmt->height = clamp_t(u32, format->format.height, VIMC_FRAME_MIN_HEIGHT,
+			      VIMC_FRAME_MAX_HEIGHT) & ~1;
+
 	/*
-	 * Do not change the format of the source pad,
-	 * it is propagated from the sink
+	 * Propagate the sink pad format to the crop rectangle and the source
+	 * pad.
 	 */
-	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;
-
-		/* Do the crop, but respect the current bounds */
-		vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
+	if (format->pad == VIMC_SCA_SINK) {
+		struct v4l2_mbus_framefmt *src_fmt;
+		struct v4l2_rect *crop;
+
+		crop = vimc_sca_pad_crop(vsca, sd_state, format->which);
+		crop->width = fmt->width;
+		crop->height = fmt->height;
+		crop->top = 0;
+		crop->left = 0;
+
+		src_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SRC,
+					      format->which);
+		*src_fmt = *fmt;
 	}
 
+	format->format = *fmt;
+
 	return 0;
 }
 
@@ -259,24 +237,17 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
 {
 	struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *sink_fmt;
-	struct v4l2_rect *crop_rect;
 
 	if (VIMC_IS_SRC(sel->pad))
 		return -EINVAL;
 
-	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		sink_fmt = &vsca->sink_fmt;
-		crop_rect = &vsca->crop_rect;
-	} else {
-		sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
-		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
-	}
-
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
-		sel->r = *crop_rect;
+		sel->r = *vimc_sca_pad_crop(vsca, sd_state, sel->which);
 		break;
 	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sink_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SINK,
+					       sel->which);
 		sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
 		break;
 	default:
@@ -286,6 +257,17 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
+				      const struct v4l2_mbus_framefmt *sink_fmt)
+{
+	const struct v4l2_rect sink_rect =
+		vimc_sca_get_crop_bound_sink(sink_fmt);
+
+	/* Disallow rectangles smaller than the minimal one. */
+	v4l2_rect_set_min_size(r, &crop_rect_min);
+	v4l2_rect_map_inside(r, &sink_rect);
+}
+
 static int vimc_sca_set_selection(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_selection *sel)
@@ -294,30 +276,18 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *sink_fmt;
 	struct v4l2_rect *crop_rect;
 
-	if (VIMC_IS_SRC(sel->pad))
+	/* Only support setting the crop of the sink pad */
+	if (VIMC_IS_SRC(sel->pad) || sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		/* Do not change the format while stream is on */
-		if (vsca->src_frame)
-			return -EBUSY;
+	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsca->src_frame)
+		return -EBUSY;
 
-		crop_rect = &vsca->crop_rect;
-		sink_fmt = &vsca->sink_fmt;
-	} else {
-		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
-		sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
-	}
-
-	switch (sel->target) {
-	case V4L2_SEL_TGT_CROP:
-		/* Do the crop, but respect the current bounds */
-		vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
-		*crop_rect = sel->r;
-		break;
-	default:
-		return -EINVAL;
-	}
+	crop_rect = vimc_sca_pad_crop(vsca, sd_state, sel->which);
+	sink_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SINK,
+				       sel->which);
+	vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
+	*crop_rect = sel->r;
 
 	return 0;
 }
@@ -344,16 +314,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
@@ -382,77 +348,32 @@ 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 snk_width = vsca->fmt[VIMC_SCA_SINK].width;
+	unsigned int src_x, src_y;
+	u8 *walker = vsca->src_frame;
+
+	/* Set each pixel at the src_frame to its sink_frame equivalent */
+	for (src_y = 0; src_y < src_fmt->height; src_y++) {
+		unsigned int snk_y, y_offset;
+
+		snk_y = (src_y * r->height) / src_fmt->height + r->top;
+		y_offset = snk_y * snk_width * vsca->bpp;
+
+		for (src_x = 0; src_x < src_fmt->width; src_x++) {
+			unsigned int snk_x, x_offset, index;
+
+			snk_x = (src_x * r->width) / src_fmt->width + r->left;
+			x_offset = snk_x * vsca->bpp;
+			index = y_offset + x_offset;
+			memcpy(walker, &sink_frame[index], vsca->bpp);
+			walker += vsca->bpp;
+		}
+	}
 }
 
 static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
@@ -492,8 +413,8 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 		return ERR_PTR(-ENOMEM);
 
 	/* 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,
@@ -508,7 +429,8 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 	vsca->ved.dev = vimc->mdev.dev;
 
 	/* Initialize the frame format */
-	vsca->sink_fmt = sink_fmt_default;
+	vsca->fmt[VIMC_SCA_SINK] = fmt_default;
+	vsca->fmt[VIMC_SCA_SRC] = fmt_default;
 
 	/* Initialize the crop selection */
 	vsca->crop_rect = crop_rect_default;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v7] media: vimc: Enable set resolution at the scaler src pad
  2021-08-31 17:48 [PATCH v7] media: vimc: Enable set resolution at the scaler src pad Pedro Terra
@ 2021-08-31 21:52 ` Laurent Pinchart
  2021-09-01  0:06   ` Pedro Terra Delboni
  2021-09-13  8:28 ` Hans Verkuil
  1 sibling, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2021-08-31 21:52 UTC (permalink / raw)
  To: Pedro Terra
  Cc: hverkuil-cisco, dafna.hirschfeld, mchehab, skhan, linux-media,
	linux-kernel, gabrielabittencourt00, gfmandaji

Hi Pedro,

Thank you for the patch.

On Tue, Aug 31, 2021 at 02:48:22PM -0300, 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 platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> media-ctl -d platform:vimc -V '"Scaler":0[crop:(100,50)/400x150]'
> media-ctl -d platform:vimc -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=300,height=700
> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
> v4l2-ctl --stream-mmap --stream-count=10 -z platform:vimc -d "RGB/YUV Capture" \
> 	-stream-to=test.raw

That should be --stream-to, not -stream-to.

> 
> The result will be a cropped stream that can be checked with the command
> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> 	-video_size "300x700" test.raw
> 
> Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> Signed-off-by: Pedro "pirate" Terra <pirate@terraco.de>
> 
> ---
> 
> Changes in V7:
> * Corrections proposed by Laurent:
> 	- Fix commit message
> 	- Fix .rst identation
> 	- Properly initialize both pads in a loop
> 	- Placed vimc_sca_get_fmt() before set_fmt to keep
> 		the order they're set in v4l2_subdev_pad_ops structure.
> 	- Renamed variables to avoid ambiguity.
> 
> Changes in V6:
> * Corrections proposed by Laurent:
> 	- Corrected commit example to become file independent.
> 	- Cleaned unnecessary code inserted at vimc_sca_init_cfg
> 	- s/__u32/u32/
> 	- Refactored vimc_sca_(set/get)_fmt and vimc_sca_(get/set)_selection
> 		as suggested to make it more readable.
> 	- Corrected code alignment.
> 	- Cleaned and optimized vimc_sca_fill_src_frame
> 
> Changes in V5:
> * Fixed code aliment mistake
> * Renamed some variables to make the code more readable
> * Propagate sink pad formatting to src resetting the 1:1 scaling ratio.
> 	(the crop is also reset when this is done).
> 
> 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.
> 
> Running
> /usr/local/bin/v4l2-compliance -m /dev/media0
> Gave the following result:
> v4l2-compliance SHA: c86aab9cc7f1 2021-07-28 11:52:45
> Grand Total for vimc device /dev/media0: 473, Succeeded: 473, Failed: 0, Warnings: 0
> ---
>  Documentation/admin-guide/media/vimc.rst      |  20 +-
>  drivers/media/test-drivers/vimc/vimc-scaler.c | 366 +++++++-----------
>  2 files changed, 148 insertions(+), 238 deletions(-)
> 
> diff --git a/Documentation/admin-guide/media/vimc.rst b/Documentation/admin-guide/media/vimc.rst
> index 211cc8972410..4e5793db6651 100644
> --- a/Documentation/admin-guide/media/vimc.rst
> +++ b/Documentation/admin-guide/media/vimc.rst
> @@ -61,9 +61,10 @@ 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).

Lines should be wrapped at 80 columns in the documentation.

Apart from that, it all looks good.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you're fine with it, I can fix the two issues above when applying the
patch to my tree, and I'll then send a pull request.

Would anyone else like to review this patch ?

>  	Exposes:
>  
>  	* 1 Pad sink
> @@ -75,16 +76,3 @@ 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).
> diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
> index 06880dd0b6ac..820b8f5b502f 100644
> --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
> +++ b/drivers/media/test-drivers/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 vic_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,19 +26,16 @@ 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];
>  };
>  
> -static const struct v4l2_mbus_framefmt sink_fmt_default = {
> +static const struct v4l2_mbus_framefmt fmt_default = {
>  	.width = VIMC_SCA_FMT_WIDTH_DEFAULT,
>  	.height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
>  	.code = MEDIA_BUS_FMT_RGB888_1X24,
> @@ -72,17 +70,6 @@ vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
>  	return r;
>  }
>  
> -static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
> -				      const struct v4l2_mbus_framefmt *sink_fmt)
> -{
> -	const struct v4l2_rect sink_rect =
> -		vimc_sca_get_crop_bound_sink(sink_fmt);
> -
> -	/* Disallow rectangles smaller than the minimal one. */
> -	v4l2_rect_set_min_size(r, &crop_rect_min);
> -	v4l2_rect_map_inside(r, &sink_rect);
> -}
> -
>  static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> @@ -90,19 +77,14 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_rect *r;
>  	unsigned int i;
>  
> -	mf = v4l2_subdev_get_try_format(sd, sd_state, 0);
> -	*mf = sink_fmt_default;
> -
> -	r = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> -	*r = crop_rect_default;
> -
> -	for (i = 1; i < sd->entity.num_pads; i++) {
> +	for (i = 0; i < sd->entity.num_pads; i++) {
>  		mf = v4l2_subdev_get_try_format(sd, sd_state, i);
> -		*mf = sink_fmt_default;
> -		mf->width = mf->width * sca_mult;
> -		mf->height = mf->height * sca_mult;
> +		*mf = fmt_default;
>  	}
>  
> +	r = v4l2_subdev_get_try_crop(sd, sd_state, VIMC_SCA_SINK);
> +	*r = crop_rect_default;
> +
>  	return 0;
>  }
>  
> @@ -144,112 +126,108 @@ 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;
>  }
>  
> +static struct v4l2_mbus_framefmt *
> +vimc_sca_pad_format(struct vimc_sca_device *vsca,
> +		    struct v4l2_subdev_state *sd_state, u32 pad,
> +		    enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_get_try_format(&vsca->sd, sd_state, pad);
> +	else
> +		return &vsca->fmt[pad];
> +}
> +
> +static struct v4l2_rect *
> +vimc_sca_pad_crop(struct vimc_sca_device *vsca,
> +		  struct v4l2_subdev_state *sd_state,
> +		  enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_get_try_crop(&vsca->sd, sd_state,
> +						VIMC_SCA_SINK);
> +	else
> +		return &vsca->crop_rect;
> +}
> +
>  static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
>  			    struct v4l2_subdev_state *sd_state,
>  			    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, sd_state, 0);
> -		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 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;
> -	}
>  
> +	format->format = *vimc_sca_pad_format(vsca, sd_state, format->pad,
> +					      format->which);
>  	return 0;
>  }
>  
> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> -{
> -	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;
> -
> -	fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> -			     VIMC_FRAME_MAX_WIDTH) & ~1;
> -	fmt->height = clamp_t(u32, fmt->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);
> -}
> -
>  static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>  			    struct v4l2_subdev_state *sd_state,
> -			    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_rect *crop_rect;
> +	struct v4l2_mbus_framefmt *fmt;
>  
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		/* Do not change the format while stream is on */
> -		if (vsca->src_frame)
> -			return -EBUSY;
> +	/* Do not change the active format while stream is on */
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsca->src_frame)
> +		return -EBUSY;
>  
> -		sink_fmt = &vsca->sink_fmt;
> -		crop_rect = &vsca->crop_rect;
> -	} else {
> -		sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> -		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> +	fmt = vimc_sca_pad_format(vsca, sd_state, format->pad, format->which);
> +
> +	/*
> +	 * The media bus code and colorspace can only be changed on the sink
> +	 * pad, the source pad only follows.
> +	 */
> +	if (format->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(format->format.code);
> +		if (vpix && !vpix->bayer)
> +			fmt->code = format->format.code;
> +		else
> +			fmt->code = fmt_default.code;
> +
> +		/* Clamp the colorspace to valid values. */
> +		fmt->colorspace = format->format.colorspace;
> +		fmt->ycbcr_enc = format->format.ycbcr_enc;
> +		fmt->quantization = format->format.quantization;
> +		fmt->xfer_func = format->format.xfer_func;
> +		vimc_colorimetry_clamp(fmt);
>  	}
>  
> +	/* Clamp and align the width and height */
> +	fmt->width = clamp_t(u32, format->format.width, VIMC_FRAME_MIN_WIDTH,
> +			     VIMC_FRAME_MAX_WIDTH) & ~1;
> +	fmt->height = clamp_t(u32, format->format.height, VIMC_FRAME_MIN_HEIGHT,
> +			      VIMC_FRAME_MAX_HEIGHT) & ~1;
> +
>  	/*
> -	 * Do not change the format of the source pad,
> -	 * it is propagated from the sink
> +	 * Propagate the sink pad format to the crop rectangle and the source
> +	 * pad.
>  	 */
> -	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;
> -
> -		/* Do the crop, but respect the current bounds */
> -		vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
> +	if (format->pad == VIMC_SCA_SINK) {
> +		struct v4l2_mbus_framefmt *src_fmt;
> +		struct v4l2_rect *crop;
> +
> +		crop = vimc_sca_pad_crop(vsca, sd_state, format->which);
> +		crop->width = fmt->width;
> +		crop->height = fmt->height;
> +		crop->top = 0;
> +		crop->left = 0;
> +
> +		src_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SRC,
> +					      format->which);
> +		*src_fmt = *fmt;
>  	}
>  
> +	format->format = *fmt;
> +
>  	return 0;
>  }
>  
> @@ -259,24 +237,17 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
>  {
>  	struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
>  	struct v4l2_mbus_framefmt *sink_fmt;
> -	struct v4l2_rect *crop_rect;
>  
>  	if (VIMC_IS_SRC(sel->pad))
>  		return -EINVAL;
>  
> -	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		sink_fmt = &vsca->sink_fmt;
> -		crop_rect = &vsca->crop_rect;
> -	} else {
> -		sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> -		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> -	}
> -
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP:
> -		sel->r = *crop_rect;
> +		sel->r = *vimc_sca_pad_crop(vsca, sd_state, sel->which);
>  		break;
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sink_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SINK,
> +					       sel->which);
>  		sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
>  		break;
>  	default:
> @@ -286,6 +257,17 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
> +				      const struct v4l2_mbus_framefmt *sink_fmt)
> +{
> +	const struct v4l2_rect sink_rect =
> +		vimc_sca_get_crop_bound_sink(sink_fmt);
> +
> +	/* Disallow rectangles smaller than the minimal one. */
> +	v4l2_rect_set_min_size(r, &crop_rect_min);
> +	v4l2_rect_map_inside(r, &sink_rect);
> +}
> +
>  static int vimc_sca_set_selection(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_selection *sel)
> @@ -294,30 +276,18 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *sink_fmt;
>  	struct v4l2_rect *crop_rect;
>  
> -	if (VIMC_IS_SRC(sel->pad))
> +	/* Only support setting the crop of the sink pad */
> +	if (VIMC_IS_SRC(sel->pad) || sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
>  
> -	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		/* Do not change the format while stream is on */
> -		if (vsca->src_frame)
> -			return -EBUSY;
> +	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsca->src_frame)
> +		return -EBUSY;
>  
> -		crop_rect = &vsca->crop_rect;
> -		sink_fmt = &vsca->sink_fmt;
> -	} else {
> -		crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> -		sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> -	}
> -
> -	switch (sel->target) {
> -	case V4L2_SEL_TGT_CROP:
> -		/* Do the crop, but respect the current bounds */
> -		vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
> -		*crop_rect = sel->r;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	crop_rect = vimc_sca_pad_crop(vsca, sd_state, sel->which);
> +	sink_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SINK,
> +				       sel->which);
> +	vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
> +	*crop_rect = sel->r;
>  
>  	return 0;
>  }
> @@ -344,16 +314,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
> @@ -382,77 +348,32 @@ 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 snk_width = vsca->fmt[VIMC_SCA_SINK].width;
> +	unsigned int src_x, src_y;
> +	u8 *walker = vsca->src_frame;
> +
> +	/* Set each pixel at the src_frame to its sink_frame equivalent */
> +	for (src_y = 0; src_y < src_fmt->height; src_y++) {
> +		unsigned int snk_y, y_offset;
> +
> +		snk_y = (src_y * r->height) / src_fmt->height + r->top;
> +		y_offset = snk_y * snk_width * vsca->bpp;
> +
> +		for (src_x = 0; src_x < src_fmt->width; src_x++) {
> +			unsigned int snk_x, x_offset, index;
> +
> +			snk_x = (src_x * r->width) / src_fmt->width + r->left;
> +			x_offset = snk_x * vsca->bpp;
> +			index = y_offset + x_offset;
> +			memcpy(walker, &sink_frame[index], vsca->bpp);
> +			walker += vsca->bpp;
> +		}
> +	}
>  }
>  
>  static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> @@ -492,8 +413,8 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>  		return ERR_PTR(-ENOMEM);
>  
>  	/* 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,
> @@ -508,7 +429,8 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>  	vsca->ved.dev = vimc->mdev.dev;
>  
>  	/* Initialize the frame format */
> -	vsca->sink_fmt = sink_fmt_default;
> +	vsca->fmt[VIMC_SCA_SINK] = fmt_default;
> +	vsca->fmt[VIMC_SCA_SRC] = fmt_default;
>  
>  	/* Initialize the crop selection */
>  	vsca->crop_rect = crop_rect_default;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v7] media: vimc: Enable set resolution at the scaler src pad
  2021-08-31 21:52 ` Laurent Pinchart
@ 2021-09-01  0:06   ` Pedro Terra Delboni
  0 siblings, 0 replies; 5+ messages in thread
From: Pedro Terra Delboni @ 2021-09-01  0:06 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Hans Verkuil, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Shuah Khan, linux-media, linux-kernel, Gabriela Bittencourt,
	Gabriel Francisco Mandaji

Hi Laurent,

Thank you for all the help =)

On Tue, Aug 31, 2021 at 6:52 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Pedro,
>
> Thank you for the patch.
>
> On Tue, Aug 31, 2021 at 02:48:22PM -0300, 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 platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> > media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> > media-ctl -d platform:vimc -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> > media-ctl -d platform:vimc -V '"Scaler":0[crop:(100,50)/400x150]'
> > media-ctl -d platform:vimc -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> > v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=300,height=700
> > v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
> > v4l2-ctl --stream-mmap --stream-count=10 -z platform:vimc -d "RGB/YUV Capture" \
> >       -stream-to=test.raw
>
> That should be --stream-to, not -stream-to.
>
> >
> > The result will be a cropped stream that can be checked with the command
> > ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> >       -video_size "300x700" test.raw
> >
> > Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> > Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> > Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> > Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> > Signed-off-by: Pedro "pirate" Terra <pirate@terraco.de>
> >
> > ---
> >
> > Changes in V7:
> > * Corrections proposed by Laurent:
> >       - Fix commit message
> >       - Fix .rst identation
> >       - Properly initialize both pads in a loop
> >       - Placed vimc_sca_get_fmt() before set_fmt to keep
> >               the order they're set in v4l2_subdev_pad_ops structure.
> >       - Renamed variables to avoid ambiguity.
> >
> > Changes in V6:
> > * Corrections proposed by Laurent:
> >       - Corrected commit example to become file independent.
> >       - Cleaned unnecessary code inserted at vimc_sca_init_cfg
> >       - s/__u32/u32/
> >       - Refactored vimc_sca_(set/get)_fmt and vimc_sca_(get/set)_selection
> >               as suggested to make it more readable.
> >       - Corrected code alignment.
> >       - Cleaned and optimized vimc_sca_fill_src_frame
> >
> > Changes in V5:
> > * Fixed code aliment mistake
> > * Renamed some variables to make the code more readable
> > * Propagate sink pad formatting to src resetting the 1:1 scaling ratio.
> >       (the crop is also reset when this is done).
> >
> > 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.
> >
> > Running
> > /usr/local/bin/v4l2-compliance -m /dev/media0
> > Gave the following result:
> > v4l2-compliance SHA: c86aab9cc7f1 2021-07-28 11:52:45
> > Grand Total for vimc device /dev/media0: 473, Succeeded: 473, Failed: 0, Warnings: 0
> > ---
> >  Documentation/admin-guide/media/vimc.rst      |  20 +-
> >  drivers/media/test-drivers/vimc/vimc-scaler.c | 366 +++++++-----------
> >  2 files changed, 148 insertions(+), 238 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/media/vimc.rst b/Documentation/admin-guide/media/vimc.rst
> > index 211cc8972410..4e5793db6651 100644
> > --- a/Documentation/admin-guide/media/vimc.rst
> > +++ b/Documentation/admin-guide/media/vimc.rst
> > @@ -61,9 +61,10 @@ 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).
>
> Lines should be wrapped at 80 columns in the documentation.
>
> Apart from that, it all looks good.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> If you're fine with it, I can fix the two issues above when applying the
> patch to my tree, and I'll then send a pull request.

I'm fine with it, but in any case I sent you a "v8" with the proposed
changes, in case it makes things easier.
>
> Would anyone else like to review this patch ?
>
> >       Exposes:
> >
> >       * 1 Pad sink
> > @@ -75,16 +76,3 @@ 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).
> > diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
> > index 06880dd0b6ac..820b8f5b502f 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
> > +++ b/drivers/media/test-drivers/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 vic_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,19 +26,16 @@ 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];
> >  };
> >
> > -static const struct v4l2_mbus_framefmt sink_fmt_default = {
> > +static const struct v4l2_mbus_framefmt fmt_default = {
> >       .width = VIMC_SCA_FMT_WIDTH_DEFAULT,
> >       .height = VIMC_SCA_FMT_HEIGHT_DEFAULT,
> >       .code = MEDIA_BUS_FMT_RGB888_1X24,
> > @@ -72,17 +70,6 @@ vimc_sca_get_crop_bound_sink(const struct v4l2_mbus_framefmt *sink_fmt)
> >       return r;
> >  }
> >
> > -static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
> > -                                   const struct v4l2_mbus_framefmt *sink_fmt)
> > -{
> > -     const struct v4l2_rect sink_rect =
> > -             vimc_sca_get_crop_bound_sink(sink_fmt);
> > -
> > -     /* Disallow rectangles smaller than the minimal one. */
> > -     v4l2_rect_set_min_size(r, &crop_rect_min);
> > -     v4l2_rect_map_inside(r, &sink_rect);
> > -}
> > -
> >  static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> >                            struct v4l2_subdev_state *sd_state)
> >  {
> > @@ -90,19 +77,14 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> >       struct v4l2_rect *r;
> >       unsigned int i;
> >
> > -     mf = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > -     *mf = sink_fmt_default;
> > -
> > -     r = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> > -     *r = crop_rect_default;
> > -
> > -     for (i = 1; i < sd->entity.num_pads; i++) {
> > +     for (i = 0; i < sd->entity.num_pads; i++) {
> >               mf = v4l2_subdev_get_try_format(sd, sd_state, i);
> > -             *mf = sink_fmt_default;
> > -             mf->width = mf->width * sca_mult;
> > -             mf->height = mf->height * sca_mult;
> > +             *mf = fmt_default;
> >       }
> >
> > +     r = v4l2_subdev_get_try_crop(sd, sd_state, VIMC_SCA_SINK);
> > +     *r = crop_rect_default;
> > +
> >       return 0;
> >  }
> >
> > @@ -144,112 +126,108 @@ 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;
> >  }
> >
> > +static struct v4l2_mbus_framefmt *
> > +vimc_sca_pad_format(struct vimc_sca_device *vsca,
> > +                 struct v4l2_subdev_state *sd_state, u32 pad,
> > +                 enum v4l2_subdev_format_whence which)
> > +{
> > +     if (which == V4L2_SUBDEV_FORMAT_TRY)
> > +             return v4l2_subdev_get_try_format(&vsca->sd, sd_state, pad);
> > +     else
> > +             return &vsca->fmt[pad];
> > +}
> > +
> > +static struct v4l2_rect *
> > +vimc_sca_pad_crop(struct vimc_sca_device *vsca,
> > +               struct v4l2_subdev_state *sd_state,
> > +               enum v4l2_subdev_format_whence which)
> > +{
> > +     if (which == V4L2_SUBDEV_FORMAT_TRY)
> > +             return v4l2_subdev_get_try_crop(&vsca->sd, sd_state,
> > +                                             VIMC_SCA_SINK);
> > +     else
> > +             return &vsca->crop_rect;
> > +}
> > +
> >  static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
> >                           struct v4l2_subdev_state *sd_state,
> >                           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, sd_state, 0);
> > -             crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 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;
> > -     }
> >
> > +     format->format = *vimc_sca_pad_format(vsca, sd_state, format->pad,
> > +                                           format->which);
> >       return 0;
> >  }
> >
> > -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> > -{
> > -     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;
> > -
> > -     fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> > -                          VIMC_FRAME_MAX_WIDTH) & ~1;
> > -     fmt->height = clamp_t(u32, fmt->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);
> > -}
> > -
> >  static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> >                           struct v4l2_subdev_state *sd_state,
> > -                         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_rect *crop_rect;
> > +     struct v4l2_mbus_framefmt *fmt;
> >
> > -     if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -             /* Do not change the format while stream is on */
> > -             if (vsca->src_frame)
> > -                     return -EBUSY;
> > +     /* Do not change the active format while stream is on */
> > +     if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsca->src_frame)
> > +             return -EBUSY;
> >
> > -             sink_fmt = &vsca->sink_fmt;
> > -             crop_rect = &vsca->crop_rect;
> > -     } else {
> > -             sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > -             crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> > +     fmt = vimc_sca_pad_format(vsca, sd_state, format->pad, format->which);
> > +
> > +     /*
> > +      * The media bus code and colorspace can only be changed on the sink
> > +      * pad, the source pad only follows.
> > +      */
> > +     if (format->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(format->format.code);
> > +             if (vpix && !vpix->bayer)
> > +                     fmt->code = format->format.code;
> > +             else
> > +                     fmt->code = fmt_default.code;
> > +
> > +             /* Clamp the colorspace to valid values. */
> > +             fmt->colorspace = format->format.colorspace;
> > +             fmt->ycbcr_enc = format->format.ycbcr_enc;
> > +             fmt->quantization = format->format.quantization;
> > +             fmt->xfer_func = format->format.xfer_func;
> > +             vimc_colorimetry_clamp(fmt);
> >       }
> >
> > +     /* Clamp and align the width and height */
> > +     fmt->width = clamp_t(u32, format->format.width, VIMC_FRAME_MIN_WIDTH,
> > +                          VIMC_FRAME_MAX_WIDTH) & ~1;
> > +     fmt->height = clamp_t(u32, format->format.height, VIMC_FRAME_MIN_HEIGHT,
> > +                           VIMC_FRAME_MAX_HEIGHT) & ~1;
> > +
> >       /*
> > -      * Do not change the format of the source pad,
> > -      * it is propagated from the sink
> > +      * Propagate the sink pad format to the crop rectangle and the source
> > +      * pad.
> >        */
> > -     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;
> > -
> > -             /* Do the crop, but respect the current bounds */
> > -             vimc_sca_adjust_sink_crop(crop_rect, sink_fmt);
> > +     if (format->pad == VIMC_SCA_SINK) {
> > +             struct v4l2_mbus_framefmt *src_fmt;
> > +             struct v4l2_rect *crop;
> > +
> > +             crop = vimc_sca_pad_crop(vsca, sd_state, format->which);
> > +             crop->width = fmt->width;
> > +             crop->height = fmt->height;
> > +             crop->top = 0;
> > +             crop->left = 0;
> > +
> > +             src_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SRC,
> > +                                           format->which);
> > +             *src_fmt = *fmt;
> >       }
> >
> > +     format->format = *fmt;
> > +
> >       return 0;
> >  }
> >
> > @@ -259,24 +237,17 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
> >  {
> >       struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> >       struct v4l2_mbus_framefmt *sink_fmt;
> > -     struct v4l2_rect *crop_rect;
> >
> >       if (VIMC_IS_SRC(sel->pad))
> >               return -EINVAL;
> >
> > -     if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -             sink_fmt = &vsca->sink_fmt;
> > -             crop_rect = &vsca->crop_rect;
> > -     } else {
> > -             sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > -             crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> > -     }
> > -
> >       switch (sel->target) {
> >       case V4L2_SEL_TGT_CROP:
> > -             sel->r = *crop_rect;
> > +             sel->r = *vimc_sca_pad_crop(vsca, sd_state, sel->which);
> >               break;
> >       case V4L2_SEL_TGT_CROP_BOUNDS:
> > +             sink_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SINK,
> > +                                            sel->which);
> >               sel->r = vimc_sca_get_crop_bound_sink(sink_fmt);
> >               break;
> >       default:
> > @@ -286,6 +257,17 @@ static int vimc_sca_get_selection(struct v4l2_subdev *sd,
> >       return 0;
> >  }
> >
> > +static void vimc_sca_adjust_sink_crop(struct v4l2_rect *r,
> > +                                   const struct v4l2_mbus_framefmt *sink_fmt)
> > +{
> > +     const struct v4l2_rect sink_rect =
> > +             vimc_sca_get_crop_bound_sink(sink_fmt);
> > +
> > +     /* Disallow rectangles smaller than the minimal one. */
> > +     v4l2_rect_set_min_size(r, &crop_rect_min);
> > +     v4l2_rect_map_inside(r, &sink_rect);
> > +}
> > +
> >  static int vimc_sca_set_selection(struct v4l2_subdev *sd,
> >                                 struct v4l2_subdev_state *sd_state,
> >                                 struct v4l2_subdev_selection *sel)
> > @@ -294,30 +276,18 @@ static int vimc_sca_set_selection(struct v4l2_subdev *sd,
> >       struct v4l2_mbus_framefmt *sink_fmt;
> >       struct v4l2_rect *crop_rect;
> >
> > -     if (VIMC_IS_SRC(sel->pad))
> > +     /* Only support setting the crop of the sink pad */
> > +     if (VIMC_IS_SRC(sel->pad) || sel->target != V4L2_SEL_TGT_CROP)
> >               return -EINVAL;
> >
> > -     if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -             /* Do not change the format while stream is on */
> > -             if (vsca->src_frame)
> > -                     return -EBUSY;
> > +     if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsca->src_frame)
> > +             return -EBUSY;
> >
> > -             crop_rect = &vsca->crop_rect;
> > -             sink_fmt = &vsca->sink_fmt;
> > -     } else {
> > -             crop_rect = v4l2_subdev_get_try_crop(sd, sd_state, 0);
> > -             sink_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > -     }
> > -
> > -     switch (sel->target) {
> > -     case V4L2_SEL_TGT_CROP:
> > -             /* Do the crop, but respect the current bounds */
> > -             vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
> > -             *crop_rect = sel->r;
> > -             break;
> > -     default:
> > -             return -EINVAL;
> > -     }
> > +     crop_rect = vimc_sca_pad_crop(vsca, sd_state, sel->which);
> > +     sink_fmt = vimc_sca_pad_format(vsca, sd_state, VIMC_SCA_SINK,
> > +                                    sel->which);
> > +     vimc_sca_adjust_sink_crop(&sel->r, sink_fmt);
> > +     *crop_rect = sel->r;
> >
> >       return 0;
> >  }
> > @@ -344,16 +314,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
> > @@ -382,77 +348,32 @@ 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 snk_width = vsca->fmt[VIMC_SCA_SINK].width;
> > +     unsigned int src_x, src_y;
> > +     u8 *walker = vsca->src_frame;
> > +
> > +     /* Set each pixel at the src_frame to its sink_frame equivalent */
> > +     for (src_y = 0; src_y < src_fmt->height; src_y++) {
> > +             unsigned int snk_y, y_offset;
> > +
> > +             snk_y = (src_y * r->height) / src_fmt->height + r->top;
> > +             y_offset = snk_y * snk_width * vsca->bpp;
> > +
> > +             for (src_x = 0; src_x < src_fmt->width; src_x++) {
> > +                     unsigned int snk_x, x_offset, index;
> > +
> > +                     snk_x = (src_x * r->width) / src_fmt->width + r->left;
> > +                     x_offset = snk_x * vsca->bpp;
> > +                     index = y_offset + x_offset;
> > +                     memcpy(walker, &sink_frame[index], vsca->bpp);
> > +                     walker += vsca->bpp;
> > +             }
> > +     }
> >  }
> >
> >  static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> > @@ -492,8 +413,8 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> >               return ERR_PTR(-ENOMEM);
> >
> >       /* 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,
> > @@ -508,7 +429,8 @@ static struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> >       vsca->ved.dev = vimc->mdev.dev;
> >
> >       /* Initialize the frame format */
> > -     vsca->sink_fmt = sink_fmt_default;
> > +     vsca->fmt[VIMC_SCA_SINK] = fmt_default;
> > +     vsca->fmt[VIMC_SCA_SRC] = fmt_default;
> >
> >       /* Initialize the crop selection */
> >       vsca->crop_rect = crop_rect_default;
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Pedro

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v7] media: vimc: Enable set resolution at the scaler src pad
  2021-08-31 17:48 [PATCH v7] media: vimc: Enable set resolution at the scaler src pad Pedro Terra
  2021-08-31 21:52 ` Laurent Pinchart
@ 2021-09-13  8:28 ` Hans Verkuil
  2021-09-13 17:01   ` Pedro Terra Delboni
  1 sibling, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2021-09-13  8:28 UTC (permalink / raw)
  To: Pedro Terra, dafna.hirschfeld, mchehab, skhan, linux-media,
	linux-kernel, gabrielabittencourt00, gfmandaji, laurent.pinchart

Hi Pedro,

Your 'From' email is Pedro Terra <pedro@terraco.de>, but...

On 31/08/2021 19:48, 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 platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> media-ctl -d platform:vimc -V '"Scaler":0[crop:(100,50)/400x150]'
> media-ctl -d platform:vimc -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=300,height=700
> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
> v4l2-ctl --stream-mmap --stream-count=10 -z platform:vimc -d "RGB/YUV Capture" \
> 	-stream-to=test.raw
> 
> The result will be a cropped stream that can be checked with the command
> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> 	-video_size "300x700" test.raw
> 
> Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> Signed-off-by: Pedro "pirate" Terra <pirate@terraco.de>

... that differs from what is used in this Signed-off-by.

checkpatch.pl complains about that.

Which of the two should I use? I can fix it myself in the pull request,
but I need to know which one I should use.

For future patches, please use consistent email addresses.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v7] media: vimc: Enable set resolution at the scaler src pad
  2021-09-13  8:28 ` Hans Verkuil
@ 2021-09-13 17:01   ` Pedro Terra Delboni
  0 siblings, 0 replies; 5+ messages in thread
From: Pedro Terra Delboni @ 2021-09-13 17:01 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Dafna Hirschfeld, Mauro Carvalho Chehab, Shuah Khan, linux-media,
	linux-kernel, Gabriela Bittencourt, Gabriel Francisco Mandaji,
	Laurent Pinchart

Hello Hans,

On Mon, Sep 13, 2021 at 5:28 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Hi Pedro,
>
> Your 'From' email is Pedro Terra <pedro@terraco.de>, but...
>
> On 31/08/2021 19:48, 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 platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> > media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> > media-ctl -d platform:vimc -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> > media-ctl -d platform:vimc -V '"Scaler":0[crop:(100,50)/400x150]'
> > media-ctl -d platform:vimc -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> > v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=300,height=700
> > v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
> > v4l2-ctl --stream-mmap --stream-count=10 -z platform:vimc -d "RGB/YUV Capture" \
> >       -stream-to=test.raw
> >
> > The result will be a cropped stream that can be checked with the command
> > ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> >       -video_size "300x700" test.raw
> >
> > Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> > Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> > Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> > Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> > Signed-off-by: Pedro "pirate" Terra <pirate@terraco.de>
>
> ... that differs from what is used in this Signed-off-by.
>
> checkpatch.pl complains about that.
>
> Which of the two should I use? I can fix it myself in the pull request,
> but I need to know which one I should use.
I would like to stay with pedro@terraco.de.
>
> For future patches, please use consistent email addresses.
I will, sorry for the trouble.
>
> Regards,
>
>         Hans

Thank you very much,
Pedro

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-09-13 17:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 17:48 [PATCH v7] media: vimc: Enable set resolution at the scaler src pad Pedro Terra
2021-08-31 21:52 ` Laurent Pinchart
2021-09-01  0:06   ` Pedro Terra Delboni
2021-09-13  8:28 ` Hans Verkuil
2021-09-13 17:01   ` Pedro Terra Delboni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).