[PATCH] amdgpu/dc: Avoid dereferencing NULL pointer

[S, Shirish]

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf 
> Of Drew Davenport
> Sent: Saturday, October 28, 2017 12:05 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander ; Drew Davenport 
> 
> Subject: [PATCH] amdgpu/dc: Avoid dereferencing NULL pointer
>
> crtc is dereferenced from within drm_atomic_get_new_crtc_state, so 
> check for NULL before initializing new_crtc_state.
>
> Signed-off-by: Drew Davenport 
Reviewed-by: Shirish S 
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 ++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> 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 d0ee1b3b8b5c..5a440fadbe18 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3874,8 +3874,7 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>   /* update planes when needed */
>   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> new_plane_state, i) {
>   struct drm_crtc *crtc = new_plane_state->crtc;
> - struct drm_crtc_state *new_crtc_state =
> - drm_atomic_get_new_crtc_state(state, crtc);
> + struct drm_crtc_state *new_crtc_state;
>   struct drm_framebuffer *fb = new_plane_state->fb;
>   bool pflip_needed;
>   struct dm_plane_state *dm_new_plane_state = 
> to_dm_plane_state(new_plane_state);
> @@ -3885,7 +3884,11 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>   continue;
>   }
>   
> - if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
> + if (!fb || !crtc || pcrtc != crtc)
> + continue;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + if (!new_crtc_state->active)
>   continue;
>   
>   pflip_needed = !state->allow_modeset;

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

[PATCH] drm/amd/display: fix null pointer dereference

[S, Shirish]

From: Shirish S 

While setting cursor position in case of mpo, input_pixel_processor is not 
available for underlay, hence add check of the same to avoid null pointer 
access issue.

Signed-off-by: Shirish S 
Reviewed-by: Harry Wentland 
Reviewed-by: Tony Cheng 
---
 drivers/gpu/drm/amd/display/dc/core/dc_stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
index 5cf69af..572b885 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
@@ -288,7 +288,7 @@ bool dc_stream_set_cursor_position(
pos_cpy.enable = false;
 
 
-   if (ipp->funcs->ipp_cursor_set_position != NULL)
+   if (ipp !=NULL && ipp->funcs->ipp_cursor_set_position != NULL)
ipp->funcs->ipp_cursor_set_position(ipp, _cpy, 
);
 
if (mi != NULL && mi->funcs->set_cursor_position != NULL)
--
2.7.4

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

[PATCH 11/17] drm/amdkfd: Use IH context ID for signal lookup

[Felix Kuehling]

This speeds up signal lookup when the IH ring entry includes a
valid context ID or partial context ID. Only if the context ID is
found to be invalid, fall back to an exhaustive search of all
signaled events.

Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |  7 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 73 +++-
 2 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
index 66164aa..3d5ccb3 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
@@ -47,6 +47,7 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
unsigned int pasid;
const struct cik_ih_ring_entry *ihre =
(const struct cik_ih_ring_entry *)ih_ring_entry;
+   uint32_t context_id = ihre->data & 0xfff;
 
pasid = (ihre->ring_id & 0x) >> 16;
 
@@ -54,11 +55,11 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
return;
 
if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
-   kfd_signal_event_interrupt(pasid, 0, 0);
+   kfd_signal_event_interrupt(pasid, context_id, 28);
else if (ihre->source_id == CIK_INTSRC_SDMA_TRAP)
-   kfd_signal_event_interrupt(pasid, 0, 0);
+   kfd_signal_event_interrupt(pasid, context_id, 28);
else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
-   kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
+   kfd_signal_event_interrupt(pasid, context_id & 0xff, 8);
else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
kfd_signal_hw_exception_event(pasid);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 41580e0..26e8045 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -53,12 +53,6 @@ struct kfd_signal_page {
uint64_t __user *user_address;
 };
 
-/*
- * For signal events, the event ID is used as the interrupt user data.
- * For SQ s_sendmsg interrupts, this is limited to 8 bits.
- */
-
-#define INTERRUPT_DATA_BITS 8
 
 static uint64_t *page_slots(struct kfd_signal_page *page)
 {
@@ -125,6 +119,54 @@ static struct kfd_event *lookup_event_by_id(struct 
kfd_process *p, uint32_t id)
return idr_find(>event_idr, id);
 }
 
+/**
+ * lookup_signaled_event_by_partial_id - Lookup signaled event from partial ID
+ * @p: Pointer to struct kfd_process
+ * @id:ID to look up
+ * @bits:  Number of valid bits in @id
+ *
+ * Finds the first signaled event with a matching partial ID. If no
+ * matching signaled event is found, returns NULL. In that case the
+ * caller should assume that the partial ID is invalid and do an
+ * exhaustive search of all siglaned events.
+ *
+ * If multiple events with the same partial ID signal at the same
+ * time, they will be found one interrupt at a time, not necessarily
+ * in the same order the interrupts occurred. As long as the number of
+ * interrupts is correct, all signaled events will be seen by the
+ * driver.
+ */
+static struct kfd_event *lookup_signaled_event_by_partial_id(
+   struct kfd_process *p, uint32_t id, uint32_t bits)
+{
+   struct kfd_event *ev;
+
+   if (!p->signal_page || id >= KFD_SIGNAL_EVENT_LIMIT)
+   return NULL;
+
+   /* Fast path for the common case that @id is not a partial ID
+* and we only need a single lookup.
+*/
+   if (bits > 31 || (1U << bits) >= KFD_SIGNAL_EVENT_LIMIT) {
+   if (page_slots(p->signal_page)[id] == UNSIGNALED_EVENT_SLOT)
+   return NULL;
+
+   return idr_find(>event_idr, id);
+   }
+
+   /* General case for partial IDs: Iterate over all matching IDs
+* and find the first one that has signaled.
+*/
+   for (ev = NULL; id < KFD_SIGNAL_EVENT_LIMIT && !ev; id += 1U << bits) {
+   if (page_slots(p->signal_page)[id] == UNSIGNALED_EVENT_SLOT)
+   continue;
+
+   ev = idr_find(>event_idr, id);
+   }
+
+   return ev;
+}
+
 static int create_signal_event(struct file *devkfd,
struct kfd_process *p,
struct kfd_event *ev)
@@ -385,7 +427,7 @@ static void set_event_from_interrupt(struct kfd_process *p,
 void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
uint32_t valid_id_bits)
 {
-   struct kfd_event *ev;
+   struct kfd_event *ev = NULL;
 
/*
 * Because we are called from arbitrary context (workqueue) as opposed
@@ -399,19 +441,24 @@ void kfd_signal_event_interrupt(unsigned int pasid, 
uint32_t partial_id,
 

[PATCH 14/17] drm/amdkfd: use standard kernel kfifo for IH

[Felix Kuehling]

From: Andres Rodriguez 

Replace our implementation of a lockless ring buffer with the standard
linux kernel kfifo.

We shouldn't maintain our own version of a standard data structure.

Signed-off-by: Andres Rodriguez 
Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 78 ++
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h  |  6 +--
 2 files changed, 27 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 70b3a99c..ffbb91a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -42,25 +42,24 @@
 
 #include 
 #include 
+#include 
 #include "kfd_priv.h"
 
-#define KFD_INTERRUPT_RING_SIZE 1024
+#define KFD_IH_NUM_ENTRIES 1024
 
 static void interrupt_wq(struct work_struct *);
 
 int kfd_interrupt_init(struct kfd_dev *kfd)
 {
-   void *interrupt_ring = kmalloc_array(KFD_INTERRUPT_RING_SIZE,
-   kfd->device_info->ih_ring_entry_size,
-   GFP_KERNEL);
-   if (!interrupt_ring)
-   return -ENOMEM;
-
-   kfd->interrupt_ring = interrupt_ring;
-   kfd->interrupt_ring_size =
-   KFD_INTERRUPT_RING_SIZE * kfd->device_info->ih_ring_entry_size;
-   atomic_set(>interrupt_ring_wptr, 0);
-   atomic_set(>interrupt_ring_rptr, 0);
+   int r;
+
+   r = kfifo_alloc(>ih_fifo,
+   KFD_IH_NUM_ENTRIES * kfd->device_info->ih_ring_entry_size,
+   GFP_KERNEL);
+   if (r) {
+   dev_err(kfd_chardev(), "Failed to allocate IH fifo\n");
+   return r;
+   }
 
spin_lock_init(>interrupt_lock);
 
@@ -98,68 +97,41 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
 */
flush_scheduled_work();
 
-   kfree(kfd->interrupt_ring);
+   kfifo_free(>ih_fifo);
 }
 
 /*
- * This assumes that it can't be called concurrently with itself
- * but only with dequeue_ih_ring_entry.
+ * Assumption: single reader/writer. This function is not re-entrant
  */
 bool enqueue_ih_ring_entry(struct kfd_dev *kfd,const void 
*ih_ring_entry)
 {
-   unsigned int rptr = atomic_read(>interrupt_ring_rptr);
-   unsigned int wptr = atomic_read(>interrupt_ring_wptr);
+   int count;
 
-   if ((rptr - wptr) % kfd->interrupt_ring_size ==
-   kfd->device_info->ih_ring_entry_size) {
-   /* This is very bad, the system is likely to hang. */
+   count = kfifo_in(>ih_fifo, ih_ring_entry,
+   kfd->device_info->ih_ring_entry_size);
+   if (count != kfd->device_info->ih_ring_entry_size) {
dev_err_ratelimited(kfd_chardev(),
-   "Interrupt ring overflow, dropping interrupt.\n");
+   "Interrupt ring overflow, dropping interrupt %d\n",
+   count);
return false;
}
 
-   memcpy(kfd->interrupt_ring + wptr, ih_ring_entry,
-   kfd->device_info->ih_ring_entry_size);
-
-   wptr = (wptr + kfd->device_info->ih_ring_entry_size) %
-   kfd->interrupt_ring_size;
-   smp_wmb(); /* Ensure memcpy'd data is visible before wptr update. */
-   atomic_set(>interrupt_ring_wptr, wptr);
-
return true;
 }
 
 /*
- * This assumes that it can't be called concurrently with itself
- * but only with enqueue_ih_ring_entry.
+ * Assumption: single reader/writer. This function is not re-entrant
  */
 static bool dequeue_ih_ring_entry(struct kfd_dev *kfd, void *ih_ring_entry)
 {
-   /*
-* Assume that wait queues have an implicit barrier, i.e. anything that
-* happened in the ISR before it queued work is visible.
-*/
-
-   unsigned int wptr = atomic_read(>interrupt_ring_wptr);
-   unsigned int rptr = atomic_read(>interrupt_ring_rptr);
+   int count;
 
-   if (rptr == wptr)
-   return false;
-
-   memcpy(ih_ring_entry, kfd->interrupt_ring + rptr,
-   kfd->device_info->ih_ring_entry_size);
-
-   rptr = (rptr + kfd->device_info->ih_ring_entry_size) %
-   kfd->interrupt_ring_size;
+   count = kfifo_out(>ih_fifo, ih_ring_entry,
+   kfd->device_info->ih_ring_entry_size);
 
-   /*
-* Ensure the rptr write update is not visible until
-* memcpy has finished reading.
-*/
-   smp_mb();
-   atomic_set(>interrupt_ring_rptr, rptr);
+   WARN_ON(count && count != kfd->device_info->ih_ring_entry_size);
 
-   return true;
+   return count == kfd->device_info->ih_ring_entry_size;
 }
 
 static void interrupt_wq(struct work_struct *work)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 

[PATCH 09/17] drm/amdkfd: Simplify events page allocator

[Felix Kuehling]

The first event page is always big enough to handle all events.
Handling of multiple events pages is not supported by user mode, and
not necessary.

Signed-off-by: Yong Zhao 
Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 197 +++-
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |   1 -
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |   4 +-
 3 files changed, 70 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 7dae26f..7cc1710 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,6 +41,9 @@ struct kfd_event_waiter {
bool activated;  /* Becomes true when event is signaled */
 };
 
+#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
+#define SLOT_BITMAP_LONGS BITS_TO_LONGS(SLOTS_PER_PAGE)
+
 /*
  * Over-complicated pooled allocator for event notification slots.
  *
@@ -51,132 +54,98 @@ struct kfd_event_waiter {
  * Individual signal events are then allocated a slot in a page.
  */
 
-struct signal_page {
-   struct list_head event_pages;   /* kfd_process.signal_event_pages */
+struct kfd_signal_page {
uint64_t *kernel_address;
uint64_t __user *user_address;
-   uint32_t page_index;/* Index into the mmap aperture. */
unsigned int free_slots;
-   unsigned long used_slot_bitmap[0];
+   unsigned long used_slot_bitmap[SLOT_BITMAP_LONGS];
 };
 
-#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
-#define SLOT_BITMAP_SIZE BITS_TO_LONGS(SLOTS_PER_PAGE)
-#define BITS_PER_PAGE (ilog2(SLOTS_PER_PAGE)+1)
-#define SIGNAL_PAGE_SIZE (sizeof(struct signal_page) + \
-   SLOT_BITMAP_SIZE * sizeof(long))
-
 /*
  * For signal events, the event ID is used as the interrupt user data.
  * For SQ s_sendmsg interrupts, this is limited to 8 bits.
  */
 
 #define INTERRUPT_DATA_BITS 8
-#define SIGNAL_EVENT_ID_SLOT_SHIFT 0
 
-static uint64_t *page_slots(struct signal_page *page)
+static uint64_t *page_slots(struct kfd_signal_page *page)
 {
return page->kernel_address;
 }
 
 static bool allocate_free_slot(struct kfd_process *process,
-   struct signal_page **out_page,
-   unsigned int *out_slot_index)
+  unsigned int *out_slot_index)
 {
-   struct signal_page *page;
+   struct kfd_signal_page *page = process->signal_page;
+   unsigned int slot;
 
-   list_for_each_entry(page, >signal_event_pages, event_pages) {
-   if (page->free_slots > 0) {
-   unsigned int slot =
-   find_first_zero_bit(page->used_slot_bitmap,
-   SLOTS_PER_PAGE);
+   if (!page || page->free_slots == 0) {
+   pr_debug("No free event signal slots were found for process 
%p\n",
+process);
 
-   __set_bit(slot, page->used_slot_bitmap);
-   page->free_slots--;
+   return false;
+   }
 
-   page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
+   slot = find_first_zero_bit(page->used_slot_bitmap, SLOTS_PER_PAGE);
 
-   *out_page = page;
-   *out_slot_index = slot;
+   __set_bit(slot, page->used_slot_bitmap);
+   page->free_slots--;
 
-   pr_debug("Allocated event signal slot in page %p, slot 
%d\n",
-   page, slot);
+   page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
 
-   return true;
-   }
-   }
+   *out_slot_index = slot;
 
-   pr_debug("No free event signal slots were found for process %p\n",
-   process);
+   pr_debug("Allocated event signal slot in page %p, slot %d\n",
+page, slot);
 
-   return false;
+   return true;
 }
 
-#define list_tail_entry(head, type, member) \
-   list_entry((head)->prev, type, member)
-
-static bool allocate_signal_page(struct file *devkfd, struct kfd_process *p)
+static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
 {
void *backing_store;
-   struct signal_page *page;
+   struct kfd_signal_page *page;
 
-   page = kzalloc(SIGNAL_PAGE_SIZE, GFP_KERNEL);
+   page = kzalloc(sizeof(*page), GFP_KERNEL);
if (!page)
-   goto fail_alloc_signal_page;
+   return NULL;
 
page->free_slots = SLOTS_PER_PAGE;
 
-   backing_store = (void *) __get_free_pages(GFP_KERNEL | __GFP_ZERO,
+   backing_store = (void *) __get_free_pages(GFP_KERNEL,
get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
if (!backing_store)
goto 

[PATCH 17/17] drm/amdkfd: use a high priority workqueue for IH work

[Felix Kuehling]

From: Andres Rodriguez 

In systems under heavy load the IH work may experience significant
scheduling delays.

Under load + system workqueue:
Max Latency: 7.023695 ms
Avg Latency: 0.263994 ms

Under load + high priority workqueue:
Max Latency: 1.162568 ms
Avg Latency: 0.163213 ms

Further work is required to measure the impact of per-cpu settings on IH
performance.

Signed-off-by: Andres Rodriguez 
Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_device.c| 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 3 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h  | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 46049f0..621a3b5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -403,7 +403,7 @@ void kgd2kfd_interrupt(struct kfd_dev *kfd, const void 
*ih_ring_entry)
if (kfd->interrupts_active
&& interrupt_is_wanted(kfd, ih_ring_entry)
&& enqueue_ih_ring_entry(kfd, ih_ring_entry))
-   schedule_work(>interrupt_work);
+   queue_work(kfd->ih_wq, >interrupt_work);
 
spin_unlock(>interrupt_lock);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 9c08d46..035c351 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -61,6 +61,7 @@ int kfd_interrupt_init(struct kfd_dev *kfd)
return r;
}
 
+   kfd->ih_wq = alloc_workqueue("KFD IH", WQ_HIGHPRI, 1);
spin_lock_init(>interrupt_lock);
 
INIT_WORK(>interrupt_work, interrupt_wq);
@@ -95,7 +96,7 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
 * work-queue items that will access interrupt_ring. New work items
 * can't be created because we stopped interrupt handling above.
 */
-   flush_work(>interrupt_work);
+   flush_workqueue(kfd->ih_wq);
 
kfifo_free(>ih_fifo);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 0aec5ca..6a91a60 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -184,6 +184,7 @@ struct kfd_dev {
 
/* Interrupts */
struct kfifo ih_fifo;
+   struct workqueue_struct *ih_wq;
struct work_struct interrupt_work;
spinlock_t interrupt_lock;
 
-- 
2.7.4

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

[PATCH 06/17] drm/amdkfd: Fix event destruction with pending waiters

[Felix Kuehling]

When an event with pending waiters is destroyed, those waiters may
end up sleeping forever unless they are notified and woken up.
Implement the notification by clearing the waiter->event pointer,
which becomes invalid anyway, when the event is freed, and waking
up the waiting tasks.

Waiters on an event that's destroyed return failure.

Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 72 +
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 33cafbb..2f0fe12 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -345,18 +345,24 @@ void kfd_event_init_process(struct kfd_process *p)
 
 static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
 {
+   /* Wake up pending waiters. They will return failure */
+   while (!list_empty(>waiters)) {
+   struct kfd_event_waiter *waiter =
+   list_first_entry(>waiters, struct kfd_event_waiter,
+waiters);
+
+   waiter->event = NULL;
+   /* _init because free_waiters will call list_del */
+   list_del_init(>waiters);
+   wake_up_process(waiter->sleeping_task);
+   }
+
if (ev->signal_page) {
release_event_notification_slot(ev->signal_page,
ev->signal_slot_index);
p->signal_event_count--;
}
 
-   /*
-* Abandon the list of waiters. Individual waiting threads will
-* clean up their own data.
-*/
-   list_del(>waiters);
-
hash_del(>events);
kfree(ev);
 }
@@ -646,22 +652,36 @@ static void init_event_waiter_add_to_waitlist(struct 
kfd_event_waiter *waiter)
list_add(>waiters, >waiters);
 }
 
-static bool test_event_condition(bool all, uint32_t num_events,
+/* test_event_condition - Test condition of events being waited for
+ * @all:   Return completion only if all events have signaled
+ * @num_events:Number of events to wait for
+ * @event_waiters: Array of event waiters, one per event
+ *
+ * Returns KFD_IOC_WAIT_RESULT_COMPLETE if all (or one) event(s) have
+ * signaled. Returns KFD_IOC_WAIT_RESULT_TIMEOUT if no (or not all)
+ * events have signaled. Returns KFD_IOC_WAIT_RESULT_FAIL if any of
+ * the events have been destroyed.
+ */
+static uint32_t test_event_condition(bool all, uint32_t num_events,
struct kfd_event_waiter *event_waiters)
 {
uint32_t i;
uint32_t activated_count = 0;
 
for (i = 0; i < num_events; i++) {
+   if (!event_waiters[i].event)
+   return KFD_IOC_WAIT_RESULT_FAIL;
+
if (event_waiters[i].activated) {
if (!all)
-   return true;
+   return KFD_IOC_WAIT_RESULT_COMPLETE;
 
activated_count++;
}
}
 
-   return activated_count == num_events;
+   return activated_count == num_events ?
+   KFD_IOC_WAIT_RESULT_COMPLETE : KFD_IOC_WAIT_RESULT_TIMEOUT;
 }
 
 /*
@@ -745,11 +765,6 @@ int kfd_wait_on_events(struct kfd_process *p,
 
mutex_lock(>event_mutex);
 
-   /* Set to something unreasonable - this is really
-* just a bool for now.
-*/
-   *wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
-
for (i = 0; i < num_events; i++) {
struct kfd_event_data event_data;
 
@@ -766,17 +781,22 @@ int kfd_wait_on_events(struct kfd_process *p,
}
 
/* Check condition once. */
-   if (test_event_condition(all, num_events, event_waiters)) {
-   *wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
+   *wait_result = test_event_condition(all, num_events, event_waiters);
+   if (*wait_result == KFD_IOC_WAIT_RESULT_COMPLETE) {
ret = copy_signaled_event_data(num_events,
   event_waiters, events);
goto out_unlock;
-   } else {
-   /* Add to wait lists if we need to wait. */
-   for (i = 0; i < num_events; i++)
-   init_event_waiter_add_to_waitlist(_waiters[i]);
+   } else if (WARN_ON(*wait_result == KFD_IOC_WAIT_RESULT_FAIL)) {
+   /* This should not happen. Events shouldn't be
+* destroyed while we're holding the event_mutex
+*/
+   goto out_unlock;
}
 
+   /* Add to wait lists if we need to wait. */
+   for (i = 0; i < num_events; i++)
+   init_event_waiter_add_to_waitlist(_waiters[i]);
+
mutex_unlock(>event_mutex);
 
while (true) {
@@ -809,15 +829,13 

[PATCH 08/17] drm/amdkfd: Use wait_queue_t to implement event waiting

[Felix Kuehling]

Use standard wait queues for waiting and waking up waiting threads
instead of inventing our own. We still have our own wait loop
because the HSA event semantics require the ability to have one
thread waiting on multiple wait queues (events) at the same time.

Signed-off-by: Kent Russell 
Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 59 -
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |  3 +-
 2 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index b4cda92..7dae26f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -33,22 +33,12 @@
 #include 
 
 /*
- * A task can only be on a single wait_queue at a time, but we need to support
- * waiting on multiple events (any/all).
- * Instead of each event simply having a wait_queue with sleeping tasks, it
- * has a singly-linked list of tasks.
- * A thread that wants to sleep creates an array of these, one for each event
- * and adds one to each event's waiter chain.
+ * Wrapper around wait_queue_entry_t
  */
 struct kfd_event_waiter {
-   struct list_head waiters;
-   struct task_struct *sleeping_task;
-
-   /* Transitions to true when the event this belongs to is signaled. */
-   bool activated;
-
-   /* Event */
-   struct kfd_event *event;
+   wait_queue_entry_t wait;
+   struct kfd_event *event; /* Event to wait for */
+   bool activated;  /* Becomes true when event is signaled */
 };
 
 /*
@@ -344,17 +334,12 @@ void kfd_event_init_process(struct kfd_process *p)
 
 static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
 {
-   /* Wake up pending waiters. They will return failure */
-   while (!list_empty(>waiters)) {
-   struct kfd_event_waiter *waiter =
-   list_first_entry(>waiters, struct kfd_event_waiter,
-waiters);
+   struct kfd_event_waiter *waiter;
 
+   /* Wake up pending waiters. They will return failure */
+   list_for_each_entry(waiter, >wq.head, wait.entry)
waiter->event = NULL;
-   /* _init because free_waiters will call list_del */
-   list_del_init(>waiters);
-   wake_up_process(waiter->sleeping_task);
-   }
+   wake_up_all(>wq);
 
if (ev->signal_page) {
release_event_notification_slot(ev->signal_page,
@@ -424,7 +409,7 @@ int kfd_event_create(struct file *devkfd, struct 
kfd_process *p,
ev->auto_reset = auto_reset;
ev->signaled = false;
 
-   INIT_LIST_HEAD(>waiters);
+   init_waitqueue_head(>wq);
 
*event_page_offset = 0;
 
@@ -482,19 +467,18 @@ int kfd_event_destroy(struct kfd_process *p, uint32_t 
event_id)
 static void set_event(struct kfd_event *ev)
 {
struct kfd_event_waiter *waiter;
-   struct kfd_event_waiter *next;
 
-   /* Auto reset if the list is non-empty and we're waking someone. */
-   ev->signaled = !ev->auto_reset || list_empty(>waiters);
+   /* Auto reset if the list is non-empty and we're waking
+* someone. waitqueue_active is safe here because we're
+* protected by the p->event_mutex, which is also held when
+* updating the wait queues in kfd_wait_on_events.
+*/
+   ev->signaled = !ev->auto_reset || !waitqueue_active(>wq);
 
-   list_for_each_entry_safe(waiter, next, >waiters, waiters) {
+   list_for_each_entry(waiter, >wq.head, wait.entry)
waiter->activated = true;
 
-   /* _init because free_waiters will call list_del */
-   list_del_init(>waiters);
-
-   wake_up_process(waiter->sleeping_task);
-   }
+   wake_up_all(>wq);
 }
 
 /* Assumes that p is current. */
@@ -614,8 +598,7 @@ static struct kfd_event_waiter 
*alloc_event_waiters(uint32_t num_events)
GFP_KERNEL);
 
for (i = 0; (event_waiters) && (i < num_events) ; i++) {
-   INIT_LIST_HEAD(_waiters[i].waiters);
-   event_waiters[i].sleeping_task = current;
+   init_wait(_waiters[i].wait);
event_waiters[i].activated = false;
}
 
@@ -646,7 +629,7 @@ static void init_event_waiter_add_to_waitlist(struct 
kfd_event_waiter *waiter)
 * wait on this event.
 */
if (!waiter->activated)
-   list_add(>waiters, >waiters);
+   add_wait_queue(>wq, >wait);
 }
 
 /* test_event_condition - Test condition of events being waited for
@@ -736,7 +719,9 @@ static void free_waiters(uint32_t num_events, struct 
kfd_event_waiter *waiters)
uint32_t i;
 
for (i = 0; i < num_events; i++)
-   list_del([i].waiters);
+   if (waiters[i].event)
+   

[PATCH 12/17] drm/amdkfd: Make event limit dependent on user mode mapping size

[Felix Kuehling]

This allows increasing the KFD_SIGNAL_EVENT_LIMIT in kfd_ioctl.h
without breaking processes built with older kfd_ioctl.h versions.

Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 25 +++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |  1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 26e8045..cb92d4b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -97,9 +97,17 @@ static int allocate_event_notification_slot(struct 
kfd_process *p,
p->signal_page = allocate_signal_page(p);
if (!p->signal_page)
return -ENOMEM;
+   /* Oldest user mode expects 256 event slots */
+   p->signal_mapped_size = 256*8;
}
 
-   id = idr_alloc(>event_idr, ev, 0, KFD_SIGNAL_EVENT_LIMIT,
+   /*
+* Compatibility with old user mode: Only use signal slots
+* user mode has mapped, may be less than
+* KFD_SIGNAL_EVENT_LIMIT. This also allows future increase
+* of the event limit without breaking user mode.
+*/
+   id = idr_alloc(>event_idr, ev, 0, p->signal_mapped_size / 8,
   GFP_KERNEL);
if (id < 0)
return id;
@@ -173,7 +181,8 @@ static int create_signal_event(struct file *devkfd,
 {
int ret;
 
-   if (p->signal_event_count == KFD_SIGNAL_EVENT_LIMIT) {
+   if (p->signal_mapped_size &&
+   p->signal_event_count == p->signal_mapped_size / 8) {
if (!p->signal_event_limit_reached) {
pr_warn("Signal event wasn't created because limit was 
reached\n");
p->signal_event_limit_reached = true;
@@ -744,12 +753,12 @@ int kfd_wait_on_events(struct kfd_process *p,
 
 int kfd_event_mmap(struct kfd_process *p, struct vm_area_struct *vma)
 {
-
unsigned long pfn;
struct kfd_signal_page *page;
+   int ret;
 
-   /* check required size is logical */
-   if (get_order(KFD_SIGNAL_EVENT_LIMIT * 8) !=
+   /* check required size doesn't exceed the allocated size */
+   if (get_order(KFD_SIGNAL_EVENT_LIMIT * 8) <
get_order(vma->vm_end - vma->vm_start)) {
pr_err("Event page mmap requested illegal size\n");
return -EINVAL;
@@ -779,8 +788,12 @@ int kfd_event_mmap(struct kfd_process *p, struct 
vm_area_struct *vma)
page->user_address = (uint64_t __user *)vma->vm_start;
 
/* mapping the page to user process */
-   return remap_pfn_range(vma, vma->vm_start, pfn,
+   ret = remap_pfn_range(vma, vma->vm_start, pfn,
vma->vm_end - vma->vm_start, vma->vm_page_prot);
+   if (!ret)
+   p->signal_mapped_size = vma->vm_end - vma->vm_start;
+
+   return ret;
 }
 
 /*
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index ebae8e1..ba26da8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -543,6 +543,7 @@ struct kfd_process {
struct idr event_idr;
/* Event page */
struct kfd_signal_page *signal_page;
+   size_t signal_mapped_size;
size_t signal_event_count;
bool signal_event_limit_reached;
 };
-- 
2.7.4

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

[PATCH 10/17] drm/amdkfd: Simplify event ID and signal slot management

[Felix Kuehling]

Signal slots are identical to event IDs.

Replace the used_slot_bitmap and events hash table with an IDR to
allocate and lookup event IDs and signal slots more efficiently.

Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 230 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |  14 +-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |   6 +-
 3 files changed, 80 insertions(+), 170 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 7cc1710..41580e0 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,24 +41,16 @@ struct kfd_event_waiter {
bool activated;  /* Becomes true when event is signaled */
 };
 
-#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
-#define SLOT_BITMAP_LONGS BITS_TO_LONGS(SLOTS_PER_PAGE)
-
 /*
- * Over-complicated pooled allocator for event notification slots.
- *
  * Each signal event needs a 64-bit signal slot where the signaler will write
- * a 1 before sending an interrupt.l (This is needed because some interrupts
+ * a 1 before sending an interrupt. (This is needed because some interrupts
  * do not contain enough spare data bits to identify an event.)
- * We get whole pages from vmalloc and map them to the process VA.
- * Individual signal events are then allocated a slot in a page.
+ * We get whole pages and map them to the process VA.
+ * Individual signal events use their event_id as slot index.
  */
-
 struct kfd_signal_page {
uint64_t *kernel_address;
uint64_t __user *user_address;
-   unsigned int free_slots;
-   unsigned long used_slot_bitmap[SLOT_BITMAP_LONGS];
 };
 
 /*
@@ -73,34 +65,6 @@ static uint64_t *page_slots(struct kfd_signal_page *page)
return page->kernel_address;
 }
 
-static bool allocate_free_slot(struct kfd_process *process,
-  unsigned int *out_slot_index)
-{
-   struct kfd_signal_page *page = process->signal_page;
-   unsigned int slot;
-
-   if (!page || page->free_slots == 0) {
-   pr_debug("No free event signal slots were found for process 
%p\n",
-process);
-
-   return false;
-   }
-
-   slot = find_first_zero_bit(page->used_slot_bitmap, SLOTS_PER_PAGE);
-
-   __set_bit(slot, page->used_slot_bitmap);
-   page->free_slots--;
-
-   page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
-
-   *out_slot_index = slot;
-
-   pr_debug("Allocated event signal slot in page %p, slot %d\n",
-page, slot);
-
-   return true;
-}
-
 static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
 {
void *backing_store;
@@ -110,8 +74,6 @@ static struct kfd_signal_page *allocate_signal_page(struct 
kfd_process *p)
if (!page)
return NULL;
 
-   page->free_slots = SLOTS_PER_PAGE;
-
backing_store = (void *) __get_free_pages(GFP_KERNEL,
get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
if (!backing_store)
@@ -132,28 +94,26 @@ static struct kfd_signal_page *allocate_signal_page(struct 
kfd_process *p)
return NULL;
 }
 
-static bool allocate_event_notification_slot(struct kfd_process *p,
-unsigned int *signal_slot_index)
+static int allocate_event_notification_slot(struct kfd_process *p,
+   struct kfd_event *ev)
 {
+   int id;
+
if (!p->signal_page) {
p->signal_page = allocate_signal_page(p);
if (!p->signal_page)
-   return false;
+   return -ENOMEM;
}
 
-   return allocate_free_slot(p, signal_slot_index);
-}
+   id = idr_alloc(>event_idr, ev, 0, KFD_SIGNAL_EVENT_LIMIT,
+  GFP_KERNEL);
+   if (id < 0)
+   return id;
 
-/* Assumes that the process's event_mutex is locked. */
-static void release_event_notification_slot(struct kfd_signal_page *page,
-   size_t slot_index)
-{
-   __clear_bit(slot_index, page->used_slot_bitmap);
-   page->free_slots++;
+   ev->event_id = id;
+   page_slots(p->signal_page)[id] = UNSIGNALED_EVENT_SLOT;
 
-   /* We don't free signal pages, they are retained by the process
-* and reused until it exits.
-*/
+   return 0;
 }
 
 /*
@@ -162,89 +122,32 @@ static void release_event_notification_slot(struct 
kfd_signal_page *page,
  */
 static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
 {
-   struct kfd_event *ev;
-
-   hash_for_each_possible(p->events, ev, events, id)
-   if (ev->event_id == id)
-   return ev;
-
-   return NULL;
-}
-
-/*
- * Produce a kfd event id for a nonsignal 

[PATCH 07/17] drm/amdkfd: remove redundant kfd_event_waiter.input_index

[Felix Kuehling]

This always identical with the index of the event_waiter in the array.
No need to store it in the waiter record.

Signed-off-by: Felix Kuehling 
Reviewed-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 2f0fe12..b4cda92 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -49,7 +49,6 @@ struct kfd_event_waiter {
 
/* Event */
struct kfd_event *event;
-   uint32_t input_index;
 };
 
 /*
@@ -625,8 +624,7 @@ static struct kfd_event_waiter 
*alloc_event_waiters(uint32_t num_events)
 
 static int init_event_waiter_get_status(struct kfd_process *p,
struct kfd_event_waiter *waiter,
-   uint32_t event_id,
-   uint32_t input_index)
+   uint32_t event_id)
 {
struct kfd_event *ev = lookup_event_by_id(p, event_id);
 
@@ -634,7 +632,6 @@ static int init_event_waiter_get_status(struct kfd_process 
*p,
return -EINVAL;
 
waiter->event = ev;
-   waiter->input_index = input_index;
waiter->activated = ev->signaled;
ev->signaled = ev->signaled && !ev->auto_reset;
 
@@ -702,7 +699,7 @@ static int copy_signaled_event_data(uint32_t num_events,
waiter = _waiters[i];
event = waiter->event;
if (waiter->activated && event->type == KFD_EVENT_TYPE_MEMORY) {
-   dst = [waiter->input_index].memory_exception_data;
+   dst = [i].memory_exception_data;
src = >memory_exception_data;
if (copy_to_user(dst, src,
sizeof(struct kfd_hsa_memory_exception_data)))
@@ -775,7 +772,7 @@ int kfd_wait_on_events(struct kfd_process *p,
}
 
ret = init_event_waiter_get_status(p, _waiters[i],
-   event_data.event_id, i);
+   event_data.event_id);
if (ret)
goto out_unlock;
}
-- 
2.7.4

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

[PATCH 05/17] drm/amdkfd: Clean up kfd_wait_on_events

[Felix Kuehling]

Cleaned up the code while resolving some potential bugs and
inconsistencies in the process.

Clean-ups:
* Remove enum kfd_event_wait_result, which duplicates
  KFD_IOC_EVENT_RESULT definitions
* alloc_event_waiters can be called without holding p->event_mutex
* Return an error code from copy_signaled_event_data instead of bool
* Clean up error handling code paths to minimize duplication in
  kfd_wait_on_events

Fixes:
* Consistently return an error code from kfd_wait_on_events and set
  wait_result to KFD_IOC_WAIT_RESULT_FAIL in all failure cases.
* Always call free_waiters while holding p->event_mutex
* copy_signaled_event_data might sleep. Don't call it while the task state
  is TASK_INTERRUPTIBLE.

Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c |  5 +--
 drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 71 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h|  8 +---
 3 files changed, 32 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 0ef82b2..a25321f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -835,15 +835,12 @@ static int kfd_ioctl_wait_events(struct file *filp, 
struct kfd_process *p,
void *data)
 {
struct kfd_ioctl_wait_events_args *args = data;
-   enum kfd_event_wait_result wait_result;
int err;
 
err = kfd_wait_on_events(p, args->num_events,
(void __user *)args->events_ptr,
(args->wait_for_all != 0),
-   args->timeout, _result);
-
-   args->wait_result = wait_result;
+   args->timeout, >wait_result);
 
return err;
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 1efd6a8..33cafbb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -668,7 +668,7 @@ static bool test_event_condition(bool all, uint32_t 
num_events,
  * Copy event specific data, if defined.
  * Currently only memory exception events have additional data to copy to user
  */
-static bool copy_signaled_event_data(uint32_t num_events,
+static int copy_signaled_event_data(uint32_t num_events,
struct kfd_event_waiter *event_waiters,
struct kfd_event_data __user *data)
 {
@@ -686,11 +686,11 @@ static bool copy_signaled_event_data(uint32_t num_events,
src = >memory_exception_data;
if (copy_to_user(dst, src,
sizeof(struct kfd_hsa_memory_exception_data)))
-   return false;
+   return -EFAULT;
}
}
 
-   return true;
+   return 0;
 
 }
 
@@ -727,7 +727,7 @@ static void free_waiters(uint32_t num_events, struct 
kfd_event_waiter *waiters)
 int kfd_wait_on_events(struct kfd_process *p,
   uint32_t num_events, void __user *data,
   bool all, uint32_t user_timeout_ms,
-  enum kfd_event_wait_result *wait_result)
+  uint32_t *wait_result)
 {
struct kfd_event_data __user *events =
(struct kfd_event_data __user *) data;
@@ -737,18 +737,18 @@ int kfd_wait_on_events(struct kfd_process *p,
struct kfd_event_waiter *event_waiters = NULL;
long timeout = user_timeout_to_jiffies(user_timeout_ms);
 
+   event_waiters = alloc_event_waiters(num_events);
+   if (!event_waiters) {
+   ret = -ENOMEM;
+   goto out;
+   }
+
mutex_lock(>event_mutex);
 
/* Set to something unreasonable - this is really
 * just a bool for now.
 */
-   *wait_result = KFD_WAIT_TIMEOUT;
-
-   event_waiters = alloc_event_waiters(num_events);
-   if (!event_waiters) {
-   ret = -ENOMEM;
-   goto fail;
-   }
+   *wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
 
for (i = 0; i < num_events; i++) {
struct kfd_event_data event_data;
@@ -756,23 +756,21 @@ int kfd_wait_on_events(struct kfd_process *p,
if (copy_from_user(_data, [i],
sizeof(struct kfd_event_data))) {
ret = -EFAULT;
-   goto fail;
+   goto out_unlock;
}
 
ret = init_event_waiter_get_status(p, _waiters[i],
event_data.event_id, i);
if (ret)
-   goto fail;
+   goto out_unlock;
}
 
/* Check condition once. */
if (test_event_condition(all, num_events, event_waiters)) {
-   if 

[PATCH 15/17] drm/amdkfd: increase IH num entries to 8192

[Felix Kuehling]

From: Andres Rodriguez 

A larger buffer will let us accommodate applications with a large amount
of semi-simultaneous event signals.

Signed-off-by: Andres Rodriguez 
Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index ffbb91a..a147269 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -45,7 +45,7 @@
 #include 
 #include "kfd_priv.h"
 
-#define KFD_IH_NUM_ENTRIES 1024
+#define KFD_IH_NUM_ENTRIES 8192
 
 static void interrupt_wq(struct work_struct *);
 
-- 
2.7.4

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

[PATCH 16/17] drm/amdkfd: wait only for IH work on IH exit

[Felix Kuehling]

From: Andres Rodriguez 

We don't need to wait for all work to complete in the IH exit function.
We only need to make sure the interrupt_work has finished executing to
guarantee that ih_kfifo is no longer in use.

Signed-off-by: Andres Rodriguez 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index a147269..9c08d46 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -91,11 +91,11 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
spin_unlock_irqrestore(>interrupt_lock, flags);
 
/*
-* Flush_scheduled_work ensures that there are no outstanding
+* flush_work ensures that there are no outstanding
 * work-queue items that will access interrupt_ring. New work items
 * can't be created because we stopped interrupt handling above.
 */
-   flush_scheduled_work();
+   flush_work(>interrupt_work);
 
kfifo_free(>ih_fifo);
 }
-- 
2.7.4

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

[PATCH 02/17] drm/amdkfd: Don't dereference kfd_process.mm v2

[Felix Kuehling]

The kfd_process doesn't own a reference to the mm_struct, so it can
disappear without warning even while the kfd_process still exists.

Therefore, avoid dereferencing the kfd_process.mm pointer and make
it opaque. Use get_task_mm to get a temporary reference to the mm
when it's needed.

v2: removed unnecessary WARN_ON

Signed-off-by: Felix Kuehling 
Reviewed-by: Oded Gabbay  (v1)
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h|  7 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c |  1 -
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 944abfa..61ce547 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -24,8 +24,8 @@
 #include 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 #include "kfd_priv.h"
@@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned 
int pasid,
 * running so the lookup function returns a locked process.
 */
struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
+   struct mm_struct *mm;
 
if (!p)
return; /* Presumably process exited. */
 
+   /* Take a safe reference to the mm_struct, which may otherwise
+* disappear even while the kfd_process is still referenced.
+*/
+   mm = get_task_mm(p->lead_thread);
+   if (!mm) {
+   mutex_unlock(>mutex);
+   return; /* Process is exiting */
+   }
+
memset(_exception_data, 0, sizeof(memory_exception_data));
 
-   down_read(>mm->mmap_sem);
-   vma = find_vma(p->mm, address);
+   down_read(>mmap_sem);
+   vma = find_vma(mm, address);
 
memory_exception_data.gpu_id = dev->id;
memory_exception_data.va = address;
@@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned 
int pasid,
}
}
 
-   up_read(>mm->mmap_sem);
+   up_read(>mmap_sem);
+   mmput(mm);
 
mutex_lock(>event_mutex);
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7d86ec9..1a483a7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -494,7 +494,12 @@ struct kfd_process {
 */
struct hlist_node kfd_processes;
 
-   struct mm_struct *mm;
+   /*
+* Opaque pointer to mm_struct. We don't hold a reference to
+* it so it should never be dereferenced from here. This is
+* only used for looking up processes by their mm.
+*/
+   void *mm;
 
struct mutex mutex;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 3ccb3b5..695fa2a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -200,7 +200,6 @@ static void kfd_process_destroy_delayed(struct rcu_head 
*rcu)
struct kfd_process *p;
 
p = container_of(rcu, struct kfd_process, rcu);
-   WARN_ON(atomic_read(>mm->mm_count) <= 0);
 
mmdrop(p->mm);
 
-- 
2.7.4

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

[PATCH 03/17] drm/amdkfd: Short cut for kfd_wait_on_events without waiting

[Felix Kuehling]

From: Sean Keely 

If kfd_wait_on_events can return immediately, we don't need to populate
the wait list and don't need to enter the sleep-loop.

Signed-off-by: Sean Keely 
Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 43 ++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 61ce547..f3d88c8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -617,7 +617,7 @@ static struct kfd_event_waiter 
*alloc_event_waiters(uint32_t num_events)
return event_waiters;
 }
 
-static int init_event_waiter(struct kfd_process *p,
+static int init_event_waiter_get_status(struct kfd_process *p,
struct kfd_event_waiter *waiter,
uint32_t event_id,
uint32_t input_index)
@@ -632,11 +632,20 @@ static int init_event_waiter(struct kfd_process *p,
waiter->activated = ev->signaled;
ev->signaled = ev->signaled && !ev->auto_reset;
 
-   list_add(>waiters, >waiters);
-
return 0;
 }
 
+static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
+{
+   struct kfd_event *ev = waiter->event;
+
+   /* Only add to the wait list if we actually need to
+* wait on this event.
+*/
+   if (!waiter->activated)
+   list_add(>waiters, >waiters);
+}
+
 static bool test_event_condition(bool all, uint32_t num_events,
struct kfd_event_waiter *event_waiters)
 {
@@ -724,11 +733,17 @@ int kfd_wait_on_events(struct kfd_process *p,
(struct kfd_event_data __user *) data;
uint32_t i;
int ret = 0;
+
struct kfd_event_waiter *event_waiters = NULL;
long timeout = user_timeout_to_jiffies(user_timeout_ms);
 
mutex_lock(>event_mutex);
 
+   /* Set to something unreasonable - this is really
+* just a bool for now.
+*/
+   *wait_result = KFD_WAIT_TIMEOUT;
+
event_waiters = alloc_event_waiters(num_events);
if (!event_waiters) {
ret = -ENOMEM;
@@ -744,14 +759,34 @@ int kfd_wait_on_events(struct kfd_process *p,
goto fail;
}
 
-   ret = init_event_waiter(p, _waiters[i],
+   ret = init_event_waiter_get_status(p, _waiters[i],
event_data.event_id, i);
if (ret)
goto fail;
}
 
+   /* Check condition once. */
+   if (test_event_condition(all, num_events, event_waiters)) {
+   if (copy_signaled_event_data(num_events,
+   event_waiters, events))
+   *wait_result = KFD_WAIT_COMPLETE;
+   else
+   *wait_result = KFD_WAIT_ERROR;
+   free_waiters(num_events, event_waiters);
+   } else {
+   /* Add to wait lists if we need to wait. */
+   for (i = 0; i < num_events; i++)
+   init_event_waiter_add_to_waitlist(_waiters[i]);
+   }
+
mutex_unlock(>event_mutex);
 
+   /* Return if all waits were already satisfied. */
+   if (*wait_result != KFD_WAIT_TIMEOUT) {
+   __set_current_state(TASK_RUNNING);
+   return ret;
+   }
+
while (true) {
if (fatal_signal_pending(current)) {
ret = -EINTR;
-- 
2.7.4

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

[PATCH 00/17] KFD interrupt and signal event handling improvements v2

[Felix Kuehling]

This patch series improves interrupt handling latency, signal event
processing overhead and replaces some custom data structures with
standard kernel data structures (idr, kfifo, waitqueue).

It also increases the capacity of the number of signals that can be
processed from 256 to 4096. This breaks ancient versions of the Thunk
that support only 256 signal events. The current WIP-version on github
supports both sizes. If support for ancient Thunks is considered
important, this could be fixed by allowing mappings that are smaller
than 4096 signals, and limiting the number of signals per process
depending on the size of the mapped events page.

v2:
 * Don't break ABI when changing KFD_SIGNAL_EVENT_LIMIT
 * Integrated review feedback for "Use wait_queue_t to implement event waiting"
 * Integrated review feedback for "Don't dereference kfd_process.mm"

Andres Rodriguez (4):
  drm/amdkfd: use standard kernel kfifo for IH
  drm/amdkfd: increase IH num entries to 8192
  drm/amdkfd: wait only for IH work on IH exit
  drm/amdkfd: use a high priority workqueue for IH work

Besar Wicaksono (1):
  drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list

Felix Kuehling (9):
  drm/amdkfd: Don't dereference kfd_process.mm v2
  drm/amdkfd: Clean up kfd_wait_on_events
  drm/amdkfd: Fix event destruction with pending waiters
  drm/amdkfd: remove redundant kfd_event_waiter.input_index
  drm/amdkfd: Use wait_queue_t to implement event waiting
  drm/amdkfd: Simplify events page allocator
  drm/amdkfd: Simplify event ID and signal slot management
  drm/amdkfd: Use IH context ID for signal lookup
  drm/amdkfd: Make event limit dependent on user mode mapping size

Oded Gabbay (1):
  drm/amdkfd: increase limit of signal events to 4096 per process

Sean Keely (2):
  drm/amdkfd: Short cut for kfd_wait_on_events without waiting
  drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop

 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |   8 +-
 drivers/gpu/drm/amd/amdkfd/cik_int.h |   3 +-
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c |   5 +-
 drivers/gpu/drm/amd/amdkfd/kfd_device.c  |   2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 615 +++
 drivers/gpu/drm/amd/amdkfd/kfd_events.h  |  18 +-
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c   |  83 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h|  33 +-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c |   1 -
 include/uapi/linux/kfd_ioctl.h   |   2 +-
 10 files changed, 350 insertions(+), 420 deletions(-)

-- 
2.7.4

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

[PATCH 01/17] drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list

[Felix Kuehling]

From: Besar Wicaksono 

This enables SDMA signalling with event interrupt.

Signed-off-by: Besar Wicaksono 
Signed-off-by: Felix Kuehling 
Reviewed-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 3 +++
 drivers/gpu/drm/amd/amdkfd/cik_int.h | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c 
b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
index 211fc48..66164aa 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
@@ -36,6 +36,7 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
/* Do not process in ISR, just request it to be forwarded to WQ. */
return (pasid != 0) &&
(ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
+   ihre->source_id == CIK_INTSRC_SDMA_TRAP ||
ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
 }
@@ -54,6 +55,8 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
 
if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
kfd_signal_event_interrupt(pasid, 0, 0);
+   else if (ihre->source_id == CIK_INTSRC_SDMA_TRAP)
+   kfd_signal_event_interrupt(pasid, 0, 0);
else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h 
b/drivers/gpu/drm/amd/amdkfd/cik_int.h
index 79a16d2..109298b 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
+++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
@@ -32,9 +32,10 @@ struct cik_ih_ring_entry {
uint32_t reserved;
 };
 
-#define CIK_INTSRC_DEQUEUE_COMPLETE0xC6
 #define CIK_INTSRC_CP_END_OF_PIPE  0xB5
 #define CIK_INTSRC_CP_BAD_OPCODE   0xB7
+#define CIK_INTSRC_DEQUEUE_COMPLETE0xC6
+#define CIK_INTSRC_SDMA_TRAP   0xE0
 #define CIK_INTSRC_SQ_INTERRUPT_MSG0xEF
 
 #endif
-- 
2.7.4

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

[PATCH 04/17] drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop

[Felix Kuehling]

From: Sean Keely 

Signed-off-by: Sean Keely 
Signed-off-by: Felix Kuehling 
Acked-by: Oded Gabbay 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index f3d88c8..1efd6a8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -806,6 +806,17 @@ int kfd_wait_on_events(struct kfd_process *p,
break;
}
 
+   /* Set task state to interruptible sleep before
+* checking wake-up conditions. A concurrent wake-up
+* will put the task back into runnable state. In that
+* case schedule_timeout will not put the task to
+* sleep and we'll get a chance to re-check the
+* updated conditions almost immediately. Otherwise,
+* this race condition would lead to a soft hang or a
+* very long sleep.
+*/
+   set_current_state(TASK_INTERRUPTIBLE);
+
if (test_event_condition(all, num_events, event_waiters)) {
if (copy_signaled_event_data(num_events,
event_waiters, events))
@@ -820,7 +831,7 @@ int kfd_wait_on_events(struct kfd_process *p,
break;
}
 
-   timeout = schedule_timeout_interruptible(timeout);
+   timeout = schedule_timeout(timeout);
}
__set_current_state(TASK_RUNNING);
 
-- 
2.7.4

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

Re: [PATCH 27/29] drm/amd/display: Explicitly call ->reset for each object

[Andrey Grodzovsky]

On 10/27/2017 03:33 PM, Harry Wentland wrote:

On 2017-10-26 10:51 PM, Andrey Grodzovsky wrote:


On 2017-10-26 02:35 PM, Harry Wentland wrote:

We need to avoid calling reset after detection.

Could you explain why please ?

Reset creates new, clean atomic_state objects. In this case we want to attach 
the freesync_capable property on the atomic_state at detection (see next change 
to convert the property from legacy to atomic). Calling reset after detection 
would clear that.


I see, maybe then add this explanation to the log message.

Thanks,
Andrey



This is much simpler
if we call ->reset on the connector right after creation but before
detection. To stay consistent call ->reset on every other object
as well after creation.

Signed-off-by: Harry Wentland 
Reviewed-by: Roman Li 
Acked-by: Harry Wentland 
---
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 --
   1 file changed, 12 insertions(+), 2 deletions(-)

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 6fc043957bbf..62e8db1f113c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1436,8 +1436,6 @@ static int amdgpu_dm_initialize_drm_device(struct 
amdgpu_device *adev)
   goto fail;
   }
   -drm_mode_config_reset(dm->ddev);

This is a standard helper called by many drivers on driver init , it's also 
called in drm_atomic_helper_resume
which we use on resume from suspend so now it's kind of asymmetrical behavior.


I'll have to take a look at the resume case. It seems atomic never intended to 
deal with immutable properties that are set in the driver (like 
freesync_capable). We might have to revisit that but in light of the fact that 
we need to redo these properties anyways in a generic fashion for upstream 
freesync I was hesitant to spend too much time on our non-upstream version of 
the freesync properties.

Harry


Thanks,
Andrey


-
   return 0;
   fail:
   kfree(aencoder);
@@ -3105,6 +3103,11 @@ static int amdgpu_dm_plane_init(struct 
amdgpu_display_manager *dm,
 drm_plane_helper_add(>base, _plane_helper_funcs);
   +/* Create (reset) the plane state */
+if (aplane->base.funcs->reset)
+aplane->base.funcs->reset(>base);
+
+
   return res;
   }
   @@ -3140,6 +3143,10 @@ static int amdgpu_dm_crtc_init(struct 
amdgpu_display_manager *dm,
 drm_crtc_helper_add(>base, _dm_crtc_helper_funcs);
   +/* Create (reset) the plane state */
+if (acrtc->base.funcs->reset)
+acrtc->base.funcs->reset(>base);
+
   acrtc->max_cursor_width = dm->adev->dm.dc->caps.max_cursor_size;
   acrtc->max_cursor_height = dm->adev->dm.dc->caps.max_cursor_size;
   @@ -3500,6 +3507,9 @@ static int amdgpu_dm_connector_init(struct 
amdgpu_display_manager *dm,
   >base,
   _dm_connector_helper_funcs);
   +if (aconnector->base.funcs->reset)
+aconnector->base.funcs->reset(>base);
+
   amdgpu_dm_connector_init_helper(
   dm,
   aconnector,


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

RE: [PATCH] drm/amd/powerplay: change ASIC temperature reading on Vega10

[Deucher, Alexander]

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Eric Huang
> Sent: Friday, October 27, 2017 3:31 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Huang, JinHuiEric
> Subject: [PATCH] drm/amd/powerplay: change ASIC temperature reading on
> Vega10
> 
> ASIC temperature reading from HOTSPOT to ASIC edge.
> 
> Signed-off-by: Eric Huang 

Maybe add to the commit message that this makes things consistent with previous 
asics.

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
> index 1feefac..dc3761b 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
> @@ -365,8 +365,8 @@ int vega10_thermal_get_temperature(struct
> pp_hwmgr *hwmgr)
> 
>   temp = cgs_read_register(hwmgr->device, reg);
> 
> - temp = (temp &
> CG_MULT_THERMAL_STATUS__ASIC_MAX_TEMP_MASK) >>
> -
>   CG_MULT_THERMAL_STATUS__ASIC_MAX_TEMP__SHIFT;
> + temp = (temp & CG_MULT_THERMAL_STATUS__CTF_TEMP_MASK)
> >>
> + CG_MULT_THERMAL_STATUS__CTF_TEMP__SHIFT;
> 
>   temp = temp & 0x1ff;
> 
> --
> 2.7.4
> 
> ___
> 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

Re: [PATCH 27/29] drm/amd/display: Explicitly call ->reset for each object

[Harry Wentland]

On 2017-10-26 10:51 PM, Andrey Grodzovsky wrote:
> 
> 
> On 2017-10-26 02:35 PM, Harry Wentland wrote:
>> We need to avoid calling reset after detection.
> 
> Could you explain why please ?

Reset creates new, clean atomic_state objects. In this case we want to attach 
the freesync_capable property on the atomic_state at detection (see next change 
to convert the property from legacy to atomic). Calling reset after detection 
would clear that.

> 
>> This is much simpler
>> if we call ->reset on the connector right after creation but before
>> detection. To stay consistent call ->reset on every other object
>> as well after creation.
>>
>> Signed-off-by: Harry Wentland 
>> Reviewed-by: Roman Li 
>> Acked-by: Harry Wentland 
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 --
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> 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 6fc043957bbf..62e8db1f113c 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1436,8 +1436,6 @@ static int amdgpu_dm_initialize_drm_device(struct 
>> amdgpu_device *adev)
>>   goto fail;
>>   }
>>   -    drm_mode_config_reset(dm->ddev);
> 
> This is a standard helper called by many drivers on driver init , it's also 
> called in drm_atomic_helper_resume
> which we use on resume from suspend so now it's kind of asymmetrical behavior.
> 

I'll have to take a look at the resume case. It seems atomic never intended to 
deal with immutable properties that are set in the driver (like 
freesync_capable). We might have to revisit that but in light of the fact that 
we need to redo these properties anyways in a generic fashion for upstream 
freesync I was hesitant to spend too much time on our non-upstream version of 
the freesync properties.

Harry

> Thanks,
> Andrey
> 
>> -
>>   return 0;
>>   fail:
>>   kfree(aencoder);
>> @@ -3105,6 +3103,11 @@ static int amdgpu_dm_plane_init(struct 
>> amdgpu_display_manager *dm,
>>     drm_plane_helper_add(>base, _plane_helper_funcs);
>>   +    /* Create (reset) the plane state */
>> +    if (aplane->base.funcs->reset)
>> +    aplane->base.funcs->reset(>base);
>> +
>> +
>>   return res;
>>   }
>>   @@ -3140,6 +3143,10 @@ static int amdgpu_dm_crtc_init(struct 
>> amdgpu_display_manager *dm,
>>     drm_crtc_helper_add(>base, _dm_crtc_helper_funcs);
>>   +    /* Create (reset) the plane state */
>> +    if (acrtc->base.funcs->reset)
>> +    acrtc->base.funcs->reset(>base);
>> +
>>   acrtc->max_cursor_width = dm->adev->dm.dc->caps.max_cursor_size;
>>   acrtc->max_cursor_height = dm->adev->dm.dc->caps.max_cursor_size;
>>   @@ -3500,6 +3507,9 @@ static int amdgpu_dm_connector_init(struct 
>> amdgpu_display_manager *dm,
>>   >base,
>>   _dm_connector_helper_funcs);
>>   +    if (aconnector->base.funcs->reset)
>> +    aconnector->base.funcs->reset(>base);
>> +
>>   amdgpu_dm_connector_init_helper(
>>   dm,
>>   aconnector,
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/powerplay: change ASIC temperature reading on Vega10

[Eric Huang]

ASIC temperature reading from HOTSPOT to ASIC edge.

Signed-off-by: Eric Huang 
---
 drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
index 1feefac..dc3761b 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
@@ -365,8 +365,8 @@ int vega10_thermal_get_temperature(struct pp_hwmgr *hwmgr)
 
temp = cgs_read_register(hwmgr->device, reg);
 
-   temp = (temp & CG_MULT_THERMAL_STATUS__ASIC_MAX_TEMP_MASK) >>
-   CG_MULT_THERMAL_STATUS__ASIC_MAX_TEMP__SHIFT;
+   temp = (temp & CG_MULT_THERMAL_STATUS__CTF_TEMP_MASK) >>
+   CG_MULT_THERMAL_STATUS__CTF_TEMP__SHIFT;
 
temp = temp & 0x1ff;
 
-- 
2.7.4

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

Re: [PATCH] amdgpu/dc: Avoid dereferencing NULL pointer

[Harry Wentland]

On 2017-10-27 02:34 PM, Drew Davenport wrote:
> crtc is dereferenced from within drm_atomic_get_new_crtc_state, so
> check for NULL before initializing new_crtc_state.
> 
> Signed-off-by: Drew Davenport 

Reviewed-by: Harry Wentland 

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> 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 d0ee1b3b8b5c..5a440fadbe18 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3874,8 +3874,7 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>   /* update planes when needed */
>   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> new_plane_state, i) {
>   struct drm_crtc *crtc = new_plane_state->crtc;
> - struct drm_crtc_state *new_crtc_state =
> - drm_atomic_get_new_crtc_state(state, crtc);
> + struct drm_crtc_state *new_crtc_state;
>   struct drm_framebuffer *fb = new_plane_state->fb;
>   bool pflip_needed;
>   struct dm_plane_state *dm_new_plane_state = 
> to_dm_plane_state(new_plane_state);
> @@ -3885,7 +3884,11 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>   continue;
>   }
>  
> - if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
> + if (!fb || !crtc || pcrtc != crtc)
> + continue;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + if (!new_crtc_state->active)
>   continue;
>  
>   pflip_needed = !state->allow_modeset;
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Felix Kuehling]

On 2017-10-27 03:41 AM, Christian König wrote:
> Am 27.10.2017 um 09:22 schrieb Christian König:
>> Am 26.10.2017 um 20:54 schrieb Felix Kuehling:
>>> On 2017-10-26 02:11 PM, Christian König wrote:
 But now reading the patch there is something else which I stumbled
 over:
> - WARN_ON(atomic_read(>mm->mm_count) <= 0);
> +    /*
> + * This cast should be safe here because we grabbed a
> + * reference to the mm in kfd_process_notifier_release
> + */
> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count)
> <= 0);
>       mmdrop(p->mm);
 Well that isn't good coding style. You shouldn't obfuscate what
 pointer it is by changing it to "void*", but rather set it to NULL as
 soon as you know that it is stale.

 Additional to that it is certainly not job of the driver to warn on a
 run over mm_count.
>>> Yeah. We don't have this in our current staging branch. The whole
>>> process teardown has changed quite a bit. I just fixed this up to make
>>> it work with current upstream.
>>>
>>> If you prefer, I could just remove the WARN_ON.
>>
>> That sounds like a good idea to me, yes.
>
> Wait a second, now that I though once more about it what do you mean
> with this comment:
>> +    /*
>> + * Opaque pointer to mm_struct. We don't hold a reference to
>> + * it so it should never be dereferenced from here. This is
>> + * only used for looking up processes by their mm.
>> + */ 
> E.g. what means looking up the processes by their mm?
>
> Do you use a hashtable or something like this and then check if you
> got the correct mm structure by comparing the pointers?
>
> If that is the case then you should certainly set p->mm to NULL after
> mmdrop(p->mm).
>
> Otherwise it can happen that a new mm structure is allocated at the
> same location as the old one and you run into problems because the
> kernel driver accidentally uses the wrong kfd_process structure.

The processes are in an SRCU-protected hashtable that's read by
find_process_by_mm. Before this function (kfd_process_destroy_delayed)
runs, the process is already removed from the hash table and
synchronize_srcu is used to ensure there are no more readers active.
That means the process being destroyed can't be found any more, so there
is no need to reset p->mm to NULL at that point.

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>    Felix
>>>
 Regards,
 Christian.

> Regards,
>     Felix
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Felix Kuehling 
>>> ---
>>>     drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19
>>> +++
>>>     drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++-
>>>     drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +-
>>>     3 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> index 944abfa..61ce547 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> @@ -24,8 +24,8 @@
>>>     #include 
>>>     #include 
>>>     #include 
>>> +#include 
>>>     #include 
>>> -#include 
>>>     #include 
>>>     #include 
>>>     #include "kfd_priv.h"
>>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>>> *dev, unsigned int pasid,
>>>  * running so the lookup function returns a locked process.
>>>  */
>>>     struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>>> +    struct mm_struct *mm;
>>>       if (!p)
>>>     return; /* Presumably process exited. */
>>>     +    /* Take a safe reference to the mm_struct, which may
>>> otherwise
>>> + * disappear even while the kfd_process is still referenced.
>>> + */
>>> +    mm = get_task_mm(p->lead_thread);
>>> +    if (!mm) {
>>> +    mutex_unlock(>mutex);
>>> +    return; /* Process is exiting */
>>> +    }
>>> +
>>>     memset(_exception_data, 0,
>>> sizeof(memory_exception_data));
>>>     -    down_read(>mm->mmap_sem);
>>> -    vma = find_vma(p->mm, address);
>>> +    down_read(>mmap_sem);
>>> +    vma = find_vma(mm, address);
>>>       memory_exception_data.gpu_id = dev->id;
>>>     memory_exception_data.va = address;
>>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev
>>> *dev,
>>> unsigned int pasid,
>>>     }
>>>     }
>>>     -    up_read(>mm->mmap_sem);
>>> +    up_read(>mmap_sem);
>>> +    mmput(mm);
>>>       mutex_lock(>event_mutex);
>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> index 7d86ec9..1a483a7 

[PATCH] amdgpu/dc: Avoid dereferencing NULL pointer

[Drew Davenport]

crtc is dereferenced from within drm_atomic_get_new_crtc_state, so
check for NULL before initializing new_crtc_state.

Signed-off-by: Drew Davenport 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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 d0ee1b3b8b5c..5a440fadbe18 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3874,8 +3874,7 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
/* update planes when needed */
for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
new_plane_state, i) {
struct drm_crtc *crtc = new_plane_state->crtc;
-   struct drm_crtc_state *new_crtc_state =
-   drm_atomic_get_new_crtc_state(state, crtc);
+   struct drm_crtc_state *new_crtc_state;
struct drm_framebuffer *fb = new_plane_state->fb;
bool pflip_needed;
struct dm_plane_state *dm_new_plane_state = 
to_dm_plane_state(new_plane_state);
@@ -3885,7 +3884,11 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
continue;
}
 
-   if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
+   if (!fb || !crtc || pcrtc != crtc)
+   continue;
+
+   new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+   if (!new_crtc_state->active)
continue;
 
pflip_needed = !state->allow_modeset;
-- 
2.15.0.rc2.357.g7e34df9404-goog

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

Re: [PATCH v3] drm/amd/display: Don't print error when bo_pin is interrupted

[Christian König]

Am 27.10.2017 um 17:51 schrieb Harry Wentland:

v2: Also don't print for ERESTARTSYS or EAGAIN
v3: Best practice is to only ignore ERESTARTSYS

Signed-off-by: Harry Wentland 


Reviewed-by: Christian König .


---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

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 cf15701f208d..8133711d85b9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2944,7 +2944,8 @@ static int dm_plane_helper_prepare_fb(struct drm_plane 
*plane,
amdgpu_bo_unreserve(rbo);
  
  	if (unlikely(r != 0)) {

-   DRM_ERROR("Failed to pin framebuffer\n");
+   if (r != -ERESTARTSYS))
+   DRM_ERROR("Failed to pin framebuffer with error %d\n", 
r);
return r;
}
  



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

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

[Christian König]

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 

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

Signed-off-by: Christian König 
---
   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



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

[PATCH v3] drm/amd/display: Don't print error when bo_pin is interrupted

[Harry Wentland]

v2: Also don't print for ERESTARTSYS or EAGAIN
v3: Best practice is to only ignore ERESTARTSYS

Signed-off-by: Harry Wentland 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 cf15701f208d..8133711d85b9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2944,7 +2944,8 @@ static int dm_plane_helper_prepare_fb(struct drm_plane 
*plane,
amdgpu_bo_unreserve(rbo);
 
if (unlikely(r != 0)) {
-   DRM_ERROR("Failed to pin framebuffer\n");
+   if (r != -ERESTARTSYS))
+   DRM_ERROR("Failed to pin framebuffer with error %d\n", 
r);
return r;
}
 
-- 
2.14.1

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

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

[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 
>>>
>>> Just set the bit so that DC does the hardware programming.
>>>
>>> Signed-off-by: Christian König 
>>> ---
>>>   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
> 
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check if modeset is required before adding plane

[Harry Wentland]

On 2017-10-26 11:25 PM, S, Shirish wrote:
> 
> From: Shirish S 
> 
> Adding affected planes without checking if modeset is requested from the user 
> space causes performance regression in video p/b scenarios when full screen 
> p/b is not composited.
> 
> Hence add a check before adding a plane as affected.
> 
> bug: https://bugs.freedesktop.org/show_bug.cgi?id=103408
> 
> Signed-off-by: Shirish S 

Reviewed-by: Harry Wentland 

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++
>  1 file changed, 3 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 f0b50d9..e6ec130 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4727,6 +4727,9 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   }
>   } else {
>   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, 
> new_crtc_state, i) {
> + if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> + continue;
> +
>   if (!new_crtc_state->enable)
>   continue;
>  
> --
> 2.7.4
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm v2 1/2] amdgpu: Add wrappers for AMDGPU_VM IOCTL.

[Christian König]

Am 27.10.2017 um 17:09 schrieb Andrey Grodzovsky:

From: Andrey Grodzovsky 

v2:
Rename wrappers to match the IOCTL naming, fix
identation and fix make check error.

Signed-off-by: Andrey Grodzovsky 


Reviewed-by: Christian König  for both.


---
  amdgpu/Makefile.sources|  1 +
  amdgpu/amdgpu-symbol-check |  2 ++
  amdgpu/amdgpu.h| 18 +
  amdgpu/amdgpu_vm.c | 50 ++
  4 files changed, 71 insertions(+)
  create mode 100644 amdgpu/amdgpu_vm.c

diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
index bc3abaa..498b64c 100644
--- a/amdgpu/Makefile.sources
+++ b/amdgpu/Makefile.sources
@@ -6,6 +6,7 @@ LIBDRM_AMDGPU_FILES := \
amdgpu_gpu_info.c \
amdgpu_internal.h \
amdgpu_vamgr.c \
+   amdgpu_vm.c \
util_hash.c \
util_hash.h \
util_hash_table.c \
diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index 095c3a0..d476038 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -64,6 +64,8 @@ amdgpu_read_mm_registers
  amdgpu_va_range_alloc
  amdgpu_va_range_free
  amdgpu_va_range_query
+amdgpu_vm_reserve_vmid
+amdgpu_vm_unreserve_vmid
  EOF
  done)
  
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h

index ecc975f..597fc2b 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -1489,6 +1489,24 @@ void amdgpu_cs_chunk_fence_to_dep(struct amdgpu_cs_fence 
*fence,
  void amdgpu_cs_chunk_fence_info_to_data(struct amdgpu_cs_fence_info 
*fence_info,
struct drm_amdgpu_cs_chunk_data *data);
  
+/**

+ * Reserve VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_reserve_vmid(amdgpu_context_handle context, uint32_t flags);
+
+/**
+ * Free reserved VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_unreserve_vmid(amdgpu_context_handle context, uint32_t flags);
+
  #ifdef __cplusplus
  }
  #endif
diff --git a/amdgpu/amdgpu_vm.c b/amdgpu/amdgpu_vm.c
new file mode 100644
index 000..8a9a0a1
--- /dev/null
+++ b/amdgpu/amdgpu_vm.c
@@ -0,0 +1,50 @@
+/*
+ * Copyright 2017 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+
+#include "amdgpu.h"
+#include "amdgpu_drm.h"
+#include "xf86drm.h"
+#include "amdgpu_internal.h"
+
+int amdgpu_vm_reserve_vmid(amdgpu_context_handle context, uint32_t flags)
+{
+   union drm_amdgpu_vm vm;
+
+   vm.in.op = AMDGPU_VM_OP_RESERVE_VMID;
+   vm.in.flags = flags;
+
+   return drmCommandWriteRead(context->dev->fd, DRM_AMDGPU_VM,
+  , sizeof(vm));
+}
+
+int amdgpu_vm_unreserve_vmid(amdgpu_context_handle context, uint32_t flags)
+{
+   union drm_amdgpu_vm vm;
+
+   vm.in.op = AMDGPU_VM_OP_UNRESERVE_VMID;
+   vm.in.flags = flags;
+
+   return drmCommandWriteRead(context->dev->fd, DRM_AMDGPU_VM,
+  , sizeof(vm));
+}



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

[PATCH libdrm v2 2/2] amdgpu: Add VMID reservation per GPU context test.

[Andrey Grodzovsky]

From: Andrey Grodzovsky 

The test will Reserve a VMID, submit a command and
unreserve the VMID.

v2:
Wrappers names were changed.

Signed-off-by: Andrey Grodzovsky 
---
 tests/amdgpu/Makefile.am   |   3 +-
 tests/amdgpu/amdgpu_test.c |   7 +++
 tests/amdgpu/amdgpu_test.h |  15 +
 tests/amdgpu/vm_tests.c| 151 +
 4 files changed, 175 insertions(+), 1 deletion(-)
 create mode 100644 tests/amdgpu/vm_tests.c

diff --git a/tests/amdgpu/Makefile.am b/tests/amdgpu/Makefile.am
index 8700c4d..e79c1bd 100644
--- a/tests/amdgpu/Makefile.am
+++ b/tests/amdgpu/Makefile.am
@@ -31,4 +31,5 @@ amdgpu_test_SOURCES = \
uvd_enc_tests.c \
vcn_tests.c \
uve_ib.h \
-   deadlock_tests.c
+   deadlock_tests.c \
+   vm_tests.c
diff --git a/tests/amdgpu/amdgpu_test.c b/tests/amdgpu/amdgpu_test.c
index 9925503..a82d9ab 100644
--- a/tests/amdgpu/amdgpu_test.c
+++ b/tests/amdgpu/amdgpu_test.c
@@ -103,6 +103,13 @@ static CU_SuiteInfo suites[] = {
.pCleanupFunc = suite_deadlock_tests_clean,
.pTests = deadlock_tests,
},
+   {
+   .pName = "VM Tests",
+   .pInitFunc = suite_vm_tests_init,
+   .pCleanupFunc = suite_vm_tests_clean,
+   .pTests = vm_tests,
+   },
+
CU_SUITE_INFO_NULL,
 };
 
diff --git a/tests/amdgpu/amdgpu_test.h b/tests/amdgpu/amdgpu_test.h
index ece93f4..4fffbc6 100644
--- a/tests/amdgpu/amdgpu_test.h
+++ b/tests/amdgpu/amdgpu_test.h
@@ -150,6 +150,21 @@ int suite_deadlock_tests_clean();
 extern CU_TestInfo deadlock_tests[];
 
 /**
+ * Initialize vm test suite
+ */
+int suite_vm_tests_init();
+
+/**
+ * Deinitialize deadlock test suite
+ */
+int suite_vm_tests_clean();
+
+/**
+ * Tests in vm test suite
+ */
+extern CU_TestInfo vm_tests[];
+
+/**
  * Helper functions
  */
 static inline amdgpu_bo_handle gpu_mem_alloc(
diff --git a/tests/amdgpu/vm_tests.c b/tests/amdgpu/vm_tests.c
new file mode 100644
index 000..cbfafe9
--- /dev/null
+++ b/tests/amdgpu/vm_tests.c
@@ -0,0 +1,151 @@
+/*
+ * Copyright 2017 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+*/
+
+#include "CUnit/Basic.h"
+
+#include "amdgpu_test.h"
+#include "amdgpu_drm.h"
+
+static  amdgpu_device_handle device_handle;
+static  uint32_t  major_version;
+static  uint32_t  minor_version;
+
+
+static void amdgpu_vmid_reserve_test(void);
+
+int suite_vm_tests_init(void)
+{
+   struct amdgpu_gpu_info gpu_info = {0};
+   int r;
+
+   r = amdgpu_device_initialize(drm_amdgpu[0], _version,
+  _version, _handle);
+
+   if (r) {
+   if ((r == -EACCES) && (errno == EACCES))
+   printf("\n\nError:%s. "
+   "Hint:Try to run this test program as root.",
+   strerror(errno));
+   return CUE_SINIT_FAILED;
+   }
+
+   return CUE_SUCCESS;
+}
+
+int suite_vm_tests_clean(void)
+{
+   int r = amdgpu_device_deinitialize(device_handle);
+
+   if (r == 0)
+   return CUE_SUCCESS;
+   else
+   return CUE_SCLEAN_FAILED;
+}
+
+
+CU_TestInfo vm_tests[] = {
+   { "resere vmid test",  amdgpu_vmid_reserve_test },
+   CU_TEST_INFO_NULL,
+};
+
+static void amdgpu_vmid_reserve_test(void)
+{
+   amdgpu_context_handle context_handle;
+   amdgpu_bo_handle ib_result_handle;
+   void *ib_result_cpu;
+   uint64_t ib_result_mc_address;
+   struct amdgpu_cs_request ibs_request;
+   struct amdgpu_cs_ib_info ib_info;
+   struct amdgpu_cs_fence fence_status;
+   uint32_t expired, flags;
+   int i, r, instance;
+   amdgpu_bo_list_handle bo_list;
+   amdgpu_va_handle va_handle;
+   union drm_amdgpu_vm vm;
+   static uint32_t *ptr;
+
+   r = 

[PATCH libdrm v2 1/2] amdgpu: Add wrappers for AMDGPU_VM IOCTL.

[Andrey Grodzovsky]

From: Andrey Grodzovsky 

v2:
Rename wrappers to match the IOCTL naming, fix
identation and fix make check error.

Signed-off-by: Andrey Grodzovsky 
---
 amdgpu/Makefile.sources|  1 +
 amdgpu/amdgpu-symbol-check |  2 ++
 amdgpu/amdgpu.h| 18 +
 amdgpu/amdgpu_vm.c | 50 ++
 4 files changed, 71 insertions(+)
 create mode 100644 amdgpu/amdgpu_vm.c

diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
index bc3abaa..498b64c 100644
--- a/amdgpu/Makefile.sources
+++ b/amdgpu/Makefile.sources
@@ -6,6 +6,7 @@ LIBDRM_AMDGPU_FILES := \
amdgpu_gpu_info.c \
amdgpu_internal.h \
amdgpu_vamgr.c \
+   amdgpu_vm.c \
util_hash.c \
util_hash.h \
util_hash_table.c \
diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index 095c3a0..d476038 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -64,6 +64,8 @@ amdgpu_read_mm_registers
 amdgpu_va_range_alloc
 amdgpu_va_range_free
 amdgpu_va_range_query
+amdgpu_vm_reserve_vmid
+amdgpu_vm_unreserve_vmid
 EOF
 done)
 
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index ecc975f..597fc2b 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -1489,6 +1489,24 @@ void amdgpu_cs_chunk_fence_to_dep(struct amdgpu_cs_fence 
*fence,
 void amdgpu_cs_chunk_fence_info_to_data(struct amdgpu_cs_fence_info 
*fence_info,
struct drm_amdgpu_cs_chunk_data *data);
 
+/**
+ * Reserve VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_reserve_vmid(amdgpu_context_handle context, uint32_t flags);
+
+/**
+ * Free reserved VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_unreserve_vmid(amdgpu_context_handle context, uint32_t flags);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/amdgpu/amdgpu_vm.c b/amdgpu/amdgpu_vm.c
new file mode 100644
index 000..8a9a0a1
--- /dev/null
+++ b/amdgpu/amdgpu_vm.c
@@ -0,0 +1,50 @@
+/*
+ * Copyright 2017 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+
+#include "amdgpu.h"
+#include "amdgpu_drm.h"
+#include "xf86drm.h"
+#include "amdgpu_internal.h"
+
+int amdgpu_vm_reserve_vmid(amdgpu_context_handle context, uint32_t flags)
+{
+   union drm_amdgpu_vm vm;
+
+   vm.in.op = AMDGPU_VM_OP_RESERVE_VMID;
+   vm.in.flags = flags;
+
+   return drmCommandWriteRead(context->dev->fd, DRM_AMDGPU_VM,
+  , sizeof(vm));
+}
+
+int amdgpu_vm_unreserve_vmid(amdgpu_context_handle context, uint32_t flags)
+{
+   union drm_amdgpu_vm vm;
+
+   vm.in.op = AMDGPU_VM_OP_UNRESERVE_VMID;
+   vm.in.flags = flags;
+
+   return drmCommandWriteRead(context->dev->fd, DRM_AMDGPU_VM,
+  , sizeof(vm));
+}
-- 
2.7.4

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

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

[Christian König]

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 

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

Signed-off-by: Christian König 
---
   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.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[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 
>>>
>>> Just set the bit so that DC does the hardware programming.
>>>
>>> Signed-off-by: Christian König 
>>> ---
>>>   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.


-- 
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 6/8] drm/amd/display: enable GPU VM support

[Tom St Denis]

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 

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

Signed-off-by: Christian König 
---
  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


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

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

[Christian König]

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 

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

Signed-off-by: Christian König 
---
  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



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

[PATCH 5/5] drm/amdgpu: allow framebuffer in GART memory as well

[Christian König]

From: Christian König 

On CZ and newer APUs we can pin the fb into GART as well as VRAM.

Signed-off-by: Christian König 
Reviewed-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   | 13 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.h   |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c| 10 ++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 +++-
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 6744e0c..71823f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -29,6 +29,7 @@
 #include "amdgpu_i2c.h"
 #include "atom.h"
 #include "amdgpu_connectors.h"
+#include "amdgpu_display.h"
 #include 
 
 #include 
@@ -188,7 +189,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
goto cleanup;
}
 
-   r = amdgpu_bo_pin(new_abo, AMDGPU_GEM_DOMAIN_VRAM, );
+   r = amdgpu_bo_pin(new_abo, amdgpu_framebuffer_domains(adev), );
if (unlikely(r != 0)) {
DRM_ERROR("failed to pin new abo buffer before flip\n");
goto unreserve;
@@ -501,6 +502,16 @@ static const struct drm_framebuffer_funcs amdgpu_fb_funcs 
= {
.create_handle = amdgpu_user_framebuffer_create_handle,
 };
 
+uint32_t amdgpu_framebuffer_domains(struct amdgpu_device *adev)
+{
+   uint32_t domain = AMDGPU_GEM_DOMAIN_VRAM;
+
+   if (adev->asic_type >= CHIP_CARRIZO && adev->flags & AMD_IS_APU)
+   domain |= AMDGPU_GEM_DOMAIN_GTT;
+
+   return domain;
+}
+
 int
 amdgpu_framebuffer_init(struct drm_device *dev,
struct amdgpu_framebuffer *rfb,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
index 11ae4ab..f241949 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
@@ -23,6 +23,7 @@
 #ifndef __AMDGPU_DISPLAY_H__
 #define __AMDGPU_DISPLAY_H__
 
+uint32_t amdgpu_framebuffer_domains(struct amdgpu_device *adev);
 struct drm_framebuffer *
 amdgpu_user_framebuffer_create(struct drm_device *dev,
   struct drm_file *file_priv,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index 90fa8e8..9be3228 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -38,6 +38,8 @@
 
 #include 
 
+#include "amdgpu_display.h"
+
 /* object hierarchy -
this contains a helper + a amdgpu fb
the helper contains a pointer to amdgpu framebuffer baseclass.
@@ -124,7 +126,7 @@ static int amdgpufb_create_pinned_object(struct 
amdgpu_fbdev *rfbdev,
struct drm_gem_object *gobj = NULL;
struct amdgpu_bo *abo = NULL;
bool fb_tiled = false; /* useful for testing */
-   u32 tiling_flags = 0;
+   u32 tiling_flags = 0, domain;
int ret;
int aligned_size, size;
int height = mode_cmd->height;
@@ -135,12 +137,12 @@ static int amdgpufb_create_pinned_object(struct 
amdgpu_fbdev *rfbdev,
/* need to align pitch with crtc limits */
mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
  fb_tiled);
+   domain = amdgpu_framebuffer_domains(adev);
 
height = ALIGN(mode_cmd->height, 8);
size = mode_cmd->pitches[0] * height;
aligned_size = ALIGN(size, PAGE_SIZE);
-   ret = amdgpu_gem_object_create(adev, aligned_size, 0,
-  AMDGPU_GEM_DOMAIN_VRAM,
+   ret = amdgpu_gem_object_create(adev, aligned_size, 0, domain,
   AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
   AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
   AMDGPU_GEM_CREATE_VRAM_CLEARED,
@@ -166,7 +168,7 @@ static int amdgpufb_create_pinned_object(struct 
amdgpu_fbdev *rfbdev,
}
 
 
-   ret = amdgpu_bo_pin(abo, AMDGPU_GEM_DOMAIN_VRAM, NULL);
+   ret = amdgpu_bo_pin(abo, domain, NULL);
if (ret) {
amdgpu_bo_unreserve(abo);
goto out_unref;
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 698cce0..01c44a8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2934,13 +2934,15 @@ static const struct drm_plane_funcs dm_plane_funcs = {
 static int dm_plane_helper_prepare_fb(struct drm_plane *plane,
  struct drm_plane_state *new_state)
 {
+   struct dm_plane_state *dm_plane_state_new, *dm_plane_state_old;
struct amdgpu_framebuffer *afb;
struct drm_gem_object 

[PATCH 2/5] drm/amdgpu: move GART recovery into GTT manager

[Christian König]

From: Christian König 

The GTT manager handles the GART address space anyway, so it is
completely pointless to keep the same information around twice.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h |  3 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |  8 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 53 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 51 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  3 +-
 5 files changed, 59 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ba1ab97..d39eeb9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1633,9 +1633,6 @@ struct amdgpu_device {
/* link all shadow bo */
struct list_headshadow_list;
struct mutexshadow_list_lock;
-   /* link all gtt */
-   spinlock_t  gtt_list_lock;
-   struct list_headgtt_list;
/* keep an lru list of rings by HW IP */
struct list_headring_lru_list;
spinlock_t  ring_lru_list_lock;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 400dfaa..d181d93 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2177,9 +2177,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
INIT_LIST_HEAD(>shadow_list);
mutex_init(>shadow_list_lock);
 
-   INIT_LIST_HEAD(>gtt_list);
-   spin_lock_init(>gtt_list_lock);
-
INIT_LIST_HEAD(>ring_lru_list);
spin_lock_init(>ring_lru_list_lock);
 
@@ -2893,7 +2890,7 @@ int amdgpu_sriov_gpu_reset(struct amdgpu_device *adev, 
struct amdgpu_job *job)
amdgpu_sriov_reinit_early(adev);
 
/* we need recover gart prior to run SMC/CP/SDMA resume */
-   amdgpu_ttm_recover_gart(adev);
+   amdgpu_gtt_mgr_recover(>mman.bdev.man[TTM_PL_TT]);
 
/* now we are okay to resume SMC/CP/SDMA */
amdgpu_sriov_reinit_late(adev);
@@ -3034,7 +3031,8 @@ int amdgpu_gpu_reset(struct amdgpu_device *adev)
DRM_ERROR("VRAM is lost!\n");
atomic_inc(>vram_lost_counter);
}
-   r = amdgpu_ttm_recover_gart(adev);
+   r = amdgpu_gtt_mgr_recover(
+   >mman.bdev.man[TTM_PL_TT]);
if (r)
goto out;
r = amdgpu_resume_phase2(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 29c5c3e..07ca4b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -31,6 +31,11 @@ struct amdgpu_gtt_mgr {
atomic64_t available;
 };
 
+struct amdgpu_gtt_node {
+   struct drm_mm_node node;
+   struct ttm_buffer_object *tbo;
+};
+
 /**
  * amdgpu_gtt_mgr_init - init GTT manager and DRM MM
  *
@@ -93,9 +98,9 @@ static int amdgpu_gtt_mgr_fini(struct ttm_mem_type_manager 
*man)
  */
 bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_mem_reg *mem)
 {
-   struct drm_mm_node *node = mem->mm_node;
+   struct amdgpu_gtt_node *node = mem->mm_node;
 
-   return (node->start != AMDGPU_BO_INVALID_OFFSET);
+   return (node->node.start != AMDGPU_BO_INVALID_OFFSET);
 }
 
 /**
@@ -115,7 +120,7 @@ static int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager 
*man,
 {
struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev);
struct amdgpu_gtt_mgr *mgr = man->priv;
-   struct drm_mm_node *node = mem->mm_node;
+   struct amdgpu_gtt_node *node = mem->mm_node;
enum drm_mm_insert_mode mode;
unsigned long fpfn, lpfn;
int r;
@@ -138,13 +143,13 @@ static int amdgpu_gtt_mgr_alloc(struct 
ttm_mem_type_manager *man,
mode = DRM_MM_INSERT_HIGH;
 
spin_lock(>lock);
-   r = drm_mm_insert_node_in_range(>mm, node,
-   mem->num_pages, mem->page_alignment, 0,
-   fpfn, lpfn, mode);
+   r = drm_mm_insert_node_in_range(>mm, >node, mem->num_pages,
+   mem->page_alignment, 0, fpfn, lpfn,
+   mode);
spin_unlock(>lock);
 
if (!r)
-   mem->start = node->start;
+   mem->start = node->node.start;
 
return r;
 }
@@ -165,7 +170,7 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager 
*man,
  struct ttm_mem_reg *mem)
 {
struct amdgpu_gtt_mgr *mgr = man->priv;
-   struct drm_mm_node *node;
+   struct amdgpu_gtt_node *node;

[PATCH 4/5] drm/amdgpu: don't use ttm_bo_move_ttm in amdgpu_ttm_bind v2

[Christian König]

From: Christian König 

Just allocate the GART space and fill it.

This prevents forcing the BO to be idle.

v2: don't unbind/bind at all, just fill the allocated GART space

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index eead348..06871a9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -878,10 +878,12 @@ static int amdgpu_ttm_backend_bind(struct ttm_tt *ttm,
 int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
 {
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
+   struct amdgpu_ttm_tt *gtt = (void*)bo->ttm;
struct ttm_mem_reg tmp;
 
struct ttm_placement placement;
struct ttm_place placements;
+   uint64_t flags;
int r;
 
if (bo->mem.mem_type != TTM_PL_TT ||
@@ -903,14 +905,21 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
if (unlikely(r))
return r;
 
-   r = ttm_bo_move_ttm(bo, true, false, );
-   if (unlikely(r))
+   flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, );
+   gtt->offset = (u64)tmp.start << PAGE_SHIFT;
+   r = amdgpu_gart_bind(adev, gtt->offset, bo->ttm->num_pages,
+bo->ttm->pages, gtt->ttm.dma_address, flags);
+   if (unlikely(r)) {
ttm_bo_mem_put(bo, );
-   else
-   bo->offset = (bo->mem.start << PAGE_SHIFT) +
-   bo->bdev->man[bo->mem.mem_type].gpu_offset;
+   return r;
+   }
 
-   return r;
+   ttm_bo_mem_put(bo, >mem);
+   bo->mem = tmp;
+   bo->offset = (bo->mem.start << PAGE_SHIFT) +
+   bo->bdev->man[bo->mem.mem_type].gpu_offset;
+
+   return 0;
 }
 
 int amdgpu_ttm_recover_gart(struct ttm_buffer_object *tbo)
-- 
2.7.4

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

[PATCH 3/5] drm/amdgpu: rename amdgpu_ttm_bind to amdgpu_ttm_alloc_gart

[Christian König]

From: Christian König 

We actually don't bind here, but rather allocate GART space if necessary.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h| 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 67fb15f..354c874 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -681,7 +681,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
if (!r && p->uf_entry.robj) {
struct amdgpu_bo *uf = p->uf_entry.robj;
 
-   r = amdgpu_ttm_bind(>tbo);
+   r = amdgpu_ttm_alloc_gart(>tbo);
p->job->uf_addr += amdgpu_bo_gpu_offset(uf);
}
 
@@ -1596,5 +1596,5 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser 
*parser,
return r;
}
 
-   return amdgpu_ttm_bind(&(*bo)->tbo);
+   return amdgpu_ttm_alloc_gart(&(*bo)->tbo);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index e44b880..7d03398 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -688,7 +688,7 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 
domain,
goto error;
}
 
-   r = amdgpu_ttm_bind(>tbo);
+   r = amdgpu_ttm_alloc_gart(>tbo);
if (unlikely(r)) {
dev_err(adev->dev, "%p bind failed\n", bo);
goto error;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c7ccd6f..eead348 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -875,7 +875,7 @@ static int amdgpu_ttm_backend_bind(struct ttm_tt *ttm,
return r;
 }
 
-int amdgpu_ttm_bind(struct ttm_buffer_object *bo)
+int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
 {
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
struct ttm_mem_reg tmp;
@@ -1605,7 +1605,7 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
}
 
if (bo->tbo.mem.mem_type == TTM_PL_TT) {
-   r = amdgpu_ttm_bind(>tbo);
+   r = amdgpu_ttm_alloc_gart(>tbo);
if (r)
return r;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index d2985de..4f9433e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -91,7 +91,7 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
struct dma_fence **fence);
 
 int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
-int amdgpu_ttm_bind(struct ttm_buffer_object *bo);
+int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo);
 int amdgpu_ttm_recover_gart(struct ttm_buffer_object *tbo);
 
 int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages);
-- 
2.7.4

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

[PATCH 1/5] drm/amdgpu: nuke amdgpu_ttm_is_bound() v2

[Christian König]

From: Christian König 

Rename amdgpu_gtt_mgr_is_allocated() to amdgpu_gtt_mgr_has_gart_addr() and use
that instead.

v2: rename the function as well.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c |  6 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 24 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  3 +--
 5 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 33535d3..29c5c3e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -85,13 +85,13 @@ static int amdgpu_gtt_mgr_fini(struct ttm_mem_type_manager 
*man)
 }
 
 /**
- * amdgpu_gtt_mgr_is_allocated - Check if mem has address space
+ * amdgpu_gtt_mgr_has_gart_addr - Check if mem has address space
  *
  * @mem: the mem object to check
  *
  * Check if a mem object has already address space allocated.
  */
-bool amdgpu_gtt_mgr_is_allocated(struct ttm_mem_reg *mem)
+bool amdgpu_gtt_mgr_has_gart_addr(struct ttm_mem_reg *mem)
 {
struct drm_mm_node *node = mem->mm_node;
 
@@ -120,7 +120,7 @@ static int amdgpu_gtt_mgr_alloc(struct ttm_mem_type_manager 
*man,
unsigned long fpfn, lpfn;
int r;
 
-   if (amdgpu_gtt_mgr_is_allocated(mem))
+   if (amdgpu_gtt_mgr_has_gart_addr(mem))
return 0;
 
if (place)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index b3351dc21..e44b880 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -982,7 +982,7 @@ u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo)
 {
WARN_ON_ONCE(bo->tbo.mem.mem_type == TTM_PL_SYSTEM);
WARN_ON_ONCE(bo->tbo.mem.mem_type == TTM_PL_TT &&
-!amdgpu_ttm_is_bound(bo->tbo.ttm));
+!amdgpu_gtt_mgr_has_gart_addr(>tbo.mem));
WARN_ON_ONCE(!ww_mutex_is_locked(>tbo.resv->lock) &&
 !bo->pin_count);
WARN_ON_ONCE(bo->tbo.mem.start == AMDGPU_BO_INVALID_OFFSET);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 428aae04..33615e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -187,7 +187,7 @@ static inline u64 amdgpu_bo_mmap_offset(struct amdgpu_bo 
*bo)
 static inline bool amdgpu_bo_gpu_accessible(struct amdgpu_bo *bo)
 {
switch (bo->tbo.mem.mem_type) {
-   case TTM_PL_TT: return amdgpu_ttm_is_bound(bo->tbo.ttm);
+   case TTM_PL_TT: return amdgpu_gtt_mgr_has_gart_addr(>tbo.mem);
case TTM_PL_VRAM: return true;
default: return false;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 52cab7a..632bfe3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -282,8 +282,7 @@ static uint64_t amdgpu_mm_node_addr(struct 
ttm_buffer_object *bo,
 {
uint64_t addr = 0;
 
-   if (mem->mem_type != TTM_PL_TT ||
-   amdgpu_gtt_mgr_is_allocated(mem)) {
+   if (mem->mem_type != TTM_PL_TT || amdgpu_gtt_mgr_has_gart_addr(mem)) {
addr = mm_node->start << PAGE_SHIFT;
addr += bo->bdev->man[mem->mem_type].gpu_offset;
}
@@ -369,7 +368,7 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
 * dst to window 1
 */
if (src->mem->mem_type == TTM_PL_TT &&
-   !amdgpu_gtt_mgr_is_allocated(src->mem)) {
+   !amdgpu_gtt_mgr_has_gart_addr(src->mem)) {
r = amdgpu_map_buffer(src->bo, src->mem,
PFN_UP(cur_size + src_page_offset),
src_node_start, 0, ring,
@@ -383,7 +382,7 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
}
 
if (dst->mem->mem_type == TTM_PL_TT &&
-   !amdgpu_gtt_mgr_is_allocated(dst->mem)) {
+   !amdgpu_gtt_mgr_has_gart_addr(dst->mem)) {
r = amdgpu_map_buffer(dst->bo, dst->mem,
PFN_UP(cur_size + dst_page_offset),
dst_node_start, 1, ring,
@@ -861,8 +860,10 @@ static int amdgpu_ttm_backend_bind(struct ttm_tt *ttm,
bo_mem->mem_type == AMDGPU_PL_OA)
return -EINVAL;
 
-   if (!amdgpu_gtt_mgr_is_allocated(bo_mem))
+   if (!amdgpu_gtt_mgr_has_gart_addr(bo_mem)) {
+   gtt->offset = AMDGPU_BO_INVALID_OFFSET;
return 0;
+   }
 
spin_lock(>adev->gtt_list_lock);

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

[Harry Wentland]

On 2017-10-26 12:06 PM, Christian König wrote:
> From: Christian König 
> 
> Just set the bit so that DC does the hardware programming.
> 
> Signed-off-by: Christian König 
> ---
>  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
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amd/display: Don't print error when bo_pin is interrupted

[Harry Wentland]

Hi Christian,

thanks for clarifying the semantics. I wasn't fully clear on that. What you say 
makes sense. I'll respin the patch.

Harry

On 2017-10-27 03:46 AM, Christian König wrote:
> Hi Harry,
>
> actually it's best practice that you only ignore ERESTARTSYS here, see other 
> code in the driver as well.
>
> EINTR means that the IOCTL was interrupted and can't be restarted because of 
> some problem.
>
> EAGAIN mean the we can't do this operation right now, but might be able to do 
> it at some point in the future.
>
> Both sound like a good idea to note to the user.
>
> Regards,
> Christian.
>
> Am 26.10.2017 um 22:16 schrieb Andrey Grodzovsky:
>>
>>
>>
>> On 2017-10-26 04:08 PM, Harry Wentland wrote:
>>> v2: Also don't print for ERESTARTSYS or EAGAIN
>>>
>>> Signed-off-by: Harry Wentland 
>>
>> Reviewed-by: Andrey Grodzovsky 
>>
>> Thanks,
>> Andrey
>>
>>> ---
>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> 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 cf15701f208d..4401f0fb3f02 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -2944,7 +2944,8 @@ static int dm_plane_helper_prepare_fb(struct 
>>> drm_plane *plane,
>>> amdgpu_bo_unreserve(rbo);
>>>  
>>> if (unlikely(r != 0)) {
>>> -   DRM_ERROR("Failed to pin framebuffer\n");
>>> +   if (!(r == -EINTR || r == -ERESTARTSYS || r == EAGAIN))
>>> +   DRM_ERROR("Failed to pin framebuffer with error %d\n", 
>>> r);
>>> return r;
>>> }
>>>  
>>
>

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

Re: [PATCH libdrm 1/2] amdgpu: Add wrappers for AMDGPU_VM IOCTL.

[Andrey Grodzovsky]

On 10/27/2017 04:52 AM, Emil Velikov wrote:

On 27 October 2017 at 01:15, Andrey Grodzovsky
 wrote:

Change-Id: I7eafb85c1ca96d6d255f0183bed0ce4129746fe0
Signed-off-by: Andrey Grodzovsky 
---
  amdgpu/Makefile.sources |  1 +
  amdgpu/amdgpu.h | 20 +++
  amdgpu/amdgpu_vm.c  | 52 +
  3 files changed, 73 insertions(+)
  create mode 100644 amdgpu/amdgpu_vm.c

diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
index bc3abaa..498b64c 100644
--- a/amdgpu/Makefile.sources
+++ b/amdgpu/Makefile.sources
@@ -6,6 +6,7 @@ LIBDRM_AMDGPU_FILES := \
 amdgpu_gpu_info.c \
 amdgpu_internal.h \
 amdgpu_vamgr.c \
+   amdgpu_vm.c \
 util_hash.c \
 util_hash.h \
 util_hash_table.c \
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index ecc975f..07f2851 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -1489,6 +1489,26 @@ void amdgpu_cs_chunk_fence_to_dep(struct amdgpu_cs_fence 
*fence,
  void amdgpu_cs_chunk_fence_info_to_data(struct amdgpu_cs_fence_info 
*fence_info,
 struct drm_amdgpu_cs_chunk_data *data);

+/**
+ * Reserve VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_alloc_reserved_vmid(amdgpu_context_handle context,
+ uint32_t  
flags);
+
+/**
+ * Free reserved VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_free_reserved_vmid(amdgpu_context_handle context,
+uint32_t   
flags);
+

Andrey, don't forget to run make check. You'll see it flag a lovely error ;-)


Thanks, didn't know about the check option, will fix.

Andrey



-Emil


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

RE: [PATCH] drm/amd/display: check if modeset is required before adding plane

[Deucher, Alexander]

> -Original Message-
> From: S, Shirish
> Sent: Thursday, October 26, 2017 11:26 PM
> To: amd-gfx@lists.freedesktop.org; Wentland, Harry; Deucher, Alexander
> Subject: [PATCH] drm/amd/display: check if modeset is required before
> adding plane
> 
> 
> From: Shirish S 
> 
> Adding affected planes without checking if modeset is requested from the
> user space causes performance regression in video p/b scenarios when full
> screen p/b is not composited.
> 
> Hence add a check before adding a plane as affected.
> 
> bug: https://bugs.freedesktop.org/show_bug.cgi?id=103408
> 
> Signed-off-by: Shirish S 

Acked-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++
>  1 file changed, 3 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 f0b50d9..e6ec130 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4727,6 +4727,9 @@ static int amdgpu_dm_atomic_check(struct
> drm_device *dev,
>   }
>   } else {
>   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> + if
> (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> + continue;
> +
>   if (!new_crtc_state->enable)
>   continue;
> 
> --
> 2.7.4

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

Re: display warnings due to recent commit

[Christian König]

Looks like the DC code can't correctly take care of the difference 
between CZ and Tonga.


Harry how should we handle this? Either not set the bit on older 
hardware or fix DC to just ignore it there?


To be honest I would prefer not to set a bit at all and just always 
program the relevant bits on hardware which supports this.


Regards,
Christian.

Am 27.10.2017 um 13:25 schrieb Tom St Denis:

The following commit:

[root@fx6 linux]# git bisect good
aa7187c5c640559ebc02caa0191c0db46b55b4a6 is the first bad commit
commit aa7187c5c640559ebc02caa0191c0db46b55b4a6
Author: Christian König 
Date:   Thu Oct 26 12:00:36 2017 +0200

    drm/amd/display: enable GPU VM support

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

    Signed-off-by: Christian König 
    Reviewed-by: Andrey Grodzovsky 

:04 04 c6d725acce6507b6d9381667d828e8e0d4683af7 
2dfcfab32ef3e7d24a31eb420dd1186cf973cd7d M  drivers


Results in the (attached) dmesg logs on my Tonga system.

Cheers,
Tom


___
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

display warnings due to recent commit

[Tom St Denis]

The following commit:

[root@fx6 linux]# git bisect good
aa7187c5c640559ebc02caa0191c0db46b55b4a6 is the first bad commit
commit aa7187c5c640559ebc02caa0191c0db46b55b4a6
Author: Christian König 
Date:   Thu Oct 26 12:00:36 2017 +0200

drm/amd/display: enable GPU VM support

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

Signed-off-by: Christian König 
Reviewed-by: Andrey Grodzovsky 

:04 04 c6d725acce6507b6d9381667d828e8e0d4683af7 
2dfcfab32ef3e7d24a31eb420dd1186cf973cd7d M  drivers


Results in the (attached) dmesg logs on my Tonga system.

Cheers,
Tom


tonga.dmesg.gz
Description: application/gzip
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/2] amdgpu: Add wrappers for AMDGPU_VM IOCTL.

[Emil Velikov]

On 27 October 2017 at 01:15, Andrey Grodzovsky
 wrote:
> Change-Id: I7eafb85c1ca96d6d255f0183bed0ce4129746fe0
> Signed-off-by: Andrey Grodzovsky 
> ---
>  amdgpu/Makefile.sources |  1 +
>  amdgpu/amdgpu.h | 20 +++
>  amdgpu/amdgpu_vm.c  | 52 
> +
>  3 files changed, 73 insertions(+)
>  create mode 100644 amdgpu/amdgpu_vm.c
>
> diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
> index bc3abaa..498b64c 100644
> --- a/amdgpu/Makefile.sources
> +++ b/amdgpu/Makefile.sources
> @@ -6,6 +6,7 @@ LIBDRM_AMDGPU_FILES := \
> amdgpu_gpu_info.c \
> amdgpu_internal.h \
> amdgpu_vamgr.c \
> +   amdgpu_vm.c \
> util_hash.c \
> util_hash.h \
> util_hash_table.c \
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index ecc975f..07f2851 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -1489,6 +1489,26 @@ void amdgpu_cs_chunk_fence_to_dep(struct 
> amdgpu_cs_fence *fence,
>  void amdgpu_cs_chunk_fence_info_to_data(struct amdgpu_cs_fence_info 
> *fence_info,
> struct drm_amdgpu_cs_chunk_data 
> *data);
>
> +/**
> + * Reserve VMID
> + * \param   context - \c [in]  GPU Context
> + * \param   flags - \c [in]  TBD
> + *
> + * \return  0 on success otherwise POSIX Error code
> +*/
> +int amdgpu_vm_alloc_reserved_vmid(amdgpu_context_handle context,
> + uint32_t
>   flags);
> +
> +/**
> + * Free reserved VMID
> + * \param   context - \c [in]  GPU Context
> + * \param   flags - \c [in]  TBD
> + *
> + * \return  0 on success otherwise POSIX Error code
> +*/
> +int amdgpu_vm_free_reserved_vmid(amdgpu_context_handle context,
> +uint32_t 
>   flags);
> +
Andrey, don't forget to run make check. You'll see it flag a lovely error ;-)

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

Re: [PATCH v2] drm/amd/display: Don't print error when bo_pin is interrupted

[Christian König]

Hi Harry,

actually it's best practice that you only ignore ERESTARTSYS here, see 
other code in the driver as well.


EINTR means that the IOCTL was interrupted and can't be restarted 
because of some problem.


EAGAIN mean the we can't do this operation right now, but might be able 
to do it at some point in the future.


Both sound like a good idea to note to the user.

Regards,
Christian.

Am 26.10.2017 um 22:16 schrieb Andrey Grodzovsky:




On 2017-10-26 04:08 PM, Harry Wentland wrote:

v2: Also don't print for ERESTARTSYS or EAGAIN

Signed-off-by: Harry Wentland


Reviewed-by: Andrey Grodzovsky

Thanks,
Andrey


---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

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 cf15701f208d..4401f0fb3f02 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2944,7 +2944,8 @@ static int dm_plane_helper_prepare_fb(struct drm_plane 
*plane,
amdgpu_bo_unreserve(rbo);
  
  	if (unlikely(r != 0)) {

-   DRM_ERROR("Failed to pin framebuffer\n");
+   if (!(r == -EINTR || r == -ERESTARTSYS || r == EAGAIN))
+   DRM_ERROR("Failed to pin framebuffer with error %d\n", 
r);
return r;
}
  




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

Re: [PATCH libdrm 1/2] amdgpu: Add wrappers for AMDGPU_VM IOCTL.

[Christian König]

Am 27.10.2017 um 02:15 schrieb Andrey Grodzovsky:

Change-Id: I7eafb85c1ca96d6d255f0183bed0ce4129746fe0
Signed-off-by: Andrey Grodzovsky 
---
  amdgpu/Makefile.sources |  1 +
  amdgpu/amdgpu.h | 20 +++
  amdgpu/amdgpu_vm.c  | 52 +
  3 files changed, 73 insertions(+)
  create mode 100644 amdgpu/amdgpu_vm.c

diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
index bc3abaa..498b64c 100644
--- a/amdgpu/Makefile.sources
+++ b/amdgpu/Makefile.sources
@@ -6,6 +6,7 @@ LIBDRM_AMDGPU_FILES := \
amdgpu_gpu_info.c \
amdgpu_internal.h \
amdgpu_vamgr.c \
+   amdgpu_vm.c \
util_hash.c \
util_hash.h \
util_hash_table.c \
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index ecc975f..07f2851 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -1489,6 +1489,26 @@ void amdgpu_cs_chunk_fence_to_dep(struct amdgpu_cs_fence 
*fence,
  void amdgpu_cs_chunk_fence_info_to_data(struct amdgpu_cs_fence_info 
*fence_info,
struct drm_amdgpu_cs_chunk_data *data);
  
+/**

+ * Reserve VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_alloc_reserved_vmid(amdgpu_context_handle context,
+ uint32_t  
flags);
+
+/**
+ * Free reserved VMID
+ * \param   context - \c [in]  GPU Context
+ * \param   flags - \c [in]  TBD
+ *
+ * \return  0 on success otherwise POSIX Error code
+*/
+int amdgpu_vm_free_reserved_vmid(amdgpu_context_handle context,
+uint32_t   
flags);
+
  #ifdef __cplusplus
  }
  #endif
diff --git a/amdgpu/amdgpu_vm.c b/amdgpu/amdgpu_vm.c
new file mode 100644
index 000..1664b7b
--- /dev/null
+++ b/amdgpu/amdgpu_vm.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright 2017 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+
+#include "amdgpu.h"
+#include "amdgpu_drm.h"
+#include "xf86drm.h"
+#include "amdgpu_internal.h"
+
+int amdgpu_vm_alloc_reserved_vmid(amdgpu_context_handle context,
+ uint32_t  
flags)


Could just be the mail client, but flags looks way to much indented to 
the right.


Additional to that I would keep the name of the kernel IOCTL for the 
function name.


E.g. amdgpu_vm_reserve_vmid() and amdgpu_vm_unreserve_vmid().

Apart from that both patches look good to me.

Christian.


+{
+   union drm_amdgpu_vm vm;
+
+   vm.in.op = AMDGPU_VM_OP_RESERVE_VMID;
+   vm.in.flags = flags;
+
+   return drmCommandWriteRead(context->dev->fd, DRM_AMDGPU_VM,
+  , sizeof(vm));
+}
+
+int amdgpu_vm_free_reserved_vmid(amdgpu_context_handle context,
+uint32_t   
flags)
+{
+   union drm_amdgpu_vm vm;
+
+   vm.in.op = AMDGPU_VM_OP_UNRESERVE_VMID;
+   vm.in.flags = flags;
+
+   return drmCommandWriteRead(context->dev->fd, DRM_AMDGPU_VM,
+  , sizeof(vm));
+}



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

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Christian König]

Am 27.10.2017 um 09:22 schrieb Christian König:

Am 26.10.2017 um 20:54 schrieb Felix Kuehling:

On 2017-10-26 02:11 PM, Christian König wrote:
But now reading the patch there is something else which I stumbled 
over:

- WARN_ON(atomic_read(>mm->mm_count) <= 0);
+    /*
+ * This cast should be safe here because we grabbed a
+ * reference to the mm in kfd_process_notifier_release
+ */
+    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 
0);

      mmdrop(p->mm);

Well that isn't good coding style. You shouldn't obfuscate what
pointer it is by changing it to "void*", but rather set it to NULL as
soon as you know that it is stale.

Additional to that it is certainly not job of the driver to warn on a
run over mm_count.

Yeah. We don't have this in our current staging branch. The whole
process teardown has changed quite a bit. I just fixed this up to make
it work with current upstream.

If you prefer, I could just remove the WARN_ON.


That sounds like a good idea to me, yes.


Wait a second, now that I though once more about it what do you mean 
with this comment:

+    /*
+ * Opaque pointer to mm_struct. We don't hold a reference to
+ * it so it should never be dereferenced from here. This is
+ * only used for looking up processes by their mm.
+ */ 

E.g. what means looking up the processes by their mm?

Do you use a hashtable or something like this and then check if you got 
the correct mm structure by comparing the pointers?


If that is the case then you should certainly set p->mm to NULL after 
mmdrop(p->mm).


Otherwise it can happen that a new mm structure is allocated at the same 
location as the old one and you run into problems because the kernel 
driver accidentally uses the wrong kfd_process structure.


Regards,
Christian.



Regards,
Christian.



Regards,
   Felix


Regards,
Christian.


Regards,
    Felix


Regards,
Christian.


Signed-off-by: Felix Kuehling 
---
    drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 
+++

    drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++-
    drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +-
    3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 944abfa..61ce547 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -24,8 +24,8 @@
    #include 
    #include 
    #include 
+#include 
    #include 
-#include 
    #include 
    #include 
    #include "kfd_priv.h"
@@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
*dev, unsigned int pasid,
 * running so the lookup function returns a locked process.
 */
    struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
+    struct mm_struct *mm;
      if (!p)
    return; /* Presumably process exited. */
    +    /* Take a safe reference to the mm_struct, which may 
otherwise

+ * disappear even while the kfd_process is still referenced.
+ */
+    mm = get_task_mm(p->lead_thread);
+    if (!mm) {
+    mutex_unlock(>mutex);
+    return; /* Process is exiting */
+    }
+
    memset(_exception_data, 0,
sizeof(memory_exception_data));
    -    down_read(>mm->mmap_sem);
-    vma = find_vma(p->mm, address);
+    down_read(>mmap_sem);
+    vma = find_vma(mm, address);
      memory_exception_data.gpu_id = dev->id;
    memory_exception_data.va = address;
@@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
unsigned int pasid,
    }
    }
    -    up_read(>mm->mmap_sem);
+    up_read(>mmap_sem);
+    mmput(mm);
      mutex_lock(>event_mutex);
    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7d86ec9..1a483a7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -494,7 +494,12 @@ struct kfd_process {
 */
    struct hlist_node kfd_processes;
    -    struct mm_struct *mm;
+    /*
+ * Opaque pointer to mm_struct. We don't hold a reference to
+ * it so it should never be dereferenced from here. This is
+ * only used for looking up processes by their mm.
+ */
+    void *mm;
      struct mutex mutex;
    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 3ccb3b5..21d27e5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
rcu_head *rcu)
    struct kfd_process *p;
      p = container_of(rcu, struct kfd_process, rcu);
-    WARN_ON(atomic_read(>mm->mm_count) <= 0);
+    /*
+ * This cast should be safe here because we grabbed a
+ * reference to the mm in kfd_process_notifier_release
+ */
+    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <=
0);
      mmdrop(p->mm);

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Christian König]

Am 26.10.2017 um 20:54 schrieb Felix Kuehling:

On 2017-10-26 02:11 PM, Christian König wrote:

But now reading the patch there is something else which I stumbled over:

-    WARN_ON(atomic_read(>mm->mm_count) <= 0);
+    /*
+ * This cast should be safe here because we grabbed a
+ * reference to the mm in kfd_process_notifier_release
+ */
+    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
      mmdrop(p->mm);

Well that isn't good coding style. You shouldn't obfuscate what
pointer it is by changing it to "void*", but rather set it to NULL as
soon as you know that it is stale.

Additional to that it is certainly not job of the driver to warn on a
run over mm_count.

Yeah. We don't have this in our current staging branch. The whole
process teardown has changed quite a bit. I just fixed this up to make
it work with current upstream.

If you prefer, I could just remove the WARN_ON.


That sounds like a good idea to me, yes.

Regards,
Christian.



Regards,
   Felix


Regards,
Christian.


Regards,
    Felix


Regards,
Christian.


Signed-off-by: Felix Kuehling 
---
    drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++
    drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++-
    drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +-
    3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 944abfa..61ce547 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -24,8 +24,8 @@
    #include 
    #include 
    #include 
+#include 
    #include 
-#include 
    #include 
    #include 
    #include "kfd_priv.h"
@@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
*dev, unsigned int pasid,
     * running so the lookup function returns a locked process.
     */
    struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
+    struct mm_struct *mm;
      if (!p)
    return; /* Presumably process exited. */
    +    /* Take a safe reference to the mm_struct, which may otherwise
+ * disappear even while the kfd_process is still referenced.
+ */
+    mm = get_task_mm(p->lead_thread);
+    if (!mm) {
+    mutex_unlock(>mutex);
+    return; /* Process is exiting */
+    }
+
    memset(_exception_data, 0,
sizeof(memory_exception_data));
    -    down_read(>mm->mmap_sem);
-    vma = find_vma(p->mm, address);
+    down_read(>mmap_sem);
+    vma = find_vma(mm, address);
      memory_exception_data.gpu_id = dev->id;
    memory_exception_data.va = address;
@@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
unsigned int pasid,
    }
    }
    -    up_read(>mm->mmap_sem);
+    up_read(>mmap_sem);
+    mmput(mm);
      mutex_lock(>event_mutex);
    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7d86ec9..1a483a7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -494,7 +494,12 @@ struct kfd_process {
     */
    struct hlist_node kfd_processes;
    -    struct mm_struct *mm;
+    /*
+ * Opaque pointer to mm_struct. We don't hold a reference to
+ * it so it should never be dereferenced from here. This is
+ * only used for looking up processes by their mm.
+ */
+    void *mm;
      struct mutex mutex;
    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 3ccb3b5..21d27e5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
rcu_head *rcu)
    struct kfd_process *p;
      p = container_of(rcu, struct kfd_process, rcu);
-    WARN_ON(atomic_read(>mm->mm_count) <= 0);
+    /*
+ * This cast should be safe here because we grabbed a
+ * reference to the mm in kfd_process_notifier_release
+ */
+    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <=
0);
      mmdrop(p->mm);


___
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