[RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Hi Alexandre,

This is a first implementation of the request API in the
control framework. It is fairly simplistic at the moment in that
it just clones all the control values (so no refcounting yet for
values as Laurent proposed, I will work on that later). But this
should not be a problem for codecs since there aren't all that many
controls involved.

The API is as follows:

struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);

This allocates a struct v4l2_ctrl_handler that is empty (i.e. has
no controls) but is refcounted and is marked as representing a
request.

int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
                            const struct v4l2_ctrl_handler *from,
                            bool (*filter)(const struct v4l2_ctrl *ctrl));

Delete any existing controls in handler 'hdl', then clone the values
from an existing handler 'from' into 'hdl'. If 'from' == NULL, then
this just clears the handler. 'from' can either be another request
control handler or a regular control handler in which case the
current values are cloned. If 'filter' != NULL then you can
filter which controls you want to clone.

void v4l2_ctrl_request_get(struct v4l2_ctrl_handler *hdl);

Increase the refcount.

void v4l2_ctrl_request_put(struct v4l2_ctrl_handler *hdl);

Decrease the refcount and delete hdl if it reaches 0.

void v4l2_ctrl_request_setup(struct v4l2_ctrl_handler *hdl);

Apply the values from the handler (i.e. request object) to the
hardware.

You will have to modify v4l_g/s/try_ext_ctrls in v4l2-ioctls.c to
obtain the request v4l2_ctrl_handler pointer based on the request
field in struct v4l2_ext_controls.

The first patch in this series is necessary to avoid cloning
controls that belong to other devices (as opposed to the subdev
or bridge device for which you make a request). It can probably
be dropped for codecs, but it is needed for MC devices like
omap3isp.

This series has only been compile tested! So if it crashes as
soon as you try to use it, then that's why :-)

Note: I'm not sure if it makes sense to refcount the control
handler, you might prefer to have a refcount in a higher-level
request struct. If that's the case, then I can drop the _get
function and replace the _put function by a v4l2_ctrl_request_free()
function.

Good luck!

	Hans

Hans Verkuil (6):
  v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev
  v4l2-ctrls: prepare internal structs for request API
  v4l2-ctrls: add core request API
  v4l2-ctrls: use ref in helper instead of ctrl
  v4l2-ctrls: support g/s_ext_ctrls for requests
  v4l2-ctrls: add v4l2_ctrl_request_setup

 drivers/media/dvb-frontends/rtl2832_sdr.c        |   5 +-
 drivers/media/pci/bt8xx/bttv-driver.c            |   2 +-
 drivers/media/pci/cx23885/cx23885-417.c          |   2 +-
 drivers/media/pci/cx88/cx88-blackbird.c          |   2 +-
 drivers/media/pci/cx88/cx88-video.c              |   2 +-
 drivers/media/pci/saa7134/saa7134-empress.c      |   4 +-
 drivers/media/pci/saa7134/saa7134-video.c        |   2 +-
 drivers/media/platform/exynos4-is/fimc-capture.c |   2 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c      |   3 +-
 drivers/media/platform/rcar_drif.c               |   2 +-
 drivers/media/platform/soc_camera/soc_camera.c   |   3 +-
 drivers/media/platform/vivid/vivid-ctrls.c       |  42 ++--
 drivers/media/usb/cx231xx/cx231xx-417.c          |   2 +-
 drivers/media/usb/cx231xx/cx231xx-video.c        |   4 +-
 drivers/media/usb/msi2500/msi2500.c              |   2 +-
 drivers/media/usb/tm6000/tm6000-video.c          |   2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c             | 242 +++++++++++++++++++++--
 drivers/media/v4l2-core/v4l2-device.c            |   3 +-
 drivers/staging/media/imx/imx-media-dev.c        |   2 +-
 drivers/staging/media/imx/imx-media-fim.c        |   2 +-
 include/media/v4l2-ctrls.h                       |  17 +-
 21 files changed, 287 insertions(+), 60 deletions(-)

-- 
2.14.1

[RFCv1 PATCH 1/6] v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add a 'bool from_other_dev' argument: set to true if the two
handlers refer to different devices (e.g. it is true when
inheriting controls from a subdev into a main v4l2 bridge
driver).

This will be used later when implementing support for the
request API since we need to skip such controls.

TODO: check drivers/staging/media/imx/imx-media-fim.c change.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/dvb-frontends/rtl2832_sdr.c        |  5 +--
 drivers/media/pci/bt8xx/bttv-driver.c            |  2 +-
 drivers/media/pci/cx23885/cx23885-417.c          |  2 +-
 drivers/media/pci/cx88/cx88-blackbird.c          |  2 +-
 drivers/media/pci/cx88/cx88-video.c              |  2 +-
 drivers/media/pci/saa7134/saa7134-empress.c      |  4 +--
 drivers/media/pci/saa7134/saa7134-video.c        |  2 +-
 drivers/media/platform/exynos4-is/fimc-capture.c |  2 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c      |  3 +-
 drivers/media/platform/rcar_drif.c               |  2 +-
 drivers/media/platform/soc_camera/soc_camera.c   |  3 +-
 drivers/media/platform/vivid/vivid-ctrls.c       | 42 ++++++++++++------------
 drivers/media/usb/cx231xx/cx231xx-417.c          |  2 +-
 drivers/media/usb/cx231xx/cx231xx-video.c        |  4 +--
 drivers/media/usb/msi2500/msi2500.c              |  2 +-
 drivers/media/usb/tm6000/tm6000-video.c          |  2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c             | 11 ++++---
 drivers/media/v4l2-core/v4l2-device.c            |  3 +-
 drivers/staging/media/imx/imx-media-dev.c        |  2 +-
 drivers/staging/media/imx/imx-media-fim.c        |  2 +-
 include/media/v4l2-ctrls.h                       |  4 ++-
 21 files changed, 56 insertions(+), 47 deletions(-)

diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
index c6e78d870ccd..6064d28224e8 100644
--- a/drivers/media/dvb-frontends/rtl2832_sdr.c
+++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
@@ -1394,7 +1394,8 @@ static int rtl2832_sdr_probe(struct platform_device *pdev)
 	case RTL2832_SDR_TUNER_E4000:
 		v4l2_ctrl_handler_init(&dev->hdl, 9);
 		if (subdev)
-			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler, NULL);
+			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler,
+					      NULL, true);
 		break;
 	case RTL2832_SDR_TUNER_R820T:
 	case RTL2832_SDR_TUNER_R828D:
@@ -1423,7 +1424,7 @@ static int rtl2832_sdr_probe(struct platform_device *pdev)
 		v4l2_ctrl_handler_init(&dev->hdl, 2);
 		if (subdev)
 			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler,
-					      NULL);
+					      NULL, true);
 		break;
 	default:
 		v4l2_ctrl_handler_init(&dev->hdl, 0);
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index b366a7e1d976..91874f775d37 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -4211,7 +4211,7 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
 	/* register video4linux + input */
 	if (!bttv_tvcards[btv->c.type].no_video) {
 		v4l2_ctrl_add_handler(&btv->radio_ctrl_handler, hdl,
-				v4l2_ctrl_radio_filter);
+				v4l2_ctrl_radio_filter, false);
 		if (btv->radio_ctrl_handler.error) {
 			result = btv->radio_ctrl_handler.error;
 			goto fail2;
diff --git a/drivers/media/pci/cx23885/cx23885-417.c b/drivers/media/pci/cx23885/cx23885-417.c
index a71f3c7569ce..762823871c78 100644
--- a/drivers/media/pci/cx23885/cx23885-417.c
+++ b/drivers/media/pci/cx23885/cx23885-417.c
@@ -1527,7 +1527,7 @@ int cx23885_417_register(struct cx23885_dev *dev)
 	dev->cxhdl.priv = dev;
 	dev->cxhdl.func = cx23885_api_func;
 	cx2341x_handler_set_50hz(&dev->cxhdl, tsport->height == 576);
-	v4l2_ctrl_add_handler(&dev->ctrl_handler, &dev->cxhdl.hdl, NULL);
+	v4l2_ctrl_add_handler(&dev->ctrl_handler, &dev->cxhdl.hdl, NULL, false);
 
 	/* Allocate and initialize V4L video device */
 	dev->v4l_device = cx23885_video_dev_alloc(tsport,
diff --git a/drivers/media/pci/cx88/cx88-blackbird.c b/drivers/media/pci/cx88/cx88-blackbird.c
index e3101f04941c..8424fb0da90c 100644
--- a/drivers/media/pci/cx88/cx88-blackbird.c
+++ b/drivers/media/pci/cx88/cx88-blackbird.c
@@ -1184,7 +1184,7 @@ static int cx8802_blackbird_probe(struct cx8802_driver *drv)
 	err = cx2341x_handler_init(&dev->cxhdl, 36);
 	if (err)
 		goto fail_core;
-	v4l2_ctrl_add_handler(&dev->cxhdl.hdl, &core->video_hdl, NULL);
+	v4l2_ctrl_add_handler(&dev->cxhdl.hdl, &core->video_hdl, NULL, false);
 
 	/* blackbird stuff */
 	pr_info("cx23416 based mpeg encoder (blackbird reference design)\n");
diff --git a/drivers/media/pci/cx88/cx88-video.c b/drivers/media/pci/cx88/cx88-video.c
index 7d25ecd4404b..fc52d80b7472 100644
--- a/drivers/media/pci/cx88/cx88-video.c
+++ b/drivers/media/pci/cx88/cx88-video.c
@@ -1376,7 +1376,7 @@ static int cx8800_initdev(struct pci_dev *pci_dev,
 		if (vc->id == V4L2_CID_CHROMA_AGC)
 			core->chroma_agc = vc;
 	}
-	v4l2_ctrl_add_handler(&core->video_hdl, &core->audio_hdl, NULL);
+	v4l2_ctrl_add_handler(&core->video_hdl, &core->audio_hdl, NULL, false);
 
 	/* load and configure helper modules */
 
diff --git a/drivers/media/pci/saa7134/saa7134-empress.c b/drivers/media/pci/saa7134/saa7134-empress.c
index 66acfd35ffc6..fc75ce00dbf8 100644
--- a/drivers/media/pci/saa7134/saa7134-empress.c
+++ b/drivers/media/pci/saa7134/saa7134-empress.c
@@ -265,9 +265,9 @@ static int empress_init(struct saa7134_dev *dev)
 		 "%s empress (%s)", dev->name,
 		 saa7134_boards[dev->board].name);
 	v4l2_ctrl_handler_init(hdl, 21);
-	v4l2_ctrl_add_handler(hdl, &dev->ctrl_handler, empress_ctrl_filter);
+	v4l2_ctrl_add_handler(hdl, &dev->ctrl_handler, empress_ctrl_filter, false);
 	if (dev->empress_sd)
-		v4l2_ctrl_add_handler(hdl, dev->empress_sd->ctrl_handler, NULL);
+		v4l2_ctrl_add_handler(hdl, dev->empress_sd->ctrl_handler, NULL, true);
 	if (hdl->error) {
 		video_device_release(dev->empress_dev);
 		return hdl->error;
diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index 82d2a24644e4..509d1e1b1942 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2134,7 +2134,7 @@ int saa7134_video_init1(struct saa7134_dev *dev)
 		hdl = &dev->radio_ctrl_handler;
 		v4l2_ctrl_handler_init(hdl, 2);
 		v4l2_ctrl_add_handler(hdl, &dev->ctrl_handler,
-				v4l2_ctrl_radio_filter);
+				v4l2_ctrl_radio_filter, false);
 		if (hdl->error)
 			return hdl->error;
 	}
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 948fe01f6c96..8280f94400c7 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1418,7 +1418,7 @@ static int fimc_link_setup(struct media_entity *entity,
 		return 0;
 
 	return v4l2_ctrl_add_handler(&vc->ctx->ctrls.handler,
-				     sensor->ctrl_handler, NULL);
+				     sensor->ctrl_handler, NULL, true);
 }
 
 static const struct media_entity_operations fimc_sd_media_ops = {
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b479b882da12..90246113fa03 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -900,7 +900,8 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	if (ret < 0)
 		return ret;
 
-	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
+	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler,
+				    NULL, true);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 63c94f4028a7..760aa307c17b 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1167,7 +1167,7 @@ static int rcar_drif_notify_complete(struct v4l2_async_notifier *notifier)
 	}
 
 	ret = v4l2_ctrl_add_handler(&sdr->ctrl_hdl,
-				    sdr->ep.subdev->ctrl_handler, NULL);
+				    sdr->ep.subdev->ctrl_handler, NULL, true);
 	if (ret) {
 		rdrif_err(sdr, "failed: ctrl add hdlr ret %d\n", ret);
 		goto error;
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 916ff68b73d4..ed81bb2677e4 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1180,7 +1180,8 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 
 	v4l2_subdev_call(sd, video, g_tvnorms, &icd->vdev->tvnorms);
 
-	ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler, NULL);
+	ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler,
+				    NULL, true);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
index 34731f71cc00..d9bc00d3328a 100644
--- a/drivers/media/platform/vivid/vivid-ctrls.c
+++ b/drivers/media/platform/vivid/vivid-ctrls.c
@@ -1652,57 +1652,57 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 		v4l2_ctrl_auto_cluster(2, &dev->autogain, 0, true);
 
 	if (dev->has_vid_cap) {
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_vid, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_aud, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_streaming, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_sdtv_cap, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_loop_cap, NULL);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_vid, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_aud, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_streaming, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_sdtv_cap, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_loop_cap, NULL, false);
 		if (hdl_vid_cap->error)
 			return hdl_vid_cap->error;
 		dev->vid_cap_dev.ctrl_handler = hdl_vid_cap;
 	}
 	if (dev->has_vid_out) {
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_aud, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_streaming, NULL);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_aud, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_streaming, NULL, false);
 		if (hdl_vid_out->error)
 			return hdl_vid_out->error;
 		dev->vid_out_dev.ctrl_handler = hdl_vid_out;
 	}
 	if (dev->has_vbi_cap) {
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_streaming, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_sdtv_cap, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_loop_cap, NULL);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_streaming, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_sdtv_cap, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_loop_cap, NULL, false);
 		if (hdl_vbi_cap->error)
 			return hdl_vbi_cap->error;
 		dev->vbi_cap_dev.ctrl_handler = hdl_vbi_cap;
 	}
 	if (dev->has_vbi_out) {
-		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_streaming, NULL);
+		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_streaming, NULL, false);
 		if (hdl_vbi_out->error)
 			return hdl_vbi_out->error;
 		dev->vbi_out_dev.ctrl_handler = hdl_vbi_out;
 	}
 	if (dev->has_radio_rx) {
-		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_aud, NULL);
+		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_aud, NULL, false);
 		if (hdl_radio_rx->error)
 			return hdl_radio_rx->error;
 		dev->radio_rx_dev.ctrl_handler = hdl_radio_rx;
 	}
 	if (dev->has_radio_tx) {
-		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_aud, NULL);
+		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_aud, NULL, false);
 		if (hdl_radio_tx->error)
 			return hdl_radio_tx->error;
 		dev->radio_tx_dev.ctrl_handler = hdl_radio_tx;
 	}
 	if (dev->has_sdr_cap) {
-		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_streaming, NULL);
+		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_streaming, NULL, false);
 		if (hdl_sdr_cap->error)
 			return hdl_sdr_cap->error;
 		dev->sdr_cap_dev.ctrl_handler = hdl_sdr_cap;
diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c
index d538fa407742..ba8a4072633a 100644
--- a/drivers/media/usb/cx231xx/cx231xx-417.c
+++ b/drivers/media/usb/cx231xx/cx231xx-417.c
@@ -1991,7 +1991,7 @@ int cx231xx_417_register(struct cx231xx *dev)
 	dev->mpeg_ctrl_handler.ops = &cx231xx_ops;
 	if (dev->sd_cx25840)
 		v4l2_ctrl_add_handler(&dev->mpeg_ctrl_handler.hdl,
-				dev->sd_cx25840->ctrl_handler, NULL);
+				dev->sd_cx25840->ctrl_handler, NULL, false);
 	if (dev->mpeg_ctrl_handler.hdl.error) {
 		err = dev->mpeg_ctrl_handler.hdl.error;
 		dprintk(3, "%s: can't add cx25840 controls\n", dev->name);
diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
index 226059fc672b..3d955b0fdbef 100644
--- a/drivers/media/usb/cx231xx/cx231xx-video.c
+++ b/drivers/media/usb/cx231xx/cx231xx-video.c
@@ -2202,10 +2202,10 @@ int cx231xx_register_analog_devices(struct cx231xx *dev)
 
 	if (dev->sd_cx25840) {
 		v4l2_ctrl_add_handler(&dev->ctrl_handler,
-				dev->sd_cx25840->ctrl_handler, NULL);
+				dev->sd_cx25840->ctrl_handler, NULL, true);
 		v4l2_ctrl_add_handler(&dev->radio_ctrl_handler,
 				dev->sd_cx25840->ctrl_handler,
-				v4l2_ctrl_radio_filter);
+				v4l2_ctrl_radio_filter, true);
 	}
 
 	if (dev->ctrl_handler.error)
diff --git a/drivers/media/usb/msi2500/msi2500.c b/drivers/media/usb/msi2500/msi2500.c
index 65ef755adfdc..4aacd77a5d58 100644
--- a/drivers/media/usb/msi2500/msi2500.c
+++ b/drivers/media/usb/msi2500/msi2500.c
@@ -1278,7 +1278,7 @@ static int msi2500_probe(struct usb_interface *intf,
 	}
 
 	/* currently all controls are from subdev */
-	v4l2_ctrl_add_handler(&dev->hdl, sd->ctrl_handler, NULL);
+	v4l2_ctrl_add_handler(&dev->hdl, sd->ctrl_handler, NULL, true);
 
 	dev->v4l2_dev.ctrl_handler = &dev->hdl;
 	dev->vdev.v4l2_dev = &dev->v4l2_dev;
diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
index 9fa25de6b5a9..45d27a2a3130 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -1631,7 +1631,7 @@ int tm6000_v4l2_register(struct tm6000_core *dev)
 	v4l2_ctrl_new_std(&dev->ctrl_handler, &tm6000_ctrl_ops,
 			V4L2_CID_HUE, -128, 127, 1, 0);
 	v4l2_ctrl_add_handler(&dev->ctrl_handler,
-			&dev->radio_ctrl_handler, NULL);
+			&dev->radio_ctrl_handler, NULL, false);
 
 	if (dev->radio_ctrl_handler.error)
 		ret = dev->radio_ctrl_handler.error;
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index cbb2ef43945f..2e58381444d1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1876,7 +1876,8 @@ EXPORT_SYMBOL(v4l2_ctrl_find);
 
 /* Allocate a new v4l2_ctrl_ref and hook it into the handler. */
 static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
-			   struct v4l2_ctrl *ctrl)
+			   struct v4l2_ctrl *ctrl,
+			   bool from_other_dev)
 {
 	struct v4l2_ctrl_ref *ref;
 	struct v4l2_ctrl_ref *new_ref;
@@ -1900,6 +1901,7 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	if (!new_ref)
 		return handler_set_err(hdl, -ENOMEM);
 	new_ref->ctrl = ctrl;
+	new_ref->from_other_dev = from_other_dev;
 	if (ctrl->handler == hdl) {
 		/* By default each control starts in a cluster of its own.
 		   new_ref->ctrl is basically a cluster array with one
@@ -2080,7 +2082,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
 	}
 
-	if (handler_new_ref(hdl, ctrl)) {
+	if (handler_new_ref(hdl, ctrl, false)) {
 		kvfree(ctrl);
 		return NULL;
 	}
@@ -2249,7 +2251,8 @@ EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 /* Add the controls from another handler to our own. */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 			  struct v4l2_ctrl_handler *add,
-			  bool (*filter)(const struct v4l2_ctrl *ctrl))
+			  bool (*filter)(const struct v4l2_ctrl *ctrl),
+			  bool from_other_dev)
 {
 	struct v4l2_ctrl_ref *ref;
 	int ret = 0;
@@ -2272,7 +2275,7 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 		/* Filter any unwanted controls */
 		if (filter && !filter(ctrl))
 			continue;
-		ret = handler_new_ref(hdl, ctrl);
+		ret = handler_new_ref(hdl, ctrl, from_other_dev);
 		if (ret)
 			break;
 	}
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 937c6de85606..8391a7f0895b 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -178,7 +178,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 
 	sd->v4l2_dev = v4l2_dev;
 	/* This just returns 0 if either of the two args is NULL */
-	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL);
+	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler,
+				    NULL, true);
 	if (err)
 		goto error_module;
 
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 47c4c954fed5..0a8eda5f455b 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -464,7 +464,7 @@ static int imx_media_inherit_controls(struct imx_media_dev *imxmd,
 
 		ret = v4l2_ctrl_add_handler(vfd->ctrl_handler,
 					    sd->ctrl_handler,
-					    NULL);
+					    NULL, true);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c
index 47275ef803f3..8372acbaf042 100644
--- a/drivers/staging/media/imx/imx-media-fim.c
+++ b/drivers/staging/media/imx/imx-media-fim.c
@@ -459,7 +459,7 @@ int imx_media_fim_add_controls(struct imx_media_fim *fim)
 {
 	/* add the FIM controls to the calling subdev ctrl handler */
 	return v4l2_ctrl_add_handler(fim->sd->ctrl_handler,
-				     &fim->ctrl_handler, NULL);
+				     &fim->ctrl_handler, NULL, false);
 }
 EXPORT_SYMBOL_GPL(imx_media_fim_add_controls);
 
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index dacfe54057f8..a762f3392d90 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -250,6 +250,7 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl_ref *next;
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_ctrl_helper *helper;
+	bool from_other_dev;
 };
 
 /**
@@ -635,7 +636,8 @@ typedef bool (*v4l2_ctrl_filter)(const struct v4l2_ctrl *ctrl);
  */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 			  struct v4l2_ctrl_handler *add,
-			  v4l2_ctrl_filter filter);
+			  v4l2_ctrl_filter filter,
+			  bool from_other_dev);
 
 /**
  * v4l2_ctrl_radio_filter() - Standard filter for radio controls.
-- 
2.14.1

[RFCv1 PATCH 2/6] v4l2-ctrls: prepare internal structs for request API

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add a refcount and is_request bool to struct v4l2_ctrl_handler:
this is used to refcount a handler that represents a request.

Add a p_req field to struct v4l2_ctrl_ref that will store the
request value.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 1 +
 include/media/v4l2-ctrls.h           | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 2e58381444d1..1ff8fc59fff5 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1761,6 +1761,7 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
 				      sizeof(hdl->buckets[0]),
 				      GFP_KERNEL | __GFP_ZERO);
 	hdl->error = hdl->buckets ? 0 : -ENOMEM;
+	hdl->is_request = false;
 	return hdl->error;
 }
 EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index a762f3392d90..a215f25a82cf 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -18,6 +18,7 @@
 #define _V4L2_CTRLS_H
 
 #include <linux/list.h>
+#include <linux/kref.h>
 #include <linux/mutex.h>
 #include <linux/videodev2.h>
 
@@ -250,6 +251,7 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl_ref *next;
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_ctrl_helper *helper;
+	union v4l2_ctrl_ptr p_req;
 	bool from_other_dev;
 };
 
@@ -285,7 +287,9 @@ struct v4l2_ctrl_handler {
 	v4l2_ctrl_notify_fnc notify;
 	void *notify_priv;
 	u16 nr_of_buckets;
+	bool is_request;
 	int error;
+	struct kref ref;
 };
 
 /**
-- 
2.14.1

[RFCv1 PATCH 3/6] v4l2-ctrls: add core request API

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add the four core request functions:

v4l2_ctrl_request_alloc() allocates a new (empty) request.
v4l2_ctrl_request_clone() resets a request based on another request
(or clears it if that request is NULL).
v4l2_ctrl_request_get(): increase refcount
v4l2_ctrl_request_put(): decrease refcount and delete if it reaches 0.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 110 ++++++++++++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           |   7 +++
 2 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1ff8fc59fff5..710a75a2e19d 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1878,6 +1878,7 @@ EXPORT_SYMBOL(v4l2_ctrl_find);
 /* Allocate a new v4l2_ctrl_ref and hook it into the handler. */
 static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 			   struct v4l2_ctrl *ctrl,
+			   struct v4l2_ctrl_ref **ctrl_ref,
 			   bool from_other_dev)
 {
 	struct v4l2_ctrl_ref *ref;
@@ -1885,6 +1886,10 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	u32 id = ctrl->id;
 	u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1;
 	int bucket = id % hdl->nr_of_buckets;	/* which bucket to use */
+	unsigned int sz_extra = 0;
+
+	if (ctrl_ref)
+		*ctrl_ref = NULL;
 
 	/*
 	 * Automatically add the control class if it is not yet present and
@@ -1898,11 +1903,16 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	if (hdl->error)
 		return hdl->error;
 
-	new_ref = kzalloc(sizeof(*new_ref), GFP_KERNEL);
+	if (hdl->is_request)
+		sz_extra = ctrl->elems * ctrl->elem_size;
+	new_ref = kzalloc(sizeof(*new_ref) + sz_extra, GFP_KERNEL);
 	if (!new_ref)
 		return handler_set_err(hdl, -ENOMEM);
 	new_ref->ctrl = ctrl;
 	new_ref->from_other_dev = from_other_dev;
+	if (sz_extra)
+		new_ref->p_req.p = &new_ref[1];
+
 	if (ctrl->handler == hdl) {
 		/* By default each control starts in a cluster of its own.
 		   new_ref->ctrl is basically a cluster array with one
@@ -1942,6 +1952,8 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	/* Insert the control node in the hash */
 	new_ref->next = hdl->buckets[bucket];
 	hdl->buckets[bucket] = new_ref;
+	if (ctrl_ref)
+		*ctrl_ref = new_ref;
 
 unlock:
 	mutex_unlock(hdl->lock);
@@ -2083,7 +2095,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
 	}
 
-	if (handler_new_ref(hdl, ctrl, false)) {
+	if (handler_new_ref(hdl, ctrl, NULL, false)) {
 		kvfree(ctrl);
 		return NULL;
 	}
@@ -2276,7 +2288,7 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 		/* Filter any unwanted controls */
 		if (filter && !filter(ctrl))
 			continue;
-		ret = handler_new_ref(hdl, ctrl, from_other_dev);
+		ret = handler_new_ref(hdl, ctrl, NULL, from_other_dev);
 		if (ret)
 			break;
 	}
@@ -2685,6 +2697,98 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm)
 }
 EXPORT_SYMBOL(v4l2_querymenu);
 
+struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void)
+{
+	struct v4l2_ctrl_handler *hdl = kzalloc(sizeof(*hdl), GFP_KERNEL);
+	int err;
+
+	if (!hdl)
+		return ERR_PTR(-ENOMEM);
+	err = v4l2_ctrl_handler_init(hdl, 0);
+	if (err) {
+		kfree(hdl);
+		return ERR_PTR(err);
+	}
+	hdl->is_request = true;
+	kref_init(&hdl->ref);
+	return hdl;
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_alloc);
+
+int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
+			    const struct v4l2_ctrl_handler *from,
+			    bool (*filter)(const struct v4l2_ctrl *ctrl))
+{
+	struct v4l2_ctrl_ref *ref;
+	int err;
+
+	if (WARN_ON(!hdl || hdl == from))
+		return -EINVAL;
+
+	if (hdl->error)
+		return hdl->error;
+
+	WARN_ON(hdl->lock != &hdl->_lock);
+	v4l2_ctrl_handler_free(hdl);
+	err = v4l2_ctrl_handler_init(hdl, (from->nr_of_buckets - 1) * 8);
+	hdl->is_request = true;
+	if (err)
+		return err;
+	if (!from)
+		return 0;
+
+	mutex_lock(from->lock);
+	list_for_each_entry(ref, &from->ctrl_refs, node) {
+		struct v4l2_ctrl *ctrl = ref->ctrl;
+		struct v4l2_ctrl_ref *new_ref;
+
+		/* Skip refs inherited from other devices */
+		if (ref->from_other_dev)
+			continue;
+		/* And buttons and control classes */
+		if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
+		    ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
+			continue;
+		/* Filter any unwanted controls */
+		if (filter && !filter(ctrl))
+			continue;
+		err = handler_new_ref(hdl, ctrl, &new_ref, false);
+		if (err)
+			break;
+		if (from->is_request)
+			ptr_to_ptr(ctrl, ref->p_req, new_ref->p_req);
+		else
+			ptr_to_ptr(ctrl, ctrl->p_cur, new_ref->p_req);
+	}
+	mutex_unlock(from->lock);
+	return err;
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_clone);
+
+void v4l2_ctrl_request_get(struct v4l2_ctrl_handler *hdl)
+{
+	if (WARN_ON(!hdl->is_request))
+		return;
+	kref_get(&hdl->ref);
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_get);
+
+static void v4l2_ctrl_request_release(struct kref *kref)
+{
+	struct v4l2_ctrl_handler *hdl =
+		container_of(kref, struct v4l2_ctrl_handler, ref);
+
+	v4l2_ctrl_handler_free(hdl);
+	kfree(hdl);
+}
+
+void v4l2_ctrl_request_put(struct v4l2_ctrl_handler *hdl)
+{
+	if (WARN_ON(!hdl->is_request))
+		return;
+	kref_put(&hdl->ref, v4l2_ctrl_request_release);
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_put);
 
 /* Some general notes on the atomic requirements of VIDIOC_G/TRY/S_EXT_CTRLS:
 
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index a215f25a82cf..f86d680880e1 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -1045,6 +1045,13 @@ int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
  */
 unsigned int v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait);
 
+struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
+int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
+			    const struct v4l2_ctrl_handler *from,
+			    bool (*filter)(const struct v4l2_ctrl *ctrl));
+void v4l2_ctrl_request_get(struct v4l2_ctrl_handler *hdl);
+void v4l2_ctrl_request_put(struct v4l2_ctrl_handler *hdl);
+
 /* Helpers for ioctl_ops */
 
 /**
-- 
2.14.1

[RFCv1 PATCH 4/6] v4l2-ctrls: use ref in helper instead of ctrl

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The next patch needs the reference to a control instead of the
control itself, so change struct v4l2_ctrl_helper accordingly.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 710a75a2e19d..f8de43032b78 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -37,8 +37,8 @@
 struct v4l2_ctrl_helper {
 	/* Pointer to the control reference of the master control */
 	struct v4l2_ctrl_ref *mref;
-	/* The control corresponding to the v4l2_ext_control ID field. */
-	struct v4l2_ctrl *ctrl;
+	/* The control ref corresponding to the v4l2_ext_control ID field. */
+	struct v4l2_ctrl_ref *ref;
 	/* v4l2_ext_control index of the next control belonging to the
 	   same cluster, or 0 if there isn't any. */
 	u32 next;
@@ -2860,6 +2860,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		ref = find_ref_lock(hdl, id);
 		if (ref == NULL)
 			return -EINVAL;
+		h->ref = ref;
 		ctrl = ref->ctrl;
 		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
 			return -EINVAL;
@@ -2882,7 +2883,6 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		}
 		/* Store the ref to the master control of the cluster */
 		h->mref = ref;
-		h->ctrl = ctrl;
 		/* Initially set next to 0, meaning that there is no other
 		   control in this helper array belonging to the same
 		   cluster */
@@ -2967,7 +2967,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 	cs->error_idx = cs->count;
 
 	for (i = 0; !ret && i < cs->count; i++)
-		if (helpers[i].ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
+		if (helpers[i].ref->ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
 			ret = -EACCES;
 
 	for (i = 0; !ret && i < cs->count; i++) {
@@ -3002,7 +3002,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 
 			do {
 				ret = ctrl_to_user(cs->controls + idx,
-						   helpers[idx].ctrl);
+						   helpers[idx].ref->ctrl);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
@@ -3141,7 +3141,7 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
 
 	cs->error_idx = cs->count;
 	for (i = 0; i < cs->count; i++) {
-		struct v4l2_ctrl *ctrl = helpers[i].ctrl;
+		struct v4l2_ctrl *ctrl = helpers[i].ref->ctrl;
 		union v4l2_ctrl_ptr p_new;
 
 		cs->error_idx = i;
@@ -3253,7 +3253,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			do {
 				/* Check if the auto control is part of the
 				   list, and remember the new value. */
-				if (helpers[tmp_idx].ctrl == master)
+				if (helpers[tmp_idx].ref->ctrl == master)
 					new_auto_val = cs->controls[tmp_idx].value;
 				tmp_idx = helpers[tmp_idx].next;
 			} while (tmp_idx);
@@ -3266,7 +3266,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		/* Copy the new caller-supplied control values.
 		   user_to_new() sets 'is_new' to 1. */
 		do {
-			struct v4l2_ctrl *ctrl = helpers[idx].ctrl;
+			struct v4l2_ctrl *ctrl = helpers[idx].ref->ctrl;
 
 			ret = user_to_new(cs->controls + idx, ctrl);
 			if (!ret && ctrl->is_ptr)
@@ -3282,7 +3282,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			idx = i;
 			do {
 				ret = new_to_user(cs->controls + idx,
-						helpers[idx].ctrl);
+						helpers[idx].ref->ctrl);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
-- 
2.14.1

[RFCv1 PATCH 5/6] v4l2-ctrls: support g/s_ext_ctrls for requests

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The v4l2_g/s_ext_ctrls functions now support control handlers that
represent requests.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 37 ++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index f8de43032b78..36b00ad2d5cb 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1528,6 +1528,13 @@ static int new_to_user(struct v4l2_ext_control *c,
 	return ptr_to_user(c, ctrl, ctrl->p_new);
 }
 
+/* Helper function: copy the request value back to the caller */
+static int req_to_user(struct v4l2_ext_control *c,
+		       struct v4l2_ctrl_ref *ref)
+{
+	return ptr_to_user(c, ref->ctrl, ref->p_req);
+}
+
 /* Helper function: copy the initial control value back to the caller */
 static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
 {
@@ -1647,6 +1654,14 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
 	ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
 }
 
+/* Copy the new value to the request value */
+static void new_to_req(struct v4l2_ctrl_ref *ref)
+{
+	if (!ref)
+		return;
+	ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
+}
+
 /* Return non-zero if one or more of the controls in the cluster has a new
    value that differs from the current value. */
 static int cluster_changed(struct v4l2_ctrl *master)
@@ -2975,7 +2990,8 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 				    struct v4l2_ctrl *ctrl);
 		struct v4l2_ctrl *master;
 
-		ctrl_to_user = def_value ? def_to_user : cur_to_user;
+		ctrl_to_user = def_value ? def_to_user :
+			       (hdl->is_request ? NULL : cur_to_user);
 
 		if (helpers[i].mref == NULL)
 			continue;
@@ -3001,8 +3017,12 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 			u32 idx = i;
 
 			do {
-				ret = ctrl_to_user(cs->controls + idx,
-						   helpers[idx].ref->ctrl);
+				if (ctrl_to_user)
+					ret = ctrl_to_user(cs->controls + idx,
+						helpers[idx].ref->ctrl);
+				else
+					ret = req_to_user(cs->controls + idx,
+						helpers[idx].ref);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
@@ -3275,7 +3295,16 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		} while (!ret && idx);
 
 		if (!ret)
-			ret = try_or_set_cluster(fh, master, set, 0);
+			ret = try_or_set_cluster(fh, master,
+						 !hdl->is_request && set, 0);
+		if (!ret && hdl->is_request && set) {
+			for (j = 0; j < master->ncontrols; j++) {
+				struct v4l2_ctrl_ref *ref =
+					find_ref(hdl, master->cluster[j]->id);
+
+				new_to_req(ref);
+			}
+		}
 
 		/* Copy the new values back to userspace. */
 		if (!ret) {
-- 
2.14.1

[RFCv1 PATCH 6/6] v4l2-ctrls: add v4l2_ctrl_request_setup

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add a helper function that can set controls from a request.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 71 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           |  2 +
 2 files changed, 73 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 36b00ad2d5cb..5513061b033d 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1662,6 +1662,14 @@ static void new_to_req(struct v4l2_ctrl_ref *ref)
 	ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
 }
 
+/* Copy the request value to the new value */
+static void req_to_new(struct v4l2_ctrl_ref *ref)
+{
+	if (!ref)
+		return;
+	ptr_to_ptr(ref->ctrl, ref->p_req, ref->ctrl->p_new);
+}
+
 /* Return non-zero if one or more of the controls in the cluster has a new
    value that differs from the current value. */
 static int cluster_changed(struct v4l2_ctrl *master)
@@ -3431,6 +3439,69 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
 }
 EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
 
+void v4l2_ctrl_request_setup(struct v4l2_ctrl_handler *hdl)
+{
+	struct v4l2_ctrl_ref *ref;
+
+	if (!hdl)
+		return;
+
+	mutex_lock(hdl->lock);
+
+	list_for_each_entry(ref, &hdl->ctrl_refs, node)
+		ref->done = false;
+
+	list_for_each_entry(ref, &hdl->ctrl_refs, node) {
+		struct v4l2_ctrl *ctrl = ref->ctrl;
+		struct v4l2_ctrl *master = ctrl->cluster[0];
+		int i;
+
+		/* Skip if this control was already handled by a cluster. */
+		/* Skip button controls and read-only controls. */
+		if (ref->done || ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
+		    (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY))
+			continue;
+
+		v4l2_ctrl_lock(master);
+		for (i = 0; i < master->ncontrols; i++) {
+			if (master->cluster[i]) {
+				struct v4l2_ctrl_ref *r =
+					find_ref(hdl, master->cluster[i]->id);
+
+				req_to_new(r);
+				master->cluster[i]->is_new = 1;
+				r->done = true;
+			}
+		}
+		/*
+		 * For volatile autoclusters that are currently in auto mode
+		 * we need to discover if it will be set to manual mode.
+		 * If so, then we have to copy the current volatile values
+		 * first since those will become the new manual values (which
+		 * may be overwritten by explicit new values from this set
+		 * of controls).
+		 */
+		if (master->is_auto && master->has_volatiles &&
+		    !is_cur_manual(master)) {
+			s32 new_auto_val = *master->p_new.p_s32;
+
+			/*
+			 * If the new value == the manual value, then copy
+			 * the current volatile values.
+			 */
+			if (new_auto_val == master->manual_mode_value)
+				update_from_auto_cluster(master);
+		}
+
+		try_or_set_cluster(NULL, master, true, 0);
+
+		v4l2_ctrl_unlock(master);
+	}
+
+	mutex_unlock(hdl->lock);
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_setup);
+
 void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv)
 {
 	if (ctrl == NULL)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f86d680880e1..2770bff1d6bd 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -252,6 +252,7 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_ctrl_helper *helper;
 	union v4l2_ctrl_ptr p_req;
+	bool done;
 	bool from_other_dev;
 };
 
@@ -1049,6 +1050,7 @@ struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
 int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
 			    const struct v4l2_ctrl_handler *from,
 			    bool (*filter)(const struct v4l2_ctrl *ctrl));
+void v4l2_ctrl_request_setup(struct v4l2_ctrl_handler *hdl);
 void v4l2_ctrl_request_get(struct v4l2_ctrl_handler *hdl);
 void v4l2_ctrl_request_put(struct v4l2_ctrl_handler *hdl);
 
-- 
2.14.1

Re: [RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[Alexandre Courbot]

Hi Hans!

Thanks for the patchset! It looks quite good at first sight, a few comments 
and
questions follow though.

On Monday, November 13, 2017 11:34:02 PM JST, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Hi Alexandre,
>
> This is a first implementation of the request API in the
> control framework. It is fairly simplistic at the moment in that
> it just clones all the control values (so no refcounting yet for
> values as Laurent proposed, I will work on that later). But this
> should not be a problem for codecs since there aren't all that many
> controls involved.

Regarding value refcounting, I think we can probably do without it if we 
parse
the requests queue when looking values up. It may be more practical (having 
a
kref for each v4l2_ctrl_ref in a request sounds overkill to me), and maybe 
also
more predictible since we would have less chance of having dangling old 
values.

> The API is as follows:
>
> struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
>
> This allocates a struct v4l2_ctrl_handler that is empty (i.e. has
> no controls) but is refcounted and is marked as representing a
> request.
>
> int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>                             const struct v4l2_ctrl_handler *from,
>                             bool (*filter)(const struct v4l2_ctrl *ctrl));
>
> Delete any existing controls in handler 'hdl', then clone the values
> from an existing handler 'from' into 'hdl'. If 'from' == NULL, then
> this just clears the handler. 'from' can either be another request
> control handler or a regular control handler in which case the
> current values are cloned. If 'filter' != NULL then you can
> filter which controls you want to clone.

One thing that seems to be missing is, what happens if you try to set a 
control
on an empty request? IIUC this would currently fail because find_ref() 
would
not be able to find the control. The value ref should probably be created 
in
that case so we can create requests with a handful of controls.

Also, if you clone a handler that is not a request, I understand that all
controls will be deduplicated, creating a full-state copy? That could be 
useful,
but since this is the only way to make the current code work, I hope that 
the
current impossibility to set a control on an empty request is a bug (or 
misunderstanding from my part).

>
> void v4l2_ctrl_request_get(struct v4l2_ctrl_handler *hdl);
>
> Increase the refcount.
>
> void v4l2_ctrl_request_put(struct v4l2_ctrl_handler *hdl);
>
> Decrease the refcount and delete hdl if it reaches 0.
>
> void v4l2_ctrl_request_setup(struct v4l2_ctrl_handler *hdl);
>
> Apply the values from the handler (i.e. request object) to the
> hardware.
>
> You will have to modify v4l_g/s/try_ext_ctrls in v4l2-ioctls.c to
> obtain the request v4l2_ctrl_handler pointer based on the request
> field in struct v4l2_ext_controls.
>
> The first patch in this series is necessary to avoid cloning
> controls that belong to other devices (as opposed to the subdev
> or bridge device for which you make a request). It can probably
> be dropped for codecs, but it is needed for MC devices like
> omap3isp.
>
> This series has only been compile tested! So if it crashes as
> soon as you try to use it, then that's why :-)
>
> Note: I'm not sure if it makes sense to refcount the control
> handler, you might prefer to have a refcount in a higher-level
> request struct. If that's the case, then I can drop the _get
> function and replace the _put function by a v4l2_ctrl_request_free()
> function.

That's exactly what I thought when I saw the refcounting. This is probably 
a
problem for later since we want to focus on codecs for now, but I think we 
will
ultimately want to manage refcounting outside of v4l2_ctrl_handler. Maybe a
higher-level request class of which the current control-framework based 
design
would be an implementation. I am thinking about IPs like the VSP1 which 
will
probably want to model the controls either in a different way, or at least 
to
add extra data beyond the controls.

All in all I think I can use this for codecs. I am still trying to shoehorn 
my
first version into the media stuff, and to nobody's surprise this is not 
that
easy. :P But the fact the control framework part is already mostly taken 
care
of greatly helps.

Thanks!
Alex.

Re: [RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[Hans Verkuil]

Hi Alexandre,

On 15/11/17 10:38, Alexandre Courbot wrote:
> Hi Hans!
> 
> Thanks for the patchset! It looks quite good at first sight, a few comments and
> questions follow though.
> 
> On Monday, November 13, 2017 11:34:02 PM JST, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Hi Alexandre,
>>
>> This is a first implementation of the request API in the
>> control framework. It is fairly simplistic at the moment in that
>> it just clones all the control values (so no refcounting yet for
>> values as Laurent proposed, I will work on that later). But this
>> should not be a problem for codecs since there aren't all that many
>> controls involved.
> 
> Regarding value refcounting, I think we can probably do without it if we parse
> the requests queue when looking values up. It may be more practical (having a
> kref for each v4l2_ctrl_ref in a request sounds overkill to me), and maybe also
> more predictible since we would have less chance of having dangling old values.
> 
>> The API is as follows:
>>
>> struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
>>
>> This allocates a struct v4l2_ctrl_handler that is empty (i.e. has
>> no controls) but is refcounted and is marked as representing a
>> request.
>>
>> int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>>                             const struct v4l2_ctrl_handler *from,
>>                             bool (*filter)(const struct v4l2_ctrl *ctrl));
>>
>> Delete any existing controls in handler 'hdl', then clone the values
>> from an existing handler 'from' into 'hdl'. If 'from' == NULL, then
>> this just clears the handler. 'from' can either be another request
>> control handler or a regular control handler in which case the
>> current values are cloned. If 'filter' != NULL then you can
>> filter which controls you want to clone.
> 
> One thing that seems to be missing is, what happens if you try to set a control
> on an empty request? IIUC this would currently fail because find_ref() would
> not be able to find the control. The value ref should probably be created in
> that case so we can create requests with a handful of controls.

Wasn't the intention that we never have an empty request but always clone?
I.e. in your code the _alloc call is always followed by a _clone call.

The reason I have a separate _alloc function is that you use that when you
want to create a new control handler ('new request'). If the user wants to reuse an
existing request, then _clone can be called directly on the existing handler.

> Also, if you clone a handler that is not a request, I understand that all
> controls will be deduplicated, creating a full-state copy? That could be useful,
> but since this is the only way to make the current code work, I hope that the
> current impossibility to set a control on an empty request is a bug (or misunderstanding from my part).

I think it is a misunderstanding. Seen from userspace you'll never have an empty
request.

> 
>>
>> void v4l2_ctrl_request_get(struct v4l2_ctrl_handler *hdl);
>>
>> Increase the refcount.
>>
>> void v4l2_ctrl_request_put(struct v4l2_ctrl_handler *hdl);
>>
>> Decrease the refcount and delete hdl if it reaches 0.
>>
>> void v4l2_ctrl_request_setup(struct v4l2_ctrl_handler *hdl);
>>
>> Apply the values from the handler (i.e. request object) to the
>> hardware.
>>
>> You will have to modify v4l_g/s/try_ext_ctrls in v4l2-ioctls.c to
>> obtain the request v4l2_ctrl_handler pointer based on the request
>> field in struct v4l2_ext_controls.
>>
>> The first patch in this series is necessary to avoid cloning
>> controls that belong to other devices (as opposed to the subdev
>> or bridge device for which you make a request). It can probably
>> be dropped for codecs, but it is needed for MC devices like
>> omap3isp.
>>
>> This series has only been compile tested! So if it crashes as
>> soon as you try to use it, then that's why :-)
>>
>> Note: I'm not sure if it makes sense to refcount the control
>> handler, you might prefer to have a refcount in a higher-level
>> request struct. If that's the case, then I can drop the _get
>> function and replace the _put function by a v4l2_ctrl_request_free()
>> function.
> 
> That's exactly what I thought when I saw the refcounting. This is probably a
> problem for later since we want to focus on codecs for now, but I think we will
> ultimately want to manage refcounting outside of v4l2_ctrl_handler. Maybe a
> higher-level request class of which the current control-framework based design
> would be an implementation. I am thinking about IPs like the VSP1 which will
> probably want to model the controls either in a different way, or at least to
> add extra data beyond the controls.
> 
> All in all I think I can use this for codecs. I am still trying to shoehorn my
> first version into the media stuff, and to nobody's surprise this is not that
> easy. :P But the fact the control framework part is already mostly taken care
> of greatly helps.

I agree with you that it is better to do the refcounting in a higher-level
request object. Just don't call _get and call _put when you want to free it.
We'll clean it up later.

Regards,

	Hans

Re: [RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[Alexandre Courbot]

Hi Hans,

On Wed, Nov 15, 2017 at 7:12 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Alexandre,
>
> On 15/11/17 10:38, Alexandre Courbot wrote:
>> Hi Hans!
>>
>> Thanks for the patchset! It looks quite good at first sight, a few comments and
>> questions follow though.
>>
>> On Monday, November 13, 2017 11:34:02 PM JST, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> Hi Alexandre,
>>>
>>> This is a first implementation of the request API in the
>>> control framework. It is fairly simplistic at the moment in that
>>> it just clones all the control values (so no refcounting yet for
>>> values as Laurent proposed, I will work on that later). But this
>>> should not be a problem for codecs since there aren't all that many
>>> controls involved.
>>
>> Regarding value refcounting, I think we can probably do without it if we parse
>> the requests queue when looking values up. It may be more practical (having a
>> kref for each v4l2_ctrl_ref in a request sounds overkill to me), and maybe also
>> more predictible since we would have less chance of having dangling old values.
>>
>>> The API is as follows:
>>>
>>> struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
>>>
>>> This allocates a struct v4l2_ctrl_handler that is empty (i.e. has
>>> no controls) but is refcounted and is marked as representing a
>>> request.
>>>
>>> int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>>>                             const struct v4l2_ctrl_handler *from,
>>>                             bool (*filter)(const struct v4l2_ctrl *ctrl));
>>>
>>> Delete any existing controls in handler 'hdl', then clone the values
>>> from an existing handler 'from' into 'hdl'. If 'from' == NULL, then
>>> this just clears the handler. 'from' can either be another request
>>> control handler or a regular control handler in which case the
>>> current values are cloned. If 'filter' != NULL then you can
>>> filter which controls you want to clone.
>>
>> One thing that seems to be missing is, what happens if you try to set a control
>> on an empty request? IIUC this would currently fail because find_ref() would
>> not be able to find the control. The value ref should probably be created in
>> that case so we can create requests with a handful of controls.
>
> Wasn't the intention that we never have an empty request but always clone?
> I.e. in your code the _alloc call is always followed by a _clone call.
>
> The reason I have a separate _alloc function is that you use that when you
> want to create a new control handler ('new request'). If the user wants to reuse an
> existing request, then _clone can be called directly on the existing handler.
>
>> Also, if you clone a handler that is not a request, I understand that all
>> controls will be deduplicated, creating a full-state copy? That could be useful,
>> but since this is the only way to make the current code work, I hope that the
>> current impossibility to set a control on an empty request is a bug (or misunderstanding from my part).
>
> I think it is a misunderstanding. Seen from userspace you'll never have an empty
> request.

It depends on what we want requests to be:

(1) A representation of what the complete state of the device should
be when processing buffers, or

(2) A set of controls to be applied before processing buffers.

IIUC your patchset currently implements (1): as we clone a request (or
the current state of the device), we create a new ref for every
control that is not inherited. And when applying the request, we
consider all these controls again.

There is the fact that on more complex pipelines, the number of
controls may be big enough that we may want to limit the number we
need to manage per request, but my main concern is that this will
probably disallow some use-cases that we discussed in Prague.

For instance, let's say I create a request for a camera sensor by
cloning the device's control_handler, and set a few controls like
exposure that I want to be used for a give frame. But let's also say
that I have another thread taking care of focus on the same device -
this thread uses some other input to constantly adjust the focus by
calling S_EXT_CTRLS directly (i.e. without a request). When my request
gets processed, the focus will be reset to whatever it was when I
cloned the request, which we want to avoid (if you remember, I
interrupted Laurent to ask whether this is the behavior we wanted, and
we all agreed it was).

Or maybe I am missing something in your implementation?

That does not change the fact that I keep working on codecs using the
current model, but I think we will want to address this. The simplest
way to do this would be to only create refs for controls that we
explicitly change (or when cloning a request and not the
state_handler). It would also have the added benefit of reducing the
number of refs. :)

Cheers,
Alex.

Re: [RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[Hans Verkuil]

On 16/11/17 09:48, Alexandre Courbot wrote:
> Hi Hans,
> 
> On Wed, Nov 15, 2017 at 7:12 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Hi Alexandre,
>>
>> On 15/11/17 10:38, Alexandre Courbot wrote:
>>> Hi Hans!
>>>
>>> Thanks for the patchset! It looks quite good at first sight, a few comments and
>>> questions follow though.
>>>
>>> On Monday, November 13, 2017 11:34:02 PM JST, Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> Hi Alexandre,
>>>>
>>>> This is a first implementation of the request API in the
>>>> control framework. It is fairly simplistic at the moment in that
>>>> it just clones all the control values (so no refcounting yet for
>>>> values as Laurent proposed, I will work on that later). But this
>>>> should not be a problem for codecs since there aren't all that many
>>>> controls involved.
>>>
>>> Regarding value refcounting, I think we can probably do without it if we parse
>>> the requests queue when looking values up. It may be more practical (having a
>>> kref for each v4l2_ctrl_ref in a request sounds overkill to me), and maybe also
>>> more predictible since we would have less chance of having dangling old values.
>>>
>>>> The API is as follows:
>>>>
>>>> struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
>>>>
>>>> This allocates a struct v4l2_ctrl_handler that is empty (i.e. has
>>>> no controls) but is refcounted and is marked as representing a
>>>> request.
>>>>
>>>> int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>>>>                             const struct v4l2_ctrl_handler *from,
>>>>                             bool (*filter)(const struct v4l2_ctrl *ctrl));
>>>>
>>>> Delete any existing controls in handler 'hdl', then clone the values
>>>> from an existing handler 'from' into 'hdl'. If 'from' == NULL, then
>>>> this just clears the handler. 'from' can either be another request
>>>> control handler or a regular control handler in which case the
>>>> current values are cloned. If 'filter' != NULL then you can
>>>> filter which controls you want to clone.
>>>
>>> One thing that seems to be missing is, what happens if you try to set a control
>>> on an empty request? IIUC this would currently fail because find_ref() would
>>> not be able to find the control. The value ref should probably be created in
>>> that case so we can create requests with a handful of controls.
>>
>> Wasn't the intention that we never have an empty request but always clone?
>> I.e. in your code the _alloc call is always followed by a _clone call.
>>
>> The reason I have a separate _alloc function is that you use that when you
>> want to create a new control handler ('new request'). If the user wants to reuse an
>> existing request, then _clone can be called directly on the existing handler.
>>
>>> Also, if you clone a handler that is not a request, I understand that all
>>> controls will be deduplicated, creating a full-state copy? That could be useful,
>>> but since this is the only way to make the current code work, I hope that the
>>> current impossibility to set a control on an empty request is a bug (or misunderstanding from my part).
>>
>> I think it is a misunderstanding. Seen from userspace you'll never have an empty
>> request.
> 
> It depends on what we want requests to be:
> 
> (1) A representation of what the complete state of the device should
> be when processing buffers, or
> 
> (2) A set of controls to be applied before processing buffers.
> 
> IIUC your patchset currently implements (1): as we clone a request (or
> the current state of the device), we create a new ref for every
> control that is not inherited. And when applying the request, we
> consider all these controls again.
> 
> There is the fact that on more complex pipelines, the number of
> controls may be big enough that we may want to limit the number we
> need to manage per request, but my main concern is that this will
> probably disallow some use-cases that we discussed in Prague.
> 
> For instance, let's say I create a request for a camera sensor by
> cloning the device's control_handler, and set a few controls like
> exposure that I want to be used for a give frame. But let's also say
> that I have another thread taking care of focus on the same device -
> this thread uses some other input to constantly adjust the focus by
> calling S_EXT_CTRLS directly (i.e. without a request). When my request
> gets processed, the focus will be reset to whatever it was when I
> cloned the request, which we want to avoid (if you remember, I
> interrupted Laurent to ask whether this is the behavior we wanted, and
> we all agreed it was).
> 
> Or maybe I am missing something in your implementation?

Such things are simply not yet implemented. This is just a first version
that should be good enough for codecs. Once we have something up and
running that can be used for some real-life testing, then I will work on
this some more.

> That does not change the fact that I keep working on codecs using the
> current model, but I think we will want to address this. The simplest
> way to do this would be to only create refs for controls that we
> explicitly change (or when cloning a request and not the
> state_handler). It would also have the added benefit of reducing the
> number of refs. :)

Memory usage is not the problem here as compared to buffers the memory used
by requests is tiny. My plan is to be smart with how p_req is set. Right now
it always points to the local copy, but this can point to wherever the
'previous' value is stored. I have several ideas, but I rather wait until
I have something I can test.

But in any case, a control handler should have refs to all the controls,
that's how it works.

Regards,

	Hans

Re: [RFCv1 PATCH 0/6] v4l2-ctrls: implement requests

[Alexandre Courbot]

On Thu, Nov 16, 2017 at 6:13 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 16/11/17 09:48, Alexandre Courbot wrote:
>> Hi Hans,
>>
>> On Wed, Nov 15, 2017 at 7:12 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> Hi Alexandre,
>>>
>>> On 15/11/17 10:38, Alexandre Courbot wrote:
>>>> Hi Hans!
>>>>
>>>> Thanks for the patchset! It looks quite good at first sight, a few comments and
>>>> questions follow though.
>>>>
>>>> On Monday, November 13, 2017 11:34:02 PM JST, Hans Verkuil wrote:
>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>
>>>>> Hi Alexandre,
>>>>>
>>>>> This is a first implementation of the request API in the
>>>>> control framework. It is fairly simplistic at the moment in that
>>>>> it just clones all the control values (so no refcounting yet for
>>>>> values as Laurent proposed, I will work on that later). But this
>>>>> should not be a problem for codecs since there aren't all that many
>>>>> controls involved.
>>>>
>>>> Regarding value refcounting, I think we can probably do without it if we parse
>>>> the requests queue when looking values up. It may be more practical (having a
>>>> kref for each v4l2_ctrl_ref in a request sounds overkill to me), and maybe also
>>>> more predictible since we would have less chance of having dangling old values.
>>>>
>>>>> The API is as follows:
>>>>>
>>>>> struct v4l2_ctrl_handler *v4l2_ctrl_request_alloc(void);
>>>>>
>>>>> This allocates a struct v4l2_ctrl_handler that is empty (i.e. has
>>>>> no controls) but is refcounted and is marked as representing a
>>>>> request.
>>>>>
>>>>> int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>>>>>                             const struct v4l2_ctrl_handler *from,
>>>>>                             bool (*filter)(const struct v4l2_ctrl *ctrl));
>>>>>
>>>>> Delete any existing controls in handler 'hdl', then clone the values
>>>>> from an existing handler 'from' into 'hdl'. If 'from' == NULL, then
>>>>> this just clears the handler. 'from' can either be another request
>>>>> control handler or a regular control handler in which case the
>>>>> current values are cloned. If 'filter' != NULL then you can
>>>>> filter which controls you want to clone.
>>>>
>>>> One thing that seems to be missing is, what happens if you try to set a control
>>>> on an empty request? IIUC this would currently fail because find_ref() would
>>>> not be able to find the control. The value ref should probably be created in
>>>> that case so we can create requests with a handful of controls.
>>>
>>> Wasn't the intention that we never have an empty request but always clone?
>>> I.e. in your code the _alloc call is always followed by a _clone call.
>>>
>>> The reason I have a separate _alloc function is that you use that when you
>>> want to create a new control handler ('new request'). If the user wants to reuse an
>>> existing request, then _clone can be called directly on the existing handler.
>>>
>>>> Also, if you clone a handler that is not a request, I understand that all
>>>> controls will be deduplicated, creating a full-state copy? That could be useful,
>>>> but since this is the only way to make the current code work, I hope that the
>>>> current impossibility to set a control on an empty request is a bug (or misunderstanding from my part).
>>>
>>> I think it is a misunderstanding. Seen from userspace you'll never have an empty
>>> request.
>>
>> It depends on what we want requests to be:
>>
>> (1) A representation of what the complete state of the device should
>> be when processing buffers, or
>>
>> (2) A set of controls to be applied before processing buffers.
>>
>> IIUC your patchset currently implements (1): as we clone a request (or
>> the current state of the device), we create a new ref for every
>> control that is not inherited. And when applying the request, we
>> consider all these controls again.
>>
>> There is the fact that on more complex pipelines, the number of
>> controls may be big enough that we may want to limit the number we
>> need to manage per request, but my main concern is that this will
>> probably disallow some use-cases that we discussed in Prague.
>>
>> For instance, let's say I create a request for a camera sensor by
>> cloning the device's control_handler, and set a few controls like
>> exposure that I want to be used for a give frame. But let's also say
>> that I have another thread taking care of focus on the same device -
>> this thread uses some other input to constantly adjust the focus by
>> calling S_EXT_CTRLS directly (i.e. without a request). When my request
>> gets processed, the focus will be reset to whatever it was when I
>> cloned the request, which we want to avoid (if you remember, I
>> interrupted Laurent to ask whether this is the behavior we wanted, and
>> we all agreed it was).
>>
>> Or maybe I am missing something in your implementation?
>
> Such things are simply not yet implemented. This is just a first version
> that should be good enough for codecs. Once we have something up and
> running that can be used for some real-life testing, then I will work on
> this some more.

Good, I just wanted to make sure we were on the same page here.

>
>> That does not change the fact that I keep working on codecs using the
>> current model, but I think we will want to address this. The simplest
>> way to do this would be to only create refs for controls that we
>> explicitly change (or when cloning a request and not the
>> state_handler). It would also have the added benefit of reducing the
>> number of refs. :)
>
> Memory usage is not the problem here as compared to buffers the memory used
> by requests is tiny. My plan is to be smart with how p_req is set. Right now
> it always points to the local copy, but this can point to wherever the
> 'previous' value is stored. I have several ideas, but I rather wait until
> I have something I can test.
>
> But in any case, a control handler should have refs to all the controls,
> that's how it works.

I am fine with either way as long as the proper semantics are
displayed. I think we may see issues due to the fact that requests may
be queued in a different order from their cloning order, but let's
discuss that once we have something concrete.

I should be able to produce something using the version you have sent
- thanks again!

Re: [RFCv1 PATCH 1/6] v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev

[Mauro Carvalho Chehab]

Em Mon, 13 Nov 2017 15:34:03 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add a 'bool from_other_dev' argument: set to true if the two
> handlers refer to different devices (e.g. it is true when
> inheriting controls from a subdev into a main v4l2 bridge
> driver).
> 
> This will be used later when implementing support for the
> request API since we need to skip such controls.
> 
> TODO: check drivers/staging/media/imx/imx-media-fim.c change.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/dvb-frontends/rtl2832_sdr.c        |  5 +--
>  drivers/media/pci/bt8xx/bttv-driver.c            |  2 +-
>  drivers/media/pci/cx23885/cx23885-417.c          |  2 +-
>  drivers/media/pci/cx88/cx88-blackbird.c          |  2 +-
>  drivers/media/pci/cx88/cx88-video.c              |  2 +-
>  drivers/media/pci/saa7134/saa7134-empress.c      |  4 +--
>  drivers/media/pci/saa7134/saa7134-video.c        |  2 +-
>  drivers/media/platform/exynos4-is/fimc-capture.c |  2 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c      |  3 +-
>  drivers/media/platform/rcar_drif.c               |  2 +-
>  drivers/media/platform/soc_camera/soc_camera.c   |  3 +-
>  drivers/media/platform/vivid/vivid-ctrls.c       | 42 ++++++++++++------------
>  drivers/media/usb/cx231xx/cx231xx-417.c          |  2 +-
>  drivers/media/usb/cx231xx/cx231xx-video.c        |  4 +--
>  drivers/media/usb/msi2500/msi2500.c              |  2 +-
>  drivers/media/usb/tm6000/tm6000-video.c          |  2 +-
>  drivers/media/v4l2-core/v4l2-ctrls.c             | 11 ++++---
>  drivers/media/v4l2-core/v4l2-device.c            |  3 +-
>  drivers/staging/media/imx/imx-media-dev.c        |  2 +-
>  drivers/staging/media/imx/imx-media-fim.c        |  2 +-
>  include/media/v4l2-ctrls.h                       |  4 ++-

You forgot to update Documentation/media/kapi/v4l2-controls.rst.

>  21 files changed, 56 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
> index c6e78d870ccd..6064d28224e8 100644
> --- a/drivers/media/dvb-frontends/rtl2832_sdr.c
> +++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
> @@ -1394,7 +1394,8 @@ static int rtl2832_sdr_probe(struct platform_device *pdev)
>  	case RTL2832_SDR_TUNER_E4000:
>  		v4l2_ctrl_handler_init(&dev->hdl, 9);
>  		if (subdev)
> -			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler, NULL);
> +			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler,
> +					      NULL, true);

Changing all drivers to tell if a control belongs to a subdev or not
seems weird. I won't doubt that people may get it wrong and fill it
with a wrong value.

IMHO, it would be better, instead, to pass some struct to the function
that would allow the function to check if the device is a subdev
or not.

For example, we could do:

int v4l2_ctrl_subdev_add_handler(struct v4l2_ctrl_handler *hdl,
				 struct v4l2_subdev *sd,
                        	 v4l2_ctrl_filter filter);

That should be used for all subdev controls. Internally, such
function would be using:
	sd->control_handler
as the add parameter for v4l2_ctrl_add_handler().

I would also try to do the same for devices: have a
v4l2_ctrl_dev_add_handler() that would take a struct v4l2_dev, and
make v4l2_ctrl_add_handler() a static function inside v4l2-ctrls.c.

This way, we should avoid the risk of wrong usages.

Thanks,
Mauro

Re: [RFCv1 PATCH 1/6] v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev

[Hans Verkuil]

On 17/11/17 14:41, Mauro Carvalho Chehab wrote:
> Em Mon, 13 Nov 2017 15:34:03 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add a 'bool from_other_dev' argument: set to true if the two
>> handlers refer to different devices (e.g. it is true when
>> inheriting controls from a subdev into a main v4l2 bridge
>> driver).
>>
>> This will be used later when implementing support for the
>> request API since we need to skip such controls.
>>
>> TODO: check drivers/staging/media/imx/imx-media-fim.c change.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/dvb-frontends/rtl2832_sdr.c        |  5 +--
>>  drivers/media/pci/bt8xx/bttv-driver.c            |  2 +-
>>  drivers/media/pci/cx23885/cx23885-417.c          |  2 +-
>>  drivers/media/pci/cx88/cx88-blackbird.c          |  2 +-
>>  drivers/media/pci/cx88/cx88-video.c              |  2 +-
>>  drivers/media/pci/saa7134/saa7134-empress.c      |  4 +--
>>  drivers/media/pci/saa7134/saa7134-video.c        |  2 +-
>>  drivers/media/platform/exynos4-is/fimc-capture.c |  2 +-
>>  drivers/media/platform/rcar-vin/rcar-v4l2.c      |  3 +-
>>  drivers/media/platform/rcar_drif.c               |  2 +-
>>  drivers/media/platform/soc_camera/soc_camera.c   |  3 +-
>>  drivers/media/platform/vivid/vivid-ctrls.c       | 42 ++++++++++++------------
>>  drivers/media/usb/cx231xx/cx231xx-417.c          |  2 +-
>>  drivers/media/usb/cx231xx/cx231xx-video.c        |  4 +--
>>  drivers/media/usb/msi2500/msi2500.c              |  2 +-
>>  drivers/media/usb/tm6000/tm6000-video.c          |  2 +-
>>  drivers/media/v4l2-core/v4l2-ctrls.c             | 11 ++++---
>>  drivers/media/v4l2-core/v4l2-device.c            |  3 +-
>>  drivers/staging/media/imx/imx-media-dev.c        |  2 +-
>>  drivers/staging/media/imx/imx-media-fim.c        |  2 +-
>>  include/media/v4l2-ctrls.h                       |  4 ++-
> 
> You forgot to update Documentation/media/kapi/v4l2-controls.rst.
> 
>>  21 files changed, 56 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
>> index c6e78d870ccd..6064d28224e8 100644
>> --- a/drivers/media/dvb-frontends/rtl2832_sdr.c
>> +++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
>> @@ -1394,7 +1394,8 @@ static int rtl2832_sdr_probe(struct platform_device *pdev)
>>  	case RTL2832_SDR_TUNER_E4000:
>>  		v4l2_ctrl_handler_init(&dev->hdl, 9);
>>  		if (subdev)
>> -			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler, NULL);
>> +			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler,
>> +					      NULL, true);
> 
> Changing all drivers to tell if a control belongs to a subdev or not
> seems weird. I won't doubt that people may get it wrong and fill it
> with a wrong value.
> 
> IMHO, it would be better, instead, to pass some struct to the function
> that would allow the function to check if the device is a subdev
> or not.
> 
> For example, we could do:
> 
> int v4l2_ctrl_subdev_add_handler(struct v4l2_ctrl_handler *hdl,
> 				 struct v4l2_subdev *sd,
>                         	 v4l2_ctrl_filter filter);
> 
> That should be used for all subdev controls. Internally, such
> function would be using:
> 	sd->control_handler
> as the add parameter for v4l2_ctrl_add_handler().
> 
> I would also try to do the same for devices: have a
> v4l2_ctrl_dev_add_handler() that would take a struct v4l2_dev, and
> make v4l2_ctrl_add_handler() a static function inside v4l2-ctrls.c.
> 
> This way, we should avoid the risk of wrong usages.

You can ignore this patch. I'm not happy with it either, and I'm pretty
sure it is not needed for the stateless codec use-case.

Regards,

	Hans