Re: [PATCH] drm: Harmonize CIK ASIC support in radeon and amdgpu (v2)

2017-05-21 Thread Michel Dänzer 
On 20/05/17 03:55 AM, Felix Kuehling wrote:
> On 17-05-18 09:17 PM, Michel Dänzer wrote:
>> Obviously, out-of-tree module builds will have to continue doing what
>> they've been doing so far with kernels without your patch. I'm thinking
>> about making it easier for them with kernels which do have your patch.
>> At some point in the future, maybe the support for kernels without your
>> patch can be dropped entirely.
> 
> I think out-of-tree builds using the CONFIG_AMDGPU_CIK option of the
> target kernel is not safe. Hybrid should use its own option that is
> determined by what the hybrid stack supports (kernel and user mode).

Sure, that's not what I mean. I mean out-of-tree builds can use

 options radeon ci_support=0 si_support=0

e.g. in /etc/modprobe.d/amdgpu-dkms.conf to prefer amdgpu in a cleaner
way than by blacklisting radeon.


 The default at this point should possibly still be for CIK GPUs to be
 driven by radeon, even if CONFIG_DRM_AMDGPU_CIK is enabled;
>>> Alex and Christian seem to think otherwise.
>> FWIW, on my AMD notebook (HP EliteBook 725 G2, Kaveri), suspend/resume
>> with amdgpu results in a black screen (can reboot blindly); works fine
>> with radeon.
> 
> Has this problem been reported anywhere? Is anyone working on fixing it?
> If it's not reported, it will never be fixed.

You're right, I'll file a bug report.


Another issue is the lack of HDMI / DP audio support without DC. It
seems weird to change the default to amdgpu before there is feature parity.


Anyway, go ahead and land your latest series, you can have my

Acked-by: Michel Dänzer 

for that. I'll propose some changes on top of that.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

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

Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

2017-05-21 Thread Michel Dänzer 
On 20/05/17 12:21 PM, John Brooks wrote:
> On Sat, May 20, 2017 at 10:27:14AM +0900, Michel Dänzer wrote:
>> On 20/05/17 04:23 AM, John Brooks wrote:
>>> On Fri, May 19, 2017 at 04:03:28PM +0900, Michel Dänzer wrote:
 On 19/05/17 12:04 PM, John Brooks wrote:
> Set GTT as the busy placement for newly created BOs that have the
> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
> established BOs to be evicted from visible VRAM.

 This is an interesting idea, but there are some issues with this patch.
>>
>> [...]
>>
> + flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> + bo->placement.num_placement = 1;
> + bo->placement.num_busy_placement = 1;
> + bo->placement.busy_placement = >placement.placement[1];
> + }

 The original placements set by amdgpu_fill_placement_to_bo need to be
 restored before returning from this function, otherwise I suspect such
 BOs which end up being created in GTT will stay there permanently.

>>>
>>> I'm curious, what makes you think that the BOs cannot move back to VRAM
>>> otherwise? VRAM is still in the placements and prefered/allowed domains, 
>>> just
>>> not in the busy placements.
>>
>> If there is not enough free space when the BO is created, there probably
>> won't be either when it's validated for GPU command streams later.
>>
>>
 Does the patch still help for Dying Light if you fix this?
>>
>> Please test this. The result should tell us whether the problem with
>> Dying Light is really pressure on CPU visible VRAM, or something else.
>>
> 
> I did some tests. The patch still helps if I restore the old placement values
> after ttm_bo_init_reserved. But while doing this, I made another observation
> that throws a wrench into things: It *does* kill performance if I remove
> AMDGPU_GEM_DOMAIN_GTT from bo->prefered_domains. I think that GTT getting into
> prefered_domains was what "fixed" the game this whole time and the BO creation
> was irrelevant.

Ah, right, so the problem I was thinking of was actually caused by the
second hunk, not the first one. :) amdgpu_cs_bo_validate re-generates
the placements for each command stream according to bo->prefered_domains
or bo->allowed_domains.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-21 Thread Michel Dänzer 
On 22/05/17 10:08 AM, Michel Dänzer wrote:
> On 19/05/17 07:02 PM, arindam.n...@amd.com wrote:
>> From: Arindam Nath 
>>
>> Change History
>> --
>>
>> v2: changes suggested by Joerg
>> - add flush flag to improve efficiency of flush operation
>>
>> v1:
>> - The idea behind flush queues is to defer the IOTLB flushing
>>   for domains for which the mappings are no longer valid. We
>>   add such domains in queue_add(), and when the queue size
>>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>>   Since we have already taken lock before __queue_flush()
>>   is called, we need to make sure the IOTLB flushing is
>>   performed as quickly as possible.
>>
>>   In the current implementation, we perform IOTLB flushing
>>   for all domains irrespective of which ones were actually
>>   added in the flush queue initially. This can be quite
>>   expensive especially for domains for which unmapping is
>>   not required at this point of time.
>>
>>   This patch makes use of domain information in
>>   'struct flush_queue_entry' to make sure we only flush
>>   IOTLBs for domains who need it, skipping others.
>>
>> Suggested-by: Joerg Roedel 
>> Signed-off-by: Arindam Nath 
> 
> Please add these tags:
> 
> Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
> Cc: sta...@vger.kernel.org

Also

Bugzilla: https://bugs.freedesktop.org/101029


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-21 Thread Michel Dänzer 
On 19/05/17 07:02 PM, arindam.n...@amd.com wrote:
> From: Arindam Nath 
> 
> Change History
> --
> 
> v2: changes suggested by Joerg
> - add flush flag to improve efficiency of flush operation
> 
> v1:
> - The idea behind flush queues is to defer the IOTLB flushing
>   for domains for which the mappings are no longer valid. We
>   add such domains in queue_add(), and when the queue size
>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
> 
>   Since we have already taken lock before __queue_flush()
>   is called, we need to make sure the IOTLB flushing is
>   performed as quickly as possible.
> 
>   In the current implementation, we perform IOTLB flushing
>   for all domains irrespective of which ones were actually
>   added in the flush queue initially. This can be quite
>   expensive especially for domains for which unmapping is
>   not required at this point of time.
> 
>   This patch makes use of domain information in
>   'struct flush_queue_entry' to make sure we only flush
>   IOTLBs for domains who need it, skipping others.
> 
> Suggested-by: Joerg Roedel 
> Signed-off-by: Arindam Nath 

Please add these tags:

Fixes: b1516a14657a ("iommu/amd: Implement flush queue")
Cc: sta...@vger.kernel.org


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/radeon: Fix oops upon driver load on PowerXpress laptops

2017-05-21 Thread Nicolai Stange 
On Thu, May 18 2017, Lukas Wunner wrote:



> Reported-by: Nicolai Stange 
> Fixes: 7ffb0ce31cf9 ("drm/radeon: Don't register Thunderbolt eGPU with 
> vga_switcheroo")
> Signed-off-by: Lukas Wunner 
> ---
>
> Awaiting a Tested-by: from Nicolai, but it's clear this is a bug and
> needs to be fixed, so sending out with a proper commit message now.
> The bug was only introduced to radeon, not amdgpu.

Tested-by: Nicolai Stange 

Thanks for the quick fix!


> @Alex Deucher: I could push this to drm-misc-fixes but then it wouldn't
> land before -rc3 because Sean Paul has already sent out the -rc2 pull.
> I notice you haven't sent out a pull for -rc2 yet, so maybe you want to
> take it yourself?  Whichever you prefer.  Thanks & sorry for the breakage!
>
>  drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
> b/drivers/gpu/drm/radeon/radeon_kms.c
> index 6a68d440bc44..d0ad03674250 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -116,7 +116,7 @@ int radeon_driver_load_kms(struct drm_device *dev, 
> unsigned long flags)
>   if ((radeon_runtime_pm != 0) &&
>   radeon_has_atpx() &&
>   ((flags & RADEON_IS_IGP) == 0) &&
> - !pci_is_thunderbolt_attached(rdev->pdev))
> + !pci_is_thunderbolt_attached(dev->pdev))
>   flags |= RADEON_IS_PX;
>  
>   /* radeon_device_init should report only fatal error
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)

2017-05-21 Thread Nath, Arindam 
>-Original Message-
>From: Jan Vesely [mailto:jv...@scarletmail.rutgers.edu] On Behalf Of Jan
>Vesely
>Sent: Friday, May 19, 2017 7:06 PM
>To: Nath, Arindam; io...@lists.linux-foundation.org
>Cc: Bridgman, John; Joerg Roedel; amd-gfx@lists.freedesktop.org;
>dr...@endlessm.com; Craig Stein; Suthikulpanit, Suravee; Deucher,
>Alexander; Kuehling, Felix; li...@endlessm.com; mic...@daenzer.net
>Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only (v2)
>
>On Fri, 2017-05-19 at 15:32 +0530, arindam.n...@amd.com wrote:
>> From: Arindam Nath 
>>
>> Change History
>> --
>>
>> v2: changes suggested by Joerg
>> - add flush flag to improve efficiency of flush operation

Joerg, do you have any comments on the patch?

Thanks,
Arindam

>>
>> v1:
>> - The idea behind flush queues is to defer the IOTLB flushing
>>   for domains for which the mappings are no longer valid. We
>>   add such domains in queue_add(), and when the queue size
>>   reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>>   Since we have already taken lock before __queue_flush()
>>   is called, we need to make sure the IOTLB flushing is
>>   performed as quickly as possible.
>>
>>   In the current implementation, we perform IOTLB flushing
>>   for all domains irrespective of which ones were actually
>>   added in the flush queue initially. This can be quite
>>   expensive especially for domains for which unmapping is
>>   not required at this point of time.
>>
>>   This patch makes use of domain information in
>>   'struct flush_queue_entry' to make sure we only flush
>>   IOTLBs for domains who need it, skipping others.
>
>Hi,
>
>just a note, the patch also fixes "AMD-Vi: Completion-Wait loop timed
>out" boot hang on my system (carrizo + iceland) [0,1,2]. Presumably the
>old loop also tried to flush domains that included powered-off devices.
>
>regards,
>Jan
>
>[0] https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver/issues/20
>[1] https://bugs.freedesktop.org/show_bug.cgi?id=101029
>[2] https://bugzilla.redhat.com/show_bug.cgi?id=1448121
>
>>
>> Suggested-by: Joerg Roedel 
>> Signed-off-by: Arindam Nath 
>> ---
>>  drivers/iommu/amd_iommu.c   | 27 ---
>>  drivers/iommu/amd_iommu_types.h |  2 ++
>>  2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 63cacf5..1edeebec 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,26 @@ static struct iommu_group
>*amd_iommu_device_group(struct device *dev)
>>
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -struct protection_domain *domain;
>> -unsigned long flags;
>>  int idx;
>>
>> -/* First flush TLB of all known domains */
>> -spin_lock_irqsave(_iommu_pd_lock, flags);
>> -list_for_each_entry(domain, _iommu_pd_list, list)
>> -domain_flush_tlb(domain);
>> -spin_unlock_irqrestore(_iommu_pd_lock, flags);
>> +/* First flush TLB of all domains which were added to flush queue */
>> +for (idx = 0; idx < queue->next; ++idx) {
>> +struct flush_queue_entry *entry;
>> +
>> +entry = queue->entries + idx;
>> +
>> +/*
>> + * There might be cases where multiple IOVA entries for the
>> + * same domain are queued in the flush queue. To avoid
>> + * flushing the same domain again, we check whether the
>> + * flag is set or not. This improves the efficiency of
>> + * flush operation.
>> + */
>> +if (!entry->dma_dom->domain.already_flushed) {
>> +entry->dma_dom->domain.already_flushed = true;
>> +domain_flush_tlb(>dma_dom->domain);
>> +}
>> +}
>>
>>  /* Wait until flushes have completed */
>>  domain_flush_complete(NULL);
>> @@ -2289,6 +2300,8 @@ static void queue_add(struct dma_ops_domain
>*dma_dom,
>>  pages = __roundup_pow_of_two(pages);
>>  address >>= PAGE_SHIFT;
>>
>> +dma_dom->domain.already_flushed = false;
>> +
>>  queue = get_cpu_ptr(_queue);
>>  spin_lock_irqsave(>lock, flags);
>>
>> diff --git a/drivers/iommu/amd_iommu_types.h
>b/drivers/iommu/amd_iommu_types.h
>> index 4de8f41..4f5519d 100644
>> --- a/drivers/iommu/amd_iommu_types.h
>> +++ b/drivers/iommu/amd_iommu_types.h
>> @@ -454,6 +454,8 @@ struct protection_domain {
>>  bool updated;   /* complete domain flush required */
>>  unsigned dev_cnt;   /* devices assigned to this domain */
>>  unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference
>count */
>> +bool already_flushed;   /* flag to avoid flushing the same domain
>again
>> +   in a single invocation of __queue_flush() */
>>  };
>>
>>  /*
>
>--
>Jan Vesely