Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

22 participants
7055 discussions

Re: [PATCH v2 00/15] accel/qda: Qualcomm DSP Accelerator driver

by Krzysztof Kozlowski

On 19/08/2026 17:48, Rob Clark wrote: > On Wed, Aug 19, 2026 at 8:27 AM Krzysztof Kozlowski <krzk(a)kernel.org> wrote: >> The rule of usptream development is that we do not accept duplicated >> code, just because a vendor wants to write something new. This is >> basically the concept applied all over the drivers tree, where we pushed >> back against all sorts of duplications all over the vendors. >> >> What I miss in this thread is why would there be any exception here. We >> do not grant exceptions from standard practices on "I want" reasons. > > I agree that we should not have duplicated drivers just for vendor > lolz. But when it comes to adopting common frameworks and integrating > better into the ecosystem, this doesn't seem like something we should > actively discourage. I don't think this is a case of vendor lolz, but No one discourages it. Following standard Linux kernel practices and requirements is not discouraging, do not twist the narrative here. Again, it is standard upstream review telling that we do not duplicate drivers. Ever, unless there is serious exception needed. I asked why there should be an exception granted? Is the reason for exception following: "We want to adopt common framework" ? > rather reacting to drm/accel emerging as the standard framework for > this sort of driver. > > So how do we get from here to there? What is wrong with my proposal? Best regards, Krzysztof

29 minutes

Re: [PATCH v4 5/5] accel/amdxdna: do not fail a sync for a BO with no debug context

by Lizhi Hou

On 8/17/26 16:07, Taimuraz Kaitmazov wrote: > amdxdna_drm_sync_bo_ioctl() calls amdxdna_hwctx_sync_debug_bo() for every > FROM_DEVICE sync, which answers -EINVAL when the BO has no assigned hwctx. > Only a BO attached with ATTACH_DEBUG_BO ever gets one, so an ordinary > read-back sync reports failure after its flush has already run. > > There is no debug buffer to sync in that case, so answer success. > > Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com> > --- > drivers/accel/amdxdna/amdxdna_ctx.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c > index 855da8c79a1c..c0d0aa53c596 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c > @@ -416,7 +416,8 @@ int amdxdna_hwctx_sync_debug_bo(struct amdxdna_client *client, u32 debug_bo_hdl) > guard(mutex)(&xdna->dev_lock); > hwctx = xa_load(&client->hwctx_xa, abo->assigned_hwctx); > if (!hwctx) { > - ret = -EINVAL; > + /* Not attached as a debug BO, so there is nothing to sync. */ > + ret = 0; It should check assigned_hwctx before entering this function: if (abo->assigned_hwctx != AMDXDNA_INVALID_CTX_HANDLE && args->direction == SYNC_DIRECT_FROM_DEVICE) ret = amdxdna_hwctx_sync_debug_bo(client, args->handle); Thanks, Lizhi > goto put_obj; > } >

9 hours, 40 minutes

Re: [PATCH v4 4/5] accel/amdxdna: refuse to flush an imported BO

by Lizhi Hou

On 8/17/26 16:07, Taimuraz Kaitmazov wrote: > SYNC_BO clflushes an imported BO's scatterlist. An importer may not do > that: the memory belongs to the exporter, and dma-buf gives the importer > no interface to ask for maintenance on it. Refuse the request instead. > > is_import_bo() is (obj)->attach, which covers more than foreign buffers. > A userptr BO arrives through a ubuf, and on a carveout device every share > BO and the device heap arrive through a cbuf, so SYNC_BO answers > -EOPNOTSUPP for those too, including the AMDXDNA_BO_DEV path that flushes > through its heap. > > Only the ubuf case gives up maintenance it was getting: on a 64 MiB > userptr BO a 4 KiB sync and a full sync both cost 659 us, this arm having > ignored the range. amdxdna_cbuf_map() fills in only the DMA address and > length, so drm_clflush_sg() already walks zero pages on carveout memory. > Userspace maintains these through the mapping it already holds, as XRT's > buffer::sync() does unless it is told to sync through the driver. > > Suggested-by: Lizhi Hou <lizhi.hou(a)amd.com> > Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com> > --- > drivers/accel/amdxdna/amdxdna_gem.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c > index 4f38f985c74e..a713a9982d34 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -1224,6 +1224,9 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size) > { > u64 end; > > + if (is_import_bo(abo)) > + return -EOPNOTSUPP; > + > if (offset >= abo->mem.size) > return -EINVAL; > > @@ -1234,9 +1237,7 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size) > if (!size) > return 0; > > - if (is_import_bo(abo)) > - drm_clflush_sg(abo->base.sgt); > - else if (amdxdna_gem_vmap(abo)) > + if (amdxdna_gem_vmap(abo)) Reviewed-by: Lizhi Hou <lizhi.hou(a)amd.com> > drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size); > else if (abo->base.pages) > drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);

9 hours, 45 minutes

Re: [PATCH v4 3/5] accel/amdxdna: do not warn when a sync request is rejected

by Lizhi Hou

On 8/17/26 16:07, Taimuraz Kaitmazov wrote: > amdxdna_drm_sync_bo_ioctl() answers a failed amdxdna_flush_bo() with > drm_WARN(). Both of that function's error returns are decided by the > ioctl's arguments, so SYNC_BO with an offset past the end of the BO > splats and taints the kernel from an unprivileged caller. > > Log it like the pin failure above it. > > Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com> > --- > drivers/accel/amdxdna/amdxdna_gem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c > index 77a9493cd7ba..4f38f985c74e 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -1310,7 +1310,7 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev, > amdxdna_gem_unpin(abo); > > if (ret) { > - drm_WARN(&xdna->ddev, 1, "Can not get flush memory"); > + XDNA_ERR(xdna, "Flush BO %d failed, ret %d", args->handle, ret); Should it be XDNA_DBG? Lizhi > goto put_obj; > } > }

9 hours, 47 minutes

Re: [PATCH v4 2/5] accel/amdxdna: check the sync range for overflow on a device BO

by Lizhi Hou

On 8/17/26 16:07, Taimuraz Kaitmazov wrote: > amdxdna_drm_sync_bo_ioctl() forms the range for a device BO by adding the > caller's offset and size to the BO address without checking either, while > amdxdna_flush_bo() one call down guards the same arithmetic with > check_add_overflow(). > > A size that wraps flush_end leaves it below the heap it is clamped > against, so every heap fails the start >= end test, and a sync that asked > for more than the address space holds reports success having flushed > nothing. Reject it instead. > > Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com> > --- > drivers/accel/amdxdna/amdxdna_gem.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c > index f88b5349cd4b..77a9493cd7ba 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -1274,8 +1274,13 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev, > struct amdxdna_gem_obj *heap; > unsigned long heap_id; > u64 bo_start = amdxdna_gem_dev_addr(abo); > - u64 flush_start = bo_start + args->offset; > - u64 flush_end = flush_start + args->size; > + u64 flush_start, flush_end; > + > + if (check_add_overflow(bo_start, args->offset, &flush_start) || > + check_add_overflow(flush_start, args->size, &flush_end)) { > + ret = -EINVAL; > + goto put_obj; > + } Reviewed-by: Lizhi Hou <lizhi.hou(a)amd.com> > > xa_for_each_range(&client->dev_heap_xa, heap_id, heap, > abo->heap_start_id, abo->heap_end_id) {

9 hours, 55 minutes

Re: [PATCH v4 1/5] accel/amdxdna: refuse an I/O memory mapping of an imported BO

by Lizhi Hou

On 8/17/26 16:07, Taimuraz Kaitmazov wrote: > amdxdna_gem_vmap() flattens the iosys_map drm_gem_vmap() fills in down to > the void * in abo->mem.kva, and iosys_map is discriminated by is_iomem, so > an exporter answering with an I/O mapping leaves a void __iomem pointer > there, which amdxdna_cmd_set_error() memsets and memcpys through. > > amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's > buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to > peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has > no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident > object, so an NPU paired with nouveau or radeon does. > > Drop such a mapping and answer NULL. Checking here rather than in the > .vmap callback leaves that callback's iosys_map contract intact for a > caller equipped to read I/O memory, and covers everything that takes a > plain kernel address through this helper. vmw_gem_vmap() refuses the > same case; unlike that one this path is reachable from an unprivileged > ioctl, so it neither warns nor logs at error level. > > Signed-off-by: Taimuraz Kaitmazov <taimuraz(a)kaitmazov.com> > --- > drivers/accel/amdxdna/amdxdna_gem.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c > index cca84fa07e9d..f88b5349cd4b 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -209,10 +209,15 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo) > > if (!abo->mem.kva) { > ret = drm_gem_vmap(to_gobj(abo), &map); > - if (ret) > + if (ret) { > XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret); > - else > + } else if (map.is_iomem) { > + /* Callers use the result as an ordinary kernel address. */ > + XDNA_DBG(abo->client->xdna, "Vmap bo returned I/O memory"); > + drm_gem_vunmap(to_gobj(abo), &map); > + } else { > abo->mem.kva = map.vaddr; > + } Reviewed-by: Lizhi Hou <lizhi.hou(a)amd.com> > } > return abo->mem.kva; > }

10 hours, 5 minutes

Re: [PATCH v4 0/5] accel/amdxdna: SYNC_BO correctness fixes

by Lizhi Hou

On 8/18/26 14:40, Taimuraz Kaitmazov wrote: > Patch 1 needs a prerequisite. It makes amdxdna_gem_vmap() answer NULL > on an iomem exporter, and the eight amdxdna_cmd_get_payload() callers > in aie2_message.c check neither the pointer nor the length it leaves > unwritten there. A command BO can be an import, so patch 1 alone turns > a silent __iomem write into a NULL deref with an uninitialised length. > https://lore.kernel.org/all/20260818002459.377641-1-taimuraz@kaitmazov.com/ > amdxdna_cmd_get_op() is always called before amdxdna_cmd_get_payload() for the same BO, and since amdxdna_gem_vmap() caches its results, the vmap inside get_payload() is currently guaranteed to succeed. The patch is therefore defensive rather than fixing a currently reachable crash. Lizhi > > fixes the callers. Happy to respin on top if you prefer them together. > > Taimuraz > > On 8/18/26 02:07, Taimuraz Kaitmazov wrote: >> Five fixes in and around amdxdna_drm_sync_bo_ioctl(). >> >> Patch 1 refuses an I/O memory mapping the driver would otherwise >> store as >> if it were an ordinary kernel address. Patch 2 checks the device-BO >> range >> for overflow. Patch 4 refuses a flush of an imported BO, which is why >> patch 3 comes first: the ioctl answers a rejected flush with drm_WARN(), >> so without it an ordinary sync on an imported BO splats. Patch 5 stops >> the ioctl reporting failure for a flush that succeeded. >> >> v3's zero-length patch has left this series. On hardware it turns out to >> be a page fault in drm_clflush_virt_range() rather than the tidy-up its >> commit message described, so it is a -fixes patch now, sent >> separately as >> "accel/amdxdna: return early from a zero-length flush" with Fixes: and >> Cc: stable. Patch 4 here needs its hunk, so this series wants that one >> first. >> >> Changes in v4: >> - patch 1: the is_iomem check moved from the .vmap callback into >> amdxdna_gem_vmap(), per Lizhi, and it logs at debug level rather >> than >> error, since an unprivileged caller can repeat it. >> - new patch 3: an unprivileged SYNC_BO with an out-of-range offset >> already reaches that drm_WARN() today. Sashiko's review of v3 3/3 >> flagged the same thing. >> - new patch 4: refuses the flush for every imported BO, as asked. >> - new patch 5: the debug-BO sync I mentioned on the v2 thread. >> Only a BO >> attached with ATTACH_DEBUG_BO has an assigned hwctx, so every other >> FROM_DEVICE sync ends in -EINVAL with its flush already done. The >> -EINVAL reproduces on a Strix Point NPU. >> >> On patch 4, one consequence worth deciding before it lands. A heap BO is >> created through amdxdna_drm_create_share_bo(), so a device running on >> carveout memory reaches its heap through a cbuf, is_import_bo() is >> true of >> it, and SYNC_BO on every AMDXDNA_BO_DEV now answers -EOPNOTSUPP. Today >> that path flushes nothing anyway -- amdxdna_cbuf_map() fills in only the >> DMA address and length, so drm_clflush_sg() walks zero pages -- so the >> change is silent no-op to hard error, and XRT's dbg_buffer::sync() >> reaches >> it without Debug.force_driver_sync. If you would rather keep our own >> exporters working, I have the variant keyed on the exporter's ops, which >> confines the refusal to foreign buffers. Say which you prefer. >> >> v3: >> https://lore.kernel.org/all/20260813164700.43960-1-taimuraz@kaitmazov.com/ >> >> Built on drm-misc-next, each commit on its own: x86_64 with >> DRM_ACCEL_AMDXDNA=m, clang 22.1.8, no warnings. Patch 5's reproducer was >> run against 7.1.8's in-tree driver, where that call is unchanged. >> >> Taimuraz Kaitmazov (5): >> accel/amdxdna: refuse an I/O memory mapping of an imported BO >> accel/amdxdna: check the sync range for overflow on a device BO >> accel/amdxdna: do not warn when a sync request is rejected >> accel/amdxdna: refuse to flush an imported BO >> accel/amdxdna: do not fail a sync for a BO with no debug context >> >> drivers/accel/amdxdna/amdxdna_ctx.c | 3 ++- >> drivers/accel/amdxdna/amdxdna_gem.c | 27 +++++++++++++++++++-------- >> 2 files changed, 21 insertions(+), 9 deletions(-) >> > >

14 hours, 14 minutes

Re: [PATCH v2 00/15] accel/qda: Qualcomm DSP Accelerator driver

by Krzysztof Kozlowski

On 19/08/2026 17:23, Rob Clark wrote: >>>> >>>> Growing old driver does not look complicated itself. The only a bit >>>> tricky thing is to present somehow exclusive interface to user-space, >>>> like usage of one disables the second etc. Depending on actual >>>> differences in that interface. >>>> >>>> But having a duplicated driver is a clear no go and it is well known >>>> upstream requirement. Nothing new here. >>> >>> Would it be acceptable as a first step to, by default (ie. when no >>> cmdline override/etc) for the new driver to bind to new compatibles >> >> You have one compatible. >> >>> and the existing driver to old? Ie. have both drivers but only one >>> binds on a given platform? >> >> I don't see how is it possible to write such DTS, because - repeating my >> question - how many hardware blocks is there? I believe only one per >> given DSP, so how could you have two device nodes? > > Maybe I'm misunderstanding something here.. I don't see any new > bindings with this series so my assumption is that the bindings are > the same. What I meant was something more like "qcom,glymur-fastrpc" > would bind to new driver but "qcom,fastrpc" (which seems to be what is > used on older platforms) would bind to the old driver. > > So single dts node, but different compatible strings picking which > driver is used. Glymur is already done, so imagining we talk about next/future SoC then we would be at point of duplicating drivers for the same hardware. So back to square one of my comments. The rule of usptream development is that we do not accept duplicated code, just because a vendor wants to write something new. This is basically the concept applied all over the drivers tree, where we pushed back against all sorts of duplications all over the vendors. What I miss in this thread is why would there be any exception here. We do not grant exceptions from standard practices on "I want" reasons. Best regards, Krzysztof

15 hours, 17 minutes

Re: [PATCH v2 00/15] accel/qda: Qualcomm DSP Accelerator driver

by Krzysztof Kozlowski

On 19/08/2026 16:49, Rob Clark wrote: > On Wed, Aug 19, 2026 at 7:43 AM Krzysztof Kozlowski <krzk(a)kernel.org> wrote: >> >> On 19/08/2026 16:38, Rob Clark wrote: >>> On Wed, Aug 19, 2026 at 7:21 AM Krzysztof Kozlowski <krzk(a)kernel.org> wrote: >>>> >>>> On 19/08/2026 15:26, Ekansh Gupta wrote: >>>>> On 19-08-2026 00:43, Krzysztof Kozlowski wrote: >>>>>> On 17/08/2026 06:47, Ekansh Gupta wrote: >>>>>>> This patch series introduces the Qualcomm DSP Accelerator (QDA) driver, >>>>>>> a DRM-based accelerator driver for Qualcomm DSPs. The driver provides a >>>>>>> standardized interface for offloading computational tasks to DSPs found >>>>>>> on Qualcomm SoCs, supporting all DSP domains. >>>>>>> >>>>>>> The QDA driver implements the FastRPC protocol over the DRM accel >>>>>>> subsystem. It uses the same device-tree node structure as the existing >>>>>>> fastrpc driver in drivers/misc/. The approach for binding the QDA driver >>>>>>> to device-tree nodes while coexisting with the fastrpc driver is an open >>>>>>> item described below. >>>>>> >>>>>> No. Grow/replace/improve existing driver instead of coming with a duplicate. >>>>>> >>>>>> That's a standard upstream requirement, basically given on every >>>>>> upstreaming guide. >>>>>> >>>>>> Please watch old talk from Greg - "I Don’t Want Your Code!". >>>>> Posted discussion threads here[1]. Would seek comments from Dmitry, >>>>> Srini as well. >>>>> >>>>> [1] >>>>> https://lore.kernel.org/all/3476b5c3-7983-4994-a901-3d7d8bd75255@oss.qualco… >>>> >>>> The rest of the comments is still valid even if you did not acknowledge >>>> them. >>>> >>>> Anyway, regarding above - again, watch the talk from Greg. >>>> >>>> You have ONE driver. Not two. >>> >>> Long term, moving to the common driver framework (which did not exist >>> when fastrpc was first created) seems like a good thing. But does >>> that not allow for some transition period? How can we get from here >>> to there without otherwise breaking userspace? Is there some other >>> precedent elsewhere in other driver subsystems? >> >> Yes, Iris and Venus where we agreed for an exception (two drivers) as >> long as new driver supports old hardware / features. >> >> This is not the case here, right? >> >>> >>> I suppose drm exposing legacy fbdev on top of drm drivers is _sort of_ >>> a precedent if you squint a bit? I'm not really familiar enough to >>> say if that would be reasonable/possible in this case. >> >> Growing old driver does not look complicated itself. The only a bit >> tricky thing is to present somehow exclusive interface to user-space, >> like usage of one disables the second etc. Depending on actual >> differences in that interface. >> >> But having a duplicated driver is a clear no go and it is well known >> upstream requirement. Nothing new here. > > Would it be acceptable as a first step to, by default (ie. when no > cmdline override/etc) for the new driver to bind to new compatibles You have one compatible. > and the existing driver to old? Ie. have both drivers but only one > binds on a given platform? I don't see how is it possible to write such DTS, because - repeating my question - how many hardware blocks is there? I believe only one per given DSP, so how could you have two device nodes? Best regards, Krzysztof

15 hours, 51 minutes

Re: [PATCH v2 00/15] accel/qda: Qualcomm DSP Accelerator driver

by Krzysztof Kozlowski

On 19/08/2026 16:38, Rob Clark wrote: > On Wed, Aug 19, 2026 at 7:21 AM Krzysztof Kozlowski <krzk(a)kernel.org> wrote: >> >> On 19/08/2026 15:26, Ekansh Gupta wrote: >>> On 19-08-2026 00:43, Krzysztof Kozlowski wrote: >>>> On 17/08/2026 06:47, Ekansh Gupta wrote: >>>>> This patch series introduces the Qualcomm DSP Accelerator (QDA) driver, >>>>> a DRM-based accelerator driver for Qualcomm DSPs. The driver provides a >>>>> standardized interface for offloading computational tasks to DSPs found >>>>> on Qualcomm SoCs, supporting all DSP domains. >>>>> >>>>> The QDA driver implements the FastRPC protocol over the DRM accel >>>>> subsystem. It uses the same device-tree node structure as the existing >>>>> fastrpc driver in drivers/misc/. The approach for binding the QDA driver >>>>> to device-tree nodes while coexisting with the fastrpc driver is an open >>>>> item described below. >>>> >>>> No. Grow/replace/improve existing driver instead of coming with a duplicate. >>>> >>>> That's a standard upstream requirement, basically given on every >>>> upstreaming guide. >>>> >>>> Please watch old talk from Greg - "I Don’t Want Your Code!". >>> Posted discussion threads here[1]. Would seek comments from Dmitry, >>> Srini as well. >>> >>> [1] >>> https://lore.kernel.org/all/3476b5c3-7983-4994-a901-3d7d8bd75255@oss.qualco… >> >> The rest of the comments is still valid even if you did not acknowledge >> them. >> >> Anyway, regarding above - again, watch the talk from Greg. >> >> You have ONE driver. Not two. > > Long term, moving to the common driver framework (which did not exist > when fastrpc was first created) seems like a good thing. But does > that not allow for some transition period? How can we get from here > to there without otherwise breaking userspace? Is there some other > precedent elsewhere in other driver subsystems? Yes, Iris and Venus where we agreed for an exception (two drivers) as long as new driver supports old hardware / features. This is not the case here, right? > > I suppose drm exposing legacy fbdev on top of drm drivers is _sort of_ > a precedent if you squint a bit? I'm not really familiar enough to > say if that would be reasonable/possible in this case. Growing old driver does not look complicated itself. The only a bit tricky thing is to present somehow exclusive interface to user-space, like usage of one disables the second etc. Depending on actual differences in that interface. But having a duplicated driver is a clear no go and it is well known upstream requirement. Nothing new here. Best regards, Krzysztof

16 hours, 1 minute

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig