HDMI and Composite capture on Lager, for kernel 4.1

HDMI and Composite capture on Lager, for kernel 4.1

[William Towle]

  This is our latest test branch for video support on Lager, ported
to kernel 4.1 as per commit 9cae84b32dd52768cf2fd2fcb214c3f570676c4b
("[media] DocBook/media: fix syntax error") on the media-tree master
branch last week.

  Single frame and video capture is working with appropriate test
cases for gstreamer, some (minor) quirks notwithstanding.
Functionally, this is in more or less the state we need it to be; for
the rest of the world we hope we have enhanced the ability to do any
necessary debugging on it. The intention is to upstream this version
as soon as possible, subject to feedback.

  NB: for single frame capture, images of appropriate resolutions are
created from both composite and HDMI inputs. For best quality video
capture we have found that gst-launch-1.0 needs a pipeline specifying
width, height, format ("=YUY2" in particular has an appropriate
turnaround time for smooth results), and framerate.

  Enclosed are:
	[PATCH 01/20] ARM: shmobile: lager dts: Add entries for VIN HDMI
	[PATCH 02/20] media: adv7180: add of match table
	[PATCH 03/20] media: adv7604: chip info and formats for ADV7612
	[PATCH 04/20] media: adv7604: document support for ADV7612 dual HDMI
	[PATCH 05/20] media: adv7604: ability to read default input port
	[PATCH 06/20] ARM: shmobile: lager dts: specify default-input for
	[PATCH 07/20] media: soc_camera: rcar_vin: Add BT.709 24-bit RGB888
	[PATCH 08/20] media: soc_camera pad-aware driver initialisation
	[PATCH 09/20] media: rcar_vin: Use correct pad number in try_fmt
	[PATCH 10/20] media: soc_camera: soc_scale_crop: Use correct pad
	[PATCH 11/20] media: soc_camera: Fill std field in enum_input
	[PATCH 12/20] media: soc_camera: Fix error reporting in expbuf
	[PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap
	[PATCH 14/20] media: rcar_vin: Reject videobufs that are too small
	[PATCH 15/20] media: rcar_vin: Don't advertise support for USERPTR
	[PATCH 16/20] media: adv7180: Fix set_pad_format() passing wrong
	[PATCH 17/20] media: adv7604: Support V4L_FIELD_INTERLACED
	[PATCH 18/20] media: adv7604: Always query_dv_timings in
	[PATCH 19/20] media: rcar_vin: Clean up format debugging statements
	[PATCH 20/20] media: soc_camera: Add debugging for get_formats

"Phew!"
Cheers,
  Wills.

[PATCH 01/20] ARM: shmobile: lager dts: Add entries for VIN HDMI input support

[William Towle]

Adds DT entries for vin0_pins, adv7612 and vin0

Signed-off-by: William Towle <william.towle@codethink.co.uk>
Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 arch/arm/boot/dts/r8a7790-lager.dts |   41 ++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index aaa4f25..90c4531 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -370,7 +370,12 @@
 		renesas,function = "usb2";
 	};
 
-	vin1_pins: vin {
+	vin0_pins: vin0 {
+		renesas,groups = "vin0_data24", "vin0_sync", "vin0_field", "vin0_clkenb", "vin0_clk";
+		renesas,function = "vin0";
+	};
+
+	vin1_pins: vin1 {
 		renesas,groups = "vin1_data8", "vin1_clk";
 		renesas,function = "vin1";
 	};
@@ -531,6 +536,18 @@
 		reg = <0x12>;
 	};
 
+	hdmi-in@4c {
+		compatible = "adi,adv7612";
+		reg = <0x4c>;
+		remote = <&vin0>;
+
+		port {
+			hdmi_in_ep: endpoint {
+				remote-endpoint = <&vin0ep0>;
+			};
+		};
+	};
+
 	composite-in@20 {
 		compatible = "adi,adv7180";
 		reg = <0x20>;
@@ -646,6 +663,28 @@
 	status = "okay";
 };
 
+/* HDMI video input */
+&vin0 {
+	pinctrl-0 = <&vin0_pins>;
+	pinctrl-names = "default";
+
+	status = "ok";
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		vin0ep0: endpoint {
+			remote-endpoint = <&hdmi_in_ep>;
+			bus-width = <24>;
+			hsync-active = <0>;
+			vsync-active = <0>;
+			pclk-sample = <1>;
+			data-active = <1>;
+		};
+	};
+};
+
 /* composite video input */
 &vin1 {
 	pinctrl-0 = <&vin1_pins>;
-- 
1.7.10.4

[PATCH 02/20] media: adv7180: add of match table

[William Towle]

From: Ben Dooks <ben.dooks@codethink.co.uk>

Add a proper of match id for use when the device is being bound via
device tree, to avoid having to use the i2c old-style binding of the
device.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Signed-off-by: William.Towle <william.towle@codethink.co.uk>
Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/i2c/adv7180.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index a493c0b..09a96df 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -25,6 +25,7 @@
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <media/v4l2-ioctl.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
@@ -1324,11 +1325,21 @@ static SIMPLE_DEV_PM_OPS(adv7180_pm_ops, adv7180_suspend, adv7180_resume);
 #define ADV7180_PM_OPS NULL
 #endif
 
+#ifdef CONFIG_OF
+static const struct of_device_id adv7180_of_id[] = {
+	{ .compatible = "adi,adv7180", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, adv7180_of_id);
+#endif
+
 static struct i2c_driver adv7180_driver = {
 	.driver = {
 		   .owner = THIS_MODULE,
 		   .name = KBUILD_MODNAME,
 		   .pm = ADV7180_PM_OPS,
+		   .of_match_table = of_match_ptr(adv7180_of_id),
 		   },
 	.probe = adv7180_probe,
 	.remove = adv7180_remove,
-- 
1.7.10.4

[PATCH 03/20] media: adv7604: chip info and formats for ADV7612

[William Towle]

Add support for the ADV7612 chip as implemented on Renesas' Lager
board to adv7604.c, including lists for formats/colourspace/timing
selection and an IRQ handler.

Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/i2c/adv7604.c |   83 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index be3f866..a2abb04 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -80,6 +80,7 @@ MODULE_LICENSE("GPL");
 enum adv76xx_type {
 	ADV7604,
 	ADV7611,
+	ADV7612,
 };
 
 struct adv76xx_reg_seq {
@@ -753,6 +754,23 @@ static const struct adv76xx_format_info adv7611_formats[] = {
 	  ADV76XX_OP_MODE_SEL_SDR_422_2X | ADV76XX_OP_FORMAT_SEL_12BIT },
 };
 
+static const struct adv76xx_format_info adv7612_formats[] = {
+	{ MEDIA_BUS_FMT_RGB888_1X24, ADV76XX_OP_CH_SEL_RGB, true, false,
+	  ADV76XX_OP_MODE_SEL_SDR_444 | ADV76XX_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_YUYV8_2X8, ADV76XX_OP_CH_SEL_RGB, false, false,
+	  ADV76XX_OP_MODE_SEL_SDR_422 | ADV76XX_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_YVYU8_2X8, ADV76XX_OP_CH_SEL_RGB, false, true,
+	  ADV76XX_OP_MODE_SEL_SDR_422 | ADV76XX_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_UYVY8_1X16, ADV76XX_OP_CH_SEL_RBG, false, false,
+	  ADV76XX_OP_MODE_SEL_SDR_422_2X | ADV76XX_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_VYUY8_1X16, ADV76XX_OP_CH_SEL_RBG, false, true,
+	  ADV76XX_OP_MODE_SEL_SDR_422_2X | ADV76XX_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_YUYV8_1X16, ADV76XX_OP_CH_SEL_RGB, false, false,
+	  ADV76XX_OP_MODE_SEL_SDR_422_2X | ADV76XX_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_YVYU8_1X16, ADV76XX_OP_CH_SEL_RGB, false, true,
+	  ADV76XX_OP_MODE_SEL_SDR_422_2X | ADV76XX_OP_FORMAT_SEL_8BIT },
+};
+
 static const struct adv76xx_format_info *
 adv76xx_format_info(struct adv76xx_state *state, u32 code)
 {
@@ -2465,6 +2483,11 @@ static void adv7611_setup_irqs(struct v4l2_subdev *sd)
 	io_write(sd, 0x41, 0xd0); /* STDI irq for any change, disable INT2 */
 }
 
+static void adv7612_setup_irqs(struct v4l2_subdev *sd)
+{
+	io_write(sd, 0x41, 0xd0); /* disable INT2 */
+}
+
 static void adv76xx_unregister_clients(struct adv76xx_state *state)
 {
 	unsigned int i;
@@ -2552,6 +2575,19 @@ static const struct adv76xx_reg_seq adv7611_recommended_settings_hdmi[] = {
 	{ ADV76XX_REG_SEQ_TERM, 0 },
 };
 
+static const struct adv76xx_reg_seq adv7612_recommended_settings_hdmi[] = {
+	{ ADV76XX_REG(ADV76XX_PAGE_CP, 0x6c), 0x00 },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x9b), 0x03 },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x6f), 0x08 },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x85), 0x1f },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x87), 0x70 },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x57), 0xda },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x58), 0x01 },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x03), 0x98 },
+	{ ADV76XX_REG(ADV76XX_PAGE_HDMI, 0x4c), 0x44 },
+	{ ADV76XX_REG_SEQ_TERM, 0 },
+};
+
 static const struct adv76xx_chip_info adv76xx_chip_info[] = {
 	[ADV7604] = {
 		.type = ADV7604,
@@ -2640,17 +2676,59 @@ static const struct adv76xx_chip_info adv76xx_chip_info[] = {
 		.field1_vsync_mask = 0x3fff,
 		.field1_vbackporch_mask = 0x3fff,
 	},
+	[ADV7612] = {
+		.type = ADV7612,
+		.has_afe = false,
+		.max_port = ADV7604_PAD_HDMI_PORT_B,
+		.num_dv_ports = 2,
+		.edid_enable_reg = 0x74,
+		.edid_status_reg = 0x76,
+		.lcf_reg = 0xa3,
+		.tdms_lock_mask = 0x43,
+		.cable_det_mask = 0x01,
+		.fmt_change_digital_mask = 0x03,
+		.formats = adv7612_formats,
+		.nformats = ARRAY_SIZE(adv7612_formats),
+		.set_termination = adv7611_set_termination,
+		.setup_irqs = adv7612_setup_irqs,
+		.read_hdmi_pixelclock = adv7611_read_hdmi_pixelclock,
+		.read_cable_det = adv7611_read_cable_det,
+		.recommended_settings = {
+		    [1] = adv7612_recommended_settings_hdmi,
+		},
+		.num_recommended_settings = {
+		    [1] = ARRAY_SIZE(adv7612_recommended_settings_hdmi),
+		},
+		.page_mask = BIT(ADV76XX_PAGE_IO) | BIT(ADV76XX_PAGE_CEC) |
+			BIT(ADV76XX_PAGE_INFOFRAME) | BIT(ADV76XX_PAGE_AFE) |
+			BIT(ADV76XX_PAGE_REP) |  BIT(ADV76XX_PAGE_EDID) |
+			BIT(ADV76XX_PAGE_HDMI) | BIT(ADV76XX_PAGE_CP),
+		.linewidth_mask = 0x1fff,
+		.field0_height_mask = 0x1fff,
+		.field1_height_mask = 0x1fff,
+		.hfrontporch_mask = 0x1fff,
+		.hsync_mask = 0x1fff,
+		.hbackporch_mask = 0x1fff,
+		.field0_vfrontporch_mask = 0x3fff,
+		.field0_vsync_mask = 0x3fff,
+		.field0_vbackporch_mask = 0x3fff,
+		.field1_vfrontporch_mask = 0x3fff,
+		.field1_vsync_mask = 0x3fff,
+		.field1_vbackporch_mask = 0x3fff,
+	},
 };
 
 static const struct i2c_device_id adv76xx_i2c_id[] = {
 	{ "adv7604", (kernel_ulong_t)&adv76xx_chip_info[ADV7604] },
 	{ "adv7611", (kernel_ulong_t)&adv76xx_chip_info[ADV7611] },
+	{ "adv7612", (kernel_ulong_t)&adv76xx_chip_info[ADV7612] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adv76xx_i2c_id);
 
 static const struct of_device_id adv76xx_of_id[] __maybe_unused = {
 	{ .compatible = "adi,adv7611", .data = &adv76xx_chip_info[ADV7611] },
+	{ .compatible = "adi,adv7612", .data = &adv76xx_chip_info[ADV7612] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adv76xx_of_id);
@@ -2805,8 +2883,9 @@ static int adv76xx_probe(struct i2c_client *client,
 	} else {
 		val = (adv_smbus_read_byte_data_check(client, 0xea, false) << 8)
 		    | (adv_smbus_read_byte_data_check(client, 0xeb, false) << 0);
-		if (val != 0x2051) {
-			v4l2_info(sd, "not an adv7611 on address 0x%x\n",
+		if ((state->info->type == ADV7611 && val != 0x2051) ||
+			(state->info->type == ADV7612 && val != 0x2041)) {
+			v4l2_info(sd, "not an adv761x on address 0x%x\n",
 					client->addr << 1);
 			return -ENODEV;
 		}
-- 
1.7.10.4

Re: [PATCH 03/20] media: adv7604: chip info and formats for ADV7612

[Guennadi Liakhovetski]

Hi William,

Just a nitpick:

On Wed, 20 May 2015, William Towle wrote:

> Add support for the ADV7612 chip as implemented on Renesas' Lager
> board to adv7604.c, including lists for formats/colourspace/timing
> selection and an IRQ handler.
> 
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/i2c/adv7604.c |   83 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 81 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index be3f866..a2abb04 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c

[snip]

> @@ -2805,8 +2883,9 @@ static int adv76xx_probe(struct i2c_client *client,
>  	} else {
>  		val = (adv_smbus_read_byte_data_check(client, 0xea, false) << 8)
>  		    | (adv_smbus_read_byte_data_check(client, 0xeb, false) << 0);
> -		if (val != 0x2051) {
> -			v4l2_info(sd, "not an adv7611 on address 0x%x\n",
> +		if ((state->info->type == ADV7611 && val != 0x2051) ||
> +			(state->info->type == ADV7612 && val != 0x2041)) {
> +			v4l2_info(sd, "not an adv761x on address 0x%x\n",

A switch / case might look slightly better here.

Thanks
Guennadi

>  					client->addr << 1);
>  			return -ENODEV;
>  		}
> -- 
> 1.7.10.4
> 

[PATCH 04/20] media: adv7604: document support for ADV7612 dual HDMI input decoder

[William Towle]

From: Ian Molton <ian.molton@codethink.co.uk>

This documentation accompanies the patch adding support for the ADV7612
dual HDMI decoder / repeater chip.

Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 .../devicetree/bindings/media/i2c/adv7604.txt        |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index c27cede..7eafdbc 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -1,15 +1,17 @@
-* Analog Devices ADV7604/11 video decoder with HDMI receiver
+* Analog Devices ADV7604/11/12 video decoder with HDMI receiver
 
-The ADV7604 and ADV7611 are multiformat video decoders with an integrated HDMI
-receiver. The ADV7604 has four multiplexed HDMI inputs and one analog input,
-and the ADV7611 has one HDMI input and no analog input.
+The ADV7604 and ADV7611/12 are multiformat video decoders with an integrated
+HDMI receiver. The ADV7604 has four multiplexed HDMI inputs and one analog
+input, and the ADV7611 has one HDMI input and no analog input. The 7612 is
+similar to the 7611 but has 2 HDMI inputs.
 
-These device tree bindings support the ADV7611 only at the moment.
+These device tree bindings support the ADV7611/12 only at the moment.
 
 Required Properties:
 
   - compatible: Must contain one of the following
     - "adi,adv7611" for the ADV7611
+    - "adi,adv7612" for the ADV7612
 
   - reg: I2C slave address
 
@@ -22,10 +24,10 @@ port, in accordance with the video interface bindings defined in
 Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
 are numbered as follows.
 
-  Port			ADV7611
+  Port			ADV7611    ADV7612
 ------------------------------------------------------------
-  HDMI			0
-  Digital output	1
+  HDMI			0             0, 1
+  Digital output	1                2
 
 The digital output port node must contain at least one endpoint.
 
-- 
1.7.10.4

[PATCH 05/20] media: adv7604: ability to read default input port from DT

[William Towle]

From: Ian Molton <ian.molton@codethink.co.uk>

Adds support to the adv7604 driver for specifying the default input
port in the Device tree. If no value is provided, the driver will be
unable to select an input without help from userspace.

Tested-by: William Towle <william.towle@codethink.co.uk>
Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
---
 Documentation/devicetree/bindings/media/i2c/adv7604.txt |    3 +++
 drivers/media/i2c/adv7604.c                             |    8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 7eafdbc..8337f75 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -47,6 +47,7 @@ Optional Endpoint Properties:
   If none of hsync-active, vsync-active and pclk-sample is specified the
   endpoint will use embedded BT.656 synchronization.
 
+  - default-input: Select which input is selected after reset.
 
 Example:
 
@@ -60,6 +61,8 @@ Example:
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		default-input = <0>;
+
 		port@0 {
 			reg = <0>;
 		};
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index a2abb04..4bde3e1 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2739,6 +2739,7 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 	struct device_node *endpoint;
 	struct device_node *np;
 	unsigned int flags;
+	u32 v;
 
 	np = state->i2c_clients[ADV76XX_PAGE_IO]->dev.of_node;
 
@@ -2748,6 +2749,12 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 		return -EINVAL;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+
+	if (!of_property_read_u32(endpoint, "default-input", &v))
+		state->pdata.default_input = v;
+	else
+		state->pdata.default_input = -1;
+
 	of_node_put(endpoint);
 
 	flags = bus_cfg.bus.parallel.flags;
@@ -2786,7 +2793,6 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 	/* Hardcode the remaining platform data fields. */
 	state->pdata.disable_pwrdnb = 0;
 	state->pdata.disable_cable_det_rst = 0;
-	state->pdata.default_input = -1;
 	state->pdata.blank_data = 1;
 	state->pdata.alt_data_sat = 1;
 	state->pdata.op_format_mode_sel = ADV7604_OP_FORMAT_MODE0;
-- 
1.7.10.4

[PATCH 06/20] ARM: shmobile: lager dts: specify default-input for ADV7612

[William Towle]

Set 'default-input' property for ADV7612. Enables image/video capture
without the need to have userspace specifying routing.

Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
Tested-by: William Towle <william.towle@codethink.co.uk>
---
 arch/arm/boot/dts/r8a7790-lager.dts |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index 90c4531..6946e9a 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -544,6 +544,7 @@
 		port {
 			hdmi_in_ep: endpoint {
 				remote-endpoint = <&vin0ep0>;
+				default-input = <0>;
 			};
 		};
 	};
-- 
1.7.10.4

[PATCH 07/20] media: soc_camera: rcar_vin: Add BT.709 24-bit RGB888 input support

[William Towle]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

Modified to use MEDIA_BUS_FMT_* constants

Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index db7700b..0f67646 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -98,6 +98,7 @@
 #define VNMC_INF_YUV10_BT656	(2 << 16)
 #define VNMC_INF_YUV10_BT601	(3 << 16)
 #define VNMC_INF_YUV16		(5 << 16)
+#define VNMC_INF_RGB888		(6 << 16)
 #define VNMC_VUP		(1 << 10)
 #define VNMC_IM_ODD		(0 << 3)
 #define VNMC_IM_ODD_EVEN	(1 << 3)
@@ -620,6 +621,10 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
 
 	/* input interface */
 	switch (icd->current_fmt->code) {
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		/* BT.601/BT.709 24-bit RGB-888 */
+		vnmc |= VNMC_INF_RGB888;
+		break;
 	case MEDIA_BUS_FMT_YUYV8_1X16:
 		/* BT.601/BT.1358 16bit YCbCr422 */
 		vnmc |= VNMC_INF_YUV16;
@@ -679,6 +684,15 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
 	if (output_is_yuv)
 		vnmc |= VNMC_BPS;
 
+	/*
+	 * The above assumes YUV input, toggle BPS for RGB input.
+	 * RGB inputs can be detected by checking that the most-significant
+	 * two bits of INF are set. This corresponds to the bits
+	 * set in VNMC_INF_RGB888.
+	 */
+	if ((vnmc & VNMC_INF_RGB888) == VNMC_INF_RGB888)
+		vnmc ^= VNMC_BPS;
+
 	/* progressive or interlaced mode */
 	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
 
@@ -1423,6 +1437,7 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 	case MEDIA_BUS_FMT_YUYV8_1X16:
 	case MEDIA_BUS_FMT_YUYV8_2X8:
 	case MEDIA_BUS_FMT_YUYV10_2X10:
+	case MEDIA_BUS_FMT_RGB888_1X24:
 		if (cam->extra_fmt)
 			break;
 
-- 
1.7.10.4

Re: [PATCH 07/20] media: soc_camera: rcar_vin: Add BT.709 24-bit RGB888 input support

[Guennadi Liakhovetski]

Hi William,

How about this version of this patch:

https://patchwork.linuxtv.org/patch/28098/

? I personally like that one better, it seems clearer to me. This one 
first sets a bit to vnmp, then make another check and inverts it, whereas 
that version clearly sets it just for equal colour-spaces. I just never 
got with proper Sob and (maybe?) authorship.

Thanks
Guennadi

On Wed, 20 May 2015, William Towle wrote:

> From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> 
> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> Modified to use MEDIA_BUS_FMT_* constants
> 
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index db7700b..0f67646 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -98,6 +98,7 @@
>  #define VNMC_INF_YUV10_BT656	(2 << 16)
>  #define VNMC_INF_YUV10_BT601	(3 << 16)
>  #define VNMC_INF_YUV16		(5 << 16)
> +#define VNMC_INF_RGB888		(6 << 16)
>  #define VNMC_VUP		(1 << 10)
>  #define VNMC_IM_ODD		(0 << 3)
>  #define VNMC_IM_ODD_EVEN	(1 << 3)
> @@ -620,6 +621,10 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>  
>  	/* input interface */
>  	switch (icd->current_fmt->code) {
> +	case MEDIA_BUS_FMT_RGB888_1X24:
> +		/* BT.601/BT.709 24-bit RGB-888 */
> +		vnmc |= VNMC_INF_RGB888;
> +		break;
>  	case MEDIA_BUS_FMT_YUYV8_1X16:
>  		/* BT.601/BT.1358 16bit YCbCr422 */
>  		vnmc |= VNMC_INF_YUV16;
> @@ -679,6 +684,15 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>  	if (output_is_yuv)
>  		vnmc |= VNMC_BPS;
>  
> +	/*
> +	 * The above assumes YUV input, toggle BPS for RGB input.
> +	 * RGB inputs can be detected by checking that the most-significant
> +	 * two bits of INF are set. This corresponds to the bits
> +	 * set in VNMC_INF_RGB888.
> +	 */
> +	if ((vnmc & VNMC_INF_RGB888) == VNMC_INF_RGB888)
> +		vnmc ^= VNMC_BPS;
> +
>  	/* progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
>  
> @@ -1423,6 +1437,7 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
>  	case MEDIA_BUS_FMT_YUYV8_1X16:
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>  	case MEDIA_BUS_FMT_YUYV10_2X10:
> +	case MEDIA_BUS_FMT_RGB888_1X24:
>  		if (cam->extra_fmt)
>  			break;
>  
> -- 
> 1.7.10.4
> 

Re: [PATCH 07/20] media: soc_camera: rcar_vin: Add BT.709 24-bit RGB888 input support

[William Towle]

On Mon, 25 May 2015, Guennadi Liakhovetski wrote:
> How about this version of this patch:
>
> https://patchwork.linuxtv.org/patch/28098/
>
> ? I personally like that one better, it seems clearer to me. This one
> first sets a bit to vnmp, then make another check and inverts it, whereas
> that version clearly sets it just for equal colour-spaces. I just never
> got with proper Sob and (maybe?) authorship.

Hi Guennadi,
   Thanks for noticing - we reverted this patch to the version
previously indicated in testing and it didn't get set back.

   We have a test branch for the next version, and I shall attend to
this and the other authorship comments you made immediately.

Thanks,
   Wills.

[PATCH 08/20] media: soc_camera pad-aware driver initialisation

[William Towle]

Add detection of source pad number for drivers aware of the media
controller API, so that soc_camera/rcar_vin can create device nodes
to support a driver such as adv7604.c (for HDMI on Lager) underneath.

Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
 drivers/media/platform/soc_camera/soc_camera.c |   27 +++++++++++++++++++++++-
 include/media/soc_camera.h                     |    1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 0f67646..b4e9b43 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 		struct v4l2_mbus_framefmt *mf = &fmt.format;
 		struct v4l2_rect rect;
 		struct device *dev = icd->parent;
+		struct media_pad *remote_pad;
 		int shift;
 
+		remote_pad = media_entity_remote_pad(
+					&icd->vdev->entity.pads[0]);
+		fmt.pad = remote_pad->index;
 		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
 		if (ret < 0)
 			return ret;
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index d708df4..126d645 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 	struct v4l2_mbus_framefmt *mf = &fmt.format;
+	int src_pad_idx = -1;
 	int ret;
 
 	sd->grp_id = soc_camera_grp_id(icd);
@@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 	}
 
 	/* At this point client .probe() should have run already */
-	ret = soc_camera_init_user_formats(icd);
+	ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0);
+	if (!ret) {
+		for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads;
+				src_pad_idx++)
+			if (sd->entity.pads[src_pad_idx].flags
+						== MEDIA_PAD_FL_SOURCE)
+				break;
+
+		if (src_pad_idx < sd->entity.num_pads) {
+			if (!media_entity_create_link(
+				&icd->vdev->entity, 0,
+				&sd->entity, src_pad_idx,
+				MEDIA_LNK_FL_IMMUTABLE |
+				MEDIA_LNK_FL_ENABLED)) {
+				ret = soc_camera_init_user_formats(icd);
+			}
+		}
+	}
+
 	if (ret < 0)
 		goto eusrfmt;
 
@@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 		goto evidstart;
 
 	/* Try to improve our guess of a reasonable window format */
+	fmt.pad = src_pad_idx;
 	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
 		icd->user_width		= mf->width;
 		icd->user_height	= mf->height;
@@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 evidstart:
 	soc_camera_free_user_formats(icd);
 eusrfmt:
+	media_entity_cleanup(&icd->vdev->entity);
 	soc_camera_remove_device(icd);
 
 	return ret;
@@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct soc_camera_device *icd)
 	if (icd->num_user_formats)
 		soc_camera_free_user_formats(icd);
 
+	if (icd->vdev->entity.num_pads) {
+		media_entity_cleanup(&icd->vdev->entity);
+	}
+
 	if (icd->clk) {
 		/* For the synchronous case */
 		v4l2_clk_unregister(icd->clk);
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 2f6261f..f0c5238 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -42,6 +42,7 @@ struct soc_camera_device {
 	unsigned char devnum;		/* Device number per host */
 	struct soc_camera_sense *sense;	/* See comment in struct definition */
 	struct video_device *vdev;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 	const struct soc_camera_format_xlate *current_fmt;
 	struct soc_camera_format_xlate *user_formats;
-- 
1.7.10.4

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Sergei Shtylyov]

Hello.

On 05/20/2015 07:39 PM, William Towle wrote:

> Add detection of source pad number for drivers aware of the media
> controller API, so that soc_camera/rcar_vin can create device nodes
> to support a driver such as adv7604.c (for HDMI on Lager) underneath.

> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> ---
>   drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
>   drivers/media/platform/soc_camera/soc_camera.c |   27 +++++++++++++++++++++++-
>   include/media/soc_camera.h                     |    1 +
>   3 files changed, 31 insertions(+), 1 deletion(-)

> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 0f67646..b4e9b43 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
[...]
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index d708df4..126d645 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
[...]
> @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>   	}
>
>   	/* At this point client .probe() should have run already */
> -	ret = soc_camera_init_user_formats(icd);
> +	ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0);
> +	if (!ret) {
> +		for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads;
> +				src_pad_idx++)
> +			if (sd->entity.pads[src_pad_idx].flags
> +						== MEDIA_PAD_FL_SOURCE)
> +				break;
> +
> +		if (src_pad_idx < sd->entity.num_pads) {
> +			if (!media_entity_create_link(
> +				&icd->vdev->entity, 0,
> +				&sd->entity, src_pad_idx,
> +				MEDIA_LNK_FL_IMMUTABLE |
> +				MEDIA_LNK_FL_ENABLED)) {

    Please either start the continuation lines under ! on the first line or 
indent them more to the right, so that's easier on the eyes.

> +				ret = soc_camera_init_user_formats(icd);
> +			}
> +		}
> +	}
> +
>   	if (ret < 0)
>   		goto eusrfmt;
>
> @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>   		goto evidstart;
>
>   	/* Try to improve our guess of a reasonable window format */
> +	fmt.pad = src_pad_idx;
>   	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
>   		icd->user_width		= mf->width;
>   		icd->user_height	= mf->height;
> @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>   evidstart:
>   	soc_camera_free_user_formats(icd);
>   eusrfmt:
> +	media_entity_cleanup(&icd->vdev->entity);
>   	soc_camera_remove_device(icd);
>
>   	return ret;
> @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct soc_camera_device *icd)
>   	if (icd->num_user_formats)
>   		soc_camera_free_user_formats(icd);
>
> +	if (icd->vdev->entity.num_pads) {
> +		media_entity_cleanup(&icd->vdev->entity);
> +	}
> +

    Brackets not needed here, and checkpatch.pl should have complained about that.

WBR, Sergei

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Hans Verkuil]

On 05/20/2015 06:39 PM, William Towle wrote:
> Add detection of source pad number for drivers aware of the media
> controller API, so that soc_camera/rcar_vin can create device nodes
> to support a driver such as adv7604.c (for HDMI on Lager) underneath.
> 
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
>  drivers/media/platform/soc_camera/soc_camera.c |   27 +++++++++++++++++++++++-
>  include/media/soc_camera.h                     |    1 +
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 0f67646..b4e9b43 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
>  		struct v4l2_mbus_framefmt *mf = &fmt.format;
>  		struct v4l2_rect rect;
>  		struct device *dev = icd->parent;
> +		struct media_pad *remote_pad;
>  		int shift;
>  
> +		remote_pad = media_entity_remote_pad(
> +					&icd->vdev->entity.pads[0]);
> +		fmt.pad = remote_pad->index;

This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media calls would all
have to be under #ifdef CONFIG_MEDIA_CONTROLLER.

Unfortunately, if it is not defined, then you still have no way of finding the source pad.

Laurent, do you think if it would make sense to add a new subdev core op that will return
the default source pad (I'm saying 'default' in case there are more) of a subdev? That way
it can be used in non-MC drivers. We never needed the source pad before, but now we do,
and this op only needs to be implemented if the default source pad != 0.

Regards,

	Hans


>  		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>  		if (ret < 0)
>  			return ret;
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index d708df4..126d645 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>  	};
>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> +	int src_pad_idx = -1;
>  	int ret;
>  
>  	sd->grp_id = soc_camera_grp_id(icd);
> @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  	}
>  
>  	/* At this point client .probe() should have run already */
> -	ret = soc_camera_init_user_formats(icd);
> +	ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0);
> +	if (!ret) {
> +		for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads;
> +				src_pad_idx++)
> +			if (sd->entity.pads[src_pad_idx].flags
> +						== MEDIA_PAD_FL_SOURCE)
> +				break;
> +
> +		if (src_pad_idx < sd->entity.num_pads) {
> +			if (!media_entity_create_link(
> +				&icd->vdev->entity, 0,
> +				&sd->entity, src_pad_idx,
> +				MEDIA_LNK_FL_IMMUTABLE |
> +				MEDIA_LNK_FL_ENABLED)) {
> +				ret = soc_camera_init_user_formats(icd);
> +			}
> +		}
> +	}
> +
>  	if (ret < 0)
>  		goto eusrfmt;
>  
> @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  		goto evidstart;
>  
>  	/* Try to improve our guess of a reasonable window format */
> +	fmt.pad = src_pad_idx;
>  	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
>  		icd->user_width		= mf->width;
>  		icd->user_height	= mf->height;
> @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  evidstart:
>  	soc_camera_free_user_formats(icd);
>  eusrfmt:
> +	media_entity_cleanup(&icd->vdev->entity);
>  	soc_camera_remove_device(icd);
>  
>  	return ret;
> @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct soc_camera_device *icd)
>  	if (icd->num_user_formats)
>  		soc_camera_free_user_formats(icd);
>  
> +	if (icd->vdev->entity.num_pads) {
> +		media_entity_cleanup(&icd->vdev->entity);
> +	}
> +
>  	if (icd->clk) {
>  		/* For the synchronous case */
>  		v4l2_clk_unregister(icd->clk);
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 2f6261f..f0c5238 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -42,6 +42,7 @@ struct soc_camera_device {
>  	unsigned char devnum;		/* Device number per host */
>  	struct soc_camera_sense *sense;	/* See comment in struct definition */
>  	struct video_device *vdev;
> +	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	const struct soc_camera_format_xlate *current_fmt;
>  	struct soc_camera_format_xlate *user_formats;
> 

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Laurent Pinchart]

Hi Hans,

On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote:
> On 05/20/2015 06:39 PM, William Towle wrote:
> > Add detection of source pad number for drivers aware of the media
> > controller API, so that soc_camera/rcar_vin can create device nodes
> > to support a driver such as adv7604.c (for HDMI on Lager) underneath.
> > 
> > Signed-off-by: William Towle <william.towle@codethink.co.uk>
> > Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> > ---
> > 
> >  drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
> >  drivers/media/platform/soc_camera/soc_camera.c |   27 ++++++++++++++++++-
> >  include/media/soc_camera.h                     |    1 +
> >  3 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
> > b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43
> > 100644
> > --- a/drivers/media/platform/soc_camera/rcar_vin.c
> > +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> > @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct
> > soc_camera_device *icd, unsigned int idx,
> >  		struct v4l2_mbus_framefmt *mf = &fmt.format;
> >  		struct v4l2_rect rect;
> >  		struct device *dev = icd->parent;
> > +		struct media_pad *remote_pad;
> >  		int shift;
> > 
> > +		remote_pad = media_entity_remote_pad(
> > +					&icd->vdev->entity.pads[0]);
> > +		fmt.pad = remote_pad->index;
> 
> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media
> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER.
> 
> Unfortunately, if it is not defined, then you still have no way of finding
> the source pad.
> 
> Laurent, do you think if it would make sense to add a new subdev core op
> that will return the default source pad (I'm saying 'default' in case there
> are more) of a subdev? That way it can be used in non-MC drivers. We never
> needed the source pad before, but now we do, and this op only needs to be
> implemented if the default source pad != 0.

I'm not too fond of that. Is there something wrong with the method implemented 
in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER an issue ?

> >  		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> >  		if (ret < 0)
> >  			return ret;
> > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > b/drivers/media/platform/soc_camera/soc_camera.c index d708df4..126d645
> > 100644
> > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > @@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct
> > soc_camera_device *icd)
> >  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >  	};
> >  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> > +	int src_pad_idx = -1;
> >  	int ret;
> >  	
> >  	sd->grp_id = soc_camera_grp_id(icd);
> > @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct
> > soc_camera_device *icd)
> >  	}
> >  	
> >  	/* At this point client .probe() should have run already */
> > -	ret = soc_camera_init_user_formats(icd);
> > +	ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0);
> > +	if (!ret) {
> > +		for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads;
> > +				src_pad_idx++)
> > +			if (sd->entity.pads[src_pad_idx].flags
> > +						== MEDIA_PAD_FL_SOURCE)
> > +				break;
> > +
> > +		if (src_pad_idx < sd->entity.num_pads) {
> > +			if (!media_entity_create_link(
> > +				&icd->vdev->entity, 0,
> > +				&sd->entity, src_pad_idx,
> > +				MEDIA_LNK_FL_IMMUTABLE |
> > +				MEDIA_LNK_FL_ENABLED)) {
> > +				ret = soc_camera_init_user_formats(icd);
> > +			}
> > +		}
> > +	}
> > +
> >  	if (ret < 0)
> >  		goto eusrfmt;
> > 
> > @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct
> > soc_camera_device *icd)
> >  		goto evidstart;
> >  	
> >  	/* Try to improve our guess of a reasonable window format */
> > +	fmt.pad = src_pad_idx;
> >  	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
> >  		icd->user_width		= mf->width;
> >  		icd->user_height	= mf->height;
> > @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct
> > soc_camera_device *icd)
> >  evidstart:
> >  	soc_camera_free_user_formats(icd);
> >  
> >  eusrfmt:
> > +	media_entity_cleanup(&icd->vdev->entity);
> >  	soc_camera_remove_device(icd);
> >  	
> >  	return ret;
> > @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct
> > soc_camera_device *icd)> 
> >  	if (icd->num_user_formats)
> >  		soc_camera_free_user_formats(icd);
> > 
> > +	if (icd->vdev->entity.num_pads) {
> > +		media_entity_cleanup(&icd->vdev->entity);
> > +	}
> > +
> >  	if (icd->clk) {
> >  		/* For the synchronous case */
> >  		v4l2_clk_unregister(icd->clk);
> > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > index 2f6261f..f0c5238 100644
> > --- a/include/media/soc_camera.h
> > +++ b/include/media/soc_camera.h
> > @@ -42,6 +42,7 @@ struct soc_camera_device {
> >  	unsigned char devnum;		/* Device number per host */
> >  	struct soc_camera_sense *sense;	/* See comment in struct definition
> >  	*/
> >  	struct video_device *vdev;
> > +	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	const struct soc_camera_format_xlate *current_fmt;
> >  	struct soc_camera_format_xlate *user_formats;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Hans Verkuil]

On 05/23/2015 08:32 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote:
>> On 05/20/2015 06:39 PM, William Towle wrote:
>>> Add detection of source pad number for drivers aware of the media
>>> controller API, so that soc_camera/rcar_vin can create device nodes
>>> to support a driver such as adv7604.c (for HDMI on Lager) underneath.
>>>
>>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
>>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
>>> ---
>>>
>>>  drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
>>>  drivers/media/platform/soc_camera/soc_camera.c |   27 ++++++++++++++++++-
>>>  include/media/soc_camera.h                     |    1 +
>>>  3 files changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
>>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43
>>> 100644
>>> --- a/drivers/media/platform/soc_camera/rcar_vin.c
>>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
>>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct
>>> soc_camera_device *icd, unsigned int idx,
>>>  		struct v4l2_mbus_framefmt *mf = &fmt.format;
>>>  		struct v4l2_rect rect;
>>>  		struct device *dev = icd->parent;
>>> +		struct media_pad *remote_pad;
>>>  		int shift;
>>>
>>> +		remote_pad = media_entity_remote_pad(
>>> +					&icd->vdev->entity.pads[0]);
>>> +		fmt.pad = remote_pad->index;
>>
>> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media
>> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER.
>>
>> Unfortunately, if it is not defined, then you still have no way of finding
>> the source pad.
>>
>> Laurent, do you think if it would make sense to add a new subdev core op
>> that will return the default source pad (I'm saying 'default' in case there
>> are more) of a subdev? That way it can be used in non-MC drivers. We never
>> needed the source pad before, but now we do, and this op only needs to be
>> implemented if the default source pad != 0.
> 
> I'm not too fond of that. Is there something wrong with the method implemented 
> in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER an issue ?

1) it's a heck of a lot of code just to get a simple source pad that the subdev
knows anyway,

2) soc-camera doesn't use the media controller today, so this would add a
dependency on the mc just for this,

3) it doesn't actually make a media device, it is just fakes enough to make the
subdev think it there is an MC.

It doesn't actually have to be a new op, it could be a new field in v4l2_subdev
as well. For those bridge drivers that do not use the MC but do need to know
the source pad this is important information.

It might even simplify the device tree if the default source pad is implied
unless stated otherwise (but that might be a step too far).

I wonder if a default input pad might also be useful to expose. I suspect this
might become important if we want to add MC support to all existing v4l2 drivers.

Regards,

	Hans

> 
>>>  		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>>>  		if (ret < 0)
>>>  			return ret;
>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
>>> b/drivers/media/platform/soc_camera/soc_camera.c index d708df4..126d645
>>> 100644
>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>> @@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct
>>> soc_camera_device *icd)
>>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>  	};
>>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
>>> +	int src_pad_idx = -1;
>>>  	int ret;
>>>  	
>>>  	sd->grp_id = soc_camera_grp_id(icd);
>>> @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct
>>> soc_camera_device *icd)
>>>  	}
>>>  	
>>>  	/* At this point client .probe() should have run already */
>>> -	ret = soc_camera_init_user_formats(icd);
>>> +	ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0);
>>> +	if (!ret) {
>>> +		for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads;
>>> +				src_pad_idx++)
>>> +			if (sd->entity.pads[src_pad_idx].flags
>>> +						== MEDIA_PAD_FL_SOURCE)
>>> +				break;
>>> +
>>> +		if (src_pad_idx < sd->entity.num_pads) {
>>> +			if (!media_entity_create_link(
>>> +				&icd->vdev->entity, 0,
>>> +				&sd->entity, src_pad_idx,
>>> +				MEDIA_LNK_FL_IMMUTABLE |
>>> +				MEDIA_LNK_FL_ENABLED)) {
>>> +				ret = soc_camera_init_user_formats(icd);
>>> +			}
>>> +		}
>>> +	}
>>> +
>>>  	if (ret < 0)
>>>  		goto eusrfmt;
>>>
>>> @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct
>>> soc_camera_device *icd)
>>>  		goto evidstart;
>>>  	
>>>  	/* Try to improve our guess of a reasonable window format */
>>> +	fmt.pad = src_pad_idx;
>>>  	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
>>>  		icd->user_width		= mf->width;
>>>  		icd->user_height	= mf->height;
>>> @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct
>>> soc_camera_device *icd)
>>>  evidstart:
>>>  	soc_camera_free_user_formats(icd);
>>>  
>>>  eusrfmt:
>>> +	media_entity_cleanup(&icd->vdev->entity);
>>>  	soc_camera_remove_device(icd);
>>>  	
>>>  	return ret;
>>> @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct
>>> soc_camera_device *icd)> 
>>>  	if (icd->num_user_formats)
>>>  		soc_camera_free_user_formats(icd);
>>>
>>> +	if (icd->vdev->entity.num_pads) {
>>> +		media_entity_cleanup(&icd->vdev->entity);
>>> +	}
>>> +
>>>  	if (icd->clk) {
>>>  		/* For the synchronous case */
>>>  		v4l2_clk_unregister(icd->clk);
>>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
>>> index 2f6261f..f0c5238 100644
>>> --- a/include/media/soc_camera.h
>>> +++ b/include/media/soc_camera.h
>>> @@ -42,6 +42,7 @@ struct soc_camera_device {
>>>  	unsigned char devnum;		/* Device number per host */
>>>  	struct soc_camera_sense *sense;	/* See comment in struct definition
>>>  	*/
>>>  	struct video_device *vdev;
>>> +	struct media_pad pad;
>>>  	struct v4l2_ctrl_handler ctrl_handler;
>>>  	const struct soc_camera_format_xlate *current_fmt;
>>>  	struct soc_camera_format_xlate *user_formats;
> 

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Laurent Pinchart]

Hi Hans,

On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote:
> On 05/23/2015 08:32 PM, Laurent Pinchart wrote:
> > On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote:
> >> On 05/20/2015 06:39 PM, William Towle wrote:
> >>> Add detection of source pad number for drivers aware of the media
> >>> controller API, so that soc_camera/rcar_vin can create device nodes
> >>> to support a driver such as adv7604.c (for HDMI on Lager) underneath.
> >>> 
> >>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> >>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> >>> ---
> >>> 
> >>>  drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
> >>>  drivers/media/platform/soc_camera/soc_camera.c |   27 ++++++++++++++++-
> >>>  include/media/soc_camera.h                     |    1 +
> >>>  3 files changed, 31 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
> >>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43
> >>> 100644
> >>> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> >>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> >>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct
> >>> soc_camera_device *icd, unsigned int idx,
> >>>  		struct v4l2_mbus_framefmt *mf = &fmt.format;
> >>>  		struct v4l2_rect rect;
> >>>  		struct device *dev = icd->parent;
> >>> +		struct media_pad *remote_pad;
> >>>  		int shift;
> >>> 
> >>> +		remote_pad = media_entity_remote_pad(
> >>> +					&icd->vdev->entity.pads[0]);
> >>> +		fmt.pad = remote_pad->index;
> >> 
> >> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media
> >> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER.
> >> 
> >> Unfortunately, if it is not defined, then you still have no way of
> >> finding the source pad.
> >> 
> >> Laurent, do you think if it would make sense to add a new subdev core op
> >> that will return the default source pad (I'm saying 'default' in case
> >> there are more) of a subdev? That way it can be used in non-MC drivers.
> >> We never needed the source pad before, but now we do, and this op only
> >> needs to be implemented if the default source pad != 0.
> > 
> > I'm not too fond of that. Is there something wrong with the method
> > implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER
> > an issue ?
>
> 1) it's a heck of a lot of code just to get a simple source pad that the
> subdev knows anyway,

I don't think the subdev knows. If a subdev has multiple source pads there's 
no concept of a default source. It all depends on how the subdevs are 
connected, and media_entity_remote_pad() is the right way to find out.
 
> 2) soc-camera doesn't use the media controller today, so this would add a
> dependency on the mc just for this,

I agree that we shouldn't pull the whole MC userspace API in just for this, 
but the kernel side of the API should be available as pad-level operations 
depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option in two.

> 3) it doesn't actually make a media device, it is just fakes enough to make
> the subdev think it there is an MC.
> 
> It doesn't actually have to be a new op, it could be a new field in
> v4l2_subdev as well. For those bridge drivers that do not use the MC but do
> need to know the source pad this is important information.
> 
> It might even simplify the device tree if the default source pad is implied
> unless stated otherwise (but that might be a step too far).
> 
> I wonder if a default input pad might also be useful to expose. I suspect
> this might become important if we want to add MC support to all existing
> v4l2 drivers.

The concept of a default sink pad makes more sense, but I'm not sure I like it 
too much either. I'd have to think about it.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Hans Verkuil]

On 05/24/2015 11:50 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote:
>> On 05/23/2015 08:32 PM, Laurent Pinchart wrote:
>>> On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote:
>>>> On 05/20/2015 06:39 PM, William Towle wrote:
>>>>> Add detection of source pad number for drivers aware of the media
>>>>> controller API, so that soc_camera/rcar_vin can create device nodes
>>>>> to support a driver such as adv7604.c (for HDMI on Lager) underneath.
>>>>>
>>>>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
>>>>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
>>>>> ---
>>>>>
>>>>>  drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
>>>>>  drivers/media/platform/soc_camera/soc_camera.c |   27 ++++++++++++++++-
>>>>>  include/media/soc_camera.h                     |    1 +
>>>>>  3 files changed, 31 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
>>>>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43
>>>>> 100644
>>>>> --- a/drivers/media/platform/soc_camera/rcar_vin.c
>>>>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
>>>>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct
>>>>> soc_camera_device *icd, unsigned int idx,
>>>>>  		struct v4l2_mbus_framefmt *mf = &fmt.format;
>>>>>  		struct v4l2_rect rect;
>>>>>  		struct device *dev = icd->parent;
>>>>> +		struct media_pad *remote_pad;
>>>>>  		int shift;
>>>>>
>>>>> +		remote_pad = media_entity_remote_pad(
>>>>> +					&icd->vdev->entity.pads[0]);
>>>>> +		fmt.pad = remote_pad->index;
>>>>
>>>> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media
>>>> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER.
>>>>
>>>> Unfortunately, if it is not defined, then you still have no way of
>>>> finding the source pad.
>>>>
>>>> Laurent, do you think if it would make sense to add a new subdev core op
>>>> that will return the default source pad (I'm saying 'default' in case
>>>> there are more) of a subdev? That way it can be used in non-MC drivers.
>>>> We never needed the source pad before, but now we do, and this op only
>>>> needs to be implemented if the default source pad != 0.
>>>
>>> I'm not too fond of that. Is there something wrong with the method
>>> implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER
>>> an issue ?
>>
>> 1) it's a heck of a lot of code just to get a simple source pad that the
>> subdev knows anyway,
> 
> I don't think the subdev knows. If a subdev has multiple source pads there's 
> no concept of a default source. It all depends on how the subdevs are 
> connected, and media_entity_remote_pad() is the right way to find out.

Note that with 'source pad' I am referring to the output pad of a subdev
(ADV7604_PAD_SOURCE in the case of the adv7604). There may be some confusion
here.

In my experience subdevs in a capture path have usually multiple input (sink)
pads, but only one output (source) pad. Subdevs in a video output path tend to
have one input (sink) pad and multiple output (source) pads.

The multiple inputs/outputs are things like composite, S-Video, HDMI, VGA, etc.
and the single input/output pad is where the device is hooked up to the mediabus
which in turn connects to a DMA engine.

>> 2) soc-camera doesn't use the media controller today, so this would add a
>> dependency on the mc just for this,
> 
> I agree that we shouldn't pull the whole MC userspace API in just for this, 
> but the kernel side of the API should be available as pad-level operations 
> depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option in two.

The way it is now is pretty OK. We just miss the information about the pad
that feeds the dma capture path and for output we miss the the pad that is fed
by the dma output path.

Bridge drivers currently just assume pad 0 in all cases, but that's obviously
not always right as the adv7604 illustrates.

The alternative would be to just hardcode it in platform data or card information.
What this patch does is just enumerating pads until it finds the first one that
fits the criteria, which fails as well if there are multiple pads that would
match and is a lot more code than just have the subdev provide the information.

Regards,

	Hans

>> 3) it doesn't actually make a media device, it is just fakes enough to make
>> the subdev think it there is an MC.
>>
>> It doesn't actually have to be a new op, it could be a new field in
>> v4l2_subdev as well. For those bridge drivers that do not use the MC but do
>> need to know the source pad this is important information.
>>
>> It might even simplify the device tree if the default source pad is implied
>> unless stated otherwise (but that might be a step too far).
>>
>> I wonder if a default input pad might also be useful to expose. I suspect
>> this might become important if we want to add MC support to all existing
>> v4l2 drivers.
> 
> The concept of a default sink pad makes more sense, but I'm not sure I like it 
> too much either. I'd have to think about it.
> 

Re: [PATCH 08/20] media: soc_camera pad-aware driver initialisation

[Laurent Pinchart]

Hi Hans,

On Monday 25 May 2015 00:43:25 Hans Verkuil wrote:
> On 05/24/2015 11:50 PM, Laurent Pinchart wrote:
> > On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote:
> >> On 05/23/2015 08:32 PM, Laurent Pinchart wrote:
> >>> On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote:
> >>>> On 05/20/2015 06:39 PM, William Towle wrote:
> >>>>> Add detection of source pad number for drivers aware of the media
> >>>>> controller API, so that soc_camera/rcar_vin can create device nodes
> >>>>> to support a driver such as adv7604.c (for HDMI on Lager) underneath.
> >>>>> 
> >>>>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> >>>>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/platform/soc_camera/rcar_vin.c   |    4 ++++
> >>>>>  drivers/media/platform/soc_camera/soc_camera.c |   27 ++++++++++++++-
> >>>>>  include/media/soc_camera.h                     |    1 +
> >>>>>  3 files changed, 31 insertions(+), 1 deletion(-)
> >>>>> 
> >>>>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43
> >>>>> 100644
> >>>>> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct
> >>>>> soc_camera_device *icd, unsigned int idx,
> >>>>>  		struct v4l2_mbus_framefmt *mf = &fmt.format;
> >>>>>  		struct v4l2_rect rect;
> >>>>>  		struct device *dev = icd->parent;
> >>>>> +		struct media_pad *remote_pad;
> >>>>>  		int shift;
> >>>>> 
> >>>>> +		remote_pad = media_entity_remote_pad(
> >>>>> +					&icd->vdev->entity.pads[0]);
> >>>>> +		fmt.pad = remote_pad->index;
> >>>> 
> >>>> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these
> >>>> media calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER.
> >>>> 
> >>>> Unfortunately, if it is not defined, then you still have no way of
> >>>> finding the source pad.
> >>>> 
> >>>> Laurent, do you think if it would make sense to add a new subdev core
> >>>> op that will return the default source pad (I'm saying 'default' in
> >>>> case there are more) of a subdev? That way it can be used in non-MC
> >>>> drivers. We never needed the source pad before, but now we do, and this
> >>>> op only needs to be implemented if the default source pad != 0.
> >>> 
> >>> I'm not too fond of that. Is there something wrong with the method
> >>> implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER
> >>> an issue ?
> >> 
> >> 1) it's a heck of a lot of code just to get a simple source pad that the
> >> subdev knows anyway,
> > 
> > I don't think the subdev knows. If a subdev has multiple source pads
> > there's no concept of a default source. It all depends on how the subdevs
> > are connected, and media_entity_remote_pad() is the right way to find
> > out.
>
> Note that with 'source pad' I am referring to the output pad of a subdev
> (ADV7604_PAD_SOURCE in the case of the adv7604). There may be some confusion
> here.

That's what I had understood :-)

> In my experience subdevs in a capture path have usually multiple input
> (sink) pads, but only one output (source) pad. Subdevs in a video output
> path tend to have one input (sink) pad and multiple output (source) pads.
> 
> The multiple inputs/outputs are things like composite, S-Video, HDMI, VGA,
> etc. and the single input/output pad is where the device is hooked up to
> the mediabus which in turn connects to a DMA engine.

There's still no concept of a "default source" in the general case, but I 
assume you want to target the most common case where the subdev will have a 
single source pad ? A new operation (or field) could possibly work in some 
cases, but I feel it's a hack that lacks genericity. The above change is 
pretty simple from a bridge driver point of view, it's a single function call 
that will return the index of the connected pad. Sure, there's a little bit 
more core behind it compared to reading a fixed value, but it doesn't seem 
much to me, especially given that, in most of the cases, there will be a 
single link to check.

> >> 2) soc-camera doesn't use the media controller today, so this would add a
> >> dependency on the mc just for this,
> > 
> > I agree that we shouldn't pull the whole MC userspace API in just for
> > this, but the kernel side of the API should be available as pad-level
> > operations depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option
> > in two.
> 
> The way it is now is pretty OK. We just miss the information about the pad
> that feeds the dma capture path and for output we miss the the pad that is
> fed by the dma output path.
> 
> Bridge drivers currently just assume pad 0 in all cases, but that's
> obviously not always right as the adv7604 illustrates.

It's indeed a bad assumption. Drivers that are MC-aware use the correct pad 
numbers. For drivers that are getting ported to the pad-level API without 
being MC-aware it's tempting to just hardcode the pad number to 0 as that's a 
common case when dealing with sensors, but that's plain wrong.

> The alternative would be to just hardcode it in platform data or card
> information. What this patch does is just enumerating pads until it finds
> the first one that fits the criteria, which fails as well if there are
> multiple pads that would match and is a lot more code than just have the
> subdev provide the information.

media_entity_remote_source() doesn't enumerate pads, it loops over links, so 
as long as links are set up properly there should be no issue, even if the 
remote subdev has multiple source pads. That's why I believe 
media_entity_remote_source() is the way to go.

> >> 3) it doesn't actually make a media device, it is just fakes enough to
> >> make the subdev think it there is an MC.
> >> 
> >> It doesn't actually have to be a new op, it could be a new field in
> >> v4l2_subdev as well. For those bridge drivers that do not use the MC but
> >> do need to know the source pad this is important information.
> >> 
> >> It might even simplify the device tree if the default source pad is
> >> implied unless stated otherwise (but that might be a step too far).
> >> 
> >> I wonder if a default input pad might also be useful to expose. I suspect
> >> this might become important if we want to add MC support to all existing
> >> v4l2 drivers.
> > 
> > The concept of a default sink pad makes more sense, but I'm not sure I
> > like it too much either. I'd have to think about it.

-- 
Regards,

Laurent Pinchart

[PATCH 09/20] media: rcar_vin: Use correct pad number in try_fmt

[William Towle]

From: Rob Taylor <rob.taylor@codethink.co.uk>

Fix rcar_vin_try_fmt to use the correct pad number when calling the
subdev set_fmt. Previously pad number 0 was always used, resulting in
EINVAL if the subdev cares about the pad number (e.g. ADV7612).

Signed-off-by: William Towle  Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   29 +++++++++++++++++---------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index b4e9b43..571ab20 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -1707,12 +1707,13 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
 	const struct soc_camera_format_xlate *xlate;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
-	struct v4l2_subdev_pad_config pad_cfg;
+	struct v4l2_subdev_pad_config pad_cfg[sd->entity.num_pads];
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
 	struct v4l2_mbus_framefmt *mf = &format.format;
 	__u32 pixfmt = pix->pixelformat;
+	struct media_pad *remote_pad;
 	int width, height;
 	int ret;
 
@@ -1744,17 +1745,24 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
 	mf->code = xlate->code;
 	mf->colorspace = pix->colorspace;
 
-	ret = v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
-					 pad, set_fmt, &pad_cfg, &format);
+	remote_pad = media_entity_remote_pad(
+				&icd->vdev->entity.pads[0]);
+	format.pad = remote_pad->index;
+
+	ret = v4l2_device_call_until_err(sd->v4l2_dev,
+					soc_camera_grp_id(icd), pad,
+					set_fmt, pad_cfg,
+					&format);
 	if (ret < 0)
 		return ret;
 
-	/* Adjust only if VIN cannot scale */
-	if (pix->width > mf->width * 2)
-		pix->width = mf->width * 2;
-	if (pix->height > mf->height * 3)
-		pix->height = mf->height * 3;
-
+	/*  In case the driver has adjusted 'fmt' to match the
+	 *  resolution of the live stream, 'pix' needs to pass this
+	 *  change out so that the buffer userland creates for the
+	 *  captured image/video has these dimensions
+	 */
+	pix->width = mf->width;
+	pix->height = mf->height;
 	pix->field = mf->field;
 	pix->colorspace = mf->colorspace;
 
@@ -1769,9 +1777,10 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
 			 */
 			mf->width = VIN_MAX_WIDTH;
 			mf->height = VIN_MAX_HEIGHT;
+			format.pad = remote_pad->index;
 			ret = v4l2_device_call_until_err(sd->v4l2_dev,
 							 soc_camera_grp_id(icd),
-							 pad, set_fmt, &pad_cfg,
+							 pad, set_fmt, pad_cfg,
 							 &format);
 			if (ret < 0) {
 				dev_err(icd->parent,
-- 
1.7.10.4

Re: [PATCH 09/20] media: rcar_vin: Use correct pad number in try_fmt

[Hans Verkuil]

On 05/20/2015 06:39 PM, William Towle wrote:
> From: Rob Taylor <rob.taylor@codethink.co.uk>
> 
> Fix rcar_vin_try_fmt to use the correct pad number when calling the
> subdev set_fmt. Previously pad number 0 was always used, resulting in
> EINVAL if the subdev cares about the pad number (e.g. ADV7612).
> 
> Signed-off-by: William Towle  Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |   29 +++++++++++++++++---------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index b4e9b43..571ab20 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -1707,12 +1707,13 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>  	const struct soc_camera_format_xlate *xlate;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> -	struct v4l2_subdev_pad_config pad_cfg;
> +	struct v4l2_subdev_pad_config pad_cfg[sd->entity.num_pads];

Same problem: this relies on the presence of CONFIG_MEDIA_CONTROLLER.
This array can also get large which is bad when it is on the stack.

Laurent, I remember that you had plans to add an op that would allocate
and initialize this for you. Any progress on that?

Regards,

	Hans

>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
>  	struct v4l2_mbus_framefmt *mf = &format.format;
>  	__u32 pixfmt = pix->pixelformat;
> +	struct media_pad *remote_pad;
>  	int width, height;
>  	int ret;
>  
> @@ -1744,17 +1745,24 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>  	mf->code = xlate->code;
>  	mf->colorspace = pix->colorspace;
>  
> -	ret = v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
> -					 pad, set_fmt, &pad_cfg, &format);
> +	remote_pad = media_entity_remote_pad(
> +				&icd->vdev->entity.pads[0]);
> +	format.pad = remote_pad->index;
> +
> +	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +					soc_camera_grp_id(icd), pad,
> +					set_fmt, pad_cfg,
> +					&format);
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Adjust only if VIN cannot scale */
> -	if (pix->width > mf->width * 2)
> -		pix->width = mf->width * 2;
> -	if (pix->height > mf->height * 3)
> -		pix->height = mf->height * 3;
> -
> +	/*  In case the driver has adjusted 'fmt' to match the
> +	 *  resolution of the live stream, 'pix' needs to pass this
> +	 *  change out so that the buffer userland creates for the
> +	 *  captured image/video has these dimensions
> +	 */
> +	pix->width = mf->width;
> +	pix->height = mf->height;
>  	pix->field = mf->field;
>  	pix->colorspace = mf->colorspace;
>  
> @@ -1769,9 +1777,10 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>  			 */
>  			mf->width = VIN_MAX_WIDTH;
>  			mf->height = VIN_MAX_HEIGHT;
> +			format.pad = remote_pad->index;
>  			ret = v4l2_device_call_until_err(sd->v4l2_dev,
>  							 soc_camera_grp_id(icd),
> -							 pad, set_fmt, &pad_cfg,
> +							 pad, set_fmt, pad_cfg,
>  							 &format);
>  			if (ret < 0) {
>  				dev_err(icd->parent,
> 

Re: [PATCH 09/20] media: rcar_vin: Use correct pad number in try_fmt

[Laurent Pinchart]

Hi Hans,

On Thursday 21 May 2015 07:58:18 Hans Verkuil wrote:
> On 05/20/2015 06:39 PM, William Towle wrote:
> > From: Rob Taylor <rob.taylor@codethink.co.uk>
> > 
> > Fix rcar_vin_try_fmt to use the correct pad number when calling the
> > subdev set_fmt. Previously pad number 0 was always used, resulting in
> > EINVAL if the subdev cares about the pad number (e.g. ADV7612).
> > 
> > Signed-off-by: William Towle  Taylor <rob.taylor@codethink.co.uk>
> > Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
> > ---
> > 
> >  drivers/media/platform/soc_camera/rcar_vin.c |   29 +++++++++++++-------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
> > b/drivers/media/platform/soc_camera/rcar_vin.c index b4e9b43..571ab20
> > 100644
> > --- a/drivers/media/platform/soc_camera/rcar_vin.c
> > +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> > @@ -1707,12 +1707,13 @@ static int rcar_vin_try_fmt(struct
> > soc_camera_device *icd,> 
> >  	const struct soc_camera_format_xlate *xlate;
> >  	struct v4l2_pix_format *pix = &f->fmt.pix;
> >  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > 
> > -	struct v4l2_subdev_pad_config pad_cfg;
> > +	struct v4l2_subdev_pad_config pad_cfg[sd->entity.num_pads];
> 
> Same problem: this relies on the presence of CONFIG_MEDIA_CONTROLLER.
> This array can also get large which is bad when it is on the stack.
> 
> Laurent, I remember that you had plans to add an op that would allocate
> and initialize this for you. Any progress on that?

You have it in your mailbox :-)

> >  	struct v4l2_subdev_format format = {
> >  		.which = V4L2_SUBDEV_FORMAT_TRY,
> >  	};
> >  	struct v4l2_mbus_framefmt *mf = &format.format;
> >  	__u32 pixfmt = pix->pixelformat;
> > +	struct media_pad *remote_pad;
> >  	int width, height;
> >  	int ret;
> > 
> > @@ -1744,17 +1745,24 @@ static int rcar_vin_try_fmt(struct
> > soc_camera_device *icd,
> >  	mf->code = xlate->code;
> >  	mf->colorspace = pix->colorspace;
> > 
> > -	ret = v4l2_device_call_until_err(sd->v4l2_dev, 
soc_camera_grp_id(icd),
> > -					 pad, set_fmt, &pad_cfg, &format);
> > +	remote_pad = media_entity_remote_pad(
> > +				&icd->vdev->entity.pads[0]);
> > +	format.pad = remote_pad->index;
> > +
> > +	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +					soc_camera_grp_id(icd), pad,
> > +					set_fmt, pad_cfg,
> > +					&format);
> >  	if (ret < 0)
> >  		return ret;
> > 
> > -	/* Adjust only if VIN cannot scale */
> > -	if (pix->width > mf->width * 2)
> > -		pix->width = mf->width * 2;
> > -	if (pix->height > mf->height * 3)
> > -		pix->height = mf->height * 3;
> > -
> > +	/*  In case the driver has adjusted 'fmt' to match the
> > +	 *  resolution of the live stream, 'pix' needs to pass this
> > +	 *  change out so that the buffer userland creates for the
> > +	 *  captured image/video has these dimensions
> > +	 */
> > +	pix->width = mf->width;
> > +	pix->height = mf->height;
> >  	pix->field = mf->field;
> >  	pix->colorspace = mf->colorspace;
> > 
> > @@ -1769,9 +1777,10 @@ static int rcar_vin_try_fmt(struct
> > soc_camera_device *icd,
> >  			 */
> >  			mf->width = VIN_MAX_WIDTH;
> >  			mf->height = VIN_MAX_HEIGHT;
> > +			format.pad = remote_pad->index;
> >  			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> >  							 soc_camera_grp_id(icd),
> > -							 pad, set_fmt, &pad_cfg,
> > +							 pad, set_fmt, pad_cfg,
> >  							 &format);
> >  			if (ret < 0) {
> >  				dev_err(icd->parent,

-- 
Regards,

Laurent Pinchart

[PATCH 10/20] media: soc_camera: soc_scale_crop: Use correct pad when calling subdev try_fmt

[William Towle]

From: Rob Taylor <rob.taylor@codethink.co.uk>

Fix calls to subdev try_fmt to use correct pad. Fixes failures with
subdevs that care about having the right pad number set.

Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_scale_crop.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
index bda29bc..d8d32ea 100644
--- a/drivers/media/platform/soc_camera/soc_scale_crop.c
+++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
@@ -224,6 +224,10 @@ static int client_set_fmt(struct soc_camera_device *icd,
 	struct v4l2_cropcap cap;
 	bool host_1to1;
 	int ret;
+	struct media_pad *remote_pad;
+
+	remote_pad = media_entity_remote_pad(&icd->vdev->entity.pads[0]);
+	format->pad = remote_pad->index;
 
 	ret = v4l2_device_call_until_err(sd->v4l2_dev,
 					 soc_camera_grp_id(icd), pad,
@@ -261,10 +265,17 @@ static int client_set_fmt(struct soc_camera_device *icd,
 	/* width <= max_width && height <= max_height - guaranteed by try_fmt */
 	while ((width > tmp_w || height > tmp_h) &&
 	       tmp_w < max_width && tmp_h < max_height) {
+		struct media_pad *remote_pad;
+
 		tmp_w = min(2 * tmp_w, max_width);
 		tmp_h = min(2 * tmp_h, max_height);
 		mf->width = tmp_w;
 		mf->height = tmp_h;
+
+		remote_pad = media_entity_remote_pad(
+			&icd->vdev->entity.pads[0]);
+		format->pad = remote_pad->index;
+
 		ret = v4l2_device_call_until_err(sd->v4l2_dev,
 					soc_camera_grp_id(icd), pad,
 					set_fmt, NULL, format);
-- 
1.7.10.4

[PATCH 11/20] media: soc_camera: Fill std field in enum_input

[William Towle]

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

Fill in the std field from the video_device tvnorms field in
enum_input.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_camera.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 126d645..5afbf65 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -309,11 +309,14 @@ static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 static int soc_camera_enum_input(struct file *file, void *priv,
 				 struct v4l2_input *inp)
 {
+	struct soc_camera_device *icd = file->private_data;
+
 	if (inp->index != 0)
 		return -EINVAL;
 
 	/* default is camera */
 	inp->type = V4L2_INPUT_TYPE_CAMERA;
+	inp->std = icd->vdev->tvnorms;
 	strcpy(inp->name, "Camera");
 
 	return 0;
-- 
1.7.10.4

[PATCH 12/20] media: soc_camera: Fix error reporting in expbuf

[William Towle]

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

Remove unnecessary check and fix the error code for vb1 drivers.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_camera.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 5afbf65..fd7497e 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -470,14 +470,10 @@ static int soc_camera_expbuf(struct file *file, void *priv,
 	struct soc_camera_device *icd = file->private_data;
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 
-	if (icd->streamer != file)
-		return -EBUSY;
-
 	/* videobuf2 only */
 	if (ici->ops->init_videobuf)
-		return -EINVAL;
-	else
-		return vb2_expbuf(&icd->vb2_vidq, p);
+		return -ENOTTY;
+	return vb2_expbuf(&icd->vb2_vidq, p);
 }
 
 /* Always entered with .host_lock held */
-- 
1.7.10.4

[PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[William Towle]

Fill in bus_info field and zero reserved field.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_camera.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index fd7497e..583c5e6 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
 	WARN_ON(priv != file->private_data);
 
 	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
+	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
+	memset(cap->reserved, 0, sizeof(cap->reserved));
 	return ici->ops->querycap(ici, cap);
 }
 
-- 
1.7.10.4

Re: [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[Hans Verkuil]

On 05/20/2015 06:39 PM, William Towle wrote:
> Fill in bus_info field and zero reserved field.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index fd7497e..583c5e6 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>  	WARN_ON(priv != file->private_data);
>  
>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
> +	memset(cap->reserved, 0, sizeof(cap->reserved));

Why the memset? That shouldn't be needed.

Regards,

	Hans

>  	return ici->ops->querycap(ici, cap);
>  }
>  
> 

Re: [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[Rob Taylor]

On 21/05/15 06:58, Hans Verkuil wrote:
> On 05/20/2015 06:39 PM, William Towle wrote:
>> Fill in bus_info field and zero reserved field.
>>
>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>> ---
>>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>> index fd7497e..583c5e6 100644
>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>>  	WARN_ON(priv != file->private_data);
>>  
>>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
>> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
> 
> Why the memset? That shouldn't be needed.

v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
v4l-utils v1.6.2 [1])

Thanks,
Rob

[1]
http://git.linuxtv.org/cgit.cgi/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-compliance.cpp?id=v4l-utils-1.6.2#n308
> Regards,
> 
> 	Hans
> 
>>  	return ici->ops->querycap(ici, cap);
>>  }
>>  
>>
> 

Re: [Linux-kernel] [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[Ben Hutchings]

On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote:
> On 21/05/15 06:58, Hans Verkuil wrote:
> > On 05/20/2015 06:39 PM, William Towle wrote:
> >> Fill in bus_info field and zero reserved field.
> >>
> >> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> >> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> >> ---
> >>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> >> index fd7497e..583c5e6 100644
> >> --- a/drivers/media/platform/soc_camera/soc_camera.c
> >> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> >> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
> >>  	WARN_ON(priv != file->private_data);
> >>  
> >>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
> >> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
> >> +	memset(cap->reserved, 0, sizeof(cap->reserved));
> > 
> > Why the memset? That shouldn't be needed.
> 
> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
> v4l-utils v1.6.2 [1])

I'm puzzled by that.  Isn't this function called by v4l_querycap(),
which is called by video_usercopy()?  And video_usercopy() zeroes the
entire structure before doing so, or at least it appears to be intended
to.

Anyway, if we're failing to initialise kernel memory that's copied to
user-space, that's a (usually minor) security issue and the fix ought to
be cc'd to stable.

Ben.

Re: [Linux-kernel] [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[Hans Verkuil]

On 05/29/2015 03:08 AM, Ben Hutchings wrote:
> On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote:
>> On 21/05/15 06:58, Hans Verkuil wrote:
>>> On 05/20/2015 06:39 PM, William Towle wrote:
>>>> Fill in bus_info field and zero reserved field.
>>>>
>>>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>>>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>>>> ---
>>>>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>>>> index fd7497e..583c5e6 100644
>>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>>>>  	WARN_ON(priv != file->private_data);
>>>>  
>>>>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
>>>> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
>>>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>>>
>>> Why the memset? That shouldn't be needed.
>>
>> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
>> v4l-utils v1.6.2 [1])

William, you should use the latest v4l-utils compiled from the git repo.
Unlikely to be related to this, though.

> 
> I'm puzzled by that.  Isn't this function called by v4l_querycap(),
> which is called by video_usercopy()?  And video_usercopy() zeroes the
> entire structure before doing so, or at least it appears to be intended
> to.

Right. So I don't understand this. Can you dig a bit deeper why this would
be needed here? It should not be necessary at all, so if reserved is non-zero,
then someone is writing data where it shouldn't.

Regards,

	Hans

> 
> Anyway, if we're failing to initialise kernel memory that's copied to
> user-space, that's a (usually minor) security issue and the fix ought to
> be cc'd to stable.
> 
> Ben.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Linux-kernel] [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[Rob Taylor]

On 29/05/15 11:10, Hans Verkuil wrote:
> On 05/29/2015 03:08 AM, Ben Hutchings wrote:
>> On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote:
>>> On 21/05/15 06:58, Hans Verkuil wrote:
>>>> On 05/20/2015 06:39 PM, William Towle wrote:
>>>>> Fill in bus_info field and zero reserved field.
>>>>>
>>>>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>>>>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>>>>> ---
>>>>>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>>>>> index fd7497e..583c5e6 100644
>>>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>>>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>>>>>  	WARN_ON(priv != file->private_data);
>>>>>  
>>>>>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
>>>>> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
>>>>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>>>>
>>>> Why the memset? That shouldn't be needed.
>>>
>>> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
>>> v4l-utils v1.6.2 [1])
> 
> William, you should use the latest v4l-utils compiled from the git repo.
> Unlikely to be related to this, though.
> 
>>
>> I'm puzzled by that.  Isn't this function called by v4l_querycap(),
>> which is called by video_usercopy()?  And video_usercopy() zeroes the
>> entire structure before doing so, or at least it appears to be intended
>> to.
> 
> Right. So I don't understand this. Can you dig a bit deeper why this would
> be needed here? It should not be necessary at all, so if reserved is non-zero,
> then someone is writing data where it shouldn't.


I've just done some digging (including rolling back to when I saw the
issue). Turns out the message I was attempting to 'fix' here was from an
ancient v4l2-compliance, and clearly the outcome of a late night hacking...

The memset is indeed not needed.

Thanks
Rob

Re: [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap

[Guennadi Liakhovetski]

Hi William,

On Wed, 20 May 2015, William Towle wrote:

> Fill in bus_info field and zero reserved field.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>

If you're the author and the submitter of this patch, why is Rob's Sob 
here needed? Or is he the author?

Thanks
Guennadi

> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index fd7497e..583c5e6 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>  	WARN_ON(priv != file->private_data);
>  
>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>  	return ici->ops->querycap(ici, cap);
>  }
>  
> -- 
> 1.7.10.4
> 

[PATCH 14/20] media: rcar_vin: Reject videobufs that are too small for current format

[William Towle]

In videobuf_setup reject buffers that are too small for the configured
format. Fixes v4l2-complience issue.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 571ab20..222002a 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -541,6 +541,9 @@ static int rcar_vin_videobuf_setup(struct vb2_queue *vq,
 		unsigned int bytes_per_line;
 		int ret;
 
+		if (fmt->fmt.pix.sizeimage < icd->sizeimage)
+			return -EINVAL;
+
 		xlate = soc_camera_xlate_by_fourcc(icd,
 						   fmt->fmt.pix.pixelformat);
 		if (!xlate)
-- 
1.7.10.4

Re: [PATCH 14/20] media: rcar_vin: Reject videobufs that are too small for current format

[Guennadi Liakhovetski]

On Wed, 20 May 2015, William Towle wrote:

> In videobuf_setup reject buffers that are too small for the configured
> format. Fixes v4l2-complience issue.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>

Ditto: why Rob's Sob if you're the author?

Thanks
Guennadi

> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 571ab20..222002a 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -541,6 +541,9 @@ static int rcar_vin_videobuf_setup(struct vb2_queue *vq,
>  		unsigned int bytes_per_line;
>  		int ret;
>  
> +		if (fmt->fmt.pix.sizeimage < icd->sizeimage)
> +			return -EINVAL;
> +
>  		xlate = soc_camera_xlate_by_fourcc(icd,
>  						   fmt->fmt.pix.pixelformat);
>  		if (!xlate)
> -- 
> 1.7.10.4
> 

[PATCH 15/20] media: rcar_vin: Don't advertise support for USERPTR

[William Towle]

rcar_vin requires physically contiguous buffer, so shouldn't advertise
support for USERPTR.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 222002a..b530503 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -1824,7 +1824,7 @@ static int rcar_vin_init_videobuf2(struct vb2_queue *vq,
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 
 	vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-	vq->io_modes = VB2_MMAP | VB2_USERPTR;
+	vq->io_modes = VB2_MMAP;
 	vq->drv_priv = icd;
 	vq->ops = &rcar_vin_vb2_ops;
 	vq->mem_ops = &vb2_dma_contig_memops;
-- 
1.7.10.4

Re: [PATCH 15/20] media: rcar_vin: Don't advertise support for USERPTR

[Hans Verkuil]

On 05/20/2015 06:39 PM, William Towle wrote:
> rcar_vin requires physically contiguous buffer, so shouldn't advertise
> support for USERPTR.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index 222002a..b530503 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -1824,7 +1824,7 @@ static int rcar_vin_init_videobuf2(struct vb2_queue *vq,
>  	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>  
>  	vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -	vq->io_modes = VB2_MMAP | VB2_USERPTR;
> +	vq->io_modes = VB2_MMAP;

NACK.

USERPTR can be used, but the user pointer must point to physically contig
memory (and this is checked). There are cases where the system will carve out
phys. contig. memory and userspace has pointers to that. I'm pretty sure some of
this is used by systems where soc-camera is run.

Unfortunately, userspace has currently no way of knowing such userptr restrictions.
That's a failing of the API. It's ugly as hell, but it is in use today and can't
be dropped.

Regards,

	Hans

>  	vq->drv_priv = icd;
>  	vq->ops = &rcar_vin_vb2_ops;
>  	vq->mem_ops = &vb2_dma_contig_memops;
> 

Re: [PATCH 15/20] media: rcar_vin: Don't advertise support for USERPTR

[Rob Taylor]

On 21/05/15 07:03, Hans Verkuil wrote:
> On 05/20/2015 06:39 PM, William Towle wrote:
>> rcar_vin requires physically contiguous buffer, so shouldn't advertise
>> support for USERPTR.
>>
>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>> ---
>>  drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
>> index 222002a..b530503 100644
>> --- a/drivers/media/platform/soc_camera/rcar_vin.c
>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
>> @@ -1824,7 +1824,7 @@ static int rcar_vin_init_videobuf2(struct vb2_queue *vq,
>>  	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>>  
>>  	vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> -	vq->io_modes = VB2_MMAP | VB2_USERPTR;
>> +	vq->io_modes = VB2_MMAP;
> 
> NACK.
> 
> USERPTR can be used, but the user pointer must point to physically contig
> memory (and this is checked). There are cases where the system will carve out
> phys. contig. memory and userspace has pointers to that. I'm pretty sure some of
> this is used by systems where soc-camera is run.
> 
> Unfortunately, userspace has currently no way of knowing such userptr restrictions.
> That's a failing of the API. It's ugly as hell, but it is in use today and can't
> be dropped.

Ah, that is ugly - I assumed because there wasn't an interface to query
restrictions it would be necessary to drop support.

We can drop this patch for now. It seems rcar_vin doesn't pass
compliance with USERPTR, but we'll have to look into fixing that at a
later date.

Thanks,

Rob


> Regards,
> 
> 	Hans
> 
>>  	vq->drv_priv = icd;
>>  	vq->ops = &rcar_vin_vb2_ops;
>>  	vq->mem_ops = &vb2_dma_contig_memops;
>>
> 

[PATCH 16/20] media: adv7180: Fix set_pad_format() passing wrong format

[William Towle]

Return a usable format (and resolution) from adv7180_set_pad_format()
in the TRY_FORMAT case

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Tested-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/i2c/adv7180.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 09a96df..ba0b92d5 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -686,12 +686,14 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 			adv7180_set_field_mode(state);
 			adv7180_set_power(state, true);
 		}
+		adv7180_mbus_fmt(sd, framefmt);
 	} else {
 		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
 		*framefmt = format->format;
+		adv7180_mbus_fmt(sd, framefmt);
+		format->format = *framefmt;
 	}
-
-	return adv7180_mbus_fmt(sd, framefmt);
+	return 0;
 }
 
 static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
-- 
1.7.10.4

Re: [PATCH 16/20] media: adv7180: Fix set_pad_format() passing wrong format

[Guennadi Liakhovetski]

On Wed, 20 May 2015, William Towle wrote:

> Return a usable format (and resolution) from adv7180_set_pad_format()
> in the TRY_FORMAT case
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Tested-by: William Towle <william.towle@codethink.co.uk>

Author?

> ---
>  drivers/media/i2c/adv7180.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 09a96df..ba0b92d5 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -686,12 +686,14 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
>  			adv7180_set_field_mode(state);
>  			adv7180_set_power(state, true);
>  		}
> +		adv7180_mbus_fmt(sd, framefmt);
>  	} else {
>  		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
>  		*framefmt = format->format;
> +		adv7180_mbus_fmt(sd, framefmt);
> +		format->format = *framefmt;

Wouldn't it be easier to:

+		adv7180_mbus_fmt(sd, &format->format);
 		framefmt = v4l2_subdev_get_try_format(sd, cfg, 0);
 		*framefmt = format->format;

and also make adv7180_mbus_fmt() return void?

Thanks
Guennadi

>  	}
> -
> -	return adv7180_mbus_fmt(sd, framefmt);
> +	return 0;
>  }
>  
>  static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
> -- 
> 1.7.10.4
> 

[PATCH 17/20] media: adv7604: Support V4L_FIELD_INTERLACED

[William Towle]

When hardware reports interlaced input, correctly set field to
V4L_FIELD_INTERLACED ini adv76xx_fill_format.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/i2c/adv7604.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 4bde3e1..d77ee1f 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1791,7 +1791,12 @@ static void adv76xx_fill_format(struct adv76xx_state *state,
 
 	format->width = state->timings.bt.width;
 	format->height = state->timings.bt.height;
-	format->field = V4L2_FIELD_NONE;
+
+	if (state->timings.bt.interlaced)
+		format->field= V4L2_FIELD_INTERLACED;
+	else
+		format->field= V4L2_FIELD_NONE;
+
 	format->colorspace = V4L2_COLORSPACE_SRGB;
 
 	if (state->timings.bt.flags & V4L2_DV_FL_IS_CE_VIDEO)
-- 
1.7.10.4

Re: [PATCH 17/20] media: adv7604: Support V4L_FIELD_INTERLACED

[Hans Verkuil]

On 05/20/2015 06:39 PM, William Towle wrote:
> When hardware reports interlaced input, correctly set field to
> V4L_FIELD_INTERLACED ini adv76xx_fill_format.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/i2c/adv7604.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 4bde3e1..d77ee1f 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1791,7 +1791,12 @@ static void adv76xx_fill_format(struct adv76xx_state *state,
>  
>  	format->width = state->timings.bt.width;
>  	format->height = state->timings.bt.height;
> -	format->field = V4L2_FIELD_NONE;
> +
> +	if (state->timings.bt.interlaced)
> +		format->field= V4L2_FIELD_INTERLACED;

No, this should be FIELD_ALTERNATE. FIELD_INTERLACED means that the two fields
are interlaced into a single frame buffer, with FIELD_ALTERNATE each buffer
contains one field. And when capturing v4l2_buffer should return which field
(TOP/BOTTOM) the buffer contains. It also complicates cropping/composing: the
crop rectangle is in frame coordinates, composing uses field coordinates.
The vivid driver handles this correctly and can be used as a reference.

Also, no space before the '='. Please add.

You might be interested in this patch series as well:

http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/90578

I'm thinking of doing some interlaced tests myself, possibly this weekend,
using the adv7604.

Regards,

	Hans

> +	else
> +		format->field= V4L2_FIELD_NONE;
> +
>  	format->colorspace = V4L2_COLORSPACE_SRGB;
>  
>  	if (state->timings.bt.flags & V4L2_DV_FL_IS_CE_VIDEO)
> 

Re: [PATCH 17/20] media: adv7604: Support V4L_FIELD_INTERLACED

[Rob Taylor]

On 21/05/15 07:10, Hans Verkuil wrote:
> On 05/20/2015 06:39 PM, William Towle wrote:
>> When hardware reports interlaced input, correctly set field to
>> V4L_FIELD_INTERLACED ini adv76xx_fill_format.
>>
>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>> ---
>>  drivers/media/i2c/adv7604.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>> index 4bde3e1..d77ee1f 100644
>> --- a/drivers/media/i2c/adv7604.c
>> +++ b/drivers/media/i2c/adv7604.c
>> @@ -1791,7 +1791,12 @@ static void adv76xx_fill_format(struct adv76xx_state *state,
>>  
>>  	format->width = state->timings.bt.width;
>>  	format->height = state->timings.bt.height;
>> -	format->field = V4L2_FIELD_NONE;
>> +
>> +	if (state->timings.bt.interlaced)
>> +		format->field= V4L2_FIELD_INTERLACED;
> 
> No, this should be FIELD_ALTERNATE. FIELD_INTERLACED means that the two fields
> are interlaced into a single frame buffer, with FIELD_ALTERNATE each buffer
> contains one field. And when capturing v4l2_buffer should return which field
> (TOP/BOTTOM) the buffer contains. It also complicates cropping/composing: the
> crop rectangle is in frame coordinates, composing uses field coordinates.
> The vivid driver handles this correctly and can be used as a reference.
>
>

OK, that makes sense. I think for now we'll just drop this patch from
the series until we can test this properly - its a bit undercooked..

> Also, no space before the '='. Please add.
> 
> You might be interested in this patch series as well:
> 
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/90578

Ah yes, this does look useful.

> I'm thinking of doing some interlaced tests myself, possibly this weekend,
> using the adv7604.


That would be great, happy to test anything on our side.

Thanks
Rob


> Regards,
> 
> 	Hans
> 
>> +	else
>> +		format->field= V4L2_FIELD_NONE;
>> +
>>  	format->colorspace = V4L2_COLORSPACE_SRGB;
>>  
>>  	if (state->timings.bt.flags & V4L2_DV_FL_IS_CE_VIDEO)
>>
> 

Re: [PATCH 17/20] media: adv7604: Support V4L_FIELD_INTERLACED

[Guennadi Liakhovetski]

On Wed, 20 May 2015, William Towle wrote:

> When hardware reports interlaced input, correctly set field to
> V4L_FIELD_INTERLACED ini adv76xx_fill_format.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/i2c/adv7604.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 4bde3e1..d77ee1f 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1791,7 +1791,12 @@ static void adv76xx_fill_format(struct adv76xx_state *state,
>  
>  	format->width = state->timings.bt.width;
>  	format->height = state->timings.bt.height;
> -	format->field = V4L2_FIELD_NONE;
> +
> +	if (state->timings.bt.interlaced)
> +		format->field= V4L2_FIELD_INTERLACED;
> +	else
> +		format->field= V4L2_FIELD_NONE;
> +

A space before "=" please.

Thanks
Guennadi

>  	format->colorspace = V4L2_COLORSPACE_SRGB;
>  
>  	if (state->timings.bt.flags & V4L2_DV_FL_IS_CE_VIDEO)
> -- 
> 1.7.10.4
> 

[PATCH 18/20] media: adv7604: Always query_dv_timings in adv76xx_fill_format

[William Towle]

Make sure we're always reporting the current format of the input.
Fixes start of day bugs.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Signed-off-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/i2c/adv7604.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index d77ee1f..526fa4e 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1787,8 +1787,12 @@ static int adv76xx_enum_mbus_code(struct v4l2_subdev *sd,
 static void adv76xx_fill_format(struct adv76xx_state *state,
 				struct v4l2_mbus_framefmt *format)
 {
+	struct v4l2_subdev *sd = &state->sd;
+
 	memset(format, 0, sizeof(*format));
 
+	v4l2_subdev_call(sd, video, query_dv_timings, &state->timings);
+
 	format->width = state->timings.bt.width;
 	format->height = state->timings.bt.height;
 
-- 
1.7.10.4

Re: [PATCH 18/20] media: adv7604: Always query_dv_timings in adv76xx_fill_format

[Hans Verkuil]

On 05/20/2015 06:39 PM, William Towle wrote:
> Make sure we're always reporting the current format of the input.
> Fixes start of day bugs.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/i2c/adv7604.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index d77ee1f..526fa4e 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1787,8 +1787,12 @@ static int adv76xx_enum_mbus_code(struct v4l2_subdev *sd,
>  static void adv76xx_fill_format(struct adv76xx_state *state,
>  				struct v4l2_mbus_framefmt *format)
>  {
> +	struct v4l2_subdev *sd = &state->sd;
> +
>  	memset(format, 0, sizeof(*format));
>  
> +	v4l2_subdev_call(sd, video, query_dv_timings, &state->timings);
> +

NACK.

Never use querystd/query_dv_timings in a driver. If the format changes, then the
required buffer sizes change as well. The only place this can be done correctly
is in userspace where the application has to call the query ioctl, then based on
that allocate the buffers.

Automagically changing the format from underneath the video pipeline is a recipe
for disaster.

Regards,

	Hans

>  	format->width = state->timings.bt.width;
>  	format->height = state->timings.bt.height;
>  
> 

[PATCH 19/20] media: rcar_vin: Clean up format debugging statements

[William Towle]

Pretty print fourcc and code in format debugging statements.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index b530503..0bebca5 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -39,6 +39,9 @@
 
 #include "soc_scale_crop.h"
 
+#define pixfmtstr(x) (x) & 0xff, ((x) >> 8) & 0xff, ((x) >> 16) & 0xff, \
+	((x) >> 24) & 0xff
+
 #define DRV_NAME "rcar_vin"
 
 /* Register offsets for R-Car VIN */
@@ -1352,10 +1355,12 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 
 	fmt = soc_mbus_get_fmtdesc(code.code);
 	if (!fmt) {
-		dev_warn(dev, "unsupported format code #%u: %d\n", idx, code.code);
+		dev_warn(dev, "unsupported format code #%u: %x\n", idx, code.code);
 		return 0;
 	}
 
+	dev_dbg(dev, "Supported format: %x (%c%c%c%c)", code.code, pixfmtstr(fmt->fourcc));
+
 	ret = rcar_vin_try_bus_param(icd, fmt->bits_per_sample);
 	if (ret < 0)
 		return 0;
@@ -1456,7 +1461,7 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 		for (k = 0; xlate && k < n; k++, xlate++) {
 			xlate->host_fmt = &rcar_vin_formats[k];
 			xlate->code = code.code;
-			dev_dbg(dev, "Providing format %s using code %d\n",
+			dev_dbg(dev, "Providing format %s using code %x\n",
 				rcar_vin_formats[k].name, code.code);
 		}
 		break;
@@ -1596,8 +1601,8 @@ static int rcar_vin_set_fmt(struct soc_camera_device *icd,
 	enum v4l2_field field;
 	v4l2_std_id std;
 
-	dev_dbg(dev, "S_FMT(pix=0x%x, %ux%u)\n",
-		pixfmt, pix->width, pix->height);
+	dev_dbg(dev, "S_FMT(pix=%c%c%c%c, %ux%u)\n",
+		pixfmtstr(pixfmt), pix->width, pix->height);
 
 	switch (pix->field) {
 	default:
@@ -1623,7 +1628,7 @@ static int rcar_vin_set_fmt(struct soc_camera_device *icd,
 
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
 	if (!xlate) {
-		dev_warn(dev, "Format %x not found\n", pixfmt);
+		dev_warn(dev, "Format %c%c%c%c not found\n", pixfmtstr(pixfmt));
 		return -EINVAL;
 	}
 	/* Calculate client output geometry */
@@ -1720,11 +1725,14 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
 	int width, height;
 	int ret;
 
+	dev_dbg(icd->parent, "TRY_FMT(%c%c%c%c, %ux%u)\n",
+		pixfmtstr(pix->pixelformat), pix->width, pix->height);
+
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
 	if (!xlate) {
 		xlate = icd->current_fmt;
-		dev_dbg(icd->parent, "Format %x not found, keeping %x\n",
-			pixfmt, xlate->host_fmt->fourcc);
+		dev_dbg(icd->parent, "Format %c%c%c%c not found, keeping %x\n",
+			pixfmtstr(pixfmt), xlate->host_fmt->fourcc);
 		pixfmt = xlate->host_fmt->fourcc;
 		pix->pixelformat = pixfmt;
 		pix->colorspace = icd->colorspace;
-- 
1.7.10.4

Re: [PATCH 19/20] media: rcar_vin: Clean up format debugging statements

[Guennadi Liakhovetski]

Hi William,

On Wed, 20 May 2015, William Towle wrote:

> Pretty print fourcc and code in format debugging statements.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index b530503..0bebca5 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c

[snip]

> @@ -1720,11 +1725,14 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>  	int width, height;
>  	int ret;
>  
> +	dev_dbg(icd->parent, "TRY_FMT(%c%c%c%c, %ux%u)\n",
> +		pixfmtstr(pix->pixelformat), pix->width, pix->height);
> +

Why is this additional debugging needed? Doesn't an identical call in 
soc_camera_try_fmt() already print the same information just before 
calling rcar_vin_try_fmt()?

Thanks
Guennadi

[PATCH 20/20] media: soc_camera: Add debugging for get_formats

[William Towle]

From: Rob Taylor <rob.taylor@codethink.co.uk>

Some helpful debugging for get_formats use, useful for debugging
v4l2-compliance issues.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_camera.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 583c5e6..503e9b6 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -522,7 +522,7 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 
 	/* Second pass - actually fill data formats */
 	fmts = 0;
-	for (i = 0; i < raw_fmts; i++)
+	for (i = 0; i < raw_fmts; i++) {
 		if (!ici->ops->get_formats) {
 			code.index = i;
 			v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
@@ -537,6 +537,8 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 				goto egfmt;
 			fmts += ret;
 		}
+		dev_dbg(icd->pdev, " Format: %x %c%c%c%c", icd->user_formats[fmts-1].code, pixfmtstr(icd->user_formats[fmts-1].host_fmt->fourcc));
+	}
 
 	icd->num_user_formats = fmts;
 	icd->current_fmt = &icd->user_formats[0];
@@ -732,6 +734,8 @@ static int soc_camera_open(struct file *file)
 		 * apart from someone else calling open() simultaneously, but
 		 * .host_lock is protecting us against it.
 		 */
+
+		dev_dbg(icd->pdev, "%s:%d calling set_fmt with size %d x %d",__func__, __LINE__, f.fmt.pix.width, f.fmt.pix.height);
 		ret = soc_camera_set_fmt(icd, &f);
 		if (ret < 0)
 			goto esfmt;
@@ -2234,6 +2238,7 @@ static int soc_camera_pdrv_probe(struct platform_device *pdev)
 	icd->user_width		= DEFAULT_WIDTH;
 	icd->user_height	= DEFAULT_HEIGHT;
 
+	dev_dbg(icd->pdev, "%s:%d setting default user size to %d x %d",__func__, __LINE__, icd->user_width, icd->user_height);
 	return soc_camera_device_register(icd);
 }
 
-- 
1.7.10.4

Re: [PATCH 20/20] media: soc_camera: Add debugging for get_formats

[Guennadi Liakhovetski]

Hi William,

In principle, we could add these or a bunch of other "useful" debugging 
prints, but mostly they are are "one-off" - you used them for your 
debugging, then you throw them away until the next time. Do you really 
find them so valuable? Usually when a new code fragment is added, it can 
bring some debugging with it for others, apart from the code author to be 
able to debug it easier. Adding debugging to existing code isn't that 
useful IMHO.

Thanks
Guennadi

On Wed, 20 May 2015, William Towle wrote:

> From: Rob Taylor <rob.taylor@codethink.co.uk>
> 
> Some helpful debugging for get_formats use, useful for debugging
> v4l2-compliance issues.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index 583c5e6..503e9b6 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -522,7 +522,7 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
>  
>  	/* Second pass - actually fill data formats */
>  	fmts = 0;
> -	for (i = 0; i < raw_fmts; i++)
> +	for (i = 0; i < raw_fmts; i++) {
>  		if (!ici->ops->get_formats) {
>  			code.index = i;
>  			v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> @@ -537,6 +537,8 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
>  				goto egfmt;
>  			fmts += ret;
>  		}
> +		dev_dbg(icd->pdev, " Format: %x %c%c%c%c", icd->user_formats[fmts-1].code, pixfmtstr(icd->user_formats[fmts-1].host_fmt->fourcc));
> +	}
>  
>  	icd->num_user_formats = fmts;
>  	icd->current_fmt = &icd->user_formats[0];
> @@ -732,6 +734,8 @@ static int soc_camera_open(struct file *file)
>  		 * apart from someone else calling open() simultaneously, but
>  		 * .host_lock is protecting us against it.
>  		 */
> +
> +		dev_dbg(icd->pdev, "%s:%d calling set_fmt with size %d x %d",__func__, __LINE__, f.fmt.pix.width, f.fmt.pix.height);
>  		ret = soc_camera_set_fmt(icd, &f);
>  		if (ret < 0)
>  			goto esfmt;
> @@ -2234,6 +2238,7 @@ static int soc_camera_pdrv_probe(struct platform_device *pdev)
>  	icd->user_width		= DEFAULT_WIDTH;
>  	icd->user_height	= DEFAULT_HEIGHT;
>  
> +	dev_dbg(icd->pdev, "%s:%d setting default user size to %d x %d",__func__, __LINE__, icd->user_width, icd->user_height);
>  	return soc_camera_device_register(icd);
>  }
>  
> -- 
> 1.7.10.4
>