- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH] dma_heap: use sg_table.orig_nents in sg_table release flow

by Robin Murphy

On 2021-11-25 12:46, guangming.cao(a)mediatek.com wrote: > From: Guangming <Guangming.Cao(a)mediatek.com> > > Use (sg_table.orig_nents) rather than (sg_table.nents) to traverse > sg_table to free sg_table. > Use (sg_table.nents) maybe will casuse some pages can't be freed. ...and this sort of bug is precisely why we have the for_each_sgtable_sg() helper ;) Robin. > Signed-off-by: Guangming <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/heaps/system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..ce10d4eb674c 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) > int i; > > table = &buffer->sg_table; > - for_each_sg(table->sgl, sg, table->nents, i) { > + for_each_sg(table->sgl, sg, table->orig_nents, i) { > struct page *page = sg_page(sg); > > __free_pages(page, compound_order(page)); >

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3] i2c: tegra: Add the ACPI support

by Dmitry Osipenko

25.11.2021 12:07, Akhil R пишет: > Add support for the ACPI based device registration so that the driver > can be also enabled through ACPI table. > > This does not include the ACPI support for Tegra VI and DVC I2C. > > Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 12 deletions(-) > > v3 changes > * removed acpi_has_method check. > * moved dev_err_probe to init_reset function to make it consistent with > init_clocks. > * Updates in commit message as suggested. > > v2 - https://lkml.org/lkml/2021/11/23/82 > v1 - https://lkml.org/lkml/2021/11/19/393 Akhil, the patch looks almost good, thank you. Please see one minor question below. > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index c883044..b889eb3 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -6,6 +6,7 @@ > * Author: Colin Cross <ccross(a)android.com> > */ > > +#include <linux/acpi.h> > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > @@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > { > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > + acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); ... > +static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev) > +{ > + if (has_acpi_companion(i2c_dev->dev)) > + return 0; Can we use ACPI_HANDLE() everywhere instead of has_acpi_companion()? For consistency. I guess that's what Andy was asking about in v1?

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by Dmitry Osipenko

24.11.2021 19:40, Akhil R пишет: >> 24.11.2021 10:18, Akhil R пишет: >>>> *i2c_dev) >>>>> i2c_dev->is_vi = true; } >>>> How are you going to differentiate the VI I2C from a non-VI? This >>>> doesn't look right. >>> This patch adds the ACPI support to only non-VI I2C. The device_ids in >>> match table are added accordingly. I suppose, of_device_is_compatible >>> always returns false as there is no device tree. >>> Agree with the other comments. >> >> Will the VI I2C have a different ACPI ID or how it's going to work? > As there is a different compatible for VI I2C in device tree, I suppose the ACPI > would have a different ID as well. I think the logic would also need an update > if to have VI I2C using the ACPI. But that wasn't actually considered in this patch. Thanks, you could reflected it in the commit message.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by Dmitry Osipenko

24.11.2021 10:18, Akhil R пишет: >> *i2c_dev) >>> i2c_dev->is_vi = true; >>> } >> How are you going to differentiate the VI I2C from a non-VI? This doesn't look >> right. > This patch adds the ACPI support to only non-VI I2C. The device_ids in match table > are added accordingly. I suppose, of_device_is_compatible always returns false as > there is no device tree. > Agree with the other comments. Will the VI I2C have a different ACPI ID or how it's going to work?

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by kernel test robot

Hi Akhil, Thank you for the patch! Yet something to improve: [auto build test ERROR on tegra/for-next] [also build test ERROR on v5.16-rc2 next-20211123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support… base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next config: riscv-buildonly-randconfig-r005-20211123 (https://download.01.org/0day-ci/archive/20211124/202111240017.bYyZ7knz-lkp@…) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 49e3838145dff1ec91c2e67a2cb562775c8d2a08) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25… git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636 git checkout dec174be801f41a9e42f4381c59c2357c25e40fb # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp(a)intel.com> All errors (new ones prefixed by >>): In file included from drivers/i2c/busses/i2c-tegra.c:13: In file included from include/linux/dmaengine.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) ^ In file included from drivers/i2c/busses/i2c-tegra.c:13: In file included from include/linux/dmaengine.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) ^ In file included from drivers/i2c/busses/i2c-tegra.c:13: In file included from include/linux/dmaengine.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; ~~~~~~~~~~ ^ >> drivers/i2c/busses/i2c-tegra.c:623:16: error: implicit declaration of function 'acpi_has_method' [-Werror,-Wimplicit-function-declaration] if (handle && acpi_has_method(handle, "_RST")) ^ drivers/i2c/busses/i2c-tegra.c:623:16: note: did you mean 'acpi_has_watchdog'? include/linux/acpi.h:1321:20: note: 'acpi_has_watchdog' declared here static inline bool acpi_has_watchdog(void) { return false; } ^ 7 warnings and 1 error generated. vim +/acpi_has_method +623 drivers/i2c/busses/i2c-tegra.c 608 609 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) 610 { 611 u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; 612 acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); 613 int err; 614 615 /* 616 * The reset shouldn't ever fail in practice. The failure will be a 617 * sign of a severe problem that needs to be resolved. Still we don't 618 * want to fail the initialization completely because this may break 619 * kernel boot up since voltage regulators use I2C. Hence, we will 620 * emit a noisy warning on error, which won't stay unnoticed and 621 * won't hose machine entirely. 622 */ > 623 if (handle && acpi_has_method(handle, "_RST")) 624 err = (acpi_evaluate_object(handle, "_RST", NULL, NULL)); 625 else 626 err = reset_control_reset(i2c_dev->rst); 627 628 WARN_ON_ONCE(err); 629 630 if (i2c_dev->is_dvc) 631 tegra_dvc_init(i2c_dev); 632 633 val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN | 634 FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2); 635 636 if (i2c_dev->hw->has_multi_master_mode) 637 val |= I2C_CNFG_MULTI_MASTER_MODE; 638 639 i2c_writel(i2c_dev, val, I2C_CNFG); 640 i2c_writel(i2c_dev, 0, I2C_INT_MASK); 641 642 if (i2c_dev->is_vi) 643 tegra_i2c_vi_init(i2c_dev); 644 645 switch (i2c_dev->bus_clk_rate) { 646 case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ: 647 default: 648 tlow = i2c_dev->hw->tlow_fast_fastplus_mode; 649 thigh = i2c_dev->hw->thigh_fast_fastplus_mode; 650 tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode; 651 652 if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ) 653 non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode; 654 else 655 non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode; 656 break; 657 658 case 0 ... I2C_MAX_STANDARD_MODE_FREQ: 659 tlow = i2c_dev->hw->tlow_std_mode; 660 thigh = i2c_dev->hw->thigh_std_mode; 661 tsu_thd = i2c_dev->hw->setup_hold_time_std_mode; 662 non_hs_mode = i2c_dev->hw->clk_divisor_std_mode; 663 break; 664 } 665 666 /* make sure clock divisor programmed correctly */ 667 clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE, 668 i2c_dev->hw->clk_divisor_hs_mode) | 669 FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode); 670 i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR); 671 672 if (i2c_dev->hw->has_interface_timing_reg) { 673 val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) | 674 FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow); 675 i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0); 676 } 677 678 /* 679 * Configure setup and hold times only when tsu_thd is non-zero. 680 * Otherwise, preserve the chip default values. 681 */ 682 if (i2c_dev->hw->has_interface_timing_reg && tsu_thd) 683 i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1); 684 685 clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1); 686 687 err = clk_set_rate(i2c_dev->div_clk, 688 i2c_dev->bus_clk_rate * clk_multiplier); 689 if (err) { 690 dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err); 691 return err; 692 } 693 694 if (!i2c_dev->is_dvc && !i2c_dev->is_vi) { 695 u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG); 696 697 sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL; 698 i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG); 699 i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1); 700 i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2); 701 } 702 703 err = tegra_i2c_flush_fifos(i2c_dev); 704 if (err) 705 return err; 706 707 if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg) 708 i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE); 709 710 err = tegra_i2c_wait_for_config_load(i2c_dev); 711 if (err) 712 return err; 713 714 return 0; 715 } 716 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by Dmitry Osipenko

23.11.2021 10:15, Akhil R пишет: > Add support for ACPI based device registration so that the driver > can be also enabled through ACPI table. > > Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index c883044..8e47889 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -6,6 +6,7 @@ > * Author: Colin Cross <ccross(a)android.com> > */ > > +#include <linux/acpi.h> > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > @@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > { > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > + acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); > int err; > > /* > @@ -618,7 +620,11 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > * emit a noisy warning on error, which won't stay unnoticed and > * won't hose machine entirely. > */ > - err = reset_control_reset(i2c_dev->rst); > + if (handle && acpi_has_method(handle, "_RST")) Which SoC version doesn't have "_RST" method? If neither, then please remove this check. > + err = (acpi_evaluate_object(handle, "_RST", NULL, NULL)); Please remove parens around acpi_evaluate_object(). Why you added them? > + else > + err = reset_control_reset(i2c_dev->rst); > + > WARN_ON_ONCE(err); > > if (i2c_dev->is_dvc) > @@ -1627,12 +1633,12 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) > bool multi_mode; > int err; > > - err = of_property_read_u32(np, "clock-frequency", > - &i2c_dev->bus_clk_rate); > + err = device_property_read_u32(i2c_dev->dev, "clock-frequency", > + &i2c_dev->bus_clk_rate); > if (err) > i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ; > > - multi_mode = of_property_read_bool(np, "multi-master"); > + multi_mode = device_property_read_bool(i2c_dev->dev, "multi-master"); > i2c_dev->multimaster_mode = multi_mode; > > if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc")) > @@ -1642,10 +1648,25 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) > i2c_dev->is_vi = true; > } How are you going to differentiate the VI I2C from a non-VI? This doesn't look right. > > +static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev) > +{ > + if (has_acpi_companion(i2c_dev->dev)) > + return 0; > + > + i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > + if (IS_ERR(i2c_dev->rst)) > + return PTR_ERR(i2c_dev->rst); > + > + return 0; > +} > + > static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev) > { > int err; > > + if (has_acpi_companion(i2c_dev->dev)) > + return 0; > + > i2c_dev->clocks[i2c_dev->nclocks++].id = "div-clk"; > > if (i2c_dev->hw == &tegra20_i2c_hw || i2c_dev->hw == &tegra30_i2c_hw) > @@ -1720,7 +1741,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) > init_completion(&i2c_dev->msg_complete); > init_completion(&i2c_dev->dma_complete); > > - i2c_dev->hw = of_device_get_match_data(&pdev->dev); > + i2c_dev->hw = device_get_match_data(&pdev->dev); > i2c_dev->cont_id = pdev->id; > i2c_dev->dev = &pdev->dev; > > @@ -1746,15 +1767,13 @@ static int tegra_i2c_probe(struct platform_device *pdev) > if (err) > return err; > > - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > - if (IS_ERR(i2c_dev->rst)) { > - dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), > - "failed to get reset control\n"); > - return PTR_ERR(i2c_dev->rst); > - } > - > tegra_i2c_parse_dt(i2c_dev); > > + err = tegra_i2c_init_reset(i2c_dev); > + if (err) > + return dev_err_probe(i2c_dev->dev, err, > + "failed to get reset control\n"); This is inconsistent with tegra_i2c_init_clocks() which returns err directly and prints error message within the function. Please move the dev_err_probe() into tegra_i2c_init_reset() to make it consistent, like I suggested before. Please don't reply with a new version of the patch to the old thread, always send a new version separately. Otherwise it's more difficult to follow patches. Lastly, each new version of a patch must contain changelog. I don't see changelog here, please add it to v3 after the " ---" separator of the commit message. Thanks! Example: Commit message. Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 12 deletions(-) Changelog: v3: bla-bla-bla v2: bla-bla-bla diff ...

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by kernel test robot

Hi Akhil, Thank you for the patch! Yet something to improve: [auto build test ERROR on tegra/for-next] [also build test ERROR on v5.16-rc2 next-20211123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support… base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next config: m68k-randconfig-r011-20211123 (https://download.01.org/0day-ci/archive/20211123/202111232153.mpoejDRV-lkp@…) compiler: m68k-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25… git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636 git checkout dec174be801f41a9e42f4381c59c2357c25e40fb # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp(a)intel.com> All errors (new ones prefixed by >>): drivers/i2c/busses/i2c-tegra.c: In function 'tegra_i2c_init': >> drivers/i2c/busses/i2c-tegra.c:623:23: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Werror=implicit-function-declaration] 623 | if (handle && acpi_has_method(handle, "_RST")) | ^~~~~~~~~~~~~~~ | acpi_has_watchdog cc1: some warnings being treated as errors vim +623 drivers/i2c/busses/i2c-tegra.c 608 609 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) 610 { 611 u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; 612 acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); 613 int err; 614 615 /* 616 * The reset shouldn't ever fail in practice. The failure will be a 617 * sign of a severe problem that needs to be resolved. Still we don't 618 * want to fail the initialization completely because this may break 619 * kernel boot up since voltage regulators use I2C. Hence, we will 620 * emit a noisy warning on error, which won't stay unnoticed and 621 * won't hose machine entirely. 622 */ > 623 if (handle && acpi_has_method(handle, "_RST")) 624 err = (acpi_evaluate_object(handle, "_RST", NULL, NULL)); 625 else 626 err = reset_control_reset(i2c_dev->rst); 627 628 WARN_ON_ONCE(err); 629 630 if (i2c_dev->is_dvc) 631 tegra_dvc_init(i2c_dev); 632 633 val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN | 634 FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2); 635 636 if (i2c_dev->hw->has_multi_master_mode) 637 val |= I2C_CNFG_MULTI_MASTER_MODE; 638 639 i2c_writel(i2c_dev, val, I2C_CNFG); 640 i2c_writel(i2c_dev, 0, I2C_INT_MASK); 641 642 if (i2c_dev->is_vi) 643 tegra_i2c_vi_init(i2c_dev); 644 645 switch (i2c_dev->bus_clk_rate) { 646 case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ: 647 default: 648 tlow = i2c_dev->hw->tlow_fast_fastplus_mode; 649 thigh = i2c_dev->hw->thigh_fast_fastplus_mode; 650 tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode; 651 652 if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ) 653 non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode; 654 else 655 non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode; 656 break; 657 658 case 0 ... I2C_MAX_STANDARD_MODE_FREQ: 659 tlow = i2c_dev->hw->tlow_std_mode; 660 thigh = i2c_dev->hw->thigh_std_mode; 661 tsu_thd = i2c_dev->hw->setup_hold_time_std_mode; 662 non_hs_mode = i2c_dev->hw->clk_divisor_std_mode; 663 break; 664 } 665 666 /* make sure clock divisor programmed correctly */ 667 clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE, 668 i2c_dev->hw->clk_divisor_hs_mode) | 669 FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode); 670 i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR); 671 672 if (i2c_dev->hw->has_interface_timing_reg) { 673 val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) | 674 FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow); 675 i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0); 676 } 677 678 /* 679 * Configure setup and hold times only when tsu_thd is non-zero. 680 * Otherwise, preserve the chip default values. 681 */ 682 if (i2c_dev->hw->has_interface_timing_reg && tsu_thd) 683 i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1); 684 685 clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1); 686 687 err = clk_set_rate(i2c_dev->div_clk, 688 i2c_dev->bus_clk_rate * clk_multiplier); 689 if (err) { 690 dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err); 691 return err; 692 } 693 694 if (!i2c_dev->is_dvc && !i2c_dev->is_vi) { 695 u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG); 696 697 sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL; 698 i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG); 699 i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1); 700 i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2); 701 } 702 703 err = tegra_i2c_flush_fifos(i2c_dev); 704 if (err) 705 return err; 706 707 if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg) 708 i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE); 709 710 err = tegra_i2c_wait_for_config_load(i2c_dev); 711 if (err) 712 return err; 713 714 return 0; 715 } 716 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/22/21 4:16 PM, Paul Cercueil wrote: > Hi Lars, > > Le lun., nov. 22 2021 at 16:08:51 +0100, Lars-Peter Clausen > <lars(a)metafoo.de> a écrit : >> On 11/21/21 9:08 PM, Paul Cercueil wrote: >>> >>> >>> Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen >>> <lars(a)metafoo.de> a écrit : >>>> On 11/21/21 6:52 PM, Paul Cercueil wrote: >>>>> Hi Lars, >>>>> >>>>> Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen >>>>> <lars(a)metafoo.de> a écrit : >>>>>> On 11/15/21 3:19 PM, Paul Cercueil wrote: >>>>>>> The buffer-dma code was using two queues, incoming and outgoing, to >>>>>>> manage the state of the blocks in use. >>>>>>> >>>>>>> While this totally works, it adds some complexity to the code, >>>>>>> especially since the code only manages 2 blocks. It is much >>>>>>> easier to >>>>>>> just check each block's state manually, and keep a counter for >>>>>>> the next >>>>>>> block to dequeue. >>>>>>> >>>>>>> Since the new DMABUF based API wouldn't use these incoming and >>>>>>> outgoing >>>>>>> queues anyway, getting rid of them now makes the upcoming changes >>>>>>> simpler. >>>>>>> >>>>>>> Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> >>>>>> The outgoing queue is going to be replaced by fences, but I think >>>>>> we need to keep the incoming queue. >>>>> >>>>> Blocks are always accessed in sequential order, so we now have a >>>>> "queue->next_dequeue" that cycles between the buffers >>>>> allocated for fileio. >>>>> >>>>>>> [...] >>>>>>> @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); >>>>>>> static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue >>>>>>> *queue, >>>>>>> struct iio_dma_buffer_block *block) >>>>>>> { >>>>>>> - if (block->state == IIO_BLOCK_STATE_DEAD) { >>>>>>> + if (block->state == IIO_BLOCK_STATE_DEAD) >>>>>>> iio_buffer_block_put(block); >>>>>>> - } else if (queue->active) { >>>>>>> + else if (queue->active) >>>>>>> iio_dma_buffer_submit_block(queue, block); >>>>>>> - } else { >>>>>>> + else >>>>>>> block->state = IIO_BLOCK_STATE_QUEUED; >>>>>>> - list_add_tail(&block->head, &queue->incoming); >>>>>> If iio_dma_buffer_enqueue() is called with a dmabuf and the >>>>>> buffer is not active, it will be marked as queued, but we >>>>>> don't actually keep a reference to it anywhere. It will >>>>>> never be submitted to the DMA, and it will never be >>>>>> signaled as completed. >>>>> >>>>> We do keep a reference to the buffers, in the queue->fileio.blocks >>>>> array. When the buffer is enabled, all the blocks in that >>>>> array that are in the "queued" state will be submitted to the >>>>> DMA. >>>>> >>>> But not when used in combination with the DMA buf changes later in >>>> this series. >>>> >>> >>> That's still the case after the DMABUF changes of the series. Or can >>> you point me exactly what you think is broken? >>> >> When you allocate a DMABUF with the allocate IOCTL and then submit it >> with the enqueue IOCTL before the buffer is enabled it will end up >> marked as queued, but not actually be queued anywhere. >> > > Ok, it works for me because I never enqueue blocks before enabling the > buffer. I can add a requirement that blocks must be enqueued only > after the buffer is enabled. I don't think that is a good idea. This way you are going to potentially drop data at the begining of your stream when the DMA isn't ready yet.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/21/21 9:08 PM, Paul Cercueil wrote: > > > Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen > <lars(a)metafoo.de> a écrit : >> On 11/21/21 6:52 PM, Paul Cercueil wrote: >>> Hi Lars, >>> >>> Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen >>> <lars(a)metafoo.de> a écrit : >>>> On 11/15/21 3:19 PM, Paul Cercueil wrote: >>>>> The buffer-dma code was using two queues, incoming and outgoing, to >>>>> manage the state of the blocks in use. >>>>> >>>>> While this totally works, it adds some complexity to the code, >>>>> especially since the code only manages 2 blocks. It is much easier to >>>>> just check each block's state manually, and keep a counter for the >>>>> next >>>>> block to dequeue. >>>>> >>>>> Since the new DMABUF based API wouldn't use these incoming and >>>>> outgoing >>>>> queues anyway, getting rid of them now makes the upcoming changes >>>>> simpler. >>>>> >>>>> Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> >>>> The outgoing queue is going to be replaced by fences, but I think >>>> we need to keep the incoming queue. >>> >>> Blocks are always accessed in sequential order, so we now have a >>> "queue->next_dequeue" that cycles between the buffers allocated for >>> fileio. >>> >>>>> [...] >>>>> @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); >>>>> static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue >>>>> *queue, >>>>> struct iio_dma_buffer_block *block) >>>>> { >>>>> - if (block->state == IIO_BLOCK_STATE_DEAD) { >>>>> + if (block->state == IIO_BLOCK_STATE_DEAD) >>>>> iio_buffer_block_put(block); >>>>> - } else if (queue->active) { >>>>> + else if (queue->active) >>>>> iio_dma_buffer_submit_block(queue, block); >>>>> - } else { >>>>> + else >>>>> block->state = IIO_BLOCK_STATE_QUEUED; >>>>> - list_add_tail(&block->head, &queue->incoming); >>>> If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer >>>> is not active, it will be marked as queued, but we don't actually >>>> keep a reference to it anywhere. It will never be submitted to >>>> the DMA, and it will never be signaled as completed. >>> >>> We do keep a reference to the buffers, in the queue->fileio.blocks >>> array. When the buffer is enabled, all the blocks in that array >>> that are in the "queued" state will be submitted to the DMA. >>> >> But not when used in combination with the DMA buf changes later in >> this series. >> > > That's still the case after the DMABUF changes of the series. Or can > you point me exactly what you think is broken? > When you allocate a DMABUF with the allocate IOCTL and then submit it with the enqueue IOCTL before the buffer is enabled it will end up marked as queued, but not actually be queued anywhere.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] i2c: tegra: Add ACPI support

by Dmitry Osipenko

19.11.2021 16:32, Akhil R пишет: > - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > - if (IS_ERR(i2c_dev->rst)) { > - dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), > - "failed to get reset control\n"); > - return PTR_ERR(i2c_dev->rst); > - } > - > tegra_i2c_parse_dt(i2c_dev); > > - err = tegra_i2c_init_clocks(i2c_dev); > - if (err) > - return err; > + if (!has_acpi_companion(&pdev->dev)) { > + i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > + if (IS_ERR(i2c_dev->rst)) { > + dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), > + "failed to get reset control\n"); > + return PTR_ERR(i2c_dev->rst); > + } > + > + err = tegra_i2c_init_clocks(i2c_dev); > + if (err) > + return err; > + } What about to factor out the reset initialization into a separate function and write it like this: static int tegra_i2c_init_reset(i2c_dev) { if (has_acpi_companion(i2c_dev->dev) return 0; i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); if (IS_ERR(i2c_dev->rst)) return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), "failed to get reset control\n"); return 0; } And then change tegra_i2c_init_clocks() to: static int tegra_i2c_init_clocks(i2c_dev) { int err; if (has_acpi_companion(i2c_dev->dev)) return 0; ... } This will make both reset/clocks initialization to look more consistent.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] i2c: tegra: Add ACPI support

by Dmitry Osipenko

19.11.2021 17:48, Andy Shevchenko пишет: >> + if (i2c_dev->nclocks == 0) >> + return; > Why? Make clocks optional. This check shouldn't be needed because both clk_disable() and clk_bulk_unprepare() should handle NULL/zero clocks without problems.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/21/21 6:52 PM, Paul Cercueil wrote: > Hi Lars, > > Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen > <lars(a)metafoo.de> a écrit : >> On 11/15/21 3:19 PM, Paul Cercueil wrote: >>> The buffer-dma code was using two queues, incoming and outgoing, to >>> manage the state of the blocks in use. >>> >>> While this totally works, it adds some complexity to the code, >>> especially since the code only manages 2 blocks. It is much easier to >>> just check each block's state manually, and keep a counter for the next >>> block to dequeue. >>> >>> Since the new DMABUF based API wouldn't use these incoming and outgoing >>> queues anyway, getting rid of them now makes the upcoming changes >>> simpler. >>> >>> Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> >> The outgoing queue is going to be replaced by fences, but I think we >> need to keep the incoming queue. > > Blocks are always accessed in sequential order, so we now have a > "queue->next_dequeue" that cycles between the buffers allocated for > fileio. > >>> [...] >>> @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); >>> static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue >>> *queue, >>> struct iio_dma_buffer_block *block) >>> { >>> - if (block->state == IIO_BLOCK_STATE_DEAD) { >>> + if (block->state == IIO_BLOCK_STATE_DEAD) >>> iio_buffer_block_put(block); >>> - } else if (queue->active) { >>> + else if (queue->active) >>> iio_dma_buffer_submit_block(queue, block); >>> - } else { >>> + else >>> block->state = IIO_BLOCK_STATE_QUEUED; >>> - list_add_tail(&block->head, &queue->incoming); >> If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is >> not active, it will be marked as queued, but we don't actually keep a >> reference to it anywhere. It will never be submitted to the DMA, and >> it will never be signaled as completed. > > We do keep a reference to the buffers, in the queue->fileio.blocks > array. When the buffer is enabled, all the blocks in that array that > are in the "queued" state will be submitted to the DMA. > But not when used in combination with the DMA buf changes later in this series.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/15/21 3:19 PM, Paul Cercueil wrote: > The buffer-dma code was using two queues, incoming and outgoing, to > manage the state of the blocks in use. > > While this totally works, it adds some complexity to the code, > especially since the code only manages 2 blocks. It is much easier to > just check each block's state manually, and keep a counter for the next > block to dequeue. > > Since the new DMABUF based API wouldn't use these incoming and outgoing > queues anyway, getting rid of them now makes the upcoming changes > simpler. > > Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> The outgoing queue is going to be replaced by fences, but I think we need to keep the incoming queue. > [...] > @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); > static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue, > struct iio_dma_buffer_block *block) > { > - if (block->state == IIO_BLOCK_STATE_DEAD) { > + if (block->state == IIO_BLOCK_STATE_DEAD) > iio_buffer_block_put(block); > - } else if (queue->active) { > + else if (queue->active) > iio_dma_buffer_submit_block(queue, block); > - } else { > + else > block->state = IIO_BLOCK_STATE_QUEUED; > - list_add_tail(&block->head, &queue->incoming); If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is not active, it will be marked as queued, but we don't actually keep a reference to it anywhere. It will never be submitted to the DMA, and it will never be signaled as completed.

4 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/15] iio: buffer-dma: write() and new DMABUF based API

by Christian König

Am 16.11.21 um 17:31 schrieb Laurent Pinchart: > On Tue, Nov 16, 2021 at 05:02:25PM +0100, Daniel Vetter wrote: >> On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: >>> Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter a écrit : >>>> On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: >>>>> Hi Jonathan, >>>>> >>>>> This patchset introduces a new userspace interface based on DMABUF >>>>> objects, to complement the existing fileio based API. >>>>> >>>>> The advantage of this DMABUF based interface vs. the fileio >>>>> interface, is that it avoids an extra copy of the data between the >>>>> kernel and userspace. This is particularly userful for high-speed >>>>> devices which produce several megabytes or even gigabytes of data per >>>>> second. >>>>> >>>>> The first few patches [01/15] to [03/15] are not really related, but >>>>> allow to reduce the size of the patches that introduce the new API. >>>>> >>>>> Patch [04/15] to [06/15] enables write() support to the buffer-dma >>>>> implementation of the buffer API, to continue the work done by >>>>> Mihail Chindris. >>>>> >>>>> Patches [07/15] to [12/15] introduce the new DMABUF based API. >>>>> >>>>> Patches [13/15] and [14/15] add support for cyclic buffers, only through >>>>> the new API. A cyclic buffer will be repeated on the output until the >>>>> buffer is disabled. >>>>> >>>>> Patch [15/15] adds documentation about the new API. >>>>> >>>>> For now, the API allows you to alloc DMABUF objects and mmap() them >>>>> to >>>>> read or write the samples. It does not yet allow to import DMABUFs >>>>> parented to other subsystems, but that should eventually be possible >>>>> once it's wired. >>>>> >>>>> This patchset is inspired by the "mmap interface" that was previously >>>>> submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be >>>>> great if I could get a review from you guys. Alexandru's commit was >>>>> signed with his @analog.com address but he doesn't work at ADI anymore, >>>>> so I believe I'll need him to sign with a new email. >>>> Why dma-buf? dma-buf looks like something super generic and useful, until >>>> you realize that there's a metric ton of gpu/accelerator bagage piled in. >>>> So unless buffer sharing with a gpu/video/accel/whatever device is the > And cameras (maybe they're included in "whatever" ?). > >>>> goal here, and it's just for a convenient way to get at buffer handles, >>>> this doesn't sound like a good idea. >>> Good question. The first reason is that a somewhat similar API was intented >>> before[1], but refused upstream as it was kind of re-inventing the wheel. >>> >>> The second reason, is that we want to be able to share buffers too, not with >>> gpu/video but with the network (zctap) and in the future with USB >>> (functionFS) too. >>> >>> [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern… >> Hm is that code merged already in upstream already? >> >> I know that dma-buf looks really generic, but like I said if there's no >> need ever to interface with any of the gpu buffer sharing it might be >> better to use something else (like get_user_pages maybe, would that work?). > Not GUP please. That brings another set of issues, especially when > dealing with DMA, we've suffered enough from the USERPTR support in V4L2 > to avoid repeating this. Let's make dma-buf better instead. Yeah, when comparing GUP and DMA-buf the later is clearly the lesser evil. DMA-buf indeed has some design issues we need to work on, especially around the async operations and synchronization. But I still think those are solvable. GUP on the other hand has some hard fundamental problems which you can only solved completely if the hardware is capable of fast and reliable recoverable page faults. Regards, Christian.

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/15] iio: buffer-dma: write() and new DMABUF based API

by Daniel Vetter

On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: > Hi Daniel, > > Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter <daniel(a)ffwll.ch> a > écrit : > > On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > > > Hi Jonathan, > > > > > > This patchset introduces a new userspace interface based on DMABUF > > > objects, to complement the existing fileio based API. > > > > > > The advantage of this DMABUF based interface vs. the fileio > > > interface, is that it avoids an extra copy of the data between the > > > kernel and userspace. This is particularly userful for high-speed > > > devices which produce several megabytes or even gigabytes of data > > > per > > > second. > > > > > > The first few patches [01/15] to [03/15] are not really related, but > > > allow to reduce the size of the patches that introduce the new API. > > > > > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > > > implementation of the buffer API, to continue the work done by > > > Mihail Chindris. > > > > > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > > > > > Patches [13/15] and [14/15] add support for cyclic buffers, only > > > through > > > the new API. A cyclic buffer will be repeated on the output until > > > the > > > buffer is disabled. > > > > > > Patch [15/15] adds documentation about the new API. > > > > > > For now, the API allows you to alloc DMABUF objects and mmap() them > > > to > > > read or write the samples. It does not yet allow to import DMABUFs > > > parented to other subsystems, but that should eventually be possible > > > once it's wired. > > > > > > This patchset is inspired by the "mmap interface" that was > > > previously > > > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would > > > be > > > great if I could get a review from you guys. Alexandru's commit was > > > signed with his @analog.com address but he doesn't work at ADI > > > anymore, > > > so I believe I'll need him to sign with a new email. > > > > Why dma-buf? dma-buf looks like something super generic and useful, > > until > > you realize that there's a metric ton of gpu/accelerator bagage piled > > in. > > So unless buffer sharing with a gpu/video/accel/whatever device is the > > goal here, and it's just for a convenient way to get at buffer handles, > > this doesn't sound like a good idea. > > Good question. The first reason is that a somewhat similar API was intented > before[1], but refused upstream as it was kind of re-inventing the wheel. > > The second reason, is that we want to be able to share buffers too, not with > gpu/video but with the network (zctap) and in the future with USB > (functionFS) too. > > [1]: https://lore.kernel.org/linux-iio/20210217073638.21681-1-alexandru.ardelean… Hm is that code merged already in upstream already? I know that dma-buf looks really generic, but like I said if there's no need ever to interface with any of the gpu buffer sharing it might be better to use something else (like get_user_pages maybe, would that work?). > > Also if the idea is to this with gpus/accelerators then I'd really like > > to > > see the full thing, since most likely at that point you also want > > dma_fence. And once we talk dma_fence things get truly horrible from a > > locking pov :-( Or well, just highly constrained and I get to review > > what > > iio is doing with these buffers to make sure it all fits. > > There is some dma_fence action in patch #10, which is enough for the > userspace apps to use the API. > > What "horribleness" are we talking about here? It doesn't look that scary to > me, but I certainly don't have the complete picture. You need to annotate all the code involved in signalling that dma_fence using dma_fence_begin/end_signalling, and then enable full lockdep and everything. You can safely assume you'll find bugs, because we even have bugs about this in gpu drivers (where that annotation isn't fully rolled out yet). The tldr is that you can allocate memory in there. And a pile of other restrictions, but not being able to allocate memory (well GFP_ATOMIC is ok, but that can fail) is a very serious restriction. -Daniel > > Cheers, > -Paul > > > Cheers, Daniel > > > > > > > > Cheers, > > > -Paul > > > > > > Alexandru Ardelean (1): > > > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > > > > > Paul Cercueil (14): > > > iio: buffer-dma: Get rid of incoming/outgoing queues > > > iio: buffer-dma: Remove unused iio_buffer_block struct > > > iio: buffer-dma: Use round_down() instead of rounddown() > > > iio: buffer-dma: Enable buffer write support > > > iio: buffer-dmaengine: Support specifying buffer direction > > > iio: buffer-dmaengine: Enable write support > > > iio: core: Add new DMABUF interface infrastructure > > > iio: buffer-dma: Use DMABUFs instead of custom solution > > > iio: buffer-dma: Implement new DMABUF based userspace API > > > iio: buffer-dma: Boost performance using write-combine cache > > > setting > > > iio: buffer-dmaengine: Support new DMABUF based userspace API > > > iio: core: Add support for cyclic buffers > > > iio: buffer-dmaengine: Add support for cyclic buffers > > > Documentation: iio: Document high-speed DMABUF based API > > > > > > Documentation/driver-api/dma-buf.rst | 2 + > > > Documentation/iio/dmabuf_api.rst | 94 +++ > > > Documentation/iio/index.rst | 2 + > > > drivers/iio/adc/adi-axi-adc.c | 3 +- > > > drivers/iio/buffer/industrialio-buffer-dma.c | 670 > > > ++++++++++++++---- > > > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > > > drivers/iio/industrialio-buffer.c | 49 ++ > > > include/linux/iio/buffer-dma.h | 43 +- > > > include/linux/iio/buffer-dmaengine.h | 5 +- > > > include/linux/iio/buffer_impl.h | 8 + > > > include/uapi/linux/iio/buffer.h | 30 + > > > 11 files changed, 783 insertions(+), 165 deletions(-) > > > create mode 100644 Documentation/iio/dmabuf_api.rst > > > > > > -- > > > 2.33.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 2/2] drm/msm: Restore error return on invalid fence

by Rob Clark

On Mon, Nov 15, 2021 at 6:43 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > > On 11/12/2021 12:54 AM, Rob Clark wrote: > > From: Rob Clark <robdclark(a)chromium.org> > > > > When converting to use an idr to map userspace fence seqno values back > > to a dma_fence, we lost the error return when userspace passes seqno > > that is larger than the last submitted fence. Restore this check. > > > > Reported-by: Akhil P Oommen <akhilpo(a)codeaurora.org> > > Fixes: a61acbbe9cf8 ("drm/msm: Track "seqno" fences by idr") > > Signed-off-by: Rob Clark <robdclark(a)chromium.org> > > --- > > Note: I will rebase "drm/msm: Handle fence rollover" on top of this, > > to simplify backporting this patch to stable kernels > > > > drivers/gpu/drm/msm/msm_drv.c | 6 ++++++ > > drivers/gpu/drm/msm/msm_gem_submit.c | 1 + > > drivers/gpu/drm/msm/msm_gpu.h | 3 +++ > > 3 files changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index cb14d997c174..56500eb5219e 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -967,6 +967,12 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id, > > struct dma_fence *fence; > > int ret; > > > > + if (fence_id > queue->last_fence) { > > But fence_id can wrap around and then this check won't be valid. that is correct, but see my note about rebasing "drm/msm: Handle fence rollover" on top of this patch, so this patch could be more easily cherry-picked to stable/lts branches BR, -R > -Akhil. > > > + DRM_ERROR_RATELIMITED("waiting on invalid fence: %u (of %u)\n", > > + fence_id, queue->last_fence); > > + return -EINVAL; > > + } > > + > > /* > > * Map submitqueue scoped "seqno" (which is actually an idr key) > > * back to underlying dma-fence > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > > index 151d19e4453c..a38f23be497d 100644 > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > > @@ -911,6 +911,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > > drm_sched_entity_push_job(&submit->base, queue->entity); > > > > args->fence = submit->fence_id; > > + queue->last_fence = submit->fence_id; > > > > msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs); > > msm_process_post_deps(post_deps, args->nr_out_syncobjs, > > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > > index bd4e0024033e..e73a5bb03544 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.h > > +++ b/drivers/gpu/drm/msm/msm_gpu.h > > @@ -376,6 +376,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu *gpu, int prio, > > * @ring_nr: the ringbuffer used by this submitqueue, which is determined > > * by the submitqueue's priority > > * @faults: the number of GPU hangs associated with this submitqueue > > + * @last_fence: the sequence number of the last allocated fence (for error > > + * checking) > > * @ctx: the per-drm_file context associated with the submitqueue (ie. > > * which set of pgtables do submits jobs associated with the > > * submitqueue use) > > @@ -391,6 +393,7 @@ struct msm_gpu_submitqueue { > > u32 flags; > > u32 ring_nr; > > int faults; > > + uint32_t last_fence; > > struct msm_file_private *ctx; > > struct list_head node; > > struct idr fence_idr; > > >

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/15] iio: buffer-dma: write() and new DMABUF based API

by Daniel Vetter

On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > Hi Jonathan, > > This patchset introduces a new userspace interface based on DMABUF > objects, to complement the existing fileio based API. > > The advantage of this DMABUF based interface vs. the fileio > interface, is that it avoids an extra copy of the data between the > kernel and userspace. This is particularly userful for high-speed > devices which produce several megabytes or even gigabytes of data per > second. > > The first few patches [01/15] to [03/15] are not really related, but > allow to reduce the size of the patches that introduce the new API. > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > implementation of the buffer API, to continue the work done by > Mihail Chindris. > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > Patches [13/15] and [14/15] add support for cyclic buffers, only through > the new API. A cyclic buffer will be repeated on the output until the > buffer is disabled. > > Patch [15/15] adds documentation about the new API. > > For now, the API allows you to alloc DMABUF objects and mmap() them to > read or write the samples. It does not yet allow to import DMABUFs > parented to other subsystems, but that should eventually be possible > once it's wired. > > This patchset is inspired by the "mmap interface" that was previously > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be > great if I could get a review from you guys. Alexandru's commit was > signed with his @analog.com address but he doesn't work at ADI anymore, > so I believe I'll need him to sign with a new email. Why dma-buf? dma-buf looks like something super generic and useful, until you realize that there's a metric ton of gpu/accelerator bagage piled in. So unless buffer sharing with a gpu/video/accel/whatever device is the goal here, and it's just for a convenient way to get at buffer handles, this doesn't sound like a good idea. Also if the idea is to this with gpus/accelerators then I'd really like to see the full thing, since most likely at that point you also want dma_fence. And once we talk dma_fence things get truly horrible from a locking pov :-( Or well, just highly constrained and I get to review what iio is doing with these buffers to make sure it all fits. Cheers, Daniel > > Cheers, > -Paul > > Alexandru Ardelean (1): > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > Paul Cercueil (14): > iio: buffer-dma: Get rid of incoming/outgoing queues > iio: buffer-dma: Remove unused iio_buffer_block struct > iio: buffer-dma: Use round_down() instead of rounddown() > iio: buffer-dma: Enable buffer write support > iio: buffer-dmaengine: Support specifying buffer direction > iio: buffer-dmaengine: Enable write support > iio: core: Add new DMABUF interface infrastructure > iio: buffer-dma: Use DMABUFs instead of custom solution > iio: buffer-dma: Implement new DMABUF based userspace API > iio: buffer-dma: Boost performance using write-combine cache setting > iio: buffer-dmaengine: Support new DMABUF based userspace API > iio: core: Add support for cyclic buffers > iio: buffer-dmaengine: Add support for cyclic buffers > Documentation: iio: Document high-speed DMABUF based API > > Documentation/driver-api/dma-buf.rst | 2 + > Documentation/iio/dmabuf_api.rst | 94 +++ > Documentation/iio/index.rst | 2 + > drivers/iio/adc/adi-axi-adc.c | 3 +- > drivers/iio/buffer/industrialio-buffer-dma.c | 670 ++++++++++++++---- > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > drivers/iio/industrialio-buffer.c | 49 ++ > include/linux/iio/buffer-dma.h | 43 +- > include/linux/iio/buffer-dmaengine.h | 5 +- > include/linux/iio/buffer_impl.h | 8 + > include/uapi/linux/iio/buffer.h | 30 + > 11 files changed, 783 insertions(+), 165 deletions(-) > create mode 100644 Documentation/iio/dmabuf_api.rst > > -- > 2.33.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: add DMA_BUF_IOCTL_SYNC_PARTIAL support

by Christian König

Am 13.11.21 um 07:22 schrieb Jianqun Xu: > Add DMA_BUF_IOCTL_SYNC_PARTIAL support for user to sync dma-buf with > offset and len. You have not given an use case for this so it is a bit hard to review. And from the existing use cases I don't see why this should be necessary. Even worse from the existing backend implementation I don't even see how drivers should be able to fulfill this semantics. Please explain further, Christian. > > Change-Id: I03d2d2e10e48d32aa83c31abade57e0931e1be49 > Signed-off-by: Jianqun Xu <jay.xu(a)rock-chips.com> > --- > drivers/dma-buf/dma-buf.c | 42 ++++++++++++++++++++++++++++++++++++ > include/uapi/linux/dma-buf.h | 8 +++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index d9948d58b3f4..78f37f7c3462 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -392,6 +392,7 @@ static long dma_buf_ioctl(struct file *file, > { > struct dma_buf *dmabuf; > struct dma_buf_sync sync; > + struct dma_buf_sync_partial sync_p; > enum dma_data_direction direction; > int ret; > > @@ -430,6 +431,47 @@ static long dma_buf_ioctl(struct file *file, > case DMA_BUF_SET_NAME_B: > return dma_buf_set_name(dmabuf, (const char __user *)arg); > > + case DMA_BUF_IOCTL_SYNC_PARTIAL: > + if (copy_from_user(&sync_p, (void __user *) arg, sizeof(sync_p))) > + return -EFAULT; > + > + if (sync_p.len == 0) > + return 0; > + > + if ((sync_p.offset % cache_line_size()) || (sync_p.len % cache_line_size())) > + return -EINVAL; > + > + if (sync_p.len > dmabuf->size || sync_p.offset > dmabuf->size - sync_p.len) > + return -EINVAL; > + > + if (sync_p.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) > + return -EINVAL; > + > + switch (sync_p.flags & DMA_BUF_SYNC_RW) { > + case DMA_BUF_SYNC_READ: > + direction = DMA_FROM_DEVICE; > + break; > + case DMA_BUF_SYNC_WRITE: > + direction = DMA_TO_DEVICE; > + break; > + case DMA_BUF_SYNC_RW: > + direction = DMA_BIDIRECTIONAL; > + break; > + default: > + return -EINVAL; > + } > + > + if (sync_p.flags & DMA_BUF_SYNC_END) > + ret = dma_buf_end_cpu_access_partial(dmabuf, direction, > + sync_p.offset, > + sync_p.len); > + else > + ret = dma_buf_begin_cpu_access_partial(dmabuf, direction, > + sync_p.offset, > + sync_p.len); > + > + return ret; > + > default: > return -ENOTTY; > } > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index 7f30393b92c3..6236c644624d 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -47,4 +47,12 @@ struct dma_buf_sync { > #define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32) > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64) > > +struct dma_buf_sync_partial { > + __u64 flags; > + __u32 offset; > + __u32 len; > +}; > + > +#define DMA_BUF_IOCTL_SYNC_PARTIAL _IOW(DMA_BUF_BASE, 2, struct dma_buf_sync_partial) > + > #endif

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] AUTOSEL series truncated was -- Re: [PATCH AUTOSEL 5.15 001/146] dma-buf: WARN on dmabuf release with pending attachments

by Randy Dunlap

On 11/8/21 11:54 PM, Pavel Machek wrote: > Hi! > > This series is truncated .. I only got first patches. Similary, 5.10 > series is truncated, [PATCH AUTOSEL 5.10 035/101] media: s5p-mfc: Add > checking to s5p_mfc_probe... is last one I got. > > I got all the patches before that, so I believe it is not problem on > my side, but I'd not mind someone confirming they are seeing the same > problem... Yes, several of the patch series were incomplete for me also... -- ~Randy

4 years, 1 month

2
1
0 0

[PATCH 0/2] drm/msm: wait_fence fixes

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> A couple of wait_fence related fixes. Rob Clark (2): drm/msm: Fix wait_fence submitqueue leak drm/msm: Restore error return on invalid fence drivers/gpu/drm/msm/msm_drv.c | 49 ++++++++++++++++++---------- drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 ++ 3 files changed, 36 insertions(+), 17 deletions(-) -- 2.31.1

4 years, 1 month

1
1
0 0

Re: [Linaro-mm-sig] [Freedreno] [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr

by Rob Clark

On Thu, Nov 11, 2021 at 7:54 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > > On 11/10/2021 10:25 PM, Rob Clark wrote: > > On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > >> > >> On 7/28/2021 6:36 AM, Rob Clark wrote: > >>> From: Rob Clark <robdclark(a)chromium.org> > >>> > >>> Previously the (non-fd) fence returned from submit ioctl was a raw > >>> seqno, which is scoped to the ring. But from UABI standpoint, the > >>> ioctls related to seqno fences all specify a submitqueue. We can > >>> take advantage of that to replace the seqno fences with a cyclic idr > >>> handle. > >>> > >>> This is in preperation for moving to drm scheduler, at which point > >>> the submit ioctl will return after queuing the submit job to the > >>> scheduler, but before the submit is written into the ring (and > >>> therefore before a ring seqno has been assigned). Which means we > >>> need to replace the dma_fence that userspace may need to wait on > >>> with a scheduler fence. > >>> > >>> Signed-off-by: Rob Clark <robdclark(a)chromium.org> > >>> Acked-by: Christian König <christian.koenig(a)amd.com> > >>> --- > >>> drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++-- > >>> drivers/gpu/drm/msm/msm_fence.c | 42 --------------------------- > >>> drivers/gpu/drm/msm/msm_fence.h | 3 -- > >>> drivers/gpu/drm/msm/msm_gem.h | 1 + > >>> drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++- > >>> drivers/gpu/drm/msm/msm_gpu.h | 5 ++++ > >>> drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++ > >>> 7 files changed, 61 insertions(+), 48 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > >>> index 9b8fa2ad0d84..1594ae39d54f 100644 > >>> --- a/drivers/gpu/drm/msm/msm_drv.c > >>> +++ b/drivers/gpu/drm/msm/msm_drv.c > >>> @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > >>> ktime_t timeout = to_ktime(args->timeout); > >>> struct msm_gpu_submitqueue *queue; > >>> struct msm_gpu *gpu = priv->gpu; > >>> + struct dma_fence *fence; > >>> int ret; > >>> > >>> if (args->pad) { > >>> @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > >>> if (!queue) > >>> return -ENOENT; > >>> > >>> - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout, > >>> - true); > >>> + /* > >>> + * Map submitqueue scoped "seqno" (which is actually an idr key) > >>> + * back to underlying dma-fence > >>> + * > >>> + * The fence is removed from the fence_idr when the submit is > >>> + * retired, so if the fence is not found it means there is nothing > >>> + * to wait for > >>> + */ > >>> + ret = mutex_lock_interruptible(&queue->lock); > >>> + if (ret) > >>> + return ret; > >>> + fence = idr_find(&queue->fence_idr, args->fence); > >>> + if (fence) > >>> + fence = dma_fence_get_rcu(fence); > >>> + mutex_unlock(&queue->lock); > >>> + > >>> + if (!fence) > >>> + return 0; > >>> > >>> + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); > >>> + if (ret == 0) { > >>> + ret = -ETIMEDOUT; > >>> + } else if (ret != -ERESTARTSYS) { > >>> + ret = 0; > >>> + } > >>> + > >>> + dma_fence_put(fence); > >>> msm_submitqueue_put(queue); > >>> + > >>> return ret; > >>> } > >>> > >>> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c > >>> index b92a9091a1e2..f2cece542c3f 100644 > >>> --- a/drivers/gpu/drm/msm/msm_fence.c > >>> +++ b/drivers/gpu/drm/msm/msm_fence.c > >>> @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr, > >>> strncpy(fctx->name, name, sizeof(fctx->name)); > >>> fctx->context = dma_fence_context_alloc(1); > >>> fctx->fenceptr = fenceptr; > >>> - init_waitqueue_head(&fctx->event); > >>> spin_lock_init(&fctx->spinlock); > >>> > >>> return fctx; > >>> @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc > >>> (int32_t)(*fctx->fenceptr - fence) >= 0; > >>> } > >>> > >>> -/* legacy path for WAIT_FENCE ioctl: */ > >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > >>> - ktime_t *timeout, bool interruptible) > >>> -{ > >>> - int ret; > >>> - > >>> - if (fence > fctx->last_fence) { > >>> - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", > >>> - fctx->name, fence, fctx->last_fence); > >>> - return -EINVAL; > >> > >> Rob, we changed this pre-existing behaviour in this patch. Now, when > >> userspace tries to wait on a future fence, we don't return an error. > >> > >> I just want to check if this was accidental or not? > > > > Hmm, perhaps we should do this to restore the previous behavior: > > > > ------------- > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 73e827641024..3dd6da56eae6 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct > > drm_device *dev, void *data, > > fence = dma_fence_get_rcu(fence); > > mutex_unlock(&queue->lock); > > > > - if (!fence) > > - return 0; > > + if (!fence) { > > + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx; > > + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: > > %u (of %u)\n", > > + fctx->name, fence, fctx->last_fence); > > + return -EINVAL; > > + } > > With this, when userspace tries to wait on a fence which is already > retired, it gets -EINVAL instead of success. Will this break userspace? Oh, right, we definitely don't want that.. I guess that was the reason for the original logic. I have a different idea.. will send a patch in a bit. BR, -R > -Akhil. > > > > > ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); > > if (ret == 0) { > > ------------- > > > > BR, > > -R > > > >> -Akhil. > >> > >>> - } > >>> - > >>> - if (!timeout) { > >>> - /* no-wait: */ > >>> - ret = fence_completed(fctx, fence) ? 0 : -EBUSY; > >>> - } else { > >>> - unsigned long remaining_jiffies = timeout_to_jiffies(timeout); > >>> - > >>> - if (interruptible) > >>> - ret = wait_event_interruptible_timeout(fctx->event, > >>> - fence_completed(fctx, fence), > >>> - remaining_jiffies); > >>> - else > >>> - ret = wait_event_timeout(fctx->event, > >>> - fence_completed(fctx, fence), > >>> - remaining_jiffies); > >>> - > >>> - if (ret == 0) { > >>> - DBG("timeout waiting for fence: %u (completed: %u)", > >>> - fence, fctx->completed_fence); > >>> - ret = -ETIMEDOUT; > >>> - } else if (ret != -ERESTARTSYS) { > >>> - ret = 0; > >>> - } > >>> - } > >>> - > >>> - return ret; > >>> -} > >>> - > >>> /* called from workqueue */ > >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence) > >>> { > >>> spin_lock(&fctx->spinlock); > >>> fctx->completed_fence = max(fence, fctx->completed_fence); > >>> spin_unlock(&fctx->spinlock); > >>> - > >>> - wake_up_all(&fctx->event); > >>> } > >>> > >>> struct msm_fence { > >>> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h > >>> index 6ab97062ff1a..4783db528bcc 100644 > >>> --- a/drivers/gpu/drm/msm/msm_fence.h > >>> +++ b/drivers/gpu/drm/msm/msm_fence.h > >>> @@ -49,7 +49,6 @@ struct msm_fence_context { > >>> */ > >>> volatile uint32_t *fenceptr; > >>> > >>> - wait_queue_head_t event; > >>> spinlock_t spinlock; > >>> }; > >>> > >>> @@ -57,8 +56,6 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev, > >>> volatile uint32_t *fenceptr, const char *name); > >>> void msm_fence_context_free(struct msm_fence_context *fctx); > >>> > >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > >>> - ktime_t *timeout, bool interruptible); > >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence); > >>> > >>> struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx); > >>> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > >>> index da3af702a6c8..e0579abda5b9 100644 > >>> --- a/drivers/gpu/drm/msm/msm_gem.h > >>> +++ b/drivers/gpu/drm/msm/msm_gem.h > >>> @@ -320,6 +320,7 @@ struct msm_gem_submit { > >>> struct ww_acquire_ctx ticket; > >>> uint32_t seqno; /* Sequence number of the submit on the ring */ > >>> struct dma_fence *fence; > >>> + int fence_id; /* key into queue->fence_idr */ > >>> struct msm_gpu_submitqueue *queue; > >>> struct pid *pid; /* submitting process */ > >>> bool fault_dumped; /* Limit devcoredump dumping to one per submit */ > >>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > >>> index 4f02fa3c78f9..f6f595aae2c5 100644 > >>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c > >>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > >>> @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref) > >>> container_of(kref, struct msm_gem_submit, ref); > >>> unsigned i; > >>> > >>> + if (submit->fence_id) { > >>> + mutex_lock(&submit->queue->lock); > >>> + idr_remove(&submit->queue->fence_idr, submit->fence_id); > >>> + mutex_unlock(&submit->queue->lock); > >>> + } > >>> + > >>> dma_fence_put(submit->fence); > >>> + > >>> put_pid(submit->pid); > >>> msm_submitqueue_put(submit->queue); > >>> > >>> @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > >>> goto out; > >>> } > >>> > >>> + /* > >>> + * Allocate an id which can be used by WAIT_FENCE ioctl to map back > >>> + * to the underlying fence. > >>> + */ > >>> + mutex_lock(&queue->lock); > >>> + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr, > >>> + submit->fence, 0, INT_MAX, GFP_KERNEL); > >>> + mutex_unlock(&queue->lock); > >>> + if (submit->fence_id < 0) { > >>> + ret = submit->fence_id = 0; > >>> + submit->fence_id = 0; > >>> + goto out; > >>> + } > >>> + > >>> if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { > >>> struct sync_file *sync_file = sync_file_create(submit->fence); > >>> if (!sync_file) { > >>> @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > >>> > >>> msm_gpu_submit(gpu, submit); > >>> > >>> - args->fence = submit->fence->seqno; > >>> + args->fence = submit->fence_id; > >>> > >>> msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs); > >>> msm_process_post_deps(post_deps, args->nr_out_syncobjs, > >>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > >>> index 96efcb31e502..579627252540 100644 > >>> --- a/drivers/gpu/drm/msm/msm_gpu.h > >>> +++ b/drivers/gpu/drm/msm/msm_gpu.h > >>> @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr { > >>> * which set of pgtables do submits jobs associated with the > >>> * submitqueue use) > >>> * @node: node in the context's list of submitqueues > >>> + * @fence_idr: maps fence-id to dma_fence for userspace visible fence > >>> + * seqno, protected by submitqueue lock > >>> + * @lock: submitqueue lock > >>> * @ref: reference count > >>> */ > >>> struct msm_gpu_submitqueue { > >>> @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue { > >>> int faults; > >>> struct msm_file_private *ctx; > >>> struct list_head node; > >>> + struct idr fence_idr; > >>> + struct mutex lock; > >>> struct kref ref; > >>> }; > >>> > >>> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c > >>> index 9e9fec61d629..66f8d0fb38b0 100644 > >>> --- a/drivers/gpu/drm/msm/msm_submitqueue.c > >>> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c > >>> @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref) > >>> struct msm_gpu_submitqueue *queue = container_of(kref, > >>> struct msm_gpu_submitqueue, ref); > >>> > >>> + idr_destroy(&queue->fence_idr); > >>> + > >>> msm_file_private_put(queue->ctx); > >>> > >>> kfree(queue); > >>> @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx, > >>> if (id) > >>> *id = queue->id; > >>> > >>> + idr_init(&queue->fence_idr); > >>> + mutex_init(&queue->lock); > >>> + > >>> list_add_tail(&queue->node, &ctx->submitqueues); > >>> > >>> write_unlock(&ctx->queuelock); > >>> > >> >

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr

by Rob Clark

On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > > On 7/28/2021 6:36 AM, Rob Clark wrote: > > From: Rob Clark <robdclark(a)chromium.org> > > > > Previously the (non-fd) fence returned from submit ioctl was a raw > > seqno, which is scoped to the ring. But from UABI standpoint, the > > ioctls related to seqno fences all specify a submitqueue. We can > > take advantage of that to replace the seqno fences with a cyclic idr > > handle. > > > > This is in preperation for moving to drm scheduler, at which point > > the submit ioctl will return after queuing the submit job to the > > scheduler, but before the submit is written into the ring (and > > therefore before a ring seqno has been assigned). Which means we > > need to replace the dma_fence that userspace may need to wait on > > with a scheduler fence. > > > > Signed-off-by: Rob Clark <robdclark(a)chromium.org> > > Acked-by: Christian König <christian.koenig(a)amd.com> > > --- > > drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++-- > > drivers/gpu/drm/msm/msm_fence.c | 42 --------------------------- > > drivers/gpu/drm/msm/msm_fence.h | 3 -- > > drivers/gpu/drm/msm/msm_gem.h | 1 + > > drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++- > > drivers/gpu/drm/msm/msm_gpu.h | 5 ++++ > > drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++ > > 7 files changed, 61 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 9b8fa2ad0d84..1594ae39d54f 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > > ktime_t timeout = to_ktime(args->timeout); > > struct msm_gpu_submitqueue *queue; > > struct msm_gpu *gpu = priv->gpu; > > + struct dma_fence *fence; > > int ret; > > > > if (args->pad) { > > @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > > if (!queue) > > return -ENOENT; > > > > - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout, > > - true); > > + /* > > + * Map submitqueue scoped "seqno" (which is actually an idr key) > > + * back to underlying dma-fence > > + * > > + * The fence is removed from the fence_idr when the submit is > > + * retired, so if the fence is not found it means there is nothing > > + * to wait for > > + */ > > + ret = mutex_lock_interruptible(&queue->lock); > > + if (ret) > > + return ret; > > + fence = idr_find(&queue->fence_idr, args->fence); > > + if (fence) > > + fence = dma_fence_get_rcu(fence); > > + mutex_unlock(&queue->lock); > > + > > + if (!fence) > > + return 0; > > > > + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); > > + if (ret == 0) { > > + ret = -ETIMEDOUT; > > + } else if (ret != -ERESTARTSYS) { > > + ret = 0; > > + } > > + > > + dma_fence_put(fence); > > msm_submitqueue_put(queue); > > + > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c > > index b92a9091a1e2..f2cece542c3f 100644 > > --- a/drivers/gpu/drm/msm/msm_fence.c > > +++ b/drivers/gpu/drm/msm/msm_fence.c > > @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr, > > strncpy(fctx->name, name, sizeof(fctx->name)); > > fctx->context = dma_fence_context_alloc(1); > > fctx->fenceptr = fenceptr; > > - init_waitqueue_head(&fctx->event); > > spin_lock_init(&fctx->spinlock); > > > > return fctx; > > @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc > > (int32_t)(*fctx->fenceptr - fence) >= 0; > > } > > > > -/* legacy path for WAIT_FENCE ioctl: */ > > -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > > - ktime_t *timeout, bool interruptible) > > -{ > > - int ret; > > - > > - if (fence > fctx->last_fence) { > > - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", > > - fctx->name, fence, fctx->last_fence); > > - return -EINVAL; > > Rob, we changed this pre-existing behaviour in this patch. Now, when > userspace tries to wait on a future fence, we don't return an error. > > I just want to check if this was accidental or not? Hmm, perhaps we should do this to restore the previous behavior: ------------- diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 73e827641024..3dd6da56eae6 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, fence = dma_fence_get_rcu(fence); mutex_unlock(&queue->lock); - if (!fence) - return 0; + if (!fence) { + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx; + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", + fctx->name, fence, fctx->last_fence); + return -EINVAL; + } ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); if (ret == 0) { ------------- BR, -R > -Akhil. > > > - } > > - > > - if (!timeout) { > > - /* no-wait: */ > > - ret = fence_completed(fctx, fence) ? 0 : -EBUSY; > > - } else { > > - unsigned long remaining_jiffies = timeout_to_jiffies(timeout); > > - > > - if (interruptible) > > - ret = wait_event_interruptible_timeout(fctx->event, > > - fence_completed(fctx, fence), > > - remaining_jiffies); > > - else > > - ret = wait_event_timeout(fctx->event, > > - fence_completed(fctx, fence), > > - remaining_jiffies); > > - > > - if (ret == 0) { > > - DBG("timeout waiting for fence: %u (completed: %u)", > > - fence, fctx->completed_fence); > > - ret = -ETIMEDOUT; > > - } else if (ret != -ERESTARTSYS) { > > - ret = 0; > > - } > > - } > > - > > - return ret; > > -} > > - > > /* called from workqueue */ > > void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence) > > { > > spin_lock(&fctx->spinlock); > > fctx->completed_fence = max(fence, fctx->completed_fence); > > spin_unlock(&fctx->spinlock); > > - > > - wake_up_all(&fctx->event); > > } > > > > struct msm_fence { > > diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h > > index 6ab97062ff1a..4783db528bcc 100644 > > --- a/drivers/gpu/drm/msm/msm_fence.h > > +++ b/drivers/gpu/drm/msm/msm_fence.h > > @@ -49,7 +49,6 @@ struct msm_fence_context { > > */ > > volatile uint32_t *fenceptr; > > > > - wait_queue_head_t event; > > spinlock_t spinlock; > > }; > > > > @@ -57,8 +56,6 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev, > > volatile uint32_t *fenceptr, const char *name); > > void msm_fence_context_free(struct msm_fence_context *fctx); > > > > -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > > - ktime_t *timeout, bool interruptible); > > void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence); > > > > struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx); > > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > > index da3af702a6c8..e0579abda5b9 100644 > > --- a/drivers/gpu/drm/msm/msm_gem.h > > +++ b/drivers/gpu/drm/msm/msm_gem.h > > @@ -320,6 +320,7 @@ struct msm_gem_submit { > > struct ww_acquire_ctx ticket; > > uint32_t seqno; /* Sequence number of the submit on the ring */ > > struct dma_fence *fence; > > + int fence_id; /* key into queue->fence_idr */ > > struct msm_gpu_submitqueue *queue; > > struct pid *pid; /* submitting process */ > > bool fault_dumped; /* Limit devcoredump dumping to one per submit */ > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > > index 4f02fa3c78f9..f6f595aae2c5 100644 > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > > @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref) > > container_of(kref, struct msm_gem_submit, ref); > > unsigned i; > > > > + if (submit->fence_id) { > > + mutex_lock(&submit->queue->lock); > > + idr_remove(&submit->queue->fence_idr, submit->fence_id); > > + mutex_unlock(&submit->queue->lock); > > + } > > + > > dma_fence_put(submit->fence); > > + > > put_pid(submit->pid); > > msm_submitqueue_put(submit->queue); > > > > @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > > goto out; > > } > > > > + /* > > + * Allocate an id which can be used by WAIT_FENCE ioctl to map back > > + * to the underlying fence. > > + */ > > + mutex_lock(&queue->lock); > > + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr, > > + submit->fence, 0, INT_MAX, GFP_KERNEL); > > + mutex_unlock(&queue->lock); > > + if (submit->fence_id < 0) { > > + ret = submit->fence_id = 0; > > + submit->fence_id = 0; > > + goto out; > > + } > > + > > if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { > > struct sync_file *sync_file = sync_file_create(submit->fence); > > if (!sync_file) { > > @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > > > > msm_gpu_submit(gpu, submit); > > > > - args->fence = submit->fence->seqno; > > + args->fence = submit->fence_id; > > > > msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs); > > msm_process_post_deps(post_deps, args->nr_out_syncobjs, > > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > > index 96efcb31e502..579627252540 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.h > > +++ b/drivers/gpu/drm/msm/msm_gpu.h > > @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr { > > * which set of pgtables do submits jobs associated with the > > * submitqueue use) > > * @node: node in the context's list of submitqueues > > + * @fence_idr: maps fence-id to dma_fence for userspace visible fence > > + * seqno, protected by submitqueue lock > > + * @lock: submitqueue lock > > * @ref: reference count > > */ > > struct msm_gpu_submitqueue { > > @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue { > > int faults; > > struct msm_file_private *ctx; > > struct list_head node; > > + struct idr fence_idr; > > + struct mutex lock; > > struct kref ref; > > }; > > > > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c > > index 9e9fec61d629..66f8d0fb38b0 100644 > > --- a/drivers/gpu/drm/msm/msm_submitqueue.c > > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c > > @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref) > > struct msm_gpu_submitqueue *queue = container_of(kref, > > struct msm_gpu_submitqueue, ref); > > > > + idr_destroy(&queue->fence_idr); > > + > > msm_file_private_put(queue->ctx); > > > > kfree(queue); > > @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx, > > if (id) > > *id = queue->id; > > > > + idr_init(&queue->fence_idr); > > + mutex_init(&queue->lock); > > + > > list_add_tail(&queue->node, &ctx->submitqueues); > > > > write_unlock(&ctx->queuelock); > > >

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] AUTOSEL series truncated was -- Re: [PATCH AUTOSEL 5.15 001/146] dma-buf: WARN on dmabuf release with pending attachments

by Christian König

Hi Pavel, Am 09.11.21 um 08:54 schrieb Pavel Machek: > Hi! > > This series is truncated .. I only got first patches. Similary, 5.10 > series is truncated, [PATCH AUTOSEL 5.10 035/101] media: s5p-mfc: Add > checking to s5p_mfc_probe... is last one I got. > > I got all the patches before that, so I believe it is not problem on > my side, but I'd not mind someone confirming they are seeing the same > problem... It could of course be a different issue, but I've been experiencing similar problems since a couple of weeks now, especially with mailing lists hosted on the freedesktop.org servers and long series of mails. The symptons are that individual mails are missing from a series. I'm usually registered with two completely separated mail accounts (private and work) on those lists and if a mail is missing it is always missing on both accounts. The interesting thing is that if it is a patch set then patchwork (https://patchwork.freedesktop.org/) always seems to get all mails. No idea what's going on here and so far it was to rarely to complain, but with this series it is totally obvious that something is wrong. Regards, Christian. > > Best regards, > Pavel >

4 years, 1 month

1
0
0 0

[PATCH AUTOSEL 5.4 01/74] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 758de0e9b2ddc..16bbc9bc9e6d1 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

4 years, 1 month

1
0
0 0

[PATCH AUTOSEL 5.10 001/101] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 922416b3aaceb..93e9bf7382595 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

4 years, 1 month

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig