[6/8] drm/amd/display: enable GPU VM support

Submitted by Christian König on Oct. 26, 2017, 4:06 p.m.

Details

Message ID 1509033983-2977-6-git-send-email-deathsimple@vodafone.de
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Christian König Oct. 26, 2017, 4:06 p.m.
From: Christian König <christian.koenig@amd.com>

Just set the bit so that DC does the hardware programming.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 2188f20..ed4351a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -417,6 +417,8 @@  static int amdgpu_dm_init(struct amdgpu_device *adev)
 
 	init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
 
+	init_data.flags.gpu_vm_support = true;
+
 	if (amdgpu_dc_log)
 		init_data.log_mask = DC_DEFAULT_LOG_MASK;
 	else

Comments

> -----Original Message-----

> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Christian König

> Sent: Thursday, October 26, 2017 12:06 PM

> To: amd-gfx@lists.freedesktop.org

> Subject: [PATCH 6/8] drm/amd/display: enable GPU VM support

> 

> From: Christian König <christian.koenig@amd.com>

> 

> Just set the bit so that DC does the hardware programming.

> 

> Signed-off-by: Christian König <christian.koenig@amd.com>


Acked-by: Alex Deucher <alexander.deucher@amd.com>


> ---

>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> index 2188f20..ed4351a 100644

> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device

> *adev)

> 

>  	init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;

> 

> +	init_data.flags.gpu_vm_support = true;

> +

>  	if (amdgpu_dc_log)

>  		init_data.log_mask = DC_DEFAULT_LOG_MASK;

>  	else

> --

> 2.7.4

> 

> _______________________________________________

> amd-gfx mailing list

> amd-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Reviewed-by: Andrey Grodzovsky<andrey.grodzovsky@amd.com>


On 2017-10-26 12:06 PM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Just set the bit so that DC does the hardware programming.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2188f20..ed4351a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   
>   	init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>   
> +	init_data.flags.gpu_vm_support = true;
> +
>   	if (amdgpu_dc_log)
>   		init_data.log_mask = DC_DEFAULT_LOG_MASK;
>   	else
On 2017-10-26 12:06 PM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> Just set the bit so that DC does the hardware programming.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2188f20..ed4351a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>  
>  	init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>  
> +	init_data.flags.gpu_vm_support = true;
> +

Hi Christian,

what ASIC has this been tested on?

I'm not sure that this won't break dGPU. dGPU doesn't have vm_support but I think DC will still try to write the same registers as for CZ on dGPU if we enable this. We at least need to make sure it doesn't blow up.

As for Raven we use this flag on Windows but only tested the case where VM is actually programmed. If VM isn't programmed it might still work but we'd need to give that a spin.

Harry

>  	if (amdgpu_dc_log)
>  		init_data.log_mask = DC_DEFAULT_LOG_MASK;
>  	else
>
Am 27.10.2017 um 16:37 schrieb Harry Wentland:
> On 2017-10-26 12:06 PM, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> Just set the bit so that DC does the hardware programming.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 2188f20..ed4351a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>>   
>>   	init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>>   
>> +	init_data.flags.gpu_vm_support = true;
>> +
> Hi Christian,
>
> what ASIC has this been tested on?

Carizzo.

> I'm not sure that this won't break dGPU. dGPU doesn't have vm_support but I think DC will still try to write the same registers as for CZ on dGPU if we enable this. We at least need to make sure it doesn't blow up.

Yeah, as Tom already stumbled over as well it causes a warning on Tonga. 
Currently giving it a spin on Polaris.

> As for Raven we use this flag on Windows but only tested the case where VM is actually programmed. If VM isn't programmed it might still work but we'd need to give that a spin.

Without this flag tilling fails and only displays a quite unusual pattern.

I would actually prefer if we can completely remove this flag and 
instead always enable the VM programming on supported hardware.

Is there any downside on doing so?

Christian.

>
> Harry
>
>>   	if (amdgpu_dc_log)
>>   		init_data.log_mask = DC_DEFAULT_LOG_MASK;
>>   	else
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 27/10/17 10:48 AM, Christian König wrote:
> Am 27.10.2017 um 16:37 schrieb Harry Wentland:
>> On 2017-10-26 12:06 PM, Christian König wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Just set the bit so that DC does the hardware programming.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 2188f20..ed4351a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device 
>>> *adev)
>>>       init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>>> +    init_data.flags.gpu_vm_support = true;
>>> +
>> Hi Christian,
>>
>> what ASIC has this been tested on?
> 
> Carizzo.
> 
>> I'm not sure that this won't break dGPU. dGPU doesn't have vm_support 
>> but I think DC will still try to write the same registers as for CZ on 
>> dGPU if we enable this. We at least need to make sure it doesn't blow up.
> 
> Yeah, as Tom already stumbled over as well it causes a warning on Tonga. 
> Currently giving it a spin on Polaris.

I saw the warnings on my CZ+P10 setup but I didn't note which device was 
complaining.

And I'd like to think I journeyed into it on my meandering path of 
testing new commits on the hodgepodge of gear I have :-).

Tom

>> As for Raven we use this flag on Windows but only tested the case 
>> where VM is actually programmed. If VM isn't programmed it might still 
>> work but we'd need to give that a spin.
> 
> Without this flag tilling fails and only displays a quite unusual pattern.
> 
> I would actually prefer if we can completely remove this flag and 
> instead always enable the VM programming on supported hardware.
> 
> Is there any downside on doing so?
> 
> Christian.
> 
>>
>> Harry
>>
>>>       if (amdgpu_dc_log)
>>>           init_data.log_mask = DC_DEFAULT_LOG_MASK;
>>>       else
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 27/10/17 04:48 PM, Christian König wrote:
> Am 27.10.2017 um 16:37 schrieb Harry Wentland:
>> On 2017-10-26 12:06 PM, Christian König wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Just set the bit so that DC does the hardware programming.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 2188f20..ed4351a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device
>>> *adev)
>>>         init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>>>   +    init_data.flags.gpu_vm_support = true;
>>> +
>> Hi Christian,
>>
>> what ASIC has this been tested on?
> 
> Carizzo.
> 
>> I'm not sure that this won't break dGPU. dGPU doesn't have vm_support
>> but I think DC will still try to write the same registers as for CZ on
>> dGPU if we enable this. We at least need to make sure it doesn't blow up.
> 
> Yeah, as Tom already stumbled over as well it causes a warning on Tonga.
> Currently giving it a spin on Polaris.
> 
>> As for Raven we use this flag on Windows but only tested the case
>> where VM is actually programmed. If VM isn't programmed it might still
>> work but we'd need to give that a spin.
> 
> Without this flag tilling fails and only displays a quite unusual pattern.
> 
> I would actually prefer if we can completely remove this flag and
> instead always enable the VM programming on supported hardware.
> 
> Is there any downside on doing so?

Is there a particular use-case for scanout from system memory with a
dGPU? Otherwise I suspect it's better not to bother, because the failure
mode might be pretty bad if the PCIe link can't keep up with scanout.
Am 27.10.2017 um 16:58 schrieb Michel Dänzer:
> On 27/10/17 04:48 PM, Christian König wrote:
>> Am 27.10.2017 um 16:37 schrieb Harry Wentland:
>>> On 2017-10-26 12:06 PM, Christian König wrote:
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> Just set the bit so that DC does the hardware programming.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 2188f20..ed4351a 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device
>>>> *adev)
>>>>          init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>>>>    +    init_data.flags.gpu_vm_support = true;
>>>> +
>>> Hi Christian,
>>>
>>> what ASIC has this been tested on?
>> Carizzo.
>>
>>> I'm not sure that this won't break dGPU. dGPU doesn't have vm_support
>>> but I think DC will still try to write the same registers as for CZ on
>>> dGPU if we enable this. We at least need to make sure it doesn't blow up.
>> Yeah, as Tom already stumbled over as well it causes a warning on Tonga.
>> Currently giving it a spin on Polaris.
>>
>>> As for Raven we use this flag on Windows but only tested the case
>>> where VM is actually programmed. If VM isn't programmed it might still
>>> work but we'd need to give that a spin.
>> Without this flag tilling fails and only displays a quite unusual pattern.
>>
>> I would actually prefer if we can completely remove this flag and
>> instead always enable the VM programming on supported hardware.
>>
>> Is there any downside on doing so?
> Is there a particular use-case for scanout from system memory with a
> dGPU? Otherwise I suspect it's better not to bother, because the failure
> mode might be pretty bad if the PCIe link can't keep up with scanout.

That's true and I actually didn't want to suggest that we scanout from 
system memory on dGPUs.

But setting up the tilling information in the DCE/DCN VM block anyway 
looks harmless to me.

And scanning out from scattered VRAM at some point still seems to make 
sense to me.

Regards,
Christian.
On 2017-10-27 10:48 AM, Christian König wrote:
> Am 27.10.2017 um 16:37 schrieb Harry Wentland:
>> On 2017-10-26 12:06 PM, Christian König wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Just set the bit so that DC does the hardware programming.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 2188f20..ed4351a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>>>         init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>>>   +    init_data.flags.gpu_vm_support = true;
>>> +
>> Hi Christian,
>>
>> what ASIC has this been tested on?
> 
> Carizzo.
> 
>> I'm not sure that this won't break dGPU. dGPU doesn't have vm_support but I think DC will still try to write the same registers as for CZ on dGPU if we enable this. We at least need to make sure it doesn't blow up.
> 
> Yeah, as Tom already stumbled over as well it causes a warning on Tonga. Currently giving it a spin on Polaris.
> 
>> As for Raven we use this flag on Windows but only tested the case where VM is actually programmed. If VM isn't programmed it might still work but we'd need to give that a spin.
> 
> Without this flag tilling fails and only displays a quite unusual pattern.
> 
> I would actually prefer if we can completely remove this flag and instead always enable the VM programming on supported hardware.
> 
> Is there any downside on doing so?
> 

The way this flag is currently used is for the base driver (amdgpu on Linux, kmd on Windows) to let us know whether VM is being used or not. The options are

1) amdgpu lets us know whether the framebuffer is in virtual memory or not
2) we assume amdgpu always uses virtual memory on ASICs that support it, set this flag to true (as in this patch) and make sure DC only programs it on ASICs that support it

I'd somewhat prefer option 2 as it's simpler from an amdgpu perspective and also keeps DC consistent.

Harry

> Christian.
> 
>>
>> Harry
>>
>>>       if (amdgpu_dc_log)
>>>           init_data.log_mask = DC_DEFAULT_LOG_MASK;
>>>       else
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
>
Am 27.10.2017 um 17:40 schrieb Harry Wentland:
> On 2017-10-27 10:48 AM, Christian König wrote:
>> Am 27.10.2017 um 16:37 schrieb Harry Wentland:
>>> On 2017-10-26 12:06 PM, Christian König wrote:
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> Just set the bit so that DC does the hardware programming.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 2188f20..ed4351a 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -417,6 +417,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>>>>          init_data.dce_environment = DCE_ENV_PRODUCTION_DRV;
>>>>    +    init_data.flags.gpu_vm_support = true;
>>>> +
>>> Hi Christian,
>>>
>>> what ASIC has this been tested on?
>> Carizzo.
>>
>>> I'm not sure that this won't break dGPU. dGPU doesn't have vm_support but I think DC will still try to write the same registers as for CZ on dGPU if we enable this. We at least need to make sure it doesn't blow up.
>> Yeah, as Tom already stumbled over as well it causes a warning on Tonga. Currently giving it a spin on Polaris.
>>
>>> As for Raven we use this flag on Windows but only tested the case where VM is actually programmed. If VM isn't programmed it might still work but we'd need to give that a spin.
>> Without this flag tilling fails and only displays a quite unusual pattern.
>>
>> I would actually prefer if we can completely remove this flag and instead always enable the VM programming on supported hardware.
>>
>> Is there any downside on doing so?
>>
> The way this flag is currently used is for the base driver (amdgpu on Linux, kmd on Windows) to let us know whether VM is being used or not. The options are
>
> 1) amdgpu lets us know whether the framebuffer is in virtual memory or not
> 2) we assume amdgpu always uses virtual memory on ASICs that support it, set this flag to true (as in this patch) and make sure DC only programs it on ASICs that support it
>
> I'd somewhat prefer option 2 as it's simpler from an amdgpu perspective and also keeps DC consistent.

I agree that option 2 sounds better. In this case please provide DC 
patches to fix the warning and clean things up.

BTW: I've also get a bunch of warning from the DRM core about things DC 
does. You're probably aware of those problem, but I just wanted to note 
once more that the backtraces in the log are kind of annoying.

Regards,
Christian.

>
> Harry
>
>> Christian.
>>
>>> Harry
>>>
>>>>        if (amdgpu_dc_log)
>>>>            init_data.log_mask = DC_DEFAULT_LOG_MASK;
>>>>        else
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx