Message ID | 20211114222353.22435-2-digetx@gmail.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | Add NVIDIA Tegra114 support to video decoder driver | expand |
15.11.2021 01:23, Dmitry Osipenko пишет: > + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); > + if (!vde->secure_bo) { > + dev_err(dev, "Failed to allocate secure BO\n"); > + goto err_pm_runtime; > + } My eye just caught that by accident err variable isn't assigned to -ENOMEM here. I'll make v2 shortly.
On Mon, Nov 15, 2021 at 01:34:18AM +0300, Dmitry Osipenko wrote: > 15.11.2021 01:23, Dmitry Osipenko пишет: > > + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); > > + if (!vde->secure_bo) { > > + dev_err(dev, "Failed to allocate secure BO\n"); > > + goto err_pm_runtime; > > + } > > My eye just caught that by accident err variable isn't assigned to > -ENOMEM here. I'll make v2 shortly. Smatch has a check for this so we would have caught it. :) regards, dan carpenter
15.11.2021 15:44, Dan Carpenter пишет: > On Mon, Nov 15, 2021 at 01:34:18AM +0300, Dmitry Osipenko wrote: >> 15.11.2021 01:23, Dmitry Osipenko пишет: >>> + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); >>> + if (!vde->secure_bo) { >>> + dev_err(dev, "Failed to allocate secure BO\n"); >>> + goto err_pm_runtime; >>> + } >> >> My eye just caught that by accident err variable isn't assigned to >> -ENOMEM here. I'll make v2 shortly. > > Smatch has a check for this so we would have caught it. :) Whish smatch was built-in into kernel and I could simply run "make smatch". On the other hand, I know that you're periodically checking upstream with smatch and patching the found bugs, so maybe it's fine as-is. Thank you for yours work on smatch, it's a very valuable tool.
On Mon, Nov 15, 2021 at 05:34:47PM +0300, Dmitry Osipenko wrote: > 15.11.2021 15:44, Dan Carpenter пишет: > > On Mon, Nov 15, 2021 at 01:34:18AM +0300, Dmitry Osipenko wrote: > >> 15.11.2021 01:23, Dmitry Osipenko пишет: > >>> + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); > >>> + if (!vde->secure_bo) { > >>> + dev_err(dev, "Failed to allocate secure BO\n"); > >>> + goto err_pm_runtime; > >>> + } > >> > >> My eye just caught that by accident err variable isn't assigned to > >> -ENOMEM here. I'll make v2 shortly. > > > > Smatch has a check for this so we would have caught it. :) > > Whish smatch was built-in into kernel and I could simply run "make > smatch". On the other hand, I know that you're periodically checking > upstream with smatch and patching the found bugs, so maybe it's fine > as-is. I run it every day, and it's integrated into the zero day bot and Mauro runs it. So someone would have caught it. regards, dan carpenter
15.11.2021 18:48, Dan Carpenter пишет: > On Mon, Nov 15, 2021 at 05:34:47PM +0300, Dmitry Osipenko wrote: >> 15.11.2021 15:44, Dan Carpenter пишет: >>> On Mon, Nov 15, 2021 at 01:34:18AM +0300, Dmitry Osipenko wrote: >>>> 15.11.2021 01:23, Dmitry Osipenko пишет: >>>>> + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); >>>>> + if (!vde->secure_bo) { >>>>> + dev_err(dev, "Failed to allocate secure BO\n"); >>>>> + goto err_pm_runtime; >>>>> + } >>>> >>>> My eye just caught that by accident err variable isn't assigned to >>>> -ENOMEM here. I'll make v2 shortly. >>> >>> Smatch has a check for this so we would have caught it. :) >> >> Whish smatch was built-in into kernel and I could simply run "make >> smatch". On the other hand, I know that you're periodically checking >> upstream with smatch and patching the found bugs, so maybe it's fine >> as-is. > > I run it every day, and it's integrated into the zero day bot and Mauro > runs it. So someone would have caught it. Very nice, I haven't noticed that 0-day runs smatch. I assume the smatch reports from the bots are private to you and Mauro since I never saw them in my inbox and on LKML, correct?
On Wed, Nov 17, 2021 at 07:19:30PM +0300, Dmitry Osipenko wrote: > 15.11.2021 18:48, Dan Carpenter пишет: > > On Mon, Nov 15, 2021 at 05:34:47PM +0300, Dmitry Osipenko wrote: > >> 15.11.2021 15:44, Dan Carpenter пишет: > >>> On Mon, Nov 15, 2021 at 01:34:18AM +0300, Dmitry Osipenko wrote: > >>>> 15.11.2021 01:23, Dmitry Osipenko пишет: > >>>>> + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); > >>>>> + if (!vde->secure_bo) { > >>>>> + dev_err(dev, "Failed to allocate secure BO\n"); > >>>>> + goto err_pm_runtime; > >>>>> + } > >>>> > >>>> My eye just caught that by accident err variable isn't assigned to > >>>> -ENOMEM here. I'll make v2 shortly. > >>> > >>> Smatch has a check for this so we would have caught it. :) > >> > >> Whish smatch was built-in into kernel and I could simply run "make > >> smatch". On the other hand, I know that you're periodically checking > >> upstream with smatch and patching the found bugs, so maybe it's fine > >> as-is. > > > > I run it every day, and it's integrated into the zero day bot and Mauro > > runs it. So someone would have caught it. > > Very nice, I haven't noticed that 0-day runs smatch. I assume the smatch > reports from the bots are private to you and Mauro since I never saw > them in my inbox and on LKML, correct? The 0-day bot is not great about running Smatch honestly... It never ran Smatch on this. It would have replied to the thread. regards, dan carpenter
It's not hard to run Smatch yourself... Depending on if you're on a apt distro or yum distro then fetch the dependencies with one of the follow commands: apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl yum install gcc make sqlite3 sqlite-devel sqlite perl-DBD-SQLite openssl-devel perl-Try-Tiny git clone https://github.com/error27/smatch cd smatch make cd ~/kernel_source/ ~/smatch/smatch_scripts/kchecker drivers/subsystem/ regards, dan carpenter
On Thu, 2021-11-18 at 09:14 +0300, Dan Carpenter wrote: > It's not hard to run Smatch yourself... > > Depending on if you're on a apt distro or yum distro then fetch the > dependencies with one of the follow commands: > apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl > yum install gcc make sqlite3 sqlite-devel sqlite perl-DBD-SQLite openssl-devel perl-Try-Tiny > > git clone https://github.com/error27/smatch > cd smatch > make > cd ~/kernel_source/ > ~/smatch/smatch_scripts/kchecker drivers/subsystem/ Might want to stick something in Documentation about that.
On Wed, Nov 17, 2021 at 10:21:00PM -0800, Joe Perches wrote: > On Thu, 2021-11-18 at 09:14 +0300, Dan Carpenter wrote: > > It's not hard to run Smatch yourself... > > > > Depending on if you're on a apt distro or yum distro then fetch the > > dependencies with one of the follow commands: > > apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl > > yum install gcc make sqlite3 sqlite-devel sqlite perl-DBD-SQLite openssl-devel perl-Try-Tiny > > > > git clone https://github.com/error27/smatch > > cd smatch > > make > > cd ~/kernel_source/ > > ~/smatch/smatch_scripts/kchecker drivers/subsystem/ > > Might want to stick something in Documentation about that. > That's a good idea. I was trying to figure out where it should go and I looked at Documentation/process/submit-checklist.rst and point 4 had me a bit puzzled: 4) ppc64 is a good architecture for cross-compilation checking because it tends to use ``unsigned long`` for 64-bit quantities. It took me a while to realize that this must have been written when 64 bit systems were new and rare. :P This documentation is old old old... regards, dan carpenter
Thanks, Joe. I'll create a Documentation/dev-tools/smatch.rst file. regards, dan carpenter
18.11.2021 09:14, Dan Carpenter пишет: > It's not hard to run Smatch yourself... > > Depending on if you're on a apt distro or yum distro then fetch the > dependencies with one of the follow commands: > apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl > yum install gcc make sqlite3 sqlite-devel sqlite perl-DBD-SQLite openssl-devel perl-Try-Tiny > > git clone https://github.com/error27/smatch > cd smatch > make > cd ~/kernel_source/ > ~/smatch/smatch_scripts/kchecker drivers/subsystem/ Thanks, I was running Smatch couple times in the past. Finding how to run Smatch isn't the problem, the thing is that Smatch either isn't packaged by distros or packaged version is outdated, hence there is a need to maintain it by yourself. Also, is it guaranteed that Smatch will always work properly with linux-next? I imagine more developers could start to engage in using Smatch if kernel supported 'make smatch' command which would automate the process of fetching, building and running Smatch. Couldn't the "kernel" version of Smatch reside in the kernel's tools/? Or maybe just the parts of Smatch that are necessary for kernel checking, like kernel's DB/scripts and etc. Doesn't it make sense?
On Thu, Nov 18, 2021 at 04:56:38PM +0300, Dmitry Osipenko wrote: > 18.11.2021 09:14, Dan Carpenter пишет: > > It's not hard to run Smatch yourself... > > > > Depending on if you're on a apt distro or yum distro then fetch the > > dependencies with one of the follow commands: > > apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl > > yum install gcc make sqlite3 sqlite-devel sqlite perl-DBD-SQLite openssl-devel perl-Try-Tiny > > > > git clone https://github.com/error27/smatch > > cd smatch > > make > > cd ~/kernel_source/ > > ~/smatch/smatch_scripts/kchecker drivers/subsystem/ > > Thanks, I was running Smatch couple times in the past. Finding how to > run Smatch isn't the problem, the thing is that Smatch either isn't > packaged by distros or packaged version is outdated, hence there is a > need to maintain it by yourself. > > Also, is it guaranteed that Smatch will always work properly with > linux-next? I work against linux-next every day so generally, yes. But that reminds me that linux-next broke while I was on vacation and I haven't yet pushed the fixes. > > I imagine more developers could start to engage in using Smatch if > kernel supported 'make smatch' command which would automate the process > of fetching, building and running Smatch. > > Couldn't the "kernel" version of Smatch reside in the kernel's tools/? > Or maybe just the parts of Smatch that are necessary for kernel > checking, like kernel's DB/scripts and etc. Doesn't it make sense? I'm not sure that makes sense really... I'll expand on that in a bit but the shorter answer is also that I don't have the bandwidth to make it work. I just suck at releases and testing. So this would bitrot and be horrible. Smatch does need a better way to manage data for other projects. Right now linux-next is the first class citizen. It's the only thing where I'm positive that it gets tested regularly. All the data in smatch_data/ is from linux-next. And also there should be a better way to check specific version of the kernel because people quite often use the same directory and just check out v4.12 to test that and switch back. I do that and I've got scripts on my system ./switch_to_tree4v1.sh which set up the symlinks for me. But for linux-next it's fine. Also by the time kernels have been released the remaining Smatch warnings are almost all false positives. To me the data in smatch_data/ is not so important as the cross function database. And the cross function database can't be distributed. It's too huge and it's specific to a given .config. regards, dan carpenter
19.11.2021 15:30, Dan Carpenter пишет: > On Thu, Nov 18, 2021 at 04:56:38PM +0300, Dmitry Osipenko wrote: >> 18.11.2021 09:14, Dan Carpenter пишет: >>> It's not hard to run Smatch yourself... >>> >>> Depending on if you're on a apt distro or yum distro then fetch the >>> dependencies with one of the follow commands: >>> apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl >>> yum install gcc make sqlite3 sqlite-devel sqlite perl-DBD-SQLite openssl-devel perl-Try-Tiny >>> >>> git clone https://github.com/error27/smatch >>> cd smatch >>> make >>> cd ~/kernel_source/ >>> ~/smatch/smatch_scripts/kchecker drivers/subsystem/ >> >> Thanks, I was running Smatch couple times in the past. Finding how to >> run Smatch isn't the problem, the thing is that Smatch either isn't >> packaged by distros or packaged version is outdated, hence there is a >> need to maintain it by yourself. >> >> Also, is it guaranteed that Smatch will always work properly with >> linux-next? > > I work against linux-next every day so generally, yes. But that reminds > me that linux-next broke while I was on vacation and I haven't yet > pushed the fixes. > >> >> I imagine more developers could start to engage in using Smatch if >> kernel supported 'make smatch' command which would automate the process >> of fetching, building and running Smatch. >> >> Couldn't the "kernel" version of Smatch reside in the kernel's tools/? >> Or maybe just the parts of Smatch that are necessary for kernel >> checking, like kernel's DB/scripts and etc. Doesn't it make sense? > > I'm not sure that makes sense really... I'll expand on that in a bit > but the shorter answer is also that I don't have the bandwidth to make > it work. I just suck at releases and testing. So this would bitrot and > be horrible. > > Smatch does need a better way to manage data for other projects. Right > now linux-next is the first class citizen. It's the only thing where > I'm positive that it gets tested regularly. All the data in > smatch_data/ is from linux-next. > > And also there should be a better way to check specific version of the > kernel because people quite often use the same directory and just check > out v4.12 to test that and switch back. I do that and I've got scripts > on my system ./switch_to_tree4v1.sh which set up the symlinks for me. > > But for linux-next it's fine. Also by the time kernels have been > released the remaining Smatch warnings are almost all false positives. > > To me the data in smatch_data/ is not so important as the cross function > database. And the cross function database can't be distributed. It's > too huge and it's specific to a given .config. Thank you for the clarification.
diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c index 859f60a70904..42da57f191ef 100644 --- a/drivers/staging/media/tegra-vde/vde.c +++ b/drivers/staging/media/tegra-vde/vde.c @@ -85,6 +85,92 @@ static int tegra_vde_wait_mbe(struct tegra_vde *vde) (tmp >= 0x10), 1, 100); } +static struct tegra_vde_bo * +tegra_vde_alloc_bo(struct tegra_vde *vde, enum dma_data_direction dma_dir, + size_t size) +{ + struct device *dev = vde->miscdev.parent; + struct tegra_vde_bo *bo; + int err; + + bo = kzalloc(sizeof(*bo), GFP_KERNEL); + if (!bo) + return NULL; + + bo->vde = vde; + bo->size = size; + bo->dma_dir = dma_dir; + bo->dma_attrs = DMA_ATTR_WRITE_COMBINE | + DMA_ATTR_NO_KERNEL_MAPPING; + + if (!vde->domain) + bo->dma_attrs |= DMA_ATTR_FORCE_CONTIGUOUS; + + bo->dma_cookie = dma_alloc_attrs(dev, bo->size, &bo->dma_handle, + GFP_KERNEL, bo->dma_attrs); + if (!bo->dma_cookie) { + dev_err(dev, "Failed to allocate DMA buffer of size: %zu\n", + bo->size); + goto free_bo; + } + + err = dma_get_sgtable_attrs(dev, &bo->sgt, bo->dma_cookie, + bo->dma_handle, bo->size, bo->dma_attrs); + if (err) { + dev_err(dev, "Failed to get DMA buffer SG table: %d\n", err); + goto free_attrs; + } + + err = dma_map_sgtable(dev, &bo->sgt, bo->dma_dir, bo->dma_attrs); + if (err) { + dev_err(dev, "Failed to map DMA buffer SG table: %d\n", err); + goto free_table; + } + + if (vde->domain) { + err = tegra_vde_iommu_map(vde, &bo->sgt, &bo->iova, bo->size); + if (err) { + dev_err(dev, "Failed to map DMA buffer IOVA: %d\n", err); + goto unmap_sgtable; + } + + bo->dma_addr = iova_dma_addr(&vde->iova, bo->iova); + } else { + bo->dma_addr = sg_dma_address(bo->sgt.sgl); + } + + return bo; + +unmap_sgtable: + dma_unmap_sgtable(dev, &bo->sgt, bo->dma_dir, bo->dma_attrs); +free_table: + sg_free_table(&bo->sgt); +free_attrs: + dma_free_attrs(dev, bo->size, bo->dma_cookie, bo->dma_handle, + bo->dma_attrs); +free_bo: + kfree(bo); + + return NULL; +} + +static void tegra_vde_free_bo(struct tegra_vde_bo *bo) +{ + struct tegra_vde *vde = bo->vde; + struct device *dev = vde->miscdev.parent; + + if (vde->domain) + tegra_vde_iommu_unmap(vde, bo->iova); + + dma_unmap_sgtable(dev, &bo->sgt, bo->dma_dir, bo->dma_attrs); + + sg_free_table(&bo->sgt); + + dma_free_attrs(dev, bo->size, bo->dma_cookie, bo->dma_handle, + bo->dma_attrs); + kfree(bo); +} + static int tegra_vde_setup_mbe_frame_idx(struct tegra_vde *vde, unsigned int refs_nb, bool setup_refs) @@ -425,6 +511,9 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde, tegra_vde_writel(vde, bitstream_data_addr, vde->sxe, 0x6C); + if (vde->soc->supports_ref_pic_marking) + tegra_vde_writel(vde, vde->secure_bo->dma_addr, vde->sxe, 0x7c); + value = 0x10000005; value |= ctx->pic_width_in_mbs << 11; value |= ctx->pic_height_in_mbs << 3; @@ -994,6 +1083,8 @@ static int tegra_vde_probe(struct platform_device *pdev) platform_set_drvdata(pdev, vde); + vde->soc = of_device_get_match_data(&pdev->dev); + vde->sxe = devm_platform_ioremap_resource_byname(pdev, "sxe"); if (IS_ERR(vde->sxe)) return PTR_ERR(vde->sxe); @@ -1119,6 +1210,12 @@ static int tegra_vde_probe(struct platform_device *pdev) pm_runtime_put(dev); + vde->secure_bo = tegra_vde_alloc_bo(vde, DMA_FROM_DEVICE, 4096); + if (!vde->secure_bo) { + dev_err(dev, "Failed to allocate secure BO\n"); + goto err_pm_runtime; + } + return 0; err_pm_runtime: @@ -1142,6 +1239,8 @@ static int tegra_vde_remove(struct platform_device *pdev) struct tegra_vde *vde = platform_get_drvdata(pdev); struct device *dev = &pdev->dev; + tegra_vde_free_bo(vde->secure_bo); + /* * As it increments RPM usage_count even on errors, we don't need to * check the returned code here. @@ -1214,8 +1313,27 @@ static const struct dev_pm_ops tegra_vde_pm_ops = { tegra_vde_pm_resume) }; +static const struct tegra_vde_soc tegra124_vde_soc = { + .supports_ref_pic_marking = true, +}; + +static const struct tegra_vde_soc tegra114_vde_soc = { + .supports_ref_pic_marking = true, +}; + +static const struct tegra_vde_soc tegra30_vde_soc = { + .supports_ref_pic_marking = false, +}; + +static const struct tegra_vde_soc tegra20_vde_soc = { + .supports_ref_pic_marking = false, +}; + static const struct of_device_id tegra_vde_of_match[] = { - { .compatible = "nvidia,tegra20-vde", }, + { .compatible = "nvidia,tegra124-vde", .data = &tegra124_vde_soc }, + { .compatible = "nvidia,tegra114-vde", .data = &tegra114_vde_soc }, + { .compatible = "nvidia,tegra30-vde", .data = &tegra30_vde_soc }, + { .compatible = "nvidia,tegra20-vde", .data = &tegra20_vde_soc }, { }, }; MODULE_DEVICE_TABLE(of, tegra_vde_of_match); diff --git a/drivers/staging/media/tegra-vde/vde.h b/drivers/staging/media/tegra-vde/vde.h index 5561291b0c88..bbd42b8d9991 100644 --- a/drivers/staging/media/tegra-vde/vde.h +++ b/drivers/staging/media/tegra-vde/vde.h @@ -24,6 +24,22 @@ struct iommu_domain; struct reset_control; struct dma_buf_attachment; +struct tegra_vde_soc { + bool supports_ref_pic_marking; +}; + +struct tegra_vde_bo { + struct iova *iova; + struct sg_table sgt; + struct tegra_vde *vde; + enum dma_data_direction dma_dir; + unsigned long dma_attrs; + dma_addr_t dma_handle; + dma_addr_t dma_addr; + void *dma_cookie; + size_t size; +}; + struct tegra_vde { void __iomem *sxe; void __iomem *bsev; @@ -48,6 +64,8 @@ struct tegra_vde { struct iova_domain iova; struct iova *iova_resv_static_addresses; struct iova *iova_resv_last_page; + const struct tegra_vde_soc *soc; + struct tegra_vde_bo *secure_bo; dma_addr_t iram_lists_addr; u32 *iram; };