Re: [Intel-gfx] [PATCH 1/2] eventfd: simplify eventfd_signal()

[Oded Gabbay]

ntfd_ctx_fileget(struct file *file);
> -__u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n);
> +__u64 eventfd_signal(struct eventfd_ctx *ctx);
>  __u64 eventfd_signal_mask(struct eventfd_ctx *ctx, __u64 n, __poll_t mask);
>  int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx, 
> wait_queue_entry_t *wait,
>   __u64 *cnt);
> @@ -58,7 +58,7 @@ static inline struct eventfd_ctx *eventfd_ctx_fdget(int fd)
> return ERR_PTR(-ENOSYS);
>  }
>
> -static inline int eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
> +static inline int eventfd_signal(struct eventfd_ctx *ctx)
>  {
> return -ENOSYS;
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e8ca4bdcb03c..891550f575a1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4228,7 +4228,7 @@ static void __mem_cgroup_threshold(struct mem_cgroup 
> *memcg, bool swap)
>  * only one element of the array here.
>  */
> for (; i >= 0 && unlikely(t->entries[i].threshold > usage); i--)
> -   eventfd_signal(t->entries[i].eventfd, 1);
> +   eventfd_signal(t->entries[i].eventfd);
>
> /* i = current_threshold + 1 */
> i++;
> @@ -4240,7 +4240,7 @@ static void __mem_cgroup_threshold(struct mem_cgroup 
> *memcg, bool swap)
>  * only one element of the array here.
>  */
> for (; i < t->size && unlikely(t->entries[i].threshold <= usage); i++)
> -   eventfd_signal(t->entries[i].eventfd, 1);
> +   eventfd_signal(t->entries[i].eventfd);
>
> /* Update current_threshold */
> t->current_threshold = i - 1;
> @@ -4280,7 +4280,7 @@ static int mem_cgroup_oom_notify_cb(struct mem_cgroup 
> *memcg)
> spin_lock(_oom_lock);
>
> list_for_each_entry(ev, >oom_notify, list)
> -   eventfd_signal(ev->eventfd, 1);
> +   eventfd_signal(ev->eventfd);
>
> spin_unlock(_oom_lock);
> return 0;
> @@ -4499,7 +4499,7 @@ static int mem_cgroup_oom_register_event(struct 
> mem_cgroup *memcg,
>
> /* already in OOM ? */
> if (memcg->under_oom)
> -   eventfd_signal(eventfd, 1);
> +       eventfd_signal(eventfd);
> spin_unlock(_oom_lock);
>
> return 0;
> @@ -4791,7 +4791,7 @@ static void memcg_event_remove(struct work_struct *work)
> event->unregister_event(memcg, event->eventfd);
>
> /* Notify userspace the event is going away. */
> -   eventfd_signal(event->eventfd, 1);
> +   eventfd_signal(event->eventfd);
>
> eventfd_ctx_put(event->eventfd);
> kfree(event);
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index b52644771cc4..ba4cdef37e42 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
> @@ -169,7 +169,7 @@ static bool vmpressure_event(struct vmpressure *vmpr,
> continue;
> if (level < ev->level)
> continue;
> -   eventfd_signal(ev->efd, 1);
> +   eventfd_signal(ev->efd);
> ret = true;
> }
> mutex_unlock(>events_lock);
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index a60801fb8660..5edcf8d738de 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1028,9 +1028,9 @@ static int mtty_trigger_interrupt(struct mdev_state 
> *mdev_state)
> }
>
> if (mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX)
> -   ret = eventfd_signal(mdev_state->msi_evtfd, 1);
> +   ret = eventfd_signal(mdev_state->msi_evtfd);
> else
> -   ret = eventfd_signal(mdev_state->intx_evtfd, 1);
> +   ret = eventfd_signal(mdev_state->intx_evtfd);
>
>  #if defined(DEBUG_INTR)
> pr_info("Intx triggered\n");
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 89912a17f5d5..c0e230f4c3e9 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -61,7 +61,7 @@ static void irqfd_resampler_notify(struct 
> kvm_kernel_irqfd_resampler *resampler)
>
> list_for_each_entry_srcu(irqfd, >list, resampler_link,
>  
> srcu_read_lock_held(>kvm->irq_srcu))
> -   eventfd_signal(irqfd->resamplefd, 1);
> +   eventfd_signal(irqfd->resamplefd);
>  }
>
>  /*
> @@ -786,7 +786,7 @@ ioeventfd_write(struct kvm_vcpu *vcpu, struct 
> kvm_io_device *this, gpa_t addr,
> if (!ioeventfd_in_range(p, addr, len, val))
> return -EOPNOTSUPP;
>
> -   eventfd_signal(p->eventfd, 1);
> +   eventfd_signal(p->eventfd);
> return 0;
>  }
>
>
> --
> 2.34.1
>
For habanalabs (device.c):
Reviewed-by: Oded Gabbay 

Re: [Intel-gfx] [RFC PATCH 00/20] Initial Xe driver submission

[Oded Gabbay]

On Fri, Feb 17, 2023 at 10:51 PM Daniel Vetter  wrote:
>
> Hi all,
>
> [I thought I've sent this out earlier this week, but alas got stuck, kinda
> bad timing now since I'm out next week but oh well]
>
> So xe is a quite substantial thing, and I think we need a clear plan how to 
> land
> this or it will take forever, and managers will panic. Also I'm not a big fan 
> of
> "Dave/me reviews everything", we defacto had that for amd's dc/dal and it was
> not fun. The idea here is how to get everything reviewed without having two
> people end up somewhat arbitrary as deciders.
>
> I've compiled a bunch of topics on what I think the important areas are, first
> code that should be consistent about new-style render drivers that are aimed 
> for
> vk/compute userspace as the primary feature driver:
>
> - figure out consensus solution for fw scheduler and drm/sched frontend among
>   interested driver parties (probably xe, amdgpu, nouveau, new panfrost)
>
> - for the interface itself it might be good to have the drm_gpu_scheduler as 
> the
>   single per-hw-engine driver api object (but internally a new structure), 
> while
>   renaming the current drm_gpu_scheduler to drm_gpu_sched_internal. That way I
>   think we can address the main critique of the current xe scheduler plan
>   - keep the drm_gpu_sched_internal : drm_sched_entity 1:1 relationship for fw
> scheduler
>   - keep the driver api relationship of drm_gpu_scheduler : drm_sched_entity
> 1:n, the api functions simply iterate over a mutex protect list of 
> internal
> schedulers. this should also help drivers with locking mistakes around
> setup/teardown and gpu reset.
>   - drivers select with a flag or something between the current mode (where 
> the
> drm_gpu_sched_internal is attached to the drm_gpu_scheduler api object) or
> the new fw scheduler mode (where drm_gpu_sched_internal is attached to the
> drm_sched_entity)
>   - overall still no fundamental changes (like the current patches) to 
> drm/sched
> data structures and algorithms. But unlike the current patches we keep the
> possibility open for eventual refactoring without having to again refactor
> all the drivers. Even better, we can delay such refactoring until we have 
> a
> handful of real-word drivers test-driving this all so we know we actually 
> do
> the right thing. This should allow us to address all the
> fairness/efficiency/whatever concerns that have been floating around 
> without
> having to fix them all up upfront, before we actually know what needs to 
> be
> fixed.
>
> - the generic scheduler code should also including the handling of endless
>   compute contexts, with the minimal scaffolding for preempt-ctx fences
>   (probably on the drm_sched_entity) and making sure drm/sched can cope with 
> the
>   lack of job completion fence. This is very minimal amounts of code, but it
>   helps a lot for cross-driver review if this works the same (with the same
>   locking and all that) for everyone. Ideally this gets extracted from amdkfd,
>   but as long as it's going to be used by all drivers supporting
>   endless/compute context going forward it's good enough.
>
> - I'm assuming this also means Matt Brost will include a patch to add himself 
> as
>   drm/sched reviewer in MAINTAINERS, or at least something like that
>
> - adopt the gem_exec/vma helpers. again we probably want consensus here among
>   the same driver projects. I don't care whether these helpers specify the 
> ioctl
>   structs or not, but they absolutely need to enforce the overall locking 
> scheme
>   for all major structs and list (so vm and vma).
>
> - we also should have cross-driver consensus on async vm_bind support. I think
>   everyone added in-syncobj support, the real fun is probably more in/out
>   userspace memory fences (and personally I'm still not sure that's a good 
> idea
>   but ... *eh*). I think cross driver consensus on how this should work 
> (ideally
>   with helper support so people don't get it wrong in all the possible ways)
>   would be best.
>
> - this also means some userptr integration and some consensus how userptr 
> should
>   work for vm_bind across drivers. I don't think allowing drivers to reinvent
>   that wheel is a bright idea, there's just a bit too much to get wrong here.
>
> - for some of these the consensus might land on more/less shared code than 
> what
>   I sketched out above, the important part really is that we have consensus on
>   these. Kinda similar to how the atomic kms infrastructure move a _lot_ more 
> of
>   the code back into drivers, because they really just needed the flexibility 
> to
>   program the hw correctly. Right now we definitely don't have enough shared
>   code, for sure with i915-gem, but we also need to make sure we're not
>   overcorrecting too badly (a bit of overcorrecting generally doesn't hurt).
>
> All the above will make sure that the driver overall is in concepts and 

Re: [Intel-gfx] [PATCH 0/1] drm: Add a gpu page-table walker

[Oded Gabbay]

On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher  wrote:
>
> On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström
>  wrote:
> >
> > Hi, Daniel,
> >
> > On 2/16/23 21:18, Daniel Vetter wrote:
> > > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote:
> > >> A slightly unusual cover letter for a single patch.
> > >>
> > >> The page table walker is currently used by the xe driver only,
> > >> but the code is generic so we can be good citizens and add it to drm
> > >> as a helper, for possible use by other drivers,
> > >> If so we can merge the commit when we merge the xe driver.
> > >>
> > >> The question raised here is
> > >> *) Should it be a generic drm helper or xe-specific with changed
> > >> prefixes?
> > > I think if there's some other drivers interested in using this, then this
> > > sounds like a good idea. Maybe more useful if it's also integrated into
> > > the vm/vma helpers that are being discussed as an optional part?
> > >
> > > Maybe some good old sales pitching here to convince people would be good.
> > >
> > > Maybe one of the new accel drivers is interested in this too?
Hi,
As the habanalabs driver is not really a new driver, I currently don't
see the benefit of moving
to this code. Our pgt code is quite mature and was tested extensively
in deployment in the
past couple of years.

Nevertheless, I'll try to offer this code for any new/future driver
that will want to join accel.

Stanislaw, I'm adding you here in case you missed this. Might be of an
interest to you.

Thanks,
Oded

- Oded



> >
> > Thanks for your thoughts on this. Yeah, I think it's a bit awkward to
> > push for having code generic when there is only one user, and the
> > prospect of having other drivers rewrite their page-table building code
> > based on this helper in the near future is probably small. Perhaps more
> > of interest to new drivers. I think what will happen otherwise is that
> > during some future cleanup this will be pushed down to xe claiming it's
> > the only user.
> >
> > I wonder whether it might be an idea to maintain a small document where
> > driver writers can list suggestions for code that could be lifted to
> > core drm and be reused by others. That way both reviewers and writers of
> > other drivers can keep an eye on that document and use it to avoid
> > duplicating code. The procedure would then be to lift it to core drm and
> > fix up prefixes as soon as we have two or more users.
> >
> > Thoughts?
>
> FWIW, when we originally wrote the GPU scheduler it was part of
> amdgpu, but we consciously kept any AMD-isms out of it so it could be
> lifted up to a core component when another user came along.  Maybe
> some comments in the top of those files to that effect to maintain the
> separation.
>
> Alex
>
>
> >
> > Thomas
> >
> >
> > >
> > >> *) If a drm helper, should we use a config option?
> > > I am no fan of Kconfig things tbh. Maybe just include it in the vma
> > > helpers, or perhaps we want to do a drm-accel-helpers with gem helpers,
> > > drm/sched, this one here, vm/vma helpers or whatever they will be and so
> > > on? Kinda like we have modeset helpers.
> > >
> > > I'd definitely not go for a Kconfig per individual file, that's just
> > > excessive.
> > > -Daniel
> > >
> > >> For usage examples, see xe_pt.c
> > >> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c
> > >>
> > >> Thanks,
> > >> Thomas
> > >>
> > >> Thomas Hellström (1):
> > >>drm: Add a gpu page-table walker helper
> > >>
> > >>   drivers/gpu/drm/Makefile  |   1 +
> > >>   drivers/gpu/drm/drm_pt_walk.c | 159 +
> > >>   include/drm/drm_pt_walk.h | 161 ++
> > >>   3 files changed, 321 insertions(+)
> > >>   create mode 100644 drivers/gpu/drm/drm_pt_walk.c
> > >>   create mode 100644 include/drm/drm_pt_walk.h
> > >>
> > >> --
> > >> 2.34.1
> > >>

Re: [Intel-gfx] linux-next: manual merge of the accel tree with the drm-misc tree

[Oded Gabbay]

On Fri, Jan 20, 2023 at 2:32 AM Stephen Rothwell  wrote:
>
> Hi all,
>
> Today's linux-next merge of the accel tree got conflicts in:
>
>   drivers/Makefile
>   drivers/accel/Kconfig
>   drivers/accel/Makefile
>
> between commit:
>
>   35b137630f08 ("accel/ivpu: Introduce a new DRM driver for Intel VPU")
>
> from the drm-misc tree and commit:
>
>   45886b6fa0f1 ("habanalabs: move driver to accel subsystem")
>
> from the accel tree.
>
> I fixed it up (I used the latter version of Makefile and see below) and
> can carry the fix as necessary. This is now fixed as far as linux-next
> is concerned, but any non trivial conflicts should be mentioned to your
> upstream maintainer when your tree is submitted for merging.  You may
> also want to consider cooperating with the maintainer of the conflicting
> tree to minimise any particularly complex conflicts.
>
> --
> Cheers,
> Stephen Rothwell
>
> diff --cc drivers/accel/Kconfig
> index 4989376e5938,a5989b55178a..
> --- a/drivers/accel/Kconfig
> +++ b/drivers/accel/Kconfig
> @@@ -23,4 -23,4 +23,5 @@@ menuconfig DRM_ACCE
>   different device files, called accel/accel* (in /dev, sysfs
>   and debugfs).
>
> + source "drivers/accel/habanalabs/Kconfig"
>  +source "drivers/accel/ivpu/Kconfig"
> diff --cc drivers/accel/Makefile
> index b1036dbc0ba4,4df38885d6c6..
> --- a/drivers/accel/Makefile
> +++ b/drivers/accel/Makefile
> @@@ -1,3 -1,3 +1,4 @@@
>   # SPDX-License-Identifier: GPL-2.0-only
>
>  -obj-y  += habanalabs/
> ++obj-y  += habanalabs/
>  +obj-y += ivpu/
>
Thanks Stephen,
Fixes looks sane to me.
Oded

Re: [Intel-gfx] [PATCH v5 1/3] drm: Use XArray instead of IDR for minors

[Oded Gabbay]

On Mon, Nov 7, 2022 at 6:20 PM Matthew Wilcox  wrote:
>
> On Sun, Nov 06, 2022 at 04:51:39PM +0200, Oded Gabbay wrote:
> > I tried executing the following code in a dummy driver I wrote:
>
> You don't need to write a dummy driver; you can write test-cases
> entirely in userspace.  Just add them to lib/test_xarray.c and then
> make -C tools/testing/radix-tree/
>
> > static DEFINE_XARRAY_ALLOC(xa_dummy);
> > void check_xa(void *pdev)
> > {
> >   void *entry;
> >   int ret, index;
> >
> >   ret = xa_alloc(_dummy, , NULL, XA_LIMIT(0, 63), GFP_NOWAIT);
> >   if (ret < 0)
> >   return ret;
> >
> >   entry = xa_cmpxchg(_dummy, index, NULL, pdev, GFP_KERNEL);
> >   if (xa_is_err(entry))
> >return;
> >
> >   xa_lock(_dummy);
> >   xa_dev = xa_load(_dummy, index);
> >   xa_unlock(_dummy);
> > }
> >
> > And to my surprise xa_dev is always NULL, when it should be pdev.
> > I believe that because we first allocate the entry with NULL, it is
> > considered a "zero" entry in the XA.
> > And when we replace it, this attribute doesn't change so when we do
> > xa_load, the xa code thinks the entry is a "zero" entry and returns
> > NULL.
>
> There's no "attribute" to mark a zero entry.  It's just a zero entry.
> The problem is that you're cmpxchg'ing against NULL, and it's not NULL,
> it's the ZERO entry.  This is even documented in the test code:
>
> /* cmpxchg sees a reserved entry as ZERO */
> XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0);
> XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, XA_ZERO_ENTRY,
> xa_mk_value(12345678), GFP_NOWAIT) != NULL);
>
> I'm not quite sure why you're using xa_cmpxchg() here anyway; if you
> allocated it, it's yours and you can just xa_store() to it.
>
Hi Matthew,
Thanks for the explanation. I believe Michal's will fix his original
patch and I'll take that code.

Oded

Re: [Intel-gfx] [PATCH v5 1/3] drm: Use XArray instead of IDR for minors

[Oded Gabbay]

On Wed, Nov 2, 2022 at 4:23 PM Oded Gabbay  wrote:
>
> On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski
>  wrote:
> >
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> >
> > Signed-off-by: Michał Winiarski 
> > Suggested-by: Matthew Wilcox 
> > ---
> >  drivers/gpu/drm/drm_drv.c | 51 ++-
> >  1 file changed, 18 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 8214a0b1ab7f..61d24cdcd0f8 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -34,6 +34,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -53,8 +54,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, 
> > Jon Smirl");
> >  MODULE_DESCRIPTION("DRM shared core routines");
> >  MODULE_LICENSE("GPL and additional rights");
> >
> > -static DEFINE_SPINLOCK(drm_minor_lock);
> > -static struct idr drm_minors_idr;
> > +static DEFINE_XARRAY_ALLOC(drm_minors_xa);
> >
> >  /*
> >   * If the drm core fails to init for whatever reason,
> > @@ -98,21 +98,19 @@ static struct drm_minor **drm_minor_get_slot(struct 
> > drm_device *dev,
> >  static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> >  {
> > struct drm_minor *minor = data;
> > -   unsigned long flags;
> >
> > WARN_ON(dev != minor->dev);
> >
> > put_device(minor->kdev);
> >
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_remove(_minors_idr, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   xa_erase(_minors_xa, minor->index);
> >  }
> >
> > +#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * 
> > _t + 63); })
> > +
> >  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> >  {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> > int r;
> >
> > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> > @@ -122,21 +120,10 @@ static int drm_minor_alloc(struct drm_device *dev, 
> > unsigned int type)
> > minor->type = type;
> > minor->dev = dev;
> >
> > -   idr_preload(GFP_KERNEL);
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   r = idr_alloc(_minors_idr,
> > - NULL,
> > - 64 * type,
> > - 64 * (type + 1),
> > - GFP_NOWAIT);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > -   idr_preload_end();
> > -
> > +   r = xa_alloc(_minors_xa, >index, NULL, 
> > DRM_MINOR_LIMIT(type), GFP_KERNEL);
This was GFP_NOWAIT in the original code.

> > if (r < 0)
> > return r;
> >
> > -   minor->index = r;
> > -
> > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
> > if (r)
> > return r;
> > @@ -152,7 +139,7 @@ static int drm_minor_alloc(struct drm_device *dev, 
> > unsigned int type)
> >  static int drm_minor_register(struct drm_device *dev, unsigned int type)
> >  {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> > +   void *entry;
> > int ret;
> >
> > DRM_DEBUG("\n");
> > @@ -172,9 +159,12 @@ static int drm_minor_register(struct drm_device *dev, 
> > unsigned int type)
> > goto err_debugfs;
> >
> > /* replace NULL with @minor so lookups will succeed from now on */
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_replace(_minors_idr, minor, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   entry = xa_cmpxchg(_minors_xa, minor->index, NULL, , 
> > GFP_KERNEL);
> I believe we should pass in "minor", without the &, as  will
> give you the address of the local pointer.
>
> Oded
>
> > +   if (xa_is_err(entry)) {
> > +   ret = xa_err(entry);
> > +   goto err_debugfs;
> > +   }
> > +   WARN_ON(entry);
> >
> >  

Re: [Intel-gfx] [PATCH v5 1/3] drm: Use XArray instead of IDR for minors

[Oded Gabbay]

On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski
 wrote:
>
> IDR is deprecated, and since XArray manages its own state with internal
> locking, it simplifies the locking on DRM side.
> Additionally, don't use the IRQ-safe variant, since operating on drm
> minor is not done in IRQ context.
>
> Signed-off-by: Michał Winiarski 
> Suggested-by: Matthew Wilcox 
> ---
>  drivers/gpu/drm/drm_drv.c | 51 ++-
>  1 file changed, 18 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..61d24cdcd0f8 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -53,8 +54,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, 
> Jon Smirl");
>  MODULE_DESCRIPTION("DRM shared core routines");
>  MODULE_LICENSE("GPL and additional rights");
>
> -static DEFINE_SPINLOCK(drm_minor_lock);
> -static struct idr drm_minors_idr;
> +static DEFINE_XARRAY_ALLOC(drm_minors_xa);
>
>  /*
>   * If the drm core fails to init for whatever reason,
> @@ -98,21 +98,19 @@ static struct drm_minor **drm_minor_get_slot(struct 
> drm_device *dev,
>  static void drm_minor_alloc_release(struct drm_device *dev, void *data)
>  {
> struct drm_minor *minor = data;
> -   unsigned long flags;
>
> WARN_ON(dev != minor->dev);
>
> put_device(minor->kdev);
>
> -   spin_lock_irqsave(_minor_lock, flags);
> -   idr_remove(_minors_idr, minor->index);
> -   spin_unlock_irqrestore(_minor_lock, flags);
> +   xa_erase(_minors_xa, minor->index);
>  }
>
> +#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t 
> + 63); })
> +
>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>  {
> struct drm_minor *minor;
> -   unsigned long flags;
> int r;
>
> minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> @@ -122,21 +120,10 @@ static int drm_minor_alloc(struct drm_device *dev, 
> unsigned int type)
> minor->type = type;
> minor->dev = dev;
>
> -   idr_preload(GFP_KERNEL);
> -   spin_lock_irqsave(_minor_lock, flags);
> -   r = idr_alloc(_minors_idr,
> - NULL,
> - 64 * type,
> - 64 * (type + 1),
> - GFP_NOWAIT);
> -   spin_unlock_irqrestore(_minor_lock, flags);
> -   idr_preload_end();
> -
> +   r = xa_alloc(_minors_xa, >index, NULL, 
> DRM_MINOR_LIMIT(type), GFP_KERNEL);
> if (r < 0)
> return r;
>
> -   minor->index = r;
> -
> r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
> if (r)
> return r;
> @@ -152,7 +139,7 @@ static int drm_minor_alloc(struct drm_device *dev, 
> unsigned int type)
>  static int drm_minor_register(struct drm_device *dev, unsigned int type)
>  {
> struct drm_minor *minor;
> -   unsigned long flags;
> +   void *entry;
> int ret;
>
> DRM_DEBUG("\n");
> @@ -172,9 +159,12 @@ static int drm_minor_register(struct drm_device *dev, 
> unsigned int type)
> goto err_debugfs;
>
> /* replace NULL with @minor so lookups will succeed from now on */
> -   spin_lock_irqsave(_minor_lock, flags);
> -   idr_replace(_minors_idr, minor, minor->index);
> -   spin_unlock_irqrestore(_minor_lock, flags);
> +   entry = xa_cmpxchg(_minors_xa, minor->index, NULL, , 
> GFP_KERNEL);
I believe we should pass in "minor", without the &, as  will
give you the address of the local pointer.

Oded

> +   if (xa_is_err(entry)) {
> +   ret = xa_err(entry);
> +   goto err_debugfs;
> +   }
> +   WARN_ON(entry);
>
> DRM_DEBUG("new minor registered %d\n", minor->index);
> return 0;
> @@ -187,16 +177,13 @@ static int drm_minor_register(struct drm_device *dev, 
> unsigned int type)
>  static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
>  {
> struct drm_minor *minor;
> -   unsigned long flags;
>
> minor = *drm_minor_get_slot(dev, type);
> if (!minor || !device_is_registered(minor->kdev))
> return;
>
> /* replace @minor with NULL so lookups will fail from now on */
> -   spin_lock_irqsave(_minor_lock, flags);
> -   idr_replace(_minors_idr, NULL, minor->index);
> -   spin_unlock_irqrestore(_minor_lock, flags);
> +   xa_store(_minors_xa, minor->index, NULL, GFP_KERNEL);
>
> device_del(minor->kdev);
> dev_set_drvdata(minor->kdev, NULL); /* safety belt */
> @@ -215,13 +202,12 @@ static void drm_minor_unregister(struct drm_device 
> *dev, unsigned int type)
>  struct drm_minor *drm_minor_acquire(unsigned int minor_id)
>  {
> struct drm_minor *minor;
> -   unsigned long flags;
>
> -   spin_lock_irqsave(_minor_lock, 

Re: [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

[Oded Gabbay]

On Wed, May 20, 2020 at 9:05 PM Daniel Vetter  wrote:
>
> On Thu, May 14, 2020 at 02:38:38PM +0300, Oded Gabbay wrote:
> > On Tue, May 12, 2020 at 9:12 AM Daniel Vetter  
> > wrote:
> > >
> > > On Tue, May 12, 2020 at 4:14 AM Dave Airlie  wrote:
> > > >
> > > > On Mon, 11 May 2020 at 19:37, Oded Gabbay  wrote:
> > > > >
> > > > > On Mon, May 11, 2020 at 12:11 PM Daniel Vetter 
> > > > >  wrote:
> > > > > >
> > > > > > It's the default.
> > > > > Thanks for catching that.
> > > > >
> > > > > >
> > > > > > Also so much for "we're not going to tell the graphics people how to
> > > > > > review their code", dma_fence is a pretty core piece of gpu driver
> > > > > > infrastructure. And it's very much uapi relevant, including piles of
> > > > > > corresponding userspace protocols and libraries for how to pass 
> > > > > > these
> > > > > > around.
> > > > > >
> > > > > > Would be great if habanalabs would not use this (from a quick look
> > > > > > it's not needed at all), since open source the userspace and playing
> > > > > > by the usual rules isn't on the table. If that's not possible 
> > > > > > (because
> > > > > > it's actually using the uapi part of dma_fence to interact with gpu
> > > > > > drivers) then we have exactly what everyone promised we'd want to
> > > > > > avoid.
> > > > >
> > > > > We don't use the uapi parts, we currently only using the fencing and
> > > > > signaling ability of this module inside our kernel code. But maybe I
> > > > > didn't understand what you request. You want us *not* to use this
> > > > > well-written piece of kernel code because it is only used by graphics
> > > > > drivers ?
> > > > > I'm sorry but I don't get this argument, if this is indeed what you 
> > > > > meant.
> > > >
> > > > We would rather drivers using a feature that has requirements on
> > > > correct userspace implementations of the feature have a userspace that
> > > > is open source and auditable.
> > > >
> > > > Fencing is tricky, cross-device fencing is really tricky, and having
> > > > the ability for a closed userspace component to mess up other people's
> > > > drivers, think i915 shared with closed habana userspace and shared
> > > > fences, decreases ability to debug things.
> > > >
> > > > Ideally we wouldn't offer users known untested/broken scenarios, so
> > > > yes we'd prefer that drivers that intend to expose a userspace fencing
> > > > api around dma-fence would adhere to the rules of the gpu drivers.
> > > >
> > > > I'm not say you have to drop using dma-fence, but if you move towards
> > > > cross-device stuff I believe other drivers would be correct in
> > > > refusing to interact with fences from here.
> > >
> > > The flip side is if you only used dma-fence.c "because it's there",
> > > and not because it comes with an uapi attached and a cross-driver
> > > kernel internal contract for how to interact with gpu drivers, then
> > > there's really not much point in using it. It's a custom-rolled
> > > wait_queue/event thing, that's all. Without the gpu uapi and gpu
> > > cross-driver contract it would be much cleaner to just use wait_queue
> > > directly, and that's a construct all kernel developers understand, not
> > > just gpu folks. From a quick look at least habanalabs doesn't use any
> > > of these uapi/cross-driver/gpu bits.
> > > -Daniel
> >
> > Hi Daniel,
> > I want to say explicitly that we don't use the dma-buf uapi parts, nor
> > we intend to use them to communicate with any GPU device. We only use
> > it as simple completion mechanism as it was convenient to use.
> > I do understand I can exchange that mechanism with a simpler one, and
> > I will add an internal task to do it (albeit not in a very high
> > priority) and upstream it, its just that it is part of our data path
> > so we need to thoroughly validate it first.
>
> Sounds good.
>
> Wrt merging this patch here, can you include that in one of your next
> pulls? Or should I toss it entirely, waiting for you to remove dma_fence
> outright?

I'll include it in the next pull.
Thanks,
Oded
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

[Oded Gabbay]

On Tue, May 12, 2020 at 9:12 AM Daniel Vetter  wrote:
>
> On Tue, May 12, 2020 at 4:14 AM Dave Airlie  wrote:
> >
> > On Mon, 11 May 2020 at 19:37, Oded Gabbay  wrote:
> > >
> > > On Mon, May 11, 2020 at 12:11 PM Daniel Vetter  
> > > wrote:
> > > >
> > > > It's the default.
> > > Thanks for catching that.
> > >
> > > >
> > > > Also so much for "we're not going to tell the graphics people how to
> > > > review their code", dma_fence is a pretty core piece of gpu driver
> > > > infrastructure. And it's very much uapi relevant, including piles of
> > > > corresponding userspace protocols and libraries for how to pass these
> > > > around.
> > > >
> > > > Would be great if habanalabs would not use this (from a quick look
> > > > it's not needed at all), since open source the userspace and playing
> > > > by the usual rules isn't on the table. If that's not possible (because
> > > > it's actually using the uapi part of dma_fence to interact with gpu
> > > > drivers) then we have exactly what everyone promised we'd want to
> > > > avoid.
> > >
> > > We don't use the uapi parts, we currently only using the fencing and
> > > signaling ability of this module inside our kernel code. But maybe I
> > > didn't understand what you request. You want us *not* to use this
> > > well-written piece of kernel code because it is only used by graphics
> > > drivers ?
> > > I'm sorry but I don't get this argument, if this is indeed what you meant.
> >
> > We would rather drivers using a feature that has requirements on
> > correct userspace implementations of the feature have a userspace that
> > is open source and auditable.
> >
> > Fencing is tricky, cross-device fencing is really tricky, and having
> > the ability for a closed userspace component to mess up other people's
> > drivers, think i915 shared with closed habana userspace and shared
> > fences, decreases ability to debug things.
> >
> > Ideally we wouldn't offer users known untested/broken scenarios, so
> > yes we'd prefer that drivers that intend to expose a userspace fencing
> > api around dma-fence would adhere to the rules of the gpu drivers.
> >
> > I'm not say you have to drop using dma-fence, but if you move towards
> > cross-device stuff I believe other drivers would be correct in
> > refusing to interact with fences from here.
>
> The flip side is if you only used dma-fence.c "because it's there",
> and not because it comes with an uapi attached and a cross-driver
> kernel internal contract for how to interact with gpu drivers, then
> there's really not much point in using it. It's a custom-rolled
> wait_queue/event thing, that's all. Without the gpu uapi and gpu
> cross-driver contract it would be much cleaner to just use wait_queue
> directly, and that's a construct all kernel developers understand, not
> just gpu folks. From a quick look at least habanalabs doesn't use any
> of these uapi/cross-driver/gpu bits.
> -Daniel

Hi Daniel,
I want to say explicitly that we don't use the dma-buf uapi parts, nor
we intend to use them to communicate with any GPU device. We only use
it as simple completion mechanism as it was convenient to use.
I do understand I can exchange that mechanism with a simpler one, and
I will add an internal task to do it (albeit not in a very high
priority) and upstream it, its just that it is part of our data path
so we need to thoroughly validate it first.

Thanks,
Oded
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

[Oded Gabbay]

And just FYI, the driver was written internally at 2016-17, where the
dma-buf module didn't check the .wait ops before calling it and that's
why the initialization of the default wait was there in the first
place.
I should have removed it when I upstreamed it but it missed my review.
Thanks,
Oded

On Mon, May 11, 2020 at 12:36 PM Oded Gabbay  wrote:
>
> On Mon, May 11, 2020 at 12:11 PM Daniel Vetter  wrote:
> >
> > It's the default.
> Thanks for catching that.
>
> >
> > Also so much for "we're not going to tell the graphics people how to
> > review their code", dma_fence is a pretty core piece of gpu driver
> > infrastructure. And it's very much uapi relevant, including piles of
> > corresponding userspace protocols and libraries for how to pass these
> > around.
> >
> > Would be great if habanalabs would not use this (from a quick look
> > it's not needed at all), since open source the userspace and playing
> > by the usual rules isn't on the table. If that's not possible (because
> > it's actually using the uapi part of dma_fence to interact with gpu
> > drivers) then we have exactly what everyone promised we'd want to
> > avoid.
>
> We don't use the uapi parts, we currently only using the fencing and
> signaling ability of this module inside our kernel code. But maybe I
> didn't understand what you request. You want us *not* to use this
> well-written piece of kernel code because it is only used by graphics
> drivers ?
> I'm sorry but I don't get this argument, if this is indeed what you meant.
>
> Oded
>
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Greg Kroah-Hartman 
> > Cc: Olof Johansson 
> > Cc: Oded Gabbay 
> > Cc: Sumit Semwal 
> > Cc: linux-me...@vger.kernel.org
> > Cc: linaro-mm-...@lists.linaro.org
> > ---
> >  drivers/misc/habanalabs/command_submission.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/misc/habanalabs/command_submission.c 
> > b/drivers/misc/habanalabs/command_submission.c
> > index 409276b6374d..cc3ce759b6c3 100644
> > --- a/drivers/misc/habanalabs/command_submission.c
> > +++ b/drivers/misc/habanalabs/command_submission.c
> > @@ -46,7 +46,6 @@ static const struct dma_fence_ops hl_fence_ops = {
> > .get_driver_name = hl_fence_get_driver_name,
> > .get_timeline_name = hl_fence_get_timeline_name,
> > .enable_signaling = hl_fence_enable_signaling,
> > -   .wait = dma_fence_default_wait,
> > .release = hl_fence_release
> >  };
> >
> > --
> > 2.26.2
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

[Oded Gabbay]

On Mon, May 11, 2020 at 12:11 PM Daniel Vetter  wrote:
>
> It's the default.
Thanks for catching that.

>
> Also so much for "we're not going to tell the graphics people how to
> review their code", dma_fence is a pretty core piece of gpu driver
> infrastructure. And it's very much uapi relevant, including piles of
> corresponding userspace protocols and libraries for how to pass these
> around.
>
> Would be great if habanalabs would not use this (from a quick look
> it's not needed at all), since open source the userspace and playing
> by the usual rules isn't on the table. If that's not possible (because
> it's actually using the uapi part of dma_fence to interact with gpu
> drivers) then we have exactly what everyone promised we'd want to
> avoid.

We don't use the uapi parts, we currently only using the fencing and
signaling ability of this module inside our kernel code. But maybe I
didn't understand what you request. You want us *not* to use this
well-written piece of kernel code because it is only used by graphics
drivers ?
I'm sorry but I don't get this argument, if this is indeed what you meant.

Oded

>
> Signed-off-by: Daniel Vetter 
> Cc: Greg Kroah-Hartman 
> Cc: Olof Johansson 
> Cc: Oded Gabbay 
> Cc: Sumit Semwal 
> Cc: linux-me...@vger.kernel.org
> Cc: linaro-mm-...@lists.linaro.org
> ---
>  drivers/misc/habanalabs/command_submission.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/misc/habanalabs/command_submission.c 
> b/drivers/misc/habanalabs/command_submission.c
> index 409276b6374d..cc3ce759b6c3 100644
> --- a/drivers/misc/habanalabs/command_submission.c
> +++ b/drivers/misc/habanalabs/command_submission.c
> @@ -46,7 +46,6 @@ static const struct dma_fence_ops hl_fence_ops = {
> .get_driver_name = hl_fence_get_driver_name,
> .get_timeline_name = hl_fence_get_timeline_name,
> .enable_signaling = hl_fence_enable_signaling,
> -   .wait = dma_fence_default_wait,
> .release = hl_fence_release
>  };
>
> --
> 2.26.2
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Oded Gabbay]

On Wed, Aug 22, 2018 at 10:58 PM Linus Torvalds
 wrote:
>
> On Wed, Aug 22, 2018 at 12:37 PM Oded Gabbay  wrote:
> >
> > Having said that, I think we *are* protected by the mmu_notifier
> > release because if the process suddenly dies, we will gracefully clean
> > the process's data in our driver and on the H/W before returning to
> > the mm core code. And before we return to the mm core code, we set the
> > mm pointer to NULL. And the graceful cleaning should be serialized
> > with the load_hqd uses.
>
> So I'm a bit nervous about the mmu_notifier model (and the largely
> equivalent exit_aio() model for the USB gardget AIO uses).
>
> The reason I'm nervous about it is that the mmu_notifier() gets called
> only after the mm_users count has already been decremented to zero
> (and the exact same thing goes for exit_aio()).
>
> Now that's fine if you actually get rid of all accesses in
> mmu_notifier_release() or in exit_aio(), because the page tables still
> exist at that point - they are in the process of being torn down, but
> they haven't been torn down yet.
>
> But for something like a kernel thread doing use_mm(), the thing that
> worries me is a pattern something like this:
>
>   kwork thread  exit thread
>     
>
> mmput() ->
>   mm_users goes to zero
>
>   use_mm(mmptr);
>   ..
>
>   mmu_notifier_release();
>   exit_mm() ->
> exit_aio()
>
> and the pattern is basically the same regatdless of whether you use
> mmu_notifier_release() or depend on some exit_aio() flushing your aio
> work: the use_mm() can be called with a mm that has already had its
> mm_users count decremented to zero, and that is now scheduled to be
> free'd.
>
> Does it "work"? Yes. Kind of. At least if the mmu notifier and/or
> exit_aio() actually makes sure to wait for any kwork thread thing. But
> it's a bit of a worrisome pattern.
>
>Linus

Yes, agreed, and that's why we will be on the safe side and eliminate
this pattern from our code and make sure we won't add this pattern in
the future.

Oded
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Oded Gabbay]

On Wed, Aug 22, 2018 at 7:44 PM Linus Torvalds
 wrote:
> One of the complex ones is the amdgpu driver. It does a
> "use_mm(mmptr)" deep deep in the guts of a macro that ends up being
> used in fa few places, and it's very hard to tell if it's right.
>
> It looks almost certainly buggy (there is no mmget/mmput to get the
> refcount), but there _is_ a "release" mmu_notifier function and that
> one - unlike the kvm case - looks like it might actually be trying to
> flush the existing pending users of that mm.
>
> But on the whole, I'm suspicious of the amdgpu use. It smells. Jann
> Horn pointed out that even if it migth be ok due to the mmu_notifier,
> the comments are garbage:
>
> >  Where "process" in the uniquely-named "struct queue" is a "struct
> >  kfd_process"; that struct's definition has this comment in it:
> >
> >/*
> > * Opaque pointer to mm_struct. We don't hold a reference to
> > * it so it should never be dereferenced from here. This is
> > * only used for looking up processes by their mm.
> > */
> >void *mm;
> >
> >  So I think either that comment is wrong, or their code is wrong?
>
> so I'm chalking the amdgpu use up in the "broken" column.
>
Hello Linus,

I looked at the amdkfd code and indeed the comment does not match the
actual code because the mm pointer is clearly dereferenced directly in
the macro you mentioned (read_user_wptr). That macro is used in the
code path of loading a descriptor to the H/W (load_hqd). That function
is called in several cases, where in some of them we are in the
context of the calling process, but in others we are in a kernel
thread context (hence the use_mm). That's why we check these two
situations inside that macro and only do use_mm if we are in kernel
thread.

We need to fix that behavior and obviously make sure that in future
code we don't cast this pointer to mm_struct* and derereference it
directly.
Actually, the original code had "mm_struct *mm" instead of "void *mm"
in the structure, and I think the reason we changed it to void* is to
"make sure" that we won't dereference it directly, but clearly that
failed :(

Having said that, I think we *are* protected by the mmu_notifier
release because if the process suddenly dies, we will gracefully clean
the process's data in our driver and on the H/W before returning to
the mm core code. And before we return to the mm core code, we set the
mm pointer to NULL. And the graceful cleaning should be serialized
with the load_hqd uses.

Felix, do you have anything to add here that I might have missed ?

Thanks,
Oded
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [drm-tip:drm-tip 15/1373] arch/frv/include/asm/pgalloc.h:48:2: error: implicit declaration of function 'pgtable_page_dtor'; did you mean 'pgdat_page_nr'?

[Oded Gabbay]

Hi,
There is a missing #include  at amdgpu_amdkfd.h
I'll send a patch to Dave to apply to his drm-next tree to fix this.
Thanks for catching this,
Oded

On Thu, Mar 15, 2018 at 4:50 AM, kbuild test robot
 wrote:
> tree:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
> head:   178cfb9373cc2bdfcb6ca73e03369d2c37cc4b58
> commit: d8d019ccffb838bb0dd98e583b5c25ccc0bc6ece [15/1373] drm/amdgpu: Add 
> KFD eviction fence
> config: frv-allyesconfig (attached as .config)
> compiler: frv-linux-gcc (GCC) 7.2.0
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout d8d019ccffb838bb0dd98e583b5c25ccc0bc6ece
> # save the attached .config to linux build tree
> make.cross ARCH=frv
>
> All errors (new ones prefixed by >>):
>
>In file included from arch/frv/include/asm/mmu_context.h:17:0,
> from include/linux/mmu_context.h:5,
> from drivers/gpu//drm/amd/amdgpu/amdgpu_amdkfd.h:29,
> from drivers/gpu//drm/amd/amdgpu/amdgpu_amdkfd_fence.c:30:
>arch/frv/include/asm/pgalloc.h: In function 'pte_free':
>>> arch/frv/include/asm/pgalloc.h:48:2: error: implicit declaration of 
>>> function 'pgtable_page_dtor'; did you mean 'pgdat_page_nr'? 
>>> [-Werror=implicit-function-declaration]
>  pgtable_page_dtor(pte);
>  ^
>  pgdat_page_nr
>cc1: some warnings being treated as errors
>
> vim +48 arch/frv/include/asm/pgalloc.h
>
> ^1da177e include/asm-frv/pgalloc.h Linus Torvalds 2005-04-16  45
> 2f569afd include/asm-frv/pgalloc.h Martin Schwidefsky 2008-02-08  46  static 
> inline void pte_free(struct mm_struct *mm, pgtable_t pte)
> ^1da177e include/asm-frv/pgalloc.h Linus Torvalds 2005-04-16  47  {
> 2f569afd include/asm-frv/pgalloc.h Martin Schwidefsky 2008-02-08 @48
> pgtable_page_dtor(pte);
> ^1da177e include/asm-frv/pgalloc.h Linus Torvalds 2005-04-16  49
> __free_page(pte);
> ^1da177e include/asm-frv/pgalloc.h Linus Torvalds 2005-04-16  50  }
> ^1da177e include/asm-frv/pgalloc.h Linus Torvalds 2005-04-16  51
>
> :: The code at line 48 was first introduced by commit
> :: 2f569afd9ced9ebec9a6eb3dbf6f83429be0a7b4 CONFIG_HIGHPTE vs. sub-page 
> page tables.
>
> :: TO: Martin Schwidefsky 
> :: CC: Linus Torvalds 
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 01/10] drm/amd-kfd: Clean up inline handling

[Oded Gabbay]

On Tue, Jun 21, 2016 at 12:10 PM, Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
> - inline functions need to be static inline, otherwise gcc can opt to
>   not inline and the linker gets unhappy.
> - no forward decls for inline functions, just include the right headers.
>
> Cc: Oded Gabbay <oded.gab...@gmail.com>
> Cc: Ben Goz <ben@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 ---
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h 
> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> index ec4036a09f3e..a625b9137da2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -187,12 +187,12 @@ int init_pipelines(struct device_queue_manager *dqm,
>  unsigned int get_first_pipe(struct device_queue_manager *dqm);
>  unsigned int get_pipes_num(struct device_queue_manager *dqm);
>
> -extern inline unsigned int get_sh_mem_bases_32(struct kfd_process_device 
> *pdd)
> +static inline unsigned int get_sh_mem_bases_32(struct kfd_process_device 
> *pdd)
>  {
> return (pdd->lds_base >> 16) & 0xFF;
>  }
>
> -extern inline unsigned int
> +static inline unsigned int
>  get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
>  {
> return (pdd->lds_base >> 60) & 0x0E;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index d0d5f4baf72d..80113c335966 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -617,10 +617,7 @@ int kgd2kfd_resume(struct kfd_dev *kfd);
>  int kfd_init_apertures(struct kfd_process *process);
>
>  /* Queue Context Management */
> -inline uint32_t lower_32(uint64_t x);
> -inline uint32_t upper_32(uint64_t x);
>  struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd);
> -inline uint32_t get_sdma_base_addr(struct cik_sdma_rlc_registers *m);
>
>  int init_queue(struct queue **q, struct queue_properties properties);
>  void uninit_queue(struct queue *q);
> --
> 2.8.1
>

Hi Daniel,
Minor comment, please change the commit message title to "drm/amdkfd:
..." (without the "-" between amd and kfd), to make this patch
consistent with all amdkfd patches.

With that change, this patch is:
Reviewed-by: Oded Gabbay <oded.gab...@gmail.com>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/7] Enable SVM for Intel VT-d

[Oded Gabbay]

On Sat, Oct 10, 2015 at 4:17 PM, David Woodhouse  wrote:
>
> On Fri, 2015-10-09 at 00:50 +0100, David Woodhouse wrote:
> > This patch set enables PASID support for the Intel IOMMU, along with
> > page request support.
> >
> > Like its AMD counterpart, it exposes an IOMMU-specific API. I believe
> > we'll have a session at the Kernel Summit later this month in which we
> > can work out a generic API which will cover the two (now) existing
> > implementations as well as upcoming ARM (and other?) versions.
> >
> > For the time being, however, exposing an Intel-specific API is good
> > enough, especially as we don't have the required TLP prefix support on
> > our PCIe root ports and we *can't* support discrete PCIe devices with
> > PASID support. It's purely on-chip stuff right now, which is basically
> > only Intel graphics.
> >
> > The AMD implementation allows a per-device PASID space, and managing
> > the PASID space is left entirely to the device driver. In contrast,
> > this implementation maintains a per-IOMMU PASID space, and drivers
> > calling intel_svm_bind_mm() will be *given* the PASID that they are to
> > use. In general we seem to be converging on using a single PASID space
> > across *all* IOMMUs in the system, and this will support that mode of
> > operation.
>
> The other noticeable difference is the lifetime management of the mm.
> My code takes a reference on it, and will only do the mmput() when the
> driver unbinds the PASID. So the mmu_notifier's .release() method won't
> get called before that.
>
> The AMD version doesn't take that refcount, and its .release() method
> therefore needs to actually call back into the device driver and ensure
> that all access to the mm, including pending page faults, is flushed.
> The locking issues there scare me a little, especially if page faults
> are currently outstanding.
>
> In the i915 case we have an open file descriptor associated with the
> gfx context. When the process dies, the fd is closed and the driver can
> go and clean up after it.
>
> The amdkfd driver, on the other hand, keeps the device-side job running
> even after the process has closed its file descriptor. So it *needs*
> the .release() call to happen when the process exits, as it otherwise
> doesn't know when to clean up.
>
> I am somewhat dubious about that as a design decision. If we're moving
> to a more explicit management of off-cpu tasks with mm access, as is to
> be discussed at the Kernel Summit, then hopefully we can fix that. It's
> a *lot* simpler if we just pin the mm while the device context has
> access to it.
>
> --
> dwmw2
>

Hi David,

There was a whole debate about this issue (amdkfd binding to mm struct
lifespan instead of to fd) when we upstreamed amdkfd, with good
arguments for and against. If you want to understand the reasons, I
suggest reading the following email thread:

https://lists.linuxfoundation.org/pipermail/iommu/2014-July/009005.html

TL;DR, IIRC, the bottom line was that (over-simplified):

1. HSA/amdkfd is not a "classic" device driver, is it performs
operations in context of a process working on multiple devices and
doesn't contain an "instance per device". It's conceptually more like
a subsystem/system call interface then a device driver.

2. It is not a one-of-a-kind in the kernel, as there are other drivers
which use this method.

Oded
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx