Re: [PATCH v7 05/12] media: dt-bindings: add bindings for i.MX7 media driver
On Fri, Aug 10, 2018 at 03:20:38PM +0100, Rui Miguel Silva wrote: > Add bindings documentation for i.MX7 media drivers. > The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface. > > Reviewed-by: Rob Herring > Signed-off-by: Rui Miguel Silva Acked-by: Sakari Ailus - -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 05/12] media: dt-bindings: add bindings for i.MX7 media driver
Hi Rui,
On Wed, Nov 21, 2018 at 11:15:51AM +, Rui Miguel Silva wrote:
> Add bindings documentation for i.MX7 media drivers.
> The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface.
>
> Signed-off-by: Rui Miguel Silva
> Reviewed-by: Rob Herring
> Acked-by: Sakari Ailus
> ---
> .../devicetree/bindings/media/imx7-csi.txt| 45 ++
> .../bindings/media/imx7-mipi-csi2.txt | 90 +++
> 2 files changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/imx7-csi.txt
> create mode 100644 Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
>
> diff --git a/Documentation/devicetree/bindings/media/imx7-csi.txt
> b/Documentation/devicetree/bindings/media/imx7-csi.txt
> new file mode 100644
> index ..171b089ee91f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
> @@ -0,0 +1,45 @@
> +Freescale i.MX7 CMOS Sensor Interface
> +=
> +
> +csi node
> +
> +
> +This is device node for the CMOS Sensor Interface (CSI) which enables the
> chip
> +to connect directly to external CMOS image sensors.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-csi";
> +- reg : base address and length of the register set for the device;
> +- interrupts: should contain CSI interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for
> details;
> +- clock-names : must contain "axi", "mclk" and "dcic" entries, matching
> + entries in the clock property;
> +
> +The device node shall contain one 'port' child node with one child 'endpoint'
> +node, according to the bindings defined in:
> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +In the following example a remote endpoint is a video multiplexer.
> +
> +example:
> +
> +csi: csi@3071 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +compatible = "fsl,imx7-csi";
> +reg = <0x3071 0x1>;
> +interrupts = ;
> +clocks = <&clks IMX7D_CLK_DUMMY>,
> +<&clks IMX7D_CSI_MCLK_ROOT_CLK>,
> +<&clks IMX7D_CLK_DUMMY>;
> +clock-names = "axi", "mclk", "dcic";
> +
> +port {
> +csi_from_csi_mux: endpoint {
> +remote-endpoint = <&csi_mux_to_csi>;
> +};
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> new file mode 100644
> index ..71fd74ed3ec8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> @@ -0,0 +1,90 @@
> +Freescale i.MX7 Mipi CSI2
> +=
> +
> +mipi_csi2 node
> +--
> +
> +This is the device node for the MIPI CSI-2 receiver core in i.MX7 SoC. It is
> +compatible with previous version of Samsung D-phy.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-mipi-csi2";
> +- reg : base address and length of the register set for the device;
> +- interrupts: should contain MIPI CSIS interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for
> details;
> +- clock-names : must contain "pclk", "wrap" and "phy" entries, matching
> + entries in the clock property;
> +- power-domains : a phandle to the power domain, see
> + Documentation/devicetree/bindings/power/power_domain.txt for
> details.
> +- reset-names : should include following entry "mrst";
> +- resets: a list of phandle, should contain reset entry of
> + reset-names;
> +- phy-supply: from the generic phy bindings, a phandle to a regulator
> that
> + provides power to MIPI CSIS core;
> +
> +Optional properties:
> +
> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
> + value when this property is not specified is 166 MHz;
> +- fsl,csis-hs-settle : differential receiver (HS-RX) settle time;
&g
Re: [PATCH v8 03/12] media: staging/imx7: add imx7 CSI subdev driver
struct v4l2_async_subdev *asd)
> +{
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> +}
> +
> +static int imx7_csi_parse_dt(struct imx7_csi *csi)
> +{
> + struct device *dev = csi->dev;
> +
> + csi->clk_disp_axi = devm_clk_get(dev, "axi");
> + if (IS_ERR(csi->clk_disp_axi)) {
> + dev_err(dev, "Could not get csi axi clock\n");
> + ret
Re: [PATCH v9 04/13] media: staging/imx7: add MIPI CSI-2 receiver subdev for i.MX7
e(state);
> +
> + ret = devm_request_irq(dev, state->irq, mipi_csis_irq_handler,
> +0, dev_name(dev), state);
> + if (ret) {
> + dev_err(dev, "Interrupt request failed\n");
> + goto disable_clock;
> + }
> +
> + platform_set_drvdata(pdev, &state->mipi_sd);
> +
> + ret = mipi_csis_subdev_init(&state->mipi_sd, pdev,
> + &mipi_csis_subdev_ops);
> + if (ret < 0)
> + goto disable_clock;
> +
> + state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&state->mipi_sd.entity, CSIS_PADS_NUM,
> + state->pads);
> + if (ret < 0)
> + goto unregister_subdev;
> +
> + memcpy(state->events, mipi_csis_events, sizeof(state->events));
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = mipi_csis_pm_resume(dev, true);
> + if (ret < 0)
> + goto unregister_all;
> + }
> +
> + dev_info(&pdev->dev, "lanes: %d, hs_settle: %d, wclk: %d, freq: %u\n",
> + state->bus.num_data_lanes, state->hs_settle,
> + state->wrap_clk ? 1 : 0, state->clk_frequency);
> + return 0;
> +
> +unregister_all:
> + media_entity_cleanup(&state->mipi_sd.entity);
> +unregister_subdev:
> + v4l2_async_unregister_subdev(&state->mipi_sd);
> +disable_clock:
> + mipi_csis_clk_disable(state);
> +
> + return ret;
> +}
> +
> +static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> + int ret = 0;
> +
> + mutex_lock(&state->lock);
> + if (state->flags & ST_POWERED) {
> + mipi_csis_stop_stream(state);
> + ret = regulator_disable(state->mipi_phy_regulator);
> + if (ret)
> + goto unlock;
> + mipi_csis_clk_disable(state);
> + state->flags &= ~ST_POWERED;
> + if (!runtime)
> + state->flags |= ST_SUSPENDED;
> + }
> +
> + unlock:
> + mutex_unlock(&state->lock);
> +
> + return ret ? -EAGAIN : 0;
> +}
> +
> +static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> + int ret = 0;
> +
> + mutex_lock(&state->lock);
> + if (!runtime && !(state->flags & ST_SUSPENDED))
> + goto unlock;
> +
> + if (!(state->flags & ST_POWERED)) {
> + ret = regulator_enable(state->mipi_phy_regulator);
> + if (ret)
> + goto unlock;
> +
> + state->flags |= ST_POWERED;
> + mipi_csis_clk_enable(state);
> + }
> + if (state->flags & ST_STREAMING)
> + mipi_csis_start_stream(state);
> +
> + state->flags &= ~ST_SUSPENDED;
> +
> + unlock:
> + mutex_unlock(&state->lock);
> +
> + return ret ? -EAGAIN : 0;
> +}
> +
> +static int mipi_csis_suspend(struct device *dev)
> +{
> + return mipi_csis_pm_suspend(dev, false);
> +}
> +
> +static int mipi_csis_resume(struct device *dev)
> +{
> + return mipi_csis_pm_resume(dev, false);
> +}
> +
> +static int mipi_csis_runtime_suspend(struct device *dev)
> +{
> + return mipi_csis_pm_suspend(dev, true);
> +}
> +
> +static int mipi_csis_runtime_resume(struct device *dev)
> +{
> + return mipi_csis_pm_resume(dev, true);
> +}
> +
> +static int mipi_csis_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> +
> + v4l2_async_unregister_subdev(&state->mipi_sd);
> + v4l2_async_notifier_unregister(&state->subdev_notifier);
> +
> + pm_runtime_disable(&pdev->dev);
> + mipi_csis_pm_suspend(&pdev->dev, true);
> + mipi_csis_clk_disable(state);
> + media_entity_cleanup(&state->mipi_sd.entity);
> + pm_runtime_set_suspended(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops mipi_csis_pm_ops = {
> + SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
> +NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
> +};
> +
> +static const struct of_device_id mipi_csis_of_match[] = {
> + { .compatible = "fsl,imx7-mipi-csi2", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mipi_csis_of_match);
> +
> +static struct platform_driver mipi_csis_driver = {
> + .probe = mipi_csis_probe,
> + .remove = mipi_csis_remove,
> + .driver = {
> + .of_match_table = mipi_csis_of_match,
> + .name = CSIS_DRIVER_NAME,
> + .pm = &mipi_csis_pm_ops,
> + },
> +};
> +
> +module_platform_driver(mipi_csis_driver);
> +
> +MODULE_DESCRIPTION("i.MX7 MIPI CSI-2 Receiver driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx7-mipi-csi2");
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: atomisp: pci: reposition braces as per coding style
dx < IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT)
> - {
> + if (idx < IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT) {
> my_css.mipi_sizes_for_check[port][idx] = size_mem_words;
> err = 0;
> }
> @@ -271,7 +268,8 @@ mipi_init(void)
> int
> calculate_mipi_buff_size(
> struct ia_css_stream_config *stream_cfg,
> -unsigned int *size_mem_words) {
> +unsigned int *size_mem_words)
I think both arguments might fit on the same line. If not, then the return
type (int) does. I think there are other similar cases.
> +{
> #if !defined(ISP2401)
> int err = -EINVAL;
> (void)stream_cfg;
> @@ -346,12 +344,10 @@ calculate_mipi_buff_size(
>
> /* Even lines for YUV420 formats are double in bits_per_pixel. */
> if (format == ATOMISP_INPUT_FORMAT_YUV420_8
> - || format == ATOMISP_INPUT_FORMAT_YUV420_10)
> - {
> + || format == ATOMISP_INPUT_FORMAT_YUV420_10) {
> even_line_bytes = (width_padded * 2 * bits_per_pixel + 7) >>
> 3; /* ceil ( bits per line / 8) */
You don't need braces here.
> - } else
> - {
> + } else {
> even_line_bytes = odd_line_bytes;
> }
>
> @@ -393,7 +389,8 @@ static bool buffers_needed(struct ia_css_pipe *pipe)
>
> int
> allocate_mipi_frames(struct ia_css_pipe *pipe,
> - struct ia_css_stream_info *info) {
> + struct ia_css_stream_info *info)
> +{
> int err = -EINVAL;
> unsigned int port;
>
> @@ -402,8 +399,7 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>
> assert(pipe);
> assert(pipe->stream);
> - if ((!pipe) || (!pipe->stream))
> - {
> + if ((!pipe) || (!pipe->stream)) {
Extra parentheses.
--
Kind regards,
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: atomisp: pci: reposition braces as per coding style
Hi Deepak, On Fri, Apr 30, 2021 at 10:34:37PM +0530, Deepak R Varma wrote: > On Fri, Apr 30, 2021 at 07:33:27PM +0300, Sakari Ailus wrote: > > Hi Deepak, > > > > If you're touching all these lines, I might do a little more. Please see > > the comments below. > > > Hello Sakari, > I can definitely include other changes, but then it will be many different > types of changes into a single patch. Will that be okay? > > I was planning to address one issue per patch as I think the volume of > change is going to be high. I mentioned that in the notes section of the > patch > message. I think I'd split the patch into smaller chunks if the result becomes too big but I don't think it's necessary yet. Splitting different kinds of simple cleanups into several patches takes longer time to review when they're touching the same piece of code. As the chunks in these patches have virtually no dependencies to other chunks, it's fine to do several kinds of cleanups at once. -- Kind regards, Sakari Ailus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 00/16] Allwinner MIPI CSI-2 support for A31/V3s/A83T
Hi Paul, On Wed, May 26, 2021 at 03:28:20PM +0200, Paul Kocialkowski wrote: > Hi, > > On Wed 26 May 21, 14:00, Hans Verkuil wrote: > > Hi Paul, > > > > On 15/01/2021 21:01, Paul Kocialkowski wrote: > > > This series introduces support for MIPI CSI-2, with the A31 controller > > > that is > > > found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific > > > controller. While the former uses the same MIPI D-PHY that is already > > > supported > > > for DSI, the latter embeds its own D-PHY. > > > > > > In order to distinguish the use of the D-PHY between Rx mode (for MIPI > > > CSI-2) > > > and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY > > > API. > > > This allows adding Rx support in the A31 D-PHY driver. > > > > > > A few changes and fixes are applied to the A31 CSI controller driver, in > > > order > > > to support the MIPI CSI-2 use-case. > > > > Besides the compile error for patch 2/16, I also get several other compile > > errors: > > > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c: In function > > ‘sun6i_csi_v4l2_fwnode_init’: > > ./include/media/v4l2-async.h:207:10: error: expected expression before ‘)’ > > token > > 207 | ((type *) \ > > | ^ > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:790:8: note: in > > expansion of macro ‘v4l2_async_notifier_add_fwnode_remote_subdev’ > > 790 | ret = v4l2_async_notifier_add_fwnode_remote_subdev(&csi->notifier, > > |^~~~ > > ./include/media/v4l2-async.h:207:10: error: expected expression before ‘)’ > > token > > 207 | ((type *) \ > > | ^ > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:811:8: note: in > > expansion of macro ‘v4l2_async_notifier_add_fwnode_remote_subdev’ > > 811 | ret = v4l2_async_notifier_add_fwnode_remote_subdev(&csi->notifier, > > |^~~~ > > make[5]: *** [scripts/Makefile.build:272: > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.o] Error 1 > > make[5]: *** Waiting for unfinished jobs > > make[4]: *** [scripts/Makefile.build:272: > > drivers/media/platform/rockchip/rkisp1/rkisp1-isp.o] Error 1 > > make[3]: *** [scripts/Makefile.build:515: > > drivers/media/platform/rockchip/rkisp1] Error 2 > > make[3]: *** Waiting for unfinished jobs > > In file included from ./include/media/v4l2-subdev.h:14, > > from ./include/media/v4l2-device.h:13, > > from > > drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c:19: > > drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c: > > In function ‘sun8i_a83t_mipi_csi2_v4l2_setup’: > > ./include/media/v4l2-async.h:207:10: error: expected expression before ‘)’ > > token > > 207 | ((type *) \ > > | ^ > > drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c:495:8: > > note: in expansion of macro > > ‘v4l2_async_notifier_add_fwnode_remote_subdev’ > > 495 | ret = v4l2_async_notifier_add_fwnode_remote_subdev(notifier, > > handle, > > |^~~~ > > In file included from ./include/media/v4l2-subdev.h:14, > > from ./include/media/v4l2-device.h:13, > > from > > drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c:18: > > drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c: In function > > ‘sun6i_mipi_csi2_v4l2_setup’: > > ./include/media/v4l2-async.h:207:10: error: expected expression before ‘)’ > > token > > 207 | ((type *) \ > > | ^ > > drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c:437:8: note: > > in expansion of macro ‘v4l2_async_notifier_add_fwnode_remote_subdev’ > > 437 | ret = v4l2_async_notifier_add_fwnode_remote_subdev(notifier, > > handle, > > |^~~~ > > > > Can you rebase this series? > > Thanks for letting me know, I'll look into this for the next iteration. > > > I also need Acked-by's for patches 1-3 from one of the PHY maintainers, but > > as > > you mentioned this might need to change as well. > > > > Was there a reason why I haven't looked at this before? It's quite an old > > series, > > usually I don't wait for so long. If it was because I was really slow, then > > I > > apologize and please kick me sooner if you see a review like this take so > > long. > > I'm not sure, but Sakari definitely went over previous interations and made > various comments,so the series definitely hasn't gone unreviewed! My acks seem to be missing. Let me go through it. As for Hans: please ping if you don't get reviews. -- Kind regards, Sakari Ailus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
ps;
> + struct fwnode_handle *fwnode;
> + struct software_node *nodes;
> + struct v4l2_subdev *sd;
> + int i, ret;
unsigned int i
> +
> + for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
> + adev = acpi_dev_get_first_match_dev(supported_devices[i],
> + NULL, -1);
> +
> + if (!adev)
> + continue;
> +
> + dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> +
> + if (!dev) {
> + pr_info("ACPI match for %s, but it has no i2c device\n",
> + supported_devices[i]);
> + continue;
> + }
> +
> + if (!dev->driver_data) {
> + pr_info("ACPI match for %s, but it has no driver\n",
> + supported_devices[i]);
> + continue;
> + } else {
> + pr_info("Found supported device %s\n",
> + supported_devices[i]);
> + }
> +
> + sensor = &bridge.sensors[bridge.n_sensors];
> + /*
> + * Store sensor's existing fwnode so that it can be restored if
> + * this module is removed.
> + */
> + sensor->fwnode = fwnode_handle_get(dev->fwnode);
> +
> + get_acpi_ssdb_sensor_data(dev, &ssdb);
> +
> + nodes = sensor->swnodes;
> + sensor_props = sensor->sensor_props;
> + cio2_props = sensor->cio2_props;
> + fwnode = sensor->fwnode;
> +
> + ret = create_endpoint_properties(dev, &ssdb, sensor_props,
> + cio2_props);
> +
> + if (ret)
> + return ret;
> +
> + /* build the software nodes */
> +
> + nodes[SWNODE_SENSOR_HID] = NODE_HID(supported_devices[i]);
> + nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0",
> +
> &nodes[SWNODE_SENSOR_HID]);
> + nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0",
> +
> &nodes[SWNODE_SENSOR_PORT],
> + sensor_props);
> + nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[(int)ssdb.link],
> + &cio2_hid_node);
> + nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0",
> +
> &nodes[SWNODE_CIO2_PORT],
> + cio2_props);
> + nodes[SWNODE_NULL_TERMINATOR] = SOFTWARE_NODE_NULL;
> +
> + ret = software_node_register_nodes(nodes);
> + if (ret) {
> + dev_err(dev,
> + "Failed to register software nodes for %s\n",
> + supported_devices[i]);
> + return ret;
> + }
> +
> + fwnode = software_node_fwnode(&nodes[SWNODE_SENSOR_HID]);
> + if (!fwnode) {
> + dev_err(dev,
> + "Failed to get software node for %s\n",
> + supported_devices[i]);
> + return ret;
> + }
> +
> + fwnode->secondary = ERR_PTR(-ENODEV);
> + dev->fwnode = fwnode;
> +
> + /*
> + * The device should by this point has driver_data set to an
> + * instance of struct v4l2_subdev; set the fwnode for that too.
> + */
> +
> + sd = dev_get_drvdata(dev);
> + sd->fwnode = fwnode;
I'm a bit lost here. Isn't it enough to have the sensor device's fwnode,
and to use that for V4L2 async fwnode matching (as usual)?
> +
> + sensor->dev = dev;
> + bridge.n_sensors++;
> + }
> +
> + return 0;
> +}
> +
> +static int cio2_bridge_init(void)
> +{
> + struct fwnode_handle *fwnode;
> + int ret;
> +
> + ret = software_node_register(&cio2_hid_node);
> +
> + if (ret < 0) {
> + pr_err("Failed to register the CIO2 HID node\n");
> + return -EINVAL;
> + }
> +
> + ret = connect_supported_devices();
> +
>
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
Hi Dan,
On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> > > +static int connect_supported_devices(void)
> > > +{
> > > + struct acpi_device *adev;
> > > + struct device *dev;
> > > + struct sensor_bios_data ssdb;
> > > + struct sensor *sensor;
> > > + struct property_entry *sensor_props;
> > > + struct property_entry *cio2_props;
> > > + struct fwnode_handle *fwnode;
> > > + struct software_node *nodes;
> > > + struct v4l2_subdev *sd;
> > > + int i, ret;
> >
> > unsigned int i
> >
>
> Why?
>
> For list iterators then "int i;" is best... For sizes then unsigned is
> sometimes best. Or if it's part of the hardware spec or network spec
> unsigned is best. Otherwise unsigned variables cause a ton of bugs.
> They're not as intuitive as signed variables. Imagine if there is an
> error in this loop and you want to unwind. With a signed variable you
> can do:
>
> while (--i >= 0)
> cleanup(&bridge.sensors[i]);
>
> There are very few times where raising the type maximum from 2 billion
> to 4 billion fixes anything.
There's simply no need for the negative integers here. Sizes (as it's a
size here) are unsigned, too, so you'd be comparing signed and unsigned
numbers later in the function.
--
Regards,
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
Hi Andy, On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote: > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote: > > On 17/09/2020 11:33, Sakari Ailus wrote: > > > a module and not enlarge everyone's kernel, and the initialisation would > > > at > > > the same time take place before the rest of what the CIO2 driver does in > > > probe. > > I thought of that as well, but wasn't sure which was preferable. I can > > compress it into the CIO2 driver though sure. > > Sakari, I tend to agree with Dan and have the board file separated from the > driver and even framework. And it'll be linked to the kernel binary then I suppose? I don't have a strong opinion either way, just thought that this will affect anyone using x86 machines, whether or not they have IPU3. I guess it could be compiled in if the ipu3-cio2 driver is enabled? -- Sakari Ailus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 15/56] media: fix kernel-doc markups
Thanks, Mauro! On Fri, Oct 23, 2020 at 06:33:02PM +0200, Mauro Carvalho Chehab wrote: > Some identifiers have different names between their prototypes > and the kernel-doc markup. Seome seems to be due to cut-and-paste > related issues. > > Others need to be fixed, as kernel-doc markups should use this format: > identifier - description > > Signed-off-by: Mauro Carvalho Chehab On IPU3 and V4L2 bits: Acked-by: Sakari Ailus -- Regards, Sakari Ailus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation
Hi Paul,
On Fri, Oct 23, 2020 at 07:45:39PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the A31 MIPI CSI-2
> controller.
>
> Signed-off-by: Paul Kocialkowski
> ---
> .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 ++
> 1 file changed, 168 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> new file mode 100644
> index ..9adc0bc27033
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-mipi-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner A31 MIPI CSI-2 Device Tree Bindings
> +
> +maintainers:
> + - Paul Kocialkowski
> +
> +properties:
> + compatible:
> +oneOf:
> + - const: allwinner,sun6i-a31-mipi-csi2
> + - items:
> + - const: allwinner,sun8i-v3s-mipi-csi2
> + - const: allwinner,sun6i-a31-mipi-csi2
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> + clocks:
> +items:
> + - description: Bus Clock
> + - description: Module Clock
> +
> + clock-names:
> +items:
> + - const: bus
> + - const: mod
> +
> + phys:
> +items:
> + - description: MIPI D-PHY
> +
> + phy-names:
> +items:
> + - const: dphy
> +
> + resets:
> +maxItems: 1
> +
> + # See ./video-interfaces.txt for details
> + ports:
> +type: object
> +
> +properties:
> + port@0:
> +type: object
> +description: Input port, connect to a MIPI CSI-2 sensor
> +
> +properties:
> + reg:
> +const: 0
> +
> + endpoint:
> +type: object
> +
> +properties:
> + remote-endpoint: true
> +
> + bus-type:
> +const: 4
> +
> + clock-lanes:
> +maxItems: 1
You can drop bus-type and clock-lanes (assuming no lane remapping is
supported by the hardware).
> +
> + data-lanes:
> +minItems: 1
> +maxItems: 4
> +
> +required:
> + - bus-type
> + - data-lanes
> + - remote-endpoint
> +
> +additionalProperties: false
> +
> +required:
> + - endpoint
> +
> +additionalProperties: false
> +
> + port@1:
> +type: object
> +description: Output port, connect to a CSI controller
> +
> +properties:
> + reg:
> +const: 1
> +
> + endpoint:
> +type: object
> +
> +properties:
> + remote-endpoint: true
> +
> + bus-type:
> +const: 4
Same here.
> +
> +additionalProperties: false
> +
> +required:
> + - endpoint
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - resets
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +#include
> +
> +mipi_csi2: mipi-csi2@1cb1000 {
> +compatible = "allwinner,sun8i-v3s-mipi-csi2",
> + "allwinner,sun6i-a31-mipi-csi2";
> +reg = <0x01cb1000 0x1000>;
> +interrupts = ;
> +clocks = <&ccu CLK_BUS_CSI>,
> + <&ccu CLK_CSI1_SCLK>;
> +clock-names = "bus", "mod";
> +resets = <&ccu RST_BUS_CSI>;
> +
> +phys = <&dphy>;
> +phy-names = "dphy";
> +
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +mipi_csi2_in: port@0 {
> +reg = <0>;
> +
> +mipi_csi2_in_ov5648: endpoint {
> +bus-type = <4>; /* MIPI CSI-2 D-PHY */
> +clock-lanes = <0>;
> +data-lanes = <1 2 3 4>;
> +
> +remote-endpoint = <&ov5648_out_mipi_csi2>;
> +};
> +};
> +
> +mipi_csi2_out: port@1 {
> +reg = <1>;
> +
> +mipi_csi2_out_csi0: endpoint {
> +bus-type = <4>; /* MIPI CSI-2 D-PHY */
> +remote-endpoint = <&csi0_in_mipi_csi2>;
> +};
> +};
> +};
> +};
> +
> +...
--
Kind regards,
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller
CTL_UNPK_EN BIT(1)
> +#define SUN6I_MIPI_CSI2_CTL_EN BIT(0)
> +#define SUN6I_MIPI_CSI2_CFG_REG 0x4
> +#define SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(v) v) - 1) << 8) & \
> + GENMASK(9, 8))
> +#define SUN6I_MIPI_CSI2_CFG_LANE_COUNT(v)(((v) - 1) & GENMASK(1,
> 0))
> +#define SUN6I_MIPI_CSI2_VCDT_RX_REG 0x8
> +#define SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(ch, vc)(((vc) &
> GENMASK(1, 0)) << \
> + ((ch) * 8 + 6))
> +#define SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(ch, t) (((t) & GENMASK(5, 0))
> << \
> + ((ch) * 8))
> +#define SUN6I_MIPI_CSI2_RX_PKT_NUM_REG 0xc
> +
> +#define SUN6I_MIPI_CSI2_VERSION_REG 0x3c
> +
> +#define SUN6I_MIPI_CSI2_CH_BASE 0x1000
> +#define SUN6I_MIPI_CSI2_CH_OFFSET0x100
> +
> +#define SUN6I_MIPI_CSI2_CH_CFG_REG 0x40
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_REG0x50
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_EOT_ERRBIT(29)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_CHKSUM_ERR BIT(28)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_ECC_WRNBIT(27)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_ECC_ERRBIT(26)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_SYNC_ERR BIT(25)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_SYNC_ERR BIT(24)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_EMB_DATA BIT(18)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_PF BIT(17)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_PH_UPDATE BIT(16)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_START_SYNCBIT(11)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_END_SYNC BIT(10)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_START_SYNC BIT(9)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_END_SYNC BIT(8)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FIFO_OVER BIT(0)
> +
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_REG0x58
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_EOT_ERRBIT(29)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_CHKSUM_ERR BIT(28)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_ECC_WRNBIT(27)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_ECC_ERRBIT(26)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_SYNC_ERR BIT(25)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_SYNC_ERR BIT(24)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_EMB_DATA BIT(18)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_PF BIT(17)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_PH_UPDATE BIT(16)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_START_SYNCBIT(11)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_END_SYNC BIT(10)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_START_SYNC BIT(9)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_END_SYNC BIT(8)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FIFO_OVER BIT(0)
> +
> +#define SUN6I_MIPI_CSI2_CH_DT_TRIGGER_REG0x60
> +#define SUN6I_MIPI_CSI2_CH_CUR_PH_REG0x70
> +#define SUN6I_MIPI_CSI2_CH_ECC_REG 0x74
> +#define SUN6I_MIPI_CSI2_CH_CKS_REG 0x78
> +#define SUN6I_MIPI_CSI2_CH_FRAME_NUM_REG 0x7c
> +#define SUN6I_MIPI_CSI2_CH_LINE_NUM_REG 0x80
> +
> +#define SUN6I_MIPI_CSI2_CH_REG(reg, ch) \
> + (SUN6I_MIPI_CSI2_CH_BASE + SUN6I_MIPI_CSI2_CH_OFFSET * (ch) + (reg))
> +
> +enum mipi_csi2_data_type {
> + MIPI_CSI2_DATA_TYPE_RAW8= 0x2a,
> + MIPI_CSI2_DATA_TYPE_RAW10 = 0x2b,
> + MIPI_CSI2_DATA_TYPE_RAW12 = 0x2c,
> +};
> +
> +struct sun6i_mipi_csi2_video {
> + struct v4l2_fwnode_endpoint endpoint;
> + struct v4l2_subdev subdev;
> + struct media_pad pads[2];
> +
> + struct v4l2_async_subdev subdev_async;
> + struct v4l2_async_notifier notifier;
> +
> + struct v4l2_subdev *remote_subdev;
> + u32 remote_pad_index;
> + u32 mbus_code;
> +};
> +
> +struct sun6i_mipi_csi2_dev {
> + struct device *dev;
> +
> + struct regmap *regmap;
> + struct clk *clk_mod;
> + struct reset_control *reset;
> + struct phy *dphy;
> +
> + struct sun6i_mipi_csi2_video video;
> +};
> +
> +#define sun6i_mipi_csi2_subdev_video(subdev) \
> + container_of(subdev, struct sun6i_mipi_csi2_video, subdev)
> +
> +#define sun6i_mipi_csi2_video_dev(video) \
> + container_of(video, struct sun6i_mipi_csi2_dev, video)
> +
> +#endif /* __SUN6I_MIPI_CSI2_H__ */
--
Regards,
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/14] dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation
Hi Paul,
On Fri, Oct 23, 2020 at 07:45:43PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the A83T MIPI CSI-2
> controller.
>
> Signed-off-by: Paul Kocialkowski
> ---
> .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 ++
> 1 file changed, 158 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml
> b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml
> new file mode 100644
> index ..2384ae4e7be0
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allwinner,sun8i-a83t-mipi-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner A83T MIPI CSI-2 Device Tree Bindings
> +
> +maintainers:
> + - Paul Kocialkowski
> +
> +properties:
> + compatible:
> +const: allwinner,sun8i-a83t-mipi-csi2
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> + clocks:
> +items:
> + - description: Bus Clock
> + - description: Module Clock
> + - description: MIPI-specific Clock
> + - description: Misc CSI Clock
> +
> + clock-names:
> +items:
> + - const: bus
> + - const: mod
> + - const: mipi
> + - const: misc
> +
> + resets:
> +maxItems: 1
> +
> + # See ./video-interfaces.txt for details
> + ports:
> +type: object
> +
> +properties:
> + port@0:
> +type: object
> +description: Input port, connect to a MIPI CSI-2 sensor
> +
> +properties:
> + reg:
> +const: 0
> +
> + endpoint:
> +type: object
> +
> +properties:
> + remote-endpoint: true
> +
> + bus-type:
> +const: 4
Again, if this is D-PHY only, you can remove this.
> +
> + clock-lanes:
> +maxItems: 1
> +
> + data-lanes:
> +minItems: 1
> +maxItems: 4
Does the device support lane reordering? If not, you can remove
clock-lanes.
> +
> +required:
> + - bus-type
> + - data-lanes
> + - remote-endpoint
> +
> +additionalProperties: false
> +
> +required:
> + - endpoint
> +
> +additionalProperties: false
> +
> + port@1:
> +type: object
> +description: Output port, connect to a CSI controller
> +
> +properties:
> + reg:
> +const: 1
> +
> + endpoint:
> +type: object
> +
> +properties:
> + remote-endpoint: true
> +
> + bus-type:
> +const: 4
Is it a MIPI CSI-2 D-PHY -> MIPI CSI-2 D-PHY device? I call that "cable".
:-)
> +
> +additionalProperties: false
> +
> +required:
> + - endpoint
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - resets
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +#include
> +
> +mipi_csi2: mipi-csi2@1cb1000 {
> +compatible = "allwinner,sun8i-a83t-mipi-csi2";
> +reg = <0x01cb1000 0x1000>;
> +interrupts = ;
> +clocks = <&ccu CLK_BUS_CSI>,
> + <&ccu CLK_CSI_SCLK>,
> + <&ccu CLK_MIPI_CSI>,
> + <&ccu CLK_CSI_MISC>;
> +clock-names = "bus", "mod", "mipi", "misc";
> +resets = <&ccu RST_BUS_CSI>;
> +
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +mipi_csi2_in: port@0 {
> +reg = <0>;
> +
> +mipi_csi2_in_ov8865: endpoint {
> +bus-type = <4>; /* MIPI CSI-2 D-PHY */
> +clock-lanes = <0>;
> +data-lanes = <1 2 3 4>;
> +
> +remote-endpoint = <&ov8865_out_mipi_csi2>;
> +};
> +};
> +
> +mipi_csi2_out: port@1 {
> +reg = <1>;
> +
> +mipi_csi2_out_csi: endpoint {
> +bus-type = <4>; /* MIPI CSI-2 D-PHY */
> +remote-endpoint = <&csi_in_mipi_csi2>;
> +};
> +};
> +};
> +};
> +
> +...
--
Regards,
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] media: staging/intel-ipu3: Fix err handle of ipu3_css_find_binary
Hi Bingbu, On Mon, Jan 07, 2019 at 10:38:19AM +0800, Bingbu Cao wrote: > Hi, Haibing > > Thanks for your patch, it looks fine for me. > Reviewed-by: Bingbu Cao > > On 12/29/2018 10:45 AM, YueHaibing wrote: > > css->pipes[pipe].bindex = binary; I'm taking Colin's patch with equivalent content; it was there first. Thanks! -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 01/13] media: staging/imx: refactor imx media device probe
>"add_internal_subdevs failed with %d\n", ret);
> - goto notifier_cleanup;
> - }
> -
> - /* no subdevs? just bail */
> - if (list_empty(&imxmd->notifier.asd_list)) {
> - ret = -ENODEV;
> - goto notifier_cleanup;
> + goto dev_cleanup;
> }
>
> - /* prepare the async subdev notifier and register it */
> - imxmd->notifier.ops = &imx_media_subdev_ops;
> - ret = v4l2_async_notifier_register(&imxmd->v4l2_dev,
> -&imxmd->notifier);
> - if (ret) {
> - v4l2_err(&imxmd->v4l2_dev,
> - "v4l2_async_notifier_register failed with %d\n", ret);
> + ret = imx_media_dev_notifier_register(imxmd);
> + if (ret)
> goto del_int;
> - }
>
> return 0;
>
> del_int:
> imx_media_remove_internal_subdevs(imxmd);
> -notifier_cleanup:
> - v4l2_async_notifier_cleanup(&imxmd->notifier);
If you remove this one, you'll miss cleaning up the notifier afterwards: an
initialised async notifier needs to be cleaned up in the end.
> - v4l2_device_unregister(&imxmd->v4l2_dev);
> -cleanup:
> - media_device_cleanup(&imxmd->md);
> +
> +dev_cleanup:
> + imx_media_dev_cleanup(imxmd);
> +
> return ret;
> }
>
> @@ -531,10 +482,9 @@ static int imx_media_remove(struct platform_device *pdev)
>
> v4l2_async_notifier_unregister(&imxmd->notifier);
> imx_media_remove_internal_subdevs(imxmd);
> - v4l2_async_notifier_cleanup(&imxmd->notifier);
> - v4l2_device_unregister(&imxmd->v4l2_dev);
> + imx_media_dev_notifier_unregister(imxmd);
> media_device_unregister(&imxmd->md);
> - media_device_cleanup(&imxmd->md);
> + imx_media_dev_cleanup(imxmd);
>
> return 0;
> }
> diff --git a/drivers/staging/media/imx/imx-media-of.c
> b/drivers/staging/media/imx/imx-media-of.c
> index a01327f6e045..03446335ac03 100644
> --- a/drivers/staging/media/imx/imx-media-of.c
> +++ b/drivers/staging/media/imx/imx-media-of.c
> @@ -20,7 +20,8 @@
> #include
> #include "imx-media.h"
>
> -static int of_add_csi(struct imx_media_dev *imxmd, struct device_node
> *csi_np)
> +int imx_media_of_add_csi(struct imx_media_dev *imxmd,
> + struct device_node *csi_np)
> {
> int ret;
>
> @@ -45,6 +46,7 @@ static int of_add_csi(struct imx_media_dev *imxmd, struct
> device_node *csi_np)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(imx_media_of_add_csi);
>
> int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
>struct device_node *np)
> @@ -57,7 +59,7 @@ int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
> if (!csi_np)
> break;
>
> - ret = of_add_csi(imxmd, csi_np);
> + ret = imx_media_of_add_csi(imxmd, csi_np);
> of_node_put(csi_np);
> if (ret)
> return ret;
> diff --git a/drivers/staging/media/imx/imx-media.h
> b/drivers/staging/media/imx/imx-media.h
> index bc7feb81937c..b7f11c36461b 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -226,6 +226,19 @@ int imx_media_add_async_subdev(struct imx_media_dev
> *imxmd,
> struct fwnode_handle *fwnode,
> struct platform_device *pdev);
>
> +int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
> +struct v4l2_subdev *sd,
> +struct v4l2_async_subdev *asd);
> +int imx_media_link_notify(struct media_link *link, u32 flags,
> + unsigned int notification);
> +int imx_media_probe_complete(struct v4l2_async_notifier *notifier);
> +
> +struct imx_media_dev *imx_media_dev_init(struct device *dev);
> +int imx_media_dev_notifier_register(struct imx_media_dev *imxmd);
> +
> +void imx_media_dev_cleanup(struct imx_media_dev *imxmd);
> +void imx_media_dev_notifier_unregister(struct imx_media_dev *imxmd);
> +
> /* imx-media-fim.c */
> struct imx_media_fim;
> void imx_media_fim_eof_monitor(struct imx_media_fim *fim, ktime_t timestamp);
> @@ -249,6 +262,8 @@ int imx_media_create_of_links(struct imx_media_dev *imxmd,
> struct v4l2_subdev *sd);
> int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
> struct v4l2_subdev *csi);
> +int imx_media_of_add_csi(struct imx_media_dev *imxmd,
> + struct device_node *csi_np);
>
> /* imx-media-capture.c */
> struct imx_media_video_dev *
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream on/off operations
mutex_destroy(&imgu->lock);
> + mutex_destroy(&imgu->streaming_lock);
> }
>
> static int __maybe_unused imgu_suspend(struct device *dev)
> diff --git a/drivers/staging/media/ipu3/ipu3.h
> b/drivers/staging/media/ipu3/ipu3.h
> index 04fc99f47ebb..f732315f0701 100644
> --- a/drivers/staging/media/ipu3/ipu3.h
> +++ b/drivers/staging/media/ipu3/ipu3.h
> @@ -146,6 +146,10 @@ struct imgu_device {
>* vid_buf.list and css->queue
>*/
> struct mutex lock;
> +
> + /* Lock to protect writes to streaming flag in this struct */
> + struct mutex streaming_lock;
> +
> /* Forbit streaming and buffer queuing during system suspend. */
> atomic_t qbuf_barrier;
> /* Indicate if system suspend take place while imgu is streaming. */
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v11 03/13] media: staging/imx7: add imx7 CSI subdev driver
_PAD_FL_SOURCE;
> +
> + /* set a default mbus format */
> + ret = imx_media_init_mbus_fmt(&csi->format_mbus[i],
> + 800, 600, 0, V4L2_FIELD_NONE,
> + &csi->cc[i]);
> + if (ret < 0)
> + return ret;
> +
> + /* init default frame interval */
> + csi->frame_interval[i].numerator = 1;
> + csi->frame_interval[i].denominator = 30;
> + }
> +
> + ret = media_entity_pads_init(&sd->entity, IMX7_CSI_PADS_NUM, csi->pad);
> + if (ret < 0)
> + return ret;
> +
> + ret = imx_media_capture_device_register(csi->vdev);
> + if (ret < 0)
> + return ret;
> +
> + ret = imx_media_add_video_device(csi->md, csi->vdev);
> + if (ret < 0) {
> + imx_media_capture_device_unregister(csi->vdev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void imx7_csi_unregistered(struct v4l2_subdev *sd)
> +{
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> +
> + imx_media_capture_device_unregister(csi->vdev);
You could call imx_media_capture_device_unregister() directly.
> +}
> +
> +static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg)
> +{
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + struct v4l2_mbus_framefmt *mf;
> + int ret;
> + int i;
> +
> + for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
> + mf = v4l2_subdev_get_try_format(sd, cfg, i);
> +
> + ret = imx_media_init_mbus_fmt(mf, 800, 600, 0, V4L2_FIELD_NONE,
> + &csi->cc[i]);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct media_entity_operations imx7_csi_entity_ops = {
> + .link_setup = imx7_csi_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static const struct v4l2_subdev_video_ops imx7_csi_video_ops = {
> + .s_stream = imx7_csi_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx7_csi_pad_ops = {
> + .init_cfg = imx7_csi_init_cfg,
> + .enum_mbus_code = imx7_csi_enum_mbus_code,
> + .get_fmt = imx7_csi_get_fmt,
> + .set_fmt = imx7_csi_set_fmt,
> + .link_validate =imx7_csi_pad_link_validate,
> +};
> +
> +static const struct v4l2_subdev_ops imx7_csi_subdev_ops = {
> + .video =&imx7_csi_video_ops,
> + .pad = &imx7_csi_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx7_csi_internal_ops = {
> + .registered = imx7_csi_registered,
> + .unregistered = imx7_csi_unregistered,
> +};
> +
> +static int imx7_csi_parse_endpoint(struct device *dev,
> +struct v4l2_fwnode_endpoint *vep,
> +struct v4l2_async_subdev *asd)
> +{
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> +}
> +
> +static int imx7_csi_clocks_get(struct imx7_csi *csi)
> +{
> + struct device *dev = csi->dev;
> + int i;
> +
> + csi->num_clks = ARRAY_SIZE(imx7_csi_clk_id);
> + csi->clks = devm_kcalloc(dev, csi->num_clks, sizeof(*csi->clks),
> + GFP_KERNEL);
> +
> + if (!csi->clks)
> + return -ENOMEM;
> +
> + for (i = 0; i < csi->num_clks; i++)
> + csi->clks[i].id = imx7_csi_clk_id[i];
> +
> + return devm_clk_bulk_get(dev, csi->num_clks, csi->clks);
> +}
> +
> +static int imx7_csi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = dev->of_node;
> + struct imx_media_dev *imxmd;
> + struct imx7_csi *csi;
> + struct resource *res;
> + int ret;
> +
> + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
> + if (!csi)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, &csi->sd);
> + csi->dev = dev;
> +
> + ret = imx7_csi_clocks_get(csi);
> + if (ret < 0) {
> + dev_err(dev, "Failed to get clocks");
> + return -ENODEV;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + csi->irq = platform_get_irq(pdev, 0);
> + if (!res || csi->irq < 0) {
> + dev_err(dev, "Missing platform resources data\n");
> + return -ENODEV;
> + }
> +
> + csi->regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(csi->regbase)) {
> + dev_err(dev, "Failed platform resources map\n");
> + return -ENODEV;
> + }
> +
> + spin_lock_init(&csi->irqlock);
> + mutex_init(&csi->lock);
> +
> + /* install interrupt handler */
> + ret = devm_request_irq(dev, csi->irq, imx7_csi_irq_handler, 0, "csi",
> +(void *)csi);
> + if (ret < 0) {
> + dev_err(dev, "Request CSI IRQ failed.\n");
> + ret = -ENODEV;
> + goto destroy_mutex;
> + }
> +
> + /* add media device */
> + imxmd = imx_media_dev_init(dev);
> + if (IS_ERR(imxmd)) {
> + ret = PTR_ERR(imxmd);
> + goto destroy_mutex;
> + }
> +
> + ret = imx_media_of_add_csi(imxmd, node);
> + if (ret < 0)
> + goto cleanup;
> +
> + ret = imx_media_dev_notifier_register(imxmd);
> + if (ret < 0)
> + goto cleanup;
> +
> + csi->imxmd = imxmd;
> + v4l2_subdev_init(&csi->sd, &imx7_csi_subdev_ops);
> + v4l2_set_subdevdata(&csi->sd, csi);
> + csi->sd.internal_ops = &imx7_csi_internal_ops;
> + csi->sd.entity.ops = &imx7_csi_entity_ops;
> + csi->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> + csi->sd.dev = &pdev->dev;
> + csi->sd.owner = THIS_MODULE;
> + csi->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> + csi->sd.grp_id = IMX_MEDIA_GRP_ID_CSI;
> + snprintf(csi->sd.name, sizeof(csi->sd.name), "csi");
> +
> + csi->vdev = imx_media_capture_device_init(&csi->sd, IMX7_CSI_PAD_SRC);
> + if (IS_ERR(csi->vdev))
> + return PTR_ERR(csi->vdev);
> +
> + v4l2_ctrl_handler_init(&csi->ctrl_hdlr, 0);
> + csi->sd.ctrl_handler = &csi->ctrl_hdlr;
> +
> + ret = v4l2_async_register_fwnode_subdev(&csi->sd,
> + sizeof(struct v4l2_async_subdev),
> + NULL, 0, imx7_csi_parse_endpoint);
> + if (ret)
> + goto free;
> +
> + return 0;
> +
> +free:
> + v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
> + imx_media_capture_device_remove(csi->vdev);
> +
> +cleanup:
> + v4l2_async_notifier_cleanup(&imxmd->notifier);
> + v4l2_device_unregister(&imxmd->v4l2_dev);
> + media_device_cleanup(&imxmd->md);
> +
> +destroy_mutex:
> + mutex_destroy(&csi->lock);
> +
> + return ret;
> +}
> +
> +static int imx7_csi_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + struct imx_media_dev *imxmd = csi->imxmd;
> +
> + imx_media_capture_device_remove(csi->vdev);
I think you're missing v4l2_async_notifier_unregister() here.
> + v4l2_async_notifier_cleanup(&imxmd->notifier);
> + v4l2_device_unregister(&imxmd->v4l2_dev);
Where is the media device unregistered? That should come before freeing
control handlers --- the same for unregistering video devices.
> + media_device_cleanup(&imxmd->md);
> + media_entity_cleanup(&sd->entity);
> + v4l2_async_unregister_subdev(sd);
> + v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
> + mutex_destroy(&csi->lock);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx7_csi_of_match[] = {
> + { .compatible = "fsl,imx7-csi" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
> +
> +static struct platform_driver imx7_csi_driver = {
> + .probe = imx7_csi_probe,
> + .remove = imx7_csi_remove,
> + .driver = {
> + .of_match_table = imx7_csi_of_match,
> + .name = "imx7-csi",
> + },
> +};
> +module_platform_driver(imx7_csi_driver);
> +
> +MODULE_DESCRIPTION("i.MX7 CSI subdev driver");
> +MODULE_AUTHOR("Rui Miguel Silva ");
> +MODULE_LICENSE("GPL");
"GPL" or "GPL v2"?
> +MODULE_ALIAS("platform:imx7-csi");
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v11 00/13] media: staging/imx7: add i.MX7 media driver
On Thu, Jan 24, 2019 at 04:09:15PM +, Rui Miguel Silva wrote: > Hi, > This series introduces the Media driver to work with the i.MX7 SoC. it uses > the > already existing imx media core drivers but since the i.MX7, contrary to > i.MX5/6, do not have an IPU and because of that some changes in the imx media > core are made along this series to make it support that case. > > This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several > configurations changes for this to work as a capture subsystem. Some bugs are > also fixed along the line. And necessary documentation. > > For a more detailed view of the capture paths, pads links in the i.MX7 please > take a look at the documentation in PATCH 10. > > The system used to test and develop this was the Warp7 board with an OV2680 > sensor, which output format is 10-bit bayer. So, only MIPI interface was > tested, a scenario with an parallel input would nice to have. > > Bellow goes an example of the output of the pads and links and the output of > v4l2-compliance testing. > > The v4l-utils version used is: > v4l2-compliance SHA: 1a6c8fe9a65c26e78ba34bd4aa2df28ede7d00cb, 32 bits > > The Media Driver fail some tests but this failures are coming from code out of > scope of this series (imx-capture), and some from the sensor OV2680 > but that I think not related with the sensor driver but with the testing and > core. > > The csi and mipi-csi entities pass all compliance tests. For patches 1, 4 and 10--13: Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v11 05/13] media: staging/imx7: add MIPI CSI-2 receiver subdev for i.MX7
TR_ERR(state->regs);
> +
> + state->irq = platform_get_irq(pdev, 0);
> + if (state->irq < 0) {
> + dev_err(dev, "Failed to get irq\n");
> + return state->irq;
> + }
> +
> + ret = mipi_csis_clk_get(state);
> + if (ret < 0)
> + return ret;
> +
> + mipi_csis_clk_enable(state);
> +
> + ret = devm_request_irq(dev, state->irq, mipi_csis_irq_handler,
> +0, dev_name(dev), state);
> + if (ret) {
> + dev_err(dev, "Interrupt request failed\n");
> + goto disable_clock;
> + }
> +
> + platform_set_drvdata(pdev, &state->mipi_sd);
> +
> + mutex_init(&state->lock);
> + ret = mipi_csis_subdev_init(&state->mipi_sd, pdev,
> + &mipi_csis_subdev_ops);
> + if (ret < 0)
> + goto disable_clock;
> +
> + state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&state->mipi_sd.entity, CSIS_PADS_NUM,
> + state->pads);
> + if (ret < 0)
> + goto unregister_subdev;
> +
> + memcpy(state->events, mipi_csis_events, sizeof(state->events));
> +
> + mipi_csis_debugfs_init(state);
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = mipi_csis_pm_resume(dev, true);
> + if (ret < 0)
> + goto unregister_all;
> + }
> +
> + dev_info(&pdev->dev, "lanes: %d, hs_settle: %d, wclk: %d, freq: %u\n",
> + state->bus.num_data_lanes, state->hs_settle,
> + state->wrap_clk ? 1 : 0, state->clk_frequency);
> +
> + return 0;
> +
> +unregister_all:
> + mipi_csis_debugfs_exit(state);
> + media_entity_cleanup(&state->mipi_sd.entity);
> +unregister_subdev:
> + v4l2_async_unregister_subdev(&state->mipi_sd);
> +disable_clock:
> + mipi_csis_clk_disable(state);
> + mutex_destroy(&state->lock);
> +
> + return ret;
> +}
> +
> +static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> + int ret = 0;
> +
> + mutex_lock(&state->lock);
> + if (state->flags & ST_POWERED) {
> + mipi_csis_stop_stream(state);
> + ret = regulator_disable(state->mipi_phy_regulator);
> + if (ret)
> + goto unlock;
> + mipi_csis_clk_disable(state);
> + state->flags &= ~ST_POWERED;
> + if (!runtime)
> + state->flags |= ST_SUSPENDED;
> + }
> +
> + unlock:
Extra space here.
> + mutex_unlock(&state->lock);
> +
> + return ret ? -EAGAIN : 0;
> +}
> +
> +static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> + int ret = 0;
> +
> + mutex_lock(&state->lock);
> + if (!runtime && !(state->flags & ST_SUSPENDED))
> + goto unlock;
> +
> + if (!(state->flags & ST_POWERED)) {
> + ret = regulator_enable(state->mipi_phy_regulator);
> + if (ret)
> + goto unlock;
> +
> + state->flags |= ST_POWERED;
> + mipi_csis_clk_enable(state);
> + }
> + if (state->flags & ST_STREAMING)
> + mipi_csis_start_stream(state);
> +
> + state->flags &= ~ST_SUSPENDED;
> +
> + unlock:
Ditto.
> + mutex_unlock(&state->lock);
> +
> + return ret ? -EAGAIN : 0;
> +}
> +
> +static int mipi_csis_suspend(struct device *dev)
> +{
> + return mipi_csis_pm_suspend(dev, false);
> +}
> +
> +static int mipi_csis_resume(struct device *dev)
> +{
> + return mipi_csis_pm_resume(dev, false);
> +}
> +
> +static int mipi_csis_runtime_suspend(struct device *dev)
> +{
> + return mipi_csis_pm_suspend(dev, true);
> +}
> +
> +static int mipi_csis_runtime_resume(struct device *dev)
__maybe_unused for these four?
> +{
> + return mipi_csis_pm_resume(dev, true);
> +}
> +
> +static int mipi_csis_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> +
> + mipi_csis_debugfs_exit(state);
> + v4l2_async_unregister_subdev(&state->mipi_sd);
> + v4l2_async_notifier_unregister(&state->subdev_notifier);
> +
> + pm_runtime_disable(&pdev->dev);
> + mipi_csis_pm_suspend(&pdev->dev, true);
> + mipi_csis_clk_disable(state);
> + media_entity_cleanup(&state->mipi_sd.entity);
> + mutex_destroy(&state->lock);
> + pm_runtime_set_suspended(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops mipi_csis_pm_ops = {
> + SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
> +NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
> +};
> +
> +static const struct of_device_id mipi_csis_of_match[] = {
> + { .compatible = "fsl,imx7-mipi-csi2", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mipi_csis_of_match);
> +
> +static struct platform_driver mipi_csis_driver = {
> + .probe = mipi_csis_probe,
> + .remove = mipi_csis_remove,
> + .driver = {
> + .of_match_table = mipi_csis_of_match,
> + .name = CSIS_DRIVER_NAME,
> + .pm = &mipi_csis_pm_ops,
> + },
> +};
> +
> +module_platform_driver(mipi_csis_driver);
> +
> +MODULE_DESCRIPTION("i.MX7 MIPI CSI-2 Receiver driver");
> +MODULE_LICENSE("GPL");
"GPL" or "GPL v2"?
> +MODULE_ALIAS("platform:imx7-mipi-csi2");
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream on/off operations
Hi Raj, On Wed, Jan 30, 2019 at 05:17:15PM +, Mani, Rajmohan wrote: > Hi Sakari, > > > -Original Message- > > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com] > > Sent: Wednesday, January 30, 2019 12:59 AM > > To: Mani, Rajmohan > > Cc: Mauro Carvalho Chehab ; Greg Kroah-Hartman > > ; linux-me...@vger.kernel.org; > > de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Laurent Pinchart > > ; Jacopo Mondi ; > > Qiu, Tian Shu ; Cao, Bingbu > > ; z...@paasikivi.fi.intel.com; Zhi, Yong > > ; hverk...@xs4all.nl; tf...@chromium.org > > Subject: Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream > > on/off operations > > > > Hi Rajmohan, > > > > On Tue, Jan 29, 2019 at 02:27:36PM -0800, Rajmohan Mani wrote: > > > Currently concurrent stream off operations on ImgU nodes are not > > > synchronized, leading to use-after-free bugs (as reported by KASAN). > > > > > > [ 250.090724] BUG: KASAN: use-after-free in > > > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [ 250.090726] Read of size 8 > > > at addr 888127b29bc0 by task yavta/18836 [ 250.090731] Hardware > > > name: HP Soraka/Soraka, BIOS Google_Soraka.10431.17.0 03/22/2018 [ > > 250.090732] Call Trace: > > > [ 250.090735] dump_stack+0x6a/0xb1 > > > [ 250.090739] print_address_description+0x8e/0x279 > > > [ 250.090743] ? ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [ > > > 250.090746] kasan_report+0x260/0x28a [ 250.090750] > > > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [ 250.090754] > > > ipu3_css_pool_cleanup+0x24/0x37 [ipu3_imgu] [ 250.090759] > > > ipu3_css_pipeline_cleanup+0x61/0xb9 [ipu3_imgu] [ 250.090763] > > > ipu3_css_stop_streaming+0x1f2/0x321 [ipu3_imgu] [ 250.090768] > > > imgu_s_stream+0x94/0x443 [ipu3_imgu] [ 250.090772] ? > > > ipu3_vb2_buf_queue+0x280/0x280 [ipu3_imgu] [ 250.090775] ? > > > vb2_dma_sg_unmap_dmabuf+0x16/0x6f [videobuf2_dma_sg] [ 250.090778] > > ? > > > vb2_buffer_in_use+0x36/0x58 [videobuf2_common] [ 250.090782] > > > ipu3_vb2_stop_streaming+0xf9/0x135 [ipu3_imgu] > > > > > > Implemented a lock to synchronize imgu stream on / off operations and > > > the modification of streaming flag (in struct imgu_device), to prevent > > > these issues. > > > > > > Reported-by: Laurent Pinchart > > > Suggested-by: Laurent Pinchart > > > > > > Signed-off-by: Rajmohan Mani > > > --- > > > drivers/staging/media/ipu3/ipu3-v4l2.c | 6 ++ > > > drivers/staging/media/ipu3/ipu3.c | 3 +++ > > > drivers/staging/media/ipu3/ipu3.h | 4 > > > 3 files changed, 13 insertions(+) > > > > > > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c > > > b/drivers/staging/media/ipu3/ipu3-v4l2.c > > > index c7936032beb9..cf7e917cd0c8 100644 > > > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > > > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > > > @@ -507,12 +507,15 @@ static int ipu3_vb2_start_streaming(struct > > vb2_queue *vq, unsigned int count) > > > goto fail_stop_pipeline; > > > } > > > > > > + mutex_lock(&imgu->streaming_lock); > > > + > > > > You appear to be using imgu_device.lock (while searching buffers to queue to > > the device) as well as imgu_video_device.lock (qbuf, dqbuf) to serialise > > access > > to imgu_video_device.buffers list. > > Ack > > > The two locks may be acquired at the same > > time but each by different processes. That needs to be addressed, but > > probably not in this patch. > > > > The node specific locks will be used by different processes and all of these > processes > will be competing commonly (and successfully) for the imgu_device lock. > I will look into this more. Oops. I think I must have misread something; looking at the code again, indeed imgu_device.lock is acquired for all accesses. Please ignore this comment. > > > I wonder if it'd be more simple to use imgu->lock here instead of adding a > > new > > one. > > > > Extending imgu->lock here, does not work in this case, as imgu_queue_buffers() > will be stuck acquiring imgu->lock, which was already acquired by > imgu_s_stream() > through ipu3_vb2_start_streaming(). > > > > /* Start streaming of the whole pipeline now */ > > > dev_dbg(dev, "IMGU streaming is ready to start"); > > > r = imgu_s_stream(imgu, true); > > &
Re: [PATCH 3/3] Staging: media: ipu3: fixed max charecter style issue
uct > ipu3_css *css) > return ERR_PTR(-EIO); > } > > - dev_dbg(css->dev, "buffer 0x%8x done from pipe %d\n", daddr, > pipe); > + dev_dbg(css->dev, "buffer 0x%8x done from pipe %d\n", > + daddr, pipe); > b->pipe = pipe; > b->state = IPU3_CSS_BUFFER_DONE; > list_del(&b->list); > @@ -2203,7 +2211,8 @@ int ipu3_css_set_parameters(struct ipu3_css *css, > unsigned int pipe, > /* get acc_old */ > map = ipu3_css_pool_last(&css_pipe->pool.acc, 1); > /* user acc */ > - r = ipu3_css_cfg_acc(css, pipe, use, acc, map->vaddr, > + r = ipu3_css_cfg_acc > + (css, pipe, use, acc, map->vaddr, > set_params ? &set_params->acc_param : NULL); > if (r < 0) > goto fail; > @@ -2239,7 +2248,8 @@ int ipu3_css_set_parameters(struct ipu3_css *css, > unsigned int pipe, > ipu3_css_pool_get(&css_pipe->pool.gdc); > map = ipu3_css_pool_last(&css_pipe->pool.gdc, 0); > gdc = map->vaddr; > - ipu3_css_cfg_gdc_table(map->vaddr, > + ipu3_css_cfg_gdc_table > + (map->vaddr, > css_pipe->aux_frames[a].bytesperline / > css_pipe->aux_frames[a].bytesperpixel, > css_pipe->aux_frames[a].height, > @@ -2334,12 +2344,12 @@ int ipu3_css_set_parameters(struct ipu3_css *css, > unsigned int pipe, > if (obgrid) > ipu3_css_pool_put(&css_pipe->pool.obgrid); > if (vmem0) > - ipu3_css_pool_put( > - &css_pipe->pool.binary_params_p > + ipu3_css_pool_put > + (&css_pipe->pool.binary_params_p > [IMGU_ABI_MEM_ISP_VMEM0]); > if (dmem0) > - ipu3_css_pool_put( > - &css_pipe->pool.binary_params_p > + ipu3_css_pool_put > + (&css_pipe->pool.binary_params_p > [IMGU_ABI_MEM_ISP_DMEM0]); > > fail_no_put: -- Regards, Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream on/off operations
Hi Raj, On Wed, Jan 30, 2019 at 05:17:15PM +, Mani, Rajmohan wrote: > Hi Sakari, > > > -Original Message- > > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com] > > Sent: Wednesday, January 30, 2019 12:59 AM > > To: Mani, Rajmohan > > Cc: Mauro Carvalho Chehab ; Greg Kroah-Hartman > > ; linux-me...@vger.kernel.org; > > de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Laurent Pinchart > > ; Jacopo Mondi ; > > Qiu, Tian Shu ; Cao, Bingbu > > ; z...@paasikivi.fi.intel.com; Zhi, Yong > > ; hverk...@xs4all.nl; tf...@chromium.org > > Subject: Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream > > on/off operations > > > > Hi Rajmohan, > > > > On Tue, Jan 29, 2019 at 02:27:36PM -0800, Rajmohan Mani wrote: > > > Currently concurrent stream off operations on ImgU nodes are not > > > synchronized, leading to use-after-free bugs (as reported by KASAN). > > > > > > [ 250.090724] BUG: KASAN: use-after-free in > > > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [ 250.090726] Read of size 8 > > > at addr 888127b29bc0 by task yavta/18836 [ 250.090731] Hardware > > > name: HP Soraka/Soraka, BIOS Google_Soraka.10431.17.0 03/22/2018 [ > > 250.090732] Call Trace: > > > [ 250.090735] dump_stack+0x6a/0xb1 > > > [ 250.090739] print_address_description+0x8e/0x279 > > > [ 250.090743] ? ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [ > > > 250.090746] kasan_report+0x260/0x28a [ 250.090750] > > > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [ 250.090754] > > > ipu3_css_pool_cleanup+0x24/0x37 [ipu3_imgu] [ 250.090759] > > > ipu3_css_pipeline_cleanup+0x61/0xb9 [ipu3_imgu] [ 250.090763] > > > ipu3_css_stop_streaming+0x1f2/0x321 [ipu3_imgu] [ 250.090768] > > > imgu_s_stream+0x94/0x443 [ipu3_imgu] [ 250.090772] ? > > > ipu3_vb2_buf_queue+0x280/0x280 [ipu3_imgu] [ 250.090775] ? > > > vb2_dma_sg_unmap_dmabuf+0x16/0x6f [videobuf2_dma_sg] [ 250.090778] > > ? > > > vb2_buffer_in_use+0x36/0x58 [videobuf2_common] [ 250.090782] > > > ipu3_vb2_stop_streaming+0xf9/0x135 [ipu3_imgu] > > > > > > Implemented a lock to synchronize imgu stream on / off operations and > > > the modification of streaming flag (in struct imgu_device), to prevent > > > these issues. > > > > > > Reported-by: Laurent Pinchart > > > Suggested-by: Laurent Pinchart > > > > > > Signed-off-by: Rajmohan Mani > > > --- > > > drivers/staging/media/ipu3/ipu3-v4l2.c | 6 ++ > > > drivers/staging/media/ipu3/ipu3.c | 3 +++ > > > drivers/staging/media/ipu3/ipu3.h | 4 > > > 3 files changed, 13 insertions(+) > > > > > > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c > > > b/drivers/staging/media/ipu3/ipu3-v4l2.c > > > index c7936032beb9..cf7e917cd0c8 100644 > > > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > > > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > > > @@ -507,12 +507,15 @@ static int ipu3_vb2_start_streaming(struct > > vb2_queue *vq, unsigned int count) > > > goto fail_stop_pipeline; > > > } > > > > > > + mutex_lock(&imgu->streaming_lock); > > > + > > > > You appear to be using imgu_device.lock (while searching buffers to queue to > > the device) as well as imgu_video_device.lock (qbuf, dqbuf) to serialise > > access > > to imgu_video_device.buffers list. > > Ack > > > The two locks may be acquired at the same > > time but each by different processes. That needs to be addressed, but > > probably not in this patch. > > > > The node specific locks will be used by different processes and all of these > processes > will be competing commonly (and successfully) for the imgu_device lock. > I will look into this more. > > > I wonder if it'd be more simple to use imgu->lock here instead of adding a > > new > > one. > > > > Extending imgu->lock here, does not work in this case, as imgu_queue_buffers() > will be stuck acquiring imgu->lock, which was already acquired by > imgu_s_stream() > through ipu3_vb2_start_streaming(). You could move acquiring the lock out of these functions. It would also seem that there is device-wide streaming state etc. information to which the access should also be serialised. Currently it's relying on the node-specific lock only which does not help. Can you grab the lock right after dev_dbg() line in the function? The lock should be also acquired before testing imgu-&
Re: [PATCH][next] media: staging: intel-ipu3: fix unsigned comparison with < 0
On Sat, Feb 02, 2019 at 10:12:41PM +, Colin Ian King wrote:
> ping?
I seem to have applied this to a wrong branch, it'll now be part of my next
pull request to Mauro.
Thanks!
>
> On 22/12/2018 11:49, Colin King wrote:
> > From: Colin Ian King
> >
> > The comparison css->pipes[pipe].bindex < 0 is always false because
> > bindex is an unsigned int. Fix this by using a signed integer for
> > the comparison.
> >
> > Detected by CoverityScan, CID#1476023 ("Unsigned compared against 0")
> >
> > Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline
> > programming")
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/staging/media/ipu3/ipu3-css.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/media/ipu3/ipu3-css.c
> > b/drivers/staging/media/ipu3/ipu3-css.c
> > index 44c55639389a..b9354d2bb692 100644
> > --- a/drivers/staging/media/ipu3/ipu3-css.c
> > +++ b/drivers/staging/media/ipu3/ipu3-css.c
> > @@ -1751,7 +1751,7 @@ int ipu3_css_fmt_try(struct ipu3_css *css,
> > &q[IPU3_CSS_QUEUE_OUT].fmt.mpix;
> > struct v4l2_pix_format_mplane *const vf =
> > &q[IPU3_CSS_QUEUE_VF].fmt.mpix;
> > - int i, s;
> > + int i, s, ret;
> >
> > /* Adjust all formats, get statistics buffer sizes and formats */
> > for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> > @@ -1826,12 +1826,12 @@ int ipu3_css_fmt_try(struct ipu3_css *css,
> > s = (bds->height - gdc->height) / 2 - FILTER_SIZE;
> > env->height = s < MIN_ENVELOPE ? MIN_ENVELOPE : s;
> >
> > - css->pipes[pipe].bindex =
> > - ipu3_css_find_binary(css, pipe, q, r);
> > - if (css->pipes[pipe].bindex < 0) {
> > + ret = ipu3_css_find_binary(css, pipe, q, r);
> > + if (ret < 0) {
> > dev_err(css->dev, "failed to find suitable binary\n");
> > return -EINVAL;
> > }
> > + css->pipes[pipe].bindex = ret;
> >
> > dev_dbg(css->dev, "Binary index %d for pipe %d found.",
> > css->pipes[pipe].bindex, pipe);
> >
>
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12 00/13] media: staging/imx7: add i.MX7 media driver
Hi Rui, On Mon, Feb 04, 2019 at 12:00:26PM +, Rui Miguel Silva wrote: > Hi, > This series introduces the Media driver to work with the i.MX7 SoC. it uses > the > already existing imx media core drivers but since the i.MX7, contrary to > i.MX5/6, do not have an IPU and because of that some changes in the imx media > core are made along this series to make it support that case. > > This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several > configurations changes for this to work as a capture subsystem. Some bugs are > also fixed along the line. And necessary documentation. > > For a more detailed view of the capture paths, pads links in the i.MX7 please > take a look at the documentation in PATCH 10. > > The system used to test and develop this was the Warp7 board with an OV2680 > sensor, which output format is 10-bit bayer. So, only MIPI interface was > tested, a scenario with an parallel input would nice to have. > > Bellow goes an example of the output of the pads and links and the output of > v4l2-compliance testing. > > The v4l-utils version used is: > v4l2-compliance SHA: 1a6c8fe9a65c26e78ba34bd4aa2df28ede7d00cb, 32 bits > > The Media Driver fail some tests but this failures are coming from code out of > scope of this series (imx-capture), and some from the sensor OV2680 > but that I think not related with the sensor driver but with the testing and > core. > > The csi and mipi-csi entities pass all compliance tests. > > Cheers, > Rui > > v11->v12: > Sakari: > - check v4l2_ctrl_handler_free and init when exposed to userspace > - check csi_remove missing v4l2_async_notifier_unregister > - media device unregister before ctrl_handler_free > - GPL => GPL v2 > - Fix squash of CSI patches, issue on v11 > - add Acked-by: Sakari Ailus 10--13 > - mipi_s_stream check for ret < 0 and call pm_runtime_put_noidle > - use __maybe_unused in pm functions > - Extra space before labels For patches 1, 2 and 4: Acked-by: Sakari Ailus Thanks! -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] media: ipu3: shut up warnings produced with W=1
Hi Mauro, On Tue, Feb 19, 2019 at 09:00:29AM -0500, Mauro Carvalho Chehab wrote: > There are lots of warnings produced by this driver. It is not > as much as atomisp, but it is still a lot. > > So, use the same solution to hide most of them. > Those need to be fixed before promoting it out of staging, > so add it at the TODO list. > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/staging/media/ipu3/Makefile | 6 ++ > drivers/staging/media/ipu3/TODO | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/staging/media/ipu3/Makefile > b/drivers/staging/media/ipu3/Makefile > index fb146d178bd4..fa7fa3372bcb 100644 > --- a/drivers/staging/media/ipu3/Makefile > +++ b/drivers/staging/media/ipu3/Makefile > @@ -9,3 +9,9 @@ ipu3-imgu-objs += \ > ipu3-css.o ipu3-v4l2.o ipu3.o > > obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o > + > +# HACK! While this driver is in bad shape, don't enable several warnings > +# that would be otherwise enabled with W=1 > +ccflags-y += $(call cc-disable-warning, packed-not-aligned) > +ccflags-y += $(call cc-disable-warning, type-limits) > +ccflags-y += $(call cc-disable-warning, unused-const-variable) I'm preparing patches to address these. Could you wait a little bit more, please? Thanks. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] ipu3-mmu: fix some kernel-doc macros
On Tue, Feb 19, 2019 at 09:00:30AM -0500, Mauro Carvalho Chehab wrote: > Some kernel-doc markups are wrong. fix them. > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
Hi Bingbu, Arnd,
On Tue, Mar 05, 2019 at 12:25:18AM +, Cao, Bingbu wrote:
...
> > @@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > struct v4l2_rect *const bds = &r[IPU3_CSS_RECT_BDS];
> > struct v4l2_rect *const env = &r[IPU3_CSS_RECT_ENVELOPE];
> > struct v4l2_rect *const gdc = &r[IPU3_CSS_RECT_GDC];
> > - struct imgu_css_queue q[IPU3_CSS_QUEUES];
> > + struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
> > +imgu_css_queue), GFP_KERNEL);
>
> Could you use the devm_kcalloc()?
No, because this is not related to the device, called instead on
e.g. VIDIOC_TRY_FMT.
> > struct v4l2_pix_format_mplane *const in =
> > &q[IPU3_CSS_QUEUE_IN].fmt.mpix;
> > struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> > int imgu_css_fmt_try(struct imgu_css *css,
> > &q[IPU3_CSS_QUEUE_VF].fmt.mpix;
> > int i, s, ret;
> >
> > + if (!q) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> [Cao, Bingbu]
> The goto here is wrong, you can just report an error, and I prefer it is next
> to the alloc.
I agree, the goto is just not needed.
> > +
> > /* Adjust all formats, get statistics buffer sizes and formats */
> > for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> > if (fmts[i])
> > @@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> > dev_notice(css->dev, "can not initialize queue %s\n",
> >qnames[i]);
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > }
> > for (i = 0; i < IPU3_CSS_RECTS; i++) { @@ -1788,7 +1795,8 @@ int
> > imgu_css_fmt_try(struct imgu_css *css,
> > if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_IN]) ||
> > !imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) {
> > dev_warn(css->dev, "required queues are disabled\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) { @@ -1829,7
> > +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > ret = imgu_css_find_binary(css, pipe, q, r);
> > if (ret < 0) {
> > dev_err(css->dev, "failed to find suitable binary\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > css->pipes[pipe].bindex = ret;
> >
> > @@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> > dev_err(css->dev,
> > "final resolution adjustment failed\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > *fmts[i] = q[i].fmt.mpix;
> > }
> > @@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > bds->width, bds->height, gdc->width, gdc->height,
> > out->width, out->height, vf->width, vf->height);
> >
> > - return 0;
> > + ret = 0;
> > +out:
> > + kfree(q);
> > + return ret;
> > }
> >
> > int imgu_css_fmt_set(struct imgu_css *css,
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
On Tue, Mar 05, 2019 at 09:40:24AM +0100, Arnd Bergmann wrote:
> On Tue, Mar 5, 2019 at 8:53 AM Sakari Ailus
> wrote:
> > On Tue, Mar 05, 2019 at 12:25:18AM +, Cao, Bingbu wrote:
>
> > > > struct v4l2_pix_format_mplane *const in =
> > > > &q[IPU3_CSS_QUEUE_IN].fmt.mpix;
> > > > struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> > > > int imgu_css_fmt_try(struct imgu_css *css,
> > > > &q[IPU3_CSS_QUEUE_VF].fmt.mpix;
> > > > int i, s, ret;
> > > >
> > > > + if (!q) {
> > > > + ret = -ENOMEM;
> > > > + goto out;
> > > > + }
> > > [Cao, Bingbu]
> > > The goto here is wrong, you can just report an error, and I prefer it is
> > > next to the alloc.
> >
> > I agree, the goto is just not needed.
>
> Should I remove all the gotos then and do an explicit kfree() in each
> error path?
>
> I'd prefer not to mix the two styles, as that can lead to subtle mistakes
> when the code is refactored again.
In this case there's no need for kfree as q is NULL. Goto is often useful
if you need to do things to undo stuff that was done earlier in the
function but it's not the case here. I prefer keeping the rest gotos.
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] [v2] media: staging/intel-ipu3: reduce kernel stack usage
On Tue, Mar 05, 2019 at 02:26:29PM +0100, Arnd Bergmann wrote:
> The imgu_css_queue structure is too large to be put on the kernel
> stack, as we can see in 32-bit builds:
>
> drivers/staging/media/ipu3/ipu3-css.c: In function 'imgu_css_fmt_try':
> drivers/staging/media/ipu3/ipu3-css.c:1863:1: error: the frame size of 1172
> bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> By dynamically allocating this array, the stack usage goes down to an
> acceptable 140 bytes for the same x86-32 configuration.
>
> Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline
> programming")
> Signed-off-by: Arnd Bergmann
> ---
> v2: restructure to use 'return -ENOMEM' instead of goto for failed
> allocation.
Thanks, Arnd! All three applied.
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 11/41] media/v4l2-core/mm: convert put_page() to put_user_page*()
On Tue, Aug 06, 2019 at 06:33:10PM -0700, john.hubb...@gmail.com wrote:
> From: John Hubbard
>
> For pages that were retained via get_user_pages*(), release those pages
> via the new put_user_page*() routines, instead of via put_page() or
> release_pages().
>
> This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
> ("mm: introduce put_user_page*(), placeholder versions").
>
> Cc: Mauro Carvalho Chehab
> Cc: Kees Cook
> Cc: Hans Verkuil
> Cc: Sakari Ailus
> Cc: Jan Kara
> Cc: Robin Murphy
> Cc: Souptick Joarder
> Cc: Dan Williams
> Cc: linux-me...@vger.kernel.org
> Signed-off-by: John Hubbard
Acked-by: Sakari Ailus
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] ARM: dts: imx6ul: Add csi node
Hi Sébastien,
On Wed, Jul 31, 2019 at 06:32:57PM +0200, Sébastien Szymanski wrote:
> Add csi node for i.MX6UL SoC.
>
> Reviewed-by: Fabio Estevam
> Signed-off-by: Sébastien Szymanski
This should be probably merged through the ARM tree.
I can take the other two.
> ---
>
> Changes for v3:
> - none
>
> Changes for v2:
> - only "mclk" clock is required now.
>
> arch/arm/boot/dts/imx6ul.dtsi | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
> index 81d4b4925127..56cfcf0e5084 100644
> --- a/arch/arm/boot/dts/imx6ul.dtsi
> +++ b/arch/arm/boot/dts/imx6ul.dtsi
> @@ -957,6 +957,15 @@
> };
> };
>
> + csi: csi@21c4000 {
> + compatible = "fsl,imx6ul-csi", "fsl,imx7-csi";
> + reg = <0x021c4000 0x4000>;
> + interrupts = ;
> + clocks = <&clks IMX6UL_CLK_CSI>;
> + clock-names = "mclk";
> + status = "disabled";
> + };
> +
> lcdif: lcdif@21c8000 {
> compatible = "fsl,imx6ul-lcdif",
> "fsl,imx28-lcdif";
> reg = <0x021c8000 0x4000>;
--
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/15] [media] Move mediabus format definition to a more standard place
Hi, On Tue, Nov 04, 2014 at 12:09:59PM +0100, Hans Verkuil wrote: > Well, I gave two alternatives :-) > > Both are fine as far as I am concerned, but it would be nice to hear > what others think. In fact I think both are good options. :-) I'd perhaps lean towards the latter, for it has the benefit of pushing to use the new definitions and the old ones can be deprecated (and eventually removed in year 2030 or so ;)). Either way, preprocessor macros should be used instead of an enum since that way it's possible to figure out at that phase whether something is defined or not. There is for enums, too, but it results in a compilation error... -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/15] [media] v4l: Update subdev-formats doc with new MEDIA_BUS_FMT values
Hi Boris,
On Tue, Nov 04, 2014 at 10:54:57AM +0100, Boris Brezillon wrote:
> In order to have subsytem agnostic media bus format definitions we've
> moved media bus definition to include/uapi/linux/media-bus-format.h and
> prefixed enum values with MEDIA_BUS_FMT instead of V4L2_MBUS_FMT.
>
> Update the v4l documentation accordingly.
>
> Signed-off-by: Boris Brezillon
> ---
> Documentation/DocBook/media/Makefile | 2 +-
> Documentation/DocBook/media/v4l/subdev-formats.xml | 308
> ++---
> include/uapi/linux/v4l2-mediabus.h | 2 +
> 3 files changed, 157 insertions(+), 155 deletions(-)
>
...
> diff --git a/include/uapi/linux/v4l2-mediabus.h
> b/include/uapi/linux/v4l2-mediabus.h
> index f471064..9fbe891 100644
> --- a/include/uapi/linux/v4l2-mediabus.h
> +++ b/include/uapi/linux/v4l2-mediabus.h
> @@ -32,6 +32,8 @@ enum v4l2_mbus_pixelcode {
> MEDIA_BUS_TO_V4L2_MBUS(RGB888_2X12_BE),
> MEDIA_BUS_TO_V4L2_MBUS(RGB888_2X12_LE),
> MEDIA_BUS_TO_V4L2_MBUS(ARGB_1X32),
> + MEDIA_BUS_TO_V4L2_MBUS(RGB444_1X12),
> + MEDIA_BUS_TO_V4L2_MBUS(RGB565_1X16),
Shouldn't this to go to a separate patch?
>
> MEDIA_BUS_TO_V4L2_MBUS(Y8_1X8),
> MEDIA_BUS_TO_V4L2_MBUS(UV8_1X8),
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
Hi Boris, On Tue, Nov 04, 2014 at 10:55:06AM +0100, Boris Brezillon wrote: > The v4l2_mbus_pixelcode enum (or its values) should be replaced by the > media_bus_format enum. > Keep this enum in v4l2-mediabus.h and create a new header containing > the v4l2_mbus_framefmt struct definition (which is not deprecated) so > that we can add a #warning statement in v4l2-mediabus.h and hopefully > encourage users to move to the new definitions. > > Replace inclusion of v4l2-mediabus.h with v4l2-mbus.h in all common headers > and update the documentation Makefile to parse v4l2-mbus.h instead of > v4l2-mediabus.h. > > Signed-off-by: Boris Brezillon > --- > Documentation/DocBook/media/Makefile | 2 +- > include/media/v4l2-mediabus.h| 2 +- > include/uapi/linux/Kbuild| 1 + > include/uapi/linux/v4l2-mbus.h | 35 +++ > include/uapi/linux/v4l2-mediabus.h | 26 -- I would keep the original file name, even if the compatibility definitions are there. I don't see any harm in having them around as well. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/15] [media] Make use of the new media_bus_format definitions
On Tue, Nov 04, 2014 at 10:54:58AM +0100, Boris Brezillon wrote: > Replace references to the v4l2_mbus_pixelcode enum with the new > media_bus_format enum in all common headers. > > Signed-off-by: Boris Brezillon Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
Hi Boris, On Wed, Nov 05, 2014 at 04:15:38PM +0100, Boris Brezillon wrote: > On Wed, 5 Nov 2014 17:08:15 +0200 > Sakari Ailus wrote: > > I would keep the original file name, even if the compatibility definitions > > are there. I don't see any harm in having them around as well. > > > > That's the part I was not sure about. > The goal of this patch (and the following ones) is to deprecate > v4l2_mbus_pixelcode enum and its values by adding a #warning when > v4l2-mediabus.h file is included, thus encouraging people to use new > definitions. > > Do you see another solution to generate such warnings at compilation > time ? Do you think we need a warning? In a general case we can't start renaming interface headers once the preferred interface changes, albeit in this case it would be a possibility. The presence of the formats defined from now on only in the new definitions should be good enough. There are many cases such as this in the V4L2 and other APIs. I wonder what others think. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 01/10] [media] Move mediabus format definition to a more standard place
> - *
> - * The pixel codes are grouped by type, bus_width, bits per component,
> samples
> - * per pixel and order of subsamples. Numerical values are sorted using
> generic
> - * numerical sort order (8 thus comes before 10).
> - *
> - * As their value can't change when a new pixel code is inserted in the
> - * enumeration, the pixel codes are explicitly given a numerical value. The
> next
> - * free values for each category are listed below, update them when inserting
> - * new pixel codes.
> - */
> -enum v4l2_mbus_pixelcode {
> - V4L2_MBUS_FMT_FIXED = 0x0001,
> -
> - /* RGB - next is 0x100e */
> - V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE = 0x1001,
> - V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE = 0x1002,
> - V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE = 0x1003,
> - V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE = 0x1004,
> - V4L2_MBUS_FMT_BGR565_2X8_BE = 0x1005,
> - V4L2_MBUS_FMT_BGR565_2X8_LE = 0x1006,
> - V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007,
> - V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008,
> - V4L2_MBUS_FMT_RGB666_1X18 = 0x1009,
> - V4L2_MBUS_FMT_RGB888_1X24 = 0x100a,
> - V4L2_MBUS_FMT_RGB888_2X12_BE = 0x100b,
> - V4L2_MBUS_FMT_RGB888_2X12_LE = 0x100c,
> - V4L2_MBUS_FMT_ARGB_1X32 = 0x100d,
> -
> - /* YUV (including grey) - next is 0x2024 */
> - V4L2_MBUS_FMT_Y8_1X8 = 0x2001,
> - V4L2_MBUS_FMT_UV8_1X8 = 0x2015,
> - V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002,
> - V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003,
> - V4L2_MBUS_FMT_YUYV8_1_5X8 = 0x2004,
> - V4L2_MBUS_FMT_YVYU8_1_5X8 = 0x2005,
> - V4L2_MBUS_FMT_UYVY8_2X8 = 0x2006,
> - V4L2_MBUS_FMT_VYUY8_2X8 = 0x2007,
> - V4L2_MBUS_FMT_YUYV8_2X8 = 0x2008,
> - V4L2_MBUS_FMT_YVYU8_2X8 = 0x2009,
> - V4L2_MBUS_FMT_Y10_1X10 = 0x200a,
> - V4L2_MBUS_FMT_UYVY10_2X10 = 0x2018,
> - V4L2_MBUS_FMT_VYUY10_2X10 = 0x2019,
> - V4L2_MBUS_FMT_YUYV10_2X10 = 0x200b,
> - V4L2_MBUS_FMT_YVYU10_2X10 = 0x200c,
> - V4L2_MBUS_FMT_Y12_1X12 = 0x2013,
> - V4L2_MBUS_FMT_UYVY8_1X16 = 0x200f,
> - V4L2_MBUS_FMT_VYUY8_1X16 = 0x2010,
> - V4L2_MBUS_FMT_YUYV8_1X16 = 0x2011,
> - V4L2_MBUS_FMT_YVYU8_1X16 = 0x2012,
> - V4L2_MBUS_FMT_YDYUYDYV8_1X16 = 0x2014,
> - V4L2_MBUS_FMT_UYVY10_1X20 = 0x201a,
> - V4L2_MBUS_FMT_VYUY10_1X20 = 0x201b,
> - V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d,
> - V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e,
> - V4L2_MBUS_FMT_YUV10_1X30 = 0x2016,
> - V4L2_MBUS_FMT_AYUV8_1X32 = 0x2017,
> - V4L2_MBUS_FMT_UYVY12_2X12 = 0x201c,
> - V4L2_MBUS_FMT_VYUY12_2X12 = 0x201d,
> - V4L2_MBUS_FMT_YUYV12_2X12 = 0x201e,
> - V4L2_MBUS_FMT_YVYU12_2X12 = 0x201f,
> - V4L2_MBUS_FMT_UYVY12_1X24 = 0x2020,
> - V4L2_MBUS_FMT_VYUY12_1X24 = 0x2021,
> - V4L2_MBUS_FMT_YUYV12_1X24 = 0x2022,
> - V4L2_MBUS_FMT_YVYU12_1X24 = 0x2023,
> -
> - /* Bayer - next is 0x3019 */
> - V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001,
> - V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013,
> - V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002,
> - V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014,
> - V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8 = 0x3015,
> - V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8 = 0x3016,
> - V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8 = 0x3017,
> - V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8 = 0x3018,
> - V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
> - V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
> - V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,
> - V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8 = 0x300d,
> - V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE = 0x3003,
> - V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE = 0x3004,
> - V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE = 0x3005,
> - V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE = 0x3006,
> - V4L2_MBUS_FMT_SBGGR10_1X10 = 0x3007,
> - V4L2_MBUS_FMT_SGBRG10_1X10 = 0x300e,
> - V4L2_MBUS_FMT_SGRBG10_1X10 = 0x300a,
> - V4L2_MBUS_FMT_SRGGB10_1X10 = 0x300f,
> - V4L2_MBUS_FMT_SBGGR12_1X12 = 0x3008,
> - V4L2_MBUS_FMT_SGBRG12_1X12 = 0x3010,
> - V4L2_MBUS_FMT_SGRBG12_1X12 = 0x3011,
> - V4L2_MBUS_FMT_SRGGB12_1X12 = 0x3012,
> -
> - /* JPEG compressed formats - next is 0x4002 */
> - V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
> -
> - /* Vendor specific formats - next is 0x5002 */
> -
> - /* S5C73M3 sensor specific interleaved UYVY and JPEG */
> - V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 = 0x5001,
> -
> - /* HSV - next is 0x6002 */
> - V4L2_MBUS_FMT_AHSV_1X32 = 0x6001,
> -};
> +#define v4l2_mbus_pixelcode media_bus_format
>
> /**
> * struct v4l2_mbus_framefmt - frame format on the media bus
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 01/10] [media] Move mediabus format definition to a more standard place
_FMT(YUYV8_1_5X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU8_1_5X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(UYVY8_2X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(VYUY8_2X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUYV8_2X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU8_2X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(Y10_1X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(UYVY10_2X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(VYUY10_2X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUYV10_2X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU10_2X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(Y12_1X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(UYVY8_1X16), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(VYUY8_1X16), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUYV8_1X16), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU8_1X16), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YDYUYDYV8_1X16), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(UYVY10_1X20), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(VYUY10_1X20), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUYV10_1X20), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU10_1X20), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUV10_1X30), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(AYUV8_1X32), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(UYVY12_2X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(VYUY12_2X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUYV12_2X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU12_2X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(UYVY12_1X24), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(VYUY12_1X24), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YUYV12_1X24), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(YVYU12_1X24), > > - /* JPEG compressed formats - next is 0x4002 */ > - V4L2_MBUS_FMT_JPEG_1X8 = 0x4001, > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGBRG8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGRBG8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SRGGB8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_ALAW8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGBRG10_ALAW8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGRBG10_ALAW8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SRGGB10_ALAW8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_DPCM8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGBRG10_DPCM8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGRBG10_DPCM8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SRGGB10_DPCM8_1X8), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_2X8_PADHI_BE), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_2X8_PADHI_LE), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_2X8_PADLO_BE), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_2X8_PADLO_LE), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR10_1X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGBRG10_1X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGRBG10_1X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SRGGB10_1X10), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SBGGR12_1X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGBRG12_1X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SGRBG12_1X12), > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(SRGGB12_1X12), > > - /* Vendor specific formats - next is 0x5002 */ > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(JPEG_1X8), > > - /* S5C73M3 sensor specific interleaved UYVY and JPEG */ > - V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 = 0x5001, > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(S5C_UYVY_JPEG_1X8), > > - /* HSV - next is 0x6002 */ > - V4L2_MBUS_FMT_AHSV_1X32 = 0x6001, > + V4L2_MBUS_FROM_MEDIA_BUS_FMT(AHSV_1X32), > }; > > /** -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 10/10] [media] v4l: Forbid usage of V4L2_MBUS_FMT definitions inside the kernel
Hi Boris,
Boris Brezillon wrote:
> @@ -102,6 +113,7 @@ enum v4l2_mbus_pixelcode {
>
> V4L2_MBUS_FROM_MEDIA_BUS_FMT(AHSV_1X32),
> };
> +#endif /* __KERNEL__ */
>
> /**
> * struct v4l2_mbus_framefmt - frame format on the media bus
Was it intended to be this way, or did I miss something? I'd put this to
beginning of the file, as Hans suggested.
With this matter sorted out, for the set:
Acked-by: Sakari Ailus
--
Kind regards,
Sakari Ailus
sakari.ai...@iki.fi
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: use help instead of ---help--- in Kconfig
On Sat, Apr 20, 2019 at 11:04:07AM +, MosesChristopher wrote: > From: Moses Christopher > > - Resolve the following warning from the Kconfig, > "WARNING: prefer 'help' over '---help---' for new help texts" > > Signed-off-by: Moses Christopher Reviewed-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: schedule removal for legacy staging drivers
On Tue, Jun 25, 2019 at 08:48:26AM -0300, Mauro Carvalho Chehab wrote: > Keeping legacy problematic code forever is not a good idea. > > So, let's schedule a date for those legacy stuff to rest in piece. > > If someone wants to steps up and take them from the staging ostracism > and do give them a rejuvenation shower in order to address the > isues pointed on their TODO lists, be our guest! > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: media: sunxi: Add bool cast to value
On Mon, Jul 22, 2019 at 07:14:08PM +0530, Nishka Dasgupta wrote:
> On 22/07/19 5:54 PM, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote:
> > > On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote:
> > > > Typecast as bool the return value of cedrus_find_format in
> > > > cedrus_check_format as the return value of cedrus_check_format is
> > > > always treated like a boolean value.
> > > >
> > > > Signed-off-by: Nishka Dasgupta
> > > > ---
> > > > Changes in v2:
> > > > - Add !! to the returned pointer to ensure that the return value is
> > > >always either true or false, and never a non-zero value other than
> > > >true.
> > > >
> > > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > index e2b530b1a956..b731745f21f8 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32
> > > > pixelformat, u32 directions,
> > > > static bool cedrus_check_format(u32 pixelformat, u32 directions,
> > > > unsigned int capabilities)
> > > > {
> > > > - return cedrus_find_format(pixelformat, directions,
> > > > capabilities);
> > > > + return !!(bool)cedrus_find_format(pixelformat, directions,
> > > > capabilities);
> > > > }
> > >
> > > I think the original was fine. The return value of cedrus_find_format
> > > will be automatically converted to bool before being returned from
> > > cedrus_check_format since that is the return-type of the function, and
> > > the result of converting any non-zero value to bool is 1.
> >
> > Okay I was a bit unsure about that and wanted to play it on the safe side
> > without really looking it up, but that gave me the occasion to verify.
> >
> > From what I could find (from my GNU system's
> > /usr/include/unistring/stdbool.h):
> >
> > Limitations of this substitute, when used in a C89 environment:
> >
> > - In C99, casts and automatic conversions to '_Bool' or 'bool' are
> > performed in such a way that every nonzero value gets converted
> > to 'true', and zero gets converted to 'false'. This doesn't work
> > with this substitute. With this substitute, only the values 0
> > and 1
> > give the expected result when converted to _Bool' or 'bool'.
> >
> > So since the kernel is built for C89 (unless I'm mistaken), I don't think
> > the
> > compiler provides any guarantee about bool values being converted to 1 when
> > they are non-zero. As a result, I think it's best to be careful.
> >
> > However, I'm not sure I really see what cocinelle was unhappy about. You
> > mentionned single-line functions, but I don't see how that can be a problem.
>
> It's not a problem per se. I'm just working on a cleanup project for which I
> went through all of staging replacing single-line functions with what they
> were calling. In some cases that makes it easier to figure out what a
> particular function call does, since the called function actually does
> something itself instead of just calling a different function?
> This function was also flagged as one such potentially-removable function by
> Coccinelle; but in order to do the same replacement that I'd done in other
> staging drivers, I thought I would do something about the type mismatch
> first, especially since find_format doesn't appear to be used anywhere else.
> However, now I won't remove check_format and replace it with find_format as
> I'd originally planned, since you've said that isn't necessary here. That
> leaves the return type issue.
>
>
> > So in the end, I think we should keep the !! and drop the (bool) cast if
> > there's
> > no particular warning about it.
>
> Should I send a version 3 that does this?
bool was introduced in C99. Converting a non-zero value to boolean will
yield true as a result. Please keep the code as-is; it's much easier to
read that way.
--
Kind regards,
Sakari Ailus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging: ipu3: Enable IOVA API only when IOMMU support is enabled
Hi Yue,
On Mon, Jul 22, 2019 at 09:47:49PM +0800, YueHaibing wrote:
> If IOMMU_SUPPORT is not set, ipu3 driver may select IOMMU_IOVA to m.
> But for many drivers, they use "select IOMMU_IOVA if IOMMU_SUPPORT"
> in the Kconfig, for example, CONFIG_TEGRA_VDE is set to y but IOMMU_IOVA
> is m, then the building fails like this:
>
> drivers/staging/media/tegra-vde/iommu.o: In function `tegra_vde_iommu_map':
> iommu.c:(.text+0x41): undefined reference to `alloc_iova'
> iommu.c:(.text+0x56): undefined reference to `__free_iova'
>
> Reported-by: Hulk Robot
> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci
> device driver")
> Signed-off-by: YueHaibing
> ---
> drivers/staging/media/ipu3/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/Kconfig
> b/drivers/staging/media/ipu3/Kconfig
> index 4b51c67..b7df18f 100644
> --- a/drivers/staging/media/ipu3/Kconfig
> +++ b/drivers/staging/media/ipu3/Kconfig
> @@ -4,7 +4,7 @@ config VIDEO_IPU3_IMGU
> depends on PCI && VIDEO_V4L2
> depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
> depends on X86
> - select IOMMU_IOVA
> + select IOMMU_IOVA if IOMMU_SUPPORT
This doesn't seem right: the ipu3-cio2 driver needs IOMMU_IOVA
independently of IOMMU_SUPPORT.
Looking at tegra-vde, it seems to depend on IOMMU_SUPPORT but that's not
declared in its Kconfig entry. I wonder if adding that would be the right
way to fix this.
Cc'ing the IOMMU list.
> select VIDEOBUF2_DMA_SG
> help
> This is the Video4Linux2 driver for Intel IPU3 image processing unit,
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: atomisp: add a driver for ov5648 camera sensor
Hi Devid, Please see my comments below. Andy: please look for "INT5648". On Sun, Sep 10, 2017 at 02:23:07PM +0200, Devid Antonio Floni wrote: > The ov5680 5-megapixel camera sensor from OmniVision supports up to 2592x1944 > resolution and MIPI CSI-2 interface. Output format is raw sRGB/Bayer with > 10 bits per colour (SGRBG10_1X10). > > This patch is a port of ov5680 driver from following > 01org/ProductionKernelQuilts patches: > - 0004-ov2680-ov5648-Fork-lift-source-from-CTS.patch > - 0005-ov2680-ov5648-gminification.patch > - 0006-ov5648-Focus-support.patch > - 0007-Fix-resolution-issues-on-rear-camera.patch > - 0008-ov2680-ov5648-Enabled-the-set_exposure-functions.patch > - 0010-IRDA-cameras-mode-list-cleanup-unification.patch > - 0012-ov5648-Add-1296x972-binned-mode.patch > - 0014-ov5648-Adapt-to-Atomisp2-Gmin-VCM-framework.patch > - 0015-dw9714-Gmin-Atomisp-specific-VCM-driver.patch > - 0017-ov5648-Fix-deadlock-on-I2C-error.patch > - 0018-gc2155-Fix-deadlock-on-error.patch > - 0019-ov5648-Add-1280x960-binned-mode.patch > - 0020-ov5648-Make-1280x960-as-default-video-resolution.patch > - 0021-MALATA-Fix-testCameraToSurfaceTextureMetadata-CTS.patch > - 0023-OV5648-Added-5MP-video-resolution.patch > > New changes introduced during the port: > - Rename entity types to entity functions > - Replace v4l2_subdev_fh by v4l2_subdev_pad_config > - Make use of media_bus_format enum > - Rename media_entity_init function to media_entity_pads_init > - Replace try_mbus_fmt by set_fmt > - Replace s_mbus_fmt by set_fmt > - Replace g_mbus_fmt by get_fmt > - Use s_ctrl/g_volatile_ctrl instead of ctrl core ops > - Update gmin platform API path > - Update and constify acpi_device_id > - Fix some checkpatch errors and warnings > - Remove FSF's mailing address from the sample GPL notice > > I was not able to properly test this patch on my Lenovo Miix 310 due to other > issues with atomisp, the output is the same as ov2680 driver which is very > similar. Has this been somehow tested? > > Signed-off-by: Devid Antonio Floni > --- > drivers/staging/media/atomisp/i2c/Kconfig | 11 + > drivers/staging/media/atomisp/i2c/Makefile |1 + > drivers/staging/media/atomisp/i2c/ov5648.c | 1867 > > drivers/staging/media/atomisp/i2c/ov5648.h | 835 + > 4 files changed, 2714 insertions(+) > create mode 100644 drivers/staging/media/atomisp/i2c/ov5648.c > create mode 100644 drivers/staging/media/atomisp/i2c/ov5648.h > > diff --git a/drivers/staging/media/atomisp/i2c/Kconfig > b/drivers/staging/media/atomisp/i2c/Kconfig > index b80d29d..8ed2cf4 100644 > --- a/drivers/staging/media/atomisp/i2c/Kconfig > +++ b/drivers/staging/media/atomisp/i2c/Kconfig > @@ -89,6 +89,17 @@ config VIDEO_OV2680 > > It currently only works with the atomisp driver. > > +config VIDEO_OV5648 It'd be good to have ATOMISP in the option as well (the others don't have it, I think I'll submit a patch) so that these won't collide with the real V4L2 subdev drivers. E.g. VIDEO_ATOMISP_OV5648. > + tristate "Omnivision OV5648 sensor support" > + depends on I2C && VIDEO_V4L2 > + ---help--- > + This is a Video4Linux2 sensor-level driver for the Omnivision > + OV5648 raw camera. > + > + ov5648 is a 5M raw sensor. > + > + It currently only works with the atomisp driver. > + While it's nice to see a new sensor driver, it'd be even better if the atomisp driver was modified to work with V4L2 sub-device sensor drivers e.g. under drivers/media/i2c. I'm not saying no to adding this specific driver as such though, this is staging... I wonder what others think. > # > # Kconfig for flash drivers > # > diff --git a/drivers/staging/media/atomisp/i2c/Makefile > b/drivers/staging/media/atomisp/i2c/Makefile > index be13fab..26195d7 100644 > --- a/drivers/staging/media/atomisp/i2c/Makefile > +++ b/drivers/staging/media/atomisp/i2c/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_VIDEO_MT9M114)+= mt9m114.o > obj-$(CONFIG_VIDEO_GC2235) += gc2235.o > obj-$(CONFIG_VIDEO_OV2722) += ov2722.o > obj-$(CONFIG_VIDEO_OV2680) += ov2680.o > +obj-$(CONFIG_VIDEO_OV5648) += ov5648.o > obj-$(CONFIG_VIDEO_GC0310) += gc0310.o > > obj-$(CONFIG_VIDEO_MSRLIST_HELPER) += libmsrlisthelper.o > diff --git a/drivers/staging/media/atomisp/i2c/ov5648.c > b/drivers/staging/media/atomisp/i2c/ov5648.c > new file mode 100644 > index 000..4815a19 > --- /dev/null > +++ b/drivers/staging/media/atomisp/i2c/ov5648.c > @@ -0,0 +1,1867 @@ > +/* > + * Support for OmniVision OV5648 5M camera sensor. > + * Based on OmniVision OV2722 driver. > + * > + * Copyright (c) 2013 Intel Corporation. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be use
Re: [PATCH] Staging: atomisp: fix alloc_cast.cocci warnings
Hi Branislav, On Thu, Sep 07, 2017 at 06:26:42PM +0200, Branislav Radocaj wrote: > Remove casting the values returned by memory allocation functions > like kmalloc, kzalloc, kmem_cache_alloc, kmem_cache_zalloc etc. > > Semantic patch information: > This makes an effort to find cases of casting of values returned by > kmalloc, kzalloc, kcalloc, kmem_cache_alloc, kmem_cache_zalloc, > kmem_cache_alloc_node, kmalloc_node and kzalloc_node and removes > the casting as it is not required. The result in the patch case may > need some reformatting. > > Generated by: scripts/coccinelle/api/alloc/alloc_cast.cocci > > Signed-off-by: Branislav Radocaj > --- > drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git > a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > index eecd8cf71951..56765d6a0498 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > @@ -133,7 +133,7 @@ sh_css_load_blob_info(const char *fw, const struct > ia_css_fw_info *bi, struct ia > char *namebuffer; > int namelength = (int)strlen(name); > > - namebuffer = (char *) kmalloc(namelength + 1, GFP_KERNEL); > + namebuffer = kmalloc(namelength + 1, GFP_KERNEL); This chunk no longer applies, I'm applying the one that does. The kmalloc has been replaced by kstrdup. > if (namebuffer == NULL) > return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY; > > @@ -149,7 +149,7 @@ sh_css_load_blob_info(const char *fw, const struct > ia_css_fw_info *bi, struct ia > size_t configstruct_size = sizeof(struct > ia_css_config_memory_offsets); > size_t statestruct_size = sizeof(struct > ia_css_state_memory_offsets); > > - char *parambuf = (char *)kmalloc(paramstruct_size + > configstruct_size + statestruct_size, > + char *parambuf = kmalloc(paramstruct_size + configstruct_size + > statestruct_size, > GFP_KERNEL); > if (parambuf == NULL) > return IA_CSS_ERR_CANNOT_ALLOCATE_MEMORY; -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: media: atomisp: Merge assignment with return
On Tue, Sep 12, 2017 at 07:55:07PM +0530, Srishti Sharma wrote: > Merge the assignment and the return statements to return the value > directly. Done using the following semantic patch by coccinelle. > > @@ > local idexpression ret; > expression e; > @@ > > -ret = > +return > e; > -return ret; > > Signed-off-by: Srishti Sharma Hi Srishti, I've merged the two patches as they're trivial and the commit message is the same. It's entirely reasonable to have a patch per driver but you should mention that in the commit message (subject line) at least. This case is a bit special because the other driver is also specific to the atomisp staging driver. Thanks. -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] [media] staging: atomisp: use clock framework for camera clocks
Hi Pierre-Louis,
On Wed, Sep 20, 2017 at 03:53:58PM -0500, Pierre-Louis Bossart wrote:
> The Atom ISP driver initializes and configures PMC clocks which are
> already handled by the clock framework.
>
> Remove all legacy vlv2_platform_clock stuff and move to the clk API to
> avoid conflicts, e.g. with audio machine drivers enabling the MCLK for
> external codecs
>
> Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> Tested-by: Carlo Caione
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Pierre-Louis Bossart
I've applied the patch with small changes, there were other patches
changing the deleted files.
The tree is here:
https://git.linuxtv.org/sailus/media_tree.git/log/?h=atomisp>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
Hi Mauro, On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote: > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. The idea is that in the union, there's a struct which is specific to the match_type field. I wouldn't call it senseless. In the two cases there's just a single field in the containing struct. You could remove the struct in that case as you do in this patch, and just use the field. But I think the result is less clean and so I wouldn't make this change. The confusion comes possibly from the fact that the struct is named the same as the field in the struct. These used to be called of and node, but with the fwnode property framework the references to the fwnode are, well, typically similarly called "fwnode". There's no underlying firmware interface with that name, fwnode property API is just an API. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
Hi Mauro, On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote: > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. The idea is that in the union, there's a struct which is specific to the match_type field. I wouldn't call it senseless. In the two cases there's just a single field in the containing struct. You could remove the struct in that case as you do in this patch, and just use the field. But I think the result is less clean and so I wouldn't make this change. The confusion comes possibly from the fact that the struct is named the same as the field in the struct. These used to be called of and node, but with the fwnode property framework the references to the fwnode are, well, typically similarly called "fwnode". There's no underlying firmware interface with that name, fwnode property API is just an API. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
Hi Mauro, On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote: > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. The idea is that in the union, there's a struct which is specific to the match_type field. I wouldn't call it senseless. In the two cases there's just a single field in the containing struct. You could remove the struct in that case as you do in this patch, and just use the field. But I think the result is less clean and so I wouldn't make this change. The confusion comes possibly from the fact that the struct is named the same as the field in the struct. These used to be called of and node, but with the fwnode property framework the references to the fwnode are, well, typically similarly called "fwnode". There's no underlying firmware interface with that name, fwnode property API is just an API. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
Hi Mauro,
(Removing the non-list recipients.)
On Fri, Sep 29, 2017 at 06:27:13AM -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Sep 2017 15:09:21 +0300
> Sakari Ailus escreveu:
>
> > Hi Mauro,
> >
> > On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote:
> > > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> > > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> > > match criteria requires just a device name.
> > >
> > > So, it doesn't make sense to enclose those into structs,
> > > as the criteria can go directly into the union.
> > >
> > > That makes easier to document it, as we don't need to document
> > > weird senseless structs.
> >
> > The idea is that in the union, there's a struct which is specific to the
> > match_type field. I wouldn't call it senseless.
>
> Why a struct for each specific match_type is **needed**? It it is not
> needed, then it is senseless per definition :-)
>
> In the specific case of fwnode, there's already a named struct
> for fwnode_handle. The only thing is that it is declared outside
> enum v4l2_async_match_type. So, I don't see any reason to do things
> like:
>
> struct {
> struct fwnode_handle *fwnode;
> } fwnode;
>
> If you're in doubt about that, think on how would you document
> both fwnode structs. Both fwnode structs specify the match
> criteria if %V4L2_ASYNC_MATCH_FWNODE.
>
> The same applies to this:
>
> struct {
> const char *name;
> } device_name;
>
> Both device_name and name specifies the match criteria if
> %V4L2_ASYNC_MATCH_DEVNAME.
>
> >
> > In the two cases there's just a single field in the containing struct. You
> > could remove the struct in that case as you do in this patch, and just use
> > the field. But I think the result is less clean and so I wouldn't make this
> > change.
>
> It is actually cleaner without the stucts.
>
> Without the useless struct, if one wants to match a firmware node, it
> should be doing:
>
> pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);
This code should be and will be moved out of drivers. See:
http://www.spinics.net/lists/linux-media/msg122688.html>
So there are going to be quite a bit fewer instances of it, and none should
remain in drivers. I frankly don't have a strong opinion on this; there are
arguments for and against. I just don't see a reason to change it.
It'd be nice to have Hans's and Laurent's opinion though.
>
> And that' it. For anyone that reads the above code, even not knowing
> all details of "match", is clear that the match criteria is whatever
> of_fwnode_handle() returns.
>
> Now, on this:
>
> pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> pdata->asd[i]->match.fwnode.fwnode = of_fwnode_handle(rem);
>
> It sounds that something is missing, as only one field of match.fwnode
> was specified. Anyone not familiar with that non-conventional usage of
> a struct with just one struct field inside would need to seek for the
> header file declaring the struct. That would consume a lot of time for
> code reviewers for no good reason.
>
> The same apply for devname search:
>
> In this case:
> asd->match_type = V4L2_ASYNC_MATCH_DEVNAME;
> asd->match.device_name.name = imxsd->devname;
>
> I would be expecting something else to be filled at device_name's
> struct, for example to specify a case sensitive or case insensitive
> match criteria, to allow seeking for a device's substring, or to
> allow using other struct device fields to narrow the seek.
>
> With this:
>
> asd->match_type = V4L2_ASYNC_MATCH_DEVNAME;
> asd->match.device_name = imxsd->devname;
>
> It is clear that the match criteria is fully specified.
>
> > The confusion comes possibly from the fact that the struct is named the
> > same as the field in the struct. These used to be called of and node, but
> > with the fwnode property framework the references to the fwnode are, well,
> > typically similarly called "fwnode". There's no underlying firmware
> > interface with that name, fwnode property API is just an API.
>
> The duplicated "fwnode" name only made it more evident that we don't
> need to enclose a single match criteria field inside a struct.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: atomisp: add a driver for ov5648 camera sensor
Hi Devid, On Wed, Oct 04, 2017 at 01:23:36AM +0200, Devid Antonio Filoni wrote: > I was not able to properly test this patch on my Lenovo Miix 310 due to other > issues with atomisp, the output is the same as ov2680 driver (OVTI2680) which > is very similar to ov5648. As reported by dmesg, atomisp-gmin-platform fails > to load CamClk, ClkSrc, CsiPort, CsiLanes variables from ACPI (although they > are set as showed by DSDT) and it fails to get regulators. My Miix 310 uses > AXP PMIC (INT33F4:00) which, as far as I can understand by looking at > 01org/ProductionKernelQuilts code, it's yet not supported by mainline kernel. Hmm. In other words this driver isn't usable due to lack of other drivers. Even if they'd be in place, this hasn't been tested. What would be the likelihood of it functioning in the end? Would it make sense to first get the other drivers to upstream and then see what's the status of atomisp? The atomisp driver has a number of entries in its TODO file, one of which is making the sensor drivers to use the standard kernel interfaces to work with the main driver (atomisp), so that they could be used with any ISP / bridge driver. Also, the interface the atomisp uses with the sensor drivers as well as how the board specific information from firmware is conveyed to the sensor drivers will change to what the rest of the sensor drivers are using. I think a most straightforward way would be to amend the ACPI tables to include the necessary information. Therefore, applying this patch would not have an effective improvement to the users in form of support for new systems and would make the upcoming interface changes more difficult as there would be one more driver to change (and test). For this reason I'm tempted to postpone applying this patch at least until the other drivers are available. Andy, Alan; any opinion? -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: atomisp: add a driver for ov5648 camera sensor
Hi Alan, On Tue, Oct 10, 2017 at 10:08:40AM +0100, Alan Cox wrote: > > Would it make sense to first get the other drivers to upstream and > > then see what's the status of atomisp? > > Agreed > > > the board specific information from firmware is conveyed to the > > sensor drivers will change to what the rest of the sensor drivers are > > using. I think a most straightforward way would be to amend the ACPI > > tables to include the necessary information. > > I don't see that happening. The firmware they have today is the > firmware they will always have, and for any new devices we manage to > get going is probably going to end up entirely hardcoded. You'd need to pass the table use initrd to amend the existing tables. Another option would be to parse the information from ACPI / EFI variables or whatever to set things up in the atomisp driver, and rely on e.g. I²C matching in the V4L2 async framework. The board specific information needed by the sensor drivers need to be somehow conveyed to the drivers. Perhaps using pset / device_add_property()? This is not trivial and would require at least either V4L2 framework or sensor driver changes to support atomisp, which currently is a staging driver. -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/2] staginng: atomisp: memory allocation cleanups
Hi Aishwarya, On Sun, Oct 08, 2017 at 02:53:20PM +0530, Aishwarya Pant wrote: > Patch series performs minor code cleanups using coccinelle to simplify memory > allocation tests and remove redundant OOM log messages. > > Aishwarya Pant (2): > staging: atomisp2: cleanup null check on memory allocation > staging: atomisp: cleanup out of memory messages Thanks for the patchset. Unfortunately neither applies anymore after other patches have been merged. Could you rebase yours on top of this tree, please? https://git.linuxtv.org/sailus/media_tree.git/log/?h=atomisp> -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging/atomisp: Convert timers to use timer_setup()
On Mon, Oct 16, 2017 at 04:24:56PM -0700, Kees Cook wrote: > In preparation for unconditionally passing the struct timer_list pointer to > all timer callbacks, switch to using the new timer_setup() and from_timer() > to pass the timer pointer explicitly. > > Cc: Mauro Carvalho Chehab > Cc: Greg Kroah-Hartman > Cc: Alan Cox > Cc: Daeseok Youn > Cc: Arnd Bergmann > Cc: linux-me...@vger.kernel.org > Cc: de...@driverdev.osuosl.org > Signed-off-by: Kees Cook This appears to be the same as the patch I've applied previously. -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] stagin: atomisp: Fix oops by unbalanced clk enable/disable call
On Tue, Oct 17, 2017 at 08:10:15AM +1100, Tobin C. Harding wrote:
> On Mon, Oct 16, 2017 at 02:34:48PM +0200, Hans de Goede wrote:
> > diff --git
> > a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > index 828fe5abd832..6671ebe4ecc9 100644
> > ---
> > a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > +++
> > b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > @@ -29,6 +29,7 @@ struct gmin_subdev {
> > struct v4l2_subdev *subdev;
> > int clock_num;
> > int clock_src;
> > + bool clock_on;
> > struct clk *pmc_clk;
> > struct gpio_desc *gpio0;
> > struct gpio_desc *gpio1;
> > @@ -583,6 +584,9 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev
> > *subdev, int on)
> > struct gmin_subdev *gs = find_gmin_subdev(subdev);
> > struct i2c_client *client = v4l2_get_subdevdata(subdev);
> >
> > + if (gs->clock_on == !!on)
> > + return 0;
> > +
> > if (on) {
> > ret = clk_set_rate(gs->pmc_clk, gs->clock_src);
>
> Which tree [and branch] are you working off please? In the staging-next
> branch of Greg's staging
> tree this function does not appear as it is in this patch.
Media tree master.
https://git.linuxtv.org/media_tree.git/log/>
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/2] staging: atomisp: memory allocation cleanups
Hi Aishwarya, On Sat, Oct 14, 2017 at 07:24:00PM +0530, Aishwarya Pant wrote: > Patch series performs minor code cleanups using coccinelle to simplify memory > allocation tests and remove redundant OOM log messages. > > Changes in v2: > Rebase and re-send patches > > Aishwarya Pant (2): > staging: atomisp2: cleanup null check on memory allocation > staging: atomisp: cleanup out of memory messages Thank you for the update, but the patches still don't apply. Some patches have been merged since I last reviewed these and I've already sent a pull request for the branch. Could you rebase your patches on this branch, please? https://git.linuxtv.org/sailus/media_tree.git/log/?h=atomisp-next> Normally it'll be "atomisp". -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging/atomisp: Convert timers to use timer_setup()
On Tue, Oct 17, 2017 at 08:16:03AM -0700, Kees Cook wrote: > On Tue, Oct 17, 2017 at 1:23 AM, Sakari Ailus wrote: > > On Mon, Oct 16, 2017 at 04:24:56PM -0700, Kees Cook wrote: > >> In preparation for unconditionally passing the struct timer_list pointer to > >> all timer callbacks, switch to using the new timer_setup() and from_timer() > >> to pass the timer pointer explicitly. > >> > >> Cc: Mauro Carvalho Chehab > >> Cc: Greg Kroah-Hartman > >> Cc: Alan Cox > >> Cc: Daeseok Youn > >> Cc: Arnd Bergmann > >> Cc: linux-me...@vger.kernel.org > >> Cc: de...@driverdev.osuosl.org > >> Signed-off-by: Kees Cook > > > > This appears to be the same as the patch I've applied previously. > > Okay, sorry for the noise. I didn't see it in -next when I did my > rebase this week. No worries, it'll get there through mediatree... but it'll still take some time (rc3 not there yet). -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3 22/29] [media] atomisp: deprecate pci_get_bus_and_slot()
Hi Sinan,
On Mon, Nov 27, 2017 at 11:57:59AM -0500, Sinan Kaya wrote:
> diff --git
> a/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
> b/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
> index 4631b1d..51dcef57 100644
> --- a/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
> +++ b/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
> @@ -39,7 +39,7 @@ static inline int platform_is(u8 model)
>
> static int intel_mid_msgbus_init(void)
> {
> - pci_root = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> + pci_root = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> if (!pci_root) {
> pr_err("%s: Error: msgbus PCI handle NULL\n", __func__);
> return -ENODEV;
This file has been removed, I'm applying the rest of the patch.
Please use the media tree as the base in the future. Thanks.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] media: staging: atomisp: prefer s16 to int16_t.
On Mon, Nov 27, 2017 at 11:30:54AM +, Jeremy Sowden wrote: > Signed-off-by: Jeremy Sowden I'd just leave it as-is, int16_t is a standard type. The commit message would be needed, too. -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/3] Sparse fixes for the Atom ISP Staging Driver
On Tue, Nov 28, 2017 at 10:27:24AM +, Jeremy Sowden wrote: > Fixed some sparse warnings in the Atom ISP staging driver. > > This time with longer commit messages. :) > > I've chosen to ignore checkpatch.pl's suggestion to change the types of > the arrays in the second patch from int16_t to s16. > > Jeremy Sowden (2): > media: staging: atomisp: fix for sparse "using plain integer as NULL > pointer" warnings. > media: staging: atomisp: fixes for "symbol was not declared. Should it > be static?" sparse warnings. > > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c| 24 > +++--- > .../isp_param/interface/ia_css_isp_param_types.h | 2 +- > 2 files changed, 13 insertions(+), 13 deletions(-) Thanks, applied! -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] media: staging: atomisp: defined as static some const arrays which don't need external linkage.
Hi Greg, On Mon, Nov 27, 2017 at 01:21:25PM +0100, Greg KH wrote: > On Mon, Nov 27, 2017 at 11:30:53AM +, Jeremy Sowden wrote: > > Signed-off-by: Jeremy Sowden > > --- > > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c| 24 > > +++--- > > 1 file changed, 12 insertions(+), 12 deletions(-) > > I can never take patches without any changelog text, and so no one > should write them that way :) I've been applying the atomisp patches to my tree for some time now, further on passing them to Mauro to be merged via the media tree. To avoid conflicts, I suggest to avoid merging them via the staging tree. Thanks. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] media: staging: atomisp: defined as static some const arrays which don't need external linkage.
On Wed, Nov 29, 2017 at 10:15:06AM +0100, Greg KH wrote: > On Wed, Nov 29, 2017 at 11:08:17AM +0200, Sakari Ailus wrote: > > Hi Greg, > > > > On Mon, Nov 27, 2017 at 01:21:25PM +0100, Greg KH wrote: > > > On Mon, Nov 27, 2017 at 11:30:53AM +, Jeremy Sowden wrote: > > > > Signed-off-by: Jeremy Sowden > > > > --- > > > > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c| 24 > > > > +++--- > > > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > > > I can never take patches without any changelog text, and so no one > > > should write them that way :) > > > > I've been applying the atomisp patches to my tree for some time now, > > further on passing them to Mauro to be merged via the media tree. To avoid > > conflicts, I suggest to avoid merging them via the staging tree. > > I'm not touching any of these patches, just commenting that patches > without changelogs should not be written :) Ack, just wanted to avoid potential mege conflicts. Thanks. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] media: atomisp: stop producing hundreds of kernel-doc warnings
Hi Mauro, Thanks for the patch. On Wed, Nov 29, 2017 at 07:08:04AM -0500, Mauro Carvalho Chehab wrote: > A recent change on Kernel 4.15-rc1 causes all tags with > /** to be handled as kernel-doc markups. Well, several > atomisp modules, it doesn't use kernel-doc, but some other > documentation markup (doxygen?). > > So, suppress all those warns by replacing /** by /*. > > Signed-off-by: Mauro Carvalho Chehab I presume you haven't written the patch manually. There are other changes that described by the comment, too, such as removing lesser than-characaters. It'd be good to mention how it's been generated. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] media: atomisp: stop producing hundreds of kernel-doc warnings
On Wed, Nov 29, 2017 at 10:28:26AM -0200, Mauro Carvalho Chehab wrote: > Em Wed, 29 Nov 2017 10:24:57 -0200 > Mauro Carvalho Chehab escreveu: > > > Em Wed, 29 Nov 2017 14:14:54 +0200 > > Sakari Ailus escreveu: > > > > > Hi Mauro, > > > > > > Thanks for the patch. > > > > > > On Wed, Nov 29, 2017 at 07:08:04AM -0500, Mauro Carvalho Chehab wrote: > > > > A recent change on Kernel 4.15-rc1 causes all tags with > > > > /** to be handled as kernel-doc markups. Well, several > > > > atomisp modules, it doesn't use kernel-doc, but some other > > > > documentation markup (doxygen?). > > > > > > > > So, suppress all those warns by replacing /** by /*. > > > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > > > > I presume you haven't written the patch manually. There are other changes > > > that described by the comment, too, such as removing lesser > > > than-characaters. > > > > > > It'd be good to mention how it's been generated. > > > > Yeah, I used a simple script, and manually fixed a few minor things > > that were still causing warnings. > > > > The core changes were done via: > > > > for i in $(find drivers/staging/media/atomisp -type f); do sed 's,/\*\* > > ,/\*, ' -i $i; done > > for i in $(find drivers/staging/media/atomisp -type f); do sed > > 's,/\*\*<,/\**,' -i $i; done > > for i in > > drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c > > drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_sp.c > > drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/rmgr/src/rmgr_vbuf.c; > > do perl -ne 's,\/\*\*$,/*,g; print $_' $i > a && mv a $i; done; > > > > I'll add it at the patch description. > > > > Thanks, > > Mauro > > Changed patch description to the one enclosed. > > > commit 699a8caa3de69b2bdaa54b9347c29644bca6 > Author: Mauro Carvalho Chehab > Date: Wed Nov 29 03:16:17 2017 -0500 > > media: atomisp: stop producing hundreds of kernel-doc warnings > > A recent change on Kernel 4.15-rc1 causes all tags with > /** to be handled as kernel-doc markups. Well, several > atomisp modules, it doesn't use kernel-doc, but some other > documentation markup (doxygen?). > > So, suppress all those warns by: > - replacing /**< by /**. > - replacing /** by /*. > > The core changes were done with: > > for i in $(find drivers/staging/media/atomisp -type f); do sed > 's,/\*\* ,/\*, ' -i $i; done > for i in $(find drivers/staging/media/atomisp -type f); do sed > 's,/\*\*<,/\**,' -i $i; done > for i in > drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c > drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_sp.c > drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/rmgr/src/rmgr_vbuf.c; > do perl -ne 's,\/\*\*$,/*,g; print $_' $i > a && mv a $i; done; > > A few manual adjustments were made, where needed. > > Signed-off-by: Mauro Carvalho Chehab Thanks. The patch doesn't conflict with the other atomisp patches I have already so feel free to apply it directly. Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/8] media: v4l2-async: simplify v4l2_async_subdev structure
On Mon, Dec 18, 2017 at 05:53:57PM -0200, Mauro Carvalho Chehab wrote: > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. > > At drivers, this makes even clearer about the match criteria. > > Acked-by: Sylwester Nawrocki > Signed-off-by: Mauro Carvalho Chehab I'm not sure this is needed but it doesn't break anything either. Feel free to add: Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.
Hi Jeremy, On Sat, Dec 02, 2017 at 10:11:59PM +, Jeremy Sowden wrote: > The CSS API uses a lot of nested anonymous structs defined in object > macros to assign default values to its data-structures. These have been > changed to use compound-literals and designated initializers to make > them more comprehensible and less fragile. > > The compound-literals can also be used in assignment, which means we can > get rid of some temporary variables whose only purpose is to be > initialized by one of these anonymous structs and then serve as the > rvalue in an assignment expression. > > Signed-off-by: Jeremy Sowden I don't think it's useful to change the struct definition macros only to remove a large number of assigned fields in the next patch. How about merging the two patches? Please also start a new thread when re-posting a set. Thanks. -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: delete empty default struct values.
On Sat, Dec 02, 2017 at 10:12:01PM +, Jeremy Sowden wrote: > Removing zero-valued struct-members left a number of the default > struct-values empty. These values have now been removed. > > Signed-off-by: Jeremy Sowden This one should be squashed as well. -- Sakari Ailus e-mail: sakari.ai...@iki.fi ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 07/13] media: dt-bindings: add bindings for i.MX7 media driver
tle : differential receiver (HS-RX) settle time;
> +
> +port node
> +-
> +
> +- reg : (required) can take the values 0 or 1, where 0 is
> the
> + related sink port and port 1 should be the source one;
Should and is -> shall?
I think you should also elaborate whether or not the port (as well as the
endpoint) nodes themselves are mandatory.
> +
> +endpoint node
> +-
> +
> +- data-lanes: (required) an array specifying active physical MIPI-CSI2
> + data input lanes and their mapping to logical lanes; the
> + array's content is unused, only its length is meaningful;
If this is for port 0 only, please document that. Which values (length) are
allowed?
> +
> +example:
> +
> +mipi_csi: mipi-csi@3075 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +compatible = "fsl,imx7-mipi-csi2";
> +reg = <0x3075 0x1>;
> +interrupts = ;
> +clocks = <&clks IMX7D_IPG_ROOT_CLK>,
> +<&clks IMX7D_MIPI_CSI_ROOT_CLK>,
> +<&clks IMX7D_MIPI_DPHY_ROOT_CLK>;
> +clock-names = "pclk", "wrap", "phy";
> +clock-frequency = <16600>;
> +power-domains = <&pgc_mipi_phy>;
> +phy-supply = <®_1p0d>;
> +resets = <&src IMX7_RESET_MIPI_PHY_MRST>;
> +reset-names = "mrst";
> +fsl,csis-hs-settle = <3>;
> +
> +port@0 {
> +reg = <0>;
> +
> +mipi_from_sensor: endpoint {
> +remote-endpoint = <&ov2680_to_mipi>;
> +data-lanes = <1>;
> +};
> +};
> +
> +port@1 {
> +reg = <1>;
> +
> +mipi_vc0_to_csi_mux: endpoint {
> +remote-endpoint = <&csi_mux_from_mipi_vc0>;
> +};
> +};
> +};
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 00/36] i.MX Media Driver
Hi Philipp, Steve and Russell, On Fri, Feb 17, 2017 at 12:43:38PM +0100, Philipp Zabel wrote: > On Thu, 2017-02-16 at 14:27 -0800, Steve Longerbeam wrote: > > > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote: > > > On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote: > > >> In version 4: > > > > > > With this version, I get: > > > > > > [28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x > > > [28762.899409] ipu1_csi0: pipeline_set_stream failed with -110 > > > > > > > Right, in the imx219, on exit from s_power(), the clock and data lanes > > must be placed in the LP-11 state. This has been done in the ov5640 and > > tc358743 subdevs. > > > > If we want to bring in the patch that adds a .prepare_stream() op, > > the csi-2 bus would need to be placed in LP-11 in that op instead. > > > > Philipp, should I go ahead and add your .prepare_stream() patch? > > I think with Russell's explanation of how the imx219 sensor operates, > we'll have to do something before calling the sensor s_stream, but right > now I'm still unsure what exactly. Indeed there appears to be no other way to achieve the LP-11 state than going through the streaming state for this particular sensor, apart from starting streaming. Is there a particular reason why you're waiting for the transmitter to transfer to LP-11 state? That appears to be the last step which is done in the csi2_s_stream() callback. What the sensor does next is to start streaming, and the first thing it does in that process is to switch to LP-11 state. Have you tried what happens if you simply drop the LP-11 check? To me that would seem the right thing to do. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 00/36] i.MX Media Driver
Hi Philipp and Russell, On Fri, Feb 17, 2017 at 04:04:30PM +0100, Philipp Zabel wrote: > On Fri, 2017-02-17 at 14:22 +0200, Sakari Ailus wrote: > > Hi Philipp, Steve and Russell, > > > > On Fri, Feb 17, 2017 at 12:43:38PM +0100, Philipp Zabel wrote: > > > On Thu, 2017-02-16 at 14:27 -0800, Steve Longerbeam wrote: > > > > > > > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote: > > > > > On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote: > > > > >> In version 4: > > > > > > > > > > With this version, I get: > > > > > > > > > > [28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x > > > > > [28762.899409] ipu1_csi0: pipeline_set_stream failed with -110 > > > > > > > > > > > > > Right, in the imx219, on exit from s_power(), the clock and data lanes > > > > must be placed in the LP-11 state. This has been done in the ov5640 and > > > > tc358743 subdevs. > > > > > > > > If we want to bring in the patch that adds a .prepare_stream() op, > > > > the csi-2 bus would need to be placed in LP-11 in that op instead. > > > > > > > > Philipp, should I go ahead and add your .prepare_stream() patch? > > > > > > I think with Russell's explanation of how the imx219 sensor operates, > > > we'll have to do something before calling the sensor s_stream, but right > > > now I'm still unsure what exactly. > > > > Indeed there appears to be no other way to achieve the LP-11 state than > > going through the streaming state for this particular sensor, apart from > > starting streaming. > > > > Is there a particular reason why you're waiting for the transmitter to > > transfer to LP-11 state? That appears to be the last step which is done in > > the csi2_s_stream() callback. > > > > What the sensor does next is to start streaming, and the first thing it does > > in that process is to switch to LP-11 state. > > > > Have you tried what happens if you simply drop the LP-11 check? To me that > > would seem the right thing to do. > > Removing the wait for LP-11 alone might not be an issue in my case, as > the TC358743 is known to be in stop state all along. So I just have to > make sure that the time between s_stream(csi2) starting the receiver and > s_stream(tc358743) causing LP-11 to be changed to the next state is long > enough for the receiver to detect LP-11 (which I really can't, I just > have to pray I2C transmissions are slow enough). Fair enough; it appears that the timing of the bus setup is indeed ill defined between the transmitter and the receiver. So there can be hardware specific matters in stream starting that have to be taken into account. :-( This is quite annoying, as there does not appear to be a good way to tell the sensor to set the receiver to LP-11 state without going through the streaming state. If there was, just doing that in s_power(, 1) callback would be quite practical. I guess then there's no really a way to avoid having an extra callback that would explicitly tell the sensor to go to LP-11 state. It should be no issue if the transmitter is already in that state from power-on, but the new callback should guarantee that. Another question is that how far do you need to proceed with streaming in a case where you want to go to LP-11 through streaming? Is simply starting streaming and stopping it right after enough? On some devices it might be but not on others. As the receiver is not started yet, you can't wait for the first frame to start either. And how long it would take for the first frame to start is not defined either in general case: or a driver such as SMIA that's not exactly aware of the underlying hardware but is relying on a standard device interface and behaviour, such approach could be best effort only. Of course it's possible to make changes to the driver if you encounter a combination of a sensor and a receiver that doesn't seem to work, but still it's hardly an ideal solution. How about calling the new callback phy_prepare(), for instance? We could document that it must explicitly set up the transmitter PHY in LP-11 state for CSI-2. The current documentation states that the device should be already in LP-11 after power-on but that apparently is not the case in general. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Steve, On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote: > From: Russell King > > Setting and getting frame rates is part of the negotiation mechanism > between subdevs. The lack of support means that a frame rate at the > sensor can't be negotiated through the subdev path. Just wondering --- what do you need this for? -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Steve, On Mon, Feb 20, 2017 at 02:56:15PM -0800, Steve Longerbeam wrote: > > > On 02/20/2017 02:04 PM, Sakari Ailus wrote: > >Hi Steve, > > > >On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote: > >>From: Russell King > >> > >>Setting and getting frame rates is part of the negotiation mechanism > >>between subdevs. The lack of support means that a frame rate at the > >>sensor can't be negotiated through the subdev path. > > > >Just wondering --- what do you need this for? > > > Hi Sakari, > > i.MX does need the ability to negotiate the frame rates in the > pipelines. The CSI has the ability to skip frames at the output, > which is something Philipp added to the CSI subdev. That affects > frame interval at the CSI output. > > But as Russell pointed out, the lack of [gs]_frame_interval op > causes media-ctl to fail: > > media-ctl -v -d /dev/media1 --set-v4l2 > '"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]' > > Opening media device /dev/media1 > Enumerating entities > Found 29 entities > Enumerating pads and links > Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1 > Format set: SGBRG8 512x512 > Setting up frame interval 1/30 on entity imx6-mipi-csi2 > Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to > setup formats: Inappropriate ioctl for device (25) > > > So i.MX needs to implement this op in every subdev in the > pipeline, otherwise it's not possible to configure the > pipeline with media-ctl. The frame rate is only set on the sub-device which you explicitly set it. I.e. setting the frame rate fails if it's not supported on a pad. Philipp recently posted patches that add frame rate propagation to media-ctl. Frame rate is typically settable (and gettable) only on sensor sub-device's source pad, which means it normally would not be propagated by the kernel but with Philipp's patches, on the sink pad of the bus receiver. Receivers don't have a way to control it nor they implement the IOCTLs, so that would indeed result in an error. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Russell,
On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > From: Russell King
> > >
> > > Setting and getting frame rates is part of the negotiation mechanism
> > > between subdevs. The lack of support means that a frame rate at the
> > > sensor can't be negotiated through the subdev path.
> >
> > Just wondering --- what do you need this for?
>
> The v4l2 documentation contradicts the media-ctl implementation.
>
> While v4l2 documentation says:
>
> These ioctls are used to get and set the frame interval at specific
> subdev pads in the image pipeline. The frame interval only makes sense
> for sub-devices that can control the frame period on their own. This
> includes, for instance, image sensors and TV tuners. Sub-devices that
> don't support frame intervals must not implement these ioctls.
>
> However, when trying to configure the pipeline using media-ctl, eg:
>
> media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel
> 0-0010":0[crop:(0,0)/3264x2464]'
> media-ctl -d /dev/media1 --set-v4l2 '"imx219
> 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> media-ctl -d /dev/media1 --set-v4l2 '"imx219
> 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> media-ctl -d /dev/media1 --set-v4l2
> '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> Unable to setup formats: Inappropriate ioctl for device (25)
> media-ctl -d /dev/media1 --set-v4l2
> '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
>
> The problem there is that the format setting for the csi2 does not get
> propagated forward:
The CSI-2 receivers typically do not implement frame interval IOCTLs as they
do not control the frame interval. Some sensors or TV tuners typically do,
so they implement these IOCTLs.
There are alternative ways to specify the frame rate.
>
> $ strace media-ctl -d /dev/media1 --set-v4l2
> '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> ...
> open("/dev/v4l-subdev16", O_RDWR) = 3
> ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY
> (Inappropriate
> ioctl for device)
> fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> write(1, "Unable to setup formats: Inappro"..., 61) = 61
> Unable to setup formats: Inappropriate ioctl for device (25)
> close(3)= 0
> exit_group(1) = ?
> +++ exited with 1 +++
>
> because media-ctl exits as soon as it encouters the error while trying
> to set the frame rate.
>
> This makes implementing setup of the media pipeline in shell scripts
> unnecessarily difficult - as you need to then know whether an entity
> is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
> and either avoid specifying a frame rate:
You should remove the frame interval setting from sub-devices that do not
support it.
>
> $ strace media-ctl -d /dev/media1 --set-v4l2
> '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
> ...
> open("/dev/v4l-subdev16", O_RDWR) = 3
> ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> open("/dev/v4l-subdev0", O_RDWR)= 4
> ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> close(4)= 0
> close(3)= 0
> exit_group(0) = ?
> +++ exited with 0 +++
>
> or manually setting the format on the sink.
>
> Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
> with the negotiation mechanism that is implemented in subdevs, and
> IMHO should be implemented inside the kernel as a pad operation along
> with the format negotiation, especially so as frame skipping is
> defined as scaling, in just the same way as the frame size is also
> scaling:
The origins of the S_FRAME_INTERVAL IOCTL for sub-devices are the S_PARM
IOCTL for video nodes. It is used to control the frame rate for more simple
devices that do not expose the Media controller interface. The similar
S_FRAME_INTERVAL was added for sub-devices as well, and it has been so far
used to control the frame interval for sensors (and G_FRAME_INTERVAL to
obtain the frame interval for TV tuners, for instance).
The pad argument was added there but media-ctl only supported setting the
frame interval on pad 0, which, coincidentally, worked well for sensor
devices.
The link validation is pri
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Russell,
On Tue, Feb 21, 2017 at 01:21:32PM +, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote:
> > Hi Russell,
> >
> > On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux wrote:
> > > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> > > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> > > > > From: Russell King
> > > > >
> > > > > Setting and getting frame rates is part of the negotiation mechanism
> > > > > between subdevs. The lack of support means that a frame rate at the
> > > > > sensor can't be negotiated through the subdev path.
> > > >
> > > > Just wondering --- what do you need this for?
> > >
> > > The v4l2 documentation contradicts the media-ctl implementation.
> > >
> > > While v4l2 documentation says:
> > >
> > > These ioctls are used to get and set the frame interval at specific
> > > subdev pads in the image pipeline. The frame interval only makes sense
> > > for sub-devices that can control the frame period on their own. This
> > > includes, for instance, image sensors and TV tuners. Sub-devices that
> > > don't support frame intervals must not implement these ioctls.
> > >
> > > However, when trying to configure the pipeline using media-ctl, eg:
> > >
> > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel
> > > 0-0010":0[crop:(0,0)/3264x2464]'
> > > media-ctl -d /dev/media1 --set-v4l2 '"imx219
> > > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > > media-ctl -d /dev/media1 --set-v4l2 '"imx219
> > > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > > media-ctl -d /dev/media1 --set-v4l2
> > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > Unable to setup formats: Inappropriate ioctl for device (25)
> > > media-ctl -d /dev/media1 --set-v4l2
> > > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > > media-ctl -d /dev/media1 --set-v4l2
> > > '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> > >
> > > The problem there is that the format setting for the csi2 does not get
> > > propagated forward:
> >
> > The CSI-2 receivers typically do not implement frame interval IOCTLs as they
> > do not control the frame interval. Some sensors or TV tuners typically do,
> > so they implement these IOCTLs.
>
> No, TV tuners do not. The frame rate for a TV tuner is set by the
> broadcaster, not by the tuner. The tuner can't change that frame rate.
> The tuner may opt to "skip" fields or frames. That's no different from
> what the CSI block in my example below is capable of doing.
>
> Treating a tuner differently from the CSI block is inconsistent and
> completely wrong.
I agree tuners in that sense are somewhat similar, and they are not treated
differently because they are tuners (and not CSI-2 receivers). Neither can
control the frame rate of the incoming video stream.
Conceivably a tuner could implement G_FRAME_INTERVAL IOCTL, but based on a
quick glance none appears to. Neither do CSI-2 receivers. Only sensor
drivers do currently.
>
> > There are alternative ways to specify the frame rate.
>
> Empty statements (or hand-waving type statements) I'm afraid don't
> contribute to the discussion, because they mean nothing to me. Please
> give an example, or flesh out what you mean.
Images are transmitted as series of lines, with each line ending in a
horizontal blanking period, and each frame ending with a similar period of
vertical blanking. The blanking configuration in the units of pixels and
lines at their pixel clock is a native unit which sensors typically use, and
some drivers expose the blanking controls directly to the user.
http://hverkuil.home.xs4all.nl/spec/uapi/v4l/extended-controls.html#image-source-control-ids>
>
> > > $ strace media-ctl -d /dev/media1 --set-v4l2
> > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > > ...
> > > open("/dev/v4l-subdev16", O_RDWR) = 3
> > > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> > > ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY
> > > (Inappropriate
> > > ioctl for device)
> > > fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> > > write(1, "Unable to setup formats: Inappro"..., 61) = 61
> > > Unable to setup f
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
On Tue, Feb 21, 2017 at 04:03:32PM +, Russell King - ARM Linux wrote: > On Tue, Feb 21, 2017 at 05:38:34PM +0200, Sakari Ailus wrote: > > Hi Russell, > > > > On Tue, Feb 21, 2017 at 01:21:32PM +, Russell King - ARM Linux wrote: > > > On Tue, Feb 21, 2017 at 02:37:57PM +0200, Sakari Ailus wrote: > > > > Hi Russell, > > > > > > > > On Tue, Feb 21, 2017 at 12:13:32AM +, Russell King - ARM Linux > > > > wrote: > > > > > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote: > > > > > > On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote: > > > > > > > From: Russell King > > > > > > > > > > > > > > Setting and getting frame rates is part of the negotiation > > > > > > > mechanism > > > > > > > between subdevs. The lack of support means that a frame rate at > > > > > > > the > > > > > > > sensor can't be negotiated through the subdev path. > > > > > > > > > > > > Just wondering --- what do you need this for? > > > > > > > > > > The v4l2 documentation contradicts the media-ctl implementation. > > > > > > > > > > While v4l2 documentation says: > > > > > > > > > > These ioctls are used to get and set the frame interval at specific > > > > > subdev pads in the image pipeline. The frame interval only makes > > > > > sense > > > > > for sub-devices that can control the frame period on their own. This > > > > > includes, for instance, image sensors and TV tuners. Sub-devices > > > > > that > > > > > don't support frame intervals must not implement these ioctls. > > > > > > > > > > However, when trying to configure the pipeline using media-ctl, eg: > > > > > > > > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel > > > > > 0-0010":0[crop:(0,0)/3264x2464]' > > > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 > > > > > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]' > > > > > media-ctl -d /dev/media1 --set-v4l2 '"imx219 > > > > > 0-0010":0[fmt:SRGGB8/816x616@1/30]' > > > > > media-ctl -d /dev/media1 --set-v4l2 > > > > > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]' > > > > > Unable to setup formats: Inappropriate ioctl for device (25) > > > > > media-ctl -d /dev/media1 --set-v4l2 > > > > > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]' > > > > > media-ctl -d /dev/media1 --set-v4l2 > > > > > '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]' > > > > > > > > > > The problem there is that the format setting for the csi2 does not get > > > > > propagated forward: > > > > > > > > The CSI-2 receivers typically do not implement frame interval IOCTLs as > > > > they > > > > do not control the frame interval. Some sensors or TV tuners typically > > > > do, > > > > so they implement these IOCTLs. > > > > > > No, TV tuners do not. The frame rate for a TV tuner is set by the > > > broadcaster, not by the tuner. The tuner can't change that frame rate. > > > The tuner may opt to "skip" fields or frames. That's no different from > > > what the CSI block in my example below is capable of doing. > > > > > > Treating a tuner differently from the CSI block is inconsistent and > > > completely wrong. > > > > I agree tuners in that sense are somewhat similar, and they are not treated > > differently because they are tuners (and not CSI-2 receivers). Neither can > > control the frame rate of the incoming video stream. > > > > Conceivably a tuner could implement G_FRAME_INTERVAL IOCTL, but based on a > > quick glance none appears to. Neither do CSI-2 receivers. Only sensor > > drivers do currently. > > Please look again. I am being very careful with "CSI" vs "CSI-2" in my > emails, you are conflating the two. > > In all my emails so far, "CSI" refers to a block of hardware that is > responsible for receiving an image stream from some kind of source. It > contains hardware that supports frame skipping. Ah, I missed the difference. Thanks f
Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event
Hi Steve,
On Wed, Feb 15, 2017 at 06:19:15PM -0800, Steve Longerbeam wrote:
> Add a new FRAME_TIMEOUT event to signal that a video capture or
> output device has timed out waiting for reception or transmit
> completion of a video frame.
>
> Signed-off-by: Steve Longerbeam
> ---
> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 5 +
> Documentation/media/videodev2.h.rst.exceptions | 1 +
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> index 8d663a7..dd77d9b 100644
> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
> @@ -197,6 +197,11 @@ call.
> the regions changes. This event has a struct
> :c:type:`v4l2_event_motion_det`
> associated with it.
> +* - ``V4L2_EVENT_FRAME_TIMEOUT``
> + - 7
> + - This event is triggered when the video capture or output device
> + has timed out waiting for the reception or transmit completion of
> + a frame of video.
As you're adding a new interface, I suppose you have an implementation
around. How do you determine what that timeout should be?
> * - ``V4L2_EVENT_PRIVATE_START``
>- 0x0800
>- Base event number for driver-private events.
> diff --git a/Documentation/media/videodev2.h.rst.exceptions
> b/Documentation/media/videodev2.h.rst.exceptions
> index e11a0d0..5b0f767 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -459,6 +459,7 @@ replace define V4L2_EVENT_CTRL event-type
> replace define V4L2_EVENT_FRAME_SYNC event-type
> replace define V4L2_EVENT_SOURCE_CHANGE event-type
> replace define V4L2_EVENT_MOTION_DET event-type
> +replace define V4L2_EVENT_FRAME_TIMEOUT event-type
> replace define V4L2_EVENT_PRIVATE_START event-type
>
> replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 46e8a2e3..e174c45 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2132,6 +2132,7 @@ struct v4l2_streamparm {
> #define V4L2_EVENT_FRAME_SYNC4
> #define V4L2_EVENT_SOURCE_CHANGE 5
> #define V4L2_EVENT_MOTION_DET6
> +#define V4L2_EVENT_FRAME_TIMEOUT 7
> #define V4L2_EVENT_PRIVATE_START 0x0800
>
> /* Payload for V4L2_EVENT_VSYNC */
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Hi Steve,
On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote:
> v4l2_pipeline_inherit_controls() will add the v4l2 controls from
> all subdev entities in a pipeline to a given video device.
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/media/v4l2-core/v4l2-mc.c | 48
> +++
> include/media/v4l2-mc.h | 25
> 2 files changed, 73 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c
> b/drivers/media/v4l2-core/v4l2-mc.c
> index 303980b..09d4d97 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q)
> }
> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source);
>
> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> + struct media_entity *start_entity)
I have a few concerns / questions:
- What's the purpose of this patch? Why not to access the sub-device node
directly?
- This implementation is only workable as long as you do not modify the
pipeline. Once you disable a link along the pipeline, a device where the
control was inherited from may no longer be a part of the pipeline.
Depending on the hardware, it could be a part of another pipeline, in
which case it certainly must not be accessible through an unrelated video
node. As the function is added to the framework, I would expect it to
handle such a case correctly.
- I assume it is the responsibility of the caller of this function to ensure
the device in question will not be powered off whilst the video node is
used as another user space interface to such a sub-device. If the driver
uses the generic PM functions in the same file, this works, but it still
has to be documented.
> +{
> + struct media_device *mdev = start_entity->graph_obj.mdev;
> + struct media_entity *entity;
> + struct media_graph graph;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + ret = media_graph_walk_init(&graph, mdev);
> + if (ret)
> + return ret;
> +
> + media_graph_walk_start(&graph, start_entity);
> +
> + while ((entity = media_graph_walk_next(&graph))) {
> + if (!is_media_entity_v4l2_subdev(entity))
> + continue;
> +
> + sd = media_entity_to_v4l2_subdev(entity);
> +
> + ret = v4l2_ctrl_add_handler(vfd->ctrl_handler,
> + sd->ctrl_handler,
> + NULL);
> + if (ret)
> + break;
> + }
> +
> + media_graph_walk_cleanup(&graph);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(__v4l2_pipeline_inherit_controls);
> +
> +int v4l2_pipeline_inherit_controls(struct video_device *vfd,
> +struct media_entity *start_entity)
> +{
> + struct media_device *mdev = start_entity->graph_obj.mdev;
> + int ret;
> +
> + mutex_lock(&mdev->graph_mutex);
> + ret = __v4l2_pipeline_inherit_controls(vfd, start_entity);
> + mutex_unlock(&mdev->graph_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_inherit_controls);
> +
> /*
> -
> * Pipeline power management
> *
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 2634d9d..9848e77 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -171,6 +171,17 @@ void v4l_disable_media_source(struct video_device *vdev);
> */
> int v4l_vb2q_enable_media_source(struct vb2_queue *q);
>
> +/**
> + * v4l2_pipeline_inherit_controls - Add the v4l2 controls from all
> + * subdev entities in a pipeline to
> + * the given video device.
> + * @vfd: the video device
> + * @start_entity: Starting entity
> + */
> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> + struct media_entity *start_entity);
> +int v4l2_pipeline_inherit_controls(struct video_device *vfd,
> +struct media_entity *start_entity);
>
> /**
> * v4l2_pipeline_pm_use - Update the use count of an entity
> @@ -231,6 +242,20 @@ static inline int v4l_vb2q_enable_media_source(struct
> vb2_queue *q)
> return 0;
> }
>
> +static inline int __v4l2_pipeline_inherit_controls(
> + struct video_device *vfd,
>
Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event
On Thu, Mar 02, 2017 at 03:07:21PM -0800, Steve Longerbeam wrote: > > > On 03/02/2017 07:53 AM, Sakari Ailus wrote: > >Hi Steve, > > > >On Wed, Feb 15, 2017 at 06:19:15PM -0800, Steve Longerbeam wrote: > >>Add a new FRAME_TIMEOUT event to signal that a video capture or > >>output device has timed out waiting for reception or transmit > >>completion of a video frame. > >> > >>Signed-off-by: Steve Longerbeam > >>--- > >> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 5 + > >> Documentation/media/videodev2.h.rst.exceptions | 1 + > >> include/uapi/linux/videodev2.h | 1 + > >> 3 files changed, 7 insertions(+) > >> > >>diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>index 8d663a7..dd77d9b 100644 > >>--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>@@ -197,6 +197,11 @@ call. > >>the regions changes. This event has a struct > >>:c:type:`v4l2_event_motion_det` > >>associated with it. > >>+* - ``V4L2_EVENT_FRAME_TIMEOUT`` > >>+ - 7 > >>+ - This event is triggered when the video capture or output device > >>+ has timed out waiting for the reception or transmit completion of > >>+ a frame of video. > > > >As you're adding a new interface, I suppose you have an implementation > >around. How do you determine what that timeout should be? > > The imx-media driver sets the timeout to 1 second, or 30 frame > periods at 30 fps. The frame rate is not necessarily constant during streaming. It may well change as a result of lighting conditions. I wouldn't add an event for this: this is unreliable and 30 times the frame period is an arbitrary value anyway. No other drivers do this either. The user space is generally in control of the frame period (or on some devices it could be the sensor, too, but *not* the CSI-2 receiver driver), so detecting the condition of not receiving any frames is more reliably done in the user space --- if needed. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] staging: media: Remove unnecessary typecast of c90 int constant
Hi Simran, On Fri, Mar 03, 2017 at 01:21:56AM +0530, simran singhal wrote: > This patch removes unnecessary typecast of c90 int constant. > > WARNING: Unnecessary typecast of c90 int constant > > Signed-off-by: simran singhal Which tree are these patches based on? -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Hi Steve, On Thu, Mar 02, 2017 at 06:12:43PM -0800, Steve Longerbeam wrote: > > > On 03/02/2017 03:48 PM, Steve Longerbeam wrote: > > > > > >On 03/02/2017 08:02 AM, Sakari Ailus wrote: > >>Hi Steve, > >> > >>On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: > >>>v4l2_pipeline_inherit_controls() will add the v4l2 controls from > >>>all subdev entities in a pipeline to a given video device. > >>> > >>>Signed-off-by: Steve Longerbeam > >>>--- > >>> drivers/media/v4l2-core/v4l2-mc.c | 48 > >>>+++ > >>> include/media/v4l2-mc.h | 25 > >>> 2 files changed, 73 insertions(+) > >>> > >>>diff --git a/drivers/media/v4l2-core/v4l2-mc.c > >>>b/drivers/media/v4l2-core/v4l2-mc.c > >>>index 303980b..09d4d97 100644 > >>>--- a/drivers/media/v4l2-core/v4l2-mc.c > >>>+++ b/drivers/media/v4l2-core/v4l2-mc.c > >>>@@ -22,6 +22,7 @@ > >>> #include > >>> #include > >>> #include > >>>+#include > >>> #include > >>> #include > >>> #include > >>>@@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct > >>>vb2_queue *q) > >>> } > >>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); > >>> > >>>+int __v4l2_pipeline_inherit_controls(struct video_device *vfd, > >>>+ struct media_entity *start_entity) > >> > >>I have a few concerns / questions: > >> > >>- What's the purpose of this patch? Why not to access the sub-device node > >> directly? > > > > > >I don't really understand what you are trying to say. > > > > Actually I think I understand what you mean now. Yes, the user can > always access a subdev's control directly from its /dev/v4l-subdevXX. > I'm only providing this feature as a convenience to the user, so that > all controls in a pipeline can be accessed from one place, i.e. the > main capture device node. No other MC based V4L2 driver does this. You'd be creating device specific behaviour that differs from what the rest of the drivers do. The purpose of MC is to provide the user with knowledge of what devices are there, and the V4L2 sub-devices interface is used to access them in this case. It does matter where a control is implemented, too. If the pipeline contains multiple sub-devices that implement the same control, only one of them may be accessed. The driver calling the function (or even less the function) would not know which one of them should be ignored. If you need such functionality, it should be implemented in the user space instead. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event
Hi Steve, On Fri, Mar 03, 2017 at 02:43:51PM -0800, Steve Longerbeam wrote: > > > On 03/03/2017 03:45 AM, Sakari Ailus wrote: > >On Thu, Mar 02, 2017 at 03:07:21PM -0800, Steve Longerbeam wrote: > >> > >> > >>On 03/02/2017 07:53 AM, Sakari Ailus wrote: > >>>Hi Steve, > >>> > >>>On Wed, Feb 15, 2017 at 06:19:15PM -0800, Steve Longerbeam wrote: > >>>>Add a new FRAME_TIMEOUT event to signal that a video capture or > >>>>output device has timed out waiting for reception or transmit > >>>>completion of a video frame. > >>>> > >>>>Signed-off-by: Steve Longerbeam > >>>>--- > >>>>Documentation/media/uapi/v4l/vidioc-dqevent.rst | 5 + > >>>>Documentation/media/videodev2.h.rst.exceptions | 1 + > >>>>include/uapi/linux/videodev2.h | 1 + > >>>>3 files changed, 7 insertions(+) > >>>> > >>>>diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>index 8d663a7..dd77d9b 100644 > >>>>--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>@@ -197,6 +197,11 @@ call. > >>>> the regions changes. This event has a struct > >>>> :c:type:`v4l2_event_motion_det` > >>>> associated with it. > >>>>+* - ``V4L2_EVENT_FRAME_TIMEOUT`` > >>>>+ - 7 > >>>>+ - This event is triggered when the video capture or output device > >>>>+ has timed out waiting for the reception or transmit completion of > >>>>+ a frame of video. > >>> > >>>As you're adding a new interface, I suppose you have an implementation > >>>around. How do you determine what that timeout should be? > >> > >>The imx-media driver sets the timeout to 1 second, or 30 frame > >>periods at 30 fps. > > > >The frame rate is not necessarily constant during streaming. It may well > >change as a result of lighting conditions. > > I think you mean that would be a _temporary_ change in frame rate, but > yes I agree the data rate can temporarily fluctuate. Although I doubt > lighting conditions would cause a sensor to pause data transmission > for a full 1 second. Likely not, at least not in typical conditions. The exposure time is still quite specific to applications: it could be minutes if you take photos e.g. of the night sky. What I'm saying here is that any static value is likely not both reasonable and workable in all potential situations all the time. Still there are cases (as yours below) that may happen in relatively common cases on some hardware (more common than taking long exposure photos of the night sky with the said hardware :)). > > > >I wouldn't add an event for this: > >this is unreliable and 30 times the frame period is an arbitrary value > >anyway. No other drivers do this either. > > If no other drivers do this I don't mind removing it. It is really meant > to deal with the ADV718x CVBS decoder, which often simply stops sending > data on the BT.656 bus if there is an interruption in the input analog > signal. But I agree that user space could detect this timeout instead. > Unless I hear from someone else that they would like to keep this > feature I'll remove it in version 5. That's a bit of a special situation --- still there are alike conditions on existing hardware. You should return the buffers to the user with the ERROR flag set --- or return -EIO from VIDIOC_DQBUF, if the condition will persist: https://www.linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-qbuf.html> Do you already obtain the frame rate from the image source (e.g. tuner, sensor, decoder) and multiply the frame time by some number in the current implementation? Not all sub-device drivers may implement g_frame_interval() so I'd disable the timeout in that case. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Hi Russell, On Fri, Mar 03, 2017 at 11:06:45PM +, Russell King - ARM Linux wrote: > On Thu, Mar 02, 2017 at 06:02:57PM +0200, Sakari Ailus wrote: > > Hi Steve, > > > > On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: > > > v4l2_pipeline_inherit_controls() will add the v4l2 controls from > > > all subdev entities in a pipeline to a given video device. > > > > > > Signed-off-by: Steve Longerbeam > > > --- > > > drivers/media/v4l2-core/v4l2-mc.c | 48 > > > +++ > > > include/media/v4l2-mc.h | 25 > > > 2 files changed, 73 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c > > > b/drivers/media/v4l2-core/v4l2-mc.c > > > index 303980b..09d4d97 100644 > > > --- a/drivers/media/v4l2-core/v4l2-mc.c > > > +++ b/drivers/media/v4l2-core/v4l2-mc.c > > > @@ -22,6 +22,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q) > > > } > > > EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); > > > > > > +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, > > > + struct media_entity *start_entity) > > > > I have a few concerns / questions: > > > > - What's the purpose of this patch? Why not to access the sub-device node > > directly? > > What tools are in existance _today_ to provide access to these controls > via the sub-device nodes? yavta, for instance: http://git.ideasonboard.org/yavta.git> VIDIOC_QUERYCAP isn't supported on sub-devices and v4l2-ctl appears to be checking for that. That check should be removed (with possible other implications taken into account). > > v4l-tools doesn't last time I looked - in fact, the only tool in v4l-tools > which is capable of accessing the subdevices is media-ctl, and that only > provides functionality for configuring the pipeline. > > So, pointing people at vapourware userspace is really quite rediculous. Do bear in mind that there are other programs that can make use of these interfaces. It's not just the test programs, or a test program you attempted to use. > > The established way to control video capture is through the main video > capture device, not through the sub-devices. Yes, the controls are > exposed through sub-devices too, but that does not mean that is the > correct way to access them. It is. That's the very purpose of the sub-devices: to provide access to the hardware independently of how the links are configured. > > The v4l2 documentation (Documentation/media/kapi/v4l2-controls.rst) > even disagrees with your statements. That talks about control > inheritence from sub-devices to the main video device, and the core > v4l2 code provides _automatic_ support for this - see > v4l2_device_register_subdev(): > > /* This just returns 0 if either of the two args is NULL */ > err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, > NULL); > > which merges the subdev's controls into the main device's control > handler. That's done on different kind of devices: those that provide plain V4L2 API to control the entire device. V4L2 sub-device interface is used *in kernel* as an interface to control sub-devices that do not need to be exposed to the user space. Devices that have complex pipeline that do essentially require using the Media controller interface to configure them are out of that scope. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 13/36] [media] v4l2: add a frame timeout event
On Sat, Mar 04, 2017 at 04:37:43PM -0800, Steve Longerbeam wrote: > > > On 03/04/2017 02:56 AM, Sakari Ailus wrote: > >Hi Steve, > > > >On Fri, Mar 03, 2017 at 02:43:51PM -0800, Steve Longerbeam wrote: > >> > >> > >>On 03/03/2017 03:45 AM, Sakari Ailus wrote: > >>>On Thu, Mar 02, 2017 at 03:07:21PM -0800, Steve Longerbeam wrote: > >>>> > >>>> > >>>>On 03/02/2017 07:53 AM, Sakari Ailus wrote: > >>>>>Hi Steve, > >>>>> > >>>>>On Wed, Feb 15, 2017 at 06:19:15PM -0800, Steve Longerbeam wrote: > >>>>>>Add a new FRAME_TIMEOUT event to signal that a video capture or > >>>>>>output device has timed out waiting for reception or transmit > >>>>>>completion of a video frame. > >>>>>> > >>>>>>Signed-off-by: Steve Longerbeam > >>>>>>--- > >>>>>>Documentation/media/uapi/v4l/vidioc-dqevent.rst | 5 + > >>>>>>Documentation/media/videodev2.h.rst.exceptions | 1 + > >>>>>>include/uapi/linux/videodev2.h | 1 + > >>>>>>3 files changed, 7 insertions(+) > >>>>>> > >>>>>>diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>>>b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>>>index 8d663a7..dd77d9b 100644 > >>>>>>--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>>>+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > >>>>>>@@ -197,6 +197,11 @@ call. > >>>>>>the regions changes. This event has a struct > >>>>>>:c:type:`v4l2_event_motion_det` > >>>>>>associated with it. > >>>>>>+* - ``V4L2_EVENT_FRAME_TIMEOUT`` > >>>>>>+ - 7 > >>>>>>+ - This event is triggered when the video capture or output device > >>>>>>+ has timed out waiting for the reception or transmit completion > >>>>>>of > >>>>>>+ a frame of video. > >>>>> > >>>>>As you're adding a new interface, I suppose you have an implementation > >>>>>around. How do you determine what that timeout should be? > >>>> > >>>>The imx-media driver sets the timeout to 1 second, or 30 frame > >>>>periods at 30 fps. > >>> > >>>The frame rate is not necessarily constant during streaming. It may well > >>>change as a result of lighting conditions. > >> > >>I think you mean that would be a _temporary_ change in frame rate, but > >>yes I agree the data rate can temporarily fluctuate. Although I doubt > >>lighting conditions would cause a sensor to pause data transmission > >>for a full 1 second. > > > >Likely not, at least not in typical conditions. The exposure time is still > >quite specific to applications: it could be minutes if you take photos e.g. > >of the night sky. > > > >What I'm saying here is that any static value is likely not both reasonable > >and workable in all potential situations all the time. Still there are cases > >(as yours below) that may happen in relatively common cases on some hardware > >(more common than taking long exposure photos of the night sky with the said > >hardware :)). > > I doubt night photography will ever be a use-case for i.MX. The most > common use-case for this driver will be used in automotive applications > such as rear-view or 360 degree view cameras. Ack. > > > > > >> > >> > >>>I wouldn't add an event for this: > >>>this is unreliable and 30 times the frame period is an arbitrary value > >>>anyway. No other drivers do this either. > >> > >>If no other drivers do this I don't mind removing it. It is really meant > >>to deal with the ADV718x CVBS decoder, which often simply stops sending > >>data on the BT.656 bus if there is an interruption in the input analog > >>signal. But I agree that user space could detect this timeout instead. > >>Unless I hear from someone else that they would like to keep this > >>feature I'll remove it in version 5. > > > >That's a bit of a special situation --- still there are alike conditions on > >existing hardware. You should return the buffers to the user with the ERROR > >flag set
Re: [PATCH 11/29] drivers, media: convert cx88_core.refcount from atomic_t to refcount_t
On Mon, Mar 06, 2017 at 04:20:58PM +0200, Elena Reshetova wrote: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > > Signed-off-by: Elena Reshetova > Signed-off-by: Hans Liljestrand > Signed-off-by: Kees Cook > Signed-off-by: David Windsor Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/29] drivers, media: convert s2255_dev.num_channels from atomic_t to refcount_t
Hi Elena, On Mon, Mar 06, 2017 at 04:20:59PM +0200, Elena Reshetova wrote: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > > Signed-off-by: Elena Reshetova > Signed-off-by: Hans Liljestrand > Signed-off-by: Kees Cook > Signed-off-by: David Windsor > --- ... > @@ -1688,7 +1689,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > "failed to register video device!\n"); > break; > } > - atomic_inc(&dev->num_channels); > + refcount_set(&dev->num_channels, 1); That's not right. The loop runs four iterations and the value after the loop should be indeed the number of channels. atomic_t isn't really used for reference counting here, but storing out how many "channels" there are per hardware device, with maximum number of four (4). I'd leave this driver using atomic_t. > v4l2_info(&dev->v4l2_dev, "V4L2 device registered as %s\n", > video_device_node_name(&vc->vdev)); > -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/29] drivers, media: convert vb2_vmarea_handler.refcount from atomic_t to refcount_t
Hi Elena,
On Mon, Mar 06, 2017 at 04:21:00PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
>
> Signed-off-by: Elena Reshetova
> Signed-off-by: Hans Liljestrand
> Signed-off-by: Kees Cook
> Signed-off-by: David Windsor
> ---
> drivers/media/v4l2-core/videobuf2-memops.c | 6 +++---
> include/media/videobuf2-memops.h | 3 ++-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-memops.c
> b/drivers/media/v4l2-core/videobuf2-memops.c
> index 1cd322e..4bb8424 100644
> --- a/drivers/media/v4l2-core/videobuf2-memops.c
> +++ b/drivers/media/v4l2-core/videobuf2-memops.c
> @@ -96,10 +96,10 @@ static void vb2_common_vm_open(struct vm_area_struct *vma)
> struct vb2_vmarea_handler *h = vma->vm_private_data;
>
> pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
> -__func__, h, atomic_read(h->refcount), vma->vm_start,
> +__func__, h, refcount_read(h->refcount), vma->vm_start,
> vma->vm_end);
>
> - atomic_inc(h->refcount);
> + refcount_inc(h->refcount);
> }
>
> /**
> @@ -114,7 +114,7 @@ static void vb2_common_vm_close(struct vm_area_struct
> *vma)
> struct vb2_vmarea_handler *h = vma->vm_private_data;
>
> pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
> -__func__, h, atomic_read(h->refcount), vma->vm_start,
> +__func__, h, refcount_read(h->refcount), vma->vm_start,
> vma->vm_end);
>
> h->put(h->arg);
> diff --git a/include/media/videobuf2-memops.h
> b/include/media/videobuf2-memops.h
> index 36565c7a..a6ed091 100644
> --- a/include/media/videobuf2-memops.h
> +++ b/include/media/videobuf2-memops.h
> @@ -16,6 +16,7 @@
>
> #include
> #include
> +#include
>
> /**
> * struct vb2_vmarea_handler - common vma refcount tracking handler
> @@ -25,7 +26,7 @@
> * @arg: argument for @put callback
> */
> struct vb2_vmarea_handler {
> - atomic_t*refcount;
> + refcount_t *refcount;
This is a pointer to refcount, not refcount itself. The refcount is part of
a memory type specific struct, the types that you change in the following
three patches. I guess it would still compile and work as separate patches
but you'd sure get warnings at least.
How about merging this and the three following patches that change the memop
refcount types?
> void(*put)(void *arg);
> void*arg;
> };
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Hi Mauro (and others), On Fri, Mar 10, 2017 at 12:53:42PM -0300, Mauro Carvalho Chehab wrote: > Em Fri, 10 Mar 2017 15:20:48 +0100 > Hans Verkuil escreveu: > > > > > > As I've already mentioned, from talking about this with Mauro, it seems > > > Mauro is in agreement with permitting the control inheritence... I wish > > > Mauro would comment for himself, as I can't quote our private discussion > > > on the subject. > > > > I can't comment either, not having seen his mail and reasoning. > > The rationale is that we should support the simplest use cases first. > > In the case of the first MC-based driver (and several subsequent > ones), the simplest use case required MC, as it was meant to suport > a custom-made sophisticated application that required fine control > on each component of the pipeline and to allow their advanced > proprietary AAA userspace-based algorithms to work. The first MC based driver (omap3isp) supports what the hardware can do, it does not support applications as such. Adding support to drivers for different "operation modes" --- this is essentially what is being asked for --- is not an approach which could serve either purpose (some functionality with simple interface vs. fully support what the hardware can do, with interfaces allowing that) adequately in the short or the long run. If we are missing pieces in the puzzle --- in this case the missing pieces in the puzzle are a generic pipeline configuration library and another library that, with the help of pipeline autoconfiguration would implement "best effort" service for regular V4L2 on top of the MC + V4L2 subdev + V4L2 --- then these pieces need to be impelemented. The solution is *not* to attempt to support different types of applications in each driver separately. That will make writing drivers painful, error prone and is unlikely ever deliver what either purpose requires. So let's continue to implement the functionality that the hardware supports. Making a different choice here is bound to create a lasting conflict between having to change kernel interface behaviour and the requirement of supporting new functionality that hasn't been previously thought of, pushing away SoC vendors from V4L2 ecosystem. This is what we all do want to avoid. As far as i.MX6 driver goes, it is always possible to implement i.MX6 plugin for libv4l to perform this. This should be much easier than getting the automatic pipe configuration library and the rest working, and as it is custom for i.MX6, the resulting plugin may make informed technical choices for better functionality. Jacek has been working on such a plugin for Samsung Exynos hardware, but I don't think he has quite finished it yey. The original plan was and continues to be sound, it's just that there have always been too few hands to implement it. :-( > > That's not true, for example, for the UVC driver. There, MC > is optional, as it should be. UVC is different. The device simply provides additional information through MC to the user but MC (or V4L2 sub-device interface) is not used for controlling the device. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 18/39] [media] v4l: subdev: Add function to validate frame interval
Hi Steve, On Thu, Mar 09, 2017 at 08:52:58PM -0800, Steve Longerbeam wrote: > If the pads on both sides of a link specify a frame interval, then > those frame intervals should match. Create the exported function > v4l2_subdev_link_validate_frame_interval() to verify this. This > function can be called in a subdevice's media_entity_operations > or v4l2_subdev_pad_ops link_validate callbacks. > > Signed-off-by: Steve Longerbeam If your only goal is to configure frame dropping on a sub-device, I suggest to implement s_frame_interval() on the pads of that sub-device only. The frames are then dropped according to the configured frame rates between the sink and source pads. Say, configuring sink for 1/30 s and source 1/15 would drop half of the incoming frames. Considering that supporting specific frame interval on most sub-devices adds no value or is not the interface through which it the frame rate configured, I think it is overkill to change the link validation to expect otherwise. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 21/39] UAPI: Add media UAPI Kbuild file
Hi Steve, On Thu, Mar 09, 2017 at 08:53:01PM -0800, Steve Longerbeam wrote: > Add an empty UAPI Kbuild file for media UAPI headers. > > Signed-off-by: Steve Longerbeam The existing V4L2 UAPI headers are under include/uapi/linux. Could you use that directory instead? I actually wouldn't really object doing this but it should have been done in 2002 or so when the first V4L2 header was added. Now the benefit is questionable. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
ld disable one in favor of the > > > other, either at compilation time or at runtime. > > > > Right. If the subdev API is disabled, then you have to inherit the subdev > > controls in the bridge driver (how else would you be able to access them?). > > And that's the usual case. > > > > If you do have the subdev API enabled, AND you use the MC, then the > > intention clearly is to give userspace full control and inheriting controls > > no longer makes any sense (and is highly confusing IMHO). > > I tend to agree with that. I agree as well. This is in line with how existing drivers behave, too. > > > > > > > This way, if the subdev API is disabled, the driver will be > > > functional for V4L2-based applications that don't support neither > > > MC or subdev APIs. > > > > I'm not sure if it makes sense for the i.MX driver to behave differently > > depending on whether the subdev API is enabled or disabled. I don't know > > enough of the hardware to tell if it would ever make sense to disable the > > subdev API. > > Yeah, I don't know enough about it either. The point is: this is > something that the driver maintainer and driver users should > decide if it either makes sense or not, based on the expected use cases. My understanding of the i.MX6 case is the hardware is configurable enough to warrant the use of the Media controller API. Some patches indicate there are choices to be made in data routing. Steve: could you enlighten us on the topic, by e.g. doing media-ctl --print-dot and sending the results to the list? What kind of different IP blocks are there and what do they do? A pointer to hardware documentation wouldn't hurt either (if it's available). -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Hi Mauro, On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote: > Em Sat, 11 Mar 2017 00:37:14 +0200 > Sakari Ailus escreveu: > > > Hi Mauro (and others), > > > > On Fri, Mar 10, 2017 at 12:53:42PM -0300, Mauro Carvalho Chehab wrote: > > > Em Fri, 10 Mar 2017 15:20:48 +0100 > > > Hans Verkuil escreveu: > > > > > > > > > > > > As I've already mentioned, from talking about this with Mauro, it > > > > > seems > > > > > Mauro is in agreement with permitting the control inheritence... I > > > > > wish > > > > > Mauro would comment for himself, as I can't quote our private > > > > > discussion > > > > > on the subject. > > > > > > > > I can't comment either, not having seen his mail and reasoning. > > > > > > The rationale is that we should support the simplest use cases first. > > > > > > In the case of the first MC-based driver (and several subsequent > > > ones), the simplest use case required MC, as it was meant to suport > > > a custom-made sophisticated application that required fine control > > > on each component of the pipeline and to allow their advanced > > > proprietary AAA userspace-based algorithms to work. > > > > The first MC based driver (omap3isp) supports what the hardware can do, it > > does not support applications as such. > > All media drivers support a subset of what the hardware can do. The > question is if such subset covers the use cases or not. Can you name a feature in the OMAP 3 ISP that is not and can not be supported using the current driver model (MC + V4L2 sub-device + V4L2) that could be even remotely useful? > > The current MC-based drivers (except for uvc) took a patch to offer a > more advanced API, to allow direct control to each IP module, as it was > said, by the time we merged the OMAP3 driver, that, for the N9/N900 camera > to work, it was mandatory to access the pipeline's individual components. > > Such approach require that some userspace software will have knowledge > about some hardware details, in order to setup pipelines and send controls > to the right components. That makes really hard to have a generic user > friendly application to use such devices. The effect you described above is true, but I disagree with the cause. The cause is the hardware is more complex and variable than what has been supported previously, and providing a generic interface for accessing such hardware will require more complex interface. The hardware we have today and the user cases we have today are more --- not less --- complex and nuanced than when the Media controller was merged back in 2010. Arguably there is thus more need for the functionality it provides, not less. > > Non-MC based drivers control the hardware via a portable interface with > doesn't require any knowledge about the hardware specifics, as either the > Kernel or some firmware at the device will set any needed pipelines. > > In the case of V4L2 controls, when there's no subdev API, the main > driver (e. g. the driver that creates the /dev/video nodes) sends a > multicast message to all bound I2C drivers. The driver(s) that need > them handle it. When the same control may be implemented on different > drivers, the main driver sends a unicast message to just one driver[1]. > > [1] There are several non-MC drivers that have multiple ways to > control some things, like doing scaling or adjust volume levels at > either the bridge driver or at a subdriver. > > There's nothing wrong with this approach: it works, it is simpler, > it is generic. So, if it covers most use cases, why not allowing it > for usecases where a finer control is not a requirement? Drivers are written to support hardware, not particular use case. Use case specific knowledge should be present only in applications, not in drivers. Doing it otherwise will lead to use case specific drivers and more driver code to maintain for any particular piece of hardware. An individual could possibly choose the right driver for his / her use case, but this approach could hardly work for Linux distribution kernels. The plain V4L2 interface is generic within its own scope: hardware can be supported within the hardware model assumed by the interface. However, on some devices this will end up being a small subset of what the hardware can do. Besides that, when writing the driver, you need to decide at detail level what kind of subset that might be. That's not something anyone writing a driver should need to confront. > > > Adding support to drivers for different "operation modes" -
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Philipp, On Tue, Feb 21, 2017 at 09:50:23AM +0100, Philipp Zabel wrote: ... > > > Then there's the issue where, if you have this setup: > > > > > > camera --> csi2 receiver --> csi --> capture > > > > > > and the "csi" subdev can skip frames, you need to know (a) at the CSI > > > sink pad what the frame rate is of the source (b) what the desired > > > source pad frame rate is, so you can configure the frame skipping. > > > So, does the csi subdev have to walk back through the media graph > > > looking for the frame rate? Does the capture device have to walk back > > > through the media graph looking for some subdev to tell it what the > > > frame rate is - the capture device certainly can't go straight to the > > > sensor to get an answer to that question, because that bypasses the > > > effect of the CSI frame skipping (which will lower the frame rate.) > > > > > > IMHO, frame rate is just another format property, just like the > > > resolution and data format itself, and v4l2 should be treating it no > > > differently. > > > > > > > I agree, frame rate, if indicated/specified by both sides of a link, > > should match. So maybe this should be part of v4l2 link validation. > > > > This might be a good time to propose the following patch. > > I agree with Steve and Russell. I don't see why the (nominal) frame > interval should be handled differently than resolution, data format, and > colorspace information. I think it should just be propagated in the same > way, and there is no reason to have two connected pads set to a > different interval. That would make implementing the g/s_frame_interval > subdev calls mandatory. The vast majority of existing drivers do not implement them nor the user space expects having to set them. Making that mandatory would break existing user space. In addition, that does not belong to link validation either: link validation should only include static properties of the link that are required for correct hardware operation. Frame rate is not such property: hardware that supports the MC interface generally does not recognise such concept (with the exception of some sensors). Additionally, it is dynamic: the frame rate can change during streaming, making its validation at streamon time useless. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Russell, On Mon, Mar 13, 2017 at 01:27:02PM +, Russell King - ARM Linux wrote: > On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote: > > The vast majority of existing drivers do not implement them nor the user > > space expects having to set them. Making that mandatory would break existing > > user space. > > > > In addition, that does not belong to link validation either: link validation > > should only include static properties of the link that are required for > > correct hardware operation. Frame rate is not such property: hardware that > > supports the MC interface generally does not recognise such concept (with > > the exception of some sensors). Additionally, it is dynamic: the frame rate > > can change during streaming, making its validation at streamon time useless. > > So how do we configure the CSI, which can do frame skipping? > > With what you're proposing, it means it's possible to configure the > camera sensor source pad to do 50fps. Configure the CSI sink pad to > an arbitary value, such as 30fps, and configure the CSI source pad to > 15fps. > > What you actually get out of the CSI is 25fps, which bears very little > with the actual values used on the CSI source pad. > > You could say "CSI should ask the camera sensor" - well, that's fine > if it's immediately downstream, but otherwise we'd need to go walking > down the graph to find something that resembles its source - there may > be mux and CSI2 interface subdev blocks in that path. Or we just accept > that frame rates are completely arbitary and bear no useful meaning what > so ever. The user is responsible for configuring the pipeline. It is thus not unreasonable to as the user to configure the frame rate as well if there are device in the pipeline that can affect the frame rate. The way I proposed to implement it is compliant with the existing API and entirely deterministic, contrary to what you're saying. It's not the job of the CSI sub-device to figure it out. S_PARM and G_PARM function as interface on the V4L2 API to access the frame rate on plain V4L2 devices. The i.MX6 IPU is not a plain V4L2 device. Essentially the V4L2 device node you have there is an interface to a rather plain DMA engine, no more. There have been plans to add device profiles to the documentation but that work has not yet been done. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
Hi Steve, On Mon, Mar 13, 2017 at 11:06:22AM -0700, Steve Longerbeam wrote: > > > On 03/13/2017 06:55 AM, Philipp Zabel wrote: > >On Mon, 2017-03-13 at 13:27 +, Russell King - ARM Linux wrote: > >>On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote: > >>>The vast majority of existing drivers do not implement them nor the user > >>>space expects having to set them. Making that mandatory would break > >>>existing > >>>user space. > >>> > >>>In addition, that does not belong to link validation either: link > >>>validation > >>>should only include static properties of the link that are required for > >>>correct hardware operation. Frame rate is not such property: hardware that > >>>supports the MC interface generally does not recognise such concept (with > >>>the exception of some sensors). Additionally, it is dynamic: the frame rate > >>>can change during streaming, making its validation at streamon time > >>>useless. > >> > >>So how do we configure the CSI, which can do frame skipping? > >> > >>With what you're proposing, it means it's possible to configure the > >>camera sensor source pad to do 50fps. Configure the CSI sink pad to > >>an arbitary value, such as 30fps, and configure the CSI source pad to > >>15fps. > >> > >>What you actually get out of the CSI is 25fps, which bears very little > >>with the actual values used on the CSI source pad. > >> > >>You could say "CSI should ask the camera sensor" - well, that's fine > >>if it's immediately downstream, but otherwise we'd need to go walking > >>down the graph to find something that resembles its source - there may > >>be mux and CSI2 interface subdev blocks in that path. Or we just accept > >>that frame rates are completely arbitary and bear no useful meaning what > >>so ever. > > > >Which would include the frame interval returned by VIDIOC_G_PARM on the > >connected video device, as that gets its information from the CSI output > >pad's frame interval. > > > > I'm kinda in the middle on this topic. I agree with Sakari that > frame rate can fluctuate, but that should only be temporary. If > the frame rate permanently shifts from what a subdev reports via > g_frame_interval, then that is a system problem. So I agree with > Phillip and Russell that a link validation of frame interval still > makes sense. > > But I also have to agree with Sakari that a subdev that has no > control over frame rate has no business implementing those ops. > > And then I agree with Russell that for subdevs that do have control > over frame rate, they would have to walk the graph to find the frame > rate source. > > So we're stuck in a broken situation: either the subdevs have to walk > the graph to find the source of frame rate, or s_frame_interval > would have to be mandatory and validated between pads, same as set_fmt. It's not broken; what we are missing though is documentation on how to control devices that can change the frame rate i.e. presumably drop frames occasionally. If you're doing something that hasn't been done before, it may be that new documentation needs to be written to accomodate that use case. As we have an existing interface (VIDIOC_SUBDEV_[GS]_FRAME_INTERVAL) it does make sense to use that. What is not possible, though, is to mandate its use in link validation everywhere. If you had a hardware limitation that would require that the frame rate is constant, then we'd need to handle that in link validation for that particular piece of hardware. But there really is no case for doing that for everything else. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 15/39] [media] v4l2: add a frame interval error event
Hi Steve, On Tue, Mar 14, 2017 at 09:43:09AM -0700, Steve Longerbeam wrote: > > > On 03/14/2017 09:21 AM, Nicolas Dufresne wrote: > >Le lundi 13 mars 2017 à 10:45 +, Russell King - ARM Linux a écrit : > >>On Mon, Mar 13, 2017 at 11:02:34AM +0100, Hans Verkuil wrote: > >>>On 03/11/2017 07:14 PM, Steve Longerbeam wrote: > >>>>The event must be user visible, otherwise the user has no indication > >>>>the error, and can't correct it by stream restart. > >>>In that case the driver can detect this and call vb2_queue_error. It's > >>>what it is there for. > >>> > >>>The event doesn't help you since only this driver has this issue. So nobody > >>>will watch this event, unless it is sw specifically written for this SoC. > >>> > >>>Much better to call vb2_queue_error to signal a fatal error (which this > >>>apparently is) since there are more drivers that do this, and vivid > >>>supports > >>>triggering this condition as well. > >>So today, I can fiddle around with the IMX219 registers to help gain > >>an understanding of how this sensor works. Several of the registers > >>(such as the PLL setup [*]) require me to disable streaming on the > >>sensor while changing them. > >> > >>This is something I've done many times while testing various ideas, > >>and is my primary way of figuring out and testing such things. > >> > >>Whenever I resume streaming (provided I've let the sensor stop > >>streaming at a frame boundary) it resumes as if nothing happened. If I > >>stop the sensor mid-frame, then I get the rolling issue that Steve > >>reports, but once the top of the frame becomes aligned with the top of > >>the capture, everything then becomes stable again as if nothing happened. > >> > >>The side effect of what you're proposing is that when I disable streaming > >>at the sensor by poking at its registers, rather than the capture just > >>stopping, an error is going to be delivered to gstreamer, and gstreamer > >>is going to exit, taking the entire capture process down. > >Indeed, there is no recovery attempt in GStreamer code, and it's hard > >for an higher level programs to handle this. Nothing prevents from > >adding something of course, but the errors are really un-specific, so > >it would be something pretty blind. For what it has been tested, this > >case was never met, usually the error is triggered by a USB camera > >being un-plugged, a driver failure or even a firmware crash. Most of > >the time, this is not recoverable. > > > >My main concern here based on what I'm reading, is that this driver is > >not even able to notice immediately that a produced frame was corrupted > >(because it's out of sync). From usability perspective, this is really > >bad. > > First, this is an isolated problem, specific to bt.656 and it only > occurs when disrupting the analog video source signal in some > way (by unplugging the RCA cable from the ADV718x connector > for example). > > Second, there is no DMA status support in i.MX6 to catch these > shifted bt.656 codes, and the ADV718x does not provide any > status indicators of this event either. So monitoring frame intervals > is the only solution available, until FSL/NXP issues a new silicon rev. > > > > Can't the driver derive a clock from some irq and calculate for > >each frame if the timing was correct ? > > That's what is being done, essentially. > > > And if not mark the buffer with > >V4L2_BUF_FLAG_ERROR ? > > I prefer to keep the private event, V4L2_BUF_FLAG_ERROR is too > unspecific. Is the reason you prefer an event that you have multiple drivers involved, or that the error flag is, well, only telling there was an error with a particular frame? Returning -EIO (by calling vb2_queue_error()) would be a better choice as it is documented behaviour. -- Regard,s Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 18/39] [media] v4l: subdev: Add function to validate frame interval
On Sat, Mar 11, 2017 at 12:31:24PM -0800, Steve Longerbeam wrote: > > > On 03/11/2017 05:41 AM, Sakari Ailus wrote: > >Hi Steve, > > > >On Thu, Mar 09, 2017 at 08:52:58PM -0800, Steve Longerbeam wrote: > >>If the pads on both sides of a link specify a frame interval, then > >>those frame intervals should match. Create the exported function > >>v4l2_subdev_link_validate_frame_interval() to verify this. This > >>function can be called in a subdevice's media_entity_operations > >>or v4l2_subdev_pad_ops link_validate callbacks. > >> > >>Signed-off-by: Steve Longerbeam > > > >If your only goal is to configure frame dropping on a sub-device, I suggest > >to implement s_frame_interval() on the pads of that sub-device only. The > >frames are then dropped according to the configured frame rates between the > >sink and source pads. Say, configuring sink for 1/30 s and source 1/15 would > >drop half of the incoming frames. > > > >Considering that supporting specific frame interval on most sub-devices adds > >no value or is not the interface through which it the frame rate configured, > >I think it is overkill to change the link validation to expect otherwise. > > > Well, while I think this function might still have validity in the future, I > do agree with you that a subdev that has no control over > frame rate has no business implementing the get|set ops. > > In the imx-media subdevs, the only one that can affect frame rate (via > frame skipping) is the CSI. So I'll go ahead and remove the > [gs]_frame_interval ops from the others. I can remove this patch as > a result. Agreed. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
Hi Russell, On 03/17/17 13:42, Russell King - ARM Linux wrote: > On Tue, Mar 14, 2017 at 08:55:36AM +0100, Hans Verkuil wrote: >> We're all very driver-development-driven, and userspace gets very little >> attention in general. So before just throwing in the towel we should take >> a good look at the reasons why there has been little or no development: is >> it because of fundamental design defects, or because nobody paid attention >> to it? >> >> I strongly suspect it is the latter. >> >> In addition, I suspect end-users of these complex devices don't really care >> about a plugin: they want full control and won't typically use generic >> applications. If they would need support for that, we'd have seen much more >> interest. The main reason for having a plugin is to simplify testing and >> if this is going to be used on cheap hobbyist devkits. > > I think you're looking at it with a programmers hat on, not a users hat. > > Are you really telling me that requiring users to 'su' to root, and then > use media-ctl to manually configure the capture device is what most > users "want" ? It depends on who the user is. I don't think anyone is suggesting a regular end user is the user of all these APIs: it is either an application tailored for that given device, a skilled user with his test scripts or as suggested previously, a libv4l plugin knowing the device or a generic library geared towards providing best effort service. The last one of this list does not exist yet and the second last item requires help. Typically this class of devices is simply not up to provide the level of service you're requesting without additional user space control library which is responsible for automatic white balance, exposure and focus. Making use of the full potential of the hardware requires using a more expressive interface. That's what the kernel interface must provide. If we decide to limit ourselves to a small sub-set of that potential on the level of the kernel interface, we have made a wrong decision. It's as simple as that. This is why the functionality (and which requires taking a lot of policy decisions) belongs to the user space. We cannot have multiple drivers providing multiple kernel interfaces for the same hardware. That said, I'm not trying to provide an excuse for not having libraries available to help the user to configure and control the device more or less automatically even in terms of best effort. It's something that does require attention, a lot more of it than it has received in recent few years. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/18] media: staging: atomisp: do some coding style improvements
On Mon, Mar 26, 2018 at 05:10:35PM -0400, Mauro Carvalho Chehab wrote: > Use make coccicheck in patch mode to do some coding style > improvements. Adjust the results manually. > > Signed-off-by: Mauro Carvalho Chehab Hi Mauro, For patches 2--18, please add: Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
