[15/15] amdgpu: remove CONFIG_DRM_AMDGPU_USERPTR

Submitted by Christoph Hellwig on Aug. 6, 2019, 4:05 p.m.

Details

Message ID 20190806160554.14046-16-hch@lst.de
State New
Headers show
Series "Series without cover letter" ( rev: 3 2 1 ) in Nouveau

Browsing this patch as part of:
"Series without cover letter" rev 1 in Nouveau
<< prev patch [15/15] next patch >>

Commit Message

Christoph Hellwig Aug. 6, 2019, 4:05 p.m.
The option is just used to select HMM mirror support and has a very
confusing help text.  Just pull in the HMM mirror code by default
instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/Kconfig                 |  2 ++
 drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------
 4 files changed, 2 insertions(+), 28 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1d80222587ad..319c1da2e74e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -226,9 +226,11 @@  config DRM_AMDGPU
 	select DRM_SCHED
         select DRM_TTM
 	select POWER_SUPPLY
+	select HMM_MIRROR
 	select HWMON
 	select BACKLIGHT_CLASS_DEVICE
 	select INTERVAL_TREE
+	select MMU_NOTIFIER
 	select CHASH
 	help
 	  Choose this option if you have a recent AMD Radeon graphics card.
diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 2e98c016cb47..c5c963164f5e 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -24,16 +24,6 @@  config DRM_AMDGPU_CIK
 
 	  radeon.cik_support=0 amdgpu.cik_support=1
 
-config DRM_AMDGPU_USERPTR
-	bool "Always enable userptr write support"
-	depends on DRM_AMDGPU
-	depends on MMU
-	select HMM_MIRROR
-	select MMU_NOTIFIER
-	help
-	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
-	  isn't already selected to enabled full userptr support.
-
 config DRM_AMDGPU_GART_DEBUGFS
 	bool "Allow GART access through debugfs"
 	depends on DRM_AMDGPU
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 8bf79288c4e2..00b74adbd790 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -751,9 +751,7 @@  struct amdgpu_ttm_tt {
 	uint64_t		userptr;
 	struct task_struct	*usertask;
 	uint32_t		userflags;
-#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
 	struct hmm_range	*range;
-#endif
 };
 
 /**
@@ -763,7 +761,6 @@  struct amdgpu_ttm_tt {
  * Calling function must call amdgpu_ttm_tt_userptr_range_done() once and only
  * once afterwards to stop HMM tracking
  */
-#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
 
 #define MAX_RETRY_HMM_RANGE_FAULT	16
 
@@ -892,7 +889,6 @@  bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm)
 
 	return r;
 }
-#endif
 
 /**
  * amdgpu_ttm_tt_set_user_pages - Copy pages in, putting old pages as necessary.
@@ -970,12 +966,10 @@  static void amdgpu_ttm_tt_unpin_userptr(struct ttm_tt *ttm)
 
 	sg_free_table(ttm->sg);
 
-#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
 	if (gtt->range &&
 	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
 						      gtt->range->pfns[0]))
 		WARN_ONCE(1, "Missing get_user_page_done\n");
-#endif
 }
 
 int amdgpu_ttm_gart_bind(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index caa76c693700..406b1c5e6dd4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -101,20 +101,8 @@  int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
 int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo);
 int amdgpu_ttm_recover_gart(struct ttm_buffer_object *tbo);
 
-#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
 int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages);
 bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm);
-#else
-static inline int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo,
-					       struct page **pages)
-{
-	return -EPERM;
-}
-static inline bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm)
-{
-	return false;
-}
-#endif
 
 void amdgpu_ttm_tt_set_user_pages(struct ttm_tt *ttm, struct page **pages);
 int amdgpu_ttm_tt_set_userptr(struct ttm_tt *ttm, uint64_t addr,

Comments

On Tue, Aug 06, 2019 at 07:05:53PM +0300, Christoph Hellwig wrote:
> The option is just used to select HMM mirror support and has a very
> confusing help text.  Just pull in the HMM mirror code by default
> instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/gpu/drm/Kconfig                 |  2 ++
>  drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------
>  4 files changed, 2 insertions(+), 28 deletions(-)

Felix, was this an effort to avoid the arch restriction on hmm or
something? Also can't see why this was like this.

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Jason
On 2019-08-06 13:44, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2019 at 07:05:53PM +0300, Christoph Hellwig wrote:

>> The option is just used to select HMM mirror support and has a very

>> confusing help text.  Just pull in the HMM mirror code by default

>> instead.

>>

>> Signed-off-by: Christoph Hellwig <hch@lst.de>

>> ---

>>   drivers/gpu/drm/Kconfig                 |  2 ++

>>   drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------

>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------

>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------

>>   4 files changed, 2 insertions(+), 28 deletions(-)

> Felix, was this an effort to avoid the arch restriction on hmm or

> something? Also can't see why this was like this.


This option predates KFD's support of userptrs, which in turn predates 
HMM. Radeon has the same kind of option, though it doesn't affect HMM in 
that case.

Alex, Christian, can you think of a good reason to maintain userptr 
support as an option in amdgpu? I suspect it was originally meant as a 
way to allow kernels with amdgpu without MMU notifiers. Now it would 
allow a kernel with amdgpu without HMM or MMU notifiers. I don't know if 
this is a useful thing to have.

Regards,
   Felix

>

> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

>

> Jason
On Tue, Aug 6, 2019 at 1:51 PM Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
>
> On 2019-08-06 13:44, Jason Gunthorpe wrote:
> > On Tue, Aug 06, 2019 at 07:05:53PM +0300, Christoph Hellwig wrote:
> >> The option is just used to select HMM mirror support and has a very
> >> confusing help text.  Just pull in the HMM mirror code by default
> >> instead.
> >>
> >> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> ---
> >>   drivers/gpu/drm/Kconfig                 |  2 ++
> >>   drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------
> >>   4 files changed, 2 insertions(+), 28 deletions(-)
> > Felix, was this an effort to avoid the arch restriction on hmm or
> > something? Also can't see why this was like this.
>
> This option predates KFD's support of userptrs, which in turn predates
> HMM. Radeon has the same kind of option, though it doesn't affect HMM in
> that case.
>
> Alex, Christian, can you think of a good reason to maintain userptr
> support as an option in amdgpu? I suspect it was originally meant as a
> way to allow kernels with amdgpu without MMU notifiers. Now it would
> allow a kernel with amdgpu without HMM or MMU notifiers. I don't know if
> this is a useful thing to have.

Right.  There were people that didn't have MMU notifiers that wanted
support for the GPU.  For a lot of older APIs, a lack of userptr
support was not a big deal (it just disabled some optimizations and
API extensions), but as it becomes more relevant it may make sense to
just make it a requirement.

Alex

>
> Regards,
>    Felix
>
> >
> > Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> >
> > Jason
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Tue, Aug 06, 2019 at 02:58:58PM -0400, Alex Deucher wrote:
> On Tue, Aug 6, 2019 at 1:51 PM Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
> >
> > On 2019-08-06 13:44, Jason Gunthorpe wrote:
> > > On Tue, Aug 06, 2019 at 07:05:53PM +0300, Christoph Hellwig wrote:
> > >> The option is just used to select HMM mirror support and has a very
> > >> confusing help text.  Just pull in the HMM mirror code by default
> > >> instead.
> > >>
> > >> Signed-off-by: Christoph Hellwig <hch@lst.de>
> > >>   drivers/gpu/drm/Kconfig                 |  2 ++
> > >>   drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------
> > >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------
> > >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------
> > >>   4 files changed, 2 insertions(+), 28 deletions(-)
> > > Felix, was this an effort to avoid the arch restriction on hmm or
> > > something? Also can't see why this was like this.
> >
> > This option predates KFD's support of userptrs, which in turn predates
> > HMM. Radeon has the same kind of option, though it doesn't affect HMM in
> > that case.
> >
> > Alex, Christian, can you think of a good reason to maintain userptr
> > support as an option in amdgpu? I suspect it was originally meant as a
> > way to allow kernels with amdgpu without MMU notifiers. Now it would
> > allow a kernel with amdgpu without HMM or MMU notifiers. I don't know if
> > this is a useful thing to have.
> 
> Right.  There were people that didn't have MMU notifiers that wanted
> support for the GPU.

?? Is that even a real thing? mmu_notifier does not have much kconfig
dependency.

Jason
Am 06.08.19 um 22:03 schrieb Jason Gunthorpe:
> On Tue, Aug 06, 2019 at 02:58:58PM -0400, Alex Deucher wrote:

>> On Tue, Aug 6, 2019 at 1:51 PM Kuehling, Felix <Felix.Kuehling@amd.com> wrote:

>>> On 2019-08-06 13:44, Jason Gunthorpe wrote:

>>>> On Tue, Aug 06, 2019 at 07:05:53PM +0300, Christoph Hellwig wrote:

>>>>> The option is just used to select HMM mirror support and has a very

>>>>> confusing help text.  Just pull in the HMM mirror code by default

>>>>> instead.

>>>>>

>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>

>>>>>    drivers/gpu/drm/Kconfig                 |  2 ++

>>>>>    drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------

>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------

>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------

>>>>>    4 files changed, 2 insertions(+), 28 deletions(-)

>>>> Felix, was this an effort to avoid the arch restriction on hmm or

>>>> something? Also can't see why this was like this.

>>> This option predates KFD's support of userptrs, which in turn predates

>>> HMM. Radeon has the same kind of option, though it doesn't affect HMM in

>>> that case.

>>>

>>> Alex, Christian, can you think of a good reason to maintain userptr

>>> support as an option in amdgpu? I suspect it was originally meant as a

>>> way to allow kernels with amdgpu without MMU notifiers. Now it would

>>> allow a kernel with amdgpu without HMM or MMU notifiers. I don't know if

>>> this is a useful thing to have.

>> Right.  There were people that didn't have MMU notifiers that wanted

>> support for the GPU.

> ?? Is that even a real thing? mmu_notifier does not have much kconfig

> dependency.


Yes, that used to be a very real thing.

Initially a lot of users didn't wanted mmu notifiers to be enabled 
because of the performance overhead they costs.

Then we had the problem that HMM mirror wasn't available on a lot of 
architectures.

Not sure if we still need it separately, but I think that it shouldn't 
be removed without asking around if somebody still needs that.

Christian.

>

> Jason
On Wed, Aug 07, 2019 at 06:57:24AM +0000, Koenig, Christian wrote:
> Am 06.08.19 um 22:03 schrieb Jason Gunthorpe:
> > On Tue, Aug 06, 2019 at 02:58:58PM -0400, Alex Deucher wrote:
> >> On Tue, Aug 6, 2019 at 1:51 PM Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
> >>> On 2019-08-06 13:44, Jason Gunthorpe wrote:
> >>>> On Tue, Aug 06, 2019 at 07:05:53PM +0300, Christoph Hellwig wrote:
> >>>>> The option is just used to select HMM mirror support and has a very
> >>>>> confusing help text.  Just pull in the HMM mirror code by default
> >>>>> instead.
> >>>>>
> >>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >>>>>    drivers/gpu/drm/Kconfig                 |  2 ++
> >>>>>    drivers/gpu/drm/amd/amdgpu/Kconfig      | 10 ----------
> >>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  6 ------
> >>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 12 ------------
> >>>>>    4 files changed, 2 insertions(+), 28 deletions(-)
> >>>> Felix, was this an effort to avoid the arch restriction on hmm or
> >>>> something? Also can't see why this was like this.
> >>> This option predates KFD's support of userptrs, which in turn predates
> >>> HMM. Radeon has the same kind of option, though it doesn't affect HMM in
> >>> that case.
> >>>
> >>> Alex, Christian, can you think of a good reason to maintain userptr
> >>> support as an option in amdgpu? I suspect it was originally meant as a
> >>> way to allow kernels with amdgpu without MMU notifiers. Now it would
> >>> allow a kernel with amdgpu without HMM or MMU notifiers. I don't know if
> >>> this is a useful thing to have.
> >> Right.  There were people that didn't have MMU notifiers that wanted
> >> support for the GPU.
> > ?? Is that even a real thing? mmu_notifier does not have much kconfig
> > dependency.
> 
> Yes, that used to be a very real thing.
> 
> Initially a lot of users didn't wanted mmu notifiers to be enabled 
> because of the performance overhead they costs.

Seems strange to hear these days, every distro ships with it on, it is
needed for kvm.

> Then we had the problem that HMM mirror wasn't available on a lot of 
> architectures.

Some patches for hmm are ready now that will fix this

Jason