[PATCH v2 0/3] Renesas R-Car VSP1 rotation support

[PATCH v2 0/3] Renesas R-Car VSP1 rotation support

[Laurent Pinchart]

Hello everybody,

This patch series implement support for rotation in the VSP1 driver. It
contains an update of the rotation support part of the "[PATCH 00/13]
Renesas R-Car VSP: Scaling and rotation support on Gen3" series.

Patch 1/3 starts by fixing the multi-line comment style through the whole
driver to avoid adding new offenders in patch 3/3.

Patch 2/3 is to biggest addition since v1. It clarifies interactions between
formats, controls and buffers in the V4L2 API documentation. The proposal is
based on an IRC meeting held on 2016-09-15 in the #v4l channel on freenode
which I have tried to translate as accurately as possible into documentation.

Patch 3/3 implements rotation support in the vsp1 driver. Beside being rebased
on top of the current vsp1/next branch, it has mostly been kept unchanged
compared to v1.

For your convenience the code is available at

	git://linuxtv.org/pinchartl/media.git vsp1-rotation-v2-20170228

Laurent Pinchart (3):
  v4l: vsp1: Fix multi-line comment style
  v4l: Clearly document interactions between formats, controls and
    buffers
  v4l: vsp1: wpf: Implement rotation support

 Documentation/media/uapi/v4l/buffer.rst   |  88 ++++++++++++
 drivers/media/platform/vsp1/vsp1_bru.c    |  27 ++--
 drivers/media/platform/vsp1/vsp1_dl.c     |  27 ++--
 drivers/media/platform/vsp1/vsp1_drm.c    |  21 ++-
 drivers/media/platform/vsp1/vsp1_drv.c    |  12 +-
 drivers/media/platform/vsp1/vsp1_entity.c |   9 +-
 drivers/media/platform/vsp1/vsp1_hsit.c   |   3 +-
 drivers/media/platform/vsp1/vsp1_lif.c    |   6 +-
 drivers/media/platform/vsp1/vsp1_pipe.c   |   9 +-
 drivers/media/platform/vsp1/vsp1_rpf.c    |  11 +-
 drivers/media/platform/vsp1/vsp1_rwpf.c   |  11 +-
 drivers/media/platform/vsp1/vsp1_rwpf.h   |   3 +-
 drivers/media/platform/vsp1/vsp1_sru.c    |   3 +-
 drivers/media/platform/vsp1/vsp1_uds.c    |   3 +-
 drivers/media/platform/vsp1/vsp1_video.c  |  45 +++++--
 drivers/media/platform/vsp1/vsp1_wpf.c    | 215 +++++++++++++++++++++++-------
 16 files changed, 382 insertions(+), 111 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/3] v4l: vsp1: Fix multi-line comment style

[Laurent Pinchart]

Fix all multi-line comments to comply with the kernel coding style.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_bru.c    | 27 ++++++++++++++++---------
 drivers/media/platform/vsp1/vsp1_dl.c     | 27 ++++++++++++++++---------
 drivers/media/platform/vsp1/vsp1_drm.c    | 21 +++++++++++++-------
 drivers/media/platform/vsp1/vsp1_drv.c    | 12 +++++++----
 drivers/media/platform/vsp1/vsp1_entity.c |  9 ++++++---
 drivers/media/platform/vsp1/vsp1_hsit.c   |  3 ++-
 drivers/media/platform/vsp1/vsp1_lif.c    |  6 ++++--
 drivers/media/platform/vsp1/vsp1_pipe.c   |  9 ++++++---
 drivers/media/platform/vsp1/vsp1_rpf.c    |  9 ++++++---
 drivers/media/platform/vsp1/vsp1_rwpf.c   |  6 ++++--
 drivers/media/platform/vsp1/vsp1_sru.c    |  3 ++-
 drivers/media/platform/vsp1/vsp1_uds.c    |  3 ++-
 drivers/media/platform/vsp1/vsp1_video.c  | 33 ++++++++++++++++++++-----------
 drivers/media/platform/vsp1/vsp1_wpf.c    | 12 +++++++----
 14 files changed, 120 insertions(+), 60 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_bru.c b/drivers/media/platform/vsp1/vsp1_bru.c
index ee8355c28f94..85362c5ef57a 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_bru.c
@@ -251,7 +251,8 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
 	sel->r.left = clamp_t(unsigned int, sel->r.left, 0, format->width - 1);
 	sel->r.top = clamp_t(unsigned int, sel->r.top, 0, format->height - 1);
 
-	/* Scaling isn't supported, the compose rectangle size must be identical
+	/*
+	 * Scaling isn't supported, the compose rectangle size must be identical
 	 * to the sink format size.
 	 */
 	format = vsp1_entity_get_pad_format(&bru->entity, config, sel->pad);
@@ -300,13 +301,15 @@ static void bru_configure(struct vsp1_entity *entity,
 	format = vsp1_entity_get_pad_format(&bru->entity, bru->entity.config,
 					    bru->entity.source_pad);
 
-	/* The hardware is extremely flexible but we have no userspace API to
+	/*
+	 * The hardware is extremely flexible but we have no userspace API to
 	 * expose all the parameters, nor is it clear whether we would have use
 	 * cases for all the supported modes. Let's just harcode the parameters
 	 * to sane default values for now.
 	 */
 
-	/* Disable dithering and enable color data normalization unless the
+	/*
+	 * Disable dithering and enable color data normalization unless the
 	 * format at the pipeline output is premultiplied.
 	 */
 	flags = pipe->output ? pipe->output->format.flags : 0;
@@ -314,7 +317,8 @@ static void bru_configure(struct vsp1_entity *entity,
 		       flags & V4L2_PIX_FMT_FLAG_PREMUL_ALPHA ?
 		       0 : VI6_BRU_INCTRL_NRM);
 
-	/* Set the background position to cover the whole output image and
+	/*
+	 * Set the background position to cover the whole output image and
 	 * configure its color.
 	 */
 	vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_SIZE,
@@ -325,7 +329,8 @@ static void bru_configure(struct vsp1_entity *entity,
 	vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_COL, bru->bgcolor |
 		       (0xff << VI6_BRU_VIRRPF_COL_A_SHIFT));
 
-	/* Route BRU input 1 as SRC input to the ROP unit and configure the ROP
+	/*
+	 * Route BRU input 1 as SRC input to the ROP unit and configure the ROP
 	 * unit with a NOP operation to make BRU input 1 available as the
 	 * Blend/ROP unit B SRC input.
 	 */
@@ -337,7 +342,8 @@ static void bru_configure(struct vsp1_entity *entity,
 		bool premultiplied = false;
 		u32 ctrl = 0;
 
-		/* Configure all Blend/ROP units corresponding to an enabled BRU
+		/*
+		 * Configure all Blend/ROP units corresponding to an enabled BRU
 		 * input for alpha blending. Blend/ROP units corresponding to
 		 * disabled BRU inputs are used in ROP NOP mode to ignore the
 		 * SRC input.
@@ -352,13 +358,15 @@ static void bru_configure(struct vsp1_entity *entity,
 			     |  VI6_BRU_CTRL_AROP(VI6_ROP_NOP);
 		}
 
-		/* Select the virtual RPF as the Blend/ROP unit A DST input to
+		/*
+		 * Select the virtual RPF as the Blend/ROP unit A DST input to
 		 * serve as a background color.
 		 */
 		if (i == 0)
 			ctrl |= VI6_BRU_CTRL_DSTSEL_VRPF;
 
-		/* Route BRU inputs 0 to 3 as SRC inputs to Blend/ROP units A to
+		/*
+		 * Route BRU inputs 0 to 3 as SRC inputs to Blend/ROP units A to
 		 * D in that order. The Blend/ROP unit B SRC is hardwired to the
 		 * ROP unit output, the corresponding register bits must be set
 		 * to 0.
@@ -368,7 +376,8 @@ static void bru_configure(struct vsp1_entity *entity,
 
 		vsp1_bru_write(bru, dl, VI6_BRU_CTRL(i), ctrl);
 
-		/* Harcode the blending formula to
+		/*
+		 * Harcode the blending formula to
 		 *
 		 *	DSTc = DSTc * (1 - SRCa) + SRCc * SRCa
 		 *	DSTa = DSTa * (1 - SRCa) + SRCa
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index ad545aff4e35..7d8f37772b56 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -240,7 +240,8 @@ static struct vsp1_dl_list *vsp1_dl_list_alloc(struct vsp1_dl_manager *dlm)
 	INIT_LIST_HEAD(&dl->fragments);
 	dl->dlm = dlm;
 
-	/* Initialize the display list body and allocate DMA memory for the body
+	/*
+	 * Initialize the display list body and allocate DMA memory for the body
 	 * and the optional header. Both are allocated together to avoid memory
 	 * fragmentation, with the header located right after the body in
 	 * memory.
@@ -511,7 +512,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
 		goto done;
 	}
 
-	/* Once the UPD bit has been set the hardware can start processing the
+	/*
+	 * Once the UPD bit has been set the hardware can start processing the
 	 * display list at any time and we can't touch the address and size
 	 * registers. In that case mark the update as pending, it will be
 	 * queued up to the hardware by the frame end interrupt handler.
@@ -523,7 +525,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
 		goto done;
 	}
 
-	/* Program the hardware with the display list body address and size.
+	/*
+	 * Program the hardware with the display list body address and size.
 	 * The UPD bit will be cleared by the device when the display list is
 	 * processed.
 	 */
@@ -547,7 +550,8 @@ void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm)
 {
 	spin_lock(&dlm->lock);
 
-	/* The display start interrupt signals the end of the display list
+	/*
+	 * The display start interrupt signals the end of the display list
 	 * processing by the device. The active display list, if any, won't be
 	 * accessed anymore and can be reused.
 	 */
@@ -566,14 +570,16 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 	__vsp1_dl_list_put(dlm->active);
 	dlm->active = NULL;
 
-	/* Header mode is used for mem-to-mem pipelines only. We don't need to
+	/*
+	 * Header mode is used for mem-to-mem pipelines only. We don't need to
 	 * perform any operation as there can't be any new display list queued
 	 * in that case.
 	 */
 	if (dlm->mode == VSP1_DL_MODE_HEADER)
 		goto done;
 
-	/* The UPD bit set indicates that the commit operation raced with the
+	/*
+	 * The UPD bit set indicates that the commit operation raced with the
 	 * interrupt and occurred after the frame end event and UPD clear but
 	 * before interrupt processing. The hardware hasn't taken the update
 	 * into account yet, we'll thus skip one frame and retry.
@@ -581,7 +587,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 	if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD)
 		goto done;
 
-	/* The device starts processing the queued display list right after the
+	/*
+	 * The device starts processing the queued display list right after the
 	 * frame end interrupt. The display list thus becomes active.
 	 */
 	if (dlm->queued) {
@@ -589,7 +596,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 		dlm->queued = NULL;
 	}
 
-	/* Now that the UPD bit has been cleared we can queue the next display
+	/*
+	 * Now that the UPD bit has been cleared we can queue the next display
 	 * list to the hardware if one has been prepared.
 	 */
 	if (dlm->pending) {
@@ -615,7 +623,8 @@ void vsp1_dlm_setup(struct vsp1_device *vsp1)
 		 | VI6_DL_CTRL_DC2 | VI6_DL_CTRL_DC1 | VI6_DL_CTRL_DC0
 		 | VI6_DL_CTRL_DLE;
 
-	/* The DRM pipeline operates with display lists in Continuous Frame
+	/*
+	 * The DRM pipeline operates with display lists in Continuous Frame
 	 * Mode, all other pipelines use manual start.
 	 */
 	if (vsp1->drm)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index d7ec980300dd..8b982960ba8d 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -83,7 +83,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 		__func__, width, height);
 
 	if (width == 0 || height == 0) {
-		/* Zero width or height means the CRTC is being disabled, stop
+		/*
+		 * Zero width or height means the CRTC is being disabled, stop
 		 * the pipeline and turn the light off.
 		 */
 		ret = vsp1_pipeline_stop(pipe);
@@ -108,7 +109,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 		return 0;
 	}
 
-	/* Configure the format at the BRU sinks and propagate it through the
+	/*
+	 * Configure the format at the BRU sinks and propagate it through the
 	 * pipeline.
 	 */
 	memset(&format, 0, sizeof(format));
@@ -177,7 +179,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 		__func__, format.format.width, format.format.height,
 		format.format.code);
 
-	/* Verify that the format at the output of the pipeline matches the
+	/*
+	 * Verify that the format at the output of the pipeline matches the
 	 * requested frame size and media bus code.
 	 */
 	if (format.format.width != width || format.format.height != height ||
@@ -186,7 +189,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 		return -EPIPE;
 	}
 
-	/* Mark the pipeline as streaming and enable the VSP1. This will store
+	/*
+	 * Mark the pipeline as streaming and enable the VSP1. This will store
 	 * the pipeline pointer in all entities, which the s_stream handlers
 	 * will need. We don't start the entities themselves right at this point
 	 * as there's no plane configured yet, so we can't start processing
@@ -318,7 +322,8 @@ static int vsp1_du_setup_rpf_pipe(struct vsp1_device *vsp1,
 	const struct v4l2_rect *crop;
 	int ret;
 
-	/* Configure the format on the RPF sink pad and propagate it up to the
+	/*
+	 * Configure the format on the RPF sink pad and propagate it up to the
 	 * BRU sink pad.
 	 */
 	crop = &vsp1->drm->inputs[rpf->entity.index].crop;
@@ -357,7 +362,8 @@ static int vsp1_du_setup_rpf_pipe(struct vsp1_device *vsp1,
 		__func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
 		rpf->entity.index);
 
-	/* RPF source, hardcode the format to ARGB8888 to turn on format
+	/*
+	 * RPF source, hardcode the format to ARGB8888 to turn on format
 	 * conversion if needed.
 	 */
 	format.pad = RWPF_PAD_SOURCE;
@@ -529,7 +535,8 @@ int vsp1_drm_create_links(struct vsp1_device *vsp1)
 	unsigned int i;
 	int ret;
 
-	/* VSPD instances require a BRU to perform composition and a LIF to
+	/*
+	 * VSPD instances require a BRU to perform composition and a LIF to
 	 * output to the DU.
 	 */
 	if (!vsp1->bru || !vsp1->lif)
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index aa237b48ad55..8d1e61b353bb 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -170,7 +170,8 @@ static int vsp1_uapi_create_links(struct vsp1_device *vsp1)
 	}
 
 	for (i = 0; i < vsp1->info->wpf_count; ++i) {
-		/* Connect the video device to the WPF. All connections are
+		/*
+		 * Connect the video device to the WPF. All connections are
 		 * immutable.
 		 */
 		struct vsp1_rwpf *wpf = vsp1->wpf[i];
@@ -227,7 +228,8 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
 	media_device_init(mdev);
 
 	vsp1->media_ops.link_setup = vsp1_entity_link_setup;
-	/* Don't perform link validation when the userspace API is disabled as
+	/*
+	 * Don't perform link validation when the userspace API is disabled as
 	 * the pipeline is configured internally by the driver in that case, and
 	 * its configuration can thus be trusted.
 	 */
@@ -279,7 +281,8 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
 
 	list_add_tail(&vsp1->hst->entity.list_dev, &vsp1->entities);
 
-	/* The LIF is only supported when used in conjunction with the DU, in
+	/*
+	 * The LIF is only supported when used in conjunction with the DU, in
 	 * which case the userspace API is disabled. If the userspace API is
 	 * enabled skip the LIF, even when present.
 	 */
@@ -391,7 +394,8 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
 	if (ret < 0)
 		goto done;
 
-	/* Register subdev nodes if the userspace API is enabled or initialize
+	/*
+	 * Register subdev nodes if the userspace API is enabled or initialize
 	 * the DRM pipeline otherwise.
 	 */
 	if (vsp1->info->uapi) {
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index da673495c222..12eca5660d6e 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -199,7 +199,8 @@ int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev,
 		struct v4l2_subdev_pad_config *config;
 		struct v4l2_mbus_framefmt *format;
 
-		/* The entity can't perform format conversion, the sink format
+		/*
+		 * The entity can't perform format conversion, the sink format
 		 * is always identical to the source format.
 		 */
 		if (code->index)
@@ -263,7 +264,8 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,
 		fse->min_height = min_height;
 		fse->max_height = max_height;
 	} else {
-		/* The size on the source pad are fixed and always identical to
+		/*
+		 * The size on the source pad are fixed and always identical to
 		 * the size on the sink pad.
 		 */
 		fse->min_width = format->width;
@@ -407,7 +409,8 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
 
 	vsp1_entity_init_cfg(subdev, NULL);
 
-	/* Allocate the pad configuration to store formats and selection
+	/*
+	 * Allocate the pad configuration to store formats and selection
 	 * rectangles.
 	 */
 	entity->config = v4l2_subdev_alloc_pad_config(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_hsit.c b/drivers/media/platform/vsp1/vsp1_hsit.c
index 94316afc54ff..764d405345ee 100644
--- a/drivers/media/platform/vsp1/vsp1_hsit.c
+++ b/drivers/media/platform/vsp1/vsp1_hsit.c
@@ -84,7 +84,8 @@ static int hsit_set_format(struct v4l2_subdev *subdev,
 	format = vsp1_entity_get_pad_format(&hsit->entity, config, fmt->pad);
 
 	if (fmt->pad == HSIT_PAD_SOURCE) {
-		/* The HST and HSI output format code and resolution can't be
+		/*
+		 * The HST and HSI output format code and resolution can't be
 		 * modified.
 		 */
 		fmt->format = *format;
diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
index e32acae1fc6e..702487f895b3 100644
--- a/drivers/media/platform/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/vsp1/vsp1_lif.c
@@ -84,7 +84,8 @@ static int lif_set_format(struct v4l2_subdev *subdev,
 	format = vsp1_entity_get_pad_format(&lif->entity, config, fmt->pad);
 
 	if (fmt->pad == LIF_PAD_SOURCE) {
-		/* The LIF source format is always identical to its sink
+		/*
+		 * The LIF source format is always identical to its sink
 		 * format.
 		 */
 		fmt->format = *format;
@@ -176,7 +177,8 @@ struct vsp1_lif *vsp1_lif_create(struct vsp1_device *vsp1)
 	lif->entity.ops = &lif_entity_ops;
 	lif->entity.type = VSP1_ENTITY_LIF;
 
-	/* The LIF is never exposed to userspace, but media entity registration
+	/*
+	 * The LIF is never exposed to userspace, but media entity registration
 	 * requires a function to be set. Use PROC_VIDEO_PIXEL_FORMATTER just to
 	 * avoid triggering a WARN_ON(), the value won't be seen anywhere.
 	 */
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 280ba0804699..3f1acf68dc6e 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -251,7 +251,8 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
 	int ret;
 
 	if (pipe->lif) {
-		/* When using display lists in continuous frame mode the only
+		/*
+		 * When using display lists in continuous frame mode the only
 		 * way to stop the pipeline is to reset the hardware.
 		 */
 		ret = vsp1_reset_wpf(pipe->output->entity.vsp1,
@@ -322,7 +323,8 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
 	if (!pipe->uds)
 		return;
 
-	/* The BRU background color has a fixed alpha value set to 255, the
+	/*
+	 * The BRU background color has a fixed alpha value set to 255, the
 	 * output alpha value is thus always equal to 255.
 	 */
 	if (pipe->uds_input->type == VSP1_ENTITY_BRU)
@@ -337,7 +339,8 @@ void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
 	unsigned int i;
 	int ret;
 
-	/* To avoid increasing the system suspend time needlessly, loop over the
+	/*
+	 * To avoid increasing the system suspend time needlessly, loop over the
 	 * pipelines twice, first to set them all to the stopping state, and
 	 * then to wait for the stop to complete.
 	 */
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index 1d0944f308ae..f5a9a4c8c74d 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -195,7 +195,8 @@ static void rpf_configure(struct vsp1_entity *entity,
 		       (left << VI6_RPF_LOC_HCOORD_SHIFT) |
 		       (top << VI6_RPF_LOC_VCOORD_SHIFT));
 
-	/* On Gen2 use the alpha channel (extended to 8 bits) when available or
+	/*
+	 * On Gen2 use the alpha channel (extended to 8 bits) when available or
 	 * a fixed alpha value set through the V4L2_CID_ALPHA_COMPONENT control
 	 * otherwise.
 	 *
@@ -225,7 +226,8 @@ static void rpf_configure(struct vsp1_entity *entity,
 		u32 mult;
 
 		if (fmtinfo->alpha) {
-			/* When the input contains an alpha channel enable the
+			/*
+			 * When the input contains an alpha channel enable the
 			 * alpha multiplier. If the input is premultiplied we
 			 * need to multiply both the alpha channel and the pixel
 			 * components by the global alpha value to keep them
@@ -240,7 +242,8 @@ static void rpf_configure(struct vsp1_entity *entity,
 				VI6_RPF_MULT_ALPHA_P_MMD_RATIO :
 				VI6_RPF_MULT_ALPHA_P_MMD_NONE);
 		} else {
-			/* When the input doesn't contain an alpha channel the
+			/*
+			 * When the input doesn't contain an alpha channel the
 			 * global alpha value is applied in the unpacking unit,
 			 * the alpha multiplier isn't needed and must be
 			 * disabled.
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c
index 04104ef28fb5..7d52c88a583e 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
@@ -86,7 +86,8 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
 	format = vsp1_entity_get_pad_format(&rwpf->entity, config, fmt->pad);
 
 	if (fmt->pad == RWPF_PAD_SOURCE) {
-		/* The RWPF performs format conversion but can't scale, only the
+		/*
+		 * The RWPF performs format conversion but can't scale, only the
 		 * format code can be changed on the source pad.
 		 */
 		format->code = fmt->format.code;
@@ -205,7 +206,8 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev,
 	format = vsp1_entity_get_pad_format(&rwpf->entity, config,
 					    RWPF_PAD_SINK);
 
-	/* Restrict the crop rectangle coordinates to multiples of 2 to avoid
+	/*
+	 * Restrict the crop rectangle coordinates to multiples of 2 to avoid
 	 * shifting the color plane.
 	 */
 	if (format->code == MEDIA_BUS_FMT_AYUV8_1X32) {
diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
index b4e568a3b4ed..30142793dfcd 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -191,7 +191,8 @@ static void sru_try_format(struct vsp1_sru *sru,
 						    SRU_PAD_SINK);
 		fmt->code = format->code;
 
-		/* We can upscale by 2 in both direction, but not independently.
+		/*
+		 * We can upscale by 2 in both direction, but not independently.
 		 * Compare the input and output rectangles areas (avoiding
 		 * integer overflows on the output): if the requested output
 		 * area is larger than 1.5^2 the input area upscale by two,
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index da8f89a31ea4..4226403ad235 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -293,7 +293,8 @@ static void uds_configure(struct vsp1_entity *entity,
 
 	dev_dbg(uds->entity.vsp1->dev, "hscale %u vscale %u\n", hscale, vscale);
 
-	/* Multi-tap scaling can't be enabled along with alpha scaling when
+	/*
+	 * Multi-tap scaling can't be enabled along with alpha scaling when
 	 * scaling down with a factor lower than or equal to 1/2 in either
 	 * direction.
 	 */
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 6c1b79f7aea5..5239e08fabc3 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -103,7 +103,8 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
 	unsigned int height = pix->height;
 	unsigned int i;
 
-	/* Backward compatibility: replace deprecated RGB formats by their XRGB
+	/*
+	 * Backward compatibility: replace deprecated RGB formats by their XRGB
 	 * equivalent. This selects the format older userspace applications want
 	 * while still exposing the new format.
 	 */
@@ -114,7 +115,8 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
 		}
 	}
 
-	/* Retrieve format information and select the default format if the
+	/*
+	 * Retrieve format information and select the default format if the
 	 * requested format isn't supported.
 	 */
 	info = vsp1_get_format_info(video->vsp1, pix->pixelformat);
@@ -140,7 +142,8 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
 	pix->height = clamp(height, VSP1_VIDEO_MIN_HEIGHT,
 			    VSP1_VIDEO_MAX_HEIGHT);
 
-	/* Compute and clamp the stride and image size. While not documented in
+	/*
+	 * Compute and clamp the stride and image size. While not documented in
 	 * the datasheet, strides not aligned to a multiple of 128 bytes result
 	 * in image corruption.
 	 */
@@ -449,7 +452,8 @@ static void vsp1_video_pipeline_frame_end(struct vsp1_pipeline *pipe)
 	state = pipe->state;
 	pipe->state = VSP1_PIPELINE_STOPPED;
 
-	/* If a stop has been requested, mark the pipeline as stopped and
+	/*
+	 * If a stop has been requested, mark the pipeline as stopped and
 	 * return. Otherwise restart the pipeline if ready.
 	 */
 	if (state == VSP1_PIPELINE_STOPPING)
@@ -491,7 +495,8 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 		entity = to_vsp1_entity(
 			media_entity_to_v4l2_subdev(pad->entity));
 
-		/* A BRU is present in the pipeline, store the BRU input pad
+		/*
+		 * A BRU is present in the pipeline, store the BRU input pad
 		 * number in the input RPF for use when configuring the RPF.
 		 */
 		if (entity->type == VSP1_ENTITY_BRU) {
@@ -526,7 +531,8 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 					: &input->entity;
 		}
 
-		/* Follow the source link. The link setup operations ensure
+		/*
+		 * Follow the source link. The link setup operations ensure
 		 * that the output fan-out can't be more than one, there is thus
 		 * no need to verify here that only a single source link is
 		 * activated.
@@ -596,7 +602,8 @@ static int vsp1_video_pipeline_build(struct vsp1_pipeline *pipe,
 	if (pipe->num_inputs == 0 || !pipe->output)
 		return -EPIPE;
 
-	/* Follow links downstream for each input and make sure the graph
+	/*
+	 * Follow links downstream for each input and make sure the graph
 	 * contains no loop and that all branches end at the output WPF.
 	 */
 	for (i = 0; i < video->vsp1->info->rpf_count; ++i) {
@@ -627,7 +634,8 @@ static struct vsp1_pipeline *vsp1_video_pipeline_get(struct vsp1_video *video)
 	struct vsp1_pipeline *pipe;
 	int ret;
 
-	/* Get a pipeline object for the video node. If a pipeline has already
+	/*
+	 * Get a pipeline object for the video node. If a pipeline has already
 	 * been allocated just increment its reference count and return it.
 	 * Otherwise allocate a new pipeline and initialize it, it will be freed
 	 * when the last reference is released.
@@ -767,7 +775,8 @@ static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
 	if (pipe->uds) {
 		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
 
-		/* If a BRU is present in the pipeline before the UDS, the alpha
+		/*
+		 * If a BRU is present in the pipeline before the UDS, the alpha
 		 * component doesn't need to be scaled as the BRU output alpha
 		 * value is fixed to 255. Otherwise we need to scale the alpha
 		 * component only when available at the input RPF.
@@ -981,7 +990,8 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (video->queue.owner && video->queue.owner != file->private_data)
 		return -EBUSY;
 
-	/* Get a pipeline for the video node and start streaming on it. No link
+	/*
+	 * Get a pipeline for the video node and start streaming on it. No link
 	 * touching an entity in the pipeline can be activated or deactivated
 	 * once streaming is started.
 	 */
@@ -1001,7 +1011,8 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	/* Verify that the configured format matches the output of the connected
+	/*
+	 * Verify that the configured format matches the output of the connected
 	 * subdev.
 	 */
 	ret = vsp1_video_verify_format(video);
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 052a83e2d489..25a2ed6e2e18 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -88,12 +88,14 @@ static int wpf_init_controls(struct vsp1_rwpf *wpf)
 		/* Only WPF0 supports flipping. */
 		num_flip_ctrls = 0;
 	} else if (vsp1->info->features & VSP1_HAS_WPF_HFLIP) {
-		/* When horizontal flip is supported the WPF implements two
+		/*
+		 * When horizontal flip is supported the WPF implements two
 		 * controls (horizontal flip and vertical flip).
 		 */
 		num_flip_ctrls = 2;
 	} else if (vsp1->info->features & VSP1_HAS_WPF_VFLIP) {
-		/* When only vertical flip is supported the WPF implements a
+		/*
+		 * When only vertical flip is supported the WPF implements a
 		 * single control (vertical flip).
 		 */
 		num_flip_ctrls = 1;
@@ -139,7 +141,8 @@ static int wpf_s_stream(struct v4l2_subdev *subdev, int enable)
 	if (enable)
 		return 0;
 
-	/* Write to registers directly when stopping the stream as there will be
+	/*
+	 * Write to registers directly when stopping the stream as there will be
 	 * no pipeline run to apply the display list.
 	 */
 	vsp1_write(vsp1, VI6_WPF_IRQ_ENB(wpf->entity.index), 0);
@@ -336,7 +339,8 @@ static void wpf_configure(struct vsp1_entity *entity,
 
 	vsp1_dl_list_write(dl, VI6_WPF_WRBCK_CTRL, 0);
 
-	/* Sources. If the pipeline has a single input and BRU is not used,
+	/*
+	 * Sources. If the pipeline has a single input and BRU is not used,
 	 * configure it as the master layer. Otherwise configure all
 	 * inputs as sub-layers and select the virtual RPF as the master
 	 * layer.
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

V4L2 exposes parameters that influence buffers sizes through the format
ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
not part of the format structure may also influence buffer sizes or
buffer layout in general. One existing such parameter is rotation, which
is implemented by the VIDIOC_ROTATE control and thus exposed through the
V4L2 control ioctls.

The interaction between those parameters and buffers is currently only
partially specified by the V4L2 API. In particular interactions between
controls and buffers isn't specified at all. The behaviour of the
VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
specified.

This commit clearly defines and documents the interactions between
formats, controls and buffers.

The preparatory discussions for the documentation change considered
completely disallowing controls that change the buffer size or layout,
in favour of extending the format API with a new ioctl that would bundle
those controls with format information. The idea has been rejected, as
this would essentially be a restricted version of the upcoming request
API that wouldn't bring any additional value.

Another option we have considered was to mandate the use of the request
API to modify controls that influence buffer size or layout. This has
also been rejected on the grounds that requiring the request API to
change rotation even when streaming is stopped would significantly
complicate implementation of drivers and usage of the V4L2 API for
applications.

Applications will however be required to use the upcoming request API to
change at runtime formats or controls that influence the buffer size or
layout, because of the need to synchronize buffers with the formats and
controls. Otherwise there would be no way to interpret the content of a
buffer correctly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 Documentation/media/uapi/v4l/buffer.rst | 88 +++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ac58966ccb9b..5c58db98ab7a 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
 buffer.
 
 
+Interactions between formats, controls and buffers
+==================================================
+
+V4L2 exposes parameters that influence the buffer size, or the way data is
+laid out in the buffer. Those parameters are exposed through both formats and
+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
+that modifies the direction in which pixels are stored in the buffer, as well
+as the buffer size when the selected format includes padding at the end of
+lines.
+
+The set of information needed to interpret the content of a buffer (e.g. the
+pixel format, the line stride, the tiling orientation or the rotation) is
+collectively referred to in the rest of this section as the buffer layout.
+
+Modifying formats or controls that influence the buffer size or layout require
+the stream to be stopped. Any attempt at such a modification while the stream
+is active shall cause the format or control set ioctl to return the ``EBUSY``
+error code.
+
+Controls that only influence the buffer layout can be modified at any time
+when the stream is stopped. As they don't influence the buffer size, no
+special handling is needed to synchronize those controls with buffer
+allocation.
+
+Formats and controls that influence the buffer size interact with buffer
+allocation. As buffer allocation is an expensive operation, drivers should
+allow format or controls that influence the buffer size to be changed with
+buffers allocated. A typical ioctl sequence to modify format and controls is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_S_FMT
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+Queued buffers must be large enough for the new format or controls.
+
+Drivers shall return a ``ENOSPC`` error in response to format change
+(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
+:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
+currently queued. As a simplification, drivers are allowed to return an error
+from these ioctls if any buffer is currently queued, without checking the
+queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
+:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
+current format or controls. Together, these requirements ensure that queued
+buffers will always be large enough for the configured format and controls.
+
+Userspace applications can query the buffer size required for a given format
+and controls by first setting the desired control values and then trying the
+desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
+buffer size.
+
+ #. VIDIOC_S_EXT_CTRLS(x)
+ #. VIDIOC_TRY_FMT()
+ #. VIDIOC_S_EXT_CTRLS(y)
+ #. VIDIOC_TRY_FMT()
+
+The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
+based on the queried sizes (for instance by allocating a set of buffers large
+enough for all the desired formats and controls, or by allocating separate set
+of appropriately sized buffers for each use case).
+
+To simplify their implementation, drivers may also require buffers to be
+reallocated in order to change formats or controls that influence the buffer
+size. In that case, to perform such changes, userspace applications shall
+first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
+is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
+they are allocated. The format or controls can then be modified, and buffers
+shall then be reallocated and the stream restarted. A typical ioctl sequence
+is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_REQBUFS(0)
+ #. VIDIOC_S_FMT
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_REQBUFS(n)
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
+value into account to compute the buffer size to allocate. Applications can
+also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
+
+When reallocation is required, any attempt to modify format or controls that
+influences the buffer size while buffers are allocated shall cause the format
+or control set ioctl to return the ``EBUSY`` error code.
+
+
 .. c:type:: v4l2_buffer
 
 struct v4l2_buffer
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Sakari Ailus]

Hi Laurent,

On Tue, Feb 28, 2017 at 05:03:19PM +0200, Laurent Pinchart wrote:
> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
> not part of the format structure may also influence buffer sizes or
> buffer layout in general. One existing such parameter is rotation, which
> is implemented by the VIDIOC_ROTATE control and thus exposed through the
> V4L2 control ioctls.
> 
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
> specified.
> 
> This commit clearly defines and documents the interactions between
> formats, controls and buffers.
> 
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
> 
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
> 
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 88 +++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..5c58db98ab7a 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>  buffer.
>  
>  
> +Interactions between formats, controls and buffers
> +==================================================
> +
> +V4L2 exposes parameters that influence the buffer size, or the way data is
> +laid out in the buffer. Those parameters are exposed through both formats and
> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> +that modifies the direction in which pixels are stored in the buffer, as well
> +as the buffer size when the selected format includes padding at the end of
> +lines.
> +
> +The set of information needed to interpret the content of a buffer (e.g. the
> +pixel format, the line stride, the tiling orientation or the rotation) is
> +collectively referred to in the rest of this section as the buffer layout.
> +
> +Modifying formats or controls that influence the buffer size or layout require
> +the stream to be stopped. Any attempt at such a modification while the stream
> +is active shall cause the format or control set ioctl to return the ``EBUSY``
> +error code.
> +
> +Controls that only influence the buffer layout can be modified at any time
> +when the stream is stopped. As they don't influence the buffer size, no
> +special handling is needed to synchronize those controls with buffer
> +allocation.
> +
> +Formats and controls that influence the buffer size interact with buffer
> +allocation. As buffer allocation is an expensive operation, drivers should
> +allow format or controls that influence the buffer size to be changed with
> +buffers allocated. A typical ioctl sequence to modify format and controls is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_S_FMT
> + #. VIDIOC_S_EXT_CTRLS

Which one do you set first, the format or the controls? Supposedly the user
would have to get the format again after setting the ROTATE control.

> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +Queued buffers must be large enough for the new format or controls.
> +
> +Drivers shall return a ``ENOSPC`` error in response to format change
> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> +currently queued. As a simplification, drivers are allowed to return an error
> +from these ioctls if any buffer is currently queued, without checking the
> +queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
> +:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
> +current format or controls. Together, these requirements ensure that queued
> +buffers will always be large enough for the configured format and controls.
> +
> +Userspace applications can query the buffer size required for a given format
> +and controls by first setting the desired control values and then trying the
> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> +buffer size.
> +
> + #. VIDIOC_S_EXT_CTRLS(x)
> + #. VIDIOC_TRY_FMT()
> + #. VIDIOC_S_EXT_CTRLS(y)
> + #. VIDIOC_TRY_FMT()
> +
> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> +based on the queried sizes (for instance by allocating a set of buffers large
> +enough for all the desired formats and controls, or by allocating separate set
> +of appropriately sized buffers for each use case).
> +
> +To simplify their implementation, drivers may also require buffers to be
> +reallocated in order to change formats or controls that influence the buffer
> +size. In that case, to perform such changes, userspace applications shall
> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> +they are allocated. The format or controls can then be modified, and buffers
> +shall then be reallocated and the stream restarted. A typical ioctl sequence
> +is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_REQBUFS(0)
> + #. VIDIOC_S_FMT
> + #. VIDIOC_S_EXT_CTRLS

Same here.

Would it be safe to say that controls are changed first? I wonder if there
could be special cases where this wouldn't apply though. It could ultimately
come down to hardware features: rotation might be only available for certain
formats so you'd need to change the format first to enable rotation.

What you're documenting above is a typical sequence so it doesn't have to be
applicable to all potential hardware. I might mention there could be such
dependencies. I wonder if one exists at the moment. No?

> + #. VIDIOC_REQBUFS(n)
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> +value into account to compute the buffer size to allocate. Applications can
> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error code.
> +
> +
>  .. c:type:: v4l2_buffer
>  
>  struct v4l2_buffer

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

On 02/03/17 16:37, Sakari Ailus wrote:
> Hi Laurent,
>
> On Tue, Feb 28, 2017 at 05:03:19PM +0200, Laurent Pinchart wrote:
>> V4L2 exposes parameters that influence buffers sizes through the format
>> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
>> not part of the format structure may also influence buffer sizes or
>> buffer layout in general. One existing such parameter is rotation, which
>> is implemented by the VIDIOC_ROTATE control and thus exposed through the
>> V4L2 control ioctls.
>>
>> The interaction between those parameters and buffers is currently only
>> partially specified by the V4L2 API. In particular interactions between
>> controls and buffers isn't specified at all. The behaviour of the
>> VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
>> specified.
>>
>> This commit clearly defines and documents the interactions between
>> formats, controls and buffers.
>>
>> The preparatory discussions for the documentation change considered
>> completely disallowing controls that change the buffer size or layout,
>> in favour of extending the format API with a new ioctl that would bundle
>> those controls with format information. The idea has been rejected, as
>> this would essentially be a restricted version of the upcoming request
>> API that wouldn't bring any additional value.
>>
>> Another option we have considered was to mandate the use of the request
>> API to modify controls that influence buffer size or layout. This has
>> also been rejected on the grounds that requiring the request API to
>> change rotation even when streaming is stopped would significantly
>> complicate implementation of drivers and usage of the V4L2 API for
>> applications.
>>
>> Applications will however be required to use the upcoming request API to
>> change at runtime formats or controls that influence the buffer size or
>> layout, because of the need to synchronize buffers with the formats and
>> controls. Otherwise there would be no way to interpret the content of a
>> buffer correctly.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> ---
>>  Documentation/media/uapi/v4l/buffer.rst | 88 +++++++++++++++++++++++++++++++++
>>  1 file changed, 88 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>> index ac58966ccb9b..5c58db98ab7a 100644
>> --- a/Documentation/media/uapi/v4l/buffer.rst
>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>> @@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>>  buffer.
>>
>>
>> +Interactions between formats, controls and buffers
>> +==================================================
>> +
>> +V4L2 exposes parameters that influence the buffer size, or the way data is
>> +laid out in the buffer. Those parameters are exposed through both formats and
>> +controls. One example of such a control is the ``V4L2_CI, and also documented in the documentation of
those ioctls (if that isn't done already). The latter can be done later, or D_ROTATE`` control
>> +that modifies the direction in which pixels are stored in the buffer, as well
>> +as the buffer size when the selected format includes padding at the end of
>> +lines.
>> +
>> +The set of information needed to interpret the content of a buffer (e.g. the
>> +pixel format, the line stride, the tiling orientation or the rotation) is
>> +collectively referred to in the rest of this section as the buffer layout.
>> +
>> +Modifying formats or controls that influence the buffer size or layout require
>> +the stream to be stopped. Any attempt at such a modification while the stream
>> +is active shall cause the format or control set ioctl to return the ``EBUSY``
>> +error code.
>> +
>> +Controls that only influence the buffer layout can be modified at any time
>> +when the stream is stopped. As they don't influence the buffer size, no
>> +special handling is needed to synchronize those controls with buffer
>> +allocation.
>> +
>> +Formats and controls that influence the buffer size interact with buffer
>> +allocation. As buffer allocation is an expensive operation, drivers should
>> +allow format or controls that influence the buffer size to be changed with
>> +buffers allocated. A typical ioctl sequence to modify format and controls is
>> +
>> + #. VIDIOC_STREAMOFF
>> + #. VIDIOC_S_FMT
>> + #. VIDIOC_S_EXT_CTRLS
>
> Which one do you set first, the format or the controls? Supposedly the user
> would have to get the format again after setting the ROTATE control.
>
>> + #. VIDIOC_QBUF
>> + #. VIDIOC_STREAMON
>> +
>> +Queued buffers must be large enough for the new format or controls.
>> +
>> +Drivers shall return a ``ENOSPC`` error in response to format change
>> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
>> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
>> +currently queued. As a simplification, drivers are allowed to return an error
>> +from these ioctls if any buffer is currently queued, without checking the
>> +queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
>> +:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
>> +current format or controls. Together, these requirements ensure that queued
>> +buffers will always be large enough for the configured format and controls.
>> +
>> +Userspace applications can query the buffer size required for a given format
>> +and controls by first setting the desired control values and then trying the
>> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
>> +buffer size.
>> +
>> + #. VIDIOC_S_EXT_CTRLS(x)
>> + #. VIDIOC_TRY_FMT()
>> + #. VIDIOC_S_EXT_CTRLS(y)
>> + #. VIDIOC_TRY_FMT()
>> +
>> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
>> +based on the queried sizes (for instance by allocating a set of buffers large
>> +enough for all the desired formats and controls, or by allocating separate set
>> +of appropriately sized buffers for each use case).
>> +
>> +To simplify their implementation, drivers may also require buffers to be
>> +reallocated in order to change formats or controls that influence the buffer
>> +size. In that case, to perform such changes, userspace applications shall
>> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
>> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
>> +they are allocated. The format or controls can then be modified, and buffers
>> +shall then be reallocated and the stream restarted. A typical ioctl sequence
>> +is
>> +
>> + #. VIDIOC_STREAMOFF
>> + #. VIDIOC_REQBUFS(0)
>> + #. VIDIOC_S_FMT
>> + #. VIDIOC_S_EXT_CTRLS
>
> Same here.
>
> Would it be safe to say that controls are changed first? I wonder if there
> could be special cases where this wouldn't apply though. It could ultimately
> come down to hardware features: rotation might be only available for certain
> formats so you'd need to change the format first to enable rotation.
>
> What you're documenting above is a typical sequence so it doesn't have to be
> applicable to all potential hardware. I might mention there could be such
> dependencies. I wonder if one exists at the moment. No?

The way V4L2 works is that the last ioctl called gets 'preference'. So the
driver should attempt to satisfy the ioctl, even if that means undoing previous
ioctls. In other words, V4L2 allows any order, but the end-result might be
different depending on the hardware capabilities.

Regards,

	Hans

>
>> + #. VIDIOC_REQBUFS(n)
>> + #. VIDIOC_QBUF
>> + #. VIDIOC_STREAMON
>> +
>> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
>> +value into account to compute the buffer size to allocate. Applications can
>> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
>> +
>> +When reallocation is required, any attempt to modify format or controls that
>> +influences the buffer size while buffers are allocated shall cause the format
>> +or control set ioctl to return the ``EBUSY`` error code.
>> +
>> +
>>  .. c:type:: v4l2_buffer
>>
>>  struct v4l2_buffer
>

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Sakari Ailus]

Hi Hans,

On Sat, Mar 04, 2017 at 11:57:32AM +0100, Hans Verkuil wrote:
...
> >>+To simplify their implementation, drivers may also require buffers to be
> >>+reallocated in order to change formats or controls that influence the buffer
> >>+size. In that case, to perform such changes, userspace applications shall
> >>+first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> >>+is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> >>+they are allocated. The format or controls can then be modified, and buffers
> >>+shall then be reallocated and the stream restarted. A typical ioctl sequence
> >>+is
> >>+
> >>+ #. VIDIOC_STREAMOFF
> >>+ #. VIDIOC_REQBUFS(0)
> >>+ #. VIDIOC_S_FMT
> >>+ #. VIDIOC_S_EXT_CTRLS
> >
> >Same here.
> >
> >Would it be safe to say that controls are changed first? I wonder if there
> >could be special cases where this wouldn't apply though. It could ultimately
> >come down to hardware features: rotation might be only available for certain
> >formats so you'd need to change the format first to enable rotation.
> >
> >What you're documenting above is a typical sequence so it doesn't have to be
> >applicable to all potential hardware. I might mention there could be such
> >dependencies. I wonder if one exists at the moment. No?
> 
> The way V4L2 works is that the last ioctl called gets 'preference'. So the
> driver should attempt to satisfy the ioctl, even if that means undoing previous
> ioctls. In other words, V4L2 allows any order, but the end-result might be
> different depending on the hardware capabilities.

Indeed. But the above sequence suggests that formats are set before
controls. I suggested to clarify that part.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

Hi Sakari,

On Saturday 04 Mar 2017 15:48:54 Sakari Ailus wrote:
> On Sat, Mar 04, 2017 at 11:57:32AM +0100, Hans Verkuil wrote:
> ...
> 
> >>> +To simplify their implementation, drivers may also require buffers to
> >>> be +reallocated in order to change formats or controls that influence
> >>> the buffer +size. In that case, to perform such changes, userspace
> >>> applications shall +first stop the video stream with the
> >>> :c:func:`VIDIOC_STREAMOFF` ioctl if it +is running and free all buffers
> >>> with the :c:func:`VIDIOC_REQBUFS` ioctl if +they are allocated. The
> >>> format or controls can then be modified, and buffers +shall then be
> >>> reallocated and the stream restarted. A typical ioctl sequence +is
> >>> +
> >>> + #. VIDIOC_STREAMOFF
> >>> + #. VIDIOC_REQBUFS(0)
> >>> + #. VIDIOC_S_FMT
> >>> + #. VIDIOC_S_EXT_CTRLS
> >>
> >> Same here.
> >>
> >> Would it be safe to say that controls are changed first? I wonder if
> >> there could be special cases where this wouldn't apply though. It could
> >> ultimately come down to hardware features: rotation might be only
> >> available for certain formats so you'd need to change the format first
> >> to enable rotation.
> >>
> >> What you're documenting above is a typical sequence so it doesn't have to
> >> be applicable to all potential hardware. I might mention there could be
> >> such dependencies. I wonder if one exists at the moment. No?
> > 
> > The way V4L2 works is that the last ioctl called gets 'preference'. So the
> > driver should attempt to satisfy the ioctl, even if that means undoing
> > previous ioctls. In other words, V4L2 allows any order, but the
> > end-result might be different depending on the hardware capabilities.
> 
> Indeed. But the above sequence suggests that formats are set before
> controls. I suggested to clarify that part.

I agree with both of you. I'll clarify that this is just an example and that 
formats and controls can be set in a different order (or even interleaved).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

Hi Laurent,

Here is my review:

On 28/02/17 16:03, Laurent Pinchart wrote:
> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters

S_SELECTION should be mentioned here as well (more about that later).

> not part of the format structure may also influence buffer sizes or
> buffer layout in general. One existing such parameter is rotation, which
> is implemented by the VIDIOC_ROTATE control and thus exposed through the
> V4L2 control ioctls.
>
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
> specified.
>
> This commit clearly defines and documents the interactions between
> formats, controls and buffers.
>
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
>
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
>
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 88 +++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..5c58db98ab7a 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>  buffer.
>
>
> +Interactions between formats, controls and buffers
> +==================================================
> +
> +V4L2 exposes parameters that influence the buffer size, or the way data is
> +laid out in the buffer. Those parameters are exposed through both formats and
> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> +that modifies the direction in which pixels are stored in the buffer, as well
> +as the buffer size when the selected format includes padding at the end of
> +lines.
> +
> +The set of information needed to interpret the content of a buffer (e.g. the
> +pixel format, the line stride, the tiling orientation or the rotation) is
> +collectively referred to in the rest of this section as the buffer layout.
> +
> +Modifying formats or controls that influence the buffer size or layout require
> +the stream to be stopped. Any attempt at such a modification while the stream
> +is active shall cause the format or control set ioctl to return the ``EBUSY``
> +error code.

This is not what happens today: it's not the streaming part that causes EBUSY to
be returned but whether or not buffers are allocated.

Today we do not support changing buffer sizes on the fly, so any attempt to
call an ioctl that would change the buffer size is blocked and EBUSY is returned.
To be precise: drivers call vb2_is_busy() to determine this.

To my knowledge all vb2-using drivers behave like this. There may be old drivers
that do not do this (and these have a high likelyhood of being wrong).

> +
> +Controls that only influence the buffer layout can be modified at any time
> +when the stream is stopped. As they don't influence the buffer size, no
> +special handling is needed to synchronize those controls with buffer
> +allocation.
> +
> +Formats and controls that influence the buffer size interact with buffer
> +allocation. As buffer allocation is an expensive operation, drivers should
> +allow format or controls that influence the buffer size to be changed with
> +buffers allocated. A typical ioctl sequence to modify format and controls is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_S_FMT
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +Queued buffers must be large enough for the new format or controls.
> +
> +Drivers shall return a ``ENOSPC`` error in response to format change
> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> +currently queued. As a simplification, drivers are allowed to return an error
> +from these ioctls if any buffer is currently queued, without checking the
> +queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
> +:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
> +current format or controls.

Actually, today qbuf will return -EINVAL (from __verify_length in videobuf2-v4l2.c)
in these cases. I am pretty sure you can't change that to ENOSPC since this has
always been -EINVAL, and so changing this would break the ABI.

Trying to change the format while buffers are allocated will return EBUSY today.

If you are trying to document what will happen when drivers allow format changes
on the fly then this is not at all clear from what you write here.

So:

If the driver does not support changing the format while buffers are queued, then
it will return EBUSY (true for almost (?) all drivers today). If it does support
this, then it will behave as described above, except for the ENOSPC error in QBUF.

Note that the meaning of ENOSPC should also be explicitly documented in the ioctls
that can return this.

 > Together, these requirements ensure that queued
> +buffers will always be large enough for the configured format and controls.
> +
> +Userspace applications can query the buffer size required for a given format
> +and controls by first setting the desired control values and then trying the
> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> +buffer size.
> +
> + #. VIDIOC_S_EXT_CTRLS(x)
> + #. VIDIOC_TRY_FMT()
> + #. VIDIOC_S_EXT_CTRLS(y)
> + #. VIDIOC_TRY_FMT()
> +
> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> +based on the queried sizes (for instance by allocating a set of buffers large
> +enough for all the desired formats and controls, or by allocating separate set
> +of appropriately sized buffers for each use case).
> +
> +To simplify their implementation, drivers may also require buffers to be
> +reallocated in order to change formats or controls that influence the buffer
> +size. In that case, to perform such changes, userspace applications shall
> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> +they are allocated. The format or controls can then be modified, and buffers
> +shall then be reallocated and the stream restarted. A typical ioctl sequence
> +is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_REQBUFS(0)
> + #. VIDIOC_S_FMT
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_REQBUFS(n)
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> +value into account to compute the buffer size to allocate. Applications can
> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error code.

Ah, here you describe the 99% situation. This should come first. You've been working
on the 1% that allows changing formats while buffers are allocated so that feels
all-important to you, but in reality almost all drivers use the 'simplified'
implementation. Describe that first, then go on describing what will happen for
your driver. That will make much more sense to me (as you can tell from the preceding
comments) and I'm sure to the end-user as well.

What should be mentioned here as well is that S_SELECTION can also implicitly change
the format, specifically if there is no scaler or if the scaler has limitations.

Also there are a few ioctls that can reset selections and formats when called:
S_INPUT/S_OUTPUT, S_STD and S_DV_TIMINGS.

I think this should be mentioned here. It can be just in passing, no need to go
in-depth on that. As long as people are aware of it.

The documentation of S_STD and S_DV_TIMINGS should also be updated saying that if
the new std or timings differ from the existing std or timings, then the format
will also change and selection rectangles will be reset to the defaults. Setting
the std/dv_timings with the current std or timings will not do anything: there
are applications that do this even when streaming so this should be allowed.

Unfortunately, this is not documented but it really should.

If you don't have time to update the S_STD/DV_TIMINGS ioctls, then let me know and
I will do that.

> +
> +
>  .. c:type:: v4l2_buffer
>
>  struct v4l2_buffer
>

Regards,

	Hans

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Sakari Ailus]

Hi Hans (and Laurent).

On Sat, Mar 04, 2017 at 11:53:45AM +0100, Hans Verkuil wrote:
> Hi Laurent,
> 
> Here is my review:
> 
> On 28/02/17 16:03, Laurent Pinchart wrote:
> >V4L2 exposes parameters that influence buffers sizes through the format
> >ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
> 
> S_SELECTION should be mentioned here as well (more about that later).
> 
> >not part of the format structure may also influence buffer sizes or
> >buffer layout in general. One existing such parameter is rotation, which
> >is implemented by the VIDIOC_ROTATE control and thus exposed through the
> >V4L2 control ioctls.
> >
> >The interaction between those parameters and buffers is currently only
> >partially specified by the V4L2 API. In particular interactions between
> >controls and buffers isn't specified at all. The behaviour of the
> >VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
> >specified.
> >
> >This commit clearly defines and documents the interactions between
> >formats, controls and buffers.
> >
> >The preparatory discussions for the documentation change considered
> >completely disallowing controls that change the buffer size or layout,
> >in favour of extending the format API with a new ioctl that would bundle
> >those controls with format information. The idea has been rejected, as
> >this would essentially be a restricted version of the upcoming request
> >API that wouldn't bring any additional value.
> >
> >Another option we have considered was to mandate the use of the request
> >API to modify controls that influence buffer size or layout. This has
> >also been rejected on the grounds that requiring the request API to
> >change rotation even when streaming is stopped would significantly
> >complicate implementation of drivers and usage of the V4L2 API for
> >applications.
> >
> >Applications will however be required to use the upcoming request API to
> >change at runtime formats or controls that influence the buffer size or
> >layout, because of the need to synchronize buffers with the formats and
> >controls. Otherwise there would be no way to interpret the content of a
> >buffer correctly.
> >
> >Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >---
> > Documentation/media/uapi/v4l/buffer.rst | 88 +++++++++++++++++++++++++++++++++
> > 1 file changed, 88 insertions(+)
> >
> >diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> >index ac58966ccb9b..5c58db98ab7a 100644
> >--- a/Documentation/media/uapi/v4l/buffer.rst
> >+++ b/Documentation/media/uapi/v4l/buffer.rst
> >@@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
> > buffer.
> >
> >
> >+Interactions between formats, controls and buffers
> >+==================================================
> >+
> >+V4L2 exposes parameters that influence the buffer size, or the way data is
> >+laid out in the buffer. Those parameters are exposed through both formats and
> >+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> >+that modifies the direction in which pixels are stored in the buffer, as well
> >+as the buffer size when the selected format includes padding at the end of
> >+lines.
> >+
> >+The set of information needed to interpret the content of a buffer (e.g. the
> >+pixel format, the line stride, the tiling orientation or the rotation) is
> >+collectively referred to in the rest of this section as the buffer layout.
> >+
> >+Modifying formats or controls that influence the buffer size or layout require
> >+the stream to be stopped. Any attempt at such a modification while the stream
> >+is active shall cause the format or control set ioctl to return the ``EBUSY``
> >+error code.
> 
> This is not what happens today: it's not the streaming part that causes EBUSY to
> be returned but whether or not buffers are allocated.
> 
> Today we do not support changing buffer sizes on the fly, so any attempt to
> call an ioctl that would change the buffer size is blocked and EBUSY is returned.
> To be precise: drivers call vb2_is_busy() to determine this.

It certainly shouldn't be like that. Not allowing S_FMT() while there are
buffers allocated makes CREATE_BUFS entirely useless.

> 
> To my knowledge all vb2-using drivers behave like this. There may be old drivers
> that do not do this (and these have a high likelyhood of being wrong).

What's really needed is that the driver verifies that the buffer is large
enough to be used for a given format. vb2_is_busy() shouldn't be used to
check whether setting format is allowed.

> 
> >+
> >+Controls that only influence the buffer layout can be modified at any time
> >+when the stream is stopped. As they don't influence the buffer size, no
> >+special handling is needed to synchronize those controls with buffer
> >+allocation.
> >+
> >+Formats and controls that influence the buffer size interact with buffer
> >+allocation. As buffer allocation is an expensive operation, drivers should
> >+allow format or controls that influence the buffer size to be changed with
> >+buffers allocated. A typical ioctl sequence to modify format and controls is
> >+
> >+ #. VIDIOC_STREAMOFF
> >+ #. VIDIOC_S_FMT
> >+ #. VIDIOC_S_EXT_CTRLS
> >+ #. VIDIOC_QBUF
> >+ #. VIDIOC_STREAMON
> >+
> >+Queued buffers must be large enough for the new format or controls.
> >+
> >+Drivers shall return a ``ENOSPC`` error in response to format change
> >+(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> >+:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> >+currently queued. As a simplification, drivers are allowed to return an error
> >+from these ioctls if any buffer is currently queued, without checking the
> >+queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
> >+:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
> >+current format or controls.
> 
> Actually, today qbuf will return -EINVAL (from __verify_length in videobuf2-v4l2.c)
> in these cases. I am pretty sure you can't change that to ENOSPC since this has
> always been -EINVAL, and so changing this would break the ABI.

*If* today drivers return -EBUSY on S_FMT if there are buffers allocated,
you can't have this happening in the first place: it is a new condition so
using a new error code is possible. I would not expect applications to try
that either for the same reason.

It is rather that applications designed for a particular device are more
likely to attempt this (e.g. capturing a still image after streaming
viewfinder first).

I realised -EBUSY is not even documented for S_FMT; I'll post a patch to fix
that.

> 
> Trying to change the format while buffers are allocated will return EBUSY today.
> 
> If you are trying to document what will happen when drivers allow format changes
> on the fly then this is not at all clear from what you write here.
> 
> So:
> 
> If the driver does not support changing the format while buffers are queued, then
> it will return EBUSY (true for almost (?) all drivers today). If it does support
> this, then it will behave as described above, except for the ENOSPC error in QBUF.
> 
> Note that the meaning of ENOSPC should also be explicitly documented in the ioctls
> that can return this.
> 
> > Together, these requirements ensure that queued
> >+buffers will always be large enough for the configured format and controls.
> >+
> >+Userspace applications can query the buffer size required for a given format
> >+and controls by first setting the desired control values and then trying the
> >+desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> >+buffer size.
> >+
> >+ #. VIDIOC_S_EXT_CTRLS(x)
> >+ #. VIDIOC_TRY_FMT()
> >+ #. VIDIOC_S_EXT_CTRLS(y)
> >+ #. VIDIOC_TRY_FMT()
> >+
> >+The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> >+based on the queried sizes (for instance by allocating a set of buffers large
> >+enough for all the desired formats and controls, or by allocating separate set
> >+of appropriately sized buffers for each use case).
> >+
> >+To simplify their implementation, drivers may also require buffers to be
> >+reallocated in order to change formats or controls that influence the buffer
> >+size. In that case, to perform such changes, userspace applications shall
> >+first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> >+is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> >+they are allocated. The format or controls can then be modified, and buffers
> >+shall then be reallocated and the stream restarted. A typical ioctl sequence
> >+is
> >+
> >+ #. VIDIOC_STREAMOFF
> >+ #. VIDIOC_REQBUFS(0)
> >+ #. VIDIOC_S_FMT
> >+ #. VIDIOC_S_EXT_CTRLS
> >+ #. VIDIOC_REQBUFS(n)
> >+ #. VIDIOC_QBUF
> >+ #. VIDIOC_STREAMON
> >+
> >+The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> >+value into account to compute the buffer size to allocate. Applications can
> >+also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> >+
> >+When reallocation is required, any attempt to modify format or controls that
> >+influences the buffer size while buffers are allocated shall cause the format
> >+or control set ioctl to return the ``EBUSY`` error code.
> 
> Ah, here you describe the 99% situation. This should come first. You've been working
> on the 1% that allows changing formats while buffers are allocated so that feels
> all-important to you, but in reality almost all drivers use the 'simplified'
> implementation. Describe that first, then go on describing what will happen for
> your driver. That will make much more sense to me (as you can tell from the preceding
> comments) and I'm sure to the end-user as well.
> 
> What should be mentioned here as well is that S_SELECTION can also implicitly change
> the format, specifically if there is no scaler or if the scaler has limitations.

I think it'd be fine to mention that, but the effect is indeed implicit
through the change in format.

> 
> Also there are a few ioctls that can reset selections and formats when called:
> S_INPUT/S_OUTPUT, S_STD and S_DV_TIMINGS.
> 
> I think this should be mentioned here. It can be just in passing, no need to go
> in-depth on that. As long as people are aware of it.
> 
> The documentation of S_STD and S_DV_TIMINGS should also be updated saying that if
> the new std or timings differ from the existing std or timings, then the format
> will also change and selection rectangles will be reset to the defaults. Setting
> the std/dv_timings with the current std or timings will not do anything: there
> are applications that do this even when streaming so this should be allowed.
> 
> Unfortunately, this is not documented but it really should.
> 
> If you don't have time to update the S_STD/DV_TIMINGS ioctls, then let me know and
> I will do that.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

On 04/03/17 15:37, Sakari Ailus wrote:
> Hi Hans (and Laurent).
>
> On Sat, Mar 04, 2017 at 11:53:45AM +0100, Hans Verkuil wrote:
>> Hi Laurent,
>>
>> Here is my review:
>>
>> On 28/02/17 16:03, Laurent Pinchart wrote:
>>> V4L2 exposes parameters that influence buffers sizes through the format
>>> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
>>
>> S_SELECTION should be mentioned here as well (more about that later).
>>
>>> not part of the format structure may also influence buffer sizes or
>>> buffer layout in general. One existing such parameter is rotation, which
>>> is implemented by the VIDIOC_ROTATE control and thus exposed through the
>>> V4L2 control ioctls.
>>>
>>> The interaction between those parameters and buffers is currently only
>>> partially specified by the V4L2 API. In particular interactions between
>>> controls and buffers isn't specified at all. The behaviour of the
>>> VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
>>> specified.
>>>
>>> This commit clearly defines and documents the interactions between
>>> formats, controls and buffers.
>>>
>>> The preparatory discussions for the documentation change considered
>>> completely disallowing controls that change the buffer size or layout,
>>> in favour of extending the format API with a new ioctl that would bundle
>>> those controls with format information. The idea has been rejected, as
>>> this would essentially be a restricted version of the upcoming request
>>> API that wouldn't bring any additional value.
>>>
>>> Another option we have considered was to mandate the use of the request
>>> API to modify controls that influence buffer size or layout. This has
>>> also been rejected on the grounds that requiring the request API to
>>> change rotation even when streaming is stopped would significantly
>>> complicate implementation of drivers and usage of the V4L2 API for
>>> applications.
>>>
>>> Applications will however be required to use the upcoming request API to
>>> change at runtime formats or controls that influence the buffer size or
>>> layout, because of the need to synchronize buffers with the formats and
>>> controls. Otherwise there would be no way to interpret the content of a
>>> buffer correctly.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>> Documentation/media/uapi/v4l/buffer.rst | 88 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 88 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>>> index ac58966ccb9b..5c58db98ab7a 100644
>>> --- a/Documentation/media/uapi/v4l/buffer.rst
>>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>>> @@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>>> buffer.
>>>
>>>
>>> +Interactions between formats, controls and buffers
>>> +==================================================
>>> +
>>> +V4L2 exposes parameters that influence the buffer size, or the way data is
>>> +laid out in the buffer. Those parameters are exposed through both formats and
>>> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
>>> +that modifies the direction in which pixels are stored in the buffer, as well
>>> +as the buffer size when the selected format includes padding at the end of
>>> +lines.
>>> +
>>> +The set of information needed to interpret the content of a buffer (e.g. the
>>> +pixel format, the line stride, the tiling orientation or the rotation) is
>>> +collectively referred to in the rest of this section as the buffer layout.
>>> +
>>> +Modifying formats or controls that influence the buffer size or layout require
>>> +the stream to be stopped. Any attempt at such a modification while the stream
>>> +is active shall cause the format or control set ioctl to return the ``EBUSY``
>>> +error code.
>>
>> This is not what happens today: it's not the streaming part that causes EBUSY to
>> be returned but whether or not buffers are allocated.
>>
>> Today we do not support changing buffer sizes on the fly, so any attempt to
>> call an ioctl that would change the buffer size is blocked and EBUSY is returned.
>> To be precise: drivers call vb2_is_busy() to determine this.
>
> It certainly shouldn't be like that. Not allowing S_FMT() while there are
> buffers allocated makes CREATE_BUFS entirely useless.

Well, CREATE_BUFS can still be used to make large buffers in which the image
is composed. But yes, CREATE_BUFS has limited usefulness today, but the simple
fact is that nobody wrote drivers that really can do on the fly format changes.

>
>>
>> To my knowledge all vb2-using drivers behave like this. There may be old drivers
>> that do not do this (and these have a high likelyhood of being wrong).
>
> What's really needed is that the driver verifies that the buffer is large
> enough to be used for a given format. vb2_is_busy() shouldn't be used to
> check whether setting format is allowed.

Again, there are no drivers that support this. Using vb2_is_busy() prevents
userspace from trying this, since without proper driver support this *will*
fail.

Of course, drivers that do support this will not test vb2_is_busy but the
vb2_is_streaming instead (and with the request API even that can be dropped).

>
>>
>>> +
>>> +Controls that only influence the buffer layout can be modified at any time
>>> +when the stream is stopped. As they don't influence the buffer size, no
>>> +special handling is needed to synchronize those controls with buffer
>>> +allocation.
>>> +
>>> +Formats and controls that influence the buffer size interact with buffer
>>> +allocation. As buffer allocation is an expensive operation, drivers should
>>> +allow format or controls that influence the buffer size to be changed with
>>> +buffers allocated. A typical ioctl sequence to modify format and controls is
>>> +
>>> + #. VIDIOC_STREAMOFF
>>> + #. VIDIOC_S_FMT
>>> + #. VIDIOC_S_EXT_CTRLS
>>> + #. VIDIOC_QBUF
>>> + #. VIDIOC_STREAMON
>>> +
>>> +Queued buffers must be large enough for the new format or controls.
>>> +
>>> +Drivers shall return a ``ENOSPC`` error in response to format change
>>> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
>>> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
>>> +currently queued. As a simplification, drivers are allowed to return an error
>>> +from these ioctls if any buffer is currently queued, without checking the
>>> +queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
>>> +:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
>>> +current format or controls.
>>
>> Actually, today qbuf will return -EINVAL (from __verify_length in videobuf2-v4l2.c)
>> in these cases. I am pretty sure you can't change that to ENOSPC since this has
>> always been -EINVAL, and so changing this would break the ABI.
>
> *If* today drivers return -EBUSY on S_FMT if there are buffers allocated,
> you can't have this happening in the first place: it is a new condition so
> using a new error code is possible. I would not expect applications to try
> that either for the same reason.

Yes, this can happen when you pass in USERPTR or DMABUF buffers. It indeed can't
happen with MMAP buffers.

> It is rather that applications designed for a particular device are more
> likely to attempt this (e.g. capturing a still image after streaming
> viewfinder first).
>
> I realised -EBUSY is not even documented for S_FMT; I'll post a patch to fix
> that.
>
>>
>> Trying to change the format while buffers are allocated will return EBUSY today.
>>
>> If you are trying to document what will happen when drivers allow format changes
>> on the fly then this is not at all clear from what you write here.
>>
>> So:
>>
>> If the driver does not support changing the format while buffers are queued, then
>> it will return EBUSY (true for almost (?) all drivers today). If it does support
>> this, then it will behave as described above, except for the ENOSPC error in QBUF.
>>
>> Note that the meaning of ENOSPC should also be explicitly documented in the ioctls
>> that can return this.
>>
>>> Together, these requirements ensure that queued
>>> +buffers will always be large enough for the configured format and controls.
>>> +
>>> +Userspace applications can query the buffer size required for a given format
>>> +and controls by first setting the desired control values and then trying the
>>> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
>>> +buffer size.
>>> +
>>> + #. VIDIOC_S_EXT_CTRLS(x)
>>> + #. VIDIOC_TRY_FMT()
>>> + #. VIDIOC_S_EXT_CTRLS(y)
>>> + #. VIDIOC_TRY_FMT()
>>> +
>>> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
>>> +based on the queried sizes (for instance by allocating a set of buffers large
>>> +enough for all the desired formats and controls, or by allocating separate set
>>> +of appropriately sized buffers for each use case).
>>> +
>>> +To simplify their implementation, drivers may also require buffers to be
>>> +reallocated in order to change formats or controls that influence the buffer
>>> +size. In that case, to perform such changes, userspace applications shall
>>> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
>>> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
>>> +they are allocated. The format or controls can then be modified, and buffers
>>> +shall then be reallocated and the stream restarted. A typical ioctl sequence
>>> +is
>>> +
>>> + #. VIDIOC_STREAMOFF
>>> + #. VIDIOC_REQBUFS(0)
>>> + #. VIDIOC_S_FMT
>>> + #. VIDIOC_S_EXT_CTRLS
>>> + #. VIDIOC_REQBUFS(n)
>>> + #. VIDIOC_QBUF
>>> + #. VIDIOC_STREAMON
>>> +
>>> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
>>> +value into account to compute the buffer size to allocate. Applications can
>>> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
>>> +
>>> +When reallocation is required, any attempt to modify format or controls that
>>> +influences the buffer size while buffers are allocated shall cause the format
>>> +or control set ioctl to return the ``EBUSY`` error code.
>>
>> Ah, here you describe the 99% situation. This should come first. You've been working
>> on the 1% that allows changing formats while buffers are allocated so that feels
>> all-important to you, but in reality almost all drivers use the 'simplified'
>> implementation. Describe that first, then go on describing what will happen for
>> your driver. That will make much more sense to me (as you can tell from the preceding
>> comments) and I'm sure to the end-user as well.
>>
>> What should be mentioned here as well is that S_SELECTION can also implicitly change
>> the format, specifically if there is no scaler or if the scaler has limitations.
>
> I think it'd be fine to mention that, but the effect is indeed implicit
> through the change in format.
>
>>
>> Also there are a few ioctls that can reset selections and formats when called:
>> S_INPUT/S_OUTPUT, S_STD and S_DV_TIMINGS.
>>
>> I think this should be mentioned here. It can be just in passing, no need to go
>> in-depth on that. As long as people are aware of it.
>>
>> The documentation of S_STD and S_DV_TIMINGS should also be updated saying that if
>> the new std or timings differ from the existing std or timings, then the format
>> will also change and selection rectangles will be reset to the defaults. Setting
>> the std/dv_timings with the current std or timings will not do anything: there
>> are applications that do this even when streaming so this should be allowed.
>>
>> Unfortunately, this is not documented but it really should.
>>
>> If you don't have time to update the S_STD/DV_TIMINGS ioctls, then let me know and
>> I will do that.
>

Regards,

	Hans

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

Hi Hans,

On Saturday 04 Mar 2017 11:53:45 Hans Verkuil wrote:
> On 28/02/17 16:03, Laurent Pinchart wrote:
> > V4L2 exposes parameters that influence buffers sizes through the format
> > ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
> 
> S_SELECTION should be mentioned here as well (more about that later).

I'll update that according to the conversation we had on IRC about this topic.

> > not part of the format structure may also influence buffer sizes or
> > buffer layout in general. One existing such parameter is rotation, which
> > is implemented by the VIDIOC_ROTATE control and thus exposed through the
> > V4L2 control ioctls.
> > 
> > The interaction between those parameters and buffers is currently only
> > partially specified by the V4L2 API. In particular interactions between
> > controls and buffers isn't specified at all. The behaviour of the
> > VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
> > specified.
> > 
> > This commit clearly defines and documents the interactions between
> > formats, controls and buffers.
> > 
> > The preparatory discussions for the documentation change considered
> > completely disallowing controls that change the buffer size or layout,
> > in favour of extending the format API with a new ioctl that would bundle
> > those controls with format information. The idea has been rejected, as
> > this would essentially be a restricted version of the upcoming request
> > API that wouldn't bring any additional value.
> > 
> > Another option we have considered was to mandate the use of the request
> > API to modify controls that influence buffer size or layout. This has
> > also been rejected on the grounds that requiring the request API to
> > change rotation even when streaming is stopped would significantly
> > complicate implementation of drivers and usage of the V4L2 API for
> > applications.
> > 
> > Applications will however be required to use the upcoming request API to
> > change at runtime formats or controls that influence the buffer size or
> > layout, because of the need to synchronize buffers with the formats and
> > controls. Otherwise there would be no way to interpret the content of a
> > buffer correctly.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  Documentation/media/uapi/v4l/buffer.rst | 88 ++++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> > 
> > diff --git a/Documentation/media/uapi/v4l/buffer.rst
> > b/Documentation/media/uapi/v4l/buffer.rst index
> > ac58966ccb9b..5c58db98ab7a 100644
> > --- a/Documentation/media/uapi/v4l/buffer.rst
> > +++ b/Documentation/media/uapi/v4l/buffer.rst
> > @@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the
> > CAPTURE video> 
> >  buffer.
> > 
> > +Interactions between formats, controls and buffers
> > +==================================================
> > +
> > +V4L2 exposes parameters that influence the buffer size, or the way data
> > is
> > +laid out in the buffer. Those parameters are exposed through both formats
> > and +controls. One example of such a control is the ``V4L2_CID_ROTATE``
> > control +that modifies the direction in which pixels are stored in the
> > buffer, as well +as the buffer size when the selected format includes
> > padding at the end of +lines.
> > +
> > +The set of information needed to interpret the content of a buffer (e.g.
> > the +pixel format, the line stride, the tiling orientation or the
> > rotation) is +collectively referred to in the rest of this section as the
> > buffer layout. +
> > +Modifying formats or controls that influence the buffer size or layout
> > require +the stream to be stopped. Any attempt at such a modification
> > while the stream +is active shall cause the format or control set ioctl
> > to return the ``EBUSY`` +error code.
> 
> This is not what happens today: it's not the streaming part that causes
> EBUSY to be returned but whether or not buffers are allocated.

Note that this patch documents the recommended behaviour we want to achieve. 
It can differ from the behaviour implemented today as long as today's 
behaviour (at least the behaviour of the drivers that we don't consider as 
non-compliant, which is the vast majority) is not contradicted by this patch. 
In other words, we can recommend here a more generic versatile and powerful 
behaviour as long as the more restricted behaviour implemented today is also 
allowed.

> Today we do not support changing buffer sizes on the fly, so any attempt to
> call an ioctl that would change the buffer size is blocked and EBUSY is
> returned. To be precise: drivers call vb2_is_busy() to determine this.
> 
> To my knowledge all vb2-using drivers behave like this. There may be old
> drivers that do not do this (and these have a high likelyhood of being
> wrong).

Please also note that the above description doesn't contradict the behaviour 
implemented today. It explains that changing formats and controls is not 
allowed while streaming, which is a subset of the restriction implemented by 
many drivers through vb2_is_busy() of not allowing changes when buffers are 
allocated.

Please see below for more about this in response to your "here's the 99% case" 
comment.

> > +
> > +Controls that only influence the buffer layout can be modified at any
> > time
> > +when the stream is stopped. As they don't influence the buffer size, no
> > +special handling is needed to synchronize those controls with buffer
> > +allocation.
> > +
> > +Formats and controls that influence the buffer size interact with buffer
> > +allocation. As buffer allocation is an expensive operation, drivers
> > should
> > +allow format or controls that influence the buffer size to be changed
> > with
> > +buffers allocated. A typical ioctl sequence to modify format and controls
> > is +
> > + #. VIDIOC_STREAMOFF
> > + #. VIDIOC_S_FMT
> > + #. VIDIOC_S_EXT_CTRLS
> > + #. VIDIOC_QBUF
> > + #. VIDIOC_STREAMON
> > +
> > +Queued buffers must be large enough for the new format or controls.
> > +
> > +Drivers shall return a ``ENOSPC`` error in response to format change
> > +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> > +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> > +currently queued. As a simplification, drivers are allowed to return an
> > error +from these ioctls if any buffer is currently queued, without
> > checking the +queued buffers sizes. Drivers shall also return a
> > ``ENOSPC`` error from the +:c:func:`VIDIOC_QBUF` ioctl if the buffer
> > being queued is too small for the +current format or controls.
> 
> Actually, today qbuf will return -EINVAL (from __verify_length in
> videobuf2-v4l2.c) in these cases.

No, __verify_length() only checks that the bytesused value doesn't exceed the 
buffer length (and only for output buffers, not for capture buffers). 
videobuf2 isn't format-aware, so it doesn't check the buffer length against 
formats and controls.

> I am pretty sure you can't change that to ENOSPC since this has always been
> -EINVAL, and so changing this would break the ABI.

As videobuf2 doesn't perform any such check today, I'm not changing the return 
value :-)

> Trying to change the format while buffers are allocated will return EBUSY
> today.
> 
> If you are trying to document what will happen when drivers allow format
> changes on the fly then this is not at all clear from what you write here.
> 
> So:
> 
> If the driver does not support changing the format while buffers are queued,
> then it will return EBUSY (true for almost (?) all drivers today). If it
> does support this, then it will behave as described above, except for the
> ENOSPC error in QBUF.
> 
> Note that the meaning of ENOSPC should also be explicitly documented in the
> ioctls that can return this.

Agreed.

> > Together, these requirements ensure that queued
> > 
> > +buffers will always be large enough for the configured format and
> > controls. +
> > +Userspace applications can query the buffer size required for a given
> > format +and controls by first setting the desired control values and then
> > trying the +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will
> > return the required +buffer size.
> > +
> > + #. VIDIOC_S_EXT_CTRLS(x)
> > + #. VIDIOC_TRY_FMT()
> > + #. VIDIOC_S_EXT_CTRLS(y)
> > + #. VIDIOC_TRY_FMT()
> > +
> > +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate
> > buffers +based on the queried sizes (for instance by allocating a set of
> > buffers large +enough for all the desired formats and controls, or by
> > allocating separate set +of appropriately sized buffers for each use
> > case).
> > +
> > +To simplify their implementation, drivers may also require buffers to be
> > +reallocated in order to change formats or controls that influence the
> > buffer +size. In that case, to perform such changes, userspace
> > applications shall +first stop the video stream with the
> > :c:func:`VIDIOC_STREAMOFF` ioctl if it +is running and free all buffers
> > with the :c:func:`VIDIOC_REQBUFS` ioctl if +they are allocated. The
> > format or controls can then be modified, and buffers +shall then be
> > reallocated and the stream restarted. A typical ioctl sequence +is
> > +
> > + #. VIDIOC_STREAMOFF
> > + #. VIDIOC_REQBUFS(0)
> > + #. VIDIOC_S_FMT
> > + #. VIDIOC_S_EXT_CTRLS
> > + #. VIDIOC_REQBUFS(n)
> > + #. VIDIOC_QBUF
> > + #. VIDIOC_STREAMON
> > +
> > +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and
> > control +value into account to compute the buffer size to allocate.
> > Applications can +also retrieve the size by calling the
> > :c:func:`VIDIOC_G_FMT` ioctl if needed. +
> > +When reallocation is required, any attempt to modify format or controls
> > that +influences the buffer size while buffers are allocated shall cause
> > the format +or control set ioctl to return the ``EBUSY`` error code.
> 
> Ah, here you describe the 99% situation. This should come first. You've been
> working on the 1% that allows changing formats while buffers are allocated
> so that feels all-important to you, but in reality almost all drivers use
> the 'simplified' implementation. Describe that first, then go on describing
> what will happen for your driver. That will make much more sense to me (as
> you can tell from the preceding comments) and I'm sure to the end-user as
> well.

This was actually my first, non published version. After discussing it with 
Sakari, we decided to change the description. As Sakari mentioned, the 
CREATE_BUFS ioctl becomes quite pointless if we don't allow changes with 
buffers allocated. We obviously can't mandate it, as most drivers don't allow 
it today, but Sakari and I thought we should recommend it. That's what I tried 
to convey in this patch by describing the recommended option first.

The approach I took here was to say

- formats and controls can't be changed while streaming in any case
- when buffers are allocated but the stream is off, drivers can allow changes
- drivers are not required to allow changes while buffers are allocated

What's your opinion about that ?

> What should be mentioned here as well is that S_SELECTION can also
> implicitly change the format, specifically if there is no scaler or if the
> scaler has limitations.
>
> Also there are a few ioctls that can reset selections and formats when
> called: S_INPUT/S_OUTPUT, S_STD and S_DV_TIMINGS.
>
> I think this should be mentioned here. It can be just in passing, no need to
> go in-depth on that. As long as people are aware of it.

Agreed, I'll mention all of these.

> The documentation of S_STD and S_DV_TIMINGS should also be updated saying
> that if the new std or timings differ from the existing std or timings,
> then the format will also change and selection rectangles will be reset to
> the defaults. Setting the std/dv_timings with the current std or timings
> will not do anything: there are applications that do this even when
> streaming so this should be allowed.
> 
> Unfortunately, this is not documented but it really should.
> 
> If you don't have time to update the S_STD/DV_TIMINGS ioctls, then let me
> know and I will do that.

I likely won't have time to do that before leaving for holidays so I'd 
appreciate if you could update the documentation of those ioctls. I would also 
appreciate if we could get this patch merged (or rather a more recent version 
thereof) without being blocked by the S_STD/DV_TIMINGS documentation update 
:-)

> > +
> > +
> > 
> >  .. c:type:: v4l2_buffer
> >  
> >  struct v4l2_buffer

I'll post a v3 now that addresses all your comments except the one about 
describing the most restrictive option first. I'm not completely against 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

On 05/03/17 15:35, Laurent Pinchart wrote:
> Hi Hans,
>
> On Saturday 04 Mar 2017 11:53:45 Hans Verkuil wrote:
>> On 28/02/17 16:03, Laurent Pinchart wrote:
>>> V4L2 exposes parameters that influence buffers sizes through the format
>>> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
>>
>> S_SELECTION should be mentioned here as well (more about that later).
>
> I'll update that according to the conversation we had on IRC about this topic.
>
>>> not part of the format structure may also influence buffer sizes or
>>> buffer layout in general. One existing such parameter is rotation, which
>>> is implemented by the VIDIOC_ROTATE control and thus exposed through the
>>> V4L2 control ioctls.
>>>
>>> The interaction between those parameters and buffers is currently only
>>> partially specified by the V4L2 API. In particular interactions between
>>> controls and buffers isn't specified at all. The behaviour of the
>>> VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
>>> specified.
>>>
>>> This commit clearly defines and documents the interactions between
>>> formats, controls and buffers.
>>>
>>> The preparatory discussions for the documentation change considered
>>> completely disallowing controls that change the buffer size or layout,
>>> in favour of extending the format API with a new ioctl that would bundle
>>> those controls with format information. The idea has been rejected, as
>>> this would essentially be a restricted version of the upcoming request
>>> API that wouldn't bring any additional value.
>>>
>>> Another option we have considered was to mandate the use of the request
>>> API to modify controls that influence buffer size or layout. This has
>>> also been rejected on the grounds that requiring the request API to
>>> change rotation even when streaming is stopped would significantly
>>> complicate implementation of drivers and usage of the V4L2 API for
>>> applications.
>>>
>>> Applications will however be required to use the upcoming request API to
>>> change at runtime formats or controls that influence the buffer size or
>>> layout, because of the need to synchronize buffers with the formats and
>>> controls. Otherwise there would be no way to interpret the content of a
>>> buffer correctly.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>  Documentation/media/uapi/v4l/buffer.rst | 88 ++++++++++++++++++++++++++++
>>>  1 file changed, 88 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/buffer.rst
>>> b/Documentation/media/uapi/v4l/buffer.rst index
>>> ac58966ccb9b..5c58db98ab7a 100644
>>> --- a/Documentation/media/uapi/v4l/buffer.rst
>>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>>> @@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the
>>> CAPTURE video>
>>>  buffer.
>>>
>>> +Interactions between formats, controls and buffers
>>> +==================================================
>>> +
>>> +V4L2 exposes parameters that influence the buffer size, or the way data
>>> is
>>> +laid out in the buffer. Those parameters are exposed through both formats
>>> and +controls. One example of such a control is the ``V4L2_CID_ROTATE``
>>> control +that modifies the direction in which pixels are stored in the
>>> buffer, as well +as the buffer size when the selected format includes
>>> padding at the end of +lines.
>>> +
>>> +The set of information needed to interpret the content of a buffer (e.g.
>>> the +pixel format, the line stride, the tiling orientation or the
>>> rotation) is +collectively referred to in the rest of this section as the
>>> buffer layout. +
>>> +Modifying formats or controls that influence the buffer size or layout
>>> require +the stream to be stopped. Any attempt at such a modification
>>> while the stream +is active shall cause the format or control set ioctl
>>> to return the ``EBUSY`` +error code.
>>
>> This is not what happens today: it's not the streaming part that causes
>> EBUSY to be returned but whether or not buffers are allocated.
>
> Note that this patch documents the recommended behaviour we want to achieve.
> It can differ from the behaviour implemented today as long as today's
> behaviour (at least the behaviour of the drivers that we don't consider as
> non-compliant, which is the vast majority) is not contradicted by this patch.
> In other words, we can recommend here a more generic versatile and powerful
> behaviour as long as the more restricted behaviour implemented today is also
> allowed.
>
>> Today we do not support changing buffer sizes on the fly, so any attempt to
>> call an ioctl that would change the buffer size is blocked and EBUSY is
>> returned. To be precise: drivers call vb2_is_busy() to determine this.
>>
>> To my knowledge all vb2-using drivers behave like this. There may be old
>> drivers that do not do this (and these have a high likelyhood of being
>> wrong).
>
> Please also note that the above description doesn't contradict the behaviour
> implemented today. It explains that changing formats and controls is not
> allowed while streaming, which is a subset of the restriction implemented by
> many drivers through vb2_is_busy() of not allowing changes when buffers are
> allocated.
>
> Please see below for more about this in response to your "here's the 99% case"
> comment.
>
>>> +
>>> +Controls that only influence the buffer layout can be modified at any
>>> time
>>> +when the stream is stopped. As they don't influence the buffer size, no
>>> +special handling is needed to synchronize those controls with buffer
>>> +allocation.
>>> +
>>> +Formats and controls that influence the buffer size interact with buffer
>>> +allocation. As buffer allocation is an expensive operation, drivers
>>> should
>>> +allow format or controls that influence the buffer size to be changed
>>> with
>>> +buffers allocated. A typical ioctl sequence to modify format and controls
>>> is +
>>> + #. VIDIOC_STREAMOFF
>>> + #. VIDIOC_S_FMT
>>> + #. VIDIOC_S_EXT_CTRLS
>>> + #. VIDIOC_QBUF
>>> + #. VIDIOC_STREAMON
>>> +
>>> +Queued buffers must be large enough for the new format or controls.
>>> +
>>> +Drivers shall return a ``ENOSPC`` error in response to format change
>>> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
>>> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
>>> +currently queued. As a simplification, drivers are allowed to return an
>>> error +from these ioctls if any buffer is currently queued, without
>>> checking the +queued buffers sizes. Drivers shall also return a
>>> ``ENOSPC`` error from the +:c:func:`VIDIOC_QBUF` ioctl if the buffer
>>> being queued is too small for the +current format or controls.
>>
>> Actually, today qbuf will return -EINVAL (from __verify_length in
>> videobuf2-v4l2.c) in these cases.
>
> No, __verify_length() only checks that the bytesused value doesn't exceed the
> buffer length (and only for output buffers, not for capture buffers).
> videobuf2 isn't format-aware, so it doesn't check the buffer length against
> formats and controls.

True, sorry about that.

However, buf_prepare *does* check for that (see uvc_buffer_prepare()) and
returns -EINVAL. So my comment stands.

>
>> I am pretty sure you can't change that to ENOSPC since this has always been
>> -EINVAL, and so changing this would break the ABI.
>
> As videobuf2 doesn't perform any such check today, I'm not changing the return
> value :-)

Sorry, no. Most drivers test this in buf_prepare (and any that do not are
almost certainly buggy) and return -EINVAL.

>
>> Trying to change the format while buffers are allocated will return EBUSY
>> today.
>>
>> If you are trying to document what will happen when drivers allow format
>> changes on the fly then this is not at all clear from what you write here.
>>
>> So:
>>
>> If the driver does not support changing the format while buffers are queued,
>> then it will return EBUSY (true for almost (?) all drivers today). If it
>> does support this, then it will behave as described above, except for the
>> ENOSPC error in QBUF.
>>
>> Note that the meaning of ENOSPC should also be explicitly documented in the
>> ioctls that can return this.
>
> Agreed.
>
>>> Together, these requirements ensure that queued
>>>
>>> +buffers will always be large enough for the configured format and
>>> controls. +
>>> +Userspace applications can query the buffer size required for a given
>>> format +and controls by first setting the desired control values and then
>>> trying the +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will
>>> return the required +buffer size.
>>> +
>>> + #. VIDIOC_S_EXT_CTRLS(x)
>>> + #. VIDIOC_TRY_FMT()
>>> + #. VIDIOC_S_EXT_CTRLS(y)
>>> + #. VIDIOC_TRY_FMT()
>>> +
>>> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate
>>> buffers +based on the queried sizes (for instance by allocating a set of
>>> buffers large +enough for all the desired formats and controls, or by
>>> allocating separate set +of appropriately sized buffers for each use
>>> case).
>>> +
>>> +To simplify their implementation, drivers may also require buffers to be
>>> +reallocated in order to change formats or controls that influence the
>>> buffer +size. In that case, to perform such changes, userspace
>>> applications shall +first stop the video stream with the
>>> :c:func:`VIDIOC_STREAMOFF` ioctl if it +is running and free all buffers
>>> with the :c:func:`VIDIOC_REQBUFS` ioctl if +they are allocated. The
>>> format or controls can then be modified, and buffers +shall then be
>>> reallocated and the stream restarted. A typical ioctl sequence +is
>>> +
>>> + #. VIDIOC_STREAMOFF
>>> + #. VIDIOC_REQBUFS(0)
>>> + #. VIDIOC_S_FMT
>>> + #. VIDIOC_S_EXT_CTRLS
>>> + #. VIDIOC_REQBUFS(n)
>>> + #. VIDIOC_QBUF
>>> + #. VIDIOC_STREAMON
>>> +
>>> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and
>>> control +value into account to compute the buffer size to allocate.
>>> Applications can +also retrieve the size by calling the
>>> :c:func:`VIDIOC_G_FMT` ioctl if needed. +
>>> +When reallocation is required, any attempt to modify format or controls
>>> that +influences the buffer size while buffers are allocated shall cause
>>> the format +or control set ioctl to return the ``EBUSY`` error code.
>>
>> Ah, here you describe the 99% situation. This should come first. You've been
>> working on the 1% that allows changing formats while buffers are allocated
>> so that feels all-important to you, but in reality almost all drivers use
>> the 'simplified' implementation. Describe that first, then go on describing
>> what will happen for your driver. That will make much more sense to me (as
>> you can tell from the preceding comments) and I'm sure to the end-user as
>> well.
>
> This was actually my first, non published version. After discussing it with
> Sakari, we decided to change the description. As Sakari mentioned, the
> CREATE_BUFS ioctl becomes quite pointless if we don't allow changes with
> buffers allocated. We obviously can't mandate it, as most drivers don't allow
> it today, but Sakari and I thought we should recommend it. That's what I tried
> to convey in this patch by describing the recommended option first.
>
> The approach I took here was to say
>
> - formats and controls can't be changed while streaming in any case
> - when buffers are allocated but the stream is off, drivers can allow changes
> - drivers are not required to allow changes while buffers are allocated
>
> What's your opinion about that ?

I disagree. Drivers needs explicit support to be able to change format when
buffers are allocated, but streaming is off: they need to check if the
allocated buffers are big enough for the new format. BTW: we need a vb2 helper
function for that.

No driver does that today. Very few drivers have any need for that, it's only
the more complex SoC drivers that would need this, and even then it is of
limited functionality: it is really the request API that would make this truly
useful.

So you should really describe the common situation, then the complex one.
Anything else is very confusing for the average reader (heck, it confused me!).

The recommended way to organize documentation is to start with the simple, common
case, then work your way up to the more complex situations. That should be done
here as well.

>
>> What should be mentioned here as well is that S_SELECTION can also
>> implicitly change the format, specifically if there is no scaler or if the
>> scaler has limitations.
>>
>> Also there are a few ioctls that can reset selections and formats when
>> called: S_INPUT/S_OUTPUT, S_STD and S_DV_TIMINGS.
>>
>> I think this should be mentioned here. It can be just in passing, no need to
>> go in-depth on that. As long as people are aware of it.
>
> Agreed, I'll mention all of these.
>
>> The documentation of S_STD and S_DV_TIMINGS should also be updated saying
>> that if the new std or timings differ from the existing std or timings,
>> then the format will also change and selection rectangles will be reset to
>> the defaults. Setting the std/dv_timings with the current std or timings
>> will not do anything: there are applications that do this even when
>> streaming so this should be allowed.
>>
>> Unfortunately, this is not documented but it really should.
>>
>> If you don't have time to update the S_STD/DV_TIMINGS ioctls, then let me
>> know and I will do that.
>
> I likely won't have time to do that before leaving for holidays so I'd
> appreciate if you could update the documentation of those ioctls. I would also
> appreciate if we could get this patch merged (or rather a more recent version
> thereof) without being blocked by the S_STD/DV_TIMINGS documentation update
> :-)

No problem.

>
>>> +
>>> +
>>>
>>>  .. c:type:: v4l2_buffer
>>>
>>>  struct v4l2_buffer
>
> I'll post a v3 now that addresses all your comments except the one about
> describing the most restrictive option first. I'm not completely against
>

Hmm, missing text here?

Regards,

	Hans

[PATCH v2.1] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

V4L2 exposes parameters that influence buffers sizes through the format
ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
the format structure may also influence buffer sizes or buffer layout in
general. One existing such parameter is rotation, which is implemented
by the VIDIOC_ROTATE control and thus exposed through the V4L2 control
ioctls.

The interaction between those parameters and buffers is currently only
partially specified by the V4L2 API. In particular interactions between
controls and buffers isn't specified at all. The behaviour of the
VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
also not fully specified.

This commit clearly defines and documents the interactions between
formats, selections, controls and buffers.

The preparatory discussions for the documentation change considered
completely disallowing controls that change the buffer size or layout,
in favour of extending the format API with a new ioctl that would bundle
those controls with format information. The idea has been rejected, as
this would essentially be a restricted version of the upcoming request
API that wouldn't bring any additional value.

Another option we have considered was to mandate the use of the request
API to modify controls that influence buffer size or layout. This has
also been rejected on the grounds that requiring the request API to
change rotation even when streaming is stopped would significantly
complicate implementation of drivers and usage of the V4L2 API for
applications.

Applications will however be required to use the upcoming request API to
change at runtime formats or controls that influence the buffer size or
layout, because of the need to synchronize buffers with the formats and
controls. Otherwise there would be no way to interpret the content of a
buffer correctly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Document the interaction with ioctls that can affect formats
  (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
  VIDIOC_S_DV_TIMINGS)
- Clarify the format/control change order
---
 Documentation/media/uapi/v4l/buffer.rst | 108 ++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ac58966ccb9b..ce46908bedfb 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
 buffer.
 
 
+Interactions between formats, controls and buffers
+==================================================
+
+V4L2 exposes parameters that influence the buffer size, or the way data is
+laid out in the buffer. Those parameters are exposed through both formats and
+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
+that modifies the direction in which pixels are stored in the buffer, as well
+as the buffer size when the selected format includes padding at the end of
+lines.
+
+The set of information needed to interpret the content of a buffer (e.g. the
+pixel format, the line stride, the tiling orientation or the rotation) is
+collectively referred to in the rest of this section as the buffer layout.
+
+Modifying formats or controls that influence the buffer size or layout require
+the stream to be stopped. Any attempt at such a modification while the stream
+is active shall cause the format or control set ioctl to return the ``EBUSY``
+error code.
+
+Controls that only influence the buffer layout can be modified at any time
+when the stream is stopped. As they don't influence the buffer size, no
+special handling is needed to synchronize those controls with buffer
+allocation.
+
+Formats and controls that influence the buffer size interact with buffer
+allocation. As buffer allocation is an expensive operation, drivers should
+allow format or controls that influence the buffer size to be changed with
+buffers allocated. A typical ioctl sequence to modify format and controls is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_S_FMT
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+.. note::
+
+   The API doesn't mandate the above order for control (2.) and format (3.)
+   changes. Format and controls can be set in a different order, or even
+   interleaved, depending on the device and use case. For instance some
+   controls might behave differently for different pixel formats, in which
+   case the format might need to be set first.
+
+Queued buffers must be large enough for the new format or controls.
+
+Drivers shall return a ``ENOSPC`` error in response to format change
+(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
+:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
+currently queued. As a simplification, drivers are allowed to return an error
+from these ioctls if any buffer is currently queued, without checking the
+queued buffers sizes.
+
+.. note::
+
+   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
+   instance if the device doesn't include a scaler), modify the format in
+   addition to the selection rectangle. Similarly, the
+   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
+   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
+   selection rectangles. Driver shall return the same ``ENOSPC`` error from
+   all ioctls that would result in formats too large for queued buffers.
+
+Drivers shall also return a ``ENOSPC`` error from the :c:func:`VIDIOC_QBUF`
+ioctl if the buffer being queued is too small for the current format or
+controls. Together, these requirements ensure that queued buffers will always
+be large enough for the configured format and controls.
+
+Userspace applications can query the buffer size required for a given format
+and controls by first setting the desired control values and then trying the
+desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
+buffer size.
+
+ #. VIDIOC_S_EXT_CTRLS(x)
+ #. VIDIOC_TRY_FMT()
+ #. VIDIOC_S_EXT_CTRLS(y)
+ #. VIDIOC_TRY_FMT()
+
+The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
+based on the queried sizes (for instance by allocating a set of buffers large
+enough for all the desired formats and controls, or by allocating separate set
+of appropriately sized buffers for each use case).
+
+To simplify their implementation, drivers may also require buffers to be
+reallocated in order to change formats or controls that influence the buffer
+size. In that case, to perform such changes, userspace applications shall
+first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
+is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
+they are allocated. The format or controls can then be modified, and buffers
+shall then be reallocated and the stream restarted. A typical ioctl sequence
+is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_REQBUFS(0)
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_S_FMT
+ #. VIDIOC_REQBUFS(n)
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
+value into account to compute the buffer size to allocate. Applications can
+also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
+
+When reallocation is required, any attempt to modify format or controls that
+influences the buffer size while buffers are allocated shall cause the format
+or control set ioctl to return the ``EBUSY`` error code.
+
+
 .. c:type:: v4l2_buffer
 
 struct v4l2_buffer
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2.1] v4l: Clearly document interactions between formats, controls and buffers

[Sakari Ailus]

Hi Laurent,

Thanks for the update.

On Sun, Mar 05, 2017 at 04:39:36PM +0200, Laurent Pinchart wrote:
> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> the format structure may also influence buffer sizes or buffer layout in
> general. One existing such parameter is rotation, which is implemented
> by the VIDIOC_ROTATE control and thus exposed through the V4L2 control

V4L2_CID_ROTATE

> ioctls.
> 
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> also not fully specified.
> 
> This commit clearly defines and documents the interactions between

It's say it's a patch, and most kernel developers seem to agree:

$ git log v4.9..v4.10|grep -ci
'this patch'
1816
$ git log v4.9..v4.10|grep -ci
'this commit'
217

Up to you. :-)

> formats, selections, controls and buffers.
> 
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
> 
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
> 
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Document the interaction with ioctls that can affect formats
>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>   VIDIOC_S_DV_TIMINGS)
> - Clarify the format/control change order
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 108 ++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..ce46908bedfb 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>  buffer.
>  
>  
> +Interactions between formats, controls and buffers
> +==================================================
> +
> +V4L2 exposes parameters that influence the buffer size, or the way data is
> +laid out in the buffer. Those parameters are exposed through both formats and
> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> +that modifies the direction in which pixels are stored in the buffer, as well
> +as the buffer size when the selected format includes padding at the end of
> +lines.
> +
> +The set of information needed to interpret the content of a buffer (e.g. the
> +pixel format, the line stride, the tiling orientation or the rotation) is
> +collectively referred to in the rest of this section as the buffer layout.
> +
> +Modifying formats or controls that influence the buffer size or layout require
> +the stream to be stopped. Any attempt at such a modification while the stream
> +is active shall cause the format or control set ioctl to return the ``EBUSY``

How about:

s/the format or control set ioctl/the ioctl setting the format or the
control/

> +error code.
> +
> +Controls that only influence the buffer layout can be modified at any time
> +when the stream is stopped. As they don't influence the buffer size, no
> +special handling is needed to synchronize those controls with buffer
> +allocation.
> +
> +Formats and controls that influence the buffer size interact with buffer
> +allocation. As buffer allocation is an expensive operation, drivers should
> +allow format or controls that influence the buffer size to be changed with
> +buffers allocated. A typical ioctl sequence to modify format and controls is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +.. note::
> +
> +   The API doesn't mandate the above order for control (2.) and format (3.)
> +   changes. Format and controls can be set in a different order, or even
> +   interleaved, depending on the device and use case. For instance some
> +   controls might behave differently for different pixel formats, in which
> +   case the format might need to be set first.
> +
> +Queued buffers must be large enough for the new format or controls.
> +
> +Drivers shall return a ``ENOSPC`` error in response to format change
> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> +currently queued. As a simplification, drivers are allowed to return an error
> +from these ioctls if any buffer is currently queued, without checking the
> +queued buffers sizes.
> +
> +.. note::
> +
> +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
> +   instance if the device doesn't include a scaler), modify the format in
> +   addition to the selection rectangle. Similarly, the
> +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
> +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
> +   selection rectangles. Driver shall return the same ``ENOSPC`` error from
> +   all ioctls that would result in formats too large for queued buffers.
> +
> +Drivers shall also return a ``ENOSPC`` error from the :c:func:`VIDIOC_QBUF`
> +ioctl if the buffer being queued is too small for the current format or
> +controls. Together, these requirements ensure that queued buffers will always
> +be large enough for the configured format and controls.
> +
> +Userspace applications can query the buffer size required for a given format
> +and controls by first setting the desired control values and then trying the
> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> +buffer size.
> +
> + #. VIDIOC_S_EXT_CTRLS(x)
> + #. VIDIOC_TRY_FMT()
> + #. VIDIOC_S_EXT_CTRLS(y)
> + #. VIDIOC_TRY_FMT()
> +
> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> +based on the queried sizes (for instance by allocating a set of buffers large
> +enough for all the desired formats and controls, or by allocating separate set
> +of appropriately sized buffers for each use case).
> +
> +To simplify their implementation, drivers may also require buffers to be
> +reallocated in order to change formats or controls that influence the buffer
> +size. In that case, to perform such changes, userspace applications shall
> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> +they are allocated. The format or controls can then be modified, and buffers
> +shall then be reallocated and the stream restarted. A typical ioctl sequence
> +is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_REQBUFS(0)
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_REQBUFS(n)
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> +value into account to compute the buffer size to allocate. Applications can
> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error code.
> +
> +
>  .. c:type:: v4l2_buffer
>  
>  struct v4l2_buffer

With the above fixed / considered,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2.1] v4l: Clearly document interactions between formats, controls and buffers

[Sakari Ailus]

Hi Laurent,

Thanks for the update.

On Sun, Mar 05, 2017 at 04:39:36PM +0200, Laurent Pinchart wrote:
> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> the format structure may also influence buffer sizes or buffer layout in
> general. One existing such parameter is rotation, which is implemented
> by the VIDIOC_ROTATE control and thus exposed through the V4L2 control

V4L2_CID_ROTATE

> ioctls.
> 
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> also not fully specified.
> 
> This commit clearly defines and documents the interactions between

It's say it's a patch, and most kernel developers seem to agree:

$�git log v4.9..v4.10|grep -ci
'this patch'
1816
$ git log v4.9..v4.10|grep -ci
'this commit'
217

Up to you. :-)

> formats, selections, controls and buffers.
> 
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
> 
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
> 
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Document the interaction with ioctls that can affect formats
>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>   VIDIOC_S_DV_TIMINGS)
> - Clarify the format/control change order
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 108 ++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..ce46908bedfb 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>  buffer.
>  
>  
> +Interactions between formats, controls and buffers
> +==================================================
> +
> +V4L2 exposes parameters that influence the buffer size, or the way data is
> +laid out in the buffer. Those parameters are exposed through both formats and
> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> +that modifies the direction in which pixels are stored in the buffer, as well
> +as the buffer size when the selected format includes padding at the end of
> +lines.
> +
> +The set of information needed to interpret the content of a buffer (e.g. the
> +pixel format, the line stride, the tiling orientation or the rotation) is
> +collectively referred to in the rest of this section as the buffer layout.
> +
> +Modifying formats or controls that influence the buffer size or layout require
> +the stream to be stopped. Any attempt at such a modification while the stream
> +is active shall cause the format or control set ioctl to return the ``EBUSY``

How about:

s/the format or control set ioctl/the ioctl setting the format or the
control/

> +error code.
> +
> +Controls that only influence the buffer layout can be modified at any time
> +when the stream is stopped. As they don't influence the buffer size, no
> +special handling is needed to synchronize those controls with buffer
> +allocation.
> +
> +Formats and controls that influence the buffer size interact with buffer
> +allocation. As buffer allocation is an expensive operation, drivers should
> +allow format or controls that influence the buffer size to be changed with
> +buffers allocated. A typical ioctl sequence to modify format and controls is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +.. note::
> +
> +   The API doesn't mandate the above order for control (2.) and format (3.)
> +   changes. Format and controls can be set in a different order, or even
> +   interleaved, depending on the device and use case. For instance some
> +   controls might behave differently for different pixel formats, in which
> +   case the format might need to be set first.
> +
> +Queued buffers must be large enough for the new format or controls.
> +
> +Drivers shall return a ``ENOSPC`` error in response to format change
> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> +currently queued. As a simplification, drivers are allowed to return an error
> +from these ioctls if any buffer is currently queued, without checking the
> +queued buffers sizes.
> +
> +.. note::
> +
> +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
> +   instance if the device doesn't include a scaler), modify the format in
> +   addition to the selection rectangle. Similarly, the
> +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
> +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
> +   selection rectangles. Driver shall return the same ``ENOSPC`` error from
> +   all ioctls that would result in formats too large for queued buffers.
> +
> +Drivers shall also return a ``ENOSPC`` error from the :c:func:`VIDIOC_QBUF`
> +ioctl if the buffer being queued is too small for the current format or
> +controls. Together, these requirements ensure that queued buffers will always
> +be large enough for the configured format and controls.
> +
> +Userspace applications can query the buffer size required for a given format
> +and controls by first setting the desired control values and then trying the
> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> +buffer size.
> +
> + #. VIDIOC_S_EXT_CTRLS(x)
> + #. VIDIOC_TRY_FMT()
> + #. VIDIOC_S_EXT_CTRLS(y)
> + #. VIDIOC_TRY_FMT()
> +
> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> +based on the queried sizes (for instance by allocating a set of buffers large
> +enough for all the desired formats and controls, or by allocating separate set
> +of appropriately sized buffers for each use case).
> +
> +To simplify their implementation, drivers may also require buffers to be
> +reallocated in order to change formats or controls that influence the buffer
> +size. In that case, to perform such changes, userspace applications shall
> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> +they are allocated. The format or controls can then be modified, and buffers
> +shall then be reallocated and the stream restarted. A typical ioctl sequence
> +is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_REQBUFS(0)
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_REQBUFS(n)
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> +value into account to compute the buffer size to allocate. Applications can
> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error code.
> +
> +
>  .. c:type:: v4l2_buffer
>  
>  struct v4l2_buffer

With the above fixed / considered,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v2.2] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

V4L2 exposes parameters that influence buffers sizes through the format
ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
the format structure may also influence buffer sizes or buffer layout in
general. One existing such parameter is rotation, which is implemented
by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
ioctls.

The interaction between those parameters and buffers is currently only
partially specified by the V4L2 API. In particular interactions between
controls and buffers isn't specified at all. The behaviour of the
VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
also not fully specified.

This patch clearly defines and documents the interactions between
formats, selections, controls and buffers.

The preparatory discussions for the documentation change considered
completely disallowing controls that change the buffer size or layout,
in favour of extending the format API with a new ioctl that would bundle
those controls with format information. The idea has been rejected, as
this would essentially be a restricted version of the upcoming request
API that wouldn't bring any additional value.

Another option we have considered was to mandate the use of the request
API to modify controls that influence buffer size or layout. This has
also been rejected on the grounds that requiring the request API to
change rotation even when streaming is stopped would significantly
complicate implementation of drivers and usage of the V4L2 API for
applications.

Applications will however be required to use the upcoming request API to
change at runtime formats or controls that influence the buffer size or
layout, because of the need to synchronize buffers with the formats and
controls. Otherwise there would be no way to interpret the content of a
buffer correctly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Changes since v2.1:

- Fixed small issues in commit message
- Simplified wording of one sentence in the documentation

Changes since v2:

- Document the interaction with ioctls that can affect formats
  (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
  VIDIOC_S_DV_TIMINGS)
- Clarify the format/control change order
---
 Documentation/media/uapi/v4l/buffer.rst | 108 ++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ac58966ccb9b..60d62a5824f8 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
 buffer.
 
 
+Interactions between formats, controls and buffers
+==================================================
+
+V4L2 exposes parameters that influence the buffer size, or the way data is
+laid out in the buffer. Those parameters are exposed through both formats and
+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
+that modifies the direction in which pixels are stored in the buffer, as well
+as the buffer size when the selected format includes padding at the end of
+lines.
+
+The set of information needed to interpret the content of a buffer (e.g. the
+pixel format, the line stride, the tiling orientation or the rotation) is
+collectively referred to in the rest of this section as the buffer layout.
+
+Modifying formats or controls that influence the buffer size or layout require
+the stream to be stopped. Any attempt at such a modification while the stream
+is active shall cause the ioctl setting the format or the control to return
+the ``EBUSY`` error code.
+
+Controls that only influence the buffer layout can be modified at any time
+when the stream is stopped. As they don't influence the buffer size, no
+special handling is needed to synchronize those controls with buffer
+allocation.
+
+Formats and controls that influence the buffer size interact with buffer
+allocation. As buffer allocation is an expensive operation, drivers should
+allow format or controls that influence the buffer size to be changed with
+buffers allocated. A typical ioctl sequence to modify format and controls is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_S_FMT
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+.. note::
+
+   The API doesn't mandate the above order for control (2.) and format (3.)
+   changes. Format and controls can be set in a different order, or even
+   interleaved, depending on the device and use case. For instance some
+   controls might behave differently for different pixel formats, in which
+   case the format might need to be set first.
+
+Queued buffers must be large enough for the new format or controls.
+
+Drivers shall return a ``ENOSPC`` error in response to format change
+(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
+:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
+currently queued. As a simplification, drivers are allowed to return an error
+from these ioctls if any buffer is currently queued, without checking the
+queued buffers sizes.
+
+.. note::
+
+   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
+   instance if the device doesn't include a scaler), modify the format in
+   addition to the selection rectangle. Similarly, the
+   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
+   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
+   selection rectangles. Driver shall return the same ``ENOSPC`` error from
+   all ioctls that would result in formats too large for queued buffers.
+
+Drivers shall also return a ``ENOSPC`` error from the :c:func:`VIDIOC_QBUF`
+ioctl if the buffer being queued is too small for the current format or
+controls. Together, these requirements ensure that queued buffers will always
+be large enough for the configured format and controls.
+
+Userspace applications can query the buffer size required for a given format
+and controls by first setting the desired control values and then trying the
+desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
+buffer size.
+
+ #. VIDIOC_S_EXT_CTRLS(x)
+ #. VIDIOC_TRY_FMT()
+ #. VIDIOC_S_EXT_CTRLS(y)
+ #. VIDIOC_TRY_FMT()
+
+The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
+based on the queried sizes (for instance by allocating a set of buffers large
+enough for all the desired formats and controls, or by allocating separate set
+of appropriately sized buffers for each use case).
+
+To simplify their implementation, drivers may also require buffers to be
+reallocated in order to change formats or controls that influence the buffer
+size. In that case, to perform such changes, userspace applications shall
+first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
+is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
+they are allocated. The format or controls can then be modified, and buffers
+shall then be reallocated and the stream restarted. A typical ioctl sequence
+is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_REQBUFS(0)
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_S_FMT
+ #. VIDIOC_REQBUFS(n)
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
+value into account to compute the buffer size to allocate. Applications can
+also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
+
+When reallocation is required, any attempt to modify format or controls that
+influences the buffer size while buffers are allocated shall cause the format
+or control set ioctl to return the ``EBUSY`` error code.
+
+
 .. c:type:: v4l2_buffer
 
 struct v4l2_buffer
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2.2] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

On 05/03/17 22:36, Laurent Pinchart wrote:
> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> the format structure may also influence buffer sizes or buffer layout in
> general. One existing such parameter is rotation, which is implemented
> by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
> ioctls.
>
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> also not fully specified.
>
> This patch clearly defines and documents the interactions between
> formats, selections, controls and buffers.
>
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
>
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
>
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Changes since v2.1:
>
> - Fixed small issues in commit message
> - Simplified wording of one sentence in the documentation
>
> Changes since v2:
>
> - Document the interaction with ioctls that can affect formats
>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>   VIDIOC_S_DV_TIMINGS)
> - Clarify the format/control change order
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 108 ++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..60d62a5824f8 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>  buffer.
>
>
> +Interactions between formats, controls and buffers
> +==================================================
> +
> +V4L2 exposes parameters that influence the buffer size, or the way data is
> +laid out in the buffer. Those parameters are exposed through both formats and
> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> +that modifies the direction in which pixels are stored in the buffer, as well
> +as the buffer size when the selected format includes padding at the end of
> +lines.
> +
> +The set of information needed to interpret the content of a buffer (e.g. the
> +pixel format, the line stride, the tiling orientation or the rotation) is
> +collectively referred to in the rest of this section as the buffer layout.
> +
> +Modifying formats or controls that influence the buffer size or layout require
> +the stream to be stopped. Any attempt at such a modification while the stream
> +is active shall cause the ioctl setting the format or the control to return
> +the ``EBUSY`` error code.

This is my problem with putting the more complex case first: if you are reading
this for the first time then the preceding paragraph is simply *wrong*.

You cannot modify the buffer size when the stream is stopped. You need to
free all buffers first before you can do that.

Unless the driver has been especially written to allow that. And I am not
aware of any.

> +
> +Controls that only influence the buffer layout can be modified at any time
> +when the stream is stopped. As they don't influence the buffer size, no
> +special handling is needed to synchronize those controls with buffer
> +allocation.
> +
> +Formats and controls that influence the buffer size interact with buffer
> +allocation. As buffer allocation is an expensive operation, drivers should
> +allow format or controls that influence the buffer size to be changed with
> +buffers allocated. A typical ioctl sequence to modify format and controls is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +.. note::
> +
> +   The API doesn't mandate the above order for control (2.) and format (3.)
> +   changes. Format and controls can be set in a different order, or even
> +   interleaved, depending on the device and use case. For instance some
> +   controls might behave differently for different pixel formats, in which
> +   case the format might need to be set first.
> +
> +Queued buffers must be large enough for the new format or controls.
> +
> +Drivers shall return a ``ENOSPC`` error in response to format change
> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> +currently queued. As a simplification, drivers are allowed to return an error

s/an error/``EBUSY``/

> +from these ioctls if any buffer is currently queued, without checking the
> +queued buffers sizes.

Again, swap the order: simple case first, more complex case next.

> +
> +.. note::
> +
> +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
> +   instance if the device doesn't include a scaler), modify the format in
> +   addition to the selection rectangle. Similarly, the
> +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
> +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
> +   selection rectangles. Driver shall return the same ``ENOSPC`` error from
> +   all ioctls that would result in formats too large for queued buffers.

This will return EBUSY for the 'simple' drivers, and ENOSPC for the complex ones.
Should be mentioned clearly to avoid confusion.

> +
> +Drivers shall also return a ``ENOSPC`` error from the :c:func:`VIDIOC_QBUF`
> +ioctl if the buffer being queued is too small for the current format or
> +controls. Together, these requirements ensure that queued buffers will always
> +be large enough for the configured format and controls.

NACK: it's EINVAL as returned by the buf_prepare() callbacks. Yes, I agree that
ENOSPC would have been more appropriate, but I do not believe we can change this
without breaking ABI. I also do not think this is all that important. Feel free
to blame this on lack for foresight :-)

> +
> +Userspace applications can query the buffer size required for a given format
> +and controls by first setting the desired control values and then trying the
> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> +buffer size.
> +
> + #. VIDIOC_S_EXT_CTRLS(x)
> + #. VIDIOC_TRY_FMT()
> + #. VIDIOC_S_EXT_CTRLS(y)
> + #. VIDIOC_TRY_FMT()
> +
> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> +based on the queried sizes (for instance by allocating a set of buffers large
> +enough for all the desired formats and controls, or by allocating separate set
> +of appropriately sized buffers for each use case).
> +
> +To simplify their implementation, drivers may also require buffers to be
> +reallocated in order to change formats or controls that influence the buffer
> +size. In that case, to perform such changes, userspace applications shall
> +first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> +is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> +they are allocated. The format or controls can then be modified, and buffers
> +shall then be reallocated and the stream restarted. A typical ioctl sequence
> +is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_REQBUFS(0)
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_REQBUFS(n)
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> +value into account to compute the buffer size to allocate. Applications can
> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error code.
> +
> +
>  .. c:type:: v4l2_buffer
>
>  struct v4l2_buffer
>

The order really has to be changed: first explain the 99%, then continue with
the more complex case. It's not the text itself, it's the order in which this
is presented. I won't accept it in this order as it will be terminally confusing
for most readers. Sorry.

Regards,

	Hans

Re: [PATCH v2.2] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

Hi Hans,

On Monday 06 Mar 2017 11:04:50 Hans Verkuil wrote:
> On 05/03/17 22:36, Laurent Pinchart wrote:
> > V4L2 exposes parameters that influence buffers sizes through the format
> > ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> > VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> > the format structure may also influence buffer sizes or buffer layout in
> > general. One existing such parameter is rotation, which is implemented
> > by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
> > ioctls.
> > 
> > The interaction between those parameters and buffers is currently only
> > partially specified by the V4L2 API. In particular interactions between
> > controls and buffers isn't specified at all. The behaviour of the
> > VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> > also not fully specified.
> > 
> > This patch clearly defines and documents the interactions between
> > formats, selections, controls and buffers.
> > 
> > The preparatory discussions for the documentation change considered
> > completely disallowing controls that change the buffer size or layout,
> > in favour of extending the format API with a new ioctl that would bundle
> > those controls with format information. The idea has been rejected, as
> > this would essentially be a restricted version of the upcoming request
> > API that wouldn't bring any additional value.
> > 
> > Another option we have considered was to mandate the use of the request
> > API to modify controls that influence buffer size or layout. This has
> > also been rejected on the grounds that requiring the request API to
> > change rotation even when streaming is stopped would significantly
> > complicate implementation of drivers and usage of the V4L2 API for
> > applications.
> > 
> > Applications will however be required to use the upcoming request API to
> > change at runtime formats or controls that influence the buffer size or
> > layout, because of the need to synchronize buffers with the formats and
> > controls. Otherwise there would be no way to interpret the content of a
> > buffer correctly.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Changes since v2.1:
> > 
> > - Fixed small issues in commit message
> > - Simplified wording of one sentence in the documentation
> > 
> > Changes since v2:
> > 
> > - Document the interaction with ioctls that can affect formats
> >   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
> >   VIDIOC_S_DV_TIMINGS)
> > - Clarify the format/control change order
> > ---
> > 
> >  Documentation/media/uapi/v4l/buffer.rst | 108 +++++++++++++++++++++++++++
> >  1 file changed, 108 insertions(+)
> > 
> > diff --git a/Documentation/media/uapi/v4l/buffer.rst
> > b/Documentation/media/uapi/v4l/buffer.rst index
> > ac58966ccb9b..60d62a5824f8 100644
> > --- a/Documentation/media/uapi/v4l/buffer.rst
> > +++ b/Documentation/media/uapi/v4l/buffer.rst
> > @@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the
> > CAPTURE video> 
> >  buffer.
> > 
> > +Interactions between formats, controls and buffers
> > +==================================================
> > +
> > +V4L2 exposes parameters that influence the buffer size, or the way data
> > is +laid out in the buffer. Those parameters are exposed through both
> > formats and +controls. One example of such a control is the
> > ``V4L2_CID_ROTATE`` control +that modifies the direction in which pixels
> > are stored in the buffer, as well +as the buffer size when the selected
> > format includes padding at the end of +lines.
> > +
> > +The set of information needed to interpret the content of a buffer (e.g.
> > the +pixel format, the line stride, the tiling orientation or the
> > rotation) is +collectively referred to in the rest of this section as the
> > buffer layout.
> > +
> > +Modifying formats or controls that influence the buffer size or layout
> > require +the stream to be stopped. Any attempt at such a modification
> > while the stream +is active shall cause the ioctl setting the format or
> > the control to return +the ``EBUSY`` error code.
> 
> This is my problem with putting the more complex case first: if you are
> reading this for the first time then the preceding paragraph is simply
> *wrong*.
> 
> You cannot modify the buffer size when the stream is stopped. You need to
> free all buffers first before you can do that.

That's driver-dependent. If I reverse the order to start with the more 
restricted case, it will also be wrong for drivers that implement the other 
case. I'm running out of time so I'll change this though. We can always 
reorder the paragraphs later when we will have more drivers implementing the 
more powerful case.

> Unless the driver has been especially written to allow that. And I am not
> aware of any.
> 
> > +
> > +Controls that only influence the buffer layout can be modified at any
> > time
> > +when the stream is stopped. As they don't influence the buffer size, no
> > +special handling is needed to synchronize those controls with buffer
> > +allocation.
> > +
> > +Formats and controls that influence the buffer size interact with buffer
> > +allocation. As buffer allocation is an expensive operation, drivers
> > should
> > +allow format or controls that influence the buffer size to be changed
> > with
> > +buffers allocated. A typical ioctl sequence to modify format and controls
> > is +
> > + #. VIDIOC_STREAMOFF
> > + #. VIDIOC_S_EXT_CTRLS
> > + #. VIDIOC_S_FMT
> > + #. VIDIOC_QBUF
> > + #. VIDIOC_STREAMON
> > +
> > +.. note::
> > +
> > +   The API doesn't mandate the above order for control (2.) and format
> > (3.) +   changes. Format and controls can be set in a different order, or
> > even +   interleaved, depending on the device and use case. For instance
> > some +   controls might behave differently for different pixel formats,
> > in which +   case the format might need to be set first.
> > +
> > +Queued buffers must be large enough for the new format or controls.
> > +
> > +Drivers shall return a ``ENOSPC`` error in response to format change
> > +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> > +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
> > +currently queued. As a simplification, drivers are allowed to return an
> > error
> s/an error/``EBUSY``/
> 
> > +from these ioctls if any buffer is currently queued, without checking the
> > +queued buffers sizes.
> 
> Again, swap the order: simple case first, more complex case next.
> 
> > +
> > +.. note::
> > +
> > +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware
> > (for +   instance if the device doesn't include a scaler), modify the
> > format in +   addition to the selection rectangle. Similarly, the
> > +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`,
> > :c:func:`VIDIOC_S_STD` +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can
> > also modify the format and +   selection rectangles. Driver shall return
> > the same ``ENOSPC`` error from +   all ioctls that would result in
> > formats too large for queued buffers.
> This will return EBUSY for the 'simple' drivers, and ENOSPC for the complex
> ones. Should be mentioned clearly to avoid confusion.
> 
> > +
> > +Drivers shall also return a ``ENOSPC`` error from the
> > :c:func:`VIDIOC_QBUF` +ioctl if the buffer being queued is too small for
> > the current format or +controls. Together, these requirements ensure that
> > queued buffers will always +be large enough for the configured format and
> > controls.
> 
> NACK: it's EINVAL as returned by the buf_prepare() callbacks. Yes, I agree
> that ENOSPC would have been more appropriate, but I do not believe we can
> change this without breaking ABI. I also do not think this is all that
> important. Feel free to blame this on lack for foresight :-)

That's driver-dependent though, but I agree that due to cargo-cult programming 
most probably return -EINVAL today. Most, because I'm pretty sure we have 
drivers that don't check that condition, resulting in potential DMA buffer 
overflows. It might make sense to move the check to videobuf2-v4l2.c (but 
that's unrelated to this patch).

We should have used -ENOSPC, and I believe that switching to -ENOSPC now 
wouldn't result in any breakage. Can you think of any application that would 
queue a too small buffer, match the error code against -EINVAL, and have a 
strategy to recover ? Queuing a too small buffer is an application but in the 
first place, I don't think application developers will have gone through the 
trouble of implementing a way to recover from their own bug instead of fixing 
them :-)

Would it be OK with you to document that -ENOSPC is the recommended error 
code, but that some drivers currently return -EINVAL ?

> > +Userspace applications can query the buffer size required for a given
> > format +and controls by first setting the desired control values and then
> > trying the +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will
> > return the required +buffer size.
> > +
> > + #. VIDIOC_S_EXT_CTRLS(x)
> > + #. VIDIOC_TRY_FMT()
> > + #. VIDIOC_S_EXT_CTRLS(y)
> > + #. VIDIOC_TRY_FMT()
> > +
> > +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate
> > buffers +based on the queried sizes (for instance by allocating a set of
> > buffers large +enough for all the desired formats and controls, or by
> > allocating separate set +of appropriately sized buffers for each use
> > case).
> > +
> > +To simplify their implementation, drivers may also require buffers to be
> > +reallocated in order to change formats or controls that influence the
> > buffer +size. In that case, to perform such changes, userspace
> > applications shall +first stop the video stream with the
> > :c:func:`VIDIOC_STREAMOFF` ioctl if it +is running and free all buffers
> > with the :c:func:`VIDIOC_REQBUFS` ioctl if +they are allocated. The
> > format or controls can then be modified, and buffers +shall then be
> > reallocated and the stream restarted. A typical ioctl sequence +is
> > +
> > + #. VIDIOC_STREAMOFF
> > + #. VIDIOC_REQBUFS(0)
> > + #. VIDIOC_S_EXT_CTRLS
> > + #. VIDIOC_S_FMT
> > + #. VIDIOC_REQBUFS(n)
> > + #. VIDIOC_QBUF
> > + #. VIDIOC_STREAMON
> > +
> > +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and
> > control +value into account to compute the buffer size to allocate.
> > Applications can +also retrieve the size by calling the
> > :c:func:`VIDIOC_G_FMT` ioctl if needed. +
> > +When reallocation is required, any attempt to modify format or controls
> > that +influences the buffer size while buffers are allocated shall cause
> > the format +or control set ioctl to return the ``EBUSY`` error code.
> > +
> > +
> > 
> >  .. c:type:: v4l2_buffer
> >  
> >  struct v4l2_buffer
> 
> The order really has to be changed: first explain the 99%, then continue
> with the more complex case. It's not the text itself, it's the order in
> which this is presented. I won't accept it in this order as it will be
> terminally confusing for most readers. Sorry.


-- 
Regards,

Laurent Pinchart

Re: [PATCH v2.2] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

On 06/03/17 11:35, Laurent Pinchart wrote:
> Hi Hans,
>
> On Monday 06 Mar 2017 11:04:50 Hans Verkuil wrote:
>> On 05/03/17 22:36, Laurent Pinchart wrote:
>>> V4L2 exposes parameters that influence buffers sizes through the format
>>> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
>>> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
>>> the format structure may also influence buffer sizes or buffer layout in
>>> general. One existing such parameter is rotation, which is implemented
>>> by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
>>> ioctls.
>>>
>>> The interaction between those parameters and buffers is currently only
>>> partially specified by the V4L2 API. In particular interactions between
>>> controls and buffers isn't specified at all. The behaviour of the
>>> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
>>> also not fully specified.
>>>
>>> This patch clearly defines and documents the interactions between
>>> formats, selections, controls and buffers.
>>>
>>> The preparatory discussions for the documentation change considered
>>> completely disallowing controls that change the buffer size or layout,
>>> in favour of extending the format API with a new ioctl that would bundle
>>> those controls with format information. The idea has been rejected, as
>>> this would essentially be a restricted version of the upcoming request
>>> API that wouldn't bring any additional value.
>>>
>>> Another option we have considered was to mandate the use of the request
>>> API to modify controls that influence buffer size or layout. This has
>>> also been rejected on the grounds that requiring the request API to
>>> change rotation even when streaming is stopped would significantly
>>> complicate implementation of drivers and usage of the V4L2 API for
>>> applications.
>>>
>>> Applications will however be required to use the upcoming request API to
>>> change at runtime formats or controls that influence the buffer size or
>>> layout, because of the need to synchronize buffers with the formats and
>>> controls. Otherwise there would be no way to interpret the content of a
>>> buffer correctly.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>> Changes since v2.1:
>>>
>>> - Fixed small issues in commit message
>>> - Simplified wording of one sentence in the documentation
>>>
>>> Changes since v2:
>>>
>>> - Document the interaction with ioctls that can affect formats
>>>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>>>   VIDIOC_S_DV_TIMINGS)
>>> - Clarify the format/control change order
>>> ---
>>>
>>>  Documentation/media/uapi/v4l/buffer.rst | 108 +++++++++++++++++++++++++++
>>>  1 file changed, 108 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/buffer.rst
>>> b/Documentation/media/uapi/v4l/buffer.rst index
>>> ac58966ccb9b..60d62a5824f8 100644
>>> --- a/Documentation/media/uapi/v4l/buffer.rst
>>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>>> @@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the
>>> CAPTURE video>
>>>  buffer.
>>>
>>> +Interactions between formats, controls and buffers
>>> +==================================================
>>> +
>>> +V4L2 exposes parameters that influence the buffer size, or the way data
>>> is +laid out in the buffer. Those parameters are exposed through both
>>> formats and +controls. One example of such a control is the
>>> ``V4L2_CID_ROTATE`` control +that modifies the direction in which pixels
>>> are stored in the buffer, as well +as the buffer size when the selected
>>> format includes padding at the end of +lines.
>>> +
>>> +The set of information needed to interpret the content of a buffer (e.g.
>>> the +pixel format, the line stride, the tiling orientation or the
>>> rotation) is +collectively referred to in the rest of this section as the
>>> buffer layout.
>>> +
>>> +Modifying formats or controls that influence the buffer size or layout
>>> require +the stream to be stopped. Any attempt at such a modification
>>> while the stream +is active shall cause the ioctl setting the format or
>>> the control to return +the ``EBUSY`` error code.
>>
>> This is my problem with putting the more complex case first: if you are
>> reading this for the first time then the preceding paragraph is simply
>> *wrong*.
>>
>> You cannot modify the buffer size when the stream is stopped. You need to
>> free all buffers first before you can do that.
>
> That's driver-dependent. If I reverse the order to start with the more
> restricted case, it will also be wrong for drivers that implement the other
> case.

It's not about wrong or right, it's about which case is the most common.

 > I'm running out of time so I'll change this though. We can always
> reorder the paragraphs later when we will have more drivers implementing the
> more powerful case.
>
>> Unless the driver has been especially written to allow that. And I am not
>> aware of any.
>>
>>> +
>>> +Controls that only influence the buffer layout can be modified at any
>>> time
>>> +when the stream is stopped. As they don't influence the buffer size, no
>>> +special handling is needed to synchronize those controls with buffer
>>> +allocation.
>>> +
>>> +Formats and controls that influence the buffer size interact with buffer
>>> +allocation. As buffer allocation is an expensive operation, drivers
>>> should
>>> +allow format or controls that influence the buffer size to be changed
>>> with
>>> +buffers allocated. A typical ioctl sequence to modify format and controls
>>> is +
>>> + #. VIDIOC_STREAMOFF
>>> + #. VIDIOC_S_EXT_CTRLS
>>> + #. VIDIOC_S_FMT
>>> + #. VIDIOC_QBUF
>>> + #. VIDIOC_STREAMON
>>> +
>>> +.. note::
>>> +
>>> +   The API doesn't mandate the above order for control (2.) and format
>>> (3.) +   changes. Format and controls can be set in a different order, or
>>> even +   interleaved, depending on the device and use case. For instance
>>> some +   controls might behave differently for different pixel formats,
>>> in which +   case the format might need to be set first.
>>> +
>>> +Queued buffers must be large enough for the new format or controls.
>>> +
>>> +Drivers shall return a ``ENOSPC`` error in response to format change
>>> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
>>> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
>>> +currently queued. As a simplification, drivers are allowed to return an
>>> error
>> s/an error/``EBUSY``/
>>
>>> +from these ioctls if any buffer is currently queued, without checking the
>>> +queued buffers sizes.
>>
>> Again, swap the order: simple case first, more complex case next.
>>
>>> +
>>> +.. note::
>>> +
>>> +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware
>>> (for +   instance if the device doesn't include a scaler), modify the
>>> format in +   addition to the selection rectangle. Similarly, the
>>> +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`,
>>> :c:func:`VIDIOC_S_STD` +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can
>>> also modify the format and +   selection rectangles. Driver shall return
>>> the same ``ENOSPC`` error from +   all ioctls that would result in
>>> formats too large for queued buffers.
>> This will return EBUSY for the 'simple' drivers, and ENOSPC for the complex
>> ones. Should be mentioned clearly to avoid confusion.
>>
>>> +
>>> +Drivers shall also return a ``ENOSPC`` error from the
>>> :c:func:`VIDIOC_QBUF` +ioctl if the buffer being queued is too small for
>>> the current format or +controls. Together, these requirements ensure that
>>> queued buffers will always +be large enough for the configured format and
>>> controls.
>>
>> NACK: it's EINVAL as returned by the buf_prepare() callbacks. Yes, I agree
>> that ENOSPC would have been more appropriate, but I do not believe we can
>> change this without breaking ABI. I also do not think this is all that
>> important. Feel free to blame this on lack for foresight :-)
>
> That's driver-dependent though, but I agree that due to cargo-cult programming
> most probably return -EINVAL today. Most, because I'm pretty sure we have
> drivers that don't check that condition, resulting in potential DMA buffer
> overflows. It might make sense to move the check to videobuf2-v4l2.c (but
> that's unrelated to this patch).

videobuf2-v4l2.c doesn't have the format information, so it can't do that
check. I agree that there are likely some drivers that do not check this,
which is definitely a bug. I think v4l2-compliance tries to check for this,
but I am not sure it is able to do so today for the DMABUF case, which is
the most relevant one.

>
> We should have used -ENOSPC, and I believe that switching to -ENOSPC now
> wouldn't result in any breakage. Can you think of any application that would
> queue a too small buffer, match the error code against -EINVAL, and have a
> strategy to recover ? Queuing a too small buffer is an application but in the
> first place, I don't think application developers will have gone through the
> trouble of implementing a way to recover from their own bug instead of fixing
> them :-)
>
> Would it be OK with you to document that -ENOSPC is the recommended error
> code, but that some drivers currently return -EINVAL ?

I really don't think it is worth changing this. Changing it to ENOSPC would
make drivers do inconsistent things. And the important thing is that it DOES
return an error.

Just stick to -EINVAL. The world won't end :-)

>
>>> +Userspace applications can query the buffer size required for a given
>>> format +and controls by first setting the desired control values and then
>>> trying the +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will
>>> return the required +buffer size.
>>> +
>>> + #. VIDIOC_S_EXT_CTRLS(x)
>>> + #. VIDIOC_TRY_FMT()
>>> + #. VIDIOC_S_EXT_CTRLS(y)
>>> + #. VIDIOC_TRY_FMT()
>>> +
>>> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate
>>> buffers +based on the queried sizes (for instance by allocating a set of
>>> buffers large +enough for all the desired formats and controls, or by
>>> allocating separate set +of appropriately sized buffers for each use
>>> case).
>>> +
>>> +To simplify their implementation, drivers may also require buffers to be
>>> +reallocated in order to change formats or controls that influence the
>>> buffer +size. In that case, to perform such changes, userspace
>>> applications shall +first stop the video stream with the
>>> :c:func:`VIDIOC_STREAMOFF` ioctl if it +is running and free all buffers
>>> with the :c:func:`VIDIOC_REQBUFS` ioctl if +they are allocated. The
>>> format or controls can then be modified, and buffers +shall then be
>>> reallocated and the stream restarted. A typical ioctl sequence +is
>>> +
>>> + #. VIDIOC_STREAMOFF
>>> + #. VIDIOC_REQBUFS(0)
>>> + #. VIDIOC_S_EXT_CTRLS
>>> + #. VIDIOC_S_FMT
>>> + #. VIDIOC_REQBUFS(n)
>>> + #. VIDIOC_QBUF
>>> + #. VIDIOC_STREAMON
>>> +
>>> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and
>>> control +value into account to compute the buffer size to allocate.
>>> Applications can +also retrieve the size by calling the
>>> :c:func:`VIDIOC_G_FMT` ioctl if needed. +
>>> +When reallocation is required, any attempt to modify format or controls
>>> that +influences the buffer size while buffers are allocated shall cause
>>> the format +or control set ioctl to return the ``EBUSY`` error code.
>>> +
>>> +
>>>
>>>  .. c:type:: v4l2_buffer
>>>
>>>  struct v4l2_buffer
>>
>> The order really has to be changed: first explain the 99%, then continue
>> with the more complex case. It's not the text itself, it's the order in
>> which this is presented. I won't accept it in this order as it will be
>> terminally confusing for most readers. Sorry.
>
>

Regards,

	Hans

Re: [PATCH v2.2] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

Hi Hans,

On Monday 06 Mar 2017 11:46:28 Hans Verkuil wrote:
> On 06/03/17 11:35, Laurent Pinchart wrote:
> > On Monday 06 Mar 2017 11:04:50 Hans Verkuil wrote:
> >> On 05/03/17 22:36, Laurent Pinchart wrote:
> >>> V4L2 exposes parameters that influence buffers sizes through the format
> >>> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> >>> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> >>> the format structure may also influence buffer sizes or buffer layout in
> >>> general. One existing such parameter is rotation, which is implemented
> >>> by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
> >>> ioctls.
> >>> 
> >>> The interaction between those parameters and buffers is currently only
> >>> partially specified by the V4L2 API. In particular interactions between
> >>> controls and buffers isn't specified at all. The behaviour of the
> >>> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> >>> also not fully specified.
> >>> 
> >>> This patch clearly defines and documents the interactions between
> >>> formats, selections, controls and buffers.
> >>> 
> >>> The preparatory discussions for the documentation change considered
> >>> completely disallowing controls that change the buffer size or layout,
> >>> in favour of extending the format API with a new ioctl that would bundle
> >>> those controls with format information. The idea has been rejected, as
> >>> this would essentially be a restricted version of the upcoming request
> >>> API that wouldn't bring any additional value.
> >>> 
> >>> Another option we have considered was to mandate the use of the request
> >>> API to modify controls that influence buffer size or layout. This has
> >>> also been rejected on the grounds that requiring the request API to
> >>> change rotation even when streaming is stopped would significantly
> >>> complicate implementation of drivers and usage of the V4L2 API for
> >>> applications.
> >>> 
> >>> Applications will however be required to use the upcoming request API to
> >>> change at runtime formats or controls that influence the buffer size or
> >>> layout, because of the need to synchronize buffers with the formats and
> >>> controls. Otherwise there would be no way to interpret the content of a
> >>> buffer correctly.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>> Changes since v2.1:
> >>> 
> >>> - Fixed small issues in commit message
> >>> - Simplified wording of one sentence in the documentation
> >>> 
> >>> Changes since v2:
> >>> 
> >>> - Document the interaction with ioctls that can affect formats
> >>>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
> >>>   VIDIOC_S_DV_TIMINGS)
> >>> - Clarify the format/control change order
> >>> ---
> >>> 
> >>>  Documentation/media/uapi/v4l/buffer.rst | 108 ++++++++++++++++++++++++
> >>>  1 file changed, 108 insertions(+)
> >>> 
> >>> diff --git a/Documentation/media/uapi/v4l/buffer.rst
> >>> b/Documentation/media/uapi/v4l/buffer.rst index
> >>> ac58966ccb9b..60d62a5824f8 100644
> >>> --- a/Documentation/media/uapi/v4l/buffer.rst
> >>> +++ b/Documentation/media/uapi/v4l/buffer.rst
> >>> @@ -34,6 +34,114 @@ flags are copied from the OUTPUT video buffer to the
> >>> CAPTURE video>
> >>> 
> >>>  buffer.
> >>> 
> >>> +Interactions between formats, controls and buffers
> >>> +==================================================
> >>> +
> >>> +V4L2 exposes parameters that influence the buffer size, or the way data
> >>> is +laid out in the buffer. Those parameters are exposed through both
> >>> formats and +controls. One example of such a control is the
> >>> ``V4L2_CID_ROTATE`` control +that modifies the direction in which pixels
> >>> are stored in the buffer, as well +as the buffer size when the selected
> >>> format includes padding at the end of +lines.
> >>> +
> >>> +The set of information needed to interpret the content of a buffer
> >>> (e.g. the +pixel format, the line stride, the tiling orientation or the
> >>> rotation) is +collectively referred to in the rest of this section as
> >>> the buffer layout.
> >>> +
> >>> +Modifying formats or controls that influence the buffer size or layout
> >>> require +the stream to be stopped. Any attempt at such a modification
> >>> while the stream +is active shall cause the ioctl setting the format or
> >>> the control to return +the ``EBUSY`` error code.
> >> 
> >> This is my problem with putting the more complex case first: if you are
> >> reading this for the first time then the preceding paragraph is simply
> >> *wrong*.
> >> 
> >> You cannot modify the buffer size when the stream is stopped. You need to
> >> free all buffers first before you can do that.
> > 
> > That's driver-dependent. If I reverse the order to start with the more
> > restricted case, it will also be wrong for drivers that implement the
> > other case.
> 
> It's not about wrong or right, it's about which case is the most common.

Note that you were the one saying the current order was "*wrong*" ;-) I'll 
change it anyway.

> > I'm running out of time so I'll change this though. We can always
> > reorder the paragraphs later when we will have more drivers implementing
> > the more powerful case.
> > 
> >> Unless the driver has been especially written to allow that. And I am not
> >> aware of any.
> >> 
> >>> +
> >>> +Controls that only influence the buffer layout can be modified at any
> >>> time +when the stream is stopped. As they don't influence the buffer
> >>> size, no +special handling is needed to synchronize those controls with
> >>> buffer +allocation.
> >>> +
> >>> +Formats and controls that influence the buffer size interact with
> >>> buffer +allocation. As buffer allocation is an expensive operation,
> >>> drivers should +allow format or controls that influence the buffer size
> >>> to be changed with +buffers allocated. A typical ioctl sequence to
> >>> modify format and controls is
> >>> +
> >>> + #. VIDIOC_STREAMOFF
> >>> + #. VIDIOC_S_EXT_CTRLS
> >>> + #. VIDIOC_S_FMT
> >>> + #. VIDIOC_QBUF
> >>> + #. VIDIOC_STREAMON
> >>> +
> >>> +.. note::
> >>> +
> >>> +   The API doesn't mandate the above order for control (2.) and format
> >>> (3.) +   changes. Format and controls can be set in a different order,
> >>> or even +   interleaved, depending on the device and use case. For
> >>> instance some +   controls might behave differently for different pixel
> >>> formats, in which +   case the format might need to be set first.
> >>> +
> >>> +Queued buffers must be large enough for the new format or controls.
> >>> +
> >>> +Drivers shall return a ``ENOSPC`` error in response to format change
> >>> +(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
> >>> +:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format
> >>> are +currently queued. As a simplification, drivers are allowed to
> >>> return an error
> >> 
> >> s/an error/``EBUSY``/
> >> 
> >>> +from these ioctls if any buffer is currently queued, without checking
> >>> the +queued buffers sizes.
> >> 
> >> Again, swap the order: simple case first, more complex case next.
> >> 
> >>> +
> >>> +.. note::
> >>> +
> >>> +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the
> >>> hardware (for +   instance if the device doesn't include a scaler),
> >>> modify the format in +   addition to the selection rectangle. Similarly,
> >>> the +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`,
> >>> 
> >>> :c:func:`VIDIOC_S_STD` +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can
> >>> 
> >>> also modify the format and +   selection rectangles. Driver shall return
> >>> the same ``ENOSPC`` error from +   all ioctls that would result in
> >>> formats too large for queued buffers.
> >> 
> >> This will return EBUSY for the 'simple' drivers, and ENOSPC for the
> >> complex ones. Should be mentioned clearly to avoid confusion.
> >> 
> >>> +
> >>> +Drivers shall also return a ``ENOSPC`` error from the
> >>> :c:func:`VIDIOC_QBUF` +ioctl if the buffer being queued is too small for
> >>> the current format or +controls. Together, these requirements ensure
> >>> that queued buffers will always +be large enough for the configured
> >>> format and controls.
> >> 
> >> NACK: it's EINVAL as returned by the buf_prepare() callbacks. Yes, I
> >> agree that ENOSPC would have been more appropriate, but I do not believe
> >> we can change this without breaking ABI. I also do not think this is all
> >> that important. Feel free to blame this on lack for foresight :-)
> > 
> > That's driver-dependent though, but I agree that due to cargo-cult
> > programming most probably return -EINVAL today. Most, because I'm pretty
> > sure we have drivers that don't check that condition, resulting in
> > potential DMA buffer overflows. It might make sense to move the check to
> > videobuf2-v4l2.c (but that's unrelated to this patch).
> 
> videobuf2-v4l2.c doesn't have the format information, so it can't do that
> check. I agree that there are likely some drivers that do not check this,
> which is definitely a bug. I think v4l2-compliance tries to check for this,
> but I am not sure it is able to do so today for the DMABUF case, which is
> the most relevant one.

This is getting out of scope for this patch, but I'll mention it for 
reference: While I agree that vb2 should not be format-aware, I believe it 
should grow to be size-aware. We could expose a new vb2 function that allows 
drivers to set the minimum size for each plane, and move all size checks 
within vb2. That would greatly simplify the implementation of the vb2 queue 
callbacks that are currently implemented in many different (and slightly or 
completely incompatible ways) by different drivers.

> > We should have used -ENOSPC, and I believe that switching to -ENOSPC now
> > wouldn't result in any breakage. Can you think of any application that
> > would queue a too small buffer, match the error code against -EINVAL, and
> > have a strategy to recover ? Queuing a too small buffer is an application
> > but in the first place, I don't think application developers will have
> > gone through the trouble of implementing a way to recover from their own
> > bug instead of fixing them :-)
> > 
> > Would it be OK with you to document that -ENOSPC is the recommended error
> > code, but that some drivers currently return -EINVAL ?
> 
> I really don't think it is worth changing this. Changing it to ENOSPC would
> make drivers do inconsistent things. And the important thing is that it DOES
> return an error.
> 
> Just stick to -EINVAL. The world won't end :-)

It will, but hopefully not because of that :-)

> >>> +Userspace applications can query the buffer size required for a given
> >>> format +and controls by first setting the desired control values and
> >>> then trying the +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will
> >>> return the required +buffer size.
> >>> +
> >>> + #. VIDIOC_S_EXT_CTRLS(x)
> >>> + #. VIDIOC_TRY_FMT()
> >>> + #. VIDIOC_S_EXT_CTRLS(y)
> >>> + #. VIDIOC_TRY_FMT()
> >>> +
> >>> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate
> >>> buffers +based on the queried sizes (for instance by allocating a set of
> >>> buffers large +enough for all the desired formats and controls, or by
> >>> allocating separate set +of appropriately sized buffers for each use
> >>> case).
> >>> +
> >>> +To simplify their implementation, drivers may also require buffers to
> >>> be +reallocated in order to change formats or controls that influence
> >>> the buffer +size. In that case, to perform such changes, userspace
> >>> applications shall +first stop the video stream with the
> >>> :c:func:`VIDIOC_STREAMOFF` ioctl if it +is running and free all buffers
> >>> with the :c:func:`VIDIOC_REQBUFS` ioctl if +they are allocated. The
> >>> format or controls can then be modified, and buffers +shall then be
> >>> reallocated and the stream restarted. A typical ioctl sequence +is
> >>> +
> >>> + #. VIDIOC_STREAMOFF
> >>> + #. VIDIOC_REQBUFS(0)
> >>> + #. VIDIOC_S_EXT_CTRLS
> >>> + #. VIDIOC_S_FMT
> >>> + #. VIDIOC_REQBUFS(n)
> >>> + #. VIDIOC_QBUF
> >>> + #. VIDIOC_STREAMON
> >>> +
> >>> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and
> >>> control +value into account to compute the buffer size to allocate.
> >>> Applications can +also retrieve the size by calling the
> >>> :c:func:`VIDIOC_G_FMT` ioctl if needed.
> >>> +
> >>> +When reallocation is required, any attempt to modify format or controls
> >>> that +influences the buffer size while buffers are allocated shall cause
> >>> the format +or control set ioctl to return the ``EBUSY`` error code.
> >>> +
> >>> +
> >>>  .. c:type:: v4l2_buffer
> >>>  
> >>>  struct v4l2_buffer
> >> 
> >> The order really has to be changed: first explain the 99%, then continue
> >> with the more complex case. It's not the text itself, it's the order in
> >> which this is presented. I won't accept it in this order as it will be
> >> terminally confusing for most readers. Sorry.

-- 
Regards,

Laurent Pinchart

[PATCH v2.3] v4l: Clearly document interactions between formats, controls and buffers

[Laurent Pinchart]

V4L2 exposes parameters that influence buffers sizes through the format
ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
the format structure may also influence buffer sizes or buffer layout in
general. One existing such parameter is rotation, which is implemented
by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
ioctls.

The interaction between those parameters and buffers is currently only
partially specified by the V4L2 API. In particular interactions between
controls and buffers isn't specified at all. The behaviour of the
VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
also not fully specified.

This patch clearly defines and documents the interactions between
formats, selections, controls and buffers.

The preparatory discussions for the documentation change considered
completely disallowing controls that change the buffer size or layout,
in favour of extending the format API with a new ioctl that would bundle
those controls with format information. The idea has been rejected, as
this would essentially be a restricted version of the upcoming request
API that wouldn't bring any additional value.

Another option we have considered was to mandate the use of the request
API to modify controls that influence buffer size or layout. This has
also been rejected on the grounds that requiring the request API to
change rotation even when streaming is stopped would significantly
complicate implementation of drivers and usage of the V4L2 API for
applications.

Applications will however be required to use the upcoming request API to
change at runtime formats or controls that influence the buffer size or
layout, because of the need to synchronize buffers with the formats and
controls. Otherwise there would be no way to interpret the content of a
buffer correctly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Changes since v2.2:

- Describe the simple option first
- Fix error codes

Changes since v2.1:

- Fixed small issues in commit message
- Simplified wording of one sentence in the documentation

Changes since v2:

- Document the interaction with ioctls that can affect formats
  (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
  VIDIOC_S_DV_TIMINGS)
- Clarify the format/control change order
---
 Documentation/media/uapi/v4l/buffer.rst | 110 ++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ac58966ccb9b..d1e0d55dc219 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -34,6 +34,116 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
 buffer.
 
 
+Interactions between formats, controls and buffers
+==================================================
+
+V4L2 exposes parameters that influence the buffer size, or the way data is
+laid out in the buffer. Those parameters are exposed through both formats and
+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
+that modifies the direction in which pixels are stored in the buffer, as well
+as the buffer size when the selected format includes padding at the end of
+lines.
+
+The set of information needed to interpret the content of a buffer (e.g. the
+pixel format, the line stride, the tiling orientation or the rotation) is
+collectively referred to in the rest of this section as the buffer layout.
+
+Modifying formats or controls that influence the buffer size or layout require
+the stream to be stopped. Any attempt at such a modification while the stream
+is active shall cause the ioctl setting the format or the control to return
+the ``EBUSY`` error code.
+
+.. note::
+
+   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
+   instance if the device doesn't include a scaler), modify the format in
+   addition to the selection rectangle. Similarly, the
+   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
+   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
+   selection rectangles. When those ioctls result in a buffer size or layout
+   change, drivers shall handle that condition as they would handle it in the
+   :c:func:`VIDIOC_S_FMT` ioctl in all cases described in this section.
+
+Controls that only influence the buffer layout can be modified at any time
+when the stream is stopped. As they don't influence the buffer size, no
+special handling is needed to synchronize those controls with buffer
+allocation.
+
+Formats and controls that influence the buffer size interact with buffer
+allocation. The simplest way to handle this is for drivers to always require
+buffers to be reallocated in order to change those formats or controls. In
+that case, to perform such changes, userspace applications shall first stop
+the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it is running
+and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if they are
+allocated. The format or controls can then be modified, and buffers shall then
+be reallocated and the stream restarted. A typical ioctl sequence is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_REQBUFS(0)
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_S_FMT
+ #. VIDIOC_REQBUFS(n)
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
+value into account to compute the buffer size to allocate. Applications can
+also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
+
+.. note::
+
+   The API doesn't mandate the above order for control (3.) and format (4.)
+   changes. Format and controls can be set in a different order, or even
+   interleaved, depending on the device and use case. For instance some
+   controls might behave differently for different pixel formats, in which
+   case the format might need to be set first.
+
+When reallocation is required, any attempt to modify format or controls that
+influences the buffer size while buffers are allocated shall cause the format
+or control set ioctl to return the ``EBUSY`` error. Any attempt to queue a
+buffer too small for the current format or controls shall cause the
+:c:func:`VIDIOC_QBUF` ioctl to return a ``EINVAL`` error.
+
+Buffer reallocation is an expensive operation. To avoid that cost, drivers can
+(and are encouraged to) allow format or controls that influence the buffer
+size to be changed with buffers allocated. In that case, a typical ioctl
+sequence to modify format and controls is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_S_FMT
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+For this sequence to operate correctly, queued buffers need to be large enough
+for the new format or controls. Drivers shall return a ``ENOSPC`` error in
+response to format change (:c:func:`VIDIOC_S_FMT`) or control changes
+(:c:func:`VIDIOC_S_CTRL` or :c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small
+for the new format are currently queued. As a simplification, drivers are
+allowed to return a ``EBUSY`` error from these ioctls if any buffer is
+currently queued, without checking the queued buffers sizes.
+
+Additionally, drivers shall return a ``EINVAL`` error from the
+:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
+current format or controls. Together, these requirements ensure that queued
+buffers will always be large enough for the configured format and controls.
+
+Userspace applications can query the buffer size required for a given format
+and controls by first setting the desired control values and then trying the
+desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
+buffer size.
+
+ #. VIDIOC_S_EXT_CTRLS(x)
+ #. VIDIOC_TRY_FMT()
+ #. VIDIOC_S_EXT_CTRLS(y)
+ #. VIDIOC_TRY_FMT()
+
+The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
+based on the queried sizes (for instance by allocating a set of buffers large
+enough for all the desired formats and controls, or by allocating separate set
+of appropriately sized buffers for each use case).
+
+
 .. c:type:: v4l2_buffer
 
 struct v4l2_buffer
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2.3] v4l: Clearly document interactions between formats, controls and buffers

[Hans Verkuil]

On 06/03/17 15:14, Laurent Pinchart wrote:
> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> the format structure may also influence buffer sizes or buffer layout in
> general. One existing such parameter is rotation, which is implemented
> by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
> ioctls.
>
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> also not fully specified.
>
> This patch clearly defines and documents the interactions between
> formats, selections, controls and buffers.
>
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
>
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
>
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
> Changes since v2.2:
>
> - Describe the simple option first
> - Fix error codes
>
> Changes since v2.1:
>
> - Fixed small issues in commit message
> - Simplified wording of one sentence in the documentation
>
> Changes since v2:
>
> - Document the interaction with ioctls that can affect formats
>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>   VIDIOC_S_DV_TIMINGS)
> - Clarify the format/control change order
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 110 ++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..d1e0d55dc219 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -34,6 +34,116 @@ flags are copied from the OUTPUT video buffer to the CAPTURE video
>  buffer.
>
>
> +Interactions between formats, controls and buffers
> +==================================================
> +
> +V4L2 exposes parameters that influence the buffer size, or the way data is
> +laid out in the buffer. Those parameters are exposed through both formats and
> +controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> +that modifies the direction in which pixels are stored in the buffer, as well
> +as the buffer size when the selected format includes padding at the end of
> +lines.
> +
> +The set of information needed to interpret the content of a buffer (e.g. the
> +pixel format, the line stride, the tiling orientation or the rotation) is
> +collectively referred to in the rest of this section as the buffer layout.
> +
> +Modifying formats or controls that influence the buffer size or layout require
> +the stream to be stopped. Any attempt at such a modification while the stream
> +is active shall cause the ioctl setting the format or the control to return
> +the ``EBUSY`` error code.
> +
> +.. note::
> +
> +   The :c:func:`VIDIOC_S_SELECTION` ioctl can, depending on the hardware (for
> +   instance if the device doesn't include a scaler), modify the format in
> +   addition to the selection rectangle. Similarly, the
> +   :c:func:`VIDIOC_S_INPUT`, :c:func:`VIDIOC_S_OUTPUT`, :c:func:`VIDIOC_S_STD`
> +   and :c:func:`VIDIOC_S_DV_TIMINGS` ioctls can also modify the format and
> +   selection rectangles. When those ioctls result in a buffer size or layout
> +   change, drivers shall handle that condition as they would handle it in the
> +   :c:func:`VIDIOC_S_FMT` ioctl in all cases described in this section.
> +
> +Controls that only influence the buffer layout can be modified at any time
> +when the stream is stopped. As they don't influence the buffer size, no
> +special handling is needed to synchronize those controls with buffer
> +allocation.
> +
> +Formats and controls that influence the buffer size interact with buffer
> +allocation. The simplest way to handle this is for drivers to always require
> +buffers to be reallocated in order to change those formats or controls. In
> +that case, to perform such changes, userspace applications shall first stop
> +the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it is running
> +and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if they are
> +allocated. The format or controls can then be modified, and buffers shall then
> +be reallocated and the stream restarted. A typical ioctl sequence is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_REQBUFS(0)
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_REQBUFS(n)
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +The second :c:func:`VIDIOC_REQBUFS` call will take the new format and control
> +value into account to compute the buffer size to allocate. Applications can
> +also retrieve the size by calling the :c:func:`VIDIOC_G_FMT` ioctl if needed.
> +
> +.. note::
> +
> +   The API doesn't mandate the above order for control (3.) and format (4.)
> +   changes. Format and controls can be set in a different order, or even
> +   interleaved, depending on the device and use case. For instance some
> +   controls might behave differently for different pixel formats, in which
> +   case the format might need to be set first.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error. Any attempt to queue a
> +buffer too small for the current format or controls shall cause the
> +:c:func:`VIDIOC_QBUF` ioctl to return a ``EINVAL`` error.
> +
> +Buffer reallocation is an expensive operation. To avoid that cost, drivers can
> +(and are encouraged to) allow format or controls that influence the buffer
> +size to be changed with buffers allocated. In that case, a typical ioctl
> +sequence to modify format and controls is
> +
> + #. VIDIOC_STREAMOFF
> + #. VIDIOC_S_EXT_CTRLS
> + #. VIDIOC_S_FMT
> + #. VIDIOC_QBUF
> + #. VIDIOC_STREAMON
> +
> +For this sequence to operate correctly, queued buffers need to be large enough
> +for the new format or controls. Drivers shall return a ``ENOSPC`` error in
> +response to format change (:c:func:`VIDIOC_S_FMT`) or control changes
> +(:c:func:`VIDIOC_S_CTRL` or :c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small
> +for the new format are currently queued. As a simplification, drivers are
> +allowed to return a ``EBUSY`` error from these ioctls if any buffer is
> +currently queued, without checking the queued buffers sizes.
> +
> +Additionally, drivers shall return a ``EINVAL`` error from the
> +:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
> +current format or controls. Together, these requirements ensure that queued
> +buffers will always be large enough for the configured format and controls.
> +
> +Userspace applications can query the buffer size required for a given format
> +and controls by first setting the desired control values and then trying the
> +desired format. The :c:func:`VIDIOC_TRY_FMT` ioctl will return the required
> +buffer size.
> +
> + #. VIDIOC_S_EXT_CTRLS(x)
> + #. VIDIOC_TRY_FMT()
> + #. VIDIOC_S_EXT_CTRLS(y)
> + #. VIDIOC_TRY_FMT()
> +
> +The :c:func:`VIDIOC_CREATE_BUFS` ioctl can then be used to allocate buffers
> +based on the queried sizes (for instance by allocating a set of buffers large
> +enough for all the desired formats and controls, or by allocating separate set
> +of appropriately sized buffers for each use case).
> +
> +
>  .. c:type:: v4l2_buffer
>
>  struct v4l2_buffer
>

Re: [PATCH v2.3] v4l: Clearly document interactions between formats, controls and buffers

[Mauro Carvalho Chehab]

Em Mon,  6 Mar 2017 16:14:41 +0200
Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> escreveu:

> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> the format structure may also influence buffer sizes or buffer layout in
> general. One existing such parameter is rotation, which is implemented
> by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
> ioctls.
> 
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> also not fully specified.
> 
> This patch clearly defines and documents the interactions between
> formats, selections, controls and buffers.
> 
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
> 
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
> 
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Changes since v2.2:
> 
> - Describe the simple option first
> - Fix error codes
> 
> Changes since v2.1:
> 
> - Fixed small issues in commit message
> - Simplified wording of one sentence in the documentation
> 
> Changes since v2:
> 
> - Document the interaction with ioctls that can affect formats
>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>   VIDIOC_S_DV_TIMINGS)
> - Clarify the format/control change order
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 110 ++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..d1e0d55dc219 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst

...

> +.. note::
> +
> +   The API doesn't mandate the above order for control (3.) and format (4.)
> +   changes. Format and controls can be set in a different order, or even
> +   interleaved, depending on the device and use case. For instance some
> +   controls might behave differently for different pixel formats, in which
> +   case the format might need to be set first.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error. Any attempt to queue a
> +buffer too small for the current format or controls shall cause the
> +:c:func:`VIDIOC_QBUF` ioctl to return a ``EINVAL`` error.

This can be problematic. As I just implemented support for controls
this weekend at Zbar, I'm now talking as an userspace app developer's
hat.

The real problem here is that applications must be aware of what
controls change the buffer layout. Blindly changing controls without
such check would cause the stream to fail with -EINVAL errors at
QBUF.

So, applications will need to to have a "black list" of controls that may
influence the buffer size  (like V4L2_CID_ROTATE), in order to know
if, for such particular control, the stream should be stopped, in
order to reallocate buffers.

If such "black list" is not updated as newer controls are added, the
final result is that, if the user changes such control, the 
application will receive EINVAL, causing it to fail streaming.

Instead of that, the best is to add control flag to be returned via
VIDIOC_QUERY_CTRL/VIDIOC_QUERY_EXT_CTRL indicating when a control modifies 
the buffer layout, e. g., something like:

#define V4L2_CTRL_FLAG_MODIFY_BUF_LAYOUT	0x0400

Such flag shall be set for V4L2_CID_ROTATE (and other controls) if,
for a particular driver, the buffer layout is modified.

This way, userspace can recognize such controls in runtime and
reallocate the buffers if required by such controls.


Regards

Thanks,
Mauro

[PATCH v2 3/3] v4l: vsp1: wpf: Implement rotation support

[Laurent Pinchart]

Some WPF instances, on Gen3 devices, can perform 90° rotation when
writing frames to memory. Implement support for this using the
V4L2_CID_ROTATE control.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_rpf.c   |   2 +-
 drivers/media/platform/vsp1/vsp1_rwpf.c  |   5 +
 drivers/media/platform/vsp1/vsp1_rwpf.h  |   3 +-
 drivers/media/platform/vsp1/vsp1_video.c |  12 +-
 drivers/media/platform/vsp1/vsp1_wpf.c   | 205 +++++++++++++++++++++++--------
 5 files changed, 175 insertions(+), 52 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index f5a9a4c8c74d..8feddd59cf8d 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -106,7 +106,7 @@ static void rpf_configure(struct vsp1_entity *entity,
 			 * of the pipeline.
 			 */
 			output = vsp1_entity_get_pad_format(wpf, wpf->config,
-							    RWPF_PAD_SOURCE);
+							    RWPF_PAD_SINK);
 
 			crop.width = pipe->partition.width * input_width
 				   / output->width;
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c
index 7d52c88a583e..cfd8f1904fa6 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
@@ -121,6 +121,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
 					    RWPF_PAD_SOURCE);
 	*format = fmt->format;
 
+	if (rwpf->flip.rotate) {
+		format->width = fmt->format.height;
+		format->height = fmt->format.width;
+	}
+
 done:
 	mutex_unlock(&rwpf->entity.lock);
 	return ret;
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
index 1c98aff3da5d..b4ffc38f48af 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.h
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
@@ -56,9 +56,10 @@ struct vsp1_rwpf {
 
 	struct {
 		spinlock_t lock;
-		struct v4l2_ctrl *ctrls[2];
+		struct v4l2_ctrl *ctrls[3];
 		unsigned int pending;
 		unsigned int active;
+		bool rotate;
 	} flip;
 
 	struct vsp1_rwpf_memory mem;
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 5239e08fabc3..795a3ca9ca03 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -187,9 +187,13 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 	struct vsp1_entity *entity;
 	unsigned int div_size;
 
+	/*
+	 * Partitions are computed on the size before rotation, use the format
+	 * at the WPF sink.
+	 */
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
 					    pipe->output->entity.config,
-					    RWPF_PAD_SOURCE);
+					    RWPF_PAD_SINK);
 	div_size = format->width;
 
 	/* Gen2 hardware doesn't require image partitioning. */
@@ -229,9 +233,13 @@ static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
 	struct v4l2_rect partition;
 	unsigned int modulus;
 
+	/*
+	 * Partitions are computed on the size before rotation, use the format
+	 * at the WPF sink.
+	 */
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
 					    pipe->output->entity.config,
-					    RWPF_PAD_SOURCE);
+					    RWPF_PAD_SINK);
 
 	/* A single partition simply processes the output size in full. */
 	if (pipe->partitions <= 1) {
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 25a2ed6e2e18..82935d97cad9 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -43,32 +43,94 @@ static inline void vsp1_wpf_write(struct vsp1_rwpf *wpf,
 enum wpf_flip_ctrl {
 	WPF_CTRL_VFLIP = 0,
 	WPF_CTRL_HFLIP = 1,
-	WPF_CTRL_MAX,
+	WPF_CTRL_ROTATE = 2,
 };
 
+static int vsp1_wpf_set_rotation(struct vsp1_rwpf *wpf, unsigned int rotation)
+{
+	struct vsp1_video *video = wpf->video;
+	struct v4l2_mbus_framefmt *sink_format;
+	struct v4l2_mbus_framefmt *source_format;
+	bool rotate;
+	int ret = 0;
+
+	/*
+	 * Only consider the 0°/180° from/to 90°/270° modifications, the rest
+	 * is taken care of by the flipping configuration.
+	 */
+	rotate = rotation == 90 || rotation == 270;
+	if (rotate == wpf->flip.rotate)
+		return 0;
+
+	/* Changing rotation isn't allowed when buffers are allocated. */
+	mutex_lock(&video->lock);
+
+	if (vb2_is_busy(&video->queue)) {
+		ret = -EBUSY;
+		goto done;
+	}
+
+	sink_format = vsp1_entity_get_pad_format(&wpf->entity,
+						 wpf->entity.config,
+						 RWPF_PAD_SINK);
+	source_format = vsp1_entity_get_pad_format(&wpf->entity,
+						   wpf->entity.config,
+						   RWPF_PAD_SOURCE);
+
+	mutex_lock(&wpf->entity.lock);
+
+	if (rotate) {
+		source_format->width = sink_format->height;
+		source_format->height = sink_format->width;
+	} else {
+		source_format->width = sink_format->width;
+		source_format->height = sink_format->height;
+	}
+
+	wpf->flip.rotate = rotate;
+
+	mutex_unlock(&wpf->entity.lock);
+
+done:
+	mutex_unlock(&video->lock);
+	return ret;
+}
+
 static int vsp1_wpf_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct vsp1_rwpf *wpf =
 		container_of(ctrl->handler, struct vsp1_rwpf, ctrls);
-	unsigned int i;
+	unsigned int rotation;
 	u32 flip = 0;
+	int ret;
 
-	switch (ctrl->id) {
-	case V4L2_CID_HFLIP:
-	case V4L2_CID_VFLIP:
-		for (i = 0; i < WPF_CTRL_MAX; ++i) {
-			if (wpf->flip.ctrls[i])
-				flip |= wpf->flip.ctrls[i]->val ? BIT(i) : 0;
-		}
+	/* Update the rotation. */
+	rotation = wpf->flip.ctrls[WPF_CTRL_ROTATE]
+		 ? wpf->flip.ctrls[WPF_CTRL_ROTATE]->val : 0;
 
-		spin_lock_irq(&wpf->flip.lock);
-		wpf->flip.pending = flip;
-		spin_unlock_irq(&wpf->flip.lock);
-		break;
+	ret = vsp1_wpf_set_rotation(wpf, rotation);
+	if (ret < 0)
+		return ret;
 
-	default:
-		return -EINVAL;
-	}
+	/*
+	 * Compute the flip value resulting from all three controls, with
+	 * rotation by 180° flipping the image in both directions. Store the
+	 * result in the pending flip field for the next frame that will be
+	 * processed.
+	 */
+	if (wpf->flip.ctrls[WPF_CTRL_VFLIP]->val)
+		flip |= BIT(WPF_CTRL_VFLIP);
+
+	if (wpf->flip.ctrls[WPF_CTRL_HFLIP] &&
+	    wpf->flip.ctrls[WPF_CTRL_HFLIP]->val)
+		flip |= BIT(WPF_CTRL_HFLIP);
+
+	if (rotation == 180 || rotation == 270)
+		flip ^= BIT(WPF_CTRL_VFLIP) | BIT(WPF_CTRL_HFLIP);
+
+	spin_lock_irq(&wpf->flip.lock);
+	wpf->flip.pending = flip;
+	spin_unlock_irq(&wpf->flip.lock);
 
 	return 0;
 }
@@ -89,10 +151,10 @@ static int wpf_init_controls(struct vsp1_rwpf *wpf)
 		num_flip_ctrls = 0;
 	} else if (vsp1->info->features & VSP1_HAS_WPF_HFLIP) {
 		/*
-		 * When horizontal flip is supported the WPF implements two
-		 * controls (horizontal flip and vertical flip).
+		 * When horizontal flip is supported the WPF implements three
+		 * controls (horizontal flip, vertical flip and rotation).
 		 */
-		num_flip_ctrls = 2;
+		num_flip_ctrls = 3;
 	} else if (vsp1->info->features & VSP1_HAS_WPF_VFLIP) {
 		/*
 		 * When only vertical flip is supported the WPF implements a
@@ -112,12 +174,14 @@ static int wpf_init_controls(struct vsp1_rwpf *wpf)
 					  V4L2_CID_VFLIP, 0, 1, 1, 0);
 	}
 
-	if (num_flip_ctrls == 2) {
+	if (num_flip_ctrls == 3) {
 		wpf->flip.ctrls[WPF_CTRL_HFLIP] =
 			v4l2_ctrl_new_std(&wpf->ctrls, &vsp1_wpf_ctrl_ops,
 					  V4L2_CID_HFLIP, 0, 1, 1, 0);
-
-		v4l2_ctrl_cluster(2, wpf->flip.ctrls);
+		wpf->flip.ctrls[WPF_CTRL_ROTATE] =
+			v4l2_ctrl_new_std(&wpf->ctrls, &vsp1_wpf_ctrl_ops,
+					  V4L2_CID_ROTATE, 0, 270, 90, 0);
+		v4l2_ctrl_cluster(3, wpf->flip.ctrls);
 	}
 
 	if (wpf->ctrls.error) {
@@ -222,8 +286,8 @@ static void wpf_configure(struct vsp1_entity *entity,
 		const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
 		struct vsp1_rwpf_memory mem = wpf->mem;
 		unsigned int flip = wpf->flip.active;
-		unsigned int width = source_format->width;
-		unsigned int height = source_format->height;
+		unsigned int width = sink_format->width;
+		unsigned int height = sink_format->height;
 		unsigned int offset;
 
 		/*
@@ -246,45 +310,78 @@ static void wpf_configure(struct vsp1_entity *entity,
 		/*
 		 * Update the memory offsets based on flipping configuration.
 		 * The destination addresses point to the locations where the
-		 * VSP starts writing to memory, which can be different corners
-		 * of the image depending on vertical flipping.
+		 * VSP starts writing to memory, which can be any corner of the
+		 * image depending on the combination of flipping and rotation.
 		 */
-		if (pipe->partitions > 1) {
-			const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
 
-			/*
-			 * Horizontal flipping is handled through a line buffer
-			 * and doesn't modify the start address, but still needs
-			 * to be handled when image partitioning is in effect to
-			 * order the partitions correctly.
-			 */
-			if (flip & BIT(WPF_CTRL_HFLIP))
-				offset = format->width - pipe->partition.left
-					- pipe->partition.width;
+		/*
+		 * First take the partition left coordinate into account.
+		 * Compute the offset to order the partitions correctly on the
+		 * output based on whether flipping is enabled. Consider
+		 * horizontal flipping when rotation is disabled but vertical
+		 * flipping when rotation is enabled, as rotating the image
+		 * switches the horizontal and vertical directions. The offset
+		 * is applied horizontally or vertically accordingly.
+		 */
+		if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
+			offset = format->width - pipe->partition.left
+				- pipe->partition.width;
+		else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
+			offset = format->height - pipe->partition.left
+				- pipe->partition.width;
+		else
+			offset = pipe->partition.left;
+
+		for (i = 0; i < format->num_planes; ++i) {
+			unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
+			unsigned int vsub = i > 0 ? fmtinfo->vsub : 1;
+
+			if (wpf->flip.rotate)
+				mem.addr[i] += offset / vsub
+					     * format->plane_fmt[i].bytesperline;
 			else
-				offset = pipe->partition.left;
-
-			mem.addr[0] += offset * fmtinfo->bpp[0] / 8;
-			if (format->num_planes > 1) {
-				mem.addr[1] += offset / fmtinfo->hsub
-					     * fmtinfo->bpp[1] / 8;
-				mem.addr[2] += offset / fmtinfo->hsub
-					     * fmtinfo->bpp[2] / 8;
-			}
+				mem.addr[i] += offset / hsub
+					     * fmtinfo->bpp[i] / 8;
 		}
 
 		if (flip & BIT(WPF_CTRL_VFLIP)) {
-			mem.addr[0] += (format->height - 1)
+			/*
+			 * When rotating the output (after rotation) image
+			 * height is equal to the partition width (before
+			 * rotation). Otherwise it is equal to the output
+			 * image height.
+			 */
+			if (wpf->flip.rotate)
+				height = pipe->partition.width;
+			else
+				height = format->height;
+
+			mem.addr[0] += (height - 1)
 				     * format->plane_fmt[0].bytesperline;
 
 			if (format->num_planes > 1) {
-				offset = (format->height / wpf->fmtinfo->vsub - 1)
+				offset = (height / fmtinfo->vsub - 1)
 				       * format->plane_fmt[1].bytesperline;
 				mem.addr[1] += offset;
 				mem.addr[2] += offset;
 			}
 		}
 
+		if (wpf->flip.rotate && !(flip & BIT(WPF_CTRL_HFLIP))) {
+			unsigned int hoffset = max(0, (int)format->width - 16);
+
+			/*
+			 * Compute the output coordinate. The partition
+			 * horizontal (left) offset becomes a vertical offset.
+			 */
+			for (i = 0; i < format->num_planes; ++i) {
+				unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
+
+				mem.addr[i] += hoffset / hsub
+					     * fmtinfo->bpp[i] / 8;
+			}
+		}
+
 		/*
 		 * On Gen3 hardware the SPUVS bit has no effect on 3-planar
 		 * formats. Swap the U and V planes manually in that case.
@@ -306,6 +403,9 @@ static void wpf_configure(struct vsp1_entity *entity,
 
 		outfmt = fmtinfo->hwfmt << VI6_WPF_OUTFMT_WRFMT_SHIFT;
 
+		if (wpf->flip.rotate)
+			outfmt |= VI6_WPF_OUTFMT_ROT;
+
 		if (fmtinfo->alpha)
 			outfmt |= VI6_WPF_OUTFMT_PXA;
 		if (fmtinfo->swap_yc)
@@ -367,9 +467,18 @@ static void wpf_configure(struct vsp1_entity *entity,
 			   VI6_WFP_IRQ_ENB_DFEE);
 }
 
+static unsigned int wpf_max_width(struct vsp1_entity *entity,
+				  struct vsp1_pipeline *pipe)
+{
+	struct vsp1_rwpf *wpf = to_rwpf(&entity->subdev);
+
+	return wpf->flip.rotate ? 256 : wpf->max_width;
+}
+
 static const struct vsp1_entity_operations wpf_entity_ops = {
 	.destroy = vsp1_wpf_destroy,
 	.configure = wpf_configure,
+	.max_width = wpf_max_width,
 };
 
 /* -----------------------------------------------------------------------------
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] v4l: vsp1: wpf: Implement rotation support

[Sakari Ailus]

Hi Laurent,

On Tue, Feb 28, 2017 at 05:03:20PM +0200, Laurent Pinchart wrote:
> Some WPF instances, on Gen3 devices, can perform 90° rotation when
> writing frames to memory. Implement support for this using the
> V4L2_CID_ROTATE control.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_rpf.c   |   2 +-
>  drivers/media/platform/vsp1/vsp1_rwpf.c  |   5 +
>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   3 +-
>  drivers/media/platform/vsp1/vsp1_video.c |  12 +-
>  drivers/media/platform/vsp1/vsp1_wpf.c   | 205 +++++++++++++++++++++++--------
>  5 files changed, 175 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
> index f5a9a4c8c74d..8feddd59cf8d 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -106,7 +106,7 @@ static void rpf_configure(struct vsp1_entity *entity,
>  			 * of the pipeline.
>  			 */
>  			output = vsp1_entity_get_pad_format(wpf, wpf->config,
> -							    RWPF_PAD_SOURCE);
> +							    RWPF_PAD_SINK);
>  
>  			crop.width = pipe->partition.width * input_width
>  				   / output->width;
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c
> index 7d52c88a583e..cfd8f1904fa6 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
> @@ -121,6 +121,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>  					    RWPF_PAD_SOURCE);
>  	*format = fmt->format;
>  
> +	if (rwpf->flip.rotate) {
> +		format->width = fmt->format.height;
> +		format->height = fmt->format.width;
> +	}
> +
>  done:
>  	mutex_unlock(&rwpf->entity.lock);
>  	return ret;
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
> index 1c98aff3da5d..b4ffc38f48af 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> @@ -56,9 +56,10 @@ struct vsp1_rwpf {
>  
>  	struct {
>  		spinlock_t lock;
> -		struct v4l2_ctrl *ctrls[2];
> +		struct v4l2_ctrl *ctrls[3];

At least what comes to this patch --- having a field for each control would
look much nicer in the code. Is there a particular reason for having an
array with all the controls in it?

>  		unsigned int pending;
>  		unsigned int active;
> +		bool rotate;
>  	} flip;
>  
>  	struct vsp1_rwpf_memory mem;

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2 3/3] v4l: vsp1: wpf: Implement rotation support

[Sakari Ailus]

Hi Laurent,

On Tue, Feb 28, 2017 at 05:03:20PM +0200, Laurent Pinchart wrote:
> Some WPF instances, on Gen3 devices, can perform 90� rotation when
> writing frames to memory. Implement support for this using the
> V4L2_CID_ROTATE control.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_rpf.c   |   2 +-
>  drivers/media/platform/vsp1/vsp1_rwpf.c  |   5 +
>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   3 +-
>  drivers/media/platform/vsp1/vsp1_video.c |  12 +-
>  drivers/media/platform/vsp1/vsp1_wpf.c   | 205 +++++++++++++++++++++++--------
>  5 files changed, 175 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
> index f5a9a4c8c74d..8feddd59cf8d 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -106,7 +106,7 @@ static void rpf_configure(struct vsp1_entity *entity,
>  			 * of the pipeline.
>  			 */
>  			output = vsp1_entity_get_pad_format(wpf, wpf->config,
> -							    RWPF_PAD_SOURCE);
> +							    RWPF_PAD_SINK);
>  
>  			crop.width = pipe->partition.width * input_width
>  				   / output->width;
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c
> index 7d52c88a583e..cfd8f1904fa6 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
> @@ -121,6 +121,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>  					    RWPF_PAD_SOURCE);
>  	*format = fmt->format;
>  
> +	if (rwpf->flip.rotate) {
> +		format->width = fmt->format.height;
> +		format->height = fmt->format.width;
> +	}
> +
>  done:
>  	mutex_unlock(&rwpf->entity.lock);
>  	return ret;
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
> index 1c98aff3da5d..b4ffc38f48af 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> @@ -56,9 +56,10 @@ struct vsp1_rwpf {
>  
>  	struct {
>  		spinlock_t lock;
> -		struct v4l2_ctrl *ctrls[2];
> +		struct v4l2_ctrl *ctrls[3];

At least what comes to this patch --- having a field for each control would
look much nicer in the code. Is there a particular reason for having an
array with all the controls in it?

>  		unsigned int pending;
>  		unsigned int active;
> +		bool rotate;
>  	} flip;
>  
>  	struct vsp1_rwpf_memory mem;

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2 3/3] v4l: vsp1: wpf: Implement rotation support

[Laurent Pinchart]

Hi Sakari,

On Tuesday 28 Feb 2017 23:13:34 Sakari Ailus wrote:
> On Tue, Feb 28, 2017 at 05:03:20PM +0200, Laurent Pinchart wrote:
> > Some WPF instances, on Gen3 devices, can perform 90° rotation when
> > writing frames to memory. Implement support for this using the
> > V4L2_CID_ROTATE control.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_rpf.c   |   2 +-
> >  drivers/media/platform/vsp1/vsp1_rwpf.c  |   5 +
> >  drivers/media/platform/vsp1/vsp1_rwpf.h  |   3 +-
> >  drivers/media/platform/vsp1/vsp1_video.c |  12 +-
> >  drivers/media/platform/vsp1/vsp1_wpf.c   | 205 ++++++++++++++++++--------
> >  5 files changed, 175 insertions(+), 52 deletions(-)

[snip]

> > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h
> > b/drivers/media/platform/vsp1/vsp1_rwpf.h index
> > 1c98aff3da5d..b4ffc38f48af 100644
> > --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > @@ -56,9 +56,10 @@ struct vsp1_rwpf {
> > 
> >  	struct {
> >  		spinlock_t lock;
> > -		struct v4l2_ctrl *ctrls[2];
> > +		struct v4l2_ctrl *ctrls[3];
> 
> At least what comes to this patch --- having a field for each control would
> look much nicer in the code. Is there a particular reason for having an
> array with all the controls in it?

Not really. I need to put the three controls in a cluster, so I stored them in 
an array to make sure they would be properly contiguous in memory. I can also 
use three separate pointers, it shouldn't hurt.

> >  		unsigned int pending;
> >  		unsigned int active;
> > +		bool rotate;
> >  	} flip;
> >  	
> >  	struct vsp1_rwpf_memory mem;

-- 
Regards,

Laurent Pinchart

[PATCH v2.1 3/3] v4l: vsp1: wpf: Implement rotation support

[Laurent Pinchart]

Some WPF instances, on Gen3 devices, can perform 90° rotation when
writing frames to memory. Implement support for this using the
V4L2_CID_ROTATE control.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Store controls in distinct fields instead of array.
---
 drivers/media/platform/vsp1/vsp1_rpf.c   |   2 +-
 drivers/media/platform/vsp1/vsp1_rwpf.c  |   5 +
 drivers/media/platform/vsp1/vsp1_rwpf.h  |   7 +-
 drivers/media/platform/vsp1/vsp1_video.c |  12 +-
 drivers/media/platform/vsp1/vsp1_wpf.c   | 205 +++++++++++++++++++++++--------
 5 files changed, 177 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index f5a9a4c8c74d..8feddd59cf8d 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -106,7 +106,7 @@ static void rpf_configure(struct vsp1_entity *entity,
 			 * of the pipeline.
 			 */
 			output = vsp1_entity_get_pad_format(wpf, wpf->config,
-							    RWPF_PAD_SOURCE);
+							    RWPF_PAD_SINK);
 
 			crop.width = pipe->partition.width * input_width
 				   / output->width;
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c
index 7d52c88a583e..cfd8f1904fa6 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
@@ -121,6 +121,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
 					    RWPF_PAD_SOURCE);
 	*format = fmt->format;
 
+	if (rwpf->flip.rotate) {
+		format->width = fmt->format.height;
+		format->height = fmt->format.width;
+	}
+
 done:
 	mutex_unlock(&rwpf->entity.lock);
 	return ret;
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
index 1c98aff3da5d..58215a7ab631 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.h
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
@@ -56,9 +56,14 @@ struct vsp1_rwpf {
 
 	struct {
 		spinlock_t lock;
-		struct v4l2_ctrl *ctrls[2];
+		struct {
+			struct v4l2_ctrl *vflip;
+			struct v4l2_ctrl *hflip;
+			struct v4l2_ctrl *rotate;
+		} ctrls;
 		unsigned int pending;
 		unsigned int active;
+		bool rotate;
 	} flip;
 
 	struct vsp1_rwpf_memory mem;
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 5239e08fabc3..795a3ca9ca03 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -187,9 +187,13 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 	struct vsp1_entity *entity;
 	unsigned int div_size;
 
+	/*
+	 * Partitions are computed on the size before rotation, use the format
+	 * at the WPF sink.
+	 */
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
 					    pipe->output->entity.config,
-					    RWPF_PAD_SOURCE);
+					    RWPF_PAD_SINK);
 	div_size = format->width;
 
 	/* Gen2 hardware doesn't require image partitioning. */
@@ -229,9 +233,13 @@ static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
 	struct v4l2_rect partition;
 	unsigned int modulus;
 
+	/*
+	 * Partitions are computed on the size before rotation, use the format
+	 * at the WPF sink.
+	 */
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
 					    pipe->output->entity.config,
-					    RWPF_PAD_SOURCE);
+					    RWPF_PAD_SINK);
 
 	/* A single partition simply processes the output size in full. */
 	if (pipe->partitions <= 1) {
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 25a2ed6e2e18..32df109b119f 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -43,32 +43,90 @@ static inline void vsp1_wpf_write(struct vsp1_rwpf *wpf,
 enum wpf_flip_ctrl {
 	WPF_CTRL_VFLIP = 0,
 	WPF_CTRL_HFLIP = 1,
-	WPF_CTRL_MAX,
 };
 
+static int vsp1_wpf_set_rotation(struct vsp1_rwpf *wpf, unsigned int rotation)
+{
+	struct vsp1_video *video = wpf->video;
+	struct v4l2_mbus_framefmt *sink_format;
+	struct v4l2_mbus_framefmt *source_format;
+	bool rotate;
+	int ret = 0;
+
+	/*
+	 * Only consider the 0°/180° from/to 90°/270° modifications, the rest
+	 * is taken care of by the flipping configuration.
+	 */
+	rotate = rotation == 90 || rotation == 270;
+	if (rotate == wpf->flip.rotate)
+		return 0;
+
+	/* Changing rotation isn't allowed when buffers are allocated. */
+	mutex_lock(&video->lock);
+
+	if (vb2_is_busy(&video->queue)) {
+		ret = -EBUSY;
+		goto done;
+	}
+
+	sink_format = vsp1_entity_get_pad_format(&wpf->entity,
+						 wpf->entity.config,
+						 RWPF_PAD_SINK);
+	source_format = vsp1_entity_get_pad_format(&wpf->entity,
+						   wpf->entity.config,
+						   RWPF_PAD_SOURCE);
+
+	mutex_lock(&wpf->entity.lock);
+
+	if (rotate) {
+		source_format->width = sink_format->height;
+		source_format->height = sink_format->width;
+	} else {
+		source_format->width = sink_format->width;
+		source_format->height = sink_format->height;
+	}
+
+	wpf->flip.rotate = rotate;
+
+	mutex_unlock(&wpf->entity.lock);
+
+done:
+	mutex_unlock(&video->lock);
+	return ret;
+}
+
 static int vsp1_wpf_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct vsp1_rwpf *wpf =
 		container_of(ctrl->handler, struct vsp1_rwpf, ctrls);
-	unsigned int i;
+	unsigned int rotation;
 	u32 flip = 0;
+	int ret;
 
-	switch (ctrl->id) {
-	case V4L2_CID_HFLIP:
-	case V4L2_CID_VFLIP:
-		for (i = 0; i < WPF_CTRL_MAX; ++i) {
-			if (wpf->flip.ctrls[i])
-				flip |= wpf->flip.ctrls[i]->val ? BIT(i) : 0;
-		}
+	/* Update the rotation. */
+	rotation = wpf->flip.ctrls.rotate ? wpf->flip.ctrls.rotate->val : 0;
+	ret = vsp1_wpf_set_rotation(wpf, rotation);
+	if (ret < 0)
+		return ret;
 
-		spin_lock_irq(&wpf->flip.lock);
-		wpf->flip.pending = flip;
-		spin_unlock_irq(&wpf->flip.lock);
-		break;
+	/*
+	 * Compute the flip value resulting from all three controls, with
+	 * rotation by 180° flipping the image in both directions. Store the
+	 * result in the pending flip field for the next frame that will be
+	 * processed.
+	 */
+	if (wpf->flip.ctrls.vflip->val)
+		flip |= BIT(WPF_CTRL_VFLIP);
 
-	default:
-		return -EINVAL;
-	}
+	if (wpf->flip.ctrls.hflip && wpf->flip.ctrls.hflip->val)
+		flip |= BIT(WPF_CTRL_HFLIP);
+
+	if (rotation == 180 || rotation == 270)
+		flip ^= BIT(WPF_CTRL_VFLIP) | BIT(WPF_CTRL_HFLIP);
+
+	spin_lock_irq(&wpf->flip.lock);
+	wpf->flip.pending = flip;
+	spin_unlock_irq(&wpf->flip.lock);
 
 	return 0;
 }
@@ -89,10 +147,10 @@ static int wpf_init_controls(struct vsp1_rwpf *wpf)
 		num_flip_ctrls = 0;
 	} else if (vsp1->info->features & VSP1_HAS_WPF_HFLIP) {
 		/*
-		 * When horizontal flip is supported the WPF implements two
-		 * controls (horizontal flip and vertical flip).
+		 * When horizontal flip is supported the WPF implements three
+		 * controls (horizontal flip, vertical flip and rotation).
 		 */
-		num_flip_ctrls = 2;
+		num_flip_ctrls = 3;
 	} else if (vsp1->info->features & VSP1_HAS_WPF_VFLIP) {
 		/*
 		 * When only vertical flip is supported the WPF implements a
@@ -107,17 +165,19 @@ static int wpf_init_controls(struct vsp1_rwpf *wpf)
 	vsp1_rwpf_init_ctrls(wpf, num_flip_ctrls);
 
 	if (num_flip_ctrls >= 1) {
-		wpf->flip.ctrls[WPF_CTRL_VFLIP] =
+		wpf->flip.ctrls.vflip =
 			v4l2_ctrl_new_std(&wpf->ctrls, &vsp1_wpf_ctrl_ops,
 					  V4L2_CID_VFLIP, 0, 1, 1, 0);
 	}
 
-	if (num_flip_ctrls == 2) {
-		wpf->flip.ctrls[WPF_CTRL_HFLIP] =
+	if (num_flip_ctrls == 3) {
+		wpf->flip.ctrls.hflip =
 			v4l2_ctrl_new_std(&wpf->ctrls, &vsp1_wpf_ctrl_ops,
 					  V4L2_CID_HFLIP, 0, 1, 1, 0);
-
-		v4l2_ctrl_cluster(2, wpf->flip.ctrls);
+		wpf->flip.ctrls.rotate =
+			v4l2_ctrl_new_std(&wpf->ctrls, &vsp1_wpf_ctrl_ops,
+					  V4L2_CID_ROTATE, 0, 270, 90, 0);
+		v4l2_ctrl_cluster(3, &wpf->flip.ctrls.vflip);
 	}
 
 	if (wpf->ctrls.error) {
@@ -222,8 +282,8 @@ static void wpf_configure(struct vsp1_entity *entity,
 		const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
 		struct vsp1_rwpf_memory mem = wpf->mem;
 		unsigned int flip = wpf->flip.active;
-		unsigned int width = source_format->width;
-		unsigned int height = source_format->height;
+		unsigned int width = sink_format->width;
+		unsigned int height = sink_format->height;
 		unsigned int offset;
 
 		/*
@@ -246,45 +306,78 @@ static void wpf_configure(struct vsp1_entity *entity,
 		/*
 		 * Update the memory offsets based on flipping configuration.
 		 * The destination addresses point to the locations where the
-		 * VSP starts writing to memory, which can be different corners
-		 * of the image depending on vertical flipping.
+		 * VSP starts writing to memory, which can be any corner of the
+		 * image depending on the combination of flipping and rotation.
 		 */
-		if (pipe->partitions > 1) {
-			const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
 
-			/*
-			 * Horizontal flipping is handled through a line buffer
-			 * and doesn't modify the start address, but still needs
-			 * to be handled when image partitioning is in effect to
-			 * order the partitions correctly.
-			 */
-			if (flip & BIT(WPF_CTRL_HFLIP))
-				offset = format->width - pipe->partition.left
-					- pipe->partition.width;
+		/*
+		 * First take the partition left coordinate into account.
+		 * Compute the offset to order the partitions correctly on the
+		 * output based on whether flipping is enabled. Consider
+		 * horizontal flipping when rotation is disabled but vertical
+		 * flipping when rotation is enabled, as rotating the image
+		 * switches the horizontal and vertical directions. The offset
+		 * is applied horizontally or vertically accordingly.
+		 */
+		if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
+			offset = format->width - pipe->partition.left
+				- pipe->partition.width;
+		else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
+			offset = format->height - pipe->partition.left
+				- pipe->partition.width;
+		else
+			offset = pipe->partition.left;
+
+		for (i = 0; i < format->num_planes; ++i) {
+			unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
+			unsigned int vsub = i > 0 ? fmtinfo->vsub : 1;
+
+			if (wpf->flip.rotate)
+				mem.addr[i] += offset / vsub
+					     * format->plane_fmt[i].bytesperline;
 			else
-				offset = pipe->partition.left;
-
-			mem.addr[0] += offset * fmtinfo->bpp[0] / 8;
-			if (format->num_planes > 1) {
-				mem.addr[1] += offset / fmtinfo->hsub
-					     * fmtinfo->bpp[1] / 8;
-				mem.addr[2] += offset / fmtinfo->hsub
-					     * fmtinfo->bpp[2] / 8;
-			}
+				mem.addr[i] += offset / hsub
+					     * fmtinfo->bpp[i] / 8;
 		}
 
 		if (flip & BIT(WPF_CTRL_VFLIP)) {
-			mem.addr[0] += (format->height - 1)
+			/*
+			 * When rotating the output (after rotation) image
+			 * height is equal to the partition width (before
+			 * rotation). Otherwise it is equal to the output
+			 * image height.
+			 */
+			if (wpf->flip.rotate)
+				height = pipe->partition.width;
+			else
+				height = format->height;
+
+			mem.addr[0] += (height - 1)
 				     * format->plane_fmt[0].bytesperline;
 
 			if (format->num_planes > 1) {
-				offset = (format->height / wpf->fmtinfo->vsub - 1)
+				offset = (height / fmtinfo->vsub - 1)
 				       * format->plane_fmt[1].bytesperline;
 				mem.addr[1] += offset;
 				mem.addr[2] += offset;
 			}
 		}
 
+		if (wpf->flip.rotate && !(flip & BIT(WPF_CTRL_HFLIP))) {
+			unsigned int hoffset = max(0, (int)format->width - 16);
+
+			/*
+			 * Compute the output coordinate. The partition
+			 * horizontal (left) offset becomes a vertical offset.
+			 */
+			for (i = 0; i < format->num_planes; ++i) {
+				unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
+
+				mem.addr[i] += hoffset / hsub
+					     * fmtinfo->bpp[i] / 8;
+			}
+		}
+
 		/*
 		 * On Gen3 hardware the SPUVS bit has no effect on 3-planar
 		 * formats. Swap the U and V planes manually in that case.
@@ -306,6 +399,9 @@ static void wpf_configure(struct vsp1_entity *entity,
 
 		outfmt = fmtinfo->hwfmt << VI6_WPF_OUTFMT_WRFMT_SHIFT;
 
+		if (wpf->flip.rotate)
+			outfmt |= VI6_WPF_OUTFMT_ROT;
+
 		if (fmtinfo->alpha)
 			outfmt |= VI6_WPF_OUTFMT_PXA;
 		if (fmtinfo->swap_yc)
@@ -367,9 +463,18 @@ static void wpf_configure(struct vsp1_entity *entity,
 			   VI6_WFP_IRQ_ENB_DFEE);
 }
 
+static unsigned int wpf_max_width(struct vsp1_entity *entity,
+				  struct vsp1_pipeline *pipe)
+{
+	struct vsp1_rwpf *wpf = to_rwpf(&entity->subdev);
+
+	return wpf->flip.rotate ? 256 : wpf->max_width;
+}
+
 static const struct vsp1_entity_operations wpf_entity_ops = {
 	.destroy = vsp1_wpf_destroy,
 	.configure = wpf_configure,
+	.max_width = wpf_max_width,
 };
 
 /* -----------------------------------------------------------------------------
-- 
Regards,

Laurent Pinchart