Re: [BUG] gpu: drm: radeon: two possible deadlocks involving locking and waiting
Hi Jia-Ju, interesting that you have found those issues with an automated tool. And yes that is a well design flaw within the radeon driver which can happen on hardware faults, e.g. when radeon_ring_backup() needs to be called. But that happens so rarely and the driver is not developed further that we decided to not address this any more. Regards, Christian. Am 01.02.22 um 08:40 schrieb Jia-Ju Bai: Hello, My static analysis tool reports a possible deadlock in the radeon driver in Linux 5.16: #BUG 1 radeon_dpm_change_power_state_locked() mutex_lock(>ring_lock); --> Line 1133 (Lock A) radeon_fence_wait_empty() radeon_fence_wait_seq_timeout() wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X) radeon_ring_backup() mutex_lock(>ring_lock); --> Line 289(Lock A) radeon_fence_count_emitted() radeon_fence_process() wake_up_all(>fence_queue); --> Line 323 (Wake X) When radeon_dpm_change_power_state_locked() is executed, "Wait X" is performed by holding "Lock A". If radeon_ring_backup() is executed at this time, "Wake X" cannot be performed to wake up "Wait X" in radeon_dpm_change_power_state_locked(), because "Lock A" has been already hold by radeon_dpm_change_power_state_locked(), causing a possible deadlock. I find that "Wait X" is performed with a timeout MAX_SCHEDULE_TIMEOUT, to relieve the possible deadlock; but I think this timeout can cause inefficient execution. #BUG 2 radeon_ring_lock() mutex_lock(>ring_lock); --> Line 147 (Lock A) radeon_ring_alloc() radeon_fence_wait_next() radeon_fence_wait_seq_timeout() wait_event_timeout(rdev->fence_queue, ...) --> Line 504 (Wait X) radeon_ring_backup() mutex_lock(>ring_lock); --> Line 289(Lock A) radeon_fence_count_emitted() radeon_fence_process() wake_up_all(>fence_queue); --> Line 323 (Wake X) When radeon_ring_lock() is executed, "Wait X" is performed by holding "Lock A". If radeon_ring_backup() is executed at this time, "Wake X" cannot be performed to wake up "Wait X" in radeon_ring_lock(), because "Lock A" has been already hold by radeon_ring_lock(), causing a possible deadlock. I find that "Wait X" is performed with a timeout MAX_SCHEDULE_TIMEOUT, to relieve the possible deadlock; but I think this timeout can cause inefficient execution. I am not quite sure whether these possible problems are real and how to fix them if they are real. Any feedback would be appreciated, thanks :) Best wishes, Jia-Ju Bai
Re: [PATCH 4/4] drm/amdgpu: restructure amdgpu_fill_buffer v2
Am 2022-01-31 um 08:52 schrieb Christian König:
We ran into the problem that clearing really larger buffer (60GiB) caused an
SDMA timeout.
Restructure the function to use the dst window instead of mapping the whole
buffer into the GART and then fill only 2MiB/256MiB chunks at a time.
v2: rebase on restructured window map.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 187 +---
1 file changed, 105 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 26c521cd1092..b9637d1cf147 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -396,8 +396,7 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE)) {
struct dma_fence *wipe_fence = NULL;
- r = amdgpu_fill_buffer(ttm_to_amdgpu_bo(bo), AMDGPU_POISON,
- NULL, _fence);
+ r = amdgpu_fill_buffer(abo, AMDGPU_POISON, NULL, _fence);
if (r) {
goto error;
} else if (wipe_fence) {
@@ -1923,19 +1922,51 @@ void amdgpu_ttm_set_buffer_funcs_status(struct
amdgpu_device *adev, bool enable)
adev->mman.buffer_funcs_enabled = enable;
}
+static int amdgpu_ttm_prepare_job(struct amdgpu_device *adev,
+ bool direct_submit,
+ unsigned int num_dw,
+ struct dma_resv *resv,
+ bool vm_needs_flush,
+ struct amdgpu_job **job)
+{
+ enum amdgpu_ib_pool_type pool = direct_submit ?
+ AMDGPU_IB_POOL_DIRECT :
+ AMDGPU_IB_POOL_DELAYED;
+ int r;
+
+ r = amdgpu_job_alloc_with_ib(adev, num_dw * 4, pool, job);
+ if (r)
+ return r;
+
+ if (vm_needs_flush) {
+ (*job)->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
+ adev->gmc.pdb0_bo :
+ adev->gart.bo);
+ (*job)->vm_needs_flush = true;
+ }
+ if (resv) {
+ r = amdgpu_sync_resv(adev, &(*job)->sync, resv,
+AMDGPU_SYNC_ALWAYS,
+AMDGPU_FENCE_OWNER_UNDEFINED);
+ if (r) {
+ DRM_ERROR("sync failed (%d).\n", r);
+ amdgpu_job_free(*job);
+ return r;
+ }
+ }
+ return 0;
+}
+
int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
uint64_t dst_offset, uint32_t byte_count,
struct dma_resv *resv,
struct dma_fence **fence, bool direct_submit,
bool vm_needs_flush, bool tmz)
{
- enum amdgpu_ib_pool_type pool = direct_submit ? AMDGPU_IB_POOL_DIRECT :
- AMDGPU_IB_POOL_DELAYED;
struct amdgpu_device *adev = ring->adev;
+ unsigned num_loops, num_dw;
struct amdgpu_job *job;
-
uint32_t max_bytes;
- unsigned num_loops, num_dw;
unsigned i;
int r;
@@ -1947,26 +1978,11 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
max_bytes = adev->mman.buffer_funcs->copy_max_bytes;
num_loops = DIV_ROUND_UP(byte_count, max_bytes);
num_dw = ALIGN(num_loops * adev->mman.buffer_funcs->copy_num_dw, 8);
-
- r = amdgpu_job_alloc_with_ib(adev, num_dw * 4, pool, );
+ r = amdgpu_ttm_prepare_job(adev, direct_submit, num_dw,
+ resv, vm_needs_flush, );
if (r)
return r;
- if (vm_needs_flush) {
- job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
- adev->gmc.pdb0_bo : adev->gart.bo);
- job->vm_needs_flush = true;
- }
- if (resv) {
- r = amdgpu_sync_resv(adev, >sync, resv,
-AMDGPU_SYNC_ALWAYS,
-AMDGPU_FENCE_OWNER_UNDEFINED);
- if (r) {
- DRM_ERROR("sync failed (%d).\n", r);
- goto error_free;
- }
- }
-
for (i = 0; i < num_loops; i++) {
uint32_t cur_size_in_bytes = min(byte_count, max_bytes);
@@ -1996,77 +2012,35 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
return r;
}
-int amdgpu_fill_buffer(struct amdgpu_bo *bo,
- uint32_t src_data,
- struct dma_resv *resv,
- struct dma_fence **fence)
+static int amdgpu_ttm_fill_mem(struct amdgpu_ring *ring, uint32_t src_data,
+
Re: [PATCH v5 09/10] tools: update hmm-test to support device coherent type
Hi Alistair,
This is the last patch to be reviewed from this series. It already has
the changes from
your last feedback (V3). Would you mind to take a look?
Thanks a lot for reviewing the rest!
Regards,
Alex Sierra
On 1/28/2022 2:08 PM, Alex Sierra wrote:
Test cases such as migrate_fault and migrate_multiple, were modified to
explicit migrate from device to sys memory without the need of page
faults, when using device coherent type.
Snapshot test case updated to read memory device type first and based
on that, get the proper returned results migrate_ping_pong test case
added to test explicit migration from device to sys memory for both
private and coherent zone types.
Helpers to migrate from device to sys memory and vicerversa
were also added.
Signed-off-by: Alex Sierra
Acked-by: Felix Kuehling
---
v2:
Set FIXTURE_VARIANT to add multiple device types to the FIXTURE. This
will run all the tests for each device type (private and coherent) in
case both existed during hmm-test driver probed.
v4:
Check for the number of pages successfully migrated from coherent
device to system at migrate_multiple test.
---
tools/testing/selftests/vm/hmm-tests.c | 123 -
1 file changed, 102 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/vm/hmm-tests.c
b/tools/testing/selftests/vm/hmm-tests.c
index 203323967b50..84ec8c4a1dc7 100644
--- a/tools/testing/selftests/vm/hmm-tests.c
+++ b/tools/testing/selftests/vm/hmm-tests.c
@@ -44,6 +44,14 @@ struct hmm_buffer {
int fd;
uint64_tcpages;
uint64_tfaults;
+ int zone_device_type;
+};
+
+enum {
+ HMM_PRIVATE_DEVICE_ONE,
+ HMM_PRIVATE_DEVICE_TWO,
+ HMM_COHERENCE_DEVICE_ONE,
+ HMM_COHERENCE_DEVICE_TWO,
};
#define TWOMEG (1 << 21)
@@ -60,6 +68,21 @@ FIXTURE(hmm)
unsigned intpage_shift;
};
+FIXTURE_VARIANT(hmm)
+{
+ int device_number;
+};
+
+FIXTURE_VARIANT_ADD(hmm, hmm_device_private)
+{
+ .device_number = HMM_PRIVATE_DEVICE_ONE,
+};
+
+FIXTURE_VARIANT_ADD(hmm, hmm_device_coherent)
+{
+ .device_number = HMM_COHERENCE_DEVICE_ONE,
+};
+
FIXTURE(hmm2)
{
int fd0;
@@ -68,6 +91,24 @@ FIXTURE(hmm2)
unsigned intpage_shift;
};
+FIXTURE_VARIANT(hmm2)
+{
+ int device_number0;
+ int device_number1;
+};
+
+FIXTURE_VARIANT_ADD(hmm2, hmm2_device_private)
+{
+ .device_number0 = HMM_PRIVATE_DEVICE_ONE,
+ .device_number1 = HMM_PRIVATE_DEVICE_TWO,
+};
+
+FIXTURE_VARIANT_ADD(hmm2, hmm2_device_coherent)
+{
+ .device_number0 = HMM_COHERENCE_DEVICE_ONE,
+ .device_number1 = HMM_COHERENCE_DEVICE_TWO,
+};
+
static int hmm_open(int unit)
{
char pathname[HMM_PATH_MAX];
@@ -81,12 +122,19 @@ static int hmm_open(int unit)
return fd;
}
+static bool hmm_is_coherent_type(int dev_num)
+{
+ return (dev_num >= HMM_COHERENCE_DEVICE_ONE);
+}
+
FIXTURE_SETUP(hmm)
{
self->page_size = sysconf(_SC_PAGE_SIZE);
self->page_shift = ffs(self->page_size) - 1;
- self->fd = hmm_open(0);
+ self->fd = hmm_open(variant->device_number);
+ if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
+ SKIP(exit(0), "DEVICE_COHERENT not available");
ASSERT_GE(self->fd, 0);
}
@@ -95,9 +143,11 @@ FIXTURE_SETUP(hmm2)
self->page_size = sysconf(_SC_PAGE_SIZE);
self->page_shift = ffs(self->page_size) - 1;
- self->fd0 = hmm_open(0);
+ self->fd0 = hmm_open(variant->device_number0);
+ if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
+ SKIP(exit(0), "DEVICE_COHERENT not available");
ASSERT_GE(self->fd0, 0);
- self->fd1 = hmm_open(1);
+ self->fd1 = hmm_open(variant->device_number1);
ASSERT_GE(self->fd1, 0);
}
@@ -144,6 +194,7 @@ static int hmm_dmirror_cmd(int fd,
}
buffer->cpages = cmd.cpages;
buffer->faults = cmd.faults;
+ buffer->zone_device_type = cmd.zone_device_type;
return 0;
}
@@ -211,6 +262,20 @@ static void hmm_nanosleep(unsigned int n)
nanosleep(, NULL);
}
+static int hmm_migrate_sys_to_dev(int fd,
+ struct hmm_buffer *buffer,
+ unsigned long npages)
+{
+ return hmm_dmirror_cmd(fd, HMM_DMIRROR_MIGRATE_TO_DEV, buffer, npages);
+}
+
+static int hmm_migrate_dev_to_sys(int fd,
+ struct hmm_buffer *buffer,
+ unsigned long npages)
+{
+ return hmm_dmirror_cmd(fd, HMM_DMIRROR_MIGRATE_TO_SYS, buffer, npages);
+}
+
/*
* Simple NULL test of device open/close.
*/
@@ -875,7 +940,7 @@ TEST_F(hmm, migrate)
ptr[i] = i;
/* Migrate memory to device. */
- ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_MIGRATE, buffer, npages);
+ ret =
Re: [PATCH] mm: add device coherent vma selection for memory migration
Thanks for fixing. I'm guessing Andrew will want you to resend this as part of
a new v6 series, but please add:
Reviewed-by: Alistair Popple
On Tuesday, 1 February 2022 6:48:13 AM AEDT Alex Sierra wrote:
> This case is used to migrate pages from device memory, back to system
> memory. Device coherent type memory is cache coherent from device and CPU
> point of view.
>
> Signed-off-by: Alex Sierra
> Acked-by: Felix Kuehling
> ---
> v2:
> condition added when migrations from device coherent pages.
> ---
> include/linux/migrate.h | 1 +
> mm/migrate.c| 12 +---
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index db96e10eb8da..66a34eae8cb6 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -130,6 +130,7 @@ static inline unsigned long migrate_pfn(unsigned long pfn)
> enum migrate_vma_direction {
> MIGRATE_VMA_SELECT_SYSTEM = 1 << 0,
> MIGRATE_VMA_SELECT_DEVICE_PRIVATE = 1 << 1,
> + MIGRATE_VMA_SELECT_DEVICE_COHERENT = 1 << 2,
> };
>
> struct migrate_vma {
> diff --git a/mm/migrate.c b/mm/migrate.c
> index cd137aedcfe5..69c6830c47c6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2264,15 +2264,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> if (is_writable_device_private_entry(entry))
> mpfn |= MIGRATE_PFN_WRITE;
> } else {
> - if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> - goto next;
> pfn = pte_pfn(pte);
> - if (is_zero_pfn(pfn)) {
> + if (is_zero_pfn(pfn) &&
> + (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
> mpfn = MIGRATE_PFN_MIGRATE;
> migrate->cpages++;
> goto next;
> }
> page = vm_normal_page(migrate->vma, addr, pte);
> + if (page && !is_zone_device_page(page) &&
> + !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> + goto next;
> + else if (page && is_device_coherent_page(page) &&
> + (!(migrate->flags &
> MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> + page->pgmap->owner != migrate->pgmap_owner))
> + goto next;
> mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
> mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
> }
>
Re: [PATCH] drm/amdgpu/display: remove statement with no effect
On 2022-01-31 16:53, Alex Deucher wrote:
> drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_link_encoder.c: In
> function ‘dcn10_link_encoder_dp_set_lane_settings’:
> drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_link_encoder.c:1123:49:
> warning: statement with no effect [-Wunused-value]
> 1123 |
> LINK_RATE_REF_FREQ_IN_KHZ;
> |
> ^
>
> Fixes: f2581998d9eb5e ("drm/amd/display: add set dp lane settings to
> link_hwss")
> Signed-off-by: Alex Deucher
Reviewed-by: Harry Wentland
Harry
> ---
> drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
> b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
> index 779110a2d948..ca39361f71c8 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
> @@ -1120,7 +1120,6 @@ void dcn10_link_encoder_dp_set_lane_settings(
> cntl.lanes_number = link_settings->lane_count;
> cntl.hpd_sel = enc10->base.hpd_source;
> cntl.pixel_clock = link_settings->link_rate * LINK_RATE_REF_FREQ_IN_KHZ;
> - LINK_RATE_REF_FREQ_IN_KHZ;
>
> for (lane = 0; lane < link_settings->lane_count; lane++) {
> /* translate lane settings */
[PATCH] drm/amdgpu/display: remove statement with no effect
drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_link_encoder.c: In
function ‘dcn10_link_encoder_dp_set_lane_settings’:
drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_link_encoder.c:1123:49:
warning: statement with no effect [-Wunused-value]
1123 |
LINK_RATE_REF_FREQ_IN_KHZ;
|
^
Fixes: f2581998d9eb5e ("drm/amd/display: add set dp lane settings to link_hwss")
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
index 779110a2d948..ca39361f71c8 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_link_encoder.c
@@ -1120,7 +1120,6 @@ void dcn10_link_encoder_dp_set_lane_settings(
cntl.lanes_number = link_settings->lane_count;
cntl.hpd_sel = enc10->base.hpd_source;
cntl.pixel_clock = link_settings->link_rate * LINK_RATE_REF_FREQ_IN_KHZ;
- LINK_RATE_REF_FREQ_IN_KHZ;
for (lane = 0; lane < link_settings->lane_count; lane++) {
/* translate lane settings */
--
2.34.1
Re: [PATCH] drm/amdgpu: Handle the GPU recovery failure in SRIOV environment.
On Mon, Jan 31, 2022 at 10:35 AM Surbhi Kakarya wrote:
>
> This patch handles the GPU recovery faliure in sriov environment by
> retrying the reset if the first reset fails. To determine the condition of
> retry, a
> new function amdgpu_is_retry_sriov_reset() is added which returns true if
> failure is due
> to ETIMEDOUT, EINVAL or EBUSY, otherwise return false.
>
> It also handles the return status in Post Asic Reset by updating the return
> code
> with asic_reset_res and eventually return the return code in
> amdgpu_job_timedout().
>
> Change-Id: I45b9743adb548606aef8774496527d29fb3de0af
Missing your s-o-b. Also, does this help on bare metal as well? If
so, we should make this generic and also set a retry limit.
Alex
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 --
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c| 6 +++-
> 2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 53af2623c58f..8a742b77eef8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5026,6 +5026,21 @@ static int amdgpu_device_suspend_display_audio(struct
> amdgpu_device *adev)
> return 0;
> }
>
> +/**
> + * amdgpu_is_retry_sriov_reset - check if we should retry sriov reset
> + *
> + * Check amdgpu_is_retry_sriov_reset and return status to see if we should
> retry reset.
> + */
> +static bool amdgpu_is_retry_sriov_reset(int r)
> +{
> +
> +if(r == -EBUSY || r == -ETIMEDOUT || r == -EINVAL)
> +return true;
> +else
> +return false;
> +
> +}
> +
> static void amdgpu_device_recheck_guilty_jobs(
> struct amdgpu_device *adev, struct list_head *device_list_handle,
> struct amdgpu_reset_context *reset_context)
> @@ -5064,8 +5079,13 @@ static void amdgpu_device_recheck_guilty_jobs(
> if (amdgpu_sriov_vf(adev)) {
> amdgpu_virt_fini_data_exchange(adev);
> r = amdgpu_device_reset_sriov(adev, false);
> - if (r)
> + if (r) {
> adev->asic_reset_res = r;
> + if (amdgpu_is_retry_sriov_reset(r)) {
> + adev->asic_reset_res = 0;
> + goto retry;
> + }
> + }
> } else {
> clear_bit(AMDGPU_SKIP_HW_RESET,
> _context->flags);
> @@ -5299,8 +5319,13 @@ int amdgpu_device_gpu_recover(struct amdgpu_device
> *adev,
> /* Host driver will handle XGMI hive reset for SRIOV */
> if (amdgpu_sriov_vf(adev)) {
> r = amdgpu_device_reset_sriov(adev, job ? false : true);
> - if (r)
> - adev->asic_reset_res = r;
> +if (r) {
> +adev->asic_reset_res = r;
> +if (amdgpu_is_retry_sriov_reset(r)) {
> + adev->asic_reset_res = 0;
> + goto retry;
> +}
> +}
> } else {
> r = amdgpu_do_asic_reset(device_list_handle, _context);
> if (r && r == -EAGAIN)
> @@ -5341,6 +5366,9 @@ int amdgpu_device_gpu_recover(struct amdgpu_device
> *adev,
> drm_helper_resume_force_mode(adev_to_drm(tmp_adev));
> }
>
> + if (tmp_adev->asic_reset_res)
> + r = tmp_adev->asic_reset_res;
> +
> tmp_adev->asic_reset_res = 0;
>
> if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index e0730ea56a8c..1f0fb21ac15a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -37,6 +37,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct
> drm_sched_job *s_job)
> struct amdgpu_task_info ti;
> struct amdgpu_device *adev = ring->adev;
> int idx;
> + int r = 0;
>
> if (!drm_dev_enter(adev_to_drm(adev), )) {
> DRM_INFO("%s - device unplugged skipping recovery on
> scheduler:%s",
> @@ -63,7 +64,10 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct
> drm_sched_job *s_job)
> ti.process_name, ti.tgid, ti.task_name, ti.pid);
>
> if (amdgpu_device_should_recover_gpu(ring->adev)) {
> - amdgpu_device_gpu_recover(ring->adev, job);
> + r = amdgpu_device_gpu_recover(ring->adev, job);
> + if (r)
> + DRM_ERROR("GPU Recovery Failed:
Re: [PATCH v2] drm/amd/display: Force link_rate as LINK_RATE_RBR2 for 2018 15" Apple Retina panels
Applied. Thanks!
Alex
On Sat, Jan 29, 2022 at 12:50 AM Aditya Garg wrote:
>
> From: Aun-Ali Zaidi
>
> The eDP link rate reported by the DP_MAX_LINK_RATE dpcd register (0xa) is
> contradictory to the highest rate supported reported by
> EDID (0xc = LINK_RATE_RBR2). The effects of this compounded with commit
> '4a8ca46bae8a ("drm/amd/display: Default max bpc to 16 for eDP")' results
> in no display modes being found and a dark panel.
>
> For now, simply force the maximum supported link rate for the eDP attached
> 2018 15" Apple Retina panels.
>
> Additionally, we must also check the firmware revision since the device ID
> reported by the DPCD is identical to that of the more capable 16,1,
> incorrectly quirking it. We also use said firmware check to quirk the
> refreshed 15,1 models with Vega graphics as they use a slightly newer
> firmware version.
>
> Tested-by: Aun-Ali Zaidi
> Reviewed-by: Harry Wentland
> Signed-off-by: Aun-Ali Zaidi
> Signed-off-by: Aditya Garg
> ---
> v2 :- Use C styled comments
> .../gpu/drm/amd/display/dc/core/dc_link_dp.c | 20 +++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> index 05e216524..086f7ee2c 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> @@ -5597,6 +5597,26 @@ static bool retrieve_link_cap(struct dc_link *link)
> dp_hw_fw_revision.ieee_fw_rev,
> sizeof(dp_hw_fw_revision.ieee_fw_rev));
>
> + /* Quirk for Apple MBP 2018 15" Retina panels: wrong DP_MAX_LINK_RATE
> */
> + {
> + uint8_t str_mbp_2018[] = { 101, 68, 21, 103, 98, 97 };
> + uint8_t fwrev_mbp_2018[] = { 7, 4 };
> + uint8_t fwrev_mbp_2018_vega[] = { 8, 4 };
> +
> + /* We also check for the firmware revision as 16,1 models
> have an
> +* identical device id and are incorrectly quirked otherwise.
> +*/
> + if ((link->dpcd_caps.sink_dev_id == 0x0010fa) &&
> + !memcmp(link->dpcd_caps.sink_dev_id_str, str_mbp_2018,
> +sizeof(str_mbp_2018)) &&
> + (!memcmp(link->dpcd_caps.sink_fw_revision, fwrev_mbp_2018,
> +sizeof(fwrev_mbp_2018)) ||
> + !memcmp(link->dpcd_caps.sink_fw_revision,
> fwrev_mbp_2018_vega,
> +sizeof(fwrev_mbp_2018_vega {
> + link->reported_link_cap.link_rate = LINK_RATE_RBR2;
> + }
> + }
> +
> memset(>dpcd_caps.dsc_caps, '\0',
> sizeof(link->dpcd_caps.dsc_caps));
> memset(>dpcd_caps.fec_cap, '\0',
> sizeof(link->dpcd_caps.fec_cap));
> --
> 2.25.1
>
>
Re: [PATCH -next] drm/amd/display: clean up some inconsistent indenting
Applied. Thanks!
Alex
On Mon, Jan 31, 2022 at 10:17 AM Harry Wentland wrote:
>
> On 2022-01-28 20:04, Yang Li wrote:
> > Eliminate the follow smatch warning:
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c:2246
> > dp_perform_8b_10b_link_training() warn: inconsistent indenting
> >
> > Reported-by: Abaci Robot
> > Signed-off-by: Yang Li
>
> Reviewed-by: Harry Wentland
>
> Harry
>
> > ---
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> > b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> > index daaec3164875..34ffcd5bb1d7 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> > @@ -2243,11 +2243,11 @@ static enum link_training_result
> > dp_perform_8b_10b_link_training(
> >
> > if (status == LINK_TRAINING_SUCCESS) {
> > status = perform_clock_recovery_sequence(link, link_res,
> > lt_settings, DPRX);
> > - if (status == LINK_TRAINING_SUCCESS) {
> > - status = perform_channel_equalization_sequence(link,
> > - link_res,
> > - lt_settings,
> > - DPRX);
> > + if (status == LINK_TRAINING_SUCCESS) {
> > + status = perform_channel_equalization_sequence(link,
> > +
> > link_res,
> > +
> > lt_settings,
> > +DPRX);
> > }
> > }
> >
>
[PATCH] mm: add device coherent vma selection for memory migration
This case is used to migrate pages from device memory, back to system
memory. Device coherent type memory is cache coherent from device and CPU
point of view.
Signed-off-by: Alex Sierra
Acked-by: Felix Kuehling
---
v2:
condition added when migrations from device coherent pages.
---
include/linux/migrate.h | 1 +
mm/migrate.c| 12 +---
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index db96e10eb8da..66a34eae8cb6 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -130,6 +130,7 @@ static inline unsigned long migrate_pfn(unsigned long pfn)
enum migrate_vma_direction {
MIGRATE_VMA_SELECT_SYSTEM = 1 << 0,
MIGRATE_VMA_SELECT_DEVICE_PRIVATE = 1 << 1,
+ MIGRATE_VMA_SELECT_DEVICE_COHERENT = 1 << 2,
};
struct migrate_vma {
diff --git a/mm/migrate.c b/mm/migrate.c
index cd137aedcfe5..69c6830c47c6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2264,15 +2264,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
if (is_writable_device_private_entry(entry))
mpfn |= MIGRATE_PFN_WRITE;
} else {
- if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
- goto next;
pfn = pte_pfn(pte);
- if (is_zero_pfn(pfn)) {
+ if (is_zero_pfn(pfn) &&
+ (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
mpfn = MIGRATE_PFN_MIGRATE;
migrate->cpages++;
goto next;
}
page = vm_normal_page(migrate->vma, addr, pte);
+ if (page && !is_zone_device_page(page) &&
+ !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
+ goto next;
+ else if (page && is_device_coherent_page(page) &&
+ (!(migrate->flags &
MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
+page->pgmap->owner != migrate->pgmap_owner))
+ goto next;
mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
}
--
2.32.0
Re: [PATCH 1/2] drm/amdgpu: Fixed the defect of soft lock caused by infinite loop
Am 2022-01-29 um 22:19 schrieb Zhou1, Tao:
[AMD Official Use Only]
-Original Message-
From: Chai, Thomas
Sent: Saturday, January 29, 2022 8:34 PM
To: amd-gfx@lists.freedesktop.org
Cc: Chai, Thomas ; Zhang, Hawking
; Zhou1, Tao ; Clements,
John ; Chai, Thomas
Subject: [PATCH 1/2] drm/amdgpu: Fixed the defect of soft lock caused by
infinite loop
1. The infinite loop case only occurs on multiple cards support
ras functions.
2. The explanation of root cause refer to 76641cbbf196523b5752c6cf68f86.
3. Create new node to manage each unique ras instance to guarantee
each device .ras_list is completely independent.
4. Fixes:7a6b8ab3231b511915cb94cac1debabf093.
5. The soft locked logs are as follows:
[ 262.165690] CPU: 93 PID: 758 Comm: kworker/93:1 Tainted: G OE
5.13.0-27-generic #29~20.04.1-Ubuntu
[ 262.165695] Hardware name: Supermicro AS -4124GS-TNR/H12DSG-O-CPU,
BIOS T20200717143848 07/17/2020 [ 262.165698] Workqueue: events
amdgpu_ras_do_recovery [amdgpu] [ 262.165980] RIP:
0010:amdgpu_ras_get_ras_block+0x86/0xd0 [amdgpu] [ 262.166239] Code: 68
d8 4c 8d 71 d8 48 39 c3 74 54 49 8b 45 38 48 85 c0 74 32 44 89 fa 44 89 e6 4c 89
ef e8 82 e4 9b dc 85 c0 74 3c 49 8b 46 28 <49> 8d 56 28 4d 89 f5 48 83 e8 28 48
39 d3 74 25 49 89 c6 49 8b 45 [ 262.166243] RSP: 0018:ac908fa87d80
EFLAGS: 0202 [ 262.166247] RAX: c1394248 RBX: 91e4ab8d6e20
RCX: c1394248 [ 262.166249] RDX: 91e4aa356e20 RSI:
000e RDI: 91e4ab8c [ 262.166252] RBP:
ac908fa87da8 R08: 0007 R09: 0001
[ 262.166254] R10: 91e4930b64ec R11: R12:
000e [ 262.166256] R13: 91e4aa356df8 R14: c1394320
R15: 0003 [ 262.166258] FS: ()
GS:92238fb4() knlGS: [ 262.166261] CS: 0010
DS: ES: CR0: 80050033 [ 262.166264] CR2:
0001004865d0 CR3: 00406d796000 CR4: 00350ee0
[ 262.166267] Call Trace:
[ 262.166272] amdgpu_ras_do_recovery+0x130/0x290 [amdgpu]
[ 262.166529] ? psi_task_switch+0xd2/0x250 [ 262.166537] ?
__switch_to+0x11d/0x460 [ 262.166542] ? __switch_to_asm+0x36/0x70
[ 262.166549] process_one_work+0x220/0x3c0 [ 262.166556]
worker_thread+0x4d/0x3f0 [ 262.166560] ? process_one_work+0x3c0/0x3c0
[ 262.166563] kthread+0x12b/0x150 [ 262.166568] ?
set_kthread_struct+0x40/0x40 [ 262.166571] ret_from_fork+0x22/0x30
Signed-off-by: yipechai
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 37 ++--
- drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h | 3 --
2 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 9d7c778c1a2d..b0aa67308c31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -75,6 +75,13 @@ const char *ras_mca_block_string[] = {
"mca_iohc",
};
+struct amdgpu_ras_block_list {
+ /* ras block link */
+ struct list_head node;
+
+ struct amdgpu_ras_block_object *ras_obj; };
+
const char *get_ras_block_str(struct ras_common_if *ras_block) {
if (!ras_block)
@@ -880,7 +887,8 @@ static struct amdgpu_ras_block_object
*amdgpu_ras_get_ras_block(struct amdgpu_de
enum amdgpu_ras_block block,
uint32_t sub_block_index) {
int loop_cnt = 0;
- struct amdgpu_ras_block_object *obj, *tmp;
+ struct amdgpu_ras_block_list *node, *tmp;
+ struct amdgpu_ras_block_object *obj;
if (block >= AMDGPU_RAS_BLOCK__LAST)
return NULL;
@@ -888,7 +896,13 @@ static struct amdgpu_ras_block_object
*amdgpu_ras_get_ras_block(struct amdgpu_de
if (!amdgpu_ras_is_supported(adev, block))
return NULL;
- list_for_each_entry_safe(obj, tmp, >ras_list, node) {
+ list_for_each_entry_safe(node, tmp, >ras_list, node) {
+ if (!node->ras_obj) {
+ DRM_ERROR("Warning: abnormal ras list node");
[Tao]: dev_warn is recommended.
+ continue;
+ }
+
+ obj = node->ras_obj;
if (obj->ras_block_match) {
if (obj->ras_block_match(obj, block, sub_block_index)
== 0)
return obj;
@@ -2527,6 +2541,7 @@ int amdgpu_ras_pre_fini(struct amdgpu_device *adev)
int amdgpu_ras_fini(struct amdgpu_device *adev) {
+ struct amdgpu_ras_block_list *ras_node, *tmp;
struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
if (!adev->ras_enabled || !con)
@@ -2545,6 +2560,12 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
amdgpu_ras_set_context(adev, NULL);
kfree(con);
+ /* Clear ras blocks from ras_list and free ras block list node */
+ list_for_each_entry_safe(ras_node, tmp, >ras_list, node) {
+ list_del(_node->node);
+
[bug report] drm/amd/display: access hpo dp link encoder only through link resource
Hello Wenjing Liu,
The patch 3d38a5839ea8: "drm/amd/display: access hpo dp link encoder
only through link resource" from Nov 26, 2021, leads to the following
Smatch static checker warning:
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:2961
dc_link_dp_sync_lt_end()
error: NULL dereference inside function dp_disable_link_phy()
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c
2954 bool dc_link_dp_sync_lt_end(struct dc_link *link, bool link_down)
2955 {
2956 /* If input parameter is set, shut down phy.
2957 * Still shouldn't turn off dp_receiver (DPCD:600h)
2958 */
2959 if (link_down == true) {
2960 struct dc_link_settings link_settings =
link->cur_link_settings;
--> 2961 dp_disable_link_phy(link, NULL,
link->connector_signal);
This NULL will lead to an Oops.
2962 if (dp_get_link_encoding_format(_settings) ==
DP_8b_10b_ENCODING)
2963 dp_set_fec_ready(link, NULL, false);
2964 }
2965
2966 link->sync_lt_in_progress = false;
2967 return true;
2968 }
regards,
dan carpenter
Re: [PATCH -next] drm/amd/display: clean up some inconsistent indenting
On 2022-01-28 20:04, Yang Li wrote:
> Eliminate the follow smatch warning:
> drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c:2246
> dp_perform_8b_10b_link_training() warn: inconsistent indenting
>
> Reported-by: Abaci Robot
> Signed-off-by: Yang Li
Reviewed-by: Harry Wentland
Harry
> ---
> drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> index daaec3164875..34ffcd5bb1d7 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> @@ -2243,11 +2243,11 @@ static enum link_training_result
> dp_perform_8b_10b_link_training(
>
> if (status == LINK_TRAINING_SUCCESS) {
> status = perform_clock_recovery_sequence(link, link_res,
> lt_settings, DPRX);
> - if (status == LINK_TRAINING_SUCCESS) {
> - status = perform_channel_equalization_sequence(link,
> - link_res,
> - lt_settings,
> - DPRX);
> + if (status == LINK_TRAINING_SUCCESS) {
> + status = perform_channel_equalization_sequence(link,
> +link_res,
> +
> lt_settings,
> +DPRX);
> }
> }
>
Re: [PATCH] drm/amdkfd: use unmap all queues for poison consumption
Am 2022-01-30 um 02:38 schrieb Tao Zhou:
Replace reset queue for specific PASID with unmap all queues, reset
queue could break CP scheduler.
Signed-off-by: Tao Zhou
The change looks reasonable, based on what kfd_process_vm_fault does.
But the function name is now a bit misleading. Maybe rename it to
something more general, e.g. kfd_process_mem_fault or kfd_dqm_evict_pasid.
Regards,
Felix
---
drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
index e8bc28009c22..dca0b5fac1db 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
@@ -109,8 +109,7 @@ static void event_interrupt_poison_consumption(struct
kfd_dev *dev,
switch (source_id) {
case SOC15_INTSRC_SQ_INTERRUPT_MSG:
- if (dev->dqm->ops.reset_queues)
- ret = dev->dqm->ops.reset_queues(dev->dqm, pasid);
+ ret = kfd_process_vm_fault(dev->dqm, pasid);
break;
case SOC15_INTSRC_SDMA_ECC:
default:
[PATCH 4/4] drm/amdgpu: restructure amdgpu_fill_buffer v2
We ran into the problem that clearing really larger buffer (60GiB) caused an
SDMA timeout.
Restructure the function to use the dst window instead of mapping the whole
buffer into the GART and then fill only 2MiB/256MiB chunks at a time.
v2: rebase on restructured window map.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 187 +---
1 file changed, 105 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 26c521cd1092..b9637d1cf147 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -396,8 +396,7 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE)) {
struct dma_fence *wipe_fence = NULL;
- r = amdgpu_fill_buffer(ttm_to_amdgpu_bo(bo), AMDGPU_POISON,
- NULL, _fence);
+ r = amdgpu_fill_buffer(abo, AMDGPU_POISON, NULL, _fence);
if (r) {
goto error;
} else if (wipe_fence) {
@@ -1923,19 +1922,51 @@ void amdgpu_ttm_set_buffer_funcs_status(struct
amdgpu_device *adev, bool enable)
adev->mman.buffer_funcs_enabled = enable;
}
+static int amdgpu_ttm_prepare_job(struct amdgpu_device *adev,
+ bool direct_submit,
+ unsigned int num_dw,
+ struct dma_resv *resv,
+ bool vm_needs_flush,
+ struct amdgpu_job **job)
+{
+ enum amdgpu_ib_pool_type pool = direct_submit ?
+ AMDGPU_IB_POOL_DIRECT :
+ AMDGPU_IB_POOL_DELAYED;
+ int r;
+
+ r = amdgpu_job_alloc_with_ib(adev, num_dw * 4, pool, job);
+ if (r)
+ return r;
+
+ if (vm_needs_flush) {
+ (*job)->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
+ adev->gmc.pdb0_bo :
+ adev->gart.bo);
+ (*job)->vm_needs_flush = true;
+ }
+ if (resv) {
+ r = amdgpu_sync_resv(adev, &(*job)->sync, resv,
+AMDGPU_SYNC_ALWAYS,
+AMDGPU_FENCE_OWNER_UNDEFINED);
+ if (r) {
+ DRM_ERROR("sync failed (%d).\n", r);
+ amdgpu_job_free(*job);
+ return r;
+ }
+ }
+ return 0;
+}
+
int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
uint64_t dst_offset, uint32_t byte_count,
struct dma_resv *resv,
struct dma_fence **fence, bool direct_submit,
bool vm_needs_flush, bool tmz)
{
- enum amdgpu_ib_pool_type pool = direct_submit ? AMDGPU_IB_POOL_DIRECT :
- AMDGPU_IB_POOL_DELAYED;
struct amdgpu_device *adev = ring->adev;
+ unsigned num_loops, num_dw;
struct amdgpu_job *job;
-
uint32_t max_bytes;
- unsigned num_loops, num_dw;
unsigned i;
int r;
@@ -1947,26 +1978,11 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring,
uint64_t src_offset,
max_bytes = adev->mman.buffer_funcs->copy_max_bytes;
num_loops = DIV_ROUND_UP(byte_count, max_bytes);
num_dw = ALIGN(num_loops * adev->mman.buffer_funcs->copy_num_dw, 8);
-
- r = amdgpu_job_alloc_with_ib(adev, num_dw * 4, pool, );
+ r = amdgpu_ttm_prepare_job(adev, direct_submit, num_dw,
+ resv, vm_needs_flush, );
if (r)
return r;
- if (vm_needs_flush) {
- job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
- adev->gmc.pdb0_bo : adev->gart.bo);
- job->vm_needs_flush = true;
- }
- if (resv) {
- r = amdgpu_sync_resv(adev, >sync, resv,
-AMDGPU_SYNC_ALWAYS,
-AMDGPU_FENCE_OWNER_UNDEFINED);
- if (r) {
- DRM_ERROR("sync failed (%d).\n", r);
- goto error_free;
- }
- }
-
for (i = 0; i < num_loops; i++) {
uint32_t cur_size_in_bytes = min(byte_count, max_bytes);
@@ -1996,77 +2012,35 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring,
uint64_t src_offset,
return r;
}
-int amdgpu_fill_buffer(struct amdgpu_bo *bo,
- uint32_t src_data,
- struct dma_resv *resv,
- struct dma_fence **fence)
+static int amdgpu_ttm_fill_mem(struct amdgpu_ring *ring, uint32_t src_data,
+ uint64_t dst_addr,
[PATCH 2/4] drm/amdgpu: lower BUG_ON into WARN_ON for AMDGPU_PL_PREEMPT
That should never happen, but make sure that we only warn instead of
crash.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2b0e83e9fa8a..bc10b3f9015a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -203,7 +203,9 @@ static int amdgpu_ttm_map_buffer(struct ttm_buffer_object
*bo,
BUG_ON(adev->mman.buffer_funcs->copy_max_bytes <
AMDGPU_GTT_MAX_TRANSFER_SIZE * 8);
- BUG_ON(mem->mem_type == AMDGPU_PL_PREEMPT);
+
+ if (WARN_ON(mem->mem_type == AMDGPU_PL_PREEMPT))
+ return -EINVAL;
/* Map only what can't be accessed directly */
if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
--
2.25.1
[PATCH 1/4] drm/amdgpu: fix logic inversion in check
We probably never trigger this, but the logic inside the check is
inverted.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 3d8a20956b74..2b0e83e9fa8a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1938,7 +1938,7 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t
src_offset,
unsigned i;
int r;
- if (direct_submit && !ring->sched.ready) {
+ if (!direct_submit && !ring->sched.ready) {
DRM_ERROR("Trying to move memory with ring turned off.\n");
return -EINVAL;
}
--
2.25.1
[PATCH 3/4] drm/amdgpu: rework GART copy window handling
Instead of limiting the size before we call the mapping
function let the function itself limit the size.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 49 -
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index bc10b3f9015a..26c521cd1092 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -175,10 +175,10 @@ static void amdgpu_evict_flags(struct ttm_buffer_object
*bo,
* @bo: buffer object to map
* @mem: memory object to map
* @mm_cur: range to map
- * @num_pages: number of pages to map
* @window: which GART window to use
* @ring: DMA ring to use for the copy
* @tmz: if we should setup a TMZ enabled mapping
+ * @size: in number of bytes to map, out number of bytes mapped
* @addr: resulting address inside the MC address space
*
* Setup one of the GART windows to access a specific piece of memory or return
@@ -187,15 +187,14 @@ static void amdgpu_evict_flags(struct ttm_buffer_object
*bo,
static int amdgpu_ttm_map_buffer(struct ttm_buffer_object *bo,
struct ttm_resource *mem,
struct amdgpu_res_cursor *mm_cur,
-unsigned num_pages, unsigned window,
-struct amdgpu_ring *ring, bool tmz,
-uint64_t *addr)
+unsigned window, struct amdgpu_ring *ring,
+bool tmz, uint64_t *size, uint64_t *addr)
{
struct amdgpu_device *adev = ring->adev;
- struct amdgpu_job *job;
- unsigned num_dw, num_bytes;
- struct dma_fence *fence;
+ unsigned offset, num_pages, num_dw, num_bytes;
uint64_t src_addr, dst_addr;
+ struct dma_fence *fence;
+ struct amdgpu_job *job;
void *cpu_addr;
uint64_t flags;
unsigned int i;
@@ -214,10 +213,22 @@ static int amdgpu_ttm_map_buffer(struct ttm_buffer_object
*bo,
return 0;
}
+
+ /*
+* If start begins at an offset inside the page, then adjust the size
+* and addr accordingly
+*/
+ offset = mm_cur->start & ~PAGE_MASK;
+
+ num_pages = PFN_UP(*size + offset);
+ num_pages = min_t(uint32_t, num_pages, AMDGPU_GTT_MAX_TRANSFER_SIZE);
+
+ *size = min(*size, (uint64_t)num_pages * PAGE_SIZE - offset);
+
*addr = adev->gmc.gart_start;
*addr += (u64)window * AMDGPU_GTT_MAX_TRANSFER_SIZE *
AMDGPU_GPU_PAGE_SIZE;
- *addr += mm_cur->start & ~PAGE_MASK;
+ *addr += offset;
num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw, 8);
num_bytes = num_pages * 8 * AMDGPU_GPU_PAGES_IN_CPU_PAGE;
@@ -298,9 +309,6 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
struct dma_resv *resv,
struct dma_fence **f)
{
- const uint32_t GTT_MAX_BYTES = (AMDGPU_GTT_MAX_TRANSFER_SIZE *
- AMDGPU_GPU_PAGE_SIZE);
-
struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
struct amdgpu_res_cursor src_mm, dst_mm;
struct dma_fence *fence = NULL;
@@ -316,29 +324,20 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
mutex_lock(>mman.gtt_window_lock);
while (src_mm.remaining) {
- uint32_t src_page_offset = src_mm.start & ~PAGE_MASK;
- uint32_t dst_page_offset = dst_mm.start & ~PAGE_MASK;
+ uint64_t from, to, cur_size;
struct dma_fence *next;
- uint32_t cur_size;
- uint64_t from, to;
- /* Copy size cannot exceed GTT_MAX_BYTES. So if src or dst
-* begins at an offset, then adjust the size accordingly
-*/
- cur_size = max(src_page_offset, dst_page_offset);
- cur_size = min(min3(src_mm.size, dst_mm.size, size),
- (uint64_t)(GTT_MAX_BYTES - cur_size));
+ /* Never copy more than 256MiB at once to avoid a timeout */
+ cur_size = min3(src_mm.size, dst_mm.size, 256ULL << 20);
/* Map src to window 0 and dst to window 1. */
r = amdgpu_ttm_map_buffer(src->bo, src->mem, _mm,
- PFN_UP(cur_size + src_page_offset),
- 0, ring, tmz, );
+ 0, ring, tmz, _size, );
if (r)
goto error;
r = amdgpu_ttm_map_buffer(dst->bo, dst->mem, _mm,
- PFN_UP(cur_size + dst_page_offset),
- 1, ring, tmz, );
+ 1, ring, tmz, _size,
Re: [PATCH 2/2] drm/amdgpu: restructure amdgpu_fill_buffer
Am 28.01.22 um 16:55 schrieb Felix Kuehling:
Am 2022-01-28 um 10:16 schrieb Christian König:
[SNIP]
- if (bo->tbo.resource->mem_type == AMDGPU_PL_PREEMPT) {
As far as I can see, you didn't add this check back elsewhere. The
promise for preemptible BOs is, that we never have to wait for the GPU
to finish accessing the BOs because the context using it is
preemptible. This was a necessary condition to disable GTT usage
accounting for such BOs. If you allow filling such BOs with this
function, you're breaking that promise.
That's now part of amdgpu_ttm_map_buffer(), but unfortunately as
BUG_ON(). I've added a patch to change that into a warning.
[SNIP]
+ cur_size = min_t(u64, dst.size, AMDGPU_GTT_MAX_TRANSFER_BYTES);
For VRAM, the cur_size could be much bigger because we're not limited
by the GART transfer window. I'm pretty sure that 2MB is going to add
too much overhead. For the extreme 60GB BO example, it would require
30-thousand IBs and IB frames to fill the entire buffer. That's a lot
of VMID-switching, IB execution, fence signalling, interrupts, etc.
I've restructured the mapping function so that we can now copy/fill in
256MiB chunks when no GART window is involved.
+
+ r = amdgpu_ttm_map_buffer(>tbo, bo->tbo.resource, ,
+ PFN_UP(cur_size), 1, ring, false,
+ );
+ if (r)
+ goto error;
+
+ r = amdgpu_ttm_fill_mem(ring, src_data, to, cur_size, resv,
+ , false);
If amdgpu_ttm_map_buffer updated the page tables, shouldn't the
vm_needs_flush parameter be true? This flag should probably be
returned by amdgpu_ttm_map_buffer.
Ah, yes. That's indeed a typo. For now I've didn't added the
vm_needs_flush parameter, but that's something we could optimize.
Regards,
Christian.
Regards,
Felix
+ if (r)
+ goto error;
+
+ dma_fence_put(fence);
+ fence = next;
+
+ amdgpu_res_next(, cur_size);
+ }
+error:
+ mutex_unlock(>mman.gtt_window_lock);
+ if (f)
+ *f = dma_fence_get(fence);
+ dma_fence_put(fence);
+ return r;
+}
+
/**
* amdgpu_ttm_evict_resources - evict memory buffers
* @adev: amdgpu device object
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index d9691f262f16..bcd34592e45d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -35,6 +35,8 @@
#define AMDGPU_GTT_MAX_TRANSFER_SIZE 512
#define AMDGPU_GTT_NUM_TRANSFER_WINDOWS 2
+#define AMDGPU_GTT_MAX_TRANSFER_BYTES (AMDGPU_GTT_MAX_TRANSFER_SIZE * \
+ AMDGPU_GPU_PAGE_SIZE)
#define AMDGPU_POISON 0xd0bed0be
Re: [PATCH v5 01/10] mm: add zone device coherent type memory support
Looks good, feel free to add:
Reviewed-by: Alistair Popple
On Saturday, 29 January 2022 7:08:16 AM AEDT Alex Sierra wrote:
> Device memory that is cache coherent from device and CPU point of view.
> This is used on platforms that have an advanced system bus (like CAPI
> or CXL). Any page of a process can be migrated to such memory. However,
> no one should be allowed to pin such memory so that it can always be
> evicted.
>
> Signed-off-by: Alex Sierra
> Acked-by: Felix Kuehling
> ---
> v4:
> - use the same system entry path for coherent device pages at
> migrate_vma_insert_page.
>
> - Add coherent device type support for try_to_migrate /
> try_to_migrate_one.
> ---
> include/linux/memremap.h | 8 +++
> include/linux/mm.h | 16 ++
> mm/memcontrol.c | 6 +++---
> mm/memory-failure.c | 8 +--
> mm/memremap.c| 14 -
> mm/migrate.c | 45
> mm/rmap.c| 5 +++--
> 7 files changed, 71 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 1fafcc38acba..727b8c789193 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -39,6 +39,13 @@ struct vmem_altmap {
> * A more complete discussion of unaddressable memory may be found in
> * include/linux/hmm.h and Documentation/vm/hmm.rst.
> *
> + * MEMORY_DEVICE_COHERENT:
> + * Device memory that is cache coherent from device and CPU point of view.
> This
> + * is used on platforms that have an advanced system bus (like CAPI or CXL).
> A
> + * driver can hotplug the device memory using ZONE_DEVICE and with that
> memory
> + * type. Any page of a process can be migrated to such memory. However no one
> + * should be allowed to pin such memory so that it can always be evicted.
> + *
> * MEMORY_DEVICE_FS_DAX:
> * Host memory that has similar access semantics as System RAM i.e. DMA
> * coherent and supports page pinning. In support of coordinating page
> @@ -59,6 +66,7 @@ struct vmem_altmap {
> enum memory_type {
> /* 0 is reserved to catch uninitialized type fields */
> MEMORY_DEVICE_PRIVATE = 1,
> + MEMORY_DEVICE_COHERENT,
> MEMORY_DEVICE_FS_DAX,
> MEMORY_DEVICE_GENERIC,
> MEMORY_DEVICE_PCI_P2PDMA,
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e1a84b1e6787..0c61bf40edef 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1106,6 +1106,7 @@ static inline bool page_is_devmap_managed(struct page
> *page)
> return false;
> switch (page->pgmap->type) {
> case MEMORY_DEVICE_PRIVATE:
> + case MEMORY_DEVICE_COHERENT:
> case MEMORY_DEVICE_FS_DAX:
> return true;
> default:
> @@ -1135,6 +1136,21 @@ static inline bool is_device_private_page(const struct
> page *page)
> page->pgmap->type == MEMORY_DEVICE_PRIVATE;
> }
>
> +static inline bool is_device_coherent_page(const struct page *page)
> +{
> + return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
> + is_zone_device_page(page) &&
> + page->pgmap->type == MEMORY_DEVICE_COHERENT;
> +}
> +
> +static inline bool is_dev_private_or_coherent_page(const struct page *page)
> +{
> + return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
> + is_zone_device_page(page) &&
> + (page->pgmap->type == MEMORY_DEVICE_PRIVATE ||
> + page->pgmap->type == MEMORY_DEVICE_COHERENT);
> +}
> +
> static inline bool is_pci_p2pdma_page(const struct page *page)
> {
> return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 09d342c7cbd0..0882b5b2a857 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5691,8 +5691,8 @@ static int mem_cgroup_move_account(struct page *page,
> * 2(MC_TARGET_SWAP): if the swap entry corresponding to this pte is a
> * target for charge migration. if @target is not NULL, the entry is
> stored
> * in target->ent.
> - * 3(MC_TARGET_DEVICE): like MC_TARGET_PAGE but page is
> MEMORY_DEVICE_PRIVATE
> - * (so ZONE_DEVICE page and thus not on the lru).
> + * 3(MC_TARGET_DEVICE): like MC_TARGET_PAGE but page is device memory and
> + * thus not on the lru.
> * For now we such page is charge like a regular page would be as for all
> * intent and purposes it is just special memory taking the place of a
> * regular page.
> @@ -5726,7 +5726,7 @@ static enum mc_target_type get_mctgt_type(struct
> vm_area_struct *vma,
>*/
> if (page_memcg(page) == mc.from) {
> ret = MC_TARGET_PAGE;
> - if (is_device_private_page(page))
> + if (is_dev_private_or_coherent_page(page))
> ret = MC_TARGET_DEVICE;
> if (target)
> target->page = page;
> diff --git
[PATCH] drm/amdgpu: initialize reg_access_ctrl
From: Tom Rix
clang build fails with
amdgpu_virt.c:878:51: error: variable 'reg_access_ctrl' is
uninitialized when used here
... + 4 * reg_access_ctrl->scratch_reg0;
^~~
The reg_access_ctrl ptr is never initialized, so
initialize once we know it is supported.
Fixes: 5d447e296701 ("drm/amdgpu: add helper for rlcg indirect reg access")
Signed-off-by: Tom Rix
---
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 80c25176c9932..c137652189190 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -875,6 +875,7 @@ static u32 amdgpu_virt_rlcg_reg_rw(struct amdgpu_device
*adev, u32 offset, u32 v
return 0;
}
+ reg_access_ctrl = >gfx.rlc.reg_access_ctrl;
scratch_reg0 = (void __iomem *)adev->rmmio + 4 *
reg_access_ctrl->scratch_reg0;
scratch_reg1 = (void __iomem *)adev->rmmio + 4 *
reg_access_ctrl->scratch_reg1;
scratch_reg2 = (void __iomem *)adev->rmmio + 4 *
reg_access_ctrl->scratch_reg2;
--
2.26.3
Re: [PATCH v5 02/10] mm: add device coherent vma selection for memory migration
On Saturday, 29 January 2022 7:08:17 AM AEDT Alex Sierra wrote:
[...]
> struct migrate_vma {
> diff --git a/mm/migrate.c b/mm/migrate.c
> index cd137aedcfe5..d3cc3589e1e8 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2264,7 +2264,8 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> if (is_writable_device_private_entry(entry))
> mpfn |= MIGRATE_PFN_WRITE;
> } else {
> - if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> + if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM) &&
> + !(migrate->flags &
> MIGRATE_VMA_SELECT_DEVICE_COHERENT))
> goto next;
> pfn = pte_pfn(pte);
> if (is_zero_pfn(pfn)) {
Sorry, but I still don't think this is quite right.
When specifying MIGRATE_VMA_SELECT_DEVICE_COHERENT we are looking for pages to
migrate from the device back to system memory. But as currently written I think
this can also select the zero pfn when MIGRATE_VMA_SELECT_DEVICE_COHERENT is
specified. As far as I know that can never point to device memory so migration
of a zero pfn should be also be skipped in that case.
We should only migrate the zero pfn if MIGRATE_VMA_SELECT_SYSTEM is specified.
> @@ -2273,6 +2274,13 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> goto next;
> }
> page = vm_normal_page(migrate->vma, addr, pte);
> + if (page && !is_zone_device_page(page) &&
> + !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> + goto next;
> + if (page && is_device_coherent_page(page) &&
> + (!(migrate->flags &
> MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> + page->pgmap->owner != migrate->pgmap_owner))
> + goto next;
> mpfn = migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
> mpfn |= pte_write(pte) ? MIGRATE_PFN_WRITE : 0;
> }
>
