[PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Yang, Philip]

Those options are needed to support HMM

Change-Id: Ieb7bb3bcec07245d79a02793e6728228decc400a
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 960a63355705..63f0542bc34b 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -26,7 +26,9 @@ config DRM_AMDGPU_CIK
 config DRM_AMDGPU_USERPTR
 	bool "Always enable userptr write support"
 	depends on DRM_AMDGPU
+	select ARCH_HAS_HMM
 	select HMM_MIRROR
+	select ZONE_DEVICE
 	help
 	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
 	  isn't already selected to enabled full userptr support.
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Kuehling, Felix]

[+Jerome]

Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring 
CPU page tables to device page tables.

ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative 
for ARM support?

Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the 
CPU architecture rather than any driver. Jerome, do you have any advice?

Thanks,
   Felix

On 2019-02-20 1:56 p.m., Yang, Philip wrote:
> Those options are needed to support HMM
>
> Change-Id: Ieb7bb3bcec07245d79a02793e6728228decc400a
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/Kconfig | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> index 960a63355705..63f0542bc34b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> @@ -26,7 +26,9 @@ config DRM_AMDGPU_CIK
>   config DRM_AMDGPU_USERPTR
>   	bool "Always enable userptr write support"
>   	depends on DRM_AMDGPU
> +	select ARCH_HAS_HMM
>   	select HMM_MIRROR
> +	select ZONE_DEVICE
>   	help
>   	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
>   	  isn't already selected to enabled full userptr support.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Jerome Glisse]

On Wed, Feb 20, 2019 at 07:18:17PM +0000, Kuehling, Felix wrote:
> [+Jerome]
> 
> Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring 
> CPU page tables to device page tables.
> 
> ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative 
> for ARM support?
> 
> Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the 
> CPU architecture rather than any driver. Jerome, do you have any advice?

This patch is wrong you need to depend on ARCH_HAS_HMM and
select HMM_MIRROR you do not need to select ZONE_DEVICE

So it should look like:

config DRM_AMDGPU_USERPTR
	bool "Always enable userptr write support"
	depends on DRM_AMDGPU
	depends on ARCH_HAS_HMM
	select HMM_MIRROR
	help
	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
	  isn't already selected to enabled full userptr support.

I have not got around to work on amdgpu on that respect yet
but it is on my todo list unless someone else beat me to it :)

Cheers,
Jérôme

> 
> Thanks,
>    Felix
> 
> On 2019-02-20 1:56 p.m., Yang, Philip wrote:
> > Those options are needed to support HMM
> >
> > Change-Id: Ieb7bb3bcec07245d79a02793e6728228decc400a
> > Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/Kconfig | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> > index 960a63355705..63f0542bc34b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> > +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> > @@ -26,7 +26,9 @@ config DRM_AMDGPU_CIK
> >   config DRM_AMDGPU_USERPTR
> >   	bool "Always enable userptr write support"
> >   	depends on DRM_AMDGPU
> > +	select ARCH_HAS_HMM
> >   	select HMM_MIRROR
> > +	select ZONE_DEVICE
> >   	help
> >   	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
> >   	  isn't already selected to enabled full userptr support.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Kuehling, Felix]

On 2019-02-20 5:12 p.m., Jerome Glisse wrote:
> On Wed, Feb 20, 2019 at 07:18:17PM +0000, Kuehling, Felix wrote:
>> [+Jerome]
>>
>> Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring
>> CPU page tables to device page tables.
>>
>> ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative
>> for ARM support?
>>
>> Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the
>> CPU architecture rather than any driver. Jerome, do you have any advice?
> This patch is wrong you need to depend on ARCH_HAS_HMM and

Who selects ARCH_HAS_HMM? Currently I don't see this selected anywhere. 
So any config option that depends on it will be invisible in menuconfig. 
Do we need ARCH_HAS_HMM somewhere in the arch/x86/Kconfig and 
arch/powerpc/Kconfig?

Also, ARCH_HAS_HMM does not currently support ARM. Does that mean we 
can't have ARM support in AMDGPU if we start using HMM?

Finally, ARCH_HAS_HMM has a bunch of dependencies. If they are not met, 
I guess it can't be enabled. Should those be "select"s instead?

config ARCH_HAS_HMM
         bool
         default y
         depends on (X86_64 || PPC64)
         depends on ZONE_DEVICE
         depends on MMU && 64BIT
         depends on MEMORY_HOTPLUG
         depends on MEMORY_HOTREMOVE
         depends on SPARSEMEM_VMEMMAP

Regards,
   Felix

> select HMM_MIRROR you do not need to select ZONE_DEVICE
>
> So it should look like:
>
> config DRM_AMDGPU_USERPTR
> 	bool "Always enable userptr write support"
> 	depends on DRM_AMDGPU
> 	depends on ARCH_HAS_HMM
> 	select HMM_MIRROR
> 	help
> 	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
> 	  isn't already selected to enabled full userptr support.
>
> I have not got around to work on amdgpu on that respect yet
> but it is on my todo list unless someone else beat me to it :)
>
> Cheers,
> Jérôme
>
>> Thanks,
>>     Felix
>>
>> On 2019-02-20 1:56 p.m., Yang, Philip wrote:
>>> Those options are needed to support HMM
>>>
>>> Change-Id: Ieb7bb3bcec07245d79a02793e6728228decc400a
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/Kconfig | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> index 960a63355705..63f0542bc34b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> @@ -26,7 +26,9 @@ config DRM_AMDGPU_CIK
>>>    config DRM_AMDGPU_USERPTR
>>>    	bool "Always enable userptr write support"
>>>    	depends on DRM_AMDGPU
>>> +	select ARCH_HAS_HMM
>>>    	select HMM_MIRROR
>>> +	select ZONE_DEVICE
>>>    	help
>>>    	  This option selects CONFIG_HMM and CONFIG_HMM_MIRROR if it
>>>    	  isn't already selected to enabled full userptr support.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Jerome Glisse]

On Wed, Feb 20, 2019 at 10:39:49PM +0000, Kuehling, Felix wrote:
> On 2019-02-20 5:12 p.m., Jerome Glisse wrote:
> > On Wed, Feb 20, 2019 at 07:18:17PM +0000, Kuehling, Felix wrote:
> >> [+Jerome]
> >>
> >> Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring
> >> CPU page tables to device page tables.
> >>
> >> ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative
> >> for ARM support?
> >>
> >> Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the
> >> CPU architecture rather than any driver. Jerome, do you have any advice?
> > This patch is wrong you need to depend on ARCH_HAS_HMM and
> 
> Who selects ARCH_HAS_HMM? Currently I don't see this selected anywhere. 
> So any config option that depends on it will be invisible in menuconfig. 
> Do we need ARCH_HAS_HMM somewhere in the arch/x86/Kconfig and 
> arch/powerpc/Kconfig?
> 
> Also, ARCH_HAS_HMM does not currently support ARM. Does that mean we 
> can't have ARM support in AMDGPU if we start using HMM?

ARCH_HAS_HMM is defined by architecture that support HMM. So par x86
and PPC. It should not be hard to add it to ARM (i can not remember if
ARM has DAX yet or not, if ARM does not have DAX then you need to add
that first).

> 
> Finally, ARCH_HAS_HMM has a bunch of dependencies. If they are not met, 
> I guess it can't be enabled. Should those be "select"s instead?

No they should not be selected, people configuring their system need
to have the freedom of doing so. All those option are selected in all
the big distribution.

> config ARCH_HAS_HMM
>          bool
>          default y
>          depends on (X86_64 || PPC64)
>          depends on ZONE_DEVICE
>          depends on MMU && 64BIT
>          depends on MEMORY_HOTPLUG
>          depends on MEMORY_HOTREMOVE
>          depends on SPARSEMEM_VMEMMAP
> 

Cheers,
Jérôme
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Kuehling, Felix]

On 2019-02-20 6:34 p.m., Jerome Glisse wrote:
> On Wed, Feb 20, 2019 at 10:39:49PM +0000, Kuehling, Felix wrote:
>> On 2019-02-20 5:12 p.m., Jerome Glisse wrote:
>>> On Wed, Feb 20, 2019 at 07:18:17PM +0000, Kuehling, Felix wrote:
>>>> [+Jerome]
>>>>
>>>> Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring
>>>> CPU page tables to device page tables.
>>>>
>>>> ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative
>>>> for ARM support?
>>>>
>>>> Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the
>>>> CPU architecture rather than any driver. Jerome, do you have any advice?
>>> This patch is wrong you need to depend on ARCH_HAS_HMM and
>> Who selects ARCH_HAS_HMM? Currently I don't see this selected anywhere.
>> So any config option that depends on it will be invisible in menuconfig.
>> Do we need ARCH_HAS_HMM somewhere in the arch/x86/Kconfig and
>> arch/powerpc/Kconfig?
>>
>> Also, ARCH_HAS_HMM does not currently support ARM. Does that mean we
>> can't have ARM support in AMDGPU if we start using HMM?
> ARCH_HAS_HMM is defined by architecture that support HMM. So par x86
> and PPC. It should not be hard to add it to ARM (i can not remember if
> ARM has DAX yet or not, if ARM does not have DAX then you need to add
> that first).

Not having ARM support is a bummer. I just enabled KFD on ARM a few 
weeks ago. Now depending on HMM makes KFD unusable on ARM. [+Mark FYI] I 
hope this is only a temporary setback.


>> Finally, ARCH_HAS_HMM has a bunch of dependencies. If they are not met,
>> I guess it can't be enabled. Should those be "select"s instead?
> No they should not be selected, people configuring their system need
> to have the freedom of doing so. All those option are selected in all
> the big distribution.
As far as I can tell, the arch/x86/Kconfig doesn't select ARCH_HAS_HMM. 
Its default is "y", so it should be enabled on anything that meets the 
dependencies. But ZONE_DEVICE was not enabled by default. I think that's 
what broke our kernel configs.

We'll fix our own kernel configs to enable ZONE_DEVICE and ARCH_HAS_HMM 
to get our internal builds to work again.

I suspect other users with their own kernel configs will stumble over 
this and wonder why KFD and userptr support are disabled in their builds.

Regards,
   Felix


>
>> config ARCH_HAS_HMM
>>           bool
>>           default y
>>           depends on (X86_64 || PPC64)
>>           depends on ZONE_DEVICE
>>           depends on MMU && 64BIT
>>           depends on MEMORY_HOTPLUG
>>           depends on MEMORY_HOTREMOVE
>>           depends on SPARSEMEM_VMEMMAP
>>
> Cheers,
> Jérôme
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Jerome Glisse]

On Thu, Feb 21, 2019 at 12:17:33AM +0000, Kuehling, Felix wrote:
> On 2019-02-20 6:34 p.m., Jerome Glisse wrote:
> > On Wed, Feb 20, 2019 at 10:39:49PM +0000, Kuehling, Felix wrote:
> >> On 2019-02-20 5:12 p.m., Jerome Glisse wrote:
> >>> On Wed, Feb 20, 2019 at 07:18:17PM +0000, Kuehling, Felix wrote:
> >>>> [+Jerome]
> >>>>
> >>>> Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring
> >>>> CPU page tables to device page tables.
> >>>>
> >>>> ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative
> >>>> for ARM support?
> >>>>
> >>>> Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the
> >>>> CPU architecture rather than any driver. Jerome, do you have any advice?
> >>> This patch is wrong you need to depend on ARCH_HAS_HMM and
> >> Who selects ARCH_HAS_HMM? Currently I don't see this selected anywhere.
> >> So any config option that depends on it will be invisible in menuconfig.
> >> Do we need ARCH_HAS_HMM somewhere in the arch/x86/Kconfig and
> >> arch/powerpc/Kconfig?
> >>
> >> Also, ARCH_HAS_HMM does not currently support ARM. Does that mean we
> >> can't have ARM support in AMDGPU if we start using HMM?
> > ARCH_HAS_HMM is defined by architecture that support HMM. So par x86
> > and PPC. It should not be hard to add it to ARM (i can not remember if
> > ARM has DAX yet or not, if ARM does not have DAX then you need to add
> > that first).
> 
> Not having ARM support is a bummer. I just enabled KFD on ARM a few 
> weeks ago. Now depending on HMM makes KFD unusable on ARM. [+Mark FYI] I 
> hope this is only a temporary setback.

It should not be hard to add in fact all it might need is a Kconfig
patch. I have no easy access to ARM with PCIE so i have not tackle
this yet.

> 
> 
> >> Finally, ARCH_HAS_HMM has a bunch of dependencies. If they are not met,
> >> I guess it can't be enabled. Should those be "select"s instead?
> > No they should not be selected, people configuring their system need
> > to have the freedom of doing so. All those option are selected in all
> > the big distribution.
> As far as I can tell, the arch/x86/Kconfig doesn't select ARCH_HAS_HMM. 
> Its default is "y", so it should be enabled on anything that meets the 
> dependencies. But ZONE_DEVICE was not enabled by default. I think that's 
> what broke our kernel configs.
> 
> We'll fix our own kernel configs to enable ZONE_DEVICE and ARCH_HAS_HMM 
> to get our internal builds to work again.

You seem to be doing weird thing with your kconfig ...

> 
> I suspect other users with their own kernel configs will stumble over 
> this and wonder why KFD and userptr support are disabled in their builds.

Patch to improve kconfig are welcome but they should not force select
thing. Configuration is there to give user freedom to select fewature
they want to give up.

Maybe following would help:
ARCH_HAS_HMM
-	bool
-	default y
+	def_bool y

Cheers,
Jérôme
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: select ARCH_HAS_HMM and ZONE_DEVICE option

[Yang, Philip]

Thanks Jerome for the the correct HMM config option, only select 
HMM_MIRROR is not good enough because CONFIG_HMM option maybe missing, 
add depends on ARCH_HAS_HMM will solve the issue.

I will submit new patch to fix the compilation error if HMM_MIRROR 
config is missing and the HMM config dependency issue.

Philip

On 2019-02-20 7:25 p.m., Jerome Glisse wrote:
> On Thu, Feb 21, 2019 at 12:17:33AM +0000, Kuehling, Felix wrote:
>> On 2019-02-20 6:34 p.m., Jerome Glisse wrote:
>>> On Wed, Feb 20, 2019 at 10:39:49PM +0000, Kuehling, Felix wrote:
>>>> On 2019-02-20 5:12 p.m., Jerome Glisse wrote:
>>>>> On Wed, Feb 20, 2019 at 07:18:17PM +0000, Kuehling, Felix wrote:
>>>>>> [+Jerome]
>>>>>>
>>>>>> Why to we need ZONE_DEVICE. I didn't think this was needed for mirroring
>>>>>> CPU page tables to device page tables.
>>>>>>
>>>>>> ARCH_HAS_HMM depends on (X86_64 || PPC64). Do we have some alternative
>>>>>> for ARM support?
>>>>>>
>>>>>> Also, the name ARCH_HAS_HMM looks like it's meant to be selected by the
>>>>>> CPU architecture rather than any driver. Jerome, do you have any advice?
>>>>> This patch is wrong you need to depend on ARCH_HAS_HMM and
>>>> Who selects ARCH_HAS_HMM? Currently I don't see this selected anywhere.
>>>> So any config option that depends on it will be invisible in menuconfig.
>>>> Do we need ARCH_HAS_HMM somewhere in the arch/x86/Kconfig and
>>>> arch/powerpc/Kconfig?
>>>>
>>>> Also, ARCH_HAS_HMM does not currently support ARM. Does that mean we
>>>> can't have ARM support in AMDGPU if we start using HMM?
>>> ARCH_HAS_HMM is defined by architecture that support HMM. So par x86
>>> and PPC. It should not be hard to add it to ARM (i can not remember if
>>> ARM has DAX yet or not, if ARM does not have DAX then you need to add
>>> that first).
>>
>> Not having ARM support is a bummer. I just enabled KFD on ARM a few
>> weeks ago. Now depending on HMM makes KFD unusable on ARM. [+Mark FYI] I
>> hope this is only a temporary setback.
> 
> It should not be hard to add in fact all it might need is a Kconfig
> patch. I have no easy access to ARM with PCIE so i have not tackle
> this yet.
> 
>>
>>
>>>> Finally, ARCH_HAS_HMM has a bunch of dependencies. If they are not met,
>>>> I guess it can't be enabled. Should those be "select"s instead?
>>> No they should not be selected, people configuring their system need
>>> to have the freedom of doing so. All those option are selected in all
>>> the big distribution.
>> As far as I can tell, the arch/x86/Kconfig doesn't select ARCH_HAS_HMM.
>> Its default is "y", so it should be enabled on anything that meets the
>> dependencies. But ZONE_DEVICE was not enabled by default. I think that's
>> what broke our kernel configs.
>>
>> We'll fix our own kernel configs to enable ZONE_DEVICE and ARCH_HAS_HMM
>> to get our internal builds to work again.
> 
> You seem to be doing weird thing with your kconfig ...
> 
>>
>> I suspect other users with their own kernel configs will stumble over
>> this and wonder why KFD and userptr support are disabled in their builds.
> 
> Patch to improve kconfig are welcome but they should not force select
> thing. Configuration is there to give user freedom to select fewature
> they want to give up.
> 
> Maybe following would help:
> ARCH_HAS_HMM
> -	bool
> -	default y
> +	def_bool y
> 
> Cheers,
> Jérôme
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx