* Re: [RFC PATCH 5/5] ARM: Samsung: Remove MIPI PHY setup code
From: Kukjin Kim @ 2013-06-16 20:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1371231951-1969-6-git-send-email-s.nawrocki@samsung.com>
On 06/15/13 02:45, Sylwester Nawrocki wrote:
> Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
> DPHYs so we can remove now unused code at arch/arm/plat-samsung.
If so, sounds good :)
> In case there is any board file for S5PV210 platforms using MIPI
> CSIS/DSIM (not any upstream currently) it should use the generic
> PHY API to bind the PHYs to respective PHY consumer drivers.
To be honest, I didn't test this on boards but if the working is fine,
please go ahead without RFC.
Thanks,
- Kukjin
^ permalink raw reply
* Re: [RFC PATCH 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Tomasz Figa @ 2013-06-16 21:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1371231951-1969-2-git-send-email-s.nawrocki@samsung.com>
Hi Sylwester,
Looks good, but I added some nitpicks inline.
On Friday 14 of June 2013 19:45:47 Sylwester Nawrocki wrote:
> Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
> receiver and MIPI DSI transmitter DPHYs.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> .../bindings/phy/exynos-video-mipi-phy.txt | 16 ++
> drivers/phy/Kconfig | 10 ++
> drivers/phy/Makefile | 3 +-
> drivers/phy/exynos_video_mipi_phy.c | 166
> ++++++++++++++++++++ 4 files changed, 194 insertions(+), 1 deletion(-)
> create mode 100644
> Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt create
> mode 100644 drivers/phy/exynos_video_mipi_phy.c
>
> diff --git
> a/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
> b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt new
> file mode 100644
> index 0000000..32311c89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
> @@ -0,0 +1,16 @@
> +Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : "samsung,<soc_name>-video-phy", currently most SoCs can
I don't like this <soc_name> here. It sounds like any SoC name can be put
here. IMHO just listing all supported compatible values should be enough.
> claim + compatibility with the S5PV210 MIPI CSIS/DSIM PHY and thus
> should use + "samsung,s5pv210-video-phy";
> +- reg : offset and length of the MIPI DPHY register set;
> +- #phy-cells : from the generic phy bindings, must be 1;
> +
> +For "samsung,s5pv210-video-phy" compatible DPHYs the second cell in the
> PHY +specifier identifies the DPHY and its meaning is as follows:
> + 0 - MIPI CSIS 0,
> + 1 - MIPI DSIM 0,
> + 2 - MIPI CSIS 1,
> + 3 - MIPI DSIM 1.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 0764a54..d234e99 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -11,3 +11,13 @@ menuconfig GENERIC_PHY
> devices present in the kernel. This layer will have the generic
> API by which phy drivers can create PHY using the phy framework
and
> phy users can obtain reference to the PHY.
> +
> +if GENERIC_PHY
> +
> +config EXYNOS_VIDEO_MIPI_PHY
> + bool "S5P/EXYNOS MIPI CSI-2/DSI PHY driver"
> + depends on OF
Hmm. Is this driver designed only for OF-enabled boards?
> + help
> + Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung
> + S5P and EXYNOS SoCs.
> +endif
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9e9560f..b16f2c1 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -2,4 +2,5 @@
> # Makefile for the phy drivers.
> #
>
> -obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> +obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> +obj-$(CONFIG_EXYNOS_VIDEO_MIPI_PHY) += exynos_video_mipi_phy.o
> diff --git a/drivers/phy/exynos_video_mipi_phy.c
> b/drivers/phy/exynos_video_mipi_phy.c new file mode 100644
> index 0000000..8d4976f
> --- /dev/null
> +++ b/drivers/phy/exynos_video_mipi_phy.c
> @@ -0,0 +1,166 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +/* MIPI_PHYn_CONTROL register bit definitions */
> +#define EXYNOS_MIPI_PHY_ENABLE (1 << 0)
> +#define EXYNOS_MIPI_PHY_SRESETN (1 << 1)
> +#define EXYNOS_MIPI_PHY_MRESETN (1 << 2)
> +#define EXYNOS_MIPI_PHY_RESET_MASK (3 << 1)
> +
> +#define EXYNOS_MAX_VIDEO_PHYS 4
> +
> +struct exynos_video_phy {
> + spinlock_t slock;
> + struct phy *phys[EXYNOS_MAX_VIDEO_PHYS];
> + void __iomem *regs;
> +};
> +
> +/*
> + * The @id argument specifies MIPI CSIS or DSIM PHY as follows:
> + * 0 - MIPI CSIS 0
> + * 1 - MIPI DSIM 0
> + * 2 - MIPI CSIS 1
> + * 3 - MIPI DSIM 1
> + */
> +static int set_phy_state(struct exynos_video_phy *state,
> + unsigned int id, int on)
> +{
> + void __iomem *addr = id < 2 ? state->regs : state->regs + 4;
I don't find this statement too readable. What about:
void __iomem *addr = state->regs;
and below:
/* CSIS 1 and DSIM 1 PHYs have separate register */
if (id >= 2)
addr += 4;
> + unsigned long flags;
> + u32 reg, reset;
> +
> + pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
> + __func__, id, on, (u32)addr, (u32)state->regs);
> +
> + if (WARN_ON(id > EXYNOS_MAX_VIDEO_PHYS))
> + return -EINVAL;
> +
> + if (id & 1)
Nice trick ;), but not very readable. What about creating an enum of PHYs
and using those defined values here:
if (id = PHY_DSI0 || id = PHY_DSI1)
> + reset = EXYNOS_MIPI_PHY_MRESETN;
> + else
> + reset = EXYNOS_MIPI_PHY_SRESETN;
> +
> + spin_lock_irqsave(&state->slock, flags);
> +
> + reg = readl(addr);
> + if (on)
> + reg |= reset;
> + else
> + reg &= ~reset;
> + writel(reg, addr);
> +
> + if (on)
> + reg |= EXYNOS_MIPI_PHY_ENABLE;
I believe this is a kind of reference counting, but a comment here would
be nice.
> + else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
> + reg &= ~EXYNOS_MIPI_PHY_ENABLE;
> +
> + writel(reg, addr);
> +
> + spin_unlock_irqrestore(&state->slock, flags);
> + return 0;
> +}
> +
> +static int exynos_video_phy_power_on(struct phy *phy)
> +{
> + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
> + return set_phy_state(state, phy->id, 1);
> +}
> +
> +static int exynos_video_phy_power_off(struct phy *phy)
> +{
> + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
> + return set_phy_state(state, phy->id, 0);
> +}
> +
> +static struct phy *exynos_video_phy_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct exynos_video_phy *state = dev_get_drvdata(dev);
> +
> + if (WARN_ON(args->args[0] > EXYNOS_MAX_VIDEO_PHYS))
> + return NULL;
> +
> + return state->phys[args->args[0]];
> +}
> +
> +static struct phy_ops exynos_video_phy_ops = {
> + .power_on = exynos_video_phy_power_on,
> + .power_off = exynos_video_phy_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static int exynos_video_phy_probe(struct platform_device *pdev)
> +{
> + struct exynos_video_phy *state;
> + struct device *dev = &pdev->dev;
> + struct resource res;
> + struct phy_provider *phy_provider;
> + int ret, i;
> +
> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(dev->of_node, 0, &res);
> + if (ret < 0)
> + return ret;
You can use platform_get_resource() here to get a resource generated for
you by of_platform_populate().
In addition you don't need to check the pointer returned by
platform_get_resource() because it is checked in devm_ioremap_resource().
> +
> + state->regs = devm_ioremap_resource(dev, &res);
> + if (IS_ERR(state->regs))
> + return PTR_ERR(state->regs);
> +
> + dev_set_drvdata(dev, state);
> +
> + phy_provider = devm_of_phy_provider_register(dev, THIS_MODULE,
> + exynos_video_phy_xlate);
> + if (IS_ERR(phy_provider))
> + return PTR_ERR(phy_provider);
> +
> + for (i = 0; i < EXYNOS_MAX_VIDEO_PHYS; i++) {
> + state->phys[i] = devm_phy_create(dev, i,
&exynos_video_phy_ops,
> +
state);
> + if (IS_ERR(state->phys[i])) {
> + dev_err(dev, "failed to create PHY %d\n", i);
> + return PTR_ERR(state->phys[i]);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_video_phy_of_match[] = {
> + { .compatible = "samsung,s5pv210-video-phy" },
> + { },
> +};
IMHO a MODULE_DEVICE_TABLE should be added here.
Best regards,
Tomasz
> +
> +static struct platform_driver exynos_video_phy_driver = {
> + .probe = exynos_video_phy_probe,
> + .driver = {
> + .of_match_table = exynos_video_phy_of_match,
> + .name = "exynos-video-phy",
> + .owner = THIS_MODULE,
> + }
> +};
> +module_platform_driver(exynos_video_phy_driver);
> +
> +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC MIPI CSI-2/DSI DPHY
> driver"); +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
^ permalink raw reply
* Re: [RFC PATCH 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi
From: Tomasz Figa @ 2013-06-16 21:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1371231951-1969-3-git-send-email-s.nawrocki@samsung.com>
On Friday 14 of June 2013 19:45:48 Sylwester Nawrocki wrote:
> Add PHY provider node for the MIPI CSIS and MIPI DSIM PHYs.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> arch/arm/boot/dts/exynos4.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4.dtsi
> b/arch/arm/boot/dts/exynos4.dtsi index d505ece..4b7ce52 100644
> --- a/arch/arm/boot/dts/exynos4.dtsi
> +++ b/arch/arm/boot/dts/exynos4.dtsi
> @@ -120,12 +120,20 @@
> reg = <0x10010000 0x400>;
> };
>
> + mipi_phy: video-phy {
nit: video-phy@10020710
Best regards,
Tomasz
> + compatible = "samsung,s5pv210-video-phy";
> + reg = <0x10020710 8>;
> + #phy-cells = <1>;
> + };
> +
> dsi_0: dsi@11C80000 {
> compatible = "samsung,exynos4210-mipi-dsi";
> reg = <0x11C80000 0x10000>;
> interrupts = <0 79 0>;
> samsung,phy-type = <0>;
> samsung,power-domain = <&pd_lcd0>;
> + phys = <&mipi_phy 1>;
> + phy-names = "dsim";
> clocks = <&clock 286>, <&clock 143>;
> clock-names = "bus_clk", "pll_clk";
> status = "disabled";
> @@ -181,6 +189,8 @@
> interrupts = <0 78 0>;
> bus-width = <4>;
> samsung,power-domain = <&pd_cam>;
> + phys = <&mipi_phy 0>;
> + phy-names = "csis";
> status = "disabled";
> };
>
> @@ -190,6 +200,8 @@
> interrupts = <0 80 0>;
> bus-width = <2>;
> samsung,power-domain = <&pd_cam>;
> + phys = <&mipi_phy 2>;
> + phy-names = "csis";
> status = "disabled";
> };
> };
^ permalink raw reply
* Re: [RFC PATCH 3/5] video: exynos_dsi: Use generic PHY driver
From: Tomasz Figa @ 2013-06-16 21:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1371231951-1969-4-git-send-email-s.nawrocki@samsung.com>
On Friday 14 of June 2013 19:45:49 Sylwester Nawrocki wrote:
> Use the generic PHY API instead of the platform callback to control
> the MIPI DSIM DPHY.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/video/display/source-exynos_dsi.c | 36
> +++++++++-------------------- include/video/exynos_dsi.h
> | 5 ----
> 2 files changed, 11 insertions(+), 30 deletions(-)
Yes, this is what I was really missing a lot while developing this driver.
Definitely looks good! It's a shame we don't have this driver in mainline
yet ;) ,
Best regards,
Tomasz
> diff --git a/drivers/video/display/source-exynos_dsi.c
> b/drivers/video/display/source-exynos_dsi.c index 145d57b..dfab790
> 100644
> --- a/drivers/video/display/source-exynos_dsi.c
> +++ b/drivers/video/display/source-exynos_dsi.c
> @@ -24,6 +24,7 @@
> #include <linux/mm.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> @@ -219,6 +220,7 @@ struct exynos_dsi {
> bool enabled;
>
> struct platform_device *pdev;
> + struct phy *phy;
> struct device *dev;
> struct resource *res;
> struct clk *pll_clk;
> @@ -816,6 +818,7 @@ again:
>
> static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi)
> {
> + static unsigned long j;
> struct exynos_dsi_transfer *xfer;
> unsigned long flags;
> bool start = true;
> @@ -824,7 +827,8 @@ static bool exynos_dsi_transfer_finish(struct
> exynos_dsi *dsi)
>
> if (list_empty(&dsi->transfer_list)) {
> spin_unlock_irqrestore(&dsi->transfer_lock, flags);
> - dev_warn(dsi->dev, "unexpected TX/RX interrupt\n");
> + if (printk_timed_ratelimit(&j, 500))
> + dev_warn(dsi->dev, "unexpected TX/RX
interrupt\n");
> return false;
> }
>
> @@ -994,8 +998,7 @@ static int exynos_dsi_enable(struct video_source
> *src) clk_prepare_enable(dsi->bus_clk);
> clk_prepare_enable(dsi->pll_clk);
>
> - if (dsi->pd->phy_enable)
> - dsi->pd->phy_enable(dsi->pdev, true);
> + phy_power_on(dsi->phy);
>
> exynos_dsi_reset(dsi);
> exynos_dsi_init_link(dsi);
> @@ -1019,8 +1022,7 @@ static int exynos_dsi_disable(struct video_source
> *src)
>
> exynos_dsi_disable_clock(dsi);
>
> - if (dsi->pd->phy_enable)
> - dsi->pd->phy_enable(dsi->pdev, false);
> + phy_power_off(dsi->phy);
>
> clk_disable_unprepare(dsi->pll_clk);
> clk_disable_unprepare(dsi->bus_clk);
> @@ -1099,12 +1101,6 @@ static const struct dsi_video_source_ops
> exynos_dsi_ops = { * Device Tree
> */
>
> -static int (* const of_phy_enables[])(struct platform_device *, bool) > { -#ifdef CONFIG_S5P_SETUP_MIPIPHY
> - [0] = s5p_dsim_phy_enable,
> -#endif
> -};
> -
> static struct exynos_dsi_platform_data *exynos_dsi_parse_dt(
> struct platform_device
*pdev)
> {
> @@ -1112,7 +1108,6 @@ static struct exynos_dsi_platform_data
> *exynos_dsi_parse_dt( struct exynos_dsi_platform_data *dsi_pd;
> struct device *dev = &pdev->dev;
> const __be32 *prop_data;
> - u32 val;
>
> dsi_pd = kzalloc(sizeof(*dsi_pd), GFP_KERNEL);
> if (!dsi_pd) {
> @@ -1120,19 +1115,6 @@ static struct exynos_dsi_platform_data
> *exynos_dsi_parse_dt( return NULL;
> }
>
> - prop_data = of_get_property(node, "samsung,phy-type", NULL);
> - if (!prop_data) {
> - dev_err(dev, "failed to get phy-type property\n");
> - goto err_free_pd;
> - }
> -
> - val = be32_to_cpu(*prop_data);
> - if (val >= ARRAY_SIZE(of_phy_enables) || !of_phy_enables[val]) {
> - dev_err(dev, "Invalid phy-type %u\n", val);
> - goto err_free_pd;
> - }
> - dsi_pd->phy_enable = of_phy_enables[val];
> -
> prop_data = of_get_property(node, "samsung,pll-stable-time",
NULL);
> if (!prop_data) {
> dev_err(dev, "failed to get pll-stable-time property\n");
> @@ -1254,6 +1236,10 @@ static int exynos_dsi_probe(struct
> platform_device *pdev) return -ENOMEM;
> }
>
> + dsi->phy = devm_phy_get(&pdev->dev, "dsim");
> + if (IS_ERR(dsi->phy))
> + return PTR_ERR(dsi->phy);
> +
> platform_set_drvdata(pdev, dsi);
>
> dsi->irq = platform_get_irq(pdev, 0);
> diff --git a/include/video/exynos_dsi.h b/include/video/exynos_dsi.h
> index 95e1568..5c062c7 100644
> --- a/include/video/exynos_dsi.h
> +++ b/include/video/exynos_dsi.h
> @@ -25,9 +25,6 @@
> */
> struct exynos_dsi_platform_data {
> unsigned int enabled;
> -
> - int (*phy_enable)(struct platform_device *pdev, bool on);
> -
> unsigned int pll_stable_time;
> unsigned long pll_clk_rate;
> unsigned long esc_clk_rate;
> @@ -36,6 +33,4 @@ struct exynos_dsi_platform_data {
> unsigned short rx_timeout;
> };
>
> -int s5p_dsim_phy_enable(struct platform_device *pdev, bool on);
> -
> #endif /* _EXYNOS_MIPI_DSIM_H */
^ permalink raw reply
* linux-next: manual merge of the fbdev tree with the drm tree
From: Stephen Rothwell @ 2013-06-17 4:21 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD, Florian Tobias Schandinat,
Tomi Valkeinen, linux-fbdev
Cc: linux-next, linux-kernel, Steffen Trumtrar, Dave Airlie
[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]
Hi all,
Today's linux-next merge of the fbdev tree got a conflict in
drivers/video/of_display_timing.c between commit f583662347c6 ("video:
display_timing: make parameter const") from the drm tree and commits
fcf7e6e5bd84 ("videomode: don't allocate mem in of_get_display_timing()")
and ffa3fd21de8a ("videomode: implement public of_get_display_timing()")
from the fbdev tree.
I fixed it up (see below) and can carry the fix as necessary (no action
is required).
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
diff --cc drivers/video/of_display_timing.c
index 2894e03,9c0f17b..0000000
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@@ -53,13 -53,12 +53,12 @@@ static int parse_timing_property(const
}
/**
- * of_get_display_timing - parse display_timing entry from device_node
+ * of_parse_display_timing - parse display_timing entry from device_node
* @np: device_node with the properties
**/
- static struct display_timing *of_get_display_timing(const struct device_node
- *np)
-static int of_parse_display_timing(struct device_node *np,
++static int of_parse_display_timing(const struct device_node *np,
+ struct display_timing *dt)
{
- struct display_timing *dt;
u32 val = 0;
int ret = 0;
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/7] xilinxfb changes
From: Michal Simek @ 2013-06-17 5:23 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: Michal Simek, linux-kernel, Arnd Bergmann, Timur Tabi,
devicetree-discuss, linux-fbdev, Tomi Valkeinen, Grant Likely,
Rob Herring
In-Reply-To: <20130606162336.GW19468@game.jcrosoft.org>
[-- Attachment #1: Type: text/plain, Size: 567 bytes --]
On 06/06/2013 06:23 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:13 Mon 03 Jun , Michal Simek wrote:
>> Hi,
>>
>
> Arnd can you take look on it again please
>
> I'll take a look on it next week
Any update on this?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: linux-next: manual merge of the fbdev tree with the drm tree
From: Tomi Valkeinen @ 2013-06-17 7:23 UTC (permalink / raw)
To: Stephen Rothwell, Jean-Christophe PLAGNIOL-VILLARD, Dave Airlie
Cc: Florian Tobias Schandinat, linux-fbdev, linux-next, linux-kernel,
Steffen Trumtrar
In-Reply-To: <20130617142120.b42dafb967974d3906a43fc9@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 748 bytes --]
Hi,
On 17/06/13 07:21, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the fbdev tree got a conflict in
> drivers/video/of_display_timing.c between commit f583662347c6 ("video:
> display_timing: make parameter const") from the drm tree and commits
> fcf7e6e5bd84 ("videomode: don't allocate mem in of_get_display_timing()")
> and ffa3fd21de8a ("videomode: implement public of_get_display_timing()")
> from the fbdev tree.
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
Thanks, looks correct to me.
I guess it'd be better to merge videomode and display_timing stuff via a
single tree from this on. Being in drivers/video/, I suggest the fbdev tree.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/7] xilinxfb changes
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-06-17 8:56 UTC (permalink / raw)
To: Michal Simek
Cc: Michal Simek, linux-kernel, Arnd Bergmann, Timur Tabi,
devicetree-discuss, linux-fbdev, Tomi Valkeinen, Grant Likely,
Rob Herring
In-Reply-To: <51BE9D6A.70001@monstr.eu>
On 07:23 Mon 17 Jun , Michal Simek wrote:
> On 06/06/2013 06:23 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:13 Mon 03 Jun , Michal Simek wrote:
> >> Hi,
> >>
> >
> > Arnd can you take look on it again please
> >
> > I'll take a look on it next week
>
> Any update on this?
look ok but I want the Ack from Arnd
Best Regards,
J.
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
>
^ permalink raw reply
* Re: [PATCH v4 0/7] xilinxfb changes
From: Michal Simek @ 2013-06-17 9:02 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD, Arnd Bergmann
Cc: Michal Simek, linux-kernel, Timur Tabi, devicetree-discuss,
linux-fbdev, Tomi Valkeinen, Grant Likely, Rob Herring
In-Reply-To: <20130617085640.GE305@game.jcrosoft.org>
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
On 06/17/2013 10:56 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 07:23 Mon 17 Jun , Michal Simek wrote:
>> On 06/06/2013 06:23 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 12:13 Mon 03 Jun , Michal Simek wrote:
>>>> Hi,
>>>>
>>>
>>> Arnd can you take look on it again please
>>>
>>> I'll take a look on it next week
>>
>> Any update on this?
> look ok but I want the Ack from Arnd
ok.
Arnd?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Inki Dae @ 2013-06-17 11:15 UTC (permalink / raw)
To: dri-devel, linux-fbdev, linux-arm-kernel, linux-media
Cc: maarten.lankhorst, daniel, robdclark, kyungmin.park, myungjoo.ham,
yj44.cho, Inki Dae
In-Reply-To: <1371112088-15310-1-git-send-email-inki.dae@samsung.com>
This patch adds a buffer synchronization framework based on DMA BUF[1]
and reservation[2] to use dma-buf resource, and based on ww-mutexes[3]
for lock mechanism.
The purpose of this framework is not only to couple cache operations,
and buffer access control to CPU and DMA but also to provide easy-to-use
interfaces for device drivers and potentially user application
(not implemented for user applications, yet). And this framework can be
used for all dma devices using system memory as dma buffer, especially
for most ARM based SoCs.
Changelog v2:
- use atomic_add_unless to avoid potential bug.
- add a macro for checking valid access type.
- code clean.
The mechanism of this framework has the following steps,
1. Register dmabufs to a sync object - A task gets a new sync object and
can add one or more dmabufs that the task wants to access.
This registering should be performed when a device context or an event
context such as a page flip event is created or before CPU accesses a shared
buffer.
dma_buf_sync_get(a sync object, a dmabuf);
2. Lock a sync object - A task tries to lock all dmabufs added in its own
sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead
lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
and DMA. Taking a lock means that others cannot access all locked dmabufs
until the task that locked the corresponding dmabufs, unlocks all the locked
dmabufs.
This locking should be performed before DMA or CPU accesses these dmabufs.
dma_buf_sync_lock(a sync object);
3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
object. The unlock means that the DMA or CPU accesses to the dmabufs have
been completed so that others may access them.
This unlocking should be performed after DMA or CPU has completed accesses
to the dmabufs.
dma_buf_sync_unlock(a sync object);
4. Unregister one or all dmabufs from a sync object - A task unregisters
the given dmabufs from the sync object. This means that the task dosen't
want to lock the dmabufs.
The unregistering should be performed after DMA or CPU has completed
accesses to the dmabufs or when dma_buf_sync_lock() is failed.
dma_buf_sync_put(a sync object, a dmabuf);
dma_buf_sync_put_all(a sync object);
The described steps may be summarized as:
get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
This framework includes the following two features.
1. read (shared) and write (exclusive) locks - A task is required to declare
the access type when the task tries to register a dmabuf;
READ, WRITE, READ DMA, or WRITE DMA.
The below is example codes,
struct dmabuf_sync *sync;
sync = dmabuf_sync_init(NULL, "test sync");
dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
...
And the below can be used as access types:
DMA_BUF_ACCESS_READ,
- CPU will access a buffer for read.
DMA_BUF_ACCESS_WRITE,
- CPU will access a buffer for read or write.
DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA,
- DMA will access a buffer for read
DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA,
- DMA will access a buffer for read or write.
2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
A task may never try to unlock a buffer after taking a lock to the buffer.
In this case, a timer handler to the corresponding sync object is called
in five (default) seconds and then the timed-out buffer is unlocked by work
queue handler to avoid lockups and to enforce resources of the buffer.
The below is how to use:
1. Allocate and Initialize a sync object:
struct dmabuf_sync *sync;
sync = dmabuf_sync_init(NULL, "test sync");
...
2. Add a dmabuf to the sync object when setting up dma buffer relevant
registers:
dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
...
3. Lock all dmabufs of the sync object before DMA or CPU accesses
the dmabufs:
dmabuf_sync_lock(sync);
...
4. Now CPU or DMA can access all dmabufs locked in step 3.
5. Unlock all dmabufs added in a sync object after DMA or CPU access
to these dmabufs is completed:
dmabuf_sync_unlock(sync);
And call the following functions to release all resources,
dmabuf_sync_put_all(sync);
dmabuf_sync_fini(sync);
You can refer to actual example codes:
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
commit/?h=dmabuf-sync&id@30bdee9bab5841ad32faade528d04cc0c5fc94
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
commit/?h=dmabuf-sync&idla548e9ea9e865592719ef6b1cde58366af9f5c
The framework performs cache operation based on the previous and current access
types to the dmabufs after the locks to all dmabufs are taken:
Call dma_buf_begin_cpu_access() to invalidate cache if,
previous access type is DMA_BUF_ACCESS_WRITE | DMA and
current access type is DMA_BUF_ACCESS_READ
Call dma_buf_end_cpu_access() to clean cache if,
previous access type is DMA_BUF_ACCESS_WRITE and
current access type is DMA_BUF_ACCESS_READ | DMA
Such cache operations are invoked via dma-buf interfaces so the dma buf exporter
should implement dmabuf->ops->begin_cpu_access/end_cpu_access callbacks.
[1] http://lwn.net/Articles/470339/
[2] http://lwn.net/Articles/532616/
[3] https://patchwork-mail1.kernel.org/patch/2625321/
Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Documentation/dma-buf-sync.txt | 246 ++++++++++++++++++
drivers/base/Kconfig | 7 +
drivers/base/Makefile | 1 +
drivers/base/dmabuf-sync.c | 545 ++++++++++++++++++++++++++++++++++++++++
include/linux/dma-buf.h | 14 +
include/linux/dmabuf-sync.h | 115 +++++++++
include/linux/reservation.h | 7 +
7 files changed, 935 insertions(+), 0 deletions(-)
create mode 100644 Documentation/dma-buf-sync.txt
create mode 100644 drivers/base/dmabuf-sync.c
create mode 100644 include/linux/dmabuf-sync.h
diff --git a/Documentation/dma-buf-sync.txt b/Documentation/dma-buf-sync.txt
new file mode 100644
index 0000000..e71b6f4
--- /dev/null
+++ b/Documentation/dma-buf-sync.txt
@@ -0,0 +1,246 @@
+ DMA Buffer Synchronization Framework
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+ Inki Dae
+ <inki dot dae at samsung dot com>
+ <daeinki at gmail dot com>
+
+This document is a guide for device-driver writers describing the DMA buffer
+synchronization API. This document also describes how to use the API to
+use buffer synchronization between CPU and CPU, CPU and DMA, and DMA and DMA.
+
+The DMA Buffer synchronization API provides buffer synchronization mechanism;
+i.e., buffer access control to CPU and DMA, cache operations, and easy-to-use
+interfaces for device drivers and potentially user application
+(not implemented for user applications, yet). And this API can be used for all
+dma devices using system memory as dma buffer, especially for most ARM based
+SoCs.
+
+
+Motivation
+----------
+
+Sharing a buffer, a device cannot be aware of when the other device will access
+the shared buffer: a device may access a buffer containing wrong data if
+the device accesses the shared buffer while another device is still accessing
+the shared buffer. Therefore, a user process should have waited for
+the completion of DMA access by another device before a device tries to access
+the shared buffer.
+
+Besides, there is the same issue when CPU and DMA are sharing a buffer; i.e.,
+a user process should consider that when the user process have to send a buffer
+to a device driver for the device driver to access the buffer as input.
+This means that a user process needs to understand how the device driver is
+worked. Hence, the conventional mechanism not only makes user application
+complicated but also incurs performance overhead because the conventional
+mechanism cannot control devices precisely without additional and complex
+implemantations.
+
+In addition, in case of ARM based SoCs, most devices have no hardware cache
+consistency mechanisms between CPU and DMA devices because they do not use ACP
+(Accelerator Coherency Port). ACP can be connected to DMA engine or similar
+devices in order to keep cache coherency between CPU cache and DMA device.
+Thus, we need additional cache operations to have the devices operate properly;
+i.e., user applications should request cache operations to kernel before DMA
+accesses the buffer and after the completion of buffer access by CPU, or vise
+versa.
+
+ buffer access by CPU -> cache clean -> buffer access by DMA
+
+Or,
+ buffer access by DMA -> cache invalidate -> buffer access by CPU
+
+The below shows why cache operations should be requested by user
+process,
+ (Presume that CPU and DMA share a buffer and the buffer is mapped
+ with user space as cachable)
+
+ handle = drm_gem_alloc(size);
+ ...
+ va1 = drm_gem_mmap(handle1);
+ va2 = malloc(size);
+ ...
+
+ while(conditions) {
+ memcpy(va1, some data, size);
+ ...
+ drm_xxx_set_dma_buffer(handle, ...);
+ ...
+
+ /* user need to request cache clean at here. */
+
+ /* blocked until dma operation is completed. */
+ drm_xxx_start_dma(...);
+ ...
+
+ /* user need to request cache invalidate at here. */
+
+ memcpy(va2, va1, size);
+ }
+
+The issue arises: user processes may incur cache operations: user processes may
+request unnecessary cache operations to kernel. Besides, kernel cannot prevent
+user processes from requesting such cache operations. Therefore, we need to
+prevent such excessive and unnecessary cache operations from user processes.
+
+
+Basic concept
+-------------
+
+The mechanism of this framework has the following steps,
+ 1. Register dmabufs to a sync object - A task gets a new sync object and
+ can add one or more dmabufs that the task wants to access.
+ This registering should be performed when a device context or an event
+ context such as a page flip event is created or before CPU accesses a shared
+ buffer.
+
+ dma_buf_sync_get(a sync object, a dmabuf);
+
+ 2. Lock a sync object - A task tries to lock all dmabufs added in its own
+ sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead
+ lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
+ and DMA. Taking a lock means that others cannot access all locked dmabufs
+ until the task that locked the corresponding dmabufs, unlocks all the locked
+ dmabufs.
+ This locking should be performed before DMA or CPU accesses these dmabufs.
+
+ dma_buf_sync_lock(a sync object);
+
+ 3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
+ object. The unlock means that the DMA or CPU accesses to the dmabufs have
+ been completed so that others may access them.
+ This unlocking should be performed after DMA or CPU has completed accesses
+ to the dmabufs.
+
+ dma_buf_sync_unlock(a sync object);
+
+ 4. Unregister one or all dmabufs from a sync object - A task unregisters
+ the given dmabufs from the sync object. This means that the task dosen't
+ want to lock the dmabufs.
+ The unregistering should be performed after DMA or CPU has completed
+ accesses to the dmabufs or when dma_buf_sync_lock() is failed.
+
+ dma_buf_sync_put(a sync object, a dmabuf);
+ dma_buf_sync_put_all(a sync object);
+
+ The described steps may be summarized as:
+ get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
+
+This framework includes the following two features.
+ 1. read (shared) and write (exclusive) locks - A task is required to declare
+ the access type when the task tries to register a dmabuf;
+ READ, WRITE, READ DMA, or WRITE DMA.
+
+ The below is example codes,
+ struct dmabuf_sync *sync;
+
+ sync = dmabuf_sync_init(NULL, "test sync");
+
+ dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
+ ...
+
+ 2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
+ A task may never try to unlock a buffer after taking a lock to the buffer.
+ In this case, a timer handler to the corresponding sync object is called
+ in five (default) seconds and then the timed-out buffer is unlocked by work
+ queue handler to avoid lockups and to enforce resources of the buffer.
+
+
+Access types
+------------
+
+DMA_BUF_ACCESS_READ - CPU will access a buffer for read.
+DMA_BUF_ACCESS_WRITE - CPU will access a buffer for read or write.
+DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA - DMA will access a buffer for read
+DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA - DMA will access a buffer for read
+ or write.
+
+
+API set
+-------
+
+bool is_dmabuf_sync_supported(void)
+ - Check if dmabuf sync is supported or not.
+
+struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name)
+ - Allocate and initialize a new sync object. The caller can get a new
+ sync object for buffer synchronization. priv is used to set caller's
+ private data and name is the name of sync object.
+
+void dmabuf_sync_fini(struct dmabuf_sync *sync)
+ - Release all resources to the sync object.
+
+int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf,
+ unsigned int type)
+ - Add a dmabuf to a sync object. The caller can group multiple dmabufs
+ by calling this function several times. Internally, this function also
+ takes a reference to a dmabuf.
+
+void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf)
+ - Remove a given dmabuf from a sync object. Internally, this function
+ also release every reference to the given dmabuf.
+
+void dmabuf_sync_put_all(struct dmabuf_sync *sync)
+ - Remove all dmabufs added in a sync object. Internally, this function
+ also release every reference to the dmabufs of the sync object.
+
+int dmabuf_sync_lock(struct dmabuf_sync *sync)
+ - Lock all dmabufs added in a sync object. The caller should call this
+ function prior to CPU or DMA access to the dmabufs so that others can
+ not access the dmabufs. Internally, this function avoids dead lock
+ issue with ww-mutex.
+
+int dmabuf_sync_unlock(struct dmabuf_sync *sync)
+ - Unlock all dmabufs added in a sync object. The caller should call
+ this function after CPU or DMA access to the dmabufs is completed so
+ that others can access the dmabufs.
+
+
+Tutorial
+--------
+
+1. Allocate and Initialize a sync object:
+ struct dmabuf_sync *sync;
+
+ sync = dmabuf_sync_init(NULL, "test sync");
+ ...
+
+2. Add a dmabuf to the sync object when setting up dma buffer relevant registers:
+ dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
+ ...
+
+3. Lock all dmabufs of the sync object before DMA or CPU accesses the dmabufs:
+ dmabuf_sync_lock(sync);
+ ...
+
+4. Now CPU or DMA can access all dmabufs locked in step 3.
+
+5. Unlock all dmabufs added in a sync object after DMA or CPU access to these
+ dmabufs is completed:
+ dmabuf_sync_unlock(sync);
+
+ And call the following functions to release all resources,
+ dmabuf_sync_put_all(sync);
+ dmabuf_sync_fini(sync);
+
+
+Cache operation
+---------------
+
+The framework performs cache operation based on the previous and current access
+types to the dmabufs after the locks to all dmabufs are taken:
+ Call dma_buf_begin_cpu_access() to invalidate cache if,
+ previous access type is DMA_BUF_ACCESS_WRITE | DMA and
+ current access type is DMA_BUF_ACCESS_READ
+
+ Call dma_buf_end_cpu_access() to clean cache if,
+ previous access type is DMA_BUF_ACCESS_WRITE and
+ current access type is DMA_BUF_ACCESS_READ | DMA
+
+Such cache operations are invoked via dma-buf interfaces. Thus, the dma buf
+exporter should implement dmabuf->ops->begin_cpu_access and end_cpu_access
+callbacks.
+
+
+References:
+[1] https://patchwork-mail1.kernel.org/patch/2625321/
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5ccf182..54a1d5a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -212,6 +212,13 @@ config FENCE_TRACE
lockup related problems for dma-buffers shared across multiple
devices.
+config DMABUF_SYNC
+ bool "DMABUF Synchronization Framework"
+ depends on DMA_SHARED_BUFFER
+ help
+ This option enables dmabuf sync framework for buffer synchronization between
+ DMA and DMA, CPU and DMA, and CPU and CPU.
+
config CMA
bool "Contiguous Memory Allocator"
depends on HAVE_DMA_CONTIGUOUS && HAVE_MEMBLOCK
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 8a55cb9..599f6c90 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -11,6 +11,7 @@ obj-y += power/
obj-$(CONFIG_HAS_DMA) += dma-mapping.o
obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o fence.o reservation.o
+obj-$(CONFIG_DMABUF_SYNC) += dmabuf-sync.o
obj-$(CONFIG_ISA) += isa.o
obj-$(CONFIG_FW_LOADER) += firmware_class.o
obj-$(CONFIG_NUMA) += node.o
diff --git a/drivers/base/dmabuf-sync.c b/drivers/base/dmabuf-sync.c
new file mode 100644
index 0000000..eeb13ca
--- /dev/null
+++ b/drivers/base/dmabuf-sync.c
@@ -0,0 +1,545 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ * Authors:
+ * Inki Dae <inki.dae@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+
+#include <linux/dmabuf-sync.h>
+
+#define MAX_SYNC_TIMEOUT 5 /* Second. */
+
+#define NEED_BEGIN_CPU_ACCESS(old, new) \
+ (old->accessed_type = DMA_BUF_ACCESS_DMA_W && \
+ (new->access_type = DMA_BUF_ACCESS_R))
+
+#define NEED_END_CPU_ACCESS(old, new) \
+ (old->accessed_type = DMA_BUF_ACCESS_W && \
+ (new->access_type & DMA_BUF_ACCESS_DMA))
+
+int dmabuf_sync_enabled = 1;
+
+MODULE_PARM_DESC(enabled, "Check if dmabuf sync is supported or not");
+module_param_named(enabled, dmabuf_sync_enabled, int, 0444);
+
+static void dmabuf_sync_timeout_worker(struct work_struct *work)
+{
+ struct dmabuf_sync *sync = container_of(work, struct dmabuf_sync, work);
+ struct dmabuf_sync_object *sobj;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (WARN_ON(!sobj->robj))
+ continue;
+
+ printk(KERN_WARNING "%s: timeout = 0x%x [type = %d, " \
+ "refcnt = %d, locked = %d]\n",
+ sync->name, (u32)sobj->dmabuf,
+ sobj->access_type,
+ atomic_read(&sobj->robj->shared_cnt),
+ sobj->robj->locked);
+
+ /* unlock only valid sync object. */
+ if (!sobj->robj->locked)
+ continue;
+
+ if (sobj->robj->shared &&
+ atomic_add_unless(&sobj->robj->shared_cnt, -1, 1))
+ continue;
+
+ ww_mutex_unlock(&sobj->robj->lock);
+
+ if (sobj->access_type & DMA_BUF_ACCESS_R)
+ printk(KERN_WARNING "%s: r-unlocked = 0x%x\n",
+ sync->name, (u32)sobj->dmabuf);
+ else
+ printk(KERN_WARNING "%s: w-unlocked = 0x%x\n",
+ sync->name, (u32)sobj->dmabuf);
+ }
+
+ sync->status = 0;
+ mutex_unlock(&sync->lock);
+
+ dmabuf_sync_put_all(sync);
+ dmabuf_sync_fini(sync);
+}
+
+static void dmabuf_sync_cache_ops(struct dmabuf_sync *sync)
+{
+ struct dmabuf_sync_object *sobj;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ struct dma_buf *dmabuf;
+
+ dmabuf = sobj->dmabuf;
+ if (WARN_ON(!dmabuf || !sobj->robj))
+ continue;
+
+ /* first time access. */
+ if (!sobj->robj->accessed_type)
+ goto out;
+
+ if (NEED_END_CPU_ACCESS(sobj->robj, sobj))
+ /* cache clean */
+ dma_buf_end_cpu_access(dmabuf, 0, dmabuf->size,
+ DMA_TO_DEVICE);
+ else if (NEED_BEGIN_CPU_ACCESS(sobj->robj, sobj))
+ /* cache invalidate */
+ dma_buf_begin_cpu_access(dmabuf, 0, dmabuf->size,
+ DMA_FROM_DEVICE);
+
+out:
+ /* Update access type to new one. */
+ sobj->robj->accessed_type = sobj->access_type;
+ }
+
+ mutex_unlock(&sync->lock);
+}
+
+static void dmabuf_sync_lock_timeout(unsigned long arg)
+{
+ struct dmabuf_sync *sync = (struct dmabuf_sync *)arg;
+
+ schedule_work(&sync->work);
+}
+
+static int dmabuf_sync_lock_objs(struct dmabuf_sync *sync,
+ struct ww_acquire_ctx *ctx)
+{
+ struct dmabuf_sync_object *contended_sobj = NULL;
+ struct dmabuf_sync_object *res_sobj = NULL;
+ struct dmabuf_sync_object *sobj = NULL;
+ int ret;
+
+ if (ctx)
+ ww_acquire_init(ctx, &reservation_ww_class);
+
+retry:
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (WARN_ON(!sobj->robj))
+ continue;
+
+ /* Don't lock in case of read and read. */
+ if (sobj->robj->accessed_type & DMA_BUF_ACCESS_R &&
+ sobj->access_type & DMA_BUF_ACCESS_R) {
+ atomic_inc(&sobj->robj->shared_cnt);
+ sobj->robj->shared = true;
+ continue;
+ }
+
+ if (sobj = res_sobj) {
+ res_sobj = NULL;
+ continue;
+ }
+
+ ret = ww_mutex_lock(&sobj->robj->lock, ctx);
+ if (ret < 0) {
+ contended_sobj = sobj;
+
+ if (ret = -EDEADLK)
+ printk(KERN_WARNING"%s: deadlock = 0x%x\n",
+ sync->name, (u32)sobj->dmabuf);
+ goto err;
+ }
+
+ sobj->robj->locked = true;
+ }
+
+ if (ctx)
+ ww_acquire_done(ctx);
+
+ init_timer(&sync->timer);
+
+ sync->timer.data = (unsigned long)sync;
+ sync->timer.function = dmabuf_sync_lock_timeout;
+ sync->timer.expires = jiffies + (HZ * MAX_SYNC_TIMEOUT);
+
+ add_timer(&sync->timer);
+
+ return 0;
+
+err:
+ list_for_each_entry_continue_reverse(sobj, &sync->syncs, head) {
+ /* Don't need to unlock in case of read and read. */
+ if (atomic_add_unless(&sobj->robj->shared_cnt, -1, 1))
+ continue;
+
+ ww_mutex_unlock(&sobj->robj->lock);
+ sobj->robj->locked = false;
+ }
+
+ if (res_sobj) {
+ if (!atomic_add_unless(&res_sobj->robj->shared_cnt, -1, 1)) {
+ ww_mutex_unlock(&res_sobj->robj->lock);
+ res_sobj->robj->locked = false;
+ }
+ }
+
+ if (ret = -EDEADLK) {
+ ww_mutex_lock_slow(&contended_sobj->robj->lock, ctx);
+ res_sobj = contended_sobj;
+
+ goto retry;
+ }
+
+ if (ctx)
+ ww_acquire_fini(ctx);
+
+ return ret;
+}
+
+static void dmabuf_sync_unlock_objs(struct dmabuf_sync *sync,
+ struct ww_acquire_ctx *ctx)
+{
+ struct dmabuf_sync_object *sobj;
+
+ if (list_empty(&sync->syncs))
+ return;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (sobj->robj->shared) {
+ if (atomic_add_unless(&sobj->robj->shared_cnt, -1 , 1))
+ continue;
+
+ ww_mutex_unlock(&sobj->robj->lock);
+ sobj->robj->shared = false;
+ sobj->robj->locked = false;
+ } else {
+ ww_mutex_unlock(&sobj->robj->lock);
+ sobj->robj->locked = false;
+ }
+ }
+
+ mutex_unlock(&sync->lock);
+
+ if (ctx)
+ ww_acquire_fini(ctx);
+
+ del_timer(&sync->timer);
+}
+
+/**
+ * is_dmabuf_sync_supported - Check if dmabuf sync is supported or not.
+ */
+bool is_dmabuf_sync_supported(void)
+{
+ return dmabuf_sync_enabled = 1;
+}
+EXPORT_SYMBOL(is_dmabuf_sync_supported);
+
+/**
+ * dmabuf_sync_init - Allocate and initialize a dmabuf sync.
+ *
+ * @priv: A device private data.
+ * @name: A sync object name.
+ *
+ * This function should be called when a device context or an event
+ * context such as a page flip event is created. And the created
+ * dmabuf_sync object should be set to the context.
+ * The caller can get a new sync object for buffer synchronization
+ * through this function.
+ */
+struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name)
+{
+ struct dmabuf_sync *sync;
+
+ sync = kzalloc(sizeof(*sync), GFP_KERNEL);
+ if (!sync)
+ return ERR_PTR(-ENOMEM);
+
+ strncpy(sync->name, name, ARRAY_SIZE(sync->name) - 1);
+
+ sync->priv = priv;
+ INIT_LIST_HEAD(&sync->syncs);
+ mutex_init(&sync->lock);
+ INIT_WORK(&sync->work, dmabuf_sync_timeout_worker);
+
+ return sync;
+}
+EXPORT_SYMBOL(dmabuf_sync_init);
+
+/**
+ * dmabuf_sync_fini - Release a given dmabuf sync.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_init call to release relevant resources, and after
+ * dmabuf_sync_unlock function is called.
+ */
+void dmabuf_sync_fini(struct dmabuf_sync *sync)
+{
+ if (WARN_ON(!sync))
+ return;
+
+ kfree(sync);
+}
+EXPORT_SYMBOL(dmabuf_sync_fini);
+
+/*
+ * dmabuf_sync_get_obj - Add a given object to syncs list.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ * @dmabuf: An object to dma_buf structure.
+ * @type: A access type to a dma buf.
+ * The DMA_BUF_ACCESS_R means that this dmabuf could be accessed by
+ * others for read access. On the other hand, the DMA_BUF_ACCESS_W
+ * means that this dmabuf couldn't be accessed by others but would be
+ * accessed by caller's dma exclusively. And the DMA_BUF_ACCESS_DMA can be
+ * combined.
+ *
+ * This function creates and initializes a new dmabuf sync object and it adds
+ * the dmabuf sync object to syncs list to track and manage all dmabufs.
+ */
+static int dmabuf_sync_get_obj(struct dmabuf_sync *sync, struct dma_buf *dmabuf,
+ unsigned int type)
+{
+ struct dmabuf_sync_object *sobj;
+
+ if (!dmabuf->resv) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ if (!IS_VALID_DMA_BUF_ACCESS_TYPE(type))
+ return -EINVAL;
+
+ if ((type & DMA_BUF_ACCESS_RW) = DMA_BUF_ACCESS_RW)
+ type &= ~DMA_BUF_ACCESS_R;
+
+ sobj = kzalloc(sizeof(*sobj), GFP_KERNEL);
+ if (!sobj) {
+ WARN_ON(1);
+ return -ENOMEM;
+ }
+
+ sobj->dmabuf = dmabuf;
+ sobj->robj = dmabuf->resv;
+
+ mutex_lock(&sync->lock);
+ list_add_tail(&sobj->head, &sync->syncs);
+ mutex_unlock(&sync->lock);
+
+ get_dma_buf(dmabuf);
+
+ sobj->access_type = type;
+
+ return 0;
+}
+
+/*
+ * dmabuf_sync_put_obj - Release a given sync object.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get_obj call to release a given sync object.
+ */
+static void dmabuf_sync_put_obj(struct dmabuf_sync *sync,
+ struct dma_buf *dmabuf)
+{
+ struct dmabuf_sync_object *sobj;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (sobj->dmabuf != dmabuf)
+ continue;
+
+ dma_buf_put(sobj->dmabuf);
+
+ list_del_init(&sobj->head);
+ kfree(sobj);
+ break;
+ }
+
+ if (list_empty(&sync->syncs))
+ sync->status = 0;
+
+ mutex_unlock(&sync->lock);
+}
+
+/*
+ * dmabuf_sync_put_objs - Release all sync objects of dmabuf_sync.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get_obj call to release all sync objects.
+ */
+static void dmabuf_sync_put_objs(struct dmabuf_sync *sync)
+{
+ struct dmabuf_sync_object *sobj, *next;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry_safe(sobj, next, &sync->syncs, head) {
+ dma_buf_put(sobj->dmabuf);
+
+ list_del_init(&sobj->head);
+ kfree(sobj);
+ }
+
+ mutex_unlock(&sync->lock);
+
+ sync->status = 0;
+}
+
+/**
+ * dmabuf_sync_lock - lock all dmabufs added to syncs list.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * The caller should call this function prior to CPU or DMA access to
+ * the dmabufs so that others can not access the dmabufs.
+ * Internally, this function avoids dead lock issue with ww-mutex.
+ */
+int dmabuf_sync_lock(struct dmabuf_sync *sync)
+{
+ int ret;
+
+ if (!sync) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ if (list_empty(&sync->syncs))
+ return -EINVAL;
+
+ if (sync->status != DMABUF_SYNC_GOT)
+ return -EINVAL;
+
+ ret = dmabuf_sync_lock_objs(sync, &sync->ctx);
+ if (ret < 0) {
+ WARN_ON(1);
+ return ret;
+ }
+
+ sync->status = DMABUF_SYNC_LOCKED;
+
+ dmabuf_sync_cache_ops(sync);
+
+ return ret;
+}
+EXPORT_SYMBOL(dmabuf_sync_lock);
+
+/**
+ * dmabuf_sync_unlock - unlock all objects added to syncs list.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * The caller should call this function after CPU or DMA access to
+ * the dmabufs is completed so that others can access the dmabufs.
+ */
+int dmabuf_sync_unlock(struct dmabuf_sync *sync)
+{
+ if (!sync) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ /* If current dmabuf sync object wasn't reserved then just return. */
+ if (sync->status != DMABUF_SYNC_LOCKED)
+ return -EAGAIN;
+
+ dmabuf_sync_unlock_objs(sync, &sync->ctx);
+
+ return 0;
+}
+EXPORT_SYMBOL(dmabuf_sync_unlock);
+
+/**
+ * dmabuf_sync_get - initialize reservation entry and update
+ * dmabuf sync.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ * @sync_buf: A dma_buf object pointer that we want to be synchronized
+ * with others.
+ *
+ * This function should be called after dmabuf_sync_init function is called.
+ * The caller can group multiple dmabufs by calling this function several
+ * times. Internally, this function also takes a reference to a dmabuf.
+ */
+int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf, unsigned int type)
+{
+ int ret;
+
+ if (!sync || !sync_buf) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ ret = dmabuf_sync_get_obj(sync, sync_buf, type);
+ if (ret < 0) {
+ WARN_ON(1);
+ return ret;
+ }
+
+ sync->status = DMABUF_SYNC_GOT;
+
+ return 0;
+}
+EXPORT_SYMBOL(dmabuf_sync_get);
+
+/**
+ * dmabuf_sync_put - Release a given dmabuf.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ * @dmabuf: An object to dma_buf structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get function is called to release the dmabuf, or
+ * dmabuf_sync_unlock function is called.
+ */
+void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf)
+{
+ if (!sync || !dmabuf) {
+ WARN_ON(1);
+ return;
+ }
+
+ if (list_empty(&sync->syncs))
+ return;
+
+ dmabuf_sync_put_obj(sync, dmabuf);
+}
+EXPORT_SYMBOL(dmabuf_sync_put);
+
+/**
+ * dmabuf_sync_put_all - Release all sync objects
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get function is called to release all sync objects, or
+ * dmabuf_sync_unlock function is called.
+ */
+void dmabuf_sync_put_all(struct dmabuf_sync *sync)
+{
+ if (!sync) {
+ WARN_ON(1);
+ return;
+ }
+
+ if (list_empty(&sync->syncs))
+ return;
+
+ dmabuf_sync_put_objs(sync);
+}
+EXPORT_SYMBOL(dmabuf_sync_put_all);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 34cfbac..ae0c87b 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -150,6 +150,20 @@ struct dma_buf_attachment {
void *priv;
};
+#define DMA_BUF_ACCESS_R 0x1
+#define DMA_BUF_ACCESS_W 0x2
+#define DMA_BUF_ACCESS_DMA 0x4
+#define DMA_BUF_ACCESS_RW (DMA_BUF_ACCESS_R | DMA_BUF_ACCESS_W)
+#define DMA_BUF_ACCESS_DMA_R (DMA_BUF_ACCESS_R | DMA_BUF_ACCESS_DMA)
+#define DMA_BUF_ACCESS_DMA_W (DMA_BUF_ACCESS_W | DMA_BUF_ACCESS_DMA)
+#define DMA_BUF_ACCESS_DMA_RW (DMA_BUF_ACCESS_DMA_R | DMA_BUF_ACCESS_DMA_W)
+#define IS_VALID_DMA_BUF_ACCESS_TYPE(t) (t = DMA_BUF_ACCESS_R || \
+ t = DMA_BUF_ACCESS_W || \
+ t = DMA_BUF_ACCESS_DMA_R || \
+ t = DMA_BUF_ACCESS_DMA_W || \
+ t = DMA_BUF_ACCESS_RW || \
+ t = DMA_BUF_ACCESS_DMA_RW)
+
/**
* get_dma_buf - convenience wrapper for get_file.
* @dmabuf: [in] pointer to dma_buf
diff --git a/include/linux/dmabuf-sync.h b/include/linux/dmabuf-sync.h
new file mode 100644
index 0000000..44c37de
--- /dev/null
+++ b/include/linux/dmabuf-sync.h
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ * Authors:
+ * Inki Dae <inki.dae@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/mutex.h>
+#include <linux/sched.h>
+#include <linux/dma-buf.h>
+#include <linux/reservation.h>
+
+enum dmabuf_sync_status {
+ DMABUF_SYNC_GOT = 1,
+ DMABUF_SYNC_LOCKED,
+};
+
+/*
+ * A structure for dmabuf_sync_object.
+ *
+ * @head: A list head to be added to syncs list.
+ * @robj: A reservation_object object.
+ * @dma_buf: A dma_buf object.
+ * @access_type: Indicate how a current task tries to access
+ * a given buffer.
+ */
+struct dmabuf_sync_object {
+ struct list_head head;
+ struct reservation_object *robj;
+ struct dma_buf *dmabuf;
+ unsigned int access_type;
+};
+
+/*
+ * A structure for dmabuf_sync.
+ *
+ * @syncs: A list head to sync object and this is global to system.
+ * @list: A list entry used as committed list node
+ * @lock: A mutex lock to current sync object.
+ * @ctx: A current context for ww mutex.
+ * @work: A work struct to release resources at timeout.
+ * @priv: A private data.
+ * @name: A string to dmabuf sync owner.
+ * @timer: A timer list to avoid lockup and release resources.
+ * @status: Indicate current status (DMABUF_SYNC_GOT or DMABUF_SYNC_LOCKED).
+ */
+struct dmabuf_sync {
+ struct list_head syncs;
+ struct list_head list;
+ struct mutex lock;
+ struct ww_acquire_ctx ctx;
+ struct work_struct work;
+ void *priv;
+ char name[64];
+ struct timer_list timer;
+ unsigned int status;
+};
+
+#ifdef CONFIG_DMABUF_SYNC
+extern bool is_dmabuf_sync_supported(void);
+
+extern struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name);
+
+extern void dmabuf_sync_fini(struct dmabuf_sync *sync);
+
+extern int dmabuf_sync_lock(struct dmabuf_sync *sync);
+
+extern int dmabuf_sync_unlock(struct dmabuf_sync *sync);
+
+extern int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf,
+ unsigned int type);
+
+extern void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf);
+
+extern void dmabuf_sync_put_all(struct dmabuf_sync *sync);
+
+#else
+static inline bool is_dmabuf_sync_supported(void) { return false; }
+
+static inline struct dmabuf_sync *dmabuf_sync_init(void *priv,
+ const char *names)
+{
+ return ERR_PTR(0);
+}
+
+static inline void dmabuf_sync_fini(struct dmabuf_sync *sync) { }
+
+static inline int dmabuf_sync_lock(struct dmabuf_sync *sync)
+{
+ return 0;
+}
+
+static inline int dmabuf_sync_unlock(struct dmabuf_sync *sync)
+{
+ return 0;
+}
+
+static inline int dmabuf_sync_get(struct dmabuf_sync *sync,
+ void *sync_buf,
+ unsigned int type)
+{
+ return 0;
+}
+
+static inline void dmabuf_sync_put(struct dmabuf_sync *sync,
+ struct dma_buf *dmabuf) { }
+
+static inline void dmabuf_sync_put_all(struct dmabuf_sync *sync) { }
+
+#endif
diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index 80050e2..4310192 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -50,6 +50,11 @@ struct reservation_object {
struct fence *fence_excl;
struct fence **fence_shared;
u32 fence_shared_count, fence_shared_max;
+
+ atomic_t shared_cnt;
+ unsigned int accessed_type;
+ unsigned int shared;
+ unsigned int locked;
};
static inline void
@@ -60,6 +65,8 @@ reservation_object_init(struct reservation_object *obj)
obj->fence_shared_count = obj->fence_shared_max = 0;
obj->fence_shared = NULL;
obj->fence_excl = NULL;
+
+ atomic_set(&obj->shared_cnt, 1);
}
static inline void
--
1.7.5.4
^ permalink raw reply related
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Maarten Lankhorst @ 2013-06-17 11:34 UTC (permalink / raw)
To: Inki Dae
Cc: dri-devel, linux-fbdev, linux-arm-kernel, linux-media, daniel,
robdclark, kyungmin.park, myungjoo.ham, yj44.cho
In-Reply-To: <1371467722-665-1-git-send-email-inki.dae@samsung.com>
Op 17-06-13 13:15, Inki Dae schreef:
> This patch adds a buffer synchronization framework based on DMA BUF[1]
> and reservation[2] to use dma-buf resource, and based on ww-mutexes[3]
> for lock mechanism.
>
> The purpose of this framework is not only to couple cache operations,
> and buffer access control to CPU and DMA but also to provide easy-to-use
> interfaces for device drivers and potentially user application
> (not implemented for user applications, yet). And this framework can be
> used for all dma devices using system memory as dma buffer, especially
> for most ARM based SoCs.
>
> Changelog v2:
> - use atomic_add_unless to avoid potential bug.
> - add a macro for checking valid access type.
> - code clean.
>
> The mechanism of this framework has the following steps,
> 1. Register dmabufs to a sync object - A task gets a new sync object and
> can add one or more dmabufs that the task wants to access.
> This registering should be performed when a device context or an event
> context such as a page flip event is created or before CPU accesses a shared
> buffer.
>
> dma_buf_sync_get(a sync object, a dmabuf);
>
> 2. Lock a sync object - A task tries to lock all dmabufs added in its own
> sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead
> lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
> and DMA. Taking a lock means that others cannot access all locked dmabufs
> until the task that locked the corresponding dmabufs, unlocks all the locked
> dmabufs.
> This locking should be performed before DMA or CPU accesses these dmabufs.
>
> dma_buf_sync_lock(a sync object);
>
> 3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
> object. The unlock means that the DMA or CPU accesses to the dmabufs have
> been completed so that others may access them.
> This unlocking should be performed after DMA or CPU has completed accesses
> to the dmabufs.
>
> dma_buf_sync_unlock(a sync object);
>
> 4. Unregister one or all dmabufs from a sync object - A task unregisters
> the given dmabufs from the sync object. This means that the task dosen't
> want to lock the dmabufs.
> The unregistering should be performed after DMA or CPU has completed
> accesses to the dmabufs or when dma_buf_sync_lock() is failed.
>
> dma_buf_sync_put(a sync object, a dmabuf);
> dma_buf_sync_put_all(a sync object);
>
> The described steps may be summarized as:
> get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
>
> This framework includes the following two features.
> 1. read (shared) and write (exclusive) locks - A task is required to declare
> the access type when the task tries to register a dmabuf;
> READ, WRITE, READ DMA, or WRITE DMA.
>
> The below is example codes,
> struct dmabuf_sync *sync;
>
> sync = dmabuf_sync_init(NULL, "test sync");
>
> dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> ...
>
> And the below can be used as access types:
> DMA_BUF_ACCESS_READ,
> - CPU will access a buffer for read.
> DMA_BUF_ACCESS_WRITE,
> - CPU will access a buffer for read or write.
> DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA,
> - DMA will access a buffer for read
> DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA,
> - DMA will access a buffer for read or write.
>
> 2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
> A task may never try to unlock a buffer after taking a lock to the buffer.
> In this case, a timer handler to the corresponding sync object is called
> in five (default) seconds and then the timed-out buffer is unlocked by work
> queue handler to avoid lockups and to enforce resources of the buffer.
>
> The below is how to use:
> 1. Allocate and Initialize a sync object:
> struct dmabuf_sync *sync;
>
> sync = dmabuf_sync_init(NULL, "test sync");
> ...
>
> 2. Add a dmabuf to the sync object when setting up dma buffer relevant
> registers:
> dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> ...
>
> 3. Lock all dmabufs of the sync object before DMA or CPU accesses
> the dmabufs:
> dmabuf_sync_lock(sync);
> ...
>
> 4. Now CPU or DMA can access all dmabufs locked in step 3.
>
> 5. Unlock all dmabufs added in a sync object after DMA or CPU access
> to these dmabufs is completed:
> dmabuf_sync_unlock(sync);
>
> And call the following functions to release all resources,
> dmabuf_sync_put_all(sync);
> dmabuf_sync_fini(sync);
>
> You can refer to actual example codes:
> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
> commit/?h=dmabuf-sync&id@30bdee9bab5841ad32faade528d04cc0c5fc94
>
> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
> commit/?h=dmabuf-sync&idla548e9ea9e865592719ef6b1cde58366af9f5c
>
> The framework performs cache operation based on the previous and current access
> types to the dmabufs after the locks to all dmabufs are taken:
> Call dma_buf_begin_cpu_access() to invalidate cache if,
> previous access type is DMA_BUF_ACCESS_WRITE | DMA and
> current access type is DMA_BUF_ACCESS_READ
>
> Call dma_buf_end_cpu_access() to clean cache if,
> previous access type is DMA_BUF_ACCESS_WRITE and
> current access type is DMA_BUF_ACCESS_READ | DMA
>
> Such cache operations are invoked via dma-buf interfaces so the dma buf exporter
> should implement dmabuf->ops->begin_cpu_access/end_cpu_access callbacks.
>
> [1] http://lwn.net/Articles/470339/
> [2] http://lwn.net/Articles/532616/
> [3] https://patchwork-mail1.kernel.org/patch/2625321/
>
Looks to me like you're just writing an api similar to the android syncpoint for this.
Is there any reason you had to reimplement the android syncpoint api?
I'm not going into a full review, you may wish to rethink the design first.
All the criticisms I had with the original design approach still apply.
A few things that stand out from a casual glance:
bool is_dmabuf_sync_supported(void)
-> any code that needs CONFIG_DMABUF_SYNC should select it in their kconfig
runtime enabling/disabling of synchronization is a recipe for disaster, remove the !CONFIG_DMABUF_SYNC fallbacks.
NEVER add a runtime way to influence locking behavior.
Considering you're also holding dmaobj->lock for the entire duration, is there any point to not simply call begin_cpu_access/end_cpu_access, and forget this ugly code ever existed?
I still don't see the problem you're trying to solve..
~Maarten
^ permalink raw reply
* RE: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Inki Dae @ 2013-06-17 13:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371467722-665-1-git-send-email-inki.dae@samsung.com>
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> Sent: Monday, June 17, 2013 8:35 PM
> To: Inki Dae
> Cc: dri-devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; linux-
> arm-kernel@lists.infradead.org; linux-media@vger.kernel.org;
> daniel@ffwll.ch; robdclark@gmail.com; kyungmin.park@samsung.com;
> myungjoo.ham@samsung.com; yj44.cho@samsung.com
> Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
> framework
>
> Op 17-06-13 13:15, Inki Dae schreef:
> > This patch adds a buffer synchronization framework based on DMA BUF[1]
> > and reservation[2] to use dma-buf resource, and based on ww-mutexes[3]
> > for lock mechanism.
> >
> > The purpose of this framework is not only to couple cache operations,
> > and buffer access control to CPU and DMA but also to provide easy-to-use
> > interfaces for device drivers and potentially user application
> > (not implemented for user applications, yet). And this framework can be
> > used for all dma devices using system memory as dma buffer, especially
> > for most ARM based SoCs.
> >
> > Changelog v2:
> > - use atomic_add_unless to avoid potential bug.
> > - add a macro for checking valid access type.
> > - code clean.
> >
> > The mechanism of this framework has the following steps,
> > 1. Register dmabufs to a sync object - A task gets a new sync object
> and
> > can add one or more dmabufs that the task wants to access.
> > This registering should be performed when a device context or an
> event
> > context such as a page flip event is created or before CPU accesses
a
> shared
> > buffer.
> >
> > dma_buf_sync_get(a sync object, a dmabuf);
> >
> > 2. Lock a sync object - A task tries to lock all dmabufs added in
its
> own
> > sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid
> dead
> > lock issue and for race condition between CPU and CPU, CPU and DMA,
> and DMA
> > and DMA. Taking a lock means that others cannot access all locked
> dmabufs
> > until the task that locked the corresponding dmabufs, unlocks all
the
> locked
> > dmabufs.
> > This locking should be performed before DMA or CPU accesses these
> dmabufs.
> >
> > dma_buf_sync_lock(a sync object);
> >
> > 3. Unlock a sync object - The task unlocks all dmabufs added in its
> own sync
> > object. The unlock means that the DMA or CPU accesses to the dmabufs
> have
> > been completed so that others may access them.
> > This unlocking should be performed after DMA or CPU has completed
> accesses
> > to the dmabufs.
> >
> > dma_buf_sync_unlock(a sync object);
> >
> > 4. Unregister one or all dmabufs from a sync object - A task
> unregisters
> > the given dmabufs from the sync object. This means that the task
> dosen't
> > want to lock the dmabufs.
> > The unregistering should be performed after DMA or CPU has completed
> > accesses to the dmabufs or when dma_buf_sync_lock() is failed.
> >
> > dma_buf_sync_put(a sync object, a dmabuf);
> > dma_buf_sync_put_all(a sync object);
> >
> > The described steps may be summarized as:
> > get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
> >
> > This framework includes the following two features.
> > 1. read (shared) and write (exclusive) locks - A task is required to
> declare
> > the access type when the task tries to register a dmabuf;
> > READ, WRITE, READ DMA, or WRITE DMA.
> >
> > The below is example codes,
> > struct dmabuf_sync *sync;
> >
> > sync = dmabuf_sync_init(NULL, "test sync");
> >
> > dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> > ...
> >
> > And the below can be used as access types:
> > DMA_BUF_ACCESS_READ,
> > - CPU will access a buffer for read.
> > DMA_BUF_ACCESS_WRITE,
> > - CPU will access a buffer for read or write.
> > DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA,
> > - DMA will access a buffer for read
> > DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA,
> > - DMA will access a buffer for read or write.
> >
> > 2. Mandatory resource releasing - a task cannot hold a lock
> indefinitely.
> > A task may never try to unlock a buffer after taking a lock to the
> buffer.
> > In this case, a timer handler to the corresponding sync object is
> called
> > in five (default) seconds and then the timed-out buffer is unlocked
> by work
> > queue handler to avoid lockups and to enforce resources of the
buffer.
> >
> > The below is how to use:
> > 1. Allocate and Initialize a sync object:
> > struct dmabuf_sync *sync;
> >
> > sync = dmabuf_sync_init(NULL, "test sync");
> > ...
> >
> > 2. Add a dmabuf to the sync object when setting up dma buffer
> relevant
> > registers:
> > dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> > ...
> >
> > 3. Lock all dmabufs of the sync object before DMA or CPU accesses
> > the dmabufs:
> > dmabuf_sync_lock(sync);
> > ...
> >
> > 4. Now CPU or DMA can access all dmabufs locked in step 3.
> >
> > 5. Unlock all dmabufs added in a sync object after DMA or CPU
> access
> > to these dmabufs is completed:
> > dmabuf_sync_unlock(sync);
> >
> > And call the following functions to release all resources,
> > dmabuf_sync_put_all(sync);
> > dmabuf_sync_fini(sync);
> >
> > You can refer to actual example codes:
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/
> > commit/?h=dmabuf-
> sync&id@30bdee9bab5841ad32faade528d04cc0c5fc94
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/
> > commit/?h=dmabuf-
> sync&idla548e9ea9e865592719ef6b1cde58366af9f5c
> >
> > The framework performs cache operation based on the previous and current
> access
> > types to the dmabufs after the locks to all dmabufs are taken:
> > Call dma_buf_begin_cpu_access() to invalidate cache if,
> > previous access type is DMA_BUF_ACCESS_WRITE | DMA and
> > current access type is DMA_BUF_ACCESS_READ
> >
> > Call dma_buf_end_cpu_access() to clean cache if,
> > previous access type is DMA_BUF_ACCESS_WRITE and
> > current access type is DMA_BUF_ACCESS_READ | DMA
> >
> > Such cache operations are invoked via dma-buf interfaces so the dma buf
> exporter
> > should implement dmabuf->ops->begin_cpu_access/end_cpu_access callbacks.
> >
> > [1] http://lwn.net/Articles/470339/
> > [2] http://lwn.net/Articles/532616/
> > [3] https://patchwork-mail1.kernel.org/patch/2625321/
> >
> Looks to me like you're just writing an api similar to the android
> syncpoint for this.
> Is there any reason you had to reimplement the android syncpoint api?
Right, only difference is that maybe android sync driver, you mentioned as
syncpoint, doesn't use dma-buf resource. What I try to do is familiar to
android's one and also ARM's KDS (Kernel Dependency System). I think I
already mentioned enough through a document file about why I try to
implement this approach based on dma-buf; coupling cache operation and
buffer synchronization between CPU and DMA.
> I'm not going into a full review, you may wish to rethink the design
first.
> All the criticisms I had with the original design approach still apply.
>
Isn't that enough if what I try to do is similar to android sync driver?
It's very simple and that's all I try to do.:)
>
>
> A few things that stand out from a casual glance:
>
> bool is_dmabuf_sync_supported(void)
> -> any code that needs CONFIG_DMABUF_SYNC should select it in their
> kconfig
> runtime enabling/disabling of synchronization is a recipe for disaster,
> remove the !CONFIG_DMABUF_SYNC fallbacks.
> NEVER add a runtime way to influence locking behavior.
>
Not enabling/disabling synchronization feature in runtime. That is
determined at build time.
> Considering you're also holding dmaobj->lock for the entire duration, is
> there any point to not simply call begin_cpu_access/end_cpu_access, and
> forget this ugly code ever existed?
You mean mutex_lock(&sync->lock)? Yeah, it seems unnecessary in that case.
> I still don't see the problem you're trying to solve..
>
It's just to implement a thin sync framework coupling cache operation. This
approach is based on dma-buf for more generic implementation against android
sync driver or KDS.
The described steps may be summarized as:
lock -> cache operation -> CPU or DMA access to a buffer/s -> unlock
I think that there is no need to get complicated for such approach at least
for most devices sharing system memory. Simple is best.
Thanks,
Inki Dae
> ~Maarten
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Russell King - ARM Linux @ 2013-06-17 13:31 UTC (permalink / raw)
To: Inki Dae
Cc: 'Maarten Lankhorst', linux-fbdev, kyungmin.park,
dri-devel, robdclark, myungjoo.ham, yj44.cho, daniel,
linux-arm-kernel, linux-media
In-Reply-To: <012501ce6b5b$3d39b0b0$b7ad1210$%dae@samsung.com>
On Mon, Jun 17, 2013 at 10:04:45PM +0900, Inki Dae wrote:
> It's just to implement a thin sync framework coupling cache operation. This
> approach is based on dma-buf for more generic implementation against android
> sync driver or KDS.
>
> The described steps may be summarized as:
> lock -> cache operation -> CPU or DMA access to a buffer/s -> unlock
>
> I think that there is no need to get complicated for such approach at least
> for most devices sharing system memory. Simple is best.
But hang on, doesn't the dmabuf API already provide that?
The dmabuf API already uses dma_map_sg() and dma_unmap_sg() by providers,
and the rules around the DMA API are that:
dma_map_sg()
/* DMA _ONLY_ has access, CPU should not access */
dma_unmap_sg()
/* DMA may not access, CPU can access */
It's a little more than that if you include the sync_sg_for_cpu and
sync_sg_for_device APIs too - but the above is the general idea. What
this means from the dmabuf API point of view is that once you attach to
a dma_buf, and call dma_buf_map_attachment() to get the SG list, the CPU
doesn't have ownership of the buffer and _must_ _not_ access it via any
other means - including using the other dma_buf methods, until either
the appropriate dma_sync_sg_for_cpu() call has been made or the DMA
mapping has been removed via dma_buf_unmap_attachment().
So, the sequence should be:
dma_buf_map_attachment()
/* do DMA */
dma_buf_unmap_attachment()
/* CPU can now access the buffer */
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Maarten Lankhorst @ 2013-06-17 13:31 UTC (permalink / raw)
To: Inki Dae
Cc: dri-devel, linux-fbdev, linux-arm-kernel, linux-media, daniel,
robdclark, kyungmin.park, myungjoo.ham, yj44.cho
In-Reply-To: <012501ce6b5b$3d39b0b0$b7ad1210$%dae@samsung.com>
Op 17-06-13 15:04, Inki Dae schreef:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
>> Sent: Monday, June 17, 2013 8:35 PM
>> To: Inki Dae
>> Cc: dri-devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; linux-
>> arm-kernel@lists.infradead.org; linux-media@vger.kernel.org;
>> daniel@ffwll.ch; robdclark@gmail.com; kyungmin.park@samsung.com;
>> myungjoo.ham@samsung.com; yj44.cho@samsung.com
>> Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
>> framework
>>
>> Op 17-06-13 13:15, Inki Dae schreef:
>>> This patch adds a buffer synchronization framework based on DMA BUF[1]
>>> and reservation[2] to use dma-buf resource, and based on ww-mutexes[3]
>>> for lock mechanism.
>>>
>>> The purpose of this framework is not only to couple cache operations,
>>> and buffer access control to CPU and DMA but also to provide easy-to-use
>>> interfaces for device drivers and potentially user application
>>> (not implemented for user applications, yet). And this framework can be
>>> used for all dma devices using system memory as dma buffer, especially
>>> for most ARM based SoCs.
>>>
>>> Changelog v2:
>>> - use atomic_add_unless to avoid potential bug.
>>> - add a macro for checking valid access type.
>>> - code clean.
>>>
>>> The mechanism of this framework has the following steps,
>>> 1. Register dmabufs to a sync object - A task gets a new sync object
>> and
>>> can add one or more dmabufs that the task wants to access.
>>> This registering should be performed when a device context or an
>> event
>>> context such as a page flip event is created or before CPU accesses
> a
>> shared
>>> buffer.
>>>
>>> dma_buf_sync_get(a sync object, a dmabuf);
>>>
>>> 2. Lock a sync object - A task tries to lock all dmabufs added in
> its
>> own
>>> sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid
>> dead
>>> lock issue and for race condition between CPU and CPU, CPU and DMA,
>> and DMA
>>> and DMA. Taking a lock means that others cannot access all locked
>> dmabufs
>>> until the task that locked the corresponding dmabufs, unlocks all
> the
>> locked
>>> dmabufs.
>>> This locking should be performed before DMA or CPU accesses these
>> dmabufs.
>>> dma_buf_sync_lock(a sync object);
>>>
>>> 3. Unlock a sync object - The task unlocks all dmabufs added in its
>> own sync
>>> object. The unlock means that the DMA or CPU accesses to the dmabufs
>> have
>>> been completed so that others may access them.
>>> This unlocking should be performed after DMA or CPU has completed
>> accesses
>>> to the dmabufs.
>>>
>>> dma_buf_sync_unlock(a sync object);
>>>
>>> 4. Unregister one or all dmabufs from a sync object - A task
>> unregisters
>>> the given dmabufs from the sync object. This means that the task
>> dosen't
>>> want to lock the dmabufs.
>>> The unregistering should be performed after DMA or CPU has completed
>>> accesses to the dmabufs or when dma_buf_sync_lock() is failed.
>>>
>>> dma_buf_sync_put(a sync object, a dmabuf);
>>> dma_buf_sync_put_all(a sync object);
>>>
>>> The described steps may be summarized as:
>>> get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
>>>
>>> This framework includes the following two features.
>>> 1. read (shared) and write (exclusive) locks - A task is required to
>> declare
>>> the access type when the task tries to register a dmabuf;
>>> READ, WRITE, READ DMA, or WRITE DMA.
>>>
>>> The below is example codes,
>>> struct dmabuf_sync *sync;
>>>
>>> sync = dmabuf_sync_init(NULL, "test sync");
>>>
>>> dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
>>> ...
>>>
>>> And the below can be used as access types:
>>> DMA_BUF_ACCESS_READ,
>>> - CPU will access a buffer for read.
>>> DMA_BUF_ACCESS_WRITE,
>>> - CPU will access a buffer for read or write.
>>> DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA,
>>> - DMA will access a buffer for read
>>> DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA,
>>> - DMA will access a buffer for read or write.
>>>
>>> 2. Mandatory resource releasing - a task cannot hold a lock
>> indefinitely.
>>> A task may never try to unlock a buffer after taking a lock to the
>> buffer.
>>> In this case, a timer handler to the corresponding sync object is
>> called
>>> in five (default) seconds and then the timed-out buffer is unlocked
>> by work
>>> queue handler to avoid lockups and to enforce resources of the
> buffer.
>>> The below is how to use:
>>> 1. Allocate and Initialize a sync object:
>>> struct dmabuf_sync *sync;
>>>
>>> sync = dmabuf_sync_init(NULL, "test sync");
>>> ...
>>>
>>> 2. Add a dmabuf to the sync object when setting up dma buffer
>> relevant
>>> registers:
>>> dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
>>> ...
>>>
>>> 3. Lock all dmabufs of the sync object before DMA or CPU accesses
>>> the dmabufs:
>>> dmabuf_sync_lock(sync);
>>> ...
>>>
>>> 4. Now CPU or DMA can access all dmabufs locked in step 3.
>>>
>>> 5. Unlock all dmabufs added in a sync object after DMA or CPU
>> access
>>> to these dmabufs is completed:
>>> dmabuf_sync_unlock(sync);
>>>
>>> And call the following functions to release all resources,
>>> dmabuf_sync_put_all(sync);
>>> dmabuf_sync_fini(sync);
>>>
>>> You can refer to actual example codes:
>>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/
>>> commit/?h=dmabuf-
>> sync&id@30bdee9bab5841ad32faade528d04cc0c5fc94
>>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/
>>> commit/?h=dmabuf-
>> sync&idla548e9ea9e865592719ef6b1cde58366af9f5c
>>> The framework performs cache operation based on the previous and current
>> access
>>> types to the dmabufs after the locks to all dmabufs are taken:
>>> Call dma_buf_begin_cpu_access() to invalidate cache if,
>>> previous access type is DMA_BUF_ACCESS_WRITE | DMA and
>>> current access type is DMA_BUF_ACCESS_READ
>>>
>>> Call dma_buf_end_cpu_access() to clean cache if,
>>> previous access type is DMA_BUF_ACCESS_WRITE and
>>> current access type is DMA_BUF_ACCESS_READ | DMA
>>>
>>> Such cache operations are invoked via dma-buf interfaces so the dma buf
>> exporter
>>> should implement dmabuf->ops->begin_cpu_access/end_cpu_access callbacks.
>>>
>>> [1] http://lwn.net/Articles/470339/
>>> [2] http://lwn.net/Articles/532616/
>>> [3] https://patchwork-mail1.kernel.org/patch/2625321/
>>>
>> Looks to me like you're just writing an api similar to the android
>> syncpoint for this.
>> Is there any reason you had to reimplement the android syncpoint api?
> Right, only difference is that maybe android sync driver, you mentioned as
> syncpoint, doesn't use dma-buf resource. What I try to do is familiar to
> android's one and also ARM's KDS (Kernel Dependency System). I think I
> already mentioned enough through a document file about why I try to
> implement this approach based on dma-buf; coupling cache operation and
> buffer synchronization between CPU and DMA.
Making sure caching works correctly can be handled entirely in the begin_cpu_access/end_cpu_access callbacks, without requiring such.. framework
>> I'm not going into a full review, you may wish to rethink the design
> first.
>> All the criticisms I had with the original design approach still apply.
>>
> Isn't that enough if what I try to do is similar to android sync driver?
> It's very simple and that's all I try to do.:)
From what I can tell you should be able to make it work with just the use of fences, you don't need to keep the buffers locked for the entire duration,
just plug in the last fence in the correct slots and you're done..
I'm not sure if the android sync api is compatible enough, but you shouldn't need to write code in this way to make it work..
>>
>> A few things that stand out from a casual glance:
>>
>> bool is_dmabuf_sync_supported(void)
>> -> any code that needs CONFIG_DMABUF_SYNC should select it in their
>> kconfig
>> runtime enabling/disabling of synchronization is a recipe for disaster,
>> remove the !CONFIG_DMABUF_SYNC fallbacks.
>> NEVER add a runtime way to influence locking behavior.
>>
> Not enabling/disabling synchronization feature in runtime. That is
> determined at build time.
>
>> Considering you're also holding dmaobj->lock for the entire duration, is
>> there any point to not simply call begin_cpu_access/end_cpu_access, and
>> forget this ugly code ever existed?
> You mean mutex_lock(&sync->lock)? Yeah, it seems unnecessary in that case.
>
>> I still don't see the problem you're trying to solve..
>>
> It's just to implement a thin sync framework coupling cache operation. This
> approach is based on dma-buf for more generic implementation against android
> sync driver or KDS.
>
> The described steps may be summarized as:
> lock -> cache operation -> CPU or DMA access to a buffer/s -> unlock
>
> I think that there is no need to get complicated for such approach at least
> for most devices sharing system memory. Simple is best.
>
>
> Thanks,
> Inki Dae
>
>> ~Maarten
^ permalink raw reply
* [PATCH] video: of_display_timing.h: Include <video/display_timing.h>
From: Fabio Estevam @ 2013-06-17 13:43 UTC (permalink / raw)
To: linux-fbdev
Commit ffa3fd21de ("videomode: implement public of_get_display_timing()") causes
the following build warning:
include/video/of_display_timing.h:18:10: warning: 'struct display_timing' declared inside parameter list [enabled by default]
include/video/of_display_timing.h:18:10: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
As 'struct display_timing' is defined at <video/display_timing.h>, let's include
this header to avoid the warning.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
include/video/of_display_timing.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/video/of_display_timing.h b/include/video/of_display_timing.h
index 6562ad9..a136f58 100644
--- a/include/video/of_display_timing.h
+++ b/include/video/of_display_timing.h
@@ -8,6 +8,7 @@
#ifndef __LINUX_OF_DISPLAY_TIMING_H
#define __LINUX_OF_DISPLAY_TIMING_H
+#include <video/display_timing.h>
struct device_node;
struct display_timings;
--
1.8.1.2
^ permalink raw reply related
* [GIT PULL] omapdss changes for 3.11, part 1/2
From: Tomi Valkeinen @ 2013-06-17 13:46 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: linux-fbdev, linux-omap
[-- Attachment #1: Type: text/plain, Size: 5893 bytes --]
The following changes since commit 317ddd256b9c24b0d78fa8018f80f1e495481a10:
Linux 3.10-rc5 (2013-06-08 17:41:04 -0700)
are available in the git repository at:
git://gitorious.org/linux-omap-dss2/linux.git tags/omapdss-for-3.11-1
for you to fetch changes up to 595470a7853848cb971d5ee3fed443b1e3aa0d1b:
OMAPDSS: gracefully disable overlay at error (2013-06-17 14:00:56 +0300)
----------------------------------------------------------------
OMAP display subsystem changes for 3.11 (part 1/2)
This is the first part of OMAP DSS changes for 3.11. This part contains fixes,
cleanups and reorganizations that are not directly related to the new DSS
device model that is added in part 2, although many of the reorganizations are
made to make the part 2 possible.
There should not be any functional changes visible to the user except the few
bug fixes.
The main new internal features:
- Display (dis)connect support, which allows us to explicitly (dis)connect a
whole display pipeline
- Panel list, which allows us to operate without the specific DSS bus
- Combine omap_dss_output to omap_dss_device, so that we have one generic
"entity" for display pipeline blocks
----------------------------------------------------------------
Emil Goode (1):
OMAPDSS: Remove kfree for memory allocated with devm_kzalloc
Sergey Kibrik (1):
OMAPDSS: gracefully disable overlay at error
Tomi Valkeinen (36):
OMAPDSS: add pdata->default_display_name
OMAPDSS: only probe pdata if there's one
OMAPDSS: add omap_dss_find_output()
OMAPDSS: add omap_dss_find_output_by_node()
OMAPDSS: fix dss_get_ctx_loss_count for DT
OMAPDSS: clean up dss_[ovl|mgr]_get_device()
OMAPDSS: add helpers to get mgr or output from display
OMAPDSS: split overlay manager creation
OMAPDRM: fix overlay manager handling
OMAPDSS: Implement display (dis)connect support
OMAPDSS: CORE: use devm_regulator_get
OMAPDSS: DSI: cleanup regulator init
OMAPDSS: DPI: cleanup pll & regulator init
OMAPDSS: DPI: fix regulators for DT
OMAPDSS: HDMI: add hdmi_init_regulator
OMAPDSS: SDI: clean up regulator init
OMAPDSS: SDI: fix regulators for DT
OMAPDSS: VENC: clean up regulator init
OMAPDSS: add videomode conversion support
OMAPDSS: remove dssdev uses in trivial cases
OMAPDSS: add panel list
OMAPDSS: use the panel list in omap_dss_get_next_device
OMAPDSS: don't use dss bus in suspend/resume
OMAPDSS: implement display sysfs without dss bus
OMAPDSS: Add panel dev pointer to dssdev
OMAPDSS: remove omap_dss_start/stop_device()
OMAPDSS: combine omap_dss_output into omap_dss_device
OMAPDSS: omapdss.h: add owner field to omap_dss_device
OMAPDSS: add module_get/put to omap_dss_get/put_device()
OMAPDSS: add THIS_MODULE owner to DSS outputs
OMAPDSS: output: increase refcount in find_output funcs
OMAPFB: use EPROBE_DEFER if default display is not present
OMAPDSS: HDMI: clean up PHY power handling
OMAPDSS: HDMI clean up hpd_gpio
OMAPDSS: remove unused fields in omap_dss_device
OMAPDSS: remove dispc's dependency to VENC/HDMI
drivers/gpu/drm/omapdrm/omap_crtc.c | 46 +++-
drivers/gpu/drm/omapdrm/omap_drv.c | 21 +-
drivers/gpu/drm/omapdrm/omap_drv.h | 1 +
drivers/video/omap2/displays/panel-acx565akm.c | 16 +-
drivers/video/omap2/displays/panel-generic-dpi.c | 26 +--
.../omap2/displays/panel-lgphilips-lb035q02.c | 10 +-
drivers/video/omap2/displays/panel-n8x0.c | 30 +--
.../omap2/displays/panel-nec-nl8048hl11-01b.c | 4 +-
drivers/video/omap2/displays/panel-picodlp.c | 34 ++-
.../video/omap2/displays/panel-sharp-ls037v7dw01.c | 10 +-
drivers/video/omap2/displays/panel-taal.c | 164 +++++++-------
drivers/video/omap2/displays/panel-tfp410.c | 32 +--
.../video/omap2/displays/panel-tpo-td043mtea1.c | 36 +--
drivers/video/omap2/dss/Kconfig | 1 +
drivers/video/omap2/dss/apply.c | 47 ++--
drivers/video/omap2/dss/core.c | 108 +++++----
drivers/video/omap2/dss/dispc-compat.c | 3 +-
drivers/video/omap2/dss/dispc.c | 24 +-
drivers/video/omap2/dss/display-sysfs.c | 154 +++++++------
drivers/video/omap2/dss/display.c | 246 ++++++++++++++-------
drivers/video/omap2/dss/dpi.c | 131 +++++------
drivers/video/omap2/dss/dsi.c | 135 +++++------
drivers/video/omap2/dss/dss.c | 3 +-
drivers/video/omap2/dss/dss.h | 35 +--
drivers/video/omap2/dss/dss_features.c | 1 -
drivers/video/omap2/dss/hdmi.c | 107 ++++-----
drivers/video/omap2/dss/manager-sysfs.c | 47 ++--
drivers/video/omap2/dss/manager.c | 29 ++-
drivers/video/omap2/dss/output.c | 78 ++++++-
drivers/video/omap2/dss/rfbi.c | 39 ++--
drivers/video/omap2/dss/sdi.c | 61 ++---
drivers/video/omap2/dss/ti_hdmi.h | 5 +-
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 87 ++++----
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h | 1 +
drivers/video/omap2/dss/venc.c | 87 +++-----
drivers/video/omap2/dss/venc_panel.c | 16 +-
drivers/video/omap2/omapfb/omapfb-ioctl.c | 9 +-
drivers/video/omap2/omapfb/omapfb-main.c | 27 +--
include/video/omapdss.h | 113 ++++++----
39 files changed, 1131 insertions(+), 893 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* [GIT PULL] omapdss changes for 3.11, part 2/2
From: Tomi Valkeinen @ 2013-06-17 13:47 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: linux-fbdev, linux-omap
In-Reply-To: <51BF1326.7010206@ti.com>
[-- Attachment #1: Type: text/plain, Size: 6017 bytes --]
The following changes since commit 595470a7853848cb971d5ee3fed443b1e3aa0d1b:
OMAPDSS: gracefully disable overlay at error (2013-06-17 14:00:56 +0300)
are available in the git repository at:
git://gitorious.org/linux-omap-dss2/linux.git tags/omapdss-for-3.11-2
for you to fetch changes up to c545b59515cca4ccaf920e12582a43836cddaa2b:
OMAPDSS: panels: add Kconfig comment (2013-06-17 14:33:21 +0300)
----------------------------------------------------------------
OMAP display subsystem changes for 3.11 (part 2/2)
This is the second part of OMAP DSS changes for 3.11. This part contains the
new DSS device model support.
The current OMAP panel drivers use a custom DSS bus, and there's a hard limit
of one external display block per video pipeline. In the new DSS device model
the devices/drivers are made according to the control bus of the display block,
usually platform, i2c or spi. The display blocks can also be chained so that we
can have separate drivers for setups with both external encoder and panel.
To allow the current board files, which use the old style panels, to function,
the old display drivers are left in their current state, and new ones are added
to drivers/video/omap2/displays-new/. When the board files have been converted
to use the new style panels, we can remove the old code. This is planned to
happen in v3.12.
Having to support two very different DSS device models makes the driver
somewhat confusing in some parts, and prevents us from properly cleaning up
some other parts. These cleanups will be done when the old code is removed.
The new device model is designed with CDF (Common Display Framework) in mind.
While CDF is still under work, the new DSS device model should be much more
similar to CDF's model than the old device model, which should make the
eventual conversion to CDF much easier.
----------------------------------------------------------------
Tomi Valkeinen (23):
OMAPDSS: public omapdss_register_output()
OMAPDSS: modify get/find functions to go through the device chain
OMAPDSS: add OMAP_DISPLAY_TYPE_DVI
drm/omap: DVI connector fix
OMAPDSS: DPI: Add ops
OMAPDSS: SDI: Add ops
OMAPDSS: DVI: Add ops
OMAPDSS: AnalogTV: Add ops
OMAPDSS: HDMI: Add ops
OMAPDSS: DSI: Add ops
OMAPDSS: Add new TFP410 Encoder driver
OMAPDSS: Add new TPD12S015 Encoder driver
OMAPDSS: Add new DVI Connector driver
OMAPDSS: Add new HDMI Connector driver
OMAPDSS: Add new Analog TV Connector driver
OMAPDSS: Add new simple DPI panel driver
OMAPDSS: Add new DSI Command Mode panel driver
OMAPDSS: Add Sony ACX565AKM panel driver
OMAPDSS: Add LG.Philips LB035Q02 panel driver
OMAPDSS: Add Sharp LS037V7DW01 panel driver
OMAPDSS: Add TPO TD043MTEA1 panel driver
OMAPDSS: Add NEC NL8048HL11 panel driver
OMAPDSS: panels: add Kconfig comment
drivers/gpu/drm/omapdrm/omap_drv.c | 6 +-
drivers/video/omap2/Kconfig | 1 +
drivers/video/omap2/Makefile | 1 +
drivers/video/omap2/displays-new/Kconfig | 73 ++
drivers/video/omap2/displays-new/Makefile | 12 +
.../video/omap2/displays-new/connector-analog-tv.c | 265 ++++
drivers/video/omap2/displays-new/connector-dvi.c | 351 +++++
drivers/video/omap2/displays-new/connector-hdmi.c | 375 ++++++
drivers/video/omap2/displays-new/encoder-tfp410.c | 267 ++++
.../video/omap2/displays-new/encoder-tpd12s015.c | 395 ++++++
drivers/video/omap2/displays-new/panel-dpi.c | 270 ++++
drivers/video/omap2/displays-new/panel-dsi-cm.c | 1336 ++++++++++++++++++++
.../omap2/displays-new/panel-lgphilips-lb035q02.c | 358 ++++++
.../omap2/displays-new/panel-nec-nl8048hl11.c | 394 ++++++
.../omap2/displays-new/panel-sharp-ls037v7dw01.c | 324 +++++
.../omap2/displays-new/panel-sony-acx565akm.c | 865 +++++++++++++
.../omap2/displays-new/panel-tpo-td043mtea1.c | 646 ++++++++++
drivers/video/omap2/displays/Kconfig | 2 +-
drivers/video/omap2/dss/apply.c | 14 +-
drivers/video/omap2/dss/display.c | 1 +
drivers/video/omap2/dss/dpi.c | 74 +-
drivers/video/omap2/dss/dsi.c | 97 +-
drivers/video/omap2/dss/dss.h | 4 -
drivers/video/omap2/dss/hdmi.c | 238 +++-
drivers/video/omap2/dss/output.c | 15 +-
drivers/video/omap2/dss/rfbi.c | 4 +-
drivers/video/omap2/dss/sdi.c | 82 +-
drivers/video/omap2/dss/venc.c | 76 +-
include/video/omap-panel-data.h | 209 +++
include/video/omapdss.h | 192 ++-
30 files changed, 6911 insertions(+), 36 deletions(-)
create mode 100644 drivers/video/omap2/displays-new/Kconfig
create mode 100644 drivers/video/omap2/displays-new/Makefile
create mode 100644 drivers/video/omap2/displays-new/connector-analog-tv.c
create mode 100644 drivers/video/omap2/displays-new/connector-dvi.c
create mode 100644 drivers/video/omap2/displays-new/connector-hdmi.c
create mode 100644 drivers/video/omap2/displays-new/encoder-tfp410.c
create mode 100644 drivers/video/omap2/displays-new/encoder-tpd12s015.c
create mode 100644 drivers/video/omap2/displays-new/panel-dpi.c
create mode 100644 drivers/video/omap2/displays-new/panel-dsi-cm.c
create mode 100644 drivers/video/omap2/displays-new/panel-lgphilips-lb035q02.c
create mode 100644 drivers/video/omap2/displays-new/panel-nec-nl8048hl11.c
create mode 100644 drivers/video/omap2/displays-new/panel-sharp-ls037v7dw01.c
create mode 100644 drivers/video/omap2/displays-new/panel-sony-acx565akm.c
create mode 100644 drivers/video/omap2/displays-new/panel-tpo-td043mtea1.c
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Russell King - ARM Linux @ 2013-06-17 15:42 UTC (permalink / raw)
To: Inki Dae
Cc: Maarten Lankhorst, linux-fbdev, Kyungmin Park, DRI mailing list,
Rob Clark, myungjoo.ham, YoungJun Cho, Daniel Vetter,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <CAAQKjZO_t_kZkU46bUPTpoJs_oE1KkEqS2OTrTYjjJYZzBf+XA@mail.gmail.com>
On Tue, Jun 18, 2013 at 12:03:31AM +0900, Inki Dae wrote:
> 2013/6/17 Russell King - ARM Linux <linux@arm.linux.org.uk>
> Exactly right. But that is not definitely my point. Could you please see
> the below simple example?:
> (Presume that CPU and DMA share a buffer and the buffer is mapped with user
> space as cachable)
>
> handle1 = drm_gem_fd_to_handle(a dmabuf fd); ----> 1
> ...
> va1 = drm_gem_mmap(handle1);
> va2 = drm_gem_mmap(handle2);
> va3 = malloc(size);
> ...
>
> while (conditions) {
> memcpy(va1, some data, size);
Nooooooooooooooooooooooooooooooooooooooooooooo!
Well, the first thing to say here is that under the requirements of the
DMA API, the above is immediately invalid, because you're writing to a
buffer which under the terms of the DMA API is currently owned by the
DMA agent, *not* by the CPU. You're supposed to call dma_sync_sg_for_cpu()
before you do that - but how is userspace supposed to know that requirement?
Why should userspace even _have_ to know these requirements of the DMA
API?
It's also entirely possible that drm_gem_fd_to_handle() (which indirectly
causes dma_map_sg() on the buffers scatterlist) followed by mmap'ing it
into userspace is a bug too, as it has the potential to touch caches or
stuff in ways that maybe the DMA or IOMMU may not expect - but I'm not
going to make too big a deal about that, because I don't think we have
anything that picky.
However, the first point above is the most important one, and exposing
the quirks of the DMA API to userland is certainly not a nice thing to be
doing. This needs to be fixed - we can't go and enforce an API which is
deeply embedded within the kernel all the way out to userland.
What we need is something along the lines of:
(a) dma_buf_map_attachment() _not_ to map the scatterlist for DMA.
or
(b) drm_gem_prime_import() not to call dma_buf_map_attachment() at all.
and for the scatterlist to be mapped for DMA at the point where the DMA
operation is initiated, and unmapped at the point where the DMA operation
is complete.
So no, the problem is not that we need more APIs and code - we need the
existing kernel API fixed so that we don't go exposing userspace to the
requirements of the DMA API. Unless we do that, we're going to end
up with a huge world of pain, where kernel architecture people need to
audit every damned DRM userspace implementation that happens to be run
on their platform, and that's not something arch people really can
afford to do.
Basically, I think the dma_buf stuff needs to be rewritten with the
requirements of the DMA API in the forefront of whosever mind is doing
the rewriting.
Note: the existing stuff does have the nice side effect of being able
to pass buffers which do not have a struct page * associated with them
through the dma_buf API - I think we can still preserve that by having
dma_buf provide a couple of new APIs to do the SG list map/sync/unmap,
but in any case we need to fix the existing API so that:
dma_buf_map_attachment() becomes dma_buf_get_sg()
dma_buf_unmap_attachment() becomes dma_buf_put_sg()
both getting rid of the DMA direction argument, and then we have four
new dma_buf calls:
dma_buf_map_sg()
dma_buf_unmap_sg()
dma_buf_sync_sg_for_cpu()
dma_buf_sync_sg_for_device()
which do the actual sg map/unmap via the DMA API *at the appropriate
time for DMA*.
So, the summary of this is - at the moment, I regard DRM Prime and dmabuf
to be utterly broken in design for architectures such as ARM where the
requirements of the DMA API have to be followed if you're going to have
a happy life.
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Russell King - ARM Linux @ 2013-06-17 16:01 UTC (permalink / raw)
To: Inki Dae
Cc: linux-fbdev, Kyungmin Park, DRI mailing list, Rob Clark,
myungjoo.ham, YoungJun Cho, Daniel Vetter, Maarten Lankhorst,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <20130617154237.GJ2718@n2100.arm.linux.org.uk>
On Mon, Jun 17, 2013 at 04:42:37PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 18, 2013 at 12:03:31AM +0900, Inki Dae wrote:
> > 2013/6/17 Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Exactly right. But that is not definitely my point. Could you please see
> > the below simple example?:
> > (Presume that CPU and DMA share a buffer and the buffer is mapped with user
> > space as cachable)
> >
> > handle1 = drm_gem_fd_to_handle(a dmabuf fd); ----> 1
> > ...
> > va1 = drm_gem_mmap(handle1);
> > va2 = drm_gem_mmap(handle2);
> > va3 = malloc(size);
> > ...
> >
> > while (conditions) {
> > memcpy(va1, some data, size);
>
> Nooooooooooooooooooooooooooooooooooooooooooooo!
>
> Well, the first thing to say here is that under the requirements of the
> DMA API, the above is immediately invalid, because you're writing to a
> buffer which under the terms of the DMA API is currently owned by the
> DMA agent, *not* by the CPU. You're supposed to call dma_sync_sg_for_cpu()
> before you do that - but how is userspace supposed to know that requirement?
> Why should userspace even _have_ to know these requirements of the DMA
> API?
>
> It's also entirely possible that drm_gem_fd_to_handle() (which indirectly
> causes dma_map_sg() on the buffers scatterlist) followed by mmap'ing it
> into userspace is a bug too, as it has the potential to touch caches or
> stuff in ways that maybe the DMA or IOMMU may not expect - but I'm not
> going to make too big a deal about that, because I don't think we have
> anything that picky.
>
> However, the first point above is the most important one, and exposing
> the quirks of the DMA API to userland is certainly not a nice thing to be
> doing. This needs to be fixed - we can't go and enforce an API which is
> deeply embedded within the kernel all the way out to userland.
>
> What we need is something along the lines of:
> (a) dma_buf_map_attachment() _not_ to map the scatterlist for DMA.
> or
> (b) drm_gem_prime_import() not to call dma_buf_map_attachment() at all.
>
> and for the scatterlist to be mapped for DMA at the point where the DMA
> operation is initiated, and unmapped at the point where the DMA operation
> is complete.
>
> So no, the problem is not that we need more APIs and code - we need the
> existing kernel API fixed so that we don't go exposing userspace to the
> requirements of the DMA API. Unless we do that, we're going to end
> up with a huge world of pain, where kernel architecture people need to
> audit every damned DRM userspace implementation that happens to be run
> on their platform, and that's not something arch people really can
> afford to do.
>
> Basically, I think the dma_buf stuff needs to be rewritten with the
> requirements of the DMA API in the forefront of whosever mind is doing
> the rewriting.
>
> Note: the existing stuff does have the nice side effect of being able
> to pass buffers which do not have a struct page * associated with them
> through the dma_buf API - I think we can still preserve that by having
> dma_buf provide a couple of new APIs to do the SG list map/sync/unmap,
> but in any case we need to fix the existing API so that:
>
> dma_buf_map_attachment() becomes dma_buf_get_sg()
> dma_buf_unmap_attachment() becomes dma_buf_put_sg()
>
> both getting rid of the DMA direction argument, and then we have four
> new dma_buf calls:
>
> dma_buf_map_sg()
> dma_buf_unmap_sg()
> dma_buf_sync_sg_for_cpu()
> dma_buf_sync_sg_for_device()
>
> which do the actual sg map/unmap via the DMA API *at the appropriate
> time for DMA*.
>
> So, the summary of this is - at the moment, I regard DRM Prime and dmabuf
> to be utterly broken in design for architectures such as ARM where the
> requirements of the DMA API have to be followed if you're going to have
> a happy life.
An addendum to the above:
I'll also point out that the whole thing of having random buffers mapped
into userspace, and doing DMA on them is _highly_ architecture specific.
I'm told that PARISC is one architecture where this does not work (because
DMA needs to have some kind of congruence factor programmed into it which
can be different between kernel and userspace mappings of the same
physical mappings.
I ran into this when trying to come up with a cross-arch DMA-API mmap API,
and PARISC ended up killing the idea off (the remains of that attempt is
the dma_mmap_* stuff in ARM's asm/dma-mapping.h.) However, this was for
the DMA coherent stuff, not the streaming API, which is what _this_
DMA prime/dma_buf stuff is about.
What I'm saying is think _very_ _carefully_ about userspace having
interleaved access to random DMA buffers. Arguably, DRM should _not_
allow this.
^ permalink raw reply
* Re: [PATCH] video: of_display_timing.h: Include <video/display_timing.h>
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-06-17 16:09 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371476589-4660-1-git-send-email-fabio.estevam@freescale.com>
On 10:43 Mon 17 Jun , Fabio Estevam wrote:
> Commit ffa3fd21de ("videomode: implement public of_get_display_timing()") causes
> the following build warning:
>
> include/video/of_display_timing.h:18:10: warning: 'struct display_timing' declared inside parameter list [enabled by default]
> include/video/of_display_timing.h:18:10: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
>
> As 'struct display_timing' is defined at <video/display_timing.h>, let's include
> this header to avoid the warning.
for 3.10 or 3.11?
Best Regards,
J.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> include/video/of_display_timing.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/video/of_display_timing.h b/include/video/of_display_timing.h
> index 6562ad9..a136f58 100644
> --- a/include/video/of_display_timing.h
> +++ b/include/video/of_display_timing.h
> @@ -8,6 +8,7 @@
>
> #ifndef __LINUX_OF_DISPLAY_TIMING_H
> #define __LINUX_OF_DISPLAY_TIMING_H
> +#include <video/display_timing.h>
>
> struct device_node;
> struct display_timings;
> --
> 1.8.1.2
>
>
^ permalink raw reply
* Re: [PATCH] video: of_display_timing.h: Include <video/display_timing.h>
From: Fabio Estevam @ 2013-06-17 16:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371476589-4660-1-git-send-email-fabio.estevam@freescale.com>
On Mon, Jun 17, 2013 at 1:09 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> On 10:43 Mon 17 Jun , Fabio Estevam wrote:
>> Commit ffa3fd21de ("videomode: implement public of_get_display_timing()") causes
>> the following build warning:
>>
>> include/video/of_display_timing.h:18:10: warning: 'struct display_timing' declared inside parameter list [enabled by default]
>> include/video/of_display_timing.h:18:10: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
>>
>> As 'struct display_timing' is defined at <video/display_timing.h>, let's include
>> this header to avoid the warning.
> for 3.10 or 3.11?
This is 3.11 material.
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Russell King - ARM Linux @ 2013-06-17 18:21 UTC (permalink / raw)
To: Inki Dae
Cc: Maarten Lankhorst, linux-fbdev, Kyungmin Park, DRI mailing list,
Rob Clark, myungjoo.ham, YoungJun Cho, Daniel Vetter,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <CAAQKjZOokFKN85pygVnm7ShSa+O0ZzwxvQ0rFssgNLp+RO5pGg@mail.gmail.com>
On Tue, Jun 18, 2013 at 02:19:04AM +0900, Inki Dae wrote:
> It seems like that all pages of the scatterlist should be mapped with
> DMA every time DMA operation is started (or drm_xxx_set_src_dma_buffer
> function call), and the pages should be unmapped from DMA again every
> time DMA operation is completed: internally, including each cache
> operation.
Correct.
> Isn't that big overhead?
Yes, if you _have_ to do a cache operation to ensure that the DMA agent
can see the data the CPU has written.
> And If there is no problem, we should accept such overhead?
If there is no problem then why are we discussing this and why do we need
this API extension? :)
> Actually, drm_gem_fd_to_handle() includes to map a
> given dmabuf with iommu table (just logical data) of the DMA. And then, the
> device address (or iova) already mapped will be set to buffer register of
> the DMA with drm_xxx_set_src/dst_dma_buffer(handle1, ...) call.
Consider this with a PIPT cache:
dma_map_sg() - at this point, the kernel addresses of these
buffers are cleaned and invalidated for the DMA
userspace writes to the buffer, the data sits in the CPU cache
Because the cache is PIPT, this data becomes visible to the
kernel as well.
DMA is started, and it writes to the buffer
Now, at this point, which is the correct data? The data physically in the
RAM which the DMA has written, or the data in the CPU cache. It may
the answer is - they both are, and the loss of either can be a potential
data corruption issue - there is no way to tell which data should be
kept but the system is in an inconsistent state and _one_ of them will
have to be discarded.
dma_unmap_sg() - at this point, the kernel addresses of the
buffers are _invalidated_ and any data in those
cache lines is discarded
Which also means that the data in userspace will also be discarded with
PIPT caches.
This is precisely why we have buffer rules associated with the DMA API,
which are these:
dma_map_sg()
- the buffer transfers ownership from the CPU to the DMA agent.
- the CPU may not alter the buffer in any way.
while (cpu_wants_access) {
dma_sync_sg_for_cpu()
- the buffer transfers ownership from the DMA to the CPU.
- the DMA may not alter the buffer in any way.
dma_sync_sg_for_device()
- the buffer transfers ownership from the CPU to the DMA
- the CPU may not alter the buffer in any way.
}
dma_unmap_sg()
- the buffer transfers ownership from the DMA to the CPU.
- the DMA may not alter the buffer in any way.
Any violation of that is likely to lead to data corruption. Now, some
may say that the DMA API is only about the kernel mapping. Yes it is,
because it takes no regard what so ever about what happens with the
userspace mappings. This is absolutely true when you have VIVT or
aliasing VIPT caches.
Now consider that with a PIPT cache, or a non-aliasing VIPT cache (which
is exactly the same behaviourally from this aspect) any modification of
a userspace mapping is precisely the same as modifying the kernel space
mapping - and what you find is that the above rules end up _inherently_
applying to the userspace mappings as well.
In other words, userspace applications which have mapped the buffers
must _also_ respect these rules.
And there's no way in hell that I'd ever trust userspace to get this
anywhere near right, and I *really* do not like these kinds of internal
kernel API rules being exposed to userspace.
And so the idea that userspace should be allowed to setup DMA transfers
via "set source buffer", "set destination buffer" calls into an API is
just plain rediculous. No, userspace should be allowed to ask for
"please copy from buffer X to buffer Y using this transform".
Also remember that drm_xxx_set_src/dst_dma_buffer() would have to
deal with the DRM object IDs for the buffer, and not the actual buffer
information themselves - that is kept within the kernel, so the kernel
knows what's happening.
^ permalink raw reply
* (no subject)
From: AFG GTBANK LOAN @ 2013-06-17 19:28 UTC (permalink / raw)
To: linux-fbdev
Loan Syndicacion
Am AFG Guaranty Trust Bank, zu strukturieren wir Kreditlinien treffen Sie
unsere
Kunden spezifischen geschäftlichen Anforderungen und einen deutlichen
Mehrwert für unsere
Kunden Unternehmen.
eine Division der AFG Finance und Private Bank plc.
Wenn Sie erwägen, eine große Akquisition oder ein Großprojekt sind, können
Sie
brauchen eine erhebliche Menge an Kredit. AFG Guaranty Trust Bank setzen
können
zusammen das Syndikat, das die gesamte Kredit schnürt für
Sie.
Als Bank mit internationaler Reichweite, sind wir gekommen, um Darlehen zu
identifizieren
Syndizierungen als Teil unseres Kerngeschäfts und durch spitzte diese Zeile
aggressiv sind wir an einem Punkt, wo wir kommen, um als erkannt haben
Hauptakteur in diesem Bereich.
öffnen Sie ein Girokonto heute mit einem Minimum Bankguthaben von 500 £ und
Getup zu £ 10.000 als Darlehen und auch den Hauch einer Chance und gewann
die Sterne
Preis von £ 500.000 in die sparen und gewinnen promo in may.aply jetzt.
mit dem Folowing Informationen über Rechtsanwalt steven lee das Konto
Offizier.
FULL NAME;
Wohnadresse;
E-MAIL-ADRESSE;
Telefonnummer;
Nächsten KINS;
MUTTER MAIDEN NAME;
Familienstand;
BÜROADRESSE;
ALTERNATIVE Telefonnummer;
TO @ yahoo.com bar.stevenlee
NOTE; ALLE Darlehen sind für 10JAHRE RATE VALID
ANGEBOT ENDET BALD SO JETZT HURRY
^ permalink raw reply
* Re: [PATCH] build some drivers only when compile-testing
From: Jiri Slaby @ 2013-06-17 20:05 UTC (permalink / raw)
To: Jeff Mahoney, Greg Kroah-Hartman
Cc: jirislaby, linux-kernel, Andrew Morton, Linus Torvalds,
Alexander Shishkin, linux-usb, Florian Tobias Schandinat,
linux-geode, linux-fbdev, Richard Cochran, netdev, Ben Hutchings,
Keller, Jacob E, Michal Marek, tomi.valkeinen
In-Reply-To: <519D8876.9050405@suse.com>
[-- Attachment #1: Type: text/plain, Size: 3819 bytes --]
On 05/23/2013 05:09 AM, Jeff Mahoney wrote:
> On 5/22/13 10:23 PM, Greg Kroah-Hartman wrote:
>> On Wed, May 22, 2013 at 11:18:46AM +0200, Jiri Slaby wrote:
>>> Some drivers can be built on more platforms than they run on. This
>>> causes users and distributors packaging burden when they have to
>>> manually deselect some drivers from their allmodconfigs. Or sometimes
>>> it is even impossible to disable the drivers without patching the
>>> kernel.
>>>
>>> Introduce a new config option COMPILE_TEST and make all those drivers
>>> to depend on the platform they run on, or on the COMPILE_TEST option.
>>> Now, when users/distributors choose COMPILE_TEST=n they will not have
>>> the drivers in their allmodconfig setups, but developers still can
>>> compile-test them with COMPILE_TEST=y.
>>
>> I understand the urge, and it's getting hard for distros to handle these
>> drivers that just don't work on other architectures, but it's really
>> valuable to ensure that they build properly, for those of us that don't
>> have many/any cross compilers set up.
But this is exactly what COMPILE_TEST will give us when set to "y", or
am I missing something?
>>> Now the drivers where we use this new option:
>>> * PTP_1588_CLOCK_PCH: The PCH EG20T is only compatible with Intel Atom
>>> processors so it should depend on x86.
>>> * FB_GEODE: Geode is 32-bit only so only enable it for X86_32.
>>> * USB_CHIPIDEA_IMX: The OF_DEVICE dependency will be met on powerpc
>>> systems -- which do not actually support the hardware via that
>>> method.
>>
>> This seems ripe to start to get really messy, really quickly. Shouldn't
>> "default configs" handle if this "should" be enabled for a platform or
>> not, and let the rest of us just build them with no problems?
>
> If every time a new Kconfig option is added, corresponding default
> config updates come with it, sure. I just don't see that happening,
> especially when it can be done much more clearly in the Kconfig while
> the developer is writing the driver.
>
>> What problems is this causing you? Are you running out of space in
>> kernel packages with drivers that will never be actually used?
>
> Wasted build resources. Wasted disk space on /every/ system the kernel
> package is installed on. We're all trying to pare down the kernel
> packages to eliminate wasted space and doing it manually means a bunch
> of research, sometimes with incorrect assumptions about the results,
> needs to be done by someone not usually associated with that code. That
> research gets repeated by people maintaining kernel packages for pretty
> much every distro.
I second all the above.
>>> +config COMPILE_TEST
>>> + bool "Compile also drivers which will not load" if EXPERT
>>
>> EXPERT is getting to be the "let's hide it here" option, isn't it...
>>
>> I don't know, if no one else strongly objects, I can be convinced that
>> this is needed, but so far, I don't see why it really is, or what this
>> is going to help with.
>
> I'm not convinced adding a || COMPILE_TEST option to every driver that
> may be arch specific is the best way to go either. Perhaps adding a new
> Kconfig verb called "archdepends on" or something that will evaluate as
> true if COMPILE_TEST is enabled but will evaluate the conditional if
> not. *waves hands*
Sam Ravnborg (the kconfig ex-maintainer) once wrote that he doesn't want
to extend the kconfig language for this purpose (which I support). That
a config option is fine and sufficient in this case [1]. Except he
called the config option "SHOW_ALL_DRIVERS". Adding the current
maintainer to CCs ;).
[1] http://comments.gmane.org/gmane.linux.kbuild.devel/9829
The last point I inclined to the Greg's argument to remove the EXPERT
dependency.
So currently I have what is attached... Comments?
thanks,
--
js
suse labs
[-- Attachment #2: 0001-build-some-drivers-only-when-compile-testing.patch --]
[-- Type: text/x-patch, Size: 5332 bytes --]
From e818e90b4f901c8d949d08bd05735203c5e88530 Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
Date: Wed, 22 May 2013 10:56:24 +0200
Subject: [PATCH v2] build some drivers only when compile-testing
Some drivers can be built on more platforms than they run on. This is
a burden for users and distributors who package a kernel. They have to
manually deselect some (for them useless) drivers when updating their
configs via oldconfig. And yet, sometimes it is even impossible to
disable the drivers without patching the kernel.
Introduce a new config option COMPILE_TEST and make all those drivers
to depend on the platform they run on, or on the COMPILE_TEST option.
Now, when users/distributors choose COMPILE_TEST=n they will not have
the drivers in their allmodconfig setups, but developers still can
compile-test them with COMPILE_TEST=y.
Now the drivers where we use this new option:
* PTP_1588_CLOCK_PCH: The PCH EG20T is only compatible with Intel Atom
processors so it should depend on x86.
* FB_GEODE: Geode is 32-bit only so only enable it for X86_32.
* USB_CHIPIDEA_IMX: The OF_DEVICE dependency will be met on powerpc
systems -- which do not actually support the hardware via that
method.
* INTEL_MID_PTI: It is specific to the Penwell type of Intel Atom
device.
[v2]
* remove EXPERT dependency
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: linux-geode@lists.infradead.org
Cc: linux-fbdev@vger.kernel.org
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: netdev@vger.kernel.org
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: "Keller, Jacob E" <jacob.e.keller@intel.com>
---
drivers/misc/Kconfig | 2 +-
drivers/ptp/Kconfig | 1 +
drivers/usb/chipidea/Kconfig | 6 ++++++
drivers/usb/chipidea/Makefile | 4 +---
drivers/video/geode/Kconfig | 2 +-
init/Kconfig | 14 ++++++++++++++
6 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 80889d5..8dacd4c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -135,7 +135,7 @@ config PHANTOM
config INTEL_MID_PTI
tristate "Parallel Trace Interface for MIPI P1149.7 cJTAG standard"
- depends on PCI && TTY
+ depends on PCI && TTY && (X86_INTEL_MID || COMPILE_TEST)
default n
help
The PTI (Parallel Trace Interface) driver directs
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 1ea6f1d..5be73ba 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -72,6 +72,7 @@ config DP83640_PHY
config PTP_1588_CLOCK_PCH
tristate "Intel PCH EG20T as PTP clock"
+ depends on X86 || COMPILE_TEST
select PTP_1588_CLOCK
help
This driver adds support for using the PCH EG20T as a PTP
diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index b2df442..3491d86 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -31,4 +31,10 @@ config USB_CHIPIDEA_DEBUG
help
Say Y here to enable debugging output of the ChipIdea driver.
+config USB_CHIPIDEA_IMX
+ bool "ChipIdea IMX support"
+ depends on OF_DEVICE && (ARM || COMPILE_TEST)
+ help
+ This option enables ChipIdea support on IMX.
+
endif
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 4113feb..76d66ff 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -16,6 +16,4 @@ ifneq ($(CONFIG_PCI),)
obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_pci.o
endif
-ifneq ($(CONFIG_OF),)
- obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o usbmisc_imx.o
-endif
+obj-$(CONFIG_USB_CHIPIDEA_IMX) += ci13xxx_imx.o usbmisc_imx.o
diff --git a/drivers/video/geode/Kconfig b/drivers/video/geode/Kconfig
index 21e351a..1e85552 100644
--- a/drivers/video/geode/Kconfig
+++ b/drivers/video/geode/Kconfig
@@ -3,7 +3,7 @@
#
config FB_GEODE
bool "AMD Geode family framebuffer support"
- depends on FB && PCI && X86
+ depends on FB && PCI && (X86_32 || (X86 && COMPILE_TEST))
---help---
Say 'Y' here to allow you to select framebuffer drivers for
the AMD Geode family of processors.
diff --git a/init/Kconfig b/init/Kconfig
index fd0e436..e1854b2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -53,6 +53,20 @@ config CROSS_COMPILE
need to set this unless you want the configured kernel build
directory to select the cross-compiler automatically.
+config COMPILE_TEST
+ bool "Compile also drivers which will not load"
+ default n
+ help
+ Some drivers can be compiled on a different platform than they are
+ intended to be run on. Despite they cannot be loaded there (or even
+ when they load they cannot be used due to missing HW support),
+ developers still, opposing to distributors, might want to build such
+ drivers to compile-test them.
+
+ If you are a developer and want to build everything available, say Y
+ here. If you are a user/distributor, say N here to exclude useless
+ drivers to be distributed.
+
config LOCALVERSION
string "Local version - append to kernel release"
help
--
1.8.3
^ permalink raw reply related
* Re: [PATCH v4 0/7] xilinxfb changes
From: Arnd Bergmann @ 2013-06-17 21:07 UTC (permalink / raw)
To: monstr-pSz03upnqPeHXe+LvDLADg
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Timur Tabi,
Rob Herring, Michal Simek, Tomi Valkeinen, Grant Likely
In-Reply-To: <51BED093.1040207-pSz03upnqPeHXe+LvDLADg@public.gmane.org>
On Monday 17 June 2013, Michal Simek wrote:
> On 06/17/2013 10:56 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 07:23 Mon 17 Jun , Michal Simek wrote:
> >> On 06/06/2013 06:23 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>> On 12:13 Mon 03 Jun , Michal Simek wrote:
> >>>> Hi,
> >>>>
> >>>
> >>> Arnd can you take look on it again please
> >>>
> >>> I'll take a look on it next week
> >>
> >> Any update on this?
> > look ok but I want the Ack from Arnd
Sorry for the delay, everything looks good to me.
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox