Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

5 participants
2544 discussions

[PATCH v2 00/21] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers mainly in the DRM subsystem. Too bad in most cases it even worked correctly if the system used simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there. As a follow-up for this patchset I will prepare a common dma_{map,sync,unmap}_sgtable() wrappers, which will use a proper sg_table entries and call respective DMA-mapping functions. I hope this will help to avoid issue like this in the future. Patches are based on top of Linux next-20200504. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt Changelog: v2: - dropped most of the changes to drm/i915 - added fixes for rcar-du, xen, media and ion - fixed a few issues pointed by kbuild test robot - added wide cc: list for each patch v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - initial version Patch summary: Marek Szyprowski (21): drm: core: fix sg_table nents vs. orig_nents misuse drm: amdgpu: fix sg_table nents vs. orig_nents misuse drm: armada: fix sg_table nents vs. orig_nents misuse drm: etnaviv: fix sg_table nents vs. orig_nents misuse drm: exynos: fix sg_table nents vs. orig_nents misuse drm: i915: fix sg_table nents vs. orig_nents misuse for dmabuf objects drm: lima: fix sg_table nents vs. orig_nents misuse drm: msm: fix sg_table nents vs. orig_nents misuse drm: panfrost: fix sg_table nents vs. orig_nents misuse drm: radeon: fix sg_table nents vs. orig_nents misuse drm: rockchip: fix sg_table nents vs. orig_nents misuse drm: tegra: fix sg_table nents vs. orig_nents misuse drm: virtio: fix sg_table nents vs. orig_nents misuse drm: vmwgfx: fix sg_table nents vs. orig_nents misuse drm: xen: fix sg_table nents vs. orig_nents misuse drm: host1x: fix sg_table nents vs. orig_nents misuse drm: rcar-du: fix sg_table nents vs. orig_nents misuse xen: gntdev: fix sg_table nents vs. orig_nents misuse dmabuf: fix sg_table nents vs. orig_nents misuse media: pci: fix common ALSA DMA-mapping related code staging: ion: fix sg_table nents vs. orig_nents misuse drivers/dma-buf/heaps/heap-helpers.c | 7 ++++--- drivers/dma-buf/udmabuf.c | 5 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 7 ++++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++---- drivers/gpu/drm/armada/armada_gem.c | 14 ++++++++----- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- drivers/gpu/drm/drm_prime.c | 9 +++++---- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 10 ++++++---- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 7 ++++--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 +++++---- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 5 +++-- drivers/gpu/drm/lima/lima_gem.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 8 ++++---- drivers/gpu/drm/msm/msm_iommu.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_mmu.c | 4 +++- drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++++++----- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 15 +++++++------- drivers/gpu/drm/tegra/gem.c | 25 ++++++++++++------------ drivers/gpu/drm/tegra/plane.c | 13 ++++++------ drivers/gpu/drm/virtio/virtgpu_object.c | 11 ++++++----- drivers/gpu/drm/virtio/virtgpu_vq.c | 8 ++++---- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 6 +++--- drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +- drivers/gpu/host1x/job.c | 17 ++++++++-------- drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- drivers/media/platform/vsp1/vsp1_drm.c | 7 ++++--- drivers/staging/android/ion/ion.c | 17 ++++++++-------- drivers/staging/android/ion/ion_heap.c | 6 +++--- drivers/staging/android/ion/ion_system_heap.c | 2 +- drivers/xen/gntdev-dmabuf.c | 10 ++++++---- 35 files changed, 149 insertions(+), 121 deletions(-) -- 1.9.1

4 years, 8 months

Patch "dma-buf: Fix SET_NAME ioctl uapi" has been added to the 5.6-stable tree

by gregkh＠linuxfoundation.org

This is a note to let you know that I've just added the patch titled dma-buf: Fix SET_NAME ioctl uapi to the 5.6-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… The filename of the patch is: dma-buf-fix-set_name-ioctl-uapi.patch and it can be found in the queue-5.6 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable(a)vger.kernel.org> know about it. >From a5bff92eaac45bdf6221badf9505c26792fdf99e Mon Sep 17 00:00:00 2001 From: Daniel Vetter <daniel.vetter(a)intel.com> Date: Tue, 7 Apr 2020 15:30:02 +0200 Subject: dma-buf: Fix SET_NAME ioctl uapi From: Daniel Vetter <daniel.vetter(a)intel.com> commit a5bff92eaac45bdf6221badf9505c26792fdf99e upstream. The uapi is the same on 32 and 64 bit, but the number isn't. Everyone who botched this please re-read: https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.ht… Also, the type argument for the ioctl macros is for the type the void __user *arg pointer points at, which in this case would be the variable-sized char[] of a 0 terminated string. So this was botched in more than just the usual ways. Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Chenbo Feng <fengc(a)google.com> Cc: Greg Hackmann <ghackmann(a)google.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: minchan(a)kernel.org Cc: surenb(a)google.com Cc: jenhaochen(a)google.com Cc: Martin Liu <liumartin(a)google.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Tested-by: Martin Liu <liumartin(a)google.com> Reviewed-by: Martin Liu <liumartin(a)google.com> Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org> [sumits: updated some checkpatch fixes, corrected author email] Link: https://patchwork.freedesktop.org/patch/msgid/20200407133002.3486387-1-dani… Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/dma-buf/dma-buf.c | 3 ++- include/uapi/linux/dma-buf.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -388,7 +388,8 @@ static long dma_buf_ioctl(struct file *f return ret; - case DMA_BUF_SET_NAME: + case DMA_BUF_SET_NAME_A: + case DMA_BUF_SET_NAME_B: return dma_buf_set_name(dmabuf, (const char __user *)arg); default: --- a/include/uapi/linux/dma-buf.h +++ b/include/uapi/linux/dma-buf.h @@ -39,6 +39,12 @@ struct dma_buf_sync { #define DMA_BUF_BASE 'b' #define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) + +/* 32/64bitness of this uapi was botched in android, there's no difference + * between them in actual uapi, they're just different numbers. + */ #define DMA_BUF_SET_NAME _IOW(DMA_BUF_BASE, 1, const char *) +#define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32) +#define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64) #endif Patches currently in stable-queue which might be from daniel.vetter(a)intel.com are queue-5.6/dma-buf-fix-set_name-ioctl-uapi.patch

4 years, 8 months

Patch "dma-buf: Fix SET_NAME ioctl uapi" has been added to the 5.4-stable tree

by gregkh＠linuxfoundation.org

This is a note to let you know that I've just added the patch titled dma-buf: Fix SET_NAME ioctl uapi to the 5.4-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… The filename of the patch is: dma-buf-fix-set_name-ioctl-uapi.patch and it can be found in the queue-5.4 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable(a)vger.kernel.org> know about it. >From a5bff92eaac45bdf6221badf9505c26792fdf99e Mon Sep 17 00:00:00 2001 From: Daniel Vetter <daniel.vetter(a)intel.com> Date: Tue, 7 Apr 2020 15:30:02 +0200 Subject: dma-buf: Fix SET_NAME ioctl uapi From: Daniel Vetter <daniel.vetter(a)intel.com> commit a5bff92eaac45bdf6221badf9505c26792fdf99e upstream. The uapi is the same on 32 and 64 bit, but the number isn't. Everyone who botched this please re-read: https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.ht… Also, the type argument for the ioctl macros is for the type the void __user *arg pointer points at, which in this case would be the variable-sized char[] of a 0 terminated string. So this was botched in more than just the usual ways. Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Chenbo Feng <fengc(a)google.com> Cc: Greg Hackmann <ghackmann(a)google.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: minchan(a)kernel.org Cc: surenb(a)google.com Cc: jenhaochen(a)google.com Cc: Martin Liu <liumartin(a)google.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Tested-by: Martin Liu <liumartin(a)google.com> Reviewed-by: Martin Liu <liumartin(a)google.com> Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org> [sumits: updated some checkpatch fixes, corrected author email] Link: https://patchwork.freedesktop.org/patch/msgid/20200407133002.3486387-1-dani… Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/dma-buf/dma-buf.c | 3 ++- include/uapi/linux/dma-buf.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -388,7 +388,8 @@ static long dma_buf_ioctl(struct file *f return ret; - case DMA_BUF_SET_NAME: + case DMA_BUF_SET_NAME_A: + case DMA_BUF_SET_NAME_B: return dma_buf_set_name(dmabuf, (const char __user *)arg); default: --- a/include/uapi/linux/dma-buf.h +++ b/include/uapi/linux/dma-buf.h @@ -39,6 +39,12 @@ struct dma_buf_sync { #define DMA_BUF_BASE 'b' #define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) + +/* 32/64bitness of this uapi was botched in android, there's no difference + * between them in actual uapi, they're just different numbers. + */ #define DMA_BUF_SET_NAME _IOW(DMA_BUF_BASE, 1, const char *) +#define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32) +#define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64) #endif Patches currently in stable-queue which might be from daniel.vetter(a)intel.com are queue-5.4/dma-buf-fix-set_name-ioctl-uapi.patch

4 years, 8 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Documentation: fix: `make htmldocs` warnings

by Sam Ravnborg

On Wed, Apr 29, 2020 at 11:27:22PM -0300, Vitor Massaru Iha wrote: > On Wed, 2020-04-29 at 19:06 -0700, Randy Dunlap wrote: > > On 4/29/20 6:59 PM, Vitor Massaru Iha wrote: > > > Add missed ":" on kernel-doc function parameter. > > > > > > This patch fixes this warnings from `make htmldocs`: > > > ./drivers/dma-buf/dma-buf.c:678: warning: Function parameter or > > > member 'importer_ops' not described in 'dma_buf_dynamic_attach' > > > ./drivers/dma-buf/dma-buf.c:678: warning: Function parameter or > > > member 'importer_priv' not described in 'dma_buf_dynamic_attach' > > > > > > Signed-off-by: Vitor Massaru Iha <vitor(a)massaru.org> > > > --- > > > drivers/dma-buf/dma-buf.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > > index ccc9eda1bc28..0756d2155745 100644 > > > --- a/drivers/dma-buf/dma-buf.c > > > +++ b/drivers/dma-buf/dma-buf.c > > > @@ -655,8 +655,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); > > > * calls attach() of dma_buf_ops to allow device-specific attach > > > functionality > > > * @dmabuf: [in] buffer to attach device to. > > > * @dev: [in] device to be attached. > > > - * @importer_ops [in] importer operations for the > > > attachment > > > - * @importer_priv [in] importer private pointer for the > > > attachment > > > + * @importer_ops: [in] importer operations for the > > > attachment > > > + * @importer_priv: [in] importer private pointer for the > > > attachment > > > * > > > * Returns struct dma_buf_attachment pointer for this attachment. > > > Attachments > > > * must be cleaned up by calling dma_buf_detach(). > > > > > > > Sumit said that he would be applying my patch from April 7: > > https://lore.kernel.org/linux-media/7bcbe6fe-0b4b-87da-d003-b68a26eb4cf0@in… > > > > thanks. > > Sorry. I didn't check if the patch has already been sent. Sumit - patch from Randy is neither applied to drm-misc-next nor drm-misc-fixes. A reminder in case it was lost somewhere. Sam

4 years, 8 months

Re: [Linaro-mm-sig] [Intel-gfx] [RFC 06/17] drm: i915: fix sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Hi On 28.04.2020 16:27, Tvrtko Ursulin wrote: > > On 28/04/2020 14:19, Marek Szyprowski wrote: >> The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the >> numer of the created entries in the DMA address space. However the >> subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg >> must be >> called with the original number of entries passed to dma_map_sg. The >> sg_table->nents in turn holds the result of the dma_map_sg call as >> stated >> in include/linux/scatterlist.h. Adapt the code to obey those rules. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski(a)samsung.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 13 +++++++------ >> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 ++-- >> drivers/gpu/drm/i915/gem/i915_gem_region.c | 4 ++-- >> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 5 +++-- >> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 10 +++++----- >> drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 5 +++-- >> drivers/gpu/drm/i915/gt/intel_ggtt.c | 12 ++++++------ >> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 +++++++----- >> drivers/gpu/drm/i915/i915_scatterlist.c | 4 ++-- >> drivers/gpu/drm/i915/selftests/scatterlist.c | 8 ++++---- >> 10 files changed, 41 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> index 7db5a79..d829852 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> @@ -36,21 +36,22 @@ static struct sg_table >> *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme >> goto err_unpin_pages; >> } >> - ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); >> + ret = sg_alloc_table(st, obj->mm.pages->orig_nents, GFP_KERNEL); >> if (ret) >> goto err_free; >> src = obj->mm.pages->sgl; >> dst = st->sgl; >> - for (i = 0; i < obj->mm.pages->nents; i++) { >> + for (i = 0; i < obj->mm.pages->orig_nents; i++) { >> sg_set_page(dst, sg_page(src), src->length, 0); >> dst = sg_next(dst); >> src = sg_next(src); >> } >> - if (!dma_map_sg_attrs(attachment->dev, >> - st->sgl, st->nents, dir, >> - DMA_ATTR_SKIP_CPU_SYNC)) { >> + st->nents = dma_map_sg_attrs(attachment->dev, >> + st->sgl, st->orig_nents, dir, >> + DMA_ATTR_SKIP_CPU_SYNC); >> + if (!st->nents) { >> ret = -ENOMEM; >> goto err_free_sg; >> } >> @@ -74,7 +75,7 @@ static void i915_gem_unmap_dma_buf(struct >> dma_buf_attachment *attachment, >> struct drm_i915_gem_object *obj = >> dma_buf_to_obj(attachment->dmabuf); >> dma_unmap_sg_attrs(attachment->dev, >> - sg->sgl, sg->nents, dir, >> + sg->sgl, sg->orig_nents, dir, >> DMA_ATTR_SKIP_CPU_SYNC); >> sg_free_table(sg); >> kfree(sg); >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c >> b/drivers/gpu/drm/i915/gem/i915_gem_internal.c >> index cbbff81..a8ebfdd 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c >> @@ -73,7 +73,7 @@ static int >> i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) >> } >> sg = st->sgl; >> - st->nents = 0; >> + st->nents = st->orig_nents = 0; >> sg_page_sizes = 0; >> do { >> @@ -94,7 +94,7 @@ static int >> i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) >> sg_set_page(sg, page, PAGE_SIZE << order, 0); >> sg_page_sizes |= PAGE_SIZE << order; >> - st->nents++; >> + st->nents = st->orig_nents = st->nents + 1; >> npages -= 1 << order; >> if (!npages) { >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c >> b/drivers/gpu/drm/i915/gem/i915_gem_region.c >> index 1515384..58ca560 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c >> @@ -53,7 +53,7 @@ >> GEM_BUG_ON(list_empty(blocks)); >> sg = st->sgl; >> - st->nents = 0; >> + st->nents = st->orig_nents = 0; >> sg_page_sizes = 0; >> prev_end = (resource_size_t)-1; >> @@ -78,7 +78,7 @@ >> sg->length = block_size; >> - st->nents++; >> + st->nents = st->orig_nents = st->nents + 1; >> } else { >> sg->length += block_size; >> sg_dma_len(sg) += block_size; >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> index 5d5d7ee..851a732 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> @@ -80,7 +80,7 @@ static int shmem_get_pages(struct >> drm_i915_gem_object *obj) >> noreclaim |= __GFP_NORETRY | __GFP_NOWARN; >> sg = st->sgl; >> - st->nents = 0; >> + st->nents = st->orig_nents = 0; >> sg_page_sizes = 0; >> for (i = 0; i < page_count; i++) { >> const unsigned int shrink[] = { >> @@ -140,7 +140,8 @@ static int shmem_get_pages(struct >> drm_i915_gem_object *obj) >> sg_page_sizes |= sg->length; >> sg = sg_next(sg); >> } >> - st->nents++; >> + st->nents = st->orig_nents = st->nents + 1; > > A bit higher up, not shown in the patch, we have allocated a table via > sg_alloc_table giving it a pessimistic max nents, sometimes much > larger than the st->nents this loops will create. But orig_nents has > been now been overwritten. Will that leak memory come sg_table_free? Indeed this will leak memory. I'm missed that sg_trim() will adjust nents and orig_nents. I will drop those changes as they are only a creative (or hacky) way of using sg_table and scatterlists. > As minimum it will nerf our i915_sg_trim optimization a bit lower > down, also not shown in the diff. > > > [...] Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland

4 years, 8 months

Re: [Linaro-mm-sig] [PATCH] drm/radeon: drm/amdgpu: Disable [1002:6611] in radeon

by Alex Deucher

On Thu, Apr 30, 2020 at 9:08 AM Jian-Hong Pan <jian-hong(a)endlessm.com> wrote: > > The AMD/ATI Oland [1002:6611]'s HDMI output status is not synchronous > as shown on UI after hot re-plug the HDMI cable, if it is radeon in > used. The amdgpu module does not hit this issue. > > This patch disables [1002:6611] in radeon and enables it in amdgpu. > > Fixes: https://gitlab.freedesktop.org/drm/amd/-/issues/1117 > Signed-off-by: Jian-Hong Pan <jian-hong(a)endlessm.com> Nack. Amdgpu does not have support for VCE or UVD yet so you are just trading one issue for another. It would be better to figure out why the audio is not updated properly in some cases. Alex > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 11 +++++++++++ > include/drm/drm_pciids.h | 1 - > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index 8ea86ffdea0d..1ad6f13a5bc0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > @@ -1017,6 +1017,15 @@ MODULE_DEVICE_TABLE(pci, pciidlist); > > static struct drm_driver kms_driver; > > +static void amdgpu_pci_fixup(struct pci_dev *pdev) > +{ > +#ifdef CONFIG_DRM_AMDGPU_SI > + /* [1002:6611] is disabled in radeon, so enable si_support in amdgpu. */ > + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x6611) > + amdgpu_si_support = 1; > +#endif > +} > + > static int amdgpu_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > { > @@ -1036,6 +1045,8 @@ static int amdgpu_pci_probe(struct pci_dev *pdev, > return -ENODEV; > } > > + amdgpu_pci_fixup(pdev); > + > #ifdef CONFIG_DRM_AMDGPU_SI > if (!amdgpu_si_support) { > switch (flags & AMD_ASIC_MASK) { > diff --git a/include/drm/drm_pciids.h b/include/drm/drm_pciids.h > index b7e899ce44f0..57368a0f5b82 100644 > --- a/include/drm/drm_pciids.h > +++ b/include/drm/drm_pciids.h > @@ -171,7 +171,6 @@ > {0x1002, 0x6607, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_IS_MOBILITY|RADEON_NEW_MEMMAP}, \ > {0x1002, 0x6608, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_NEW_MEMMAP}, \ > {0x1002, 0x6610, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_NEW_MEMMAP}, \ > - {0x1002, 0x6611, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_NEW_MEMMAP}, \ > {0x1002, 0x6613, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_NEW_MEMMAP}, \ > {0x1002, 0x6617, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_IS_MOBILITY|RADEON_NEW_MEMMAP}, \ > {0x1002, 0x6620, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_OLAND|RADEON_IS_MOBILITY|RADEON_NEW_MEMMAP}, \ > -- > 2.26.2 > > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

4 years, 8 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Documentation: fix: `make htmldocs` warnings

by Randy Dunlap

On 4/29/20 6:59 PM, Vitor Massaru Iha wrote: > Add missed ":" on kernel-doc function parameter. > > This patch fixes this warnings from `make htmldocs`: > ./drivers/dma-buf/dma-buf.c:678: warning: Function parameter or member 'importer_ops' not described in 'dma_buf_dynamic_attach' > ./drivers/dma-buf/dma-buf.c:678: warning: Function parameter or member 'importer_priv' not described in 'dma_buf_dynamic_attach' > > Signed-off-by: Vitor Massaru Iha <vitor(a)massaru.org> > --- > drivers/dma-buf/dma-buf.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index ccc9eda1bc28..0756d2155745 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -655,8 +655,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); > * calls attach() of dma_buf_ops to allow device-specific attach functionality > * @dmabuf: [in] buffer to attach device to. > * @dev: [in] device to be attached. > - * @importer_ops [in] importer operations for the attachment > - * @importer_priv [in] importer private pointer for the attachment > + * @importer_ops: [in] importer operations for the attachment > + * @importer_priv: [in] importer private pointer for the attachment > * > * Returns struct dma_buf_attachment pointer for this attachment. Attachments > * must be cleaned up by calling dma_buf_detach(). > Sumit said that he would be applying my patch from April 7: https://lore.kernel.org/linux-media/7bcbe6fe-0b4b-87da-d003-b68a26eb4cf0@in… thanks. -- ~Randy

4 years, 8 months

[RFC 00/17] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents in the DRM subsystem and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers. Too bad in most cases it even worked correctly if the system used simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. I wonder what to do to avoid such misuse in the future, as this case clearly shows that the current sg_table structure is a bit hard to understand. I have the following ideas and I would like to know your opinion before I prepare more patches and check sg_table usage all over the kernel: 1. introduce a dma_{map,sync,unmap}_sgtable() wrappers, which will use a proper sg_table entries and call respective DMA-mapping functions and adapt current code to it 2. rename nents and orig_nents to nr_pages, nr_dmas to clearly state which one refers to which part of the scatterlist; I'm open for other names for those entries I will appreciate your comments. Patches are based on top of Linux next-20200428. I've reduced the recipients list mainly to mailing lists, the next version I will try to send to the maintainers of the respective drivers. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt Patch summary: Marek Szyprowski (17): drm: core: fix sg_table nents vs. orig_nents usage drm: amdgpu: fix sg_table nents vs. orig_nents usage drm: armada: fix sg_table nents vs. orig_nents usage drm: etnaviv: fix sg_table nents vs. orig_nents usage drm: exynos: fix sg_table nents vs. orig_nents usage drm: i915: fix sg_table nents vs. orig_nents usage drm: lima: fix sg_table nents vs. orig_nents usage drm: msm: fix sg_table nents vs. orig_nents usage drm: panfrost: fix sg_table nents vs. orig_nents usage drm: radeon: fix sg_table nents vs. orig_nents usage drm: rockchip: fix sg_table nents vs. orig_nents usage drm: tegra: fix sg_table nents vs. orig_nents usage drm: virtio: fix sg_table nents vs. orig_nents usage drm: vmwgfx: fix sg_table nents vs. orig_nents usage drm: xen: fix sg_table nents vs. orig_nents usage drm: host1x: fix sg_table nents vs. orig_nents usage dmabuf: fix sg_table nents vs. orig_nents usage drivers/dma-buf/heaps/heap-helpers.c | 7 ++++--- drivers/dma-buf/udmabuf.c | 5 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 7 ++++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++---- drivers/gpu/drm/armada/armada_gem.c | 14 ++++++++----- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- drivers/gpu/drm/drm_prime.c | 9 +++++---- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 10 ++++++---- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 7 ++++--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 13 ++++++------ drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 ++-- drivers/gpu/drm/i915/gem/i915_gem_region.c | 4 ++-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 5 +++-- drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 10 +++++----- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 5 +++-- drivers/gpu/drm/i915/gt/intel_ggtt.c | 12 ++++++------ drivers/gpu/drm/i915/i915_gem_gtt.c | 12 +++++++----- drivers/gpu/drm/i915/i915_scatterlist.c | 4 ++-- drivers/gpu/drm/i915/selftests/scatterlist.c | 8 ++++---- drivers/gpu/drm/lima/lima_gem.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 8 ++++---- drivers/gpu/drm/msm/msm_iommu.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_mmu.c | 4 +++- drivers/gpu/drm/radeon/radeon_ttm.c | 10 +++++----- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 15 +++++++------- drivers/gpu/drm/tegra/gem.c | 25 ++++++++++++------------ drivers/gpu/drm/tegra/plane.c | 13 ++++++------ drivers/gpu/drm/virtio/virtgpu_object.c | 11 ++++++----- drivers/gpu/drm/virtio/virtgpu_vq.c | 8 ++++---- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 6 +++--- drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +- drivers/gpu/host1x/job.c | 17 ++++++++-------- 34 files changed, 154 insertions(+), 128 deletions(-) -- 1.9.1

4 years, 8 months

[PATCH] dma-buf: Fix SET_NAME ioctl uapi

by Daniel Vetter

The uapi is the same on 32 and 64 bit, but the number isnt. Everyone who botched this please re-read: https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.ht… Also, the type argument for the ioctl macros is for the type the void __user *arg pointer points at, which in this case would be the variable-sized char[] of a 0 terminated string. So this was botched in more than just the usual ways. Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Chenbo Feng <fengc(a)google.com> Cc: Greg Hackmann <ghackmann(a)google.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: minchan(a)kernel.org Cc: surenb(a)google.com Cc: jenhaochen(a)google.com Cc: Martin Liu <liumartin(a)google.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/dma-buf/dma-buf.c | 3 ++- include/uapi/linux/dma-buf.h | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 570c923023e6..1d923b8e4c59 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -388,7 +388,8 @@ static long dma_buf_ioctl(struct file *file, return ret; - case DMA_BUF_SET_NAME: + case DMA_BUF_SET_NAME_A: + case DMA_BUF_SET_NAME_B: return dma_buf_set_name(dmabuf, (const char __user *)arg); default: diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h index dbc7092e04b5..21dfac815dc0 100644 --- a/include/uapi/linux/dma-buf.h +++ b/include/uapi/linux/dma-buf.h @@ -39,6 +39,10 @@ struct dma_buf_sync { #define DMA_BUF_BASE 'b' #define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) +/* 32/64bitness of this uapi was botched in android, there's no difference + * between them in actual uapi, they're just different numbers. */ #define DMA_BUF_SET_NAME _IOW(DMA_BUF_BASE, 1, const char *) +#define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32) +#define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64) #endif -- 2.25.1

4 years, 8 months

Re: [Linaro-mm-sig] [PATCH] drm/amdgpu: remove conversion to bool in amdgpu_device.c

by Christian König

Am 27.04.20 um 08:36 schrieb Jason Yan: > The '>' expression itself is bool, no need to convert it to bool again. > This fixes the following coccicheck warning: > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:3004:68-73: WARNING: > conversion to bool not needed here > > Signed-off-by: Jason Yan <yanaijie(a)huawei.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 3d601d5dd5af..ad94de3632d8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -3000,7 +3000,7 @@ int amdgpu_device_init(struct amdgpu_device *adev, > INIT_WORK(&adev->xgmi_reset_work, amdgpu_device_xgmi_reset_func); > > adev->gfx.gfx_off_req_count = 1; > - adev->pm.ac_power = power_supply_is_system_supplied() > 0 ? true : false; > + adev->pm.ac_power = power_supply_is_system_supplied() > 0; > > /* Registers mapping */ > /* TODO: block userspace mapping of io register */

4 years, 8 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig