[PATCH xf86-video-amdgpu 01/10] Remove drmmode_crtc->scanout_destroy[] array

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

No longer necessary since we're reference counting framebuffers.

(Ported from radeon commit 3f120fa1d5d921656a367751bc079e020e9ab105)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 22 --
 src/drmmode_display.h |  1 -
 2 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 552bff896..108fb6803 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -457,19 +457,10 @@ drmmode_crtc_scanout_destroy(drmmode_ptr drmmode,
 static void
 drmmode_crtc_scanout_free(drmmode_crtc_private_ptr drmmode_crtc)
 {
-   if (drmmode_crtc->flip_pending) {
-   drmmode_crtc->scanout_destroy[0] = drmmode_crtc->scanout[0];
-   drmmode_crtc->scanout[0].pixmap = NULL;
-   drmmode_crtc->scanout[0].bo = NULL;
-   drmmode_crtc->scanout_destroy[1] = drmmode_crtc->scanout[1];
-   drmmode_crtc->scanout[1].pixmap = NULL;
-   drmmode_crtc->scanout[1].bo = NULL;
-   } else {
-   drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
-_crtc->scanout[0]);
-   drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
-_crtc->scanout[1]);
-   }
+   drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
+_crtc->scanout[0]);
+   drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
+_crtc->scanout[1]);
 
if (drmmode_crtc->scanout_damage)
DamageDestroy(drmmode_crtc->scanout_damage);
@@ -2238,11 +2229,6 @@ drmmode_clear_pending_flip(xf86CrtcPtr crtc)
 
drmmode_crtc_dpms(crtc, drmmode_crtc->pending_dpms_mode);
}
-
-   drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
-_crtc->scanout_destroy[0]);
-   drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
-_crtc->scanout_destroy[1]);
 }
 
 static void
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index ca556f254..7071a748e 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -79,7 +79,6 @@ typedef struct {
struct amdgpu_buffer *cursor_buffer;
struct drmmode_scanout rotate;
struct drmmode_scanout scanout[2];
-   struct drmmode_scanout scanout_destroy[2];
DamagePtr scanout_damage;
RegionRec scanout_last_region;
unsigned scanout_id;
-- 
2.14.1

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


[PATCH xf86-video-amdgpu 05/10] Wait for pending flips synchronously before turning off a CRTC

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Allows removing drmmode_clear_pending_flip and the pending_dpms_mode
field and cleaning up the code considerably.

(Ported from radeon commit e6d7dc2070f4d21a6900916bb70a31839112882c)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c  | 12 ++-
 src/amdgpu_present.c  |  2 +-
 src/amdgpu_video.c|  2 +-
 src/drmmode_display.c | 60 ++-
 src/drmmode_display.h |  5 +
 5 files changed, 21 insertions(+), 60 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 3e2112578..d8f667df0 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -634,7 +634,7 @@ amdgpu_prime_scanout_update(PixmapDirtyUpdatePtr dirty)
drmmode_crtc = xf86_crtc->driver_private;
if (drmmode_crtc->scanout_update_pending ||
!drmmode_crtc->scanout[drmmode_crtc->scanout_id].pixmap ||
-   drmmode_crtc->pending_dpms_mode != DPMSModeOn)
+   drmmode_crtc->dpms_mode != DPMSModeOn)
return;
 
drm_queue_seq = amdgpu_drm_queue_alloc(xf86_crtc,
@@ -663,10 +663,12 @@ amdgpu_prime_scanout_update(PixmapDirtyUpdatePtr dirty)
 static void
 amdgpu_prime_scanout_flip_abort(xf86CrtcPtr crtc, void *event_data)
 {
+   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
drmmode_crtc_private_ptr drmmode_crtc = event_data;
 
drmmode_crtc->scanout_update_pending = FALSE;
-   drmmode_clear_pending_flip(crtc);
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
+NULL);
 }
 
 static void
@@ -698,7 +700,7 @@ amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
drmmode_crtc = crtc->driver_private;
if (drmmode_crtc->scanout_update_pending ||
!drmmode_crtc->scanout[drmmode_crtc->scanout_id].pixmap ||
-   drmmode_crtc->pending_dpms_mode != DPMSModeOn)
+   drmmode_crtc->dpms_mode != DPMSModeOn)
return;
 
scanout_id = drmmode_crtc->scanout_id ^ 1;
@@ -916,7 +918,7 @@ amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
 
if (!xf86_crtc->enabled ||
drmmode_crtc->scanout_update_pending ||
-   drmmode_crtc->pending_dpms_mode != DPMSModeOn)
+   drmmode_crtc->dpms_mode != DPMSModeOn)
return;
 
pDamage = drmmode_crtc->scanout_damage;
@@ -982,7 +984,7 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
unsigned scanout_id;
 
if (drmmode_crtc->scanout_update_pending ||
-   drmmode_crtc->pending_dpms_mode != DPMSModeOn)
+   drmmode_crtc->dpms_mode != DPMSModeOn)
return;
 
scanout_id = drmmode_crtc->scanout_id ^ 1;
diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c
index b6792f361..c7fc402cd 100644
--- a/src/amdgpu_present.c
+++ b/src/amdgpu_present.c
@@ -361,7 +361,7 @@ modeset:
if (!crtc->enabled)
continue;
 
-   if (drmmode_crtc->pending_dpms_mode == DPMSModeOn)
+   if (drmmode_crtc->dpms_mode == DPMSModeOn)
crtc->funcs->set_mode_major(crtc, >mode, 
crtc->rotation,
crtc->x, crtc->y);
else
diff --git a/src/amdgpu_video.c b/src/amdgpu_video.c
index 3f441e79a..8f9a2b977 100644
--- a/src/amdgpu_video.c
+++ b/src/amdgpu_video.c
@@ -67,7 +67,7 @@ static int amdgpu_box_area(BoxPtr box)
 Bool amdgpu_crtc_is_enabled(xf86CrtcPtr crtc)
 {
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-   return drmmode_crtc->pending_dpms_mode == DPMSModeOn;
+   return drmmode_crtc->dpms_mode == DPMSModeOn;
 }
 
 xf86CrtcPtr
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index bdd3866b8..0d5aa26d2 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -284,14 +284,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
CARD64 ust;
int ret;
 
-   drmmode_crtc->pending_dpms_mode = mode;
-
if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
uint32_t seq;
 
-   /* Wait for any pending flip to finish */
-   if (drmmode_crtc->flip_pending)
-   return;
+   drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
+   drmmode_crtc->flip_pending);
 
/*
 * On->Off transition: record the last vblank time,
@@ -345,10 +342,8 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
 
/* Disable unused CRTCs and enable/disable active CRTCs */
if (!crtc->enabled || mode != DPMSModeOn) {
-   /* Wait for any pending flip to finish */
-  

[PATCH xf86-video-amdgpu 08/10] Pass extents to amdgpu_scanout_do_update

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Preparation for following change, no functional change intended yet.

(Ported from radeon commit 65e0c5ea1b4adff21d673dbf54af99704c429627)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_drv.h  |  2 +-
 src/amdgpu_kms.c  | 41 +
 src/drmmode_display.c |  3 ++-
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/src/amdgpu_drv.h b/src/amdgpu_drv.h
index 515d46625..13237470a 100644
--- a/src/amdgpu_drv.h
+++ b/src/amdgpu_drv.h
@@ -341,7 +341,7 @@ Bool amdgpu_dri3_screen_init(ScreenPtr screen);
 
 /* amdgpu_kms.c */
 Bool amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id,
-  DrawablePtr src_draw);
+  DrawablePtr src_draw, BoxPtr extents);
 void AMDGPUWindowExposures_oneshot(WindowPtr pWin, RegionPtr pRegion
 #if XORG_VERSION_CURRENT < XORG_VERSION_NUMERIC(1,16,99,901,0)
   , RegionPtr pBSRegion
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 230a779fc..aedb3f066 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -793,32 +793,27 @@ amdgpu_dirty_update(ScrnInfoPtr scrn)
 
 Bool
 amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id,
- DrawablePtr src_draw)
+DrawablePtr src_draw, BoxPtr extents)
 {
drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
-   RegionPtr pRegion = DamageRegion(drmmode_crtc->scanout_damage);
+   RegionRec region = { .extents = *extents, .data = NULL };
ScrnInfoPtr scrn = xf86_crtc->scrn;
ScreenPtr pScreen = scrn->pScreen;
DrawablePtr pDraw;
-   BoxRec extents;
 
if (!xf86_crtc->enabled ||
-   !drmmode_crtc->scanout[scanout_id].pixmap)
-   return FALSE;
-
-   if (!RegionNotEmpty(pRegion))
+   !drmmode_crtc->scanout[scanout_id].pixmap ||
+   extents->x1 >= extents->x2 || extents->y1 >= extents->y2)
return FALSE;
 
pDraw = _crtc->scanout[scanout_id].pixmap->drawable;
-   extents = *RegionExtents(pRegion);
-   if (!amdgpu_scanout_extents_intersect(xf86_crtc, ))
+   if (!amdgpu_scanout_extents_intersect(xf86_crtc, extents))
return FALSE;
 
if (drmmode_crtc->tear_free) {
-   amdgpu_sync_scanout_pixmaps(xf86_crtc, pRegion, scanout_id);
-   RegionCopy(_crtc->scanout_last_region, pRegion);
+   amdgpu_sync_scanout_pixmaps(xf86_crtc, , scanout_id);
+   RegionCopy(_crtc->scanout_last_region, );
}
-   RegionEmpty(pRegion);
 
 #if XF86_CRTC_VERSION >= 4
if (xf86_crtc->driverIsPerformingTransform) {
@@ -856,9 +851,9 @@ amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int 
scanout_id,
pScreen->SourceValidate = NULL;
CompositePicture(PictOpSrc,
 src, NULL, dst,
-extents.x1, extents.y1, 0, 0, extents.x1,
-extents.y1, extents.x2 - extents.x1,
-extents.y2 - extents.y1);
+extents->x1, extents->y1, 0, 0, extents->x1,
+extents->y1, extents->x2 - extents->x1,
+extents->y2 - extents->y1);
pScreen->SourceValidate = SourceValidate;
 
  free_dst:
@@ -873,9 +868,9 @@ amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int 
scanout_id,
 
ValidateGC(pDraw, gc);
(*gc->ops->CopyArea)(src_draw, pDraw, gc,
-xf86_crtc->x + extents.x1, xf86_crtc->y + 
extents.y1,
-extents.x2 - extents.x1, extents.y2 - 
extents.y1,
-extents.x1, extents.y1);
+xf86_crtc->x + extents->x1, xf86_crtc->y + 
extents->y1,
+extents->x2 - extents->x1, extents->y2 - 
extents->y1,
+extents->x1, extents->y1);
FreeScratchGC(gc);
}
 
@@ -898,9 +893,12 @@ amdgpu_scanout_update_handler(xf86CrtcPtr crtc, uint32_t 
frame, uint64_t usec,
 {
drmmode_crtc_private_ptr drmmode_crtc = event_data;
ScreenPtr screen = crtc->scrn->pScreen;
+   RegionPtr region = DamageRegion(drmmode_crtc->scanout_damage);
 
amdgpu_scanout_do_update(crtc, drmmode_crtc->scanout_id,
-
>GetWindowPixmap(screen->root)->drawable);
+
>GetWindowPixmap(screen->root)->drawable,
+>extents);
+   RegionEmpty(region);
 
amdgpu_sca

[PATCH xf86-video-amdgpu 06/10] Handle multiple "pending" Present flips

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

The xserver Present code can submit a flip in response to notifying it
that a vblank event arrived. This can happen before the completion event
of the previous flip is processed. In that case, we were clearing the
drmmode_crtc->flip_pending field prematurely.

Prevent this by only clearing drmmode_crtc->flip_pending when it matches
the framebuffer being scanned out since the flip whose completion event
we're processing.

(Ported from radeon commit 7c10ee9c88378d773c0bcf651fdc5d9f2c6dc5e5)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 27 ---
 src/drmmode_display.h |  3 ++-
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 0d5aa26d2..eb701a89a 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2227,6 +2227,7 @@ drmmode_flip_abort(xf86CrtcPtr crtc, void *event_data)
if (!flipdata->fe_crtc)
flipdata->fe_crtc = crtc;
flipdata->abort(flipdata->fe_crtc, flipdata->event_data);
+   drmmode_fb_reference(pAMDGPUEnt->fd, >fb, NULL);
free(flipdata);
}
 
@@ -2248,6 +2249,13 @@ drmmode_flip_handler(xf86CrtcPtr crtc, uint32_t frame, 
uint64_t usec, void *even
flipdata->fe_usec = usec;
}
 
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->fb,
+flipdata->fb);
+   if (drmmode_crtc->flip_pending == flipdata->fb) {
+   drmmode_fb_reference(pAMDGPUEnt->fd,
+_crtc->flip_pending, NULL);
+   }
+
if (--flipdata->flip_count == 0) {
/* Deliver MSC & UST from reference/current CRTC to flip event
 * handler
@@ -2258,13 +2266,9 @@ drmmode_flip_handler(xf86CrtcPtr crtc, uint32_t frame, 
uint64_t usec, void *even
else
flipdata->handler(crtc, frame, usec, 
flipdata->event_data);
 
+   drmmode_fb_reference(pAMDGPUEnt->fd, >fb, NULL);
free(flipdata);
}
-
-   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->fb,
-drmmode_crtc->flip_pending);
-   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
-NULL);
 }
 
 #if HAVE_NOTIFY_FD
@@ -2761,7 +2765,6 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
uint32_t flip_flags = flip_sync == FLIP_ASYNC ? 
DRM_MODE_PAGE_FLIP_ASYNC : 0;
drmmode_flipdata_ptr flipdata;
uintptr_t drm_queue_seq = 0;
-   struct drmmode_fb *fb;
 
flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
if (!flipdata) {
@@ -2770,8 +2773,9 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
goto error;
}
 
-   fb = amdgpu_pixmap_get_fb(new_front);
-   if (!fb) {
+   drmmode_fb_reference(pAMDGPUEnt->fd, >fb,
+amdgpu_pixmap_get_fb(new_front));
+   if (!flipdata->fb) {
ErrorF("Failed to get FB for flip\n");
goto error;
}
@@ -2813,7 +2817,7 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
if (crtc == ref_crtc) {
if (drmmode_page_flip_target_absolute(pAMDGPUEnt,
  drmmode_crtc,
- fb->handle,
+ 
flipdata->fb->handle,
  flip_flags,
  drm_queue_seq,
  target_msc) != 0)
@@ -2821,14 +2825,14 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
} else {
if (drmmode_page_flip_target_relative(pAMDGPUEnt,
  drmmode_crtc,
- fb->handle,
+ 
flipdata->fb->handle,
  flip_flags,
  drm_queue_seq, 0) 
!= 0)
goto flip_error;
}
 
drmmode_fb_reference(pAMDGPUEnt->fd, 
_crtc->flip_pending,
-fb);
+flipdata->fb);
drm_queue_seq = 0;
}
 
@@ -2846,6 +2850,7 @@ error:
drmmode_flip_abort(crtc, flipdata);
else {
abort(NULL, data);
+ 

[PATCH xf86-video-amdgpu 09/10] Always allow Present page flipping with TearFree

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Even if TearFree is active for the the CRTC we're synchronizing to. In
that case, for Present flips synchronized to vertical blank, the other
scanout buffer is immediately synchronized and flipped to during the
target vertical blank period. For Present flips not synchronized to
vertical blank, we simply use the MSC and timestamp values of the last
vertical blank period for timing purposes, and let the normal TearFree
mechanism handle display updates.

(Ported from radeon commit 4445765af5b97d0cfd10889fe6d6f58f2ce85659)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c  | 27 ---
 src/amdgpu_present.c  | 75 +++
 src/drmmode_display.c | 61 -
 src/drmmode_display.h | 12 -
 4 files changed, 152 insertions(+), 23 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index aedb3f066..b0f1a3d94 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -41,6 +41,10 @@
 #include "shadow.h"
 #include 
 
+#if HAVE_PRESENT_H
+#include 
+#endif
+
 /* DPMS */
 #ifdef HAVE_XEXTPROTO_71
 #include 
@@ -681,6 +685,15 @@ amdgpu_prime_scanout_flip_handler(xf86CrtcPtr crtc, 
uint32_t msc, uint64_t usec,
drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->fb,
 drmmode_crtc->flip_pending);
amdgpu_prime_scanout_flip_abort(crtc, event_data);
+
+#ifdef HAVE_PRESENT_H
+   if (drmmode_crtc->present_vblank_event_id) {
+   present_event_notify(drmmode_crtc->present_vblank_event_id,
+drmmode_crtc->present_vblank_usec,
+drmmode_crtc->present_vblank_msc);
+   drmmode_crtc->present_vblank_event_id = 0;
+   }
+#endif
 }
 
 static void
@@ -895,10 +908,14 @@ amdgpu_scanout_update_handler(xf86CrtcPtr crtc, uint32_t 
frame, uint64_t usec,
ScreenPtr screen = crtc->scrn->pScreen;
RegionPtr region = DamageRegion(drmmode_crtc->scanout_damage);
 
-   amdgpu_scanout_do_update(crtc, drmmode_crtc->scanout_id,
-
>GetWindowPixmap(screen->root)->drawable,
->extents);
-   RegionEmpty(region);
+   if (crtc->enabled &&
+   !drmmode_crtc->flip_pending &&
+   drmmode_crtc->dpms_mode == DPMSModeOn) {
+   if (amdgpu_scanout_do_update(crtc, drmmode_crtc->scanout_id,
+
>GetWindowPixmap(screen->root)->drawable,
+>extents))
+   RegionEmpty(region);
+   }
 
amdgpu_scanout_update_abort(crtc, event_data);
 }
@@ -915,6 +932,7 @@ amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
 
if (!xf86_crtc->enabled ||
drmmode_crtc->scanout_update_pending ||
+   drmmode_crtc->flip_pending ||
drmmode_crtc->dpms_mode != DPMSModeOn)
return;
 
@@ -982,6 +1000,7 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
unsigned scanout_id;
 
if (drmmode_crtc->scanout_update_pending ||
+   drmmode_crtc->flip_pending ||
drmmode_crtc->dpms_mode != DPMSModeOn)
return;
 
diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c
index c7fc402cd..cef93c068 100644
--- a/src/amdgpu_present.c
+++ b/src/amdgpu_present.c
@@ -52,6 +52,7 @@ static present_screen_info_rec amdgpu_present_screen_info;
 
 struct amdgpu_present_vblank_event {
uint64_t event_id;
+   Bool vblank_for_flip;
Bool unflip;
 };
 
@@ -119,9 +120,26 @@ static void
 amdgpu_present_vblank_handler(xf86CrtcPtr crtc, unsigned int msc,
  uint64_t usec, void *data)
 {
+   drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
struct amdgpu_present_vblank_event *event = data;
 
-   present_event_notify(event->event_id, usec, msc);
+   if (event->vblank_for_flip &&
+   drmmode_crtc->tear_free &&
+   drmmode_crtc->scanout_update_pending) {
+   if (drmmode_crtc->present_vblank_event_id != 0) {
+   xf86DrvMsg(crtc->scrn->scrnIndex, X_WARNING,
+  "Need to handle previously deferred vblank 
event\n");
+   
present_event_notify(drmmode_crtc->present_vblank_event_id,
+drmmode_crtc->present_vblank_usec,
+drmmode_crtc->present_vblank_msc);
+   }
+
+   drmmode_crtc->present_vblank_event_id = event->event_id;
+   drmmode_crtc->present_vblank_msc = msc;
+   drmmode_crtc->present_v

[PATCH xf86-video-amdgpu 2/3] Use xorg_list_append for the DRM event list

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

We were adding entries at the start of the list, i.e. the list was
ordered from most recently added to least recently added. However, the
corresponding DRM events are generally expected to arrive in the same
order as they are queued, which means that amdgpu_drm_queue_alloc would
generally have to traverse the whole list to find the entry
corresponding to an arrived event. Fix this by adding entries at the end
of the list.

(Ported from radeon commit 3e24770b1b472fc15df56d06f5f04778c9db63dd)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_drm_queue.c | 2 +-
 src/amdgpu_list.h  | 7 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/amdgpu_drm_queue.c b/src/amdgpu_drm_queue.c
index e8810fdf0..eb3ca633d 100644
--- a/src/amdgpu_drm_queue.c
+++ b/src/amdgpu_drm_queue.c
@@ -105,7 +105,7 @@ amdgpu_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
e->handler = handler;
e->abort = abort;
 
-   xorg_list_add(>list, _drm_queue);
+   xorg_list_append(>list, _drm_queue);
 
return e->seq;
 }
diff --git a/src/amdgpu_list.h b/src/amdgpu_list.h
index a2b1d3cc4..eb28835e3 100644
--- a/src/amdgpu_list.h
+++ b/src/amdgpu_list.h
@@ -35,6 +35,13 @@
 #define xorg_list_del  list_del
 #define xorg_list_for_each_entry   list_for_each_entry
 #define xorg_list_for_each_entry_safe  list_for_each_entry_safe
+
+static inline void
+xorg_list_append(struct list *entry, struct list *head)
+{
+   __list_add(entry, head->prev, head);
+}
+
 #endif
 
 #endif /* _AMDGPU_LIST_H_ */
-- 
2.14.1

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


[PATCH xf86-video-amdgpu 3/3] Make amdgpu_scanout_do_update take a PixmapPtr instead of a DrawablePtr

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

All callers were already passing in a pixmap.

This allows simplifying the rotated scanout case slightly.

(Ported from radeon commit d822a0f47070374ad0c1a97b559bae27724dc52a)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_drv.h  |  2 +-
 src/amdgpu_kms.c  | 13 ++---
 src/drmmode_display.c |  6 +++---
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/amdgpu_drv.h b/src/amdgpu_drv.h
index 13237470a..75c2a2653 100644
--- a/src/amdgpu_drv.h
+++ b/src/amdgpu_drv.h
@@ -341,7 +341,7 @@ Bool amdgpu_dri3_screen_init(ScreenPtr screen);
 
 /* amdgpu_kms.c */
 Bool amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id,
-  DrawablePtr src_draw, BoxPtr extents);
+ PixmapPtr src_pix, BoxPtr extents);
 void AMDGPUWindowExposures_oneshot(WindowPtr pWin, RegionPtr pRegion
 #if XORG_VERSION_CURRENT < XORG_VERSION_NUMERIC(1,16,99,901,0)
   , RegionPtr pBSRegion
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index f665a01cd..e0b735819 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -806,7 +806,7 @@ amdgpu_dirty_update(ScrnInfoPtr scrn)
 
 Bool
 amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id,
-DrawablePtr src_draw, BoxPtr extents)
+PixmapPtr src_pix, BoxPtr extents)
 {
drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
RegionRec region = { .extents = *extents, .data = NULL };
@@ -834,10 +834,9 @@ amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int 
scanout_id,
PictFormatPtr format = PictureWindowFormat(pScreen->root);
int error;
PicturePtr src, dst;
-   XID include_inferiors = IncludeInferiors;
 
-   src = CreatePicture(None, src_draw, format, CPSubwindowMode,
-   _inferiors, serverClient, );
+   src = CreatePicture(None, _pix->drawable, format, 0L, NULL,
+   serverClient, );
if (!src) {
ErrorF("Failed to create source picture for transformed 
scanout "
   "update\n");
@@ -880,7 +879,7 @@ amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int 
scanout_id,
GCPtr gc = GetScratchGC(pDraw->depth, pScreen);
 
ValidateGC(pDraw, gc);
-   (*gc->ops->CopyArea)(src_draw, pDraw, gc,
+   (*gc->ops->CopyArea)(_pix->drawable, pDraw, gc,
 xf86_crtc->x + extents->x1, xf86_crtc->y + 
extents->y1,
 extents->x2 - extents->x1, extents->y2 - 
extents->y1,
 extents->x1, extents->y1);
@@ -912,7 +911,7 @@ amdgpu_scanout_update_handler(xf86CrtcPtr crtc, uint32_t 
frame, uint64_t usec,
!drmmode_crtc->flip_pending &&
drmmode_crtc->dpms_mode == DPMSModeOn) {
if (amdgpu_scanout_do_update(crtc, drmmode_crtc->scanout_id,
-
>GetWindowPixmap(screen->root)->drawable,
+
screen->GetWindowPixmap(screen->root),
 >extents))
RegionEmpty(region);
}
@@ -993,7 +992,7 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
 
scanout_id = drmmode_crtc->scanout_id ^ 1;
if (!amdgpu_scanout_do_update(xf86_crtc, scanout_id,
- 
>GetWindowPixmap(pScreen->root)->drawable,
+ pScreen->GetWindowPixmap(pScreen->root),
  >extents))
return;
RegionEmpty(region);
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index edd955ece..ad3325be6 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -779,7 +779,7 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
*x = *y = 0;
 
amdgpu_scanout_do_update(crtc, scanout_id,
-
>GetWindowPixmap(screen->root)->drawable,
+screen->GetWindowPixmap(screen->root),
 box);
amdgpu_glamor_finish(scrn);
}
@@ -2850,8 +2850,8 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
goto error;
}
 
-   amdgpu_scanout_do_update(crtc, scanout_id,
-_front->drawable, 
);
+   amdgpu_scanout_do_update(cr

[PATCH xf86-video-amdgpu 10/10] Always allow DRI2 page flipping with TearFree

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Even if TearFree is enabled for the CRTC we're synchronizing to.

(Ported from radeon commit d314cbfb228bb4b8762714f98d0c114a8ee3f061)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 man/amdgpu.man| 6 ++
 src/amdgpu_dri2.c | 2 --
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/man/amdgpu.man b/man/amdgpu.man
index ebb472aba..c38defeb3 100644
--- a/man/amdgpu.man
+++ b/man/amdgpu.man
@@ -77,10 +77,8 @@ Set the default value of the per-output 'TearFree' property, 
which controls
 tearing prevention using the hardware page flipping mechanism. TearFree is
 on for any CRTC associated with one or more outputs with TearFree on. Two
 separate scanout buffers need to be allocated for each CRTC with TearFree
-on. While TearFree is on for any CRTC, it may prevent clients from using
-DRI page flipping. If this option is set, the default value of the property
-is 'on' or 'off' accordingly. If this option isn't set, the default value of 
the
-property is
+on. If this option is set, the default value of the property is 'on' or 'off'
+accordingly. If this option isn't set, the default value of the property is
 .B auto,
 which means that TearFree is on for outputs with rotation or other RandR
 transforms, and for RandR 1.4 slave outputs, otherwise off.
diff --git a/src/amdgpu_dri2.c b/src/amdgpu_dri2.c
index bf85dfa63..ccf5477cf 100644
--- a/src/amdgpu_dri2.c
+++ b/src/amdgpu_dri2.c
@@ -641,8 +641,6 @@ can_flip(xf86CrtcPtr crtc, DrawablePtr draw,
for (i = 0, num_crtcs_on = 0; i < config->num_crtc; i++) {
if (drmmode_crtc_can_flip(config->crtc[i]))
num_crtcs_on++;
-   else if (config->crtc[i] == crtc)
-   return FALSE;
}
 
return num_crtcs_on > 0 && can_exchange(pScrn, draw, front, back);
-- 
2.14.1

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


[PATCH xf86-video-amdgpu 04/10] Create drmmode_crtc_wait_pending_event helper macro

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Preparation for following change, no functional change intended yet.

(Ported from radeon commit f87acdbfb1b0b6d2769764772a52ea8b81675e20)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 1a805b82d..bdd3866b8 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -96,6 +96,14 @@ AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, 
char *output_name)
return FALSE;
 }
 
+
+/* Wait for the boolean condition to be FALSE */
+#define drmmode_crtc_wait_pending_event(drmmode_crtc, fd, condition) \
+   do {} while ((condition) && \
+drmHandleEvent(fd, _crtc->drmmode->event_context) \
+> 0);
+
+
 static PixmapPtr drmmode_create_bo_pixmap(ScrnInfoPtr pScrn,
  int width, int height,
  int depth, int bpp,
@@ -891,10 +899,8 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr 
mode,
goto done;
}
 
-   /* Wait for any pending flip to finish */
-   do {} while (drmmode_crtc->flip_pending &&
-drmHandleEvent(pAMDGPUEnt->fd,
-   >event_context) > 0);
+   drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
+   drmmode_crtc->flip_pending);
 
if (drmModeSetCrtc(pAMDGPUEnt->fd,
   drmmode_crtc->mode_crtc->crtc_id,
-- 
2.14.1

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


[PATCH xf86-video-amdgpu 07/10] Add source drawable parameter to amdgpu_scanout_do_update

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Preparation for following changes, no functional change intended yet.

(Ported from radeon commit 1443270e52e8562bd8dc3603f301963bd4027cef)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_drv.h  |  3 ++-
 src/amdgpu_kms.c  | 18 +-
 src/drmmode_display.c |  3 ++-
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/amdgpu_drv.h b/src/amdgpu_drv.h
index da71ce848..515d46625 100644
--- a/src/amdgpu_drv.h
+++ b/src/amdgpu_drv.h
@@ -340,7 +340,8 @@ typedef struct {
 Bool amdgpu_dri3_screen_init(ScreenPtr screen);
 
 /* amdgpu_kms.c */
-Bool amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id);
+Bool amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id,
+  DrawablePtr src_draw);
 void AMDGPUWindowExposures_oneshot(WindowPtr pWin, RegionPtr pRegion
 #if XORG_VERSION_CURRENT < XORG_VERSION_NUMERIC(1,16,99,901,0)
   , RegionPtr pBSRegion
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index d8f667df0..230a779fc 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -792,7 +792,8 @@ amdgpu_dirty_update(ScrnInfoPtr scrn)
 #endif
 
 Bool
-amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id)
+amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int scanout_id,
+ DrawablePtr src_draw)
 {
drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
RegionPtr pRegion = DamageRegion(drmmode_crtc->scanout_damage);
@@ -827,10 +828,7 @@ amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int 
scanout_id)
PicturePtr src, dst;
XID include_inferiors = IncludeInferiors;
 
-   src = CreatePicture(None,
-   >root->drawable,
-   format,
-   CPSubwindowMode,
+   src = CreatePicture(None, src_draw, format, CPSubwindowMode,
_inferiors, serverClient, );
if (!src) {
ErrorF("Failed to create source picture for transformed 
scanout "
@@ -874,8 +872,7 @@ amdgpu_scanout_do_update(xf86CrtcPtr xf86_crtc, int 
scanout_id)
GCPtr gc = GetScratchGC(pDraw->depth, pScreen);
 
ValidateGC(pDraw, gc);
-   
(*gc->ops->CopyArea)(>GetWindowPixmap(pScreen->root)->drawable,
-pDraw, gc,
+   (*gc->ops->CopyArea)(src_draw, pDraw, gc,
 xf86_crtc->x + extents.x1, xf86_crtc->y + 
extents.y1,
 extents.x2 - extents.x1, extents.y2 - 
extents.y1,
 extents.x1, extents.y1);
@@ -900,8 +897,10 @@ amdgpu_scanout_update_handler(xf86CrtcPtr crtc, uint32_t 
frame, uint64_t usec,
  void *event_data)
 {
drmmode_crtc_private_ptr drmmode_crtc = event_data;
+   ScreenPtr screen = crtc->scrn->pScreen;
 
-   amdgpu_scanout_do_update(crtc, drmmode_crtc->scanout_id);
+   amdgpu_scanout_do_update(crtc, drmmode_crtc->scanout_id,
+
>GetWindowPixmap(screen->root)->drawable);
 
amdgpu_scanout_update_abort(crtc, event_data);
 }
@@ -988,7 +987,8 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
return;
 
scanout_id = drmmode_crtc->scanout_id ^ 1;
-   if (!amdgpu_scanout_do_update(xf86_crtc, scanout_id))
+   if (!amdgpu_scanout_do_update(xf86_crtc, scanout_id,
+ 
>GetWindowPixmap(pScreen->root)->drawable))
return;
 
drm_queue_seq = amdgpu_drm_queue_alloc(xf86_crtc,
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index eb701a89a..972a6f217 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -770,7 +770,8 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
*fb = 
amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
*x = *y = 0;
 
-   amdgpu_scanout_do_update(crtc, scanout_id);
+   amdgpu_scanout_do_update(crtc, scanout_id,
+
>GetWindowPixmap(screen->root)->drawable);
amdgpu_glamor_finish(scrn);
}
 }
-- 
2.14.1

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


Re: [PATCH] drm/amdgpu/virtual_dce: Virtual display doesn't support disable vblank immediately

2017-08-18 Thread Michel Dänzer
On 18/08/17 02:48 PM, Emily Deng wrote:
> For virtual display, it uses software timer to emulate the vsync interrupt,
> it doesn't have high precision, so doesn't support disable vblank immediately.
> 
> BUG: SWDEV-129274
> 
> Signed-off-by: Emily Deng <emily.d...@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index dc74c0b..914c5bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -237,9 +237,11 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>   }
>  
>   if (!amdgpu_device_has_dc_support(adev)) {
> - /* Disable vblank irqs aggressively for power-saving */
> - /* XXX: can this be enabled for DC? */
> - adev->ddev->vblank_disable_immediate = true;
> + if (!adev->enable_virtual_display) {
> + /* Disable vblank irqs aggressively for power-saving */
> + /* XXX: can this be enabled for DC? */
> + adev->ddev->vblank_disable_immediate = true;
> + }
>  
>       r = drm_vblank_init(adev->ddev, adev->mode_info.num_crtc);
>   if (r)
> 

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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


[PATCH xf86-video-amdgpu 1/3] Consolidate amdgpu_scanout_flip_abort/handler helpers

2017-08-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

While at it, make them use crtc->driver_private.

(Ported from radeon commit 36ce7920136c0d723c9397a84e7dd5926a9c7943)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c | 85 
 1 file changed, 36 insertions(+), 49 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index b0f1a3d94..f665a01cd 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -420,6 +420,38 @@ amdgpu_sync_scanout_pixmaps(xf86CrtcPtr xf86_crtc, 
RegionPtr new_region,
RegionUninit();
 }
 
+static void
+amdgpu_scanout_flip_abort(xf86CrtcPtr crtc, void *event_data)
+{
+   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+   drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+
+   drmmode_crtc->scanout_update_pending = FALSE;
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
+NULL);
+}
+
+static void
+amdgpu_scanout_flip_handler(xf86CrtcPtr crtc, uint32_t msc, uint64_t usec,
+   void *event_data)
+{
+   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+   drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->fb,
+drmmode_crtc->flip_pending);
+   amdgpu_scanout_flip_abort(crtc, event_data);
+
+#ifdef HAVE_PRESENT_H
+   if (drmmode_crtc->present_vblank_event_id) {
+   present_event_notify(drmmode_crtc->present_vblank_event_id,
+drmmode_crtc->present_vblank_usec,
+drmmode_crtc->present_vblank_msc);
+   drmmode_crtc->present_vblank_event_id = 0;
+   }
+#endif
+}
+
 #ifdef AMDGPU_PIXMAP_SHARING
 
 static RegionPtr
@@ -664,38 +696,6 @@ amdgpu_prime_scanout_update(PixmapDirtyUpdatePtr dirty)
drmmode_crtc->scanout_update_pending = TRUE;
 }
 
-static void
-amdgpu_prime_scanout_flip_abort(xf86CrtcPtr crtc, void *event_data)
-{
-   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
-   drmmode_crtc_private_ptr drmmode_crtc = event_data;
-
-   drmmode_crtc->scanout_update_pending = FALSE;
-   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
-NULL);
-}
-
-static void
-amdgpu_prime_scanout_flip_handler(xf86CrtcPtr crtc, uint32_t msc, uint64_t 
usec,
- void *event_data)
-{
-   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
-   drmmode_crtc_private_ptr drmmode_crtc = event_data;
-
-   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->fb,
-drmmode_crtc->flip_pending);
-   amdgpu_prime_scanout_flip_abort(crtc, event_data);
-
-#ifdef HAVE_PRESENT_H
-   if (drmmode_crtc->present_vblank_event_id) {
-   present_event_notify(drmmode_crtc->present_vblank_event_id,
-drmmode_crtc->present_vblank_usec,
-drmmode_crtc->present_vblank_msc);
-   drmmode_crtc->present_vblank_event_id = 0;
-   }
-#endif
-}
-
 static void
 amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
 {
@@ -723,9 +723,9 @@ amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
drm_queue_seq = amdgpu_drm_queue_alloc(crtc,
   AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
   AMDGPU_DRM_QUEUE_ID_DEFAULT,
-  drmmode_crtc,
-  
amdgpu_prime_scanout_flip_handler,
-  amdgpu_prime_scanout_flip_abort);
+  NULL,
+  amdgpu_scanout_flip_handler,
+  amdgpu_scanout_flip_abort);
if (drm_queue_seq == AMDGPU_DRM_QUEUE_ERROR) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Allocating DRM event queue entry failed for PRIME 
flip.\n");
@@ -975,19 +975,6 @@ amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
drmmode_crtc->scanout_update_pending = TRUE;
 }
 
-static void
-amdgpu_scanout_flip_abort(xf86CrtcPtr crtc, void *event_data)
-{
-   amdgpu_prime_scanout_flip_abort(crtc, event_data);
-}
-
-static void
-amdgpu_scanout_flip_handler(xf86CrtcPtr crtc, uint32_t msc, uint64_t usec,
-   void *event_data)
-{
-   amdgpu_prime_scanout_flip_handler(crtc, msc, usec, event_data);
-}
-
 static void
 amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
xf86CrtcPtr xf86_crtc)
@@ -1014,7 +1001,7 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
drm_queue_

[PATCH xf86-video-ati] modesetting: re-set the crtc's mode when link-status goes BAD

2017-08-17 Thread Michel Dänzer
From: Martin Peres <martin.pe...@linux.intel.com>

Despite all the careful planning of the kernel, a link may become
insufficient to handle the currently-set mode. At this point, the
kernel should mark this particular configuration as being broken
and potentially prune the mode before setting the offending connector's
link-status to BAD and send the userspace a hotplug event. This may
happen right after a modeset or later on.

Upon receiving a hot-plug event, we iterate through the connectors to
re-apply the currently-set mode on all the connectors that have a
link-status property set to BAD. The kernel may be able to get the
link to work by dropping to using a lower link bpp (with the same
display bpp). However, the modeset may fail if the kernel has pruned
the mode, so to make users aware of this problem a warning is outputed
in the logs to warn about having a potentially-black display.

This patch does not modify the current behaviour of always propagating
the events to the randr clients. This allows desktop environments to
re-probe the connectors and select a new resolution based on the new
(currated) mode list if a mode disapeared. This behaviour is expected in
order to pass the Display Port compliance tests.

(Ported from xserver commit bcee1b76aa0db8525b491485e90b8740763d7de6)

[ Michel: Bump libdrm dependency to >= 2.4.78 for
  DRM_MODE_LINK_STATUS_BAD ]

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 configure.ac  |  2 +-
 src/drmmode_display.c | 43 +++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index c9bc0a958..170f42ef7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -70,7 +70,7 @@ XORG_DRIVER_CHECK_EXT(XV, videoproto)
 XORG_DRIVER_CHECK_EXT(DPMSExtension, xextproto)
 
 # Checks for libraries.
-PKG_CHECK_MODULES(LIBDRM, [libdrm >= 2.4.60])
+PKG_CHECK_MODULES(LIBDRM, [libdrm >= 2.4.78])
 PKG_CHECK_MODULES(LIBDRM_RADEON, [libdrm_radeon])
 
 # Obtain compiler/linker options for the driver dependencies
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 4839b415c..f926bc018 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2826,6 +2826,49 @@ radeon_mode_hotplug(ScrnInfoPtr scrn, drmmode_ptr 
drmmode)
Bool changed = FALSE;
int num_dvi = 0, num_hdmi = 0;
 
+   /* Try to re-set the mode on all the connectors with a BAD link-state:
+* This may happen if a link degrades and a new modeset is necessary, 
using
+* different link-training parameters. If the kernel found that the 
current
+* mode is not achievable anymore, it should have pruned the mode before
+* sending the hotplug event. Try to re-set the currently-set mode to 
keep
+* the display alive, this will fail if the mode has been pruned.
+* In any case, we will send randr events for the Desktop Environment to
+* deal with it, if it wants to.
+*/
+   for (i = 0; i < config->num_output; i++) {
+   xf86OutputPtr output = config->output[i];
+   drmmode_output_private_ptr drmmode_output = 
output->driver_private;
+   uint32_t con_id = drmmode_output->mode_output->connector_id;
+   drmModeConnectorPtr koutput;
+
+   /* Get an updated view of the properties for the current 
connector and
+* look for the link-status property
+*/
+   koutput = drmModeGetConnectorCurrent(pRADEONEnt->fd, con_id);
+   for (j = 0; koutput && j < koutput->count_props; j++) {
+   drmModePropertyPtr props;
+   props = drmModeGetProperty(pRADEONEnt->fd, 
koutput->props[j]);
+   if (props && props->flags & DRM_MODE_PROP_ENUM &&
+   !strcmp(props->name, "link-status") &&
+   koutput->prop_values[j] == 
DRM_MODE_LINK_STATUS_BAD) {
+   xf86CrtcPtr crtc = output->crtc;
+   if (!crtc)
+   continue;
+
+   /* the connector got a link failure, re-set the 
current mode */
+   drmmode_set_mode_major(crtc, >mode, 
crtc->rotation,
+  crtc->x, crtc->y);
+
+   xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+  "hotplug event: connector %u's 
link-state is BAD, "
+  "tried resetting the current mode. 
You may be left "
+  "with a black screen if this 
fails...\n", con_id);
+   }
+   drmModeFreeProperty(props

Re: [PATCH 1/1] amdgpu: move asic id table to a separate file

2017-05-11 Thread Michel Dänzer
On 12/05/17 06:13 AM, Li, Samuel wrote:
> Submitted a request to create a new repo on freedesktop.

What's the point of having a separate repository upstream? Can't we just
keep it in the libdrm repository?


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


Re: [PATCH] drm/amdgpu: properly byteswap gpu_info firmware

2017-05-11 Thread Michel Dänzer
On 12/05/17 08:10 AM, Alex Deucher wrote:
> It's stored in LE format.
> 
> Signed-off-by: Alex Deucher <alexander.deuc...@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d95d4c9..8a5bb42 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1459,19 +1459,19 @@ static int amdgpu_device_parse_gpu_info_fw(struct 
> amdgpu_device *adev)
>   (const struct gpu_info_firmware_v1_0 *)(fw->data +
>   
> le32_to_cpu(hdr->header.ucode_array_offset_bytes));
>  
> - adev->gfx.config.max_shader_engines = gpu_info_fw->gc_num_se;
> - adev->gfx.config.max_cu_per_sh = gpu_info_fw->gc_num_cu_per_sh;
> - adev->gfx.config.max_sh_per_se = gpu_info_fw->gc_num_sh_per_se;
> - adev->gfx.config.max_backends_per_se = 
> gpu_info_fw->gc_num_rb_per_se;
> + adev->gfx.config.max_shader_engines = 
> le32_to_cpu(gpu_info_fw->gc_num_se);
> + adev->gfx.config.max_cu_per_sh = 
> le32_to_cpu(gpu_info_fw->gc_num_cu_per_sh);
> + adev->gfx.config.max_sh_per_se = 
> le32_to_cpu(gpu_info_fw->gc_num_sh_per_se);
> + adev->gfx.config.max_backends_per_se = 
> le32_to_cpu(gpu_info_fw->gc_num_rb_per_se);
>   adev->gfx.config.max_texture_channel_caches =
> - gpu_info_fw->gc_num_tccs;
> - adev->gfx.config.max_gprs = gpu_info_fw->gc_num_gprs;
> - adev->gfx.config.max_gs_threads = 
> gpu_info_fw->gc_num_max_gs_thds;
> - adev->gfx.config.gs_vgt_table_depth = 
> gpu_info_fw->gc_gs_table_depth;
> - adev->gfx.config.gs_prim_buffer_depth = 
> gpu_info_fw->gc_gsprim_buff_depth;
> + le32_to_cpu(gpu_info_fw->gc_num_tccs);
> + adev->gfx.config.max_gprs = 
> le32_to_cpu(gpu_info_fw->gc_num_gprs);
> + adev->gfx.config.max_gs_threads = 
> le32_to_cpu(gpu_info_fw->gc_num_max_gs_thds);
> + adev->gfx.config.gs_vgt_table_depth = 
> le32_to_cpu(gpu_info_fw->gc_gs_table_depth);
> + adev->gfx.config.gs_prim_buffer_depth = 
> le32_to_cpu(gpu_info_fw->gc_gsprim_buff_depth);
>   adev->gfx.config.double_offchip_lds_buf =
> - gpu_info_fw->gc_double_offchip_lds_buffer;
> - adev->gfx.cu_info.wave_front_size = gpu_info_fw->gc_wave_size;
> +         le32_to_cpu(gpu_info_fw->gc_double_offchip_lds_buffer);
> + adev->gfx.cu_info.wave_front_size = 
> le32_to_cpu(gpu_info_fw->gc_wave_size);
>   break;
>   }
>   default:
> 

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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 1/1] amdgpu: move asic id table to a separate file

2017-05-14 Thread Michel Dänzer
On 13/05/17 12:21 AM, Li, Samuel wrote:
> My understanding is this is actually a data file. Similar to amdgpu
> firmware, which is also separate from the kernel source code.

I don't think the reasons for the linux-firmware repository being
separate from linux apply to this file.

Please provide specific reasons why this file cannot be in the libdrm
repository upstream.


-- 
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: Plan: BO move throttling for visible VRAM evictions

2017-05-14 Thread Michel Dänzer
On 14/05/17 06:31 AM, Marek Olšák wrote:
> On Mon, Apr 17, 2017 at 11:55 AM, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 17/04/17 07:58 AM, Marek Olšák wrote:
>>> On Fri, Apr 14, 2017 at 12:14 PM, Michel Dänzer <mic...@daenzer.net> wrote:
>>>> On 04/04/17 05:11 AM, Marek Olšák wrote:
>>>>> On Fri, Mar 31, 2017 at 5:24 AM, Michel Dänzer <mic...@daenzer.net> wrote:
>>>>>> On 30/03/17 07:03 PM, Michel Dänzer wrote:
>>>>>>> On 25/03/17 01:33 AM, Marek Olšák wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I'm sharing this idea here, because it's something that has been
>>>>>>>> decreasing our performance a lot recently, for example:
>>>>>>>> http://openbenchmarking.org/prospect/1703011-RI-RADEONDIR06/7b7668cfc109d1c3dc27e871c8aea71ca13f23fa
>>>>>>>
>>>>>>> The attached proof-of-concept patch (on top of Christian's "CPU mapping
>>>>>>> of split VRAM buffers" series, ported from radeon) results in 145.05 fps
>>>>>>> on my Tonga.
>>>>>>
>>>>>> I get the same result without my or Christian's patches though, with
>>>>>> 4.11 based DRM or amd-staging-4.9. So I guess I just can't reproduce the
>>>>>> problem with this test. Are there any other tests for it?
>>>>>
>>>>> It's random. Sometimes the benchmark runs OK, other times it's slow.
>>>>> You can easily see the difference but observing how smooth it is. The
>>>>> visible VRAM evictions result in constant 100-200ms stalls but not
>>>>> every frame, which feels like the frame rate is much lower than it
>>>>> actually is.
>>>>>
>>>>> Make sure your graphics details are maxed out. The best score I can
>>>>> get with my rig is 70 fps. (Fiji & Core i5 3570)
>>>>
>>>> I'm getting around 53-54 fps at Ultra with Tonga, both with Mesa 13.0.6
>>>> and Git.
>>>>
>>>> Have you tried if Christian's patches for CPU access to split VRAM
>>>> buffers help? I can imagine that forcing contiguous VRAM buffers for CPU
>>>> access could cause lots of other BOs to be unnecessarily evicted from
>>>> VRAM, if at least one of their fragments happens to be in the CPU
>>>> visible part of VRAM.
>>>
>>> I've finally tested latest amd-staging-4.9 and I'm very pleased. For
>>> the first time, the Deus Ex benchmark has almost no hiccups. I've
>>> never seen it so smooth. At one point, the MB/s BO move rate increase
>>> to 200MB/s, stayed there for a couple of seconds, and then it dropped
>>> to 0 again. The frame rate was OK-ish, so I guess the moves didn't
>>> happen all at once. I also tested DiRT Rally and I haven't been able
>>> to reproduce the low FPS with the consistently-high BO move rate that
>>> I saw several months ago.
>>>
>>> We could do some move throttling there for sure, but it's much better
>>> than it ever was.
>>
>> That's great to hear. If you get a chance, it would be interesting if
>> the attached updated patch improves things even more for you. (The patch
>> I attached previously couldn't work as intended, this one at least might :)
> 
> Frogging101 on IRC noticed that we get a ton of TTM BO moves due to
> visible VRAM thrashing and Michel's patch doesn't help. His kernel is
> up to date with amd-staging. It looks like the only option left is my
> original plan: BO move throttling for visible VRAM by redirecting
> mapped buffers to GTT and not allowing them to go back to VRAM if some
> counter is too high.
> 
> Opinions?

I think the next step should be to make radeonsi keep track of how much
VRAM it's trying to use that's expected to be accessed by the CPU, and
to use GTT instead when that exceeds a threshold (probably derived from
vram_vis_size).


-- 
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 libdrm] amdgpu: add missing extern "C" headers

2017-05-14 Thread Michel Dänzer
On 15/05/17 11:09 AM, Michel Dänzer wrote:
> 
> BTW, don't send patches for review as HTML. Ideally use git send-email.

Sorry Alex, I was confused and thought you sent the patch.


-- 
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 libdrm] amdgpu: add missing extern "C" headers

2017-05-14 Thread Michel Dänzer
On 15/05/17 10:10 AM, Xie, AlexBin wrote:
>  amdgpu/amdgpu.h | 8 
>  1 file changed, 8 insertions(+)

This header is synchronized with the kernel, see include/drm/README.


BTW, don't send patches for review as HTML. Ideally use git send-email.


-- 
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


[PATCH xf86-video-amdgpu] Simplify tracking of PRIME scanout pixmap

2017-05-12 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Remember the shared pixmap passed to drmmode_set_scanout_pixmap for each
CRTC, and just compare against that.

Fixes leaving stale entries in ScreenRec::pixmap_dirty_list under some
circumstances, which would usually result in use-after-free and a crash
down the line.

(Ported from radeon commit 7dc68e26755466f9056f8c72195ab8690660693d)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c  |  7 ++-
 src/drmmode_display.c | 21 +++--
 src/drmmode_display.h |  3 +++
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 4df81f993..a418cf9d3 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -562,8 +562,7 @@ amdgpu_prime_dirty_to_crtc(PixmapDirtyUpdatePtr dirty)
xf86CrtcPtr xf86_crtc = xf86_config->crtc[c];
drmmode_crtc_private_ptr drmmode_crtc = 
xf86_crtc->driver_private;
 
-   if (drmmode_crtc->scanout[0].pixmap == dirty->slave_dst ||
-   drmmode_crtc->scanout[1].pixmap == dirty->slave_dst)
+   if (drmmode_crtc->prime_scanout_pixmap == dirty->src)
return xf86_crtc;
}
 
@@ -576,13 +575,11 @@ amdgpu_prime_scanout_do_update(xf86CrtcPtr crtc, unsigned 
scanout_id)
ScrnInfoPtr scrn = crtc->scrn;
ScreenPtr screen = scrn->pScreen;
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-   PixmapPtr scanoutpix = crtc->randr_crtc->scanout_pixmap;
PixmapDirtyUpdatePtr dirty;
Bool ret = FALSE;
 
xorg_list_for_each_entry(dirty, >pixmap_dirty_list, ent) {
-   if (dirty->src == scanoutpix && dirty->slave_dst ==
-   drmmode_crtc->scanout[scanout_id ^ 
drmmode_crtc->tear_free].pixmap) {
+   if (dirty->src == drmmode_crtc->prime_scanout_pixmap) {
RegionPtr region;
 
if (master_has_sync_shared_pixmap(scrn, dirty))
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 9996d2f70..add8287a0 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -681,9 +681,7 @@ drmmode_crtc_prime_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
 
xorg_list_for_each_entry(dirty, >pixmap_dirty_list,
 ent) {
-   if (dirty->src == crtc->randr_crtc->scanout_pixmap &&
-   dirty->slave_dst ==
-   
drmmode_crtc->scanout[drmmode_crtc->scanout_id].pixmap) {
+   if (dirty->src == drmmode_crtc->prime_scanout_pixmap) {
dirty->slave_dst =

drmmode_crtc->scanout[scanout_id].pixmap;
break;
@@ -838,7 +836,7 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr 
mode,
 
fb_id = drmmode->fb_id;
 #ifdef AMDGPU_PIXMAP_SHARING
-   if (crtc->randr_crtc && crtc->randr_crtc->scanout_pixmap) {
+   if (drmmode_crtc->prime_scanout_pixmap) {
drmmode_crtc_prime_scanout_update(crtc, mode, 
scanout_id,
  _id, , );
} else
@@ -1242,14 +1240,15 @@ static Bool drmmode_set_scanout_pixmap(xf86CrtcPtr 
crtc, PixmapPtr ppix)
PixmapDirtyUpdatePtr dirty;
 
xorg_list_for_each_entry(dirty, >pixmap_dirty_list, ent) {
-   if (dirty->slave_dst != 
drmmode_crtc->scanout[scanout_id].pixmap)
-   continue;
-
-   PixmapStopDirtyTracking(dirty->src, dirty->slave_dst);
-   drmmode_crtc_scanout_free(drmmode_crtc);
-   break;
+   if (dirty->src == drmmode_crtc->prime_scanout_pixmap) {
+   PixmapStopDirtyTracking(dirty->src, dirty->slave_dst);
+   break;
+   }
}
 
+   drmmode_crtc_scanout_free(drmmode_crtc);
+   drmmode_crtc->prime_scanout_pixmap = NULL;
+
if (!ppix)
return TRUE;
 
@@ -1266,6 +1265,8 @@ static Bool drmmode_set_scanout_pixmap(xf86CrtcPtr crtc, 
PixmapPtr ppix)
return FALSE;
}
 
+   drmmode_crtc->prime_scanout_pixmap = ppix;
+
 #ifdef HAS_DIRTYTRACKING_ROTATION
PixmapStartDirtyTracking(ppix, drmmode_crtc->scanout[scanout_id].pixmap,
 0, 0, 0, 0, RR_Rotate_0);
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 2d5698f61..6a57fd23b 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -83,6 +83,9 @@ typedef struct {
unsigned scanout_id;
Bool scanout_update_pending;
Bool tear

[PATCH xf86-video-amdgpu] Remove unused struct members from drmmode_display.h

2017-05-10 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 1 -
 src/drmmode_display.h | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 5b2431495..9996d2f70 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2370,7 +2370,6 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, int cpp)
xf86CrtcConfigInit(pScrn, _xf86crtc_config_funcs);
 
drmmode->scrn = pScrn;
-   drmmode->cpp = cpp;
mode_res = drmModeGetResources(pAMDGPUEnt->fd);
if (!mode_res)
return FALSE;
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index fa15a4f72..2d5698f61 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -38,8 +38,6 @@
 
 typedef struct {
unsigned fb_id;
-   drmModeFBPtr mode_fb;
-   int cpp;
ScrnInfoPtr scrn;
 #ifdef HAVE_LIBUDEV
struct udev_monitor *uevent_monitor;
@@ -55,7 +53,6 @@ typedef struct {
 } drmmode_rec, *drmmode_ptr;
 
 typedef struct {
-   int fd;
unsigned old_fb_id;
int flip_count;
void *event_data;
-- 
2.11.0

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


[PATCH xf86-video-amdgpu] Don't enable DRI3 without glamor

2017-05-10 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Can't work currently. Fixes crash when trying to run a DRI3 client when
glamor isn't enabled.

Bugzilla: https://bugs.freedesktop.org/100968
Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 0182cbe0a..4df81f993 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1741,7 +1741,10 @@ Bool AMDGPUScreenInit_KMS(SCREEN_INIT_ARGS_DECL)
 #endif
 
if (!amdgpu_is_gpu_screen(pScreen)) {
-   value = xorgGetVersion() >= XORG_VERSION_NUMERIC(1,18,3,0,0);
+   if (xorgGetVersion() >= XORG_VERSION_NUMERIC(1,18,3,0,0))
+   value = info->use_glamor;
+   else
+   value = FALSE;
from = X_DEFAULT;
 
if (info->use_glamor) {
-- 
2.11.0

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


[PATCH xf86-video-ati 2/2] Use reference counting for tracking KMS framebuffer lifetimes

2017-05-10 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

References are held by the pixmaps corresponding to the FBs (so
the same KMS FB can be reused as long as the pixmap exists) and by the
CRTCs scanning out from them (so a KMS FB is only destroyed once it's
not being scanned out anymore, preventing intermittent black screens and
worse issues due to a CRTC turning off when it should be on).

v2:
* Only increase reference count in drmmode_fb_reference if it was sane
  before
* Make drmmode_fb_reference's indentation match the rest of
  drmmode_display.h

Reviewed-by: Alex Deucher <alexander.deuc...@amd.com> # v1
Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c  | 147 +
 src/drmmode_display.h  |  39 +++--
 src/radeon.h   |  73 
 src/radeon_bo_helper.h |   3 -
 src/radeon_exa.c   |   2 +
 src/radeon_kms.c   |  64 ++---
 src/radeon_present.c   |   8 ---
 7 files changed, 226 insertions(+), 110 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index a101ac233..ec3072621 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -375,6 +375,7 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
 
drmModeSetCrtc(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
   0, 0, 0, NULL, 0, NULL);
+   drmmode_fb_reference(drmmode->fd, _crtc->fb, NULL);
} else if (drmmode_crtc->dpms_mode != DPMSModeOn)
crtc->funcs->set_mode_major(crtc, >mode, crtc->rotation,
crtc->x, crtc->y);
@@ -447,8 +448,9 @@ void drmmode_copy_fb(ScrnInfoPtr pScrn, drmmode_ptr drmmode)
 {
xf86CrtcConfigPtr   xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
RADEONInfoPtr info = RADEONPTR(pScrn);
-   PixmapPtr src, dst;
ScreenPtr pScreen = pScrn->pScreen;
+   PixmapPtr src, dst = pScreen->GetScreenPixmap(pScreen);
+   struct drmmode_fb *fb = radeon_pixmap_get_fb(dst);
int fbcon_id = 0;
Bool force;
GCPtr gc;
@@ -464,7 +466,7 @@ void drmmode_copy_fb(ScrnInfoPtr pScrn, drmmode_ptr drmmode)
if (!fbcon_id)
return;
 
-   if (fbcon_id == drmmode->fb_id) {
+   if (fbcon_id == fb->handle) {
/* in some rare case there might be no fbcon and we might 
already
 * be the one with the current fb to avoid a false deadlck in
 * kernel ttm code just do nothing as anyway there is nothing
@@ -477,8 +479,6 @@ void drmmode_copy_fb(ScrnInfoPtr pScrn, drmmode_ptr drmmode)
if (!src)
return;
 
-   dst = pScreen->GetScreenPixmap(pScreen);
-
gc = GetScratchGC(pScrn->depth, pScreen);
ValidateGC(>drawable, gc);
 
@@ -505,8 +505,6 @@ drmmode_crtc_scanout_destroy(drmmode_ptr drmmode,
}
 
if (scanout->bo) {
-   drmModeRmFB(drmmode->fd, scanout->fb_id);
-   scanout->fb_id = 0;
radeon_bo_unmap(scanout->bo);
radeon_bo_unref(scanout->bo);
scanout->bo = NULL;
@@ -571,15 +569,9 @@ drmmode_crtc_scanout_create(xf86CrtcPtr crtc, struct 
drmmode_scanout *scanout,
scanout->bo = radeon_alloc_pixmap_bo(pScrn, width, height, pScrn->depth,
 tiling, pScrn->bitsPerPixel,
 , , );
-   if (scanout->bo == NULL)
-   goto error;
-
-   if (drmModeAddFB(drmmode->fd, width, height, pScrn->depth,
-  pScrn->bitsPerPixel, pitch,
-  scanout->bo->handle,
-  >fb_id) != 0) {
-   ErrorF("failed to add scanout fb\n");
-   goto error;
+   if (!scanout->bo) {
+   ErrorF("failed to create CRTC scanout BO\n");
+   return NULL;
}
 
scanout->pixmap = drmmode_create_bo_pixmap(pScrn,
@@ -587,13 +579,17 @@ drmmode_crtc_scanout_create(xf86CrtcPtr crtc, struct 
drmmode_scanout *scanout,
 pScrn->depth,
 pScrn->bitsPerPixel,
 pitch, scanout->bo, NULL);
-   if (scanout->pixmap) {
+   if (!scanout->pixmap) {
+   ErrorF("failed to create CRTC scanout pixmap\n");
+   goto error;
+   }
+
+   if (radeon_pixmap_get_fb(scanout->pixmap)) {
scanout->width = width;
scanout->height = height;
} else {
-   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-  "Couldn't allocate scanout pixmap for CRTC\n");
-error:

[PATCH xf86-video-ati 1/2] Pass pixmap instead of handle to radeon_do_pageflip

2017-05-10 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

This brings us in line with amdgpu and prepares for the following
change, no functional change intended.

(Ported from amdgpu commit e463b849f3e9d7b69e64a65619a22e00e78d297b)

v2:
* Be more consistent with the amdgpu code, which should make porting
  the following change to amdgpu easier

Reviewed-by: Alex Deucher <alexander.deuc...@amd.com> # v1
Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 26 +-
 src/drmmode_display.h |  2 +-
 src/radeon_dri2.c |  5 +
 src/radeon_present.c  | 15 ++-
 4 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 0b823754b..a101ac233 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2950,36 +2950,27 @@ void drmmode_uevent_fini(ScrnInfoPtr scrn, drmmode_ptr 
drmmode)
 }
 
 Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
-   uint32_t new_front_handle, uint64_t id, void *data,
+   PixmapPtr new_front, uint64_t id, void *data,
int ref_crtc_hw_id, radeon_drm_handler_proc handler,
radeon_drm_abort_proc abort,
enum drmmode_flip_sync flip_sync,
uint32_t target_msc)
 {
RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
-   RADEONInfoPtr info = RADEONPTR(scrn);
xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
xf86CrtcPtr crtc = NULL;
drmmode_crtc_private_ptr drmmode_crtc = config->crtc[0]->driver_private;
drmmode_ptr drmmode = drmmode_crtc->drmmode;
-   unsigned int pitch;
int i;
-   uint32_t tiling_flags = 0;
uint32_t flip_flags = flip_sync == FLIP_ASYNC ? 
DRM_MODE_PAGE_FLIP_ASYNC : 0;
drmmode_flipdata_ptr flipdata;
uintptr_t drm_queue_seq = 0;
+   uint32_t new_front_handle;
 
-   if (info->allowColorTiling) {
-   if (info->ChipFamily >= CHIP_FAMILY_R600)
-   tiling_flags |= RADEON_TILING_MICRO;
-   else
-   tiling_flags |= RADEON_TILING_MACRO;
-   }
-
-   pitch = RADEON_ALIGN(scrn->displayWidth, drmmode_get_pitch_align(scrn, 
info->pixel_bytes, tiling_flags)) *
-   info->pixel_bytes;
-   if (info->ChipFamily >= CHIP_FAMILY_R600 && info->surf_man) {
-   pitch = info->front_surface.level[0].pitch_bytes;
+   if (!radeon_get_pixmap_handle(new_front, _front_handle)) {
+   xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+  "flip queue: failed to get new front handle\n");
+   return FALSE;
}
 
 flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
@@ -2993,8 +2984,9 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
 * Create a new handle for the back buffer
 */
flipdata->old_fb_id = drmmode->fb_id;
-   if (drmModeAddFB(drmmode->fd, scrn->virtualX, scrn->virtualY,
-scrn->depth, scrn->bitsPerPixel, pitch,
+   if (drmModeAddFB(drmmode->fd, new_front->drawable.width,
+new_front->drawable.height, scrn->depth,
+scrn->bitsPerPixel, new_front->devKind,
 new_front_handle, >fb_id))
goto error;
 
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index bd3f5f987..35d64179d 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -168,7 +168,7 @@ extern int drmmode_get_base_align(ScrnInfoPtr scrn, int 
bpe, uint32_t tiling);
 extern void drmmode_clear_pending_flip(xf86CrtcPtr crtc);
 
 Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
-   uint32_t new_front_handle, uint64_t id, void *data,
+   PixmapPtr new_front, uint64_t id, void *data,
int ref_crtc_hw_id, radeon_drm_handler_proc handler,
radeon_drm_abort_proc abort,
enum drmmode_flip_sync flip_sync,
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index c108ceab2..cc72bd52d 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -652,7 +652,6 @@ radeon_dri2_schedule_flip(xf86CrtcPtr crtc, ClientPtr 
client,
 ScrnInfoPtr scrn = crtc->scrn;
 RADEONInfoPtr info = RADEONPTR(scrn);
 struct dri2_buffer_priv *back_priv;
-struct radeon_bo *bo;
 DRI2FrameEventPtr flip_info;
 int ref_crtc_hw_id = drmmode_get_crtc_id(crtc);
 
@@ -673,9 +672,7 @@ radeon_dri2_schedule_flip(xf86CrtcPtr crtc, ClientPtr 
client,
 
 /* Page flip the full screen buffer */
 back_priv = back->driverPrivate;
-bo = radeon_get_pixmap_bo(back_priv->pixmap);
-
-if (radeon_do_pageflip(scrn, client, bo->ha

[PATCH xf86-video-ati] Simplify tracking of PRIME scanout pixmap

2017-05-10 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Remember the shared pixmap passed to drmmode_set_scanout_pixmap for each
CRTC, and just compare against that.

Fixes leaving stale entries in ScreenRec::pixmap_dirty_list under some
circumstances, which would usually result in use-after-free and a crash
down the line.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 21 +++--
 src/drmmode_display.h |  3 +++
 src/radeon_kms.c  |  7 ++-
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index ec3072621..e2899cf50 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -732,9 +732,7 @@ drmmode_crtc_prime_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
 
xorg_list_for_each_entry(dirty, >pixmap_dirty_list,
 ent) {
-   if (dirty->src == crtc->randr_crtc->scanout_pixmap &&
-   dirty->slave_dst ==
-   
drmmode_crtc->scanout[drmmode_crtc->scanout_id].pixmap) {
+   if (dirty->src == drmmode_crtc->prime_scanout_pixmap) {
dirty->slave_dst =

drmmode_crtc->scanout[scanout_id].pixmap;
break;
@@ -887,7 +885,7 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr 
mode,
drmmode_ConvertToKMode(crtc->scrn, , mode);
 
 #ifdef RADEON_PIXMAP_SHARING
-   if (crtc->randr_crtc && crtc->randr_crtc->scanout_pixmap) {
+   if (drmmode_crtc->prime_scanout_pixmap) {
drmmode_crtc_prime_scanout_update(crtc, mode, 
scanout_id,
  , , );
} else
@@ -1278,14 +1276,15 @@ drmmode_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr 
ppix)
PixmapDirtyUpdatePtr dirty;
 
xorg_list_for_each_entry(dirty, >pixmap_dirty_list, ent) {
-   if (dirty->slave_dst != 
drmmode_crtc->scanout[scanout_id].pixmap)
-   continue;
-
-   PixmapStopDirtyTracking(dirty->src, dirty->slave_dst);
-   drmmode_crtc_scanout_free(drmmode_crtc);
-   break;
+   if (dirty->src == drmmode_crtc->prime_scanout_pixmap) {
+   PixmapStopDirtyTracking(dirty->src, dirty->slave_dst);
+   break;
+   }
}
 
+   drmmode_crtc_scanout_free(drmmode_crtc);
+   drmmode_crtc->prime_scanout_pixmap = NULL;
+
if (!ppix)
return TRUE;
 
@@ -1302,6 +1301,8 @@ drmmode_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr 
ppix)
return FALSE;
}
 
+   drmmode_crtc->prime_scanout_pixmap = ppix;
+
 #ifdef HAS_DIRTYTRACKING_ROTATION
PixmapStartDirtyTracking(ppix, drmmode_crtc->scanout[scanout_id].pixmap,
 0, 0, 0, 0, RR_Rotate_0);
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 14d1cb034..df2c4b7bb 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -92,6 +92,9 @@ typedef struct {
 unsigned scanout_id;
 Bool scanout_update_pending;
 Bool tear_free;
+
+PixmapPtr prime_scanout_pixmap;
+
 int dpms_mode;
 /* For when a flip is pending when DPMS off requested */
 int pending_dpms_mode;
diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index 2b410eb3d..c4bdfcfac 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -657,8 +657,7 @@ radeon_prime_dirty_to_crtc(PixmapDirtyUpdatePtr dirty)
xf86CrtcPtr xf86_crtc = xf86_config->crtc[c];
drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
 
-   if (drmmode_crtc->scanout[0].pixmap == dirty->slave_dst ||
-   drmmode_crtc->scanout[1].pixmap == dirty->slave_dst)
+   if (drmmode_crtc->prime_scanout_pixmap == dirty->src)
return xf86_crtc;
 }
 
@@ -671,13 +670,11 @@ radeon_prime_scanout_do_update(xf86CrtcPtr crtc, unsigned 
scanout_id)
 ScrnInfoPtr scrn = crtc->scrn;
 ScreenPtr screen = scrn->pScreen;
 drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-PixmapPtr scanoutpix = crtc->randr_crtc->scanout_pixmap;
 PixmapDirtyUpdatePtr dirty;
 Bool ret = FALSE;
 
 xorg_list_for_each_entry(dirty, >pixmap_dirty_list, ent) {
-   if (dirty->src == scanoutpix && dirty->slave_dst ==
-   drmmode_crtc->scanout[scanout_id ^ drmmode_crtc->tear_free].pixmap) 
{
+   if (dirty->src == drmmode_crtc->prime_scanout_pixmap) {
RegionPtr region;
 
if (master_has_sync_shared_pixmap(scrn, dirty))
-- 
2.11.0

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


Re: (radeon?) WARNING: drivers/gpu/drm/drm_irq.c:1195 drm_vblank_put (v4.11-12441-g56868a4)

2017-05-10 Thread Michel Dänzer
On 11/05/17 04:33 AM, Tommi Rantala wrote:
> Hi,
> 
> I just tested v4.11-12441-g56868a4 on HP xw6600 with radeon graphics,
> and I'm seeing the following WARNING triggered constantly.
> 
> I have not seen this earlier e.g. with the distro kernel 
> 4.10.13-200.fc25.x86_64
> 
> $ lspci|grep -i amd
> 60:00.0 VGA compatible controller: Advanced Micro Devices, Inc.
> [AMD/ATI] Curacao PRO [Radeon R7 370 / R9 270/370 OEM]
> 60:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Cape
> Verde/Pitcairn HDMI Audio [Radeon HD 7700/7800 Series]
> 
> Complete kernel log:
> http://termbin.com/dzy5
> 
> [  249.952546] [ cut here ]
> [  249.952593] WARNING: CPU: 5 PID: 0 at
> /home/ttrantal/git/linux/drivers/gpu/drm/drm_irq.c:1195
> drm_vblank_put+0xc4/0x120 [drm]
> [  249.952596] Modules linked in: fuse tun bridge stp llc af_packet
> pl2303 usbserial shpchp acpi_cpufreq binfmt_misc amdgpu hid_generic
> uhci_hcd radeon 3c59x mii tg3 ehci_pci ehci_hcd i2c_algo_bit
> drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm
> agpgart unix autofs4
> [  249.952675] CPU: 5 PID: 0 Comm: swapper/5 Tainted: GW
> 4.11.0+ #4
> [  249.952678] Hardware name: Hewlett-Packard HP xw6600
> Workstation/0A9Ch, BIOS 786F4 v01.46 09/20/2012
> [  249.952681] task: 88080aea task.stack: c900031b
> [  249.952695] RIP: 0010:drm_vblank_put+0xc4/0x120 [drm]
> [  249.952698] RSP: 0018:88080f003d70 EFLAGS: 00010046
> [  249.952703] RAX:  RBX: 880801d53000 RCX: 
> 
> [  249.952706] RDX:  RSI:  RDI: 
> 88080a4ac000
> [  249.952709] RBP: 88080f003d88 R08: 0001 R09: 
> 0003
> [  249.952711] R10: 88080f003d08 R11: 001da540 R12: 
> 88080a4ac000
> [  249.952714] R13:  R14: 0086 R15: 
> 8808019a
> [  249.952717] FS:  () GS:88080f00()
> knlGS:
> [  249.952720] CS:  0010 DS:  ES:  CR0: 80050033
> [  249.952723] CR2: 7f8bcc3a5810 CR3: 000808789000 CR4: 
> 06e0
> [  249.952726] Call Trace:
> [  249.952731]  
> [  249.952746]  drm_crtc_vblank_put+0x1b/0x30 [drm]
> [  249.952813]  radeon_crtc_handle_flip+0xdc/0x140 [radeon]
> [  249.952843]  si_irq_process+0x610/0x1e90 [radeon]
> [  249.952872]  radeon_driver_irq_handler_kms+0x39/0xc0 [radeon]
> [  249.952881]  __handle_irq_event_percpu+0x60/0x580
> [  249.952887]  handle_irq_event_percpu+0x20/0x90
> [  249.952892]  handle_irq_event+0x46/0xb0
> [  249.952897]  handle_edge_irq+0x13d/0x370
> [  249.952903]  handle_irq+0x66/0x210
> [  249.952908]  ? __local_bh_enable+0x34/0x50
> [  249.952914]  do_IRQ+0x7e/0x1b0
> [  249.952920]  common_interrupt+0x95/0x95

Weird, not sure how this could happen. Can you bisect?


-- 
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 0/3] GPU-DRM-Radeon: Fine-tuning for three function implementations

2017-05-10 Thread Michel Dänzer
On 10/05/17 08:30 PM, Christian König wrote:
> Am 10.05.2017 um 02:23 schrieb Michel Dänzer:
>> On 03/05/17 09:46 PM, Christian König wrote:
>>> Am 02.05.2017 um 22:04 schrieb SF Markus Elfring:
>>>> From: Markus Elfring <elfr...@users.sourceforge.net>
>>>> Date: Tue, 2 May 2017 22:00:02 +0200
>>>>
>>>> Three update suggestions were taken into account
>>>> from static source code analysis.
>>>>
>>>> Markus Elfring (3):
>>>> Use seq_putc() in radeon_sa_bo_dump_debug_info()
>>>> Use seq_puts() in radeon_debugfs_pm_info()
>>>> Use seq_puts() in r100_debugfs_cp_csq_fifo()
>>> Reviewed-by: Christian König <christian.koe...@amd.com>
>> Based on
>> https://lists.freedesktop.org/archives/dri-devel/2017-May/140837.html
>> and followups, I'm afraid we'll have to make sure Markus' patches have
>> been tested adequately before applying them.
> 
> I can't judge the background of that decision, but at least those tree
> patches for radeon looked trivial to me.

Which is part of the issue, see also
https://lists.freedesktop.org/archives/dri-devel/2017-May/140694.html
and other posts in that thread.


-- 
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 1/1] amdgpu: move asic id table to a separate file

2017-05-10 Thread Michel Dänzer
On 11/05/17 06:10 AM, Li, Samuel wrote:
> Also attach a sample ids file for reference. The names are from marketing, not
> related to source code and no reviews necessary here:)

Just because it's not source code doesn't mean no review is necessary. :)


> It can be put in directory /usr/share/libdrm.

What is the canonical location where distros or users building upstream
libdrm can get this file from? There needs to be a good solution for
that before this can land.


-- 
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


[PATCH xf86-video-ati] Remove unused struct members from drmmode_display.h

2017-05-11 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

(Ported from amdgpu commit 462ac3341e5bfbded9086d3d9043821d19352b3e)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 1 -
 src/drmmode_display.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index e2899cf50..d0ecfa855 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2540,7 +2540,6 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, int cpp)
xf86CrtcConfigInit(pScrn, _xf86crtc_config_funcs);
 
drmmode->scrn = pScrn;
-   drmmode->cpp = cpp;
mode_res = drmModeGetResources(drmmode->fd);
if (!mode_res)
return FALSE;
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index df2c4b7bb..db68054a7 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -41,8 +41,6 @@
 
 typedef struct {
   int fd;
-  drmModeFBPtr mode_fb;
-  int cpp;
   struct radeon_bo_manager *bufmgr;
   ScrnInfoPtr scrn;
 #ifdef HAVE_LIBUDEV
-- 
2.11.0

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


[PATCH xf86-video-ati] Update URLs

2017-05-17 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

* Point to the amd-gfx mailing list
* Specify the component in all bugzilla URLs
* Use https:// for all HTML URLs

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 README | 14 +++---
 configure.ac   |  2 +-
 man/radeon.man |  6 +++---
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/README b/README
index 4b92a1858..db982fbed 100644
--- a/README
+++ b/README
@@ -1,25 +1,25 @@
-xf86-video-ati - ATI Radeon video driver for the Xorg X server
+xf86-video-ati - ATI/AMD Radeon video driver for the Xorg X server
 
 All questions regarding this software should be directed at the
-Xorg mailing list:
+amd-gfx mailing list:
 
-http://lists.freedesktop.org/mailman/listinfo/xorg
+https://lists.freedesktop.org/mailman/listinfo/amd-gfx
 
 Please submit bug reports to the Xorg bugzilla:
 
-https://bugs.freedesktop.org/enter_bug.cgi?product=xorg
+
https://bugs.freedesktop.org/enter_bug.cgi?product=xorg=Driver/Radeon
 
 The master development code repository can be found at:
 
 git://anongit.freedesktop.org/git/xorg/driver/xf86-video-ati
 
-http://cgit.freedesktop.org/xorg/driver/xf86-video-ati
+https://cgit.freedesktop.org/xorg/driver/xf86-video-ati
 
 For patch submission instructions, see:
 
-   http://www.x.org/wiki/Development/Documentation/SubmittingPatches
+   https://www.x.org/wiki/Development/Documentation/SubmittingPatches
 
 For more information on the git code manager, see:
 
-http://wiki.x.org/wiki/GitPage
+https://wiki.x.org/wiki/GitPage
 
diff --git a/configure.ac b/configure.ac
index c9ccfb3dc..700e01a5a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -24,7 +24,7 @@
 AC_PREREQ([2.60])
 AC_INIT([xf86-video-ati],
 [7.9.99],
-[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg],
+
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg=Driver/Radeon],
 [xf86-video-ati])
 
 AC_CONFIG_SRCDIR([Makefile.am])
diff --git a/man/radeon.man b/man/radeon.man
index 9334d9e86..3e1723f21 100644
--- a/man/radeon.man
+++ b/man/radeon.man
@@ -392,17 +392,17 @@ __xservername__(__appmansuffix__), 
__xconfigfile__(__filemansuffix__), Xserver(_
 .IP " 1." 4
 Wiki page:
 .RS 4
-http://www.x.org/wiki/radeon
+https://www.x.org/wiki/radeon
 .RE
 .IP " 2." 4
 Overview about radeon development code:
 .RS 4
-http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/
+https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/
 .RE
 .IP " 3." 4
 Mailing list:
 .RS 4
-http://lists.x.org/mailman/listinfo/xorg-driver-ati
+https://lists.freedesktop.org/mailman/listinfo/amd-gfx
 .RE
 .IP " 4." 4
 IRC channel:
-- 
2.11.0

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


[PATCH 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
flag set to CPU visible VRAM with more force.

For other BOs, this gives another chance to stay in VRAM if they
happened to lie in the CPU visible part and another BO needs to go
there.

This should allow BOs to stay in VRAM longer in some cases.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 19 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 27 +++
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 41ee353b22c8..d45c2325c61a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -963,11 +963,20 @@ int amdgpu_bo_fault_reserve_notify(struct 
ttm_buffer_object *bo)
 AMDGPU_GEM_DOMAIN_GTT);
abo->placements[0].lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
 
-   /* Only set GTT as busy placement; if there is no space in CPU visible
-* VRAM, move this BO to GTT instead of evicting other BOs
-*/
-   abo->placement.busy_placement = >placements[1];
-   abo->placement.num_busy_placement = 1;
+   if (abo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+   /* Only set VRAM as normal placement; if there is no space in
+* CPU visible VRAM, evict other BOs, only fall back to GTT as
+* last resort
+*/
+   abo->placement.num_placement = 1;
+   } else {
+   /* Only set GTT as busy placement; if there is no space in CPU
+* visible VRAM, move this BO to GTT instead of evicting other
+* BOs
+*/
+   abo->placement.busy_placement = >placements[1];
+   abo->placement.num_busy_placement = 1;
+   }
 
return ttm_bo_validate(bo, >placement, false, false);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 57789b860768..d5ed85026542 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
adev->mman.buffer_funcs_ring &&
adev->mman.buffer_funcs_ring->ready == false) {
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_CPU);
+   } else if (adev->mc.visible_vram_size < 
adev->mc.real_vram_size) {
+   unsigned fpfn = adev->mc.visible_vram_size >> 
PAGE_SHIFT;
+   struct drm_mm_node *node = bo->mem.mm_node;
+   unsigned long pages_left;
+
+   for (pages_left = bo->mem.num_pages;
+pages_left;
+pages_left -= node->size, node++) {
+   if (node->start < fpfn)
+   break;
+   }
+
+   if (!pages_left)
+   goto gtt;
+
+   /* Try evicting to the CPU inaccessible part of VRAM
+* first, but only set GTT as busy placement, so this
+* BO will be evicted to GTT rather than causing other
+* BOs to be evicted from VRAM
+*/
+   amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_VRAM |
+AMDGPU_GEM_DOMAIN_GTT);
+   abo->placements[0].fpfn = fpfn;
+   abo->placements[0].lpfn = 0;
+   abo->placement.busy_placement = >placements[1];
+   abo->placement.num_busy_placement = 1;
} else {
+gtt:
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_GTT);
/* Set an upper limit to force directly allocating
 * address space for the BO.
-- 
2.11.0

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


Re: [PATCH 1/3] drm/amdgpu: Drop useless loops for placement restrictions

2017-05-18 Thread Michel Dänzer
On 18/05/17 06:17 PM, Christian König wrote:
> Am 18.05.2017 um 11:08 schrieb Michel Dänzer:
>> From: Michel Dänzer <michel.daen...@amd.com>
>>
>> We know how the placements were initialized in these cases, so we can
>> set the restrictions directly without a loop.
>>
>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>

[...]

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>> index 2ca09f111f08..60688fa5ef98 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
>> @@ -375,11 +375,7 @@ void amdgpu_uvd_free_handles(struct amdgpu_device
>> *adev, struct drm_file *filp)
>> static void amdgpu_uvd_force_into_uvd_segment(struct amdgpu_bo *abo)
>>   {
>> -int i;
>> -for (i = 0; i < abo->placement.num_placement; ++i) {
>> -abo->placements[i].fpfn = 0 >> PAGE_SHIFT;
>> -abo->placements[i].lpfn = (256 * 1024 * 1024) >> PAGE_SHIFT;
>> -}
>> +abo->placements[0].lpfn = (256 * 1024 * 1024) >> PAGE_SHIFT;
> 
> This is not correct. The restriction applies to all placements, not only
> the first one.

I see, there can be multiple placements in amdgpu_uvd_cs_pass1, if cmd
!= 0x0 and != 0x3? I'll drop this hunk then, thanks.


-- 
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


[PATCH 0/3] drm/amdgpu: Tweaks for high pressure on CPU visible VRAM

2017-05-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

This series was developed and tested under the following scenario:

Running the PTS dirt-rally benchmark (1920x1080, Ultra) on Tonga with
2G, with CPU visible VRAM artificially restricted to 64 MB.

Without this series, there's a lot of stutter during about the first
minute of a benchmark run. During this time there are significant amounts
of buffer moves (starting from about 500 MB on the HUD) and evictions,
gradually declining until the buffer moves settle around 8 MB on the HUD.

With this series, there's only slight stutter during the first seconds
after the car launches, even though the buffer move volume is about the
same as without the series. Buffer evictions are eliminated almost
completely, except for a few at the beginning. Buffer moves still settle
around 8 MB on the HUD, but with less variance than before.

Note that there's only little if any improvement of the average framerate
reported, but the minimum framerate as seen on the HUD goes from ~10 fps
to ~17.


Patch 1 is a cleanup that I noticed along the way.

Patch 2 makes the main difference for the above scenario.

Patch 3 doesn't make as much difference, I'm fine with it not landing at
least for now.

Michel Dänzer (3):
  drm/amdgpu: Drop useless loops for placement restrictions
  drm/amdgpu: Don't evict other BOs from VRAM for page faults
  drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 42 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 46 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c|  6 +---
 3 files changed, 51 insertions(+), 43 deletions(-)

-- 
2.11.0

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


[PATCH 1/3] drm/amdgpu: Drop useless loops for placement restrictions

2017-05-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

We know how the placements were initialized in these cases, so we can
set the restrictions directly without a loop.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 19 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c|  6 +-
 3 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 365883d7948d..b3252bc8fe51 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -939,8 +939,8 @@ int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object 
*bo)
 {
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
struct amdgpu_bo *abo;
-   unsigned long offset, size, lpfn;
-   int i, r;
+   unsigned long offset, size;
+   int r;
 
if (!amdgpu_ttm_bo_is_amdgpu_bo(bo))
return 0;
@@ -961,14 +961,7 @@ int amdgpu_bo_fault_reserve_notify(struct 
ttm_buffer_object *bo)
 
/* hurrah the memory is not visible ! */
amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_VRAM);
-   lpfn =  adev->mc.visible_vram_size >> PAGE_SHIFT;
-   for (i = 0; i < abo->placement.num_placement; i++) {
-   /* Force into visible VRAM */
-   if ((abo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
-   (!abo->placements[i].lpfn ||
-abo->placements[i].lpfn > lpfn))
-   abo->placements[i].lpfn = lpfn;
-   }
+   abo->placements[0].lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
r = ttm_bo_validate(bo, >placement, false, false);
if (unlikely(r == -ENOMEM)) {
amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_GTT);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 5db0230e45c6..57789b860768 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -191,7 +191,6 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
.lpfn = 0,
.flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM
};
-   unsigned i;
 
if (!amdgpu_ttm_bo_is_amdgpu_bo(bo)) {
placement->placement = 
@@ -209,20 +208,10 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_CPU);
} else {
amdgpu_ttm_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_GTT);
-   for (i = 0; i < abo->placement.num_placement; ++i) {
-   if (!(abo->placements[i].flags &
- TTM_PL_FLAG_TT))
-   continue;
-
-   if (abo->placements[i].lpfn)
-   continue;
-
-   /* set an upper limit to force directly
-* allocating address space for the BO.
-*/
-   abo->placements[i].lpfn =
-   adev->mc.gtt_size >> PAGE_SHIFT;
-   }
+   /* Set an upper limit to force directly allocating
+* address space for the BO.
+*/
+   abo->placements[0].lpfn = adev->mc.gtt_size >> 
PAGE_SHIFT;
}
break;
case TTM_PL_TT:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index 2ca09f111f08..60688fa5ef98 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -375,11 +375,7 @@ void amdgpu_uvd_free_handles(struct amdgpu_device *adev, 
struct drm_file *filp)
 
 static void amdgpu_uvd_force_into_uvd_segment(struct amdgpu_bo *abo)
 {
-   int i;
-   for (i = 0; i < abo->placement.num_placement; ++i) {
-   abo->placements[i].fpfn = 0 >> PAGE_SHIFT;
-   abo->placements[i].lpfn = (256 * 1024 * 1024) >> PAGE_SHIFT;
-   }
+   abo->placements[0].lpfn = (256 * 1024 * 1024) >> PAGE_SHIFT;
 }
 
 static u64 amdgpu_uvd_get_addr_from_ctx(struct amdgpu_uvd_cs_ctx *ctx)
-- 
2.11.0

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


[PATCH 2/3] drm/amdgpu: Don't evict other BOs from VRAM for page faults

2017-05-18 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

If there is no free space in CPU visible VRAM for the faulting BO, move
it to GTT instead of evicting other BOs from CPU visible VRAM.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 22 --
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index b3252bc8fe51..41ee353b22c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -940,7 +940,6 @@ int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object 
*bo)
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
struct amdgpu_bo *abo;
unsigned long offset, size;
-   int r;
 
if (!amdgpu_ttm_bo_is_amdgpu_bo(bo))
return 0;
@@ -960,22 +959,17 @@ int amdgpu_bo_fault_reserve_notify(struct 
ttm_buffer_object *bo)
return -EINVAL;
 
/* hurrah the memory is not visible ! */
-   amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_VRAM);
+   amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_VRAM |
+AMDGPU_GEM_DOMAIN_GTT);
abo->placements[0].lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
-   r = ttm_bo_validate(bo, >placement, false, false);
-   if (unlikely(r == -ENOMEM)) {
-   amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_GTT);
-   return ttm_bo_validate(bo, >placement, false, false);
-   } else if (unlikely(r != 0)) {
-   return r;
-   }
 
-   offset = bo->mem.start << PAGE_SHIFT;
-   /* this should never happen */
-   if ((offset + size) > adev->mc.visible_vram_size)
-   return -EINVAL;
+   /* Only set GTT as busy placement; if there is no space in CPU visible
+* VRAM, move this BO to GTT instead of evicting other BOs
+*/
+   abo->placement.busy_placement = >placements[1];
+   abo->placement.num_busy_placement = 1;
 
-   return 0;
+   return ttm_bo_validate(bo, >placement, false, false);
 }
 
 /**
-- 
2.11.0

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


Re: Plan: BO move throttling for visible VRAM evictions

2017-05-18 Thread Michel Dänzer
On 17/05/17 09:35 PM, Marek Olšák wrote:
> On May 16, 2017 3:57 AM, "Michel Dänzer" <mic...@daenzer.net
> <mailto:mic...@daenzer.net>> wrote:
> On 15/05/17 07:11 PM, Marek Olšák wrote:
> > On May 15, 2017 4:29 AM, "Michel Dänzer" <mic...@daenzer.net
> <mailto:mic...@daenzer.net>
> > <mailto:mic...@daenzer.net <mailto:mic...@daenzer.net>>> wrote:
> >
> > I think the next step should be to make radeonsi keep track of
> how much
> > VRAM it's trying to use that's expected to be accessed by the
> CPU, and
> > to use GTT instead when that exceeds a threshold (probably
> derived from
> > vram_vis_size).
> >
> > That's difficult to estimate. There are apps with 600MB of mapped VRAM
> > and don't experience any performance issues. And some apps with
> 300MB of
> > mapped VRAM do. It only depends on the CPU access pattern, not what
> > radeonsi sees.
> 
> What I mean is keeping track of the total size of resources which have
> RADEON_DOMAIN_VRAM and RADEON_FLAG_CPU_ACCESS set, and if it exceeds a
> threshold, create new ones having those flags in GTT instead. Even
> though this might not be strictly necessary with amdgpu in the long run,
> it probably is for radeon anyway, and in the short term it might help
> even with amdgpu.
> 
> 
> That might hurt us more than it can help.

You may be right, but I think I'll play with that idea a little anyway
to see how it goes. :)

> All mappable buffers have the CPU access flag set, but many of them are
> immutable.

You mean they're only written to once by the CPU? We shouldn't set the
RADEON_FLAG_CPU_ACCESS flag for BOs where we expect that, because it
will currently prevent them from being in the CPU invisible part of VRAM.


> The only place where this can be handled​ is the kernel.

Ideally, the placement of a BO should be determined based on how it's
actually being used by the GPU vs CPU. But I'm not sure how to determine
that in a useful way.

> Even if it's as simple as: if (bo->numcpufaults > 10) domain = GTT_WC;

I'm skeptical about the number of CPU page faults per se being a useful
metric. It doesn't tell us much about how the BO is used even by the
CPU, let alone the GPU. But let's see where this leads you.


One thing that might help would be if we could swap individual memory
nodes between visible and invisible VRAM for CPU page faults, instead of
moving/evicting whole BOs. Christian, do you think something like that
would be possible?


Another idea (to avoid issues such as the recent one with Rocket League)
was to make VRAM CPU mappings write-only, and move the BO to GTT if
there's a read fault. But not sure if this is possible at all, or how
much effort it would be.


-- 
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 4/4] drm/amdgpu: reset fpriv vram_lost_counter

2017-05-16 Thread Michel Dänzer
On 17/05/17 12:04 PM, zhoucm1 wrote:
> On 2017年05月17日 09:18, Michel Dänzer wrote:
>> On 16/05/17 06:25 PM, Chunming Zhou wrote:
>>> Change-Id: I8eb6d7f558da05510e429d3bf1d48c8cec6c1977
>>> Signed-off-by: Chunming Zhou <david1.z...@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +++
>>>   include/uapi/drm/amdgpu_drm.h  | 1 +
>>>   2 files changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index bca1fb5..f3e7525 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2547,6 +2547,9 @@ int amdgpu_vm_ioctl(struct drm_device *dev,
>>> void *data, struct drm_file *filp)
>>>   case AMDGPU_VM_OP_UNRESERVE_VMID:
>>>   amdgpu_vm_free_reserved_vmid(adev, >vm, AMDGPU_GFXHUB);
>>>   break;
>>> +case AMDGPU_VM_OP_RESET:
>>> +fpriv->vram_lost_counter =
>>> atomic_read(>vram_lost_counter);
>>> +break;
>> How do you envision the UMDs using this? I can mostly think of them
>> calling this ioctl when a context is created or destroyed. But that
>> would also allow any other remaining contexts using the same DRM file
>> descriptor to use all ioctls again. So, I think there needs to be a
>> vram_lost_counter in struct amdgpu_ctx instead of in struct amdgpu_fpriv.
> struct amdgpu_fpriv for vram_lost_counter is proper place, especially
> for ioctl return value.
> if you need to reset ctx one by one, we can mark all contexts of that
> vm, and then reset by userspace.

I'm not following. With vram_lost_counter in amdgpu_fpriv, if any
context calls this ioctl, all other contexts using the same file
descriptor will also be considered safe again, right? So I'm still not
sure how this is supposed to be used by the UMDs. Can you describe your
idea for that?


>> It's hard to be sure whether that's workable for the UMD without at
>> least a working prototype...
> Totally agree, if you can help to do this in userspace, I'd like to
> support you from kernel side, or Christian.

I'm busy with other stuff.


-- 
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


[PATCH xf86-video-amdgpu] Use plain glamor_egl_create_textured_screen().

2017-05-17 Thread Michel Dänzer
From: Eric Anholt <e...@anholt.net>

Since 5064ffab631 (2014), glamor's implementation of _ext just drops the
back_pixmap arg, which we were passing NULL (the default) to anyway.

Signed-off-by: Eric Anholt <e...@anholt.net>
(Ported from radeon commit 2b7d77b90108911777a11ecaa63435552000c958)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_glamor.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/amdgpu_glamor.c b/src/amdgpu_glamor.c
index 1c5dfc2d1..5583cd382 100644
--- a/src/amdgpu_glamor.c
+++ b/src/amdgpu_glamor.c
@@ -66,10 +66,9 @@ Bool amdgpu_glamor_create_screen_resources(ScreenPtr screen)
 #endif
 
if (!amdgpu_bo_get_handle(info->front_buffer, _handle) ||
-   !glamor_egl_create_textured_screen_ext(screen,
-  bo_handle,
-  scrn->displayWidth *
-  info->pixel_bytes, NULL)) {
+   !glamor_egl_create_textured_screen(screen, bo_handle,
+  scrn->displayWidth *
+  info->pixel_bytes)) {
return FALSE;
}
 
-- 
2.11.0

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


Re: [PATCH 4/4] drm/amdgpu: reset fpriv vram_lost_counter

2017-05-17 Thread Michel Dänzer
On 17/05/17 01:28 PM, zhoucm1 wrote:
> On 2017年05月17日 11:15, Michel Dänzer wrote:
>> On 17/05/17 12:04 PM, zhoucm1 wrote:
>>> On 2017年05月17日 09:18, Michel Dänzer wrote:
>>>> On 16/05/17 06:25 PM, Chunming Zhou wrote:
>>>>> Change-Id: I8eb6d7f558da05510e429d3bf1d48c8cec6c1977
>>>>> Signed-off-by: Chunming Zhou <david1.z...@amd.com>
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index bca1fb5..f3e7525 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -2547,6 +2547,9 @@ int amdgpu_vm_ioctl(struct drm_device *dev,
>>>>> void *data, struct drm_file *filp)
>>>>>case AMDGPU_VM_OP_UNRESERVE_VMID:
>>>>>amdgpu_vm_free_reserved_vmid(adev, >vm,
>>>>> AMDGPU_GFXHUB);
>>>>>break;
>>>>> +case AMDGPU_VM_OP_RESET:
>>>>> +fpriv->vram_lost_counter =
>>>>> atomic_read(>vram_lost_counter);
>>>>> +break;
>>>> How do you envision the UMDs using this? I can mostly think of them
>>>> calling this ioctl when a context is created or destroyed. But that
>>>> would also allow any other remaining contexts using the same DRM file
>>>> descriptor to use all ioctls again. So, I think there needs to be a
>>>> vram_lost_counter in struct amdgpu_ctx instead of in struct
>>>> amdgpu_fpriv.
>>> struct amdgpu_fpriv for vram_lost_counter is proper place, especially
>>> for ioctl return value.
>>> if you need to reset ctx one by one, we can mark all contexts of that
>>> vm, and then reset by userspace.
>> I'm not following. With vram_lost_counter in amdgpu_fpriv, if any
>> context calls this ioctl, all other contexts using the same file
>> descriptor will also be considered safe again, right?
> Yes, but it really depends on userspace requirement, if you need to
> reset ctx one by one, we can mark all contexts of that vm to guilty, and
> then reset one context by userspace.

Still not sure what you mean by that.

E.g. what do you mean by "guilty"? I thought that refers to the context
which caused a hang. But it seems like you're using it to refer to any
context which hasn't reacted yet to VRAM contents being lost.

Also not sure what you mean by "if you need to reset ctx one by one".


-- 
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 4/4] drm/amdgpu: reset fpriv vram_lost_counter

2017-05-17 Thread Michel Dänzer
On 17/05/17 04:13 PM, zhoucm1 wrote:
> On 2017年05月17日 14:57, Michel Dänzer wrote:
>> On 17/05/17 01:28 PM, zhoucm1 wrote:
>>> On 2017年05月17日 11:15, Michel Dänzer wrote:
>>>> On 17/05/17 12:04 PM, zhoucm1 wrote:
>>>>> On 2017年05月17日 09:18, Michel Dänzer wrote:
>>>>>> On 16/05/17 06:25 PM, Chunming Zhou wrote:
>>>>>>> Change-Id: I8eb6d7f558da05510e429d3bf1d48c8cec6c1977
>>>>>>> Signed-off-by: Chunming Zhou <david1.z...@amd.com>
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> index bca1fb5..f3e7525 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> @@ -2547,6 +2547,9 @@ int amdgpu_vm_ioctl(struct drm_device *dev,
>>>>>>> void *data, struct drm_file *filp)
>>>>>>> case AMDGPU_VM_OP_UNRESERVE_VMID:
>>>>>>> amdgpu_vm_free_reserved_vmid(adev, >vm,
>>>>>>> AMDGPU_GFXHUB);
>>>>>>> break;
>>>>>>> +case AMDGPU_VM_OP_RESET:
>>>>>>> +fpriv->vram_lost_counter =
>>>>>>> atomic_read(>vram_lost_counter);
>>>>>>> +break;
>>>>>> How do you envision the UMDs using this? I can mostly think of them
>>>>>> calling this ioctl when a context is created or destroyed. But that
>>>>>> would also allow any other remaining contexts using the same DRM file
>>>>>> descriptor to use all ioctls again. So, I think there needs to be a
>>>>>> vram_lost_counter in struct amdgpu_ctx instead of in struct
>>>>>> amdgpu_fpriv.
>>>>> struct amdgpu_fpriv for vram_lost_counter is proper place, especially
>>>>> for ioctl return value.
>>>>> if you need to reset ctx one by one, we can mark all contexts of that
>>>>> vm, and then reset by userspace.
>>>> I'm not following. With vram_lost_counter in amdgpu_fpriv, if any
>>>> context calls this ioctl, all other contexts using the same file
>>>> descriptor will also be considered safe again, right?
>>> Yes, but it really depends on userspace requirement, if you need to
>>> reset ctx one by one, we can mark all contexts of that vm to guilty, and
>>> then reset one context by userspace.
>> Still not sure what you mean by that.
>>
>> E.g. what do you mean by "guilty"? I thought that refers to the context
>> which caused a hang. But it seems like you're using it to refer to any
>> context which hasn't reacted yet to VRAM contents being lost.
> When vram is lost, we treat all contexts need to reset.

Essentially, your patches only track VRAM contents being lost per file
descriptor, not per context. I'm not sure (rather skeptical) that this
is suitable for OpenGL UMDs, since state is usually tracked per context.
Marek / Nicolai?


-- 
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 4/4] drm/amdgpu: reset fpriv vram_lost_counter

2017-05-17 Thread Michel Dänzer
On 17/05/17 05:46 PM, zhoucm1 wrote:
> 
> 
> On 2017年05月17日 16:40, Christian König wrote:
>> Am 17.05.2017 um 10:01 schrieb Michel Dänzer:
>>> On 17/05/17 04:13 PM, zhoucm1 wrote:
>>>> On 2017年05月17日 14:57, Michel Dänzer wrote:
>>>>> On 17/05/17 01:28 PM, zhoucm1 wrote:
>>>>>> On 2017年05月17日 11:15, Michel Dänzer wrote:
>>>>>>> On 17/05/17 12:04 PM, zhoucm1 wrote:
>>>>>>>> On 2017年05月17日 09:18, Michel Dänzer wrote:
>>>>>>>>> On 16/05/17 06:25 PM, Chunming Zhou wrote:
>>>>>>>>>> Change-Id: I8eb6d7f558da05510e429d3bf1d48c8cec6c1977
>>>>>>>>>> Signed-off-by: Chunming Zhou <david1.z...@amd.com>
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> index bca1fb5..f3e7525 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> @@ -2547,6 +2547,9 @@ int amdgpu_vm_ioctl(struct drm_device *dev,
>>>>>>>>>> void *data, struct drm_file *filp)
>>>>>>>>>>  case AMDGPU_VM_OP_UNRESERVE_VMID:
>>>>>>>>>>  amdgpu_vm_free_reserved_vmid(adev, >vm,
>>>>>>>>>> AMDGPU_GFXHUB);
>>>>>>>>>>  break;
>>>>>>>>>> +case AMDGPU_VM_OP_RESET:
>>>>>>>>>> +fpriv->vram_lost_counter =
>>>>>>>>>> atomic_read(>vram_lost_counter);
>>>>>>>>>> +break;
>>>>>>>>> How do you envision the UMDs using this? I can mostly think of
>>>>>>>>> them
>>>>>>>>> calling this ioctl when a context is created or destroyed. But
>>>>>>>>> that
>>>>>>>>> would also allow any other remaining contexts using the same
>>>>>>>>> DRM file
>>>>>>>>> descriptor to use all ioctls again. So, I think there needs to
>>>>>>>>> be a
>>>>>>>>> vram_lost_counter in struct amdgpu_ctx instead of in struct
>>>>>>>>> amdgpu_fpriv.
>>>>>>>> struct amdgpu_fpriv for vram_lost_counter is proper place,
>>>>>>>> especially
>>>>>>>> for ioctl return value.
>>>>>>>> if you need to reset ctx one by one, we can mark all contexts of
>>>>>>>> that
>>>>>>>> vm, and then reset by userspace.
>>>>>>> I'm not following. With vram_lost_counter in amdgpu_fpriv, if any
>>>>>>> context calls this ioctl, all other contexts using the same file
>>>>>>> descriptor will also be considered safe again, right?
>>>>>> Yes, but it really depends on userspace requirement, if you need to
>>>>>> reset ctx one by one, we can mark all contexts of that vm to
>>>>>> guilty, and
>>>>>> then reset one context by userspace.
>>>>> Still not sure what you mean by that.
>>>>>
>>>>> E.g. what do you mean by "guilty"? I thought that refers to the
>>>>> context
>>>>> which caused a hang. But it seems like you're using it to refer to any
>>>>> context which hasn't reacted yet to VRAM contents being lost.
>>>> When vram is lost, we treat all contexts need to reset.
>>> Essentially, your patches only track VRAM contents being lost per file
>>> descriptor, not per context. I'm not sure (rather skeptical) that this
>>> is suitable for OpenGL UMDs, since state is usually tracked per context.
>>> Marek / Nicolai?
>>
>> Oh, yeah that's a good point.
>>
>> The problem with tracking it per context is that Vulkan also wants the
>> ENODEV on the amdgpu_gem_va_ioct() and amdgpu_info_ioctl() which are
>> context less.
>>
>> But thinking more about this blocking those two doesn't make much
>> sense. The VM content can be restored and why should be disallow
>> reading GPU info?
> I can re-paste the Vulkan APIs requiring ENODEV:
> "
> 
> The Vulkan APIs listed below could return VK_ERROR_DEVICE_LOST according
> to the spec.
> 
> I tries to provide a list of u/k interfaces that could be called for
> each vk API.
> 
>  
> 
> vkCreateDevice
> 
> -  amdgpu_device_initialize.
> -  amdgpu_query_gpu_info

[...]

> vkQueueBindSparse**
> 
> amdgpu_bo_va_op
> amdgpu_bo_va_op_raw

So these *can* return VK_ERROR_DEVICE_LOST, but do they *have to* do
that after a reset? :)


-- 
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


[PATCH xf86-video-ati] Apply gamma correction to HW cursor

2017-05-10 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

The display hardware CLUT we're currently using for gamma correction
doesn't affect the HW cursor, so we have to apply it manually when
uploading the HW cursor data.

This currently only works in depth 24/32.

(Ported from amdgpu commit 82fa615f38137add75f9cd4bb49c48dd88de916f)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 66 ---
 1 file changed, 58 insertions(+), 8 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 84e7ef967..0b823754b 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -812,6 +812,17 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
}
 }
 
+static void
+drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
+ uint16_t *blue, int size)
+{
+   drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+
+   drmModeCrtcSetGamma(drmmode_crtc->drmmode->fd,
+   drmmode_crtc->mode_crtc->crtc_id, size, red, green,
+   blue);
+}
+
 static Bool
 drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 Rotation rotation, int x, int y)
@@ -873,8 +884,8 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr 
mode,
if (drmmode_crtc->tear_free)
scanout_id = drmmode_crtc->scanout_id;
 
-   crtc->funcs->gamma_set(crtc, crtc->gamma_red, crtc->gamma_green,
-  crtc->gamma_blue, crtc->gamma_size);
+   drmmode_crtc_gamma_do_set(crtc, crtc->gamma_red, 
crtc->gamma_green,
+ crtc->gamma_blue, crtc->gamma_size);
 
drmmode_ConvertToKMode(crtc->scrn, , mode);
 
@@ -1043,6 +1054,31 @@ drmmode_cursor_src_offset(Rotation rotation, int width, 
int height,
 
 #endif
 
+static uint32_t
+drmmode_cursor_gamma(xf86CrtcPtr crtc, uint32_t argb)
+{
+   uint32_t alpha = argb >> 24;
+   uint32_t rgb[3];
+   int i;
+
+   if (!alpha)
+   return 0;
+
+   if (crtc->scrn->depth != 24 && crtc->scrn->depth != 32)
+   return argb;
+
+   /* Un-premultiply alpha */
+   for (i = 0; i < 3; i++)
+   rgb[i] = ((argb >> (i * 8)) & 0xff) * 0xff / alpha;
+
+   /* Apply gamma correction and pre-multiply alpha */
+   rgb[0] = (crtc->gamma_blue[rgb[0]] >> 8) * alpha / 0xff;
+   rgb[1] = (crtc->gamma_green[rgb[1]] >> 8) * alpha / 0xff;
+   rgb[2] = (crtc->gamma_red[rgb[2]] >> 8) * alpha / 0xff;
+
+   return alpha << 24 | rgb[2] << 16 | rgb[1] << 8 | rgb[0];
+}
+
 static void
 drmmode_load_cursor_argb (xf86CrtcPtr crtc, CARD32 *image)
 {
@@ -1068,7 +1104,8 @@ drmmode_load_cursor_argb (xf86CrtcPtr crtc, CARD32 *image)
  dstx, 
dsty);
 
ptr[dsty * info->cursor_w + dstx] =
-   cpu_to_le32(image[srcoffset]);
+   cpu_to_le32(drmmode_cursor_gamma(crtc,
+
image[srcoffset]));
}
}
} else
@@ -1078,7 +1115,7 @@ drmmode_load_cursor_argb (xf86CrtcPtr crtc, CARD32 *image)
int i;
 
for (i = 0; i < cursor_size; i++)
-   ptr[i] = cpu_to_le32(image[i]);
+   ptr[i] = cpu_to_le32(drmmode_cursor_gamma(crtc, 
image[i]));
}
 }
 
@@ -1209,11 +1246,24 @@ static void
 drmmode_crtc_gamma_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
   uint16_t *blue, int size)
 {
-   drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-   drmmode_ptr drmmode = drmmode_crtc->drmmode;
+   ScrnInfoPtr scrn = crtc->scrn;
+   xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
+   RADEONInfoPtr info = RADEONPTR(scrn);
+   int i;
 
-   drmModeCrtcSetGamma(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
-   size, red, green, blue);
+   drmmode_crtc_gamma_do_set(crtc, red, green, blue, size);
+
+   /* Compute index of this CRTC into xf86_config->crtc */
+   for (i = 0; xf86_config->crtc[i] != crtc; i++) {}
+
+   if (info->hwcursor_disabled & (1 << i))
+   return;
+
+#ifdef HAVE_XF86_CURSOR_RESET_CURSOR
+   xf86CursorResetCursor(scrn->pScreen);
+#else
+   xf86_reload_cursors(scrn->pScreen);
+#endif
 }
 
 #ifdef RADEON_PIXMAP_SHARING
-- 
2.11.0

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


Re: [PATCH 2/4] drm/radeon: Allow vblank_disable_immediate.

2017-06-21 Thread Michel Dänzer
On 21/06/17 10:44 AM, Mario Kleiner wrote:
> With instantaneous high precision vblank timestamping
> that updates at leading edge of vblank, a cooked hw
> vblank counter which increments at leading edge of
> vblank, and reliable page flip execution and completion
> at leading edge of vblank, we should meet the requirements
> for fast/immediate vblank irq disable/enable.
> 
> Testing on Linux-4.12-rc5 + drm-next on a Radeon HD 5770
> (DCE 4) with timing measurement equipment indicates this
> works fine, so allow immediate vblank disable for power
> saving.
> 
> For debugging in case of unexpected trouble, booting
> with kernel cmdline option drm.vblankoffdelay=0
> (or echo 0 > /sys/module/drm/parameters/vblankoffdelay)
> would keep vblank irqs permanently on to approximate old
> behavior.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner...@gmail.com>
> Cc: Alex Deucher <alexander.deuc...@amd.com>
> Cc: Michel Dänzer <michel.daen...@amd.com>

My only doubt is whether this is also reliable on older (e.g. pre-R600)
GPUs. For newer GPUs (tested on Kaveri):

Reviewed-and-Tested-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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: [Nouveau] [PATCH 01/11] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set

2017-06-21 Thread Michel Dänzer
On 21/06/17 05:14 PM, Daniel Vetter wrote:
> On Wed, Jun 21, 2017 at 04:59:31PM +0900, Michel Dänzer wrote:
>> On 21/06/17 04:38 PM, Daniel Vetter wrote:
>>> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote:
>>>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
>>>> totally obsolete.
>>>>
>>>> I think the gamma_store can end up invalid on error. But the way I read
>>>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
>>>> this pesky legacy fbdev stuff be any better?
>>>>
>>>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However,
>>>> it saves it to the gamma_store which should already be up to date with what
>>>> .gamma_get would return and is thus a nop. So, zap it.
>>>
>>> Removing drm_fb_helper_save_lut_atomic should be a separate patch I
>>> think.
>>>  
>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>
>> [...]
>>
>>>> @@ -1167,50 +1150,6 @@ void drm_fb_helper_set_suspend_unlocked(struct 
>>>> drm_fb_helper *fb_helper,
>>>>  }
>>>>  EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>>>>  
>>>> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
>>>> -   u16 blue, u16 regno, struct fb_info *info)
>>>> -{
>>>> -  struct drm_fb_helper *fb_helper = info->par;
>>>> -  struct drm_framebuffer *fb = fb_helper->fb;
>>>> -
>>>> -  if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
>>>
>>> This case here seems gone, and it was also the pièce de résistance when I
>>> tried tackling fbdev lut support. As far as I understand this stuff we do
>>> not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is
>>> pointless. But I'm honestly not really clear.
>>>
>>> I think removing this case should also be a separate patch, and I'd very
>>> much prefer that someone with some fbdev-clue would ack it.
>>
>> No need for anyone with fbdev-clue, just run fbset -i. :)

Adding Bartlomiej and the linux-fbdev list, just in case I'm wrong below.


>> fbcon uses a TRUECOLOR visual at depths > 8.
> 
> Then I understand even less what exactly this code does ... Should we keep
> it?

I think we need it. It updates the entries of info->pseudo_palette,
which is kind of a pseudocolor palette mapping 16 indices to pixel
values to write to the framebuffer.

If the setcolreg hook is removed, the setcmap hook needs to do this instead.


-- 
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 3/4] drm/amdgpu: Allow vblank_disable_immediate.

2017-06-21 Thread Michel Dänzer
On 21/06/17 10:44 AM, Mario Kleiner wrote:
> With instantaneous high precision vblank timestamping
> that updates at leading edge of vblank, a cooked hw
> vblank counter which increments at leading edge of
> vblank, and reliable page flip execution and completion
> at leading edge of vblank, we should meet the requirements
> for fast/immediate vblank irq disable/enable.
> 
> Testing on Linux-4.12-rc5 + drm-next on a Radeon R9 380
> Tonga Pro (DCE 10) with timing measurement equipment
> indicates this works fine, so allow immediate vblank
> disable for power saving.
> 
> For debugging in case of unexpected trouble, booting
> with kernel cmdline option drm.vblankoffdelay=0
> (or echo 0 > /sys/module/drm/parameters/vblankoffdelay)
> would keep vblank irqs permanently on to approximate old
> behavior.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner...@gmail.com>
> Cc: Alex Deucher <alexander.deuc...@amd.com>
> Cc: Michel Dänzer <michel.daen...@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 62da6c5..a28f8aa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -220,6 +220,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>   int r = 0;
>  
>   spin_lock_init(>irq.lock);
> +
> + /* Disable vblank irqs aggressively for power-saving */
> + adev->ddev->vblank_disable_immediate = true;
> +
>   r = drm_vblank_init(adev->ddev, adev->mode_info.num_crtc);
>   if (r) {
>   return r;
> 

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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 01/11] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set

2017-06-21 Thread Michel Dänzer
On 21/06/17 04:38 PM, Daniel Vetter wrote:
> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote:
>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
>> totally obsolete.
>>
>> I think the gamma_store can end up invalid on error. But the way I read
>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
>> this pesky legacy fbdev stuff be any better?
>>
>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However,
>> it saves it to the gamma_store which should already be up to date with what
>> .gamma_get would return and is thus a nop. So, zap it.
> 
> Removing drm_fb_helper_save_lut_atomic should be a separate patch I
> think.
>  
>> Signed-off-by: Peter Rosin <p...@axentia.se>

[...]

>> @@ -1167,50 +1150,6 @@ void drm_fb_helper_set_suspend_unlocked(struct 
>> drm_fb_helper *fb_helper,
>>  }
>>  EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>>  
>> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
>> - u16 blue, u16 regno, struct fb_info *info)
>> -{
>> -struct drm_fb_helper *fb_helper = info->par;
>> -struct drm_framebuffer *fb = fb_helper->fb;
>> -
>> -if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> 
> This case here seems gone, and it was also the pièce de résistance when I
> tried tackling fbdev lut support. As far as I understand this stuff we do
> not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is
> pointless. But I'm honestly not really clear.
> 
> I think removing this case should also be a separate patch, and I'd very
> much prefer that someone with some fbdev-clue would ack it.

No need for anyone with fbdev-clue, just run fbset -i. :) fbcon uses a
TRUECOLOR visual at depths > 8.


> It's a pre-existing bug, but should we also try to restore the fbdev lut
> in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug,
> but might be relevant for your use-case. Just try to run both an fbdev
> application and some kms-native thing, and then SIGKILL the native kms
> app.
> 
> But since pre-existing not really required, and probably too much effort.

I suspect something like that indeed needs to be done though. E.g.
killing Xorg results in fbcon continuing to use the LUT set by Xorg.


-- 
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


[PATCH xf86-video-amdgpu] Improve drmmode_fb_reference debugging code

2017-06-22 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

If a reference count is <= 0, call FatalError with the call location
(in case it doesn't get resolved in the backtrace printed by
FatalError).

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.h | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index b64a938cd..f351bb09c 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -133,32 +133,36 @@ enum drmmode_flip_sync {
 
 
 static inline void
-drmmode_fb_reference(int drm_fd, struct drmmode_fb **old, struct drmmode_fb 
*new)
+drmmode_fb_reference_loc(int drm_fd, struct drmmode_fb **old, struct 
drmmode_fb *new,
+const char *caller, unsigned line)
 {
if (new) {
if (new->refcnt <= 0) {
-   ErrorF("New FB's refcnt was %d in %s\n", new->refcnt,
-  __func__);
-   } else {
-   new->refcnt++;
+   FatalError("New FB's refcnt was %d at %s:%u",
+  new->refcnt, caller, line);
}
+
+   new->refcnt++;
}
 
if (*old) {
if ((*old)->refcnt <= 0) {
-   ErrorF("Old FB's refcnt was %d in %s\n",
-  (*old)->refcnt, __func__);
-   } else {
-   if (--(*old)->refcnt == 0) {
-   drmModeRmFB(drm_fd, (*old)->handle);
-   free(*old);
-   }
+   FatalError("Old FB's refcnt was %d at %s:%u",
+  (*old)->refcnt, caller, line);
+   }
+
+   if (--(*old)->refcnt == 0) {
+   drmModeRmFB(drm_fd, (*old)->handle);
+   free(*old);
}
}
 
*old = new;
 }
 
+#define drmmode_fb_reference(fd, old, new) \
+   drmmode_fb_reference_loc(fd, old, new, __func__, __LINE__)
+
 
 extern int drmmode_page_flip_target_absolute(AMDGPUEntPtr pAMDGPUEnt,
 drmmode_crtc_private_ptr 
drmmode_crtc,
-- 
2.11.0

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


Re: [PATCH 2/3] drm/amdgpu: change a function to static function

2017-06-21 Thread Michel Dänzer
On 22/06/17 11:42 AM, Alex Xie wrote:
> The function is called only once inside the .c file.
> 
> Signed-off-by: Alex Xie <alexbin@amd.com>

The shortlog should explicitly say "drm/amdgpu: Make
amdgpu_cs_parser_init static". With that, this patch and patch 1 are

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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


[PATCH xf86-video-ati] Only call drmmode_scanout_free for non-GPU screens in LeaveVT

2017-06-21 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Destroying the scanout buffers of GPU screens resulted in a crash when
switching back to the Xorg VT.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/radeon_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index c4bdfcfac..5637e7f8a 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -2478,7 +2478,8 @@ void RADEONLeaveVT_KMS(VT_FUNC_ARGS_DECL)
 radeon_drop_drm_master(pScrn);
 
 xf86RotateFreeShadow(pScrn);
-drmmode_scanout_free(pScrn);
+if (!pScrn->is_gpu)
+   drmmode_scanout_free(pScrn);
 
 xf86_hide_cursors (pScrn);
 info->accel_state->XInited3D = FALSE;
-- 
2.11.0

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


[PATCH xf86-video-amdgpu] Increase reference count of FB assigned to drmmode_crtc->flip_pending

2017-06-21 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Otherwise, it could happen that we destroy the FB before the flip
completes, resulting in use-after-free and most likely a crash.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c  | 8 
 src/drmmode_display.c | 8 ++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 784f7388a..143294a9f 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -722,8 +722,8 @@ amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
return;
}
 
-   drmmode_crtc->flip_pending =
-   amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
+
amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap));
if (!drmmode_crtc->flip_pending) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Failed to get FB for PRIME flip.\n");
@@ -1011,8 +1011,8 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
return;
}
 
-   drmmode_crtc->flip_pending =
-   amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
+
amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap));
if (!drmmode_crtc->flip_pending) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Failed to get FB for scanout flip.\n");
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 0d900418a..ce46f7ba6 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2196,8 +2196,11 @@ void
 drmmode_clear_pending_flip(xf86CrtcPtr crtc)
 {
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+   ScrnInfoPtr scrn = crtc->scrn;
+   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
 
-   drmmode_crtc->flip_pending = NULL;
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->flip_pending,
+NULL);
 
if (!crtc->enabled ||
(drmmode_crtc->pending_dpms_mode != DPMSModeOn &&
@@ -2835,7 +2838,8 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
goto flip_error;
}
 
-   drmmode_crtc->flip_pending = fb;
+   drmmode_fb_reference(pAMDGPUEnt->fd, 
_crtc->flip_pending,
+fb);
drm_queue_seq = 0;
}
 
-- 
2.11.0

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


Re: [PATCH] drm/amdgpu: optimize out a spin lock (v2)

2017-06-21 Thread Michel Dänzer
On 22/06/17 11:45 AM, Alex Xie wrote:
> Use atomic instead of spin lock.
> v2: Adjust commit message
> 
> Signed-off-by: Alex Xie <alexbin@amd.com>

The shortlog should be more specific, e.g. something like "drm/amdgpu:
Drop spinlock from mm_stats".

It's important for the Git commit shortlog to be as specific as possible
because in many cases only the shortlogs of commits are visible.


I'll leave it to others to judge whether the conversion from spinlock to
atomics is safe / an overall win.


>   /* This returns 0 if the driver is in debt to disallow (optional)
>* buffer moves.
>*/
> - max_bytes = us_to_bytes(adev, adev->mm_stats.accum_us);
> -
> - spin_unlock(>mm_stats.lock);
> + max_bytes = us_to_bytes(adev, accum_us);
>   return max_bytes;
>  }

You can just make this

return us_to_bytes(adev, accum_us);

and remove the max_bytes local.


-- 
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 3/3] drm/amdgpu: add gtt_sys_limit

2017-06-26 Thread Michel Dänzer
On 27/06/17 08:39 AM, John Brooks wrote:
> On Mon, Jun 26, 2017 at 03:39:57PM +0200, Christian König wrote:
>> From: Christian König <christian.koe...@amd.com>
>>
>> Limit the size of the GART table for the system domain.
>>
>> This saves us a bunch of visible VRAM, but also limitates the maximum BO 
>> size we can swap out.
>>
>> Signed-off-by: Christian König <christian.koe...@amd.com>
> 
> Hmm.
> 
> On my system, GTT is 4096MiB (1048576 pages). For this, the table takes up
> 1048576*8 bytes = 8MiB. Reducing GTT to 256MiB (65536 pages) would reduce the
> size of the table to 512 KiB. A relatively large saving, to be sure. But in 
> the
> grander scheme of things, is saving 7.5MiB (3% of visible VRAM @ 256M) worth
> cutting GTT memory by a factor of 16?

I'm afraid not, especially since it would limit the maximum BO size to <
256MB, if I understand correctly. Pretty sure that would cause failures
with real world apps.


-- 
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] amdgpu: sync amdgpu_drm with kernel.

2017-06-26 Thread Michel Dänzer
On 27/06/17 11:58 AM, Dave Airlie wrote:
> From: Dave Airlie <airl...@redhat.com>
> 
> This syncs the amdgpu_drm header with my drm-next branch as of
> 6d61e70ccc21606ffb8a0a03bd3aba24f659502b.
> 
> It brings over the VM and semaphore API changes.
> 
> Generated using make headers_install.
> Generated from git://people.freedesktop.org/~airlied/linux drm-next commit 
> 6d61e70ccc2.
> 
> Signed-off-by: Dave Airlie <airl...@redhat.com>

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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 1/3] drm/amdgpu: fix a typo

2017-06-26 Thread Michel Dänzer
On 23/06/17 07:49 PM, Marek Olšák wrote:
> On Fri, Jun 23, 2017 at 11:27 AM, Christian König
> <deathsim...@vodafone.de> wrote:
>> Am 23.06.2017 um 11:08 schrieb zhoucm1:
>>> On 2017年06月23日 17:01, zhoucm1 wrote:
>>>> On 2017年06月23日 16:25, Christian König wrote:
>>>>> Am 23.06.2017 um 09:09 schrieb zhoucm1:
>>>>>> On 2017年06月23日 14:57, Christian König wrote:
>>>>>>>
>>>>>>> But giving the CS IOCTL an option for directly specifying the BOs
>>>>>>> instead of a BO list like Marek suggested would indeed save us some time
>>>>>>> here.
>>>>>>
>>>>>> interesting, I always follow how to improve our cs ioctl, since UMD
>>>>>> guys aften complain our command submission is slower than windows.
>>>>>> Then how to directly specifying the BOs instead of a BO list? BO handle
>>>>>> array from UMD? Could your guys describe more clear? Is it doable?
>>>>>
>>>>>
>>>>> Making the BO list part of the CS IOCTL wouldn't help at all for the
>>>>> close source UMDs. To be precise we actually came up with the BO list
>>>>> approach because of their requirement.
>>>>>
>>>>> The biggest bunch of work during CS is reserving all the buffers,
>>>>> validating them and checking their VM status.
>>>>
>>>> Totally agree. Every time when I read code there, I often want to
>>>> optimize them.
>>>>
>>>>> It doesn't matter if the BOs come from the BO list or directly in the CS
>>>>> IOCTL.
>>>>>
>>>>> The key point is that CS overhead is pretty much irrelevant for the open
>>>>> source stack, since Mesa does command submission from a separate thread
>>>>> anyway.
>>>>
>>>> If irrelevant for the open stack, then how does open source stack handle
>>>> "The biggest bunch of work during CS is reserving all the buffers,
>>>> validating them and checking their VM status."?
>>
>>
>> Command submission on the open stack is outsourced to a separate user space
>> thread. E.g. when an application triggers a flush the IBs created so far are
>> just put on a queue and another thread pushes them down to the kernel.
>>
>> I mean reducing the overhead of the CS IOCTL is always nice, but you usual
>> won't see any fps increase as long as not all CPUs are completely bound to
>> some tasks.
>>
>>>> If open stack has a better way, I think closed stack can follow it, I
>>>> don't know the history.
>>>
>>> Do you not use bo list at all in mesa? radv as well?
>>
>>
>> I don't think so. Mesa just wants to send the list of used BOs down to the
>> kernel with every IOCTL.
> 
> The CS ioctl actually costs us some performance, but not as much as on
> closed source drivers.
> 
> MesaGL always executes all CS ioctls in a separate thread (in parallel
> with the UMD) except for the last IB that's submitted by SwapBuffers.

... or by an explicit glFinish or glFlush (at least when the current
draw buffer isn't a back buffer) call, right?


> For us, it's certainly useful to optimize the CS ioctl because of apps
> that submit only 1 IB per frame where multithreading has no effect or
> may even hurt performance.

Another possibility might be flushing earlier, e.g. when the GPU and/or
CS submission thread are idle. But optimizing the CS ioctl would still
help in that case.

Finding good heuristics which allows better utilization of the GPU / CS
submission thread and doesn't hurt performance in any scenario might be
tricky though.


-- 
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 libdrm] amdgpu: update the exported always on CU bitmap

2017-06-26 Thread Michel Dänzer
On 26/06/17 03:12 PM, Flora Cui wrote:
> keep cu_ao_mask unchanged for backward compatibility.
> 
> Change-Id: I9f497aadd309977468e246fea333b392c0150276
> Signed-off-by: Flora Cui <flora@amd.com>
> ---
> This patch should be landed after the kmd patch upsteam. right?

Right. Also, include/drm/amdgpu_drm.h should be updated in a separate
patch, see  include/drm/README (in particular the "When and how to
update these files").


-- 
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 2/9] drm/amdgpu: Add vis_vramlimit module parameter

2017-06-26 Thread Michel Dänzer
On 24/06/17 02:39 AM, John Brooks wrote:
> Allow specifying a limit on visible VRAM via a module parameter. This is
> helpful for testing performance under visible VRAM pressure.
> 
> Signed-off-by: John Brooks <j...@fastquake.com>

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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/9] drm/amdgpu: Set/clear CPU_ACCESS_REQUIRED flag on page fault and CS

2017-06-26 Thread Michel Dänzer
On 25/06/17 03:00 AM, Christian König wrote:
> Am 23.06.2017 um 19:39 schrieb John Brooks:
>> When the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag is given by
>> userspace,
>> it should only be treated as a hint to initially place a BO somewhere CPU
>> accessible, rather than having a permanent effect on BO placement.
> 
> And that is a clear NAK from my side.
> 
> CPU_ACCESS_REQUIRED is a permanent limitation to where the buffer should
> be placed.

It really can't be more than a hint. The userspace driver cannot
reliably know ahead of time whether a BO will be accessed by the CPU at
all, let alone how often. A BO which incorrectly has this flag set
creates artificial pressure on CPU visible VRAM.


> Ignoring that and changing the IOCTL interface like this is clearly not
> acceptable.

I'd say it's more adapting the semantics of the flag to reality. :)


>> Instead of the flag being set in stone at BO creation, set the flag
>> when a page fault occurs so that it goes somewhere CPU-visible, and clear it
>> when the BO is requested by the GPU.

My idea was to only clear the flag when the BO is moved from GTT to
VRAM. That way, any BO which was ever accessed by the CPU since it was
last moved to VRAM will be preferably put in the CPU visible part of VRAM.

Not sure which way is better though.


-- 
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 3/9] drm/amdgpu: Don't force BOs into visible VRAM for page faults

2017-06-26 Thread Michel Dänzer
On 24/06/17 02:39 AM, John Brooks wrote:
> There is no need for page faults to force BOs into visible VRAM if it's
> full, and the time it takes to do so is great enough to cause noticeable
> stuttering. Add GTT as a possible placement so that if visible VRAM is
> full, page faults move BOs to GTT instead of evicting other BOs from VRAM.
> 
> Signed-off-by: John Brooks <j...@fastquake.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 244a7d3..751bc05 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -981,10 +981,11 @@ int amdgpu_bo_fault_reserve_notify(struct 
> ttm_buffer_object *bo)
>  
>   /* hurrah the memory is not visible ! */
>   atomic64_inc(>num_vram_cpu_page_faults);
> - amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_VRAM);
> + amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_VRAM |
> +  AMDGPU_GEM_DOMAIN_GTT);
>   lpfn =  adev->mc.visible_vram_size >> PAGE_SHIFT;
>   for (i = 0; i < abo->placement.num_placement; i++) {
> - /* Force into visible VRAM */
> + /* Try to move the BO into visible VRAM */
>   if ((abo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>   (!abo->placements[i].lpfn ||
>abo->placements[i].lpfn > lpfn))
> @@ -993,16 +994,13 @@ int amdgpu_bo_fault_reserve_notify(struct 
> ttm_buffer_object *bo)
>   abo->placement.busy_placement = abo->placement.placement;
>   abo->placement.num_busy_placement = abo->placement.num_placement;
>   r = ttm_bo_validate(bo, >placement, false, false);
> - if (unlikely(r == -ENOMEM)) {
> - amdgpu_ttm_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_GTT);
> - return ttm_bo_validate(bo, >placement, false, false);
> - } else if (unlikely(r != 0)) {
> + if (unlikely(r != 0))
>   return r;
> - }
>  
>   offset = bo->mem.start << PAGE_SHIFT;
>   /* this should never happen */
> - if ((offset + size) > adev->mc.visible_vram_size)
> + if (bo->mem.mem_type == TTM_PL_VRAM &&
> + (offset + size) > adev->mc.visible_vram_size)
>   return -EINVAL;
>  
>   return 0;
> 

AFAICT this is essentially the same as
https://patchwork.freedesktop.org/patch/156993/ . I retracted that patch
due to it causing Rally Dirt performance degradation for Marek.
Presumably other patches in this series compensate for that, but at the
least this patch should be moved to the end of the series.


-- 
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 7/9] drm/amdgpu: Throttle visible VRAM moves separately

2017-06-26 Thread Michel Dänzer
On 24/06/17 02:39 AM, John Brooks wrote:
> The BO move throttling code is designed to allow VRAM to fill quickly if it
> is relatively empty. However, this does not take into account situations
> where the visible VRAM is smaller than total VRAM, and total VRAM may not
> be close to full but the visible VRAM segment is under pressure. In such
> situations, visible VRAM would experience unrestricted swapping and
> performance would drop.
> 
> Add a separate counter specifically for moves involving visible VRAM, and
> check it before moving BOs there.
> 
> Fixes: 95844d20ae02 (drm/amdgpu: throttle buffer migrations at CS using a 
> fixed MBps limit (v2))
> Signed-off-by: John Brooks <j...@fastquake.com>

Something like this patch is definitely needed, good catch.

However, as is one issue is that it incurs CPU overhead even when all of
VRAM is CPU visible. Can that be avoided somehow?


-- 
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


[PATCH xf86-video-ati 1/2] Increase reference count of FB assigned to drmmode_crtc->flip_pending

2017-06-27 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Otherwise, it could happen that we destroy the FB before the flip
completes, resulting in use-after-free and most likely a crash.

(Ported from amdgpu commit af7221e1c4d2dbdfd488eb0976a835584ea8441c)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.c | 8 ++--
 src/radeon_kms.c  | 8 
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 9deaa575d..dd394ec1d 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2369,8 +2369,11 @@ void
 drmmode_clear_pending_flip(xf86CrtcPtr crtc)
 {
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+   ScrnInfoPtr scrn = crtc->scrn;
+   RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
 
-   drmmode_crtc->flip_pending = NULL;
+   drmmode_fb_reference(pRADEONEnt->fd, _crtc->flip_pending,
+NULL);
 
if (!crtc->enabled ||
(drmmode_crtc->pending_dpms_mode != DPMSModeOn &&
@@ -3030,7 +3033,8 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
goto flip_error;
}
 
-   drmmode_crtc->flip_pending = fb;
+   drmmode_fb_reference(pRADEONEnt->fd, 
_crtc->flip_pending,
+fb);
drm_queue_seq = 0;
}
 
diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index 5637e7f8a..691fcdf5b 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -815,8 +815,8 @@ radeon_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
return;
 }
 
-drmmode_crtc->flip_pending =
-   radeon_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
+drmmode_fb_reference(pRADEONEnt->fd, _crtc->flip_pending,
+ 
radeon_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap));
 if (!drmmode_crtc->flip_pending) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Failed to get FB for PRIME flip.\n");
@@ -1110,8 +1110,8 @@ radeon_scanout_flip(ScreenPtr pScreen, RADEONInfoPtr info,
return;
 }
 
-drmmode_crtc->flip_pending =
-   radeon_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
+drmmode_fb_reference(pRADEONEnt->fd, _crtc->flip_pending,
+ 
radeon_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap));
 if (!drmmode_crtc->flip_pending) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Failed to get FB for scanout flip.\n");
-- 
2.13.1

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


[PATCH xf86-video-ati 2/2] Improve drmmode_fb_reference debugging code

2017-06-27 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

If a reference count is <= 0, call FatalError with the call location
(in case it doesn't get resolved in the backtrace printed by
FatalError).

(Ported from amdgpu commit 1b6ff5fd9933c00ec1ec90dfc62e0b531927749b)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/drmmode_display.h | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index db68054a7..dde27a009 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -141,29 +141,36 @@ enum drmmode_flip_sync {
 
 
 static inline void
-drmmode_fb_reference(int drm_fd, struct drmmode_fb **old, struct drmmode_fb 
*new)
+drmmode_fb_reference_loc(int drm_fd, struct drmmode_fb **old, struct 
drmmode_fb *new,
+const char *caller, unsigned line)
 {
 if (new) {
-   if (new->refcnt <= 0)
-   ErrorF("New FB's refcnt was %d in %s\n", new->refcnt, __func__);
-   else
-   new->refcnt++;
+   if (new->refcnt <= 0) {
+   FatalError("New FB's refcnt was %d at %s:%u",
+  new->refcnt, caller, line);
+   }
+
+   new->refcnt++;
 }
 
 if (*old) {
if ((*old)->refcnt <= 0) {
-   ErrorF("Old FB's refcnt was %d in %s\n", (*old)->refcnt, __func__);
-   } else {
-   if (--(*old)->refcnt == 0) {
-   drmModeRmFB(drm_fd, (*old)->handle);
-   free(*old);
-   }
+   FatalError("Old FB's refcnt was %d at %s:%u",
+  (*old)->refcnt, caller, line);
+   }
+
+   if (--(*old)->refcnt == 0) {
+   drmModeRmFB(drm_fd, (*old)->handle);
+   free(*old);
}
 }
 
 *old = new;
 }
 
+#define drmmode_fb_reference(fd, old, new) \
+drmmode_fb_reference_loc(fd, old, new, __func__, __LINE__)
+
 
 extern int drmmode_page_flip_target_absolute(RADEONEntPtr pRADEONEnt,
 drmmode_crtc_private_ptr 
drmmode_crtc,
-- 
2.13.1

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


[PATCH xf86-video-amdgpu] Only call drmmode_scanout_free for non-GPU screens in LeaveVT

2017-06-27 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Destroying the scanout buffers of GPU screens resulted in a crash when
switching back to the Xorg VT.

Fixes: b10ecdbd89b0 ("Use drmmode_crtc_scanout_* helpers for RandR 1.4
  scanout pixmaps")
(Ported from radeon commit c9dd28cb0c9c3de676eadac61e727732510f6b9b)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 143294a9f..b625250fd 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1975,7 +1975,8 @@ void AMDGPULeaveVT_KMS(VT_FUNC_ARGS_DECL)
amdgpu_drop_drm_master(pScrn);
 
xf86RotateFreeShadow(pScrn);
-   drmmode_scanout_free(pScrn);
+   if (!pScrn->is_gpu)
+   drmmode_scanout_free(pScrn);
 
xf86_hide_cursors(pScrn);
 
-- 
2.13.1

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


Re: [PATCH 7/9] drm/amdgpu: Throttle visible VRAM moves separately

2017-06-27 Thread Michel Dänzer
On 27/06/17 07:29 AM, John Brooks wrote:
> On Mon, Jun 26, 2017 at 06:44:30PM +0900, Michel Dänzer wrote:
>> On 24/06/17 02:39 AM, John Brooks wrote:
>>> The BO move throttling code is designed to allow VRAM to fill quickly if it
>>> is relatively empty. However, this does not take into account situations
>>> where the visible VRAM is smaller than total VRAM, and total VRAM may not
>>> be close to full but the visible VRAM segment is under pressure. In such
>>> situations, visible VRAM would experience unrestricted swapping and
>>> performance would drop.
>>>
>>> Add a separate counter specifically for moves involving visible VRAM, and
>>> check it before moving BOs there.
>>>
>>> Fixes: 95844d20ae02 (drm/amdgpu: throttle buffer migrations at CS using a 
>>> fixed MBps limit (v2))
>>> Signed-off-by: John Brooks <j...@fastquake.com>
>>
>> Something like this patch is definitely needed, good catch.
>>
>> However, as is one issue is that it incurs CPU overhead even when all of
>> VRAM is CPU visible. Can that be avoided somehow?
>>
> 
> I guess that depends which part of it you consider to be expensive.
> 
> bytes_moved_vis_threshold isn't used unless visible VRAM is smaller than total
> VRAM, so any work/instructions that go into computing it could be skipped in
> that case (at the cost of checking that visible_vram_size < real_vram_size)
> Would that help?

I think so. E.g. in amdgpu_cs_get_threshold_for_moves, it should be
possible to handle most of this in a single if (visible_vram_size <
real_vram_size) block, which should be mostly free in the
visible_vram_size == real_vram_size case thanks to branch prediction.

Not sure there's much that can be done elsewhere though.


-- 
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 0/3] drm/amdgpu: Tweaks for high pressure on CPU visible VRAM

2017-05-19 Thread Michel Dänzer
On 20/05/17 01:14 AM, Marek Olšák wrote:
> Hi Michel,
> 
> I've applied your series

Thanks for testing it.

> and it doesn't help with low Dirt Rally performance on Fiji. I see TTM
> buffer moves at 800MB/s and many VRAM page faults.

Did you see this:

>> Note that there's only little if any improvement of the average framerate
>> reported, but the minimum framerate as seen on the HUD goes from ~10 fps
>> to ~17.

I.e. it mostly affects the minimum framerate and smoothness for me as well.


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


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

2017-05-19 Thread Michel Dänzer
On 20/05/17 12:52 AM, Marek Olšák wrote:
> On Fri, May 19, 2017 at 9:03 AM, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 19/05/17 12:04 PM, John Brooks wrote:
>>> Set GTT as the busy placement for newly created BOs that have the
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
>>> established BOs to be evicted from visible VRAM.
>>
>> This is an interesting idea, but there are some issues with this patch.
>>
>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 365883d..655718a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device 
>>> *adev,
>>>  #endif
>>>
>>>   amdgpu_fill_placement_to_bo(bo, placement);
>>> +
>>> + /* This is a new BO that wants to be CPU-visible; set GTT as the busy
>>> +  * placement so that it does not evict established BOs from visible 
>>> VRAM.
>>> +  */
>>> + if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
>>
>> Should be something like
>>
>> if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
>>
>> otherwise it would also match e.g. BOs with domain ==
>> AMDGPU_GEM_DOMAIN_GTT and the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
>> flag set (not that this makes sense, but there's nothing to prevent it).
> 
> I think it should be like this, which trivially rules out GTT and
> VRAM|GTT cases:
> if (domain == AMDGPU_GEM_DOMAIN_VRAM &&

That won't work as is due to the second hunk of the patch, which adds in
AMDGPU_GEM_DOMAIN_GTT before calling this function.


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


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

2017-05-19 Thread Michel Dänzer
On 20/05/17 04:23 AM, John Brooks wrote:
> On Fri, May 19, 2017 at 04:03:28PM +0900, Michel Dänzer wrote:
>> On 19/05/17 12:04 PM, John Brooks wrote:
>>> Set GTT as the busy placement for newly created BOs that have the
>>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
>>> established BOs to be evicted from visible VRAM.
>>
>> This is an interesting idea, but there are some issues with this patch.

[...]

>>> +   flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
>>> +   bo->placement.num_placement = 1;
>>> +   bo->placement.num_busy_placement = 1;
>>> +   bo->placement.busy_placement = >placement.placement[1];
>>> +   }
>>
>> The original placements set by amdgpu_fill_placement_to_bo need to be
>> restored before returning from this function, otherwise I suspect such
>> BOs which end up being created in GTT will stay there permanently.
>>
> 
> I'm curious, what makes you think that the BOs cannot move back to VRAM
> otherwise? VRAM is still in the placements and prefered/allowed domains, just
> not in the busy placements.

If there is not enough free space when the BO is created, there probably
won't be either when it's validated for GPU command streams later.


>> Does the patch still help for Dying Light if you fix this?

Please test this. The result should tell us whether the problem with
Dying Light is really pressure on CPU visible VRAM, or something else.


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


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

2017-05-22 Thread Michel Dänzer
On 23/05/17 12:50 PM, Lukas Wunner wrote:
> On Tue, May 23, 2017 at 12:09:49PM +0900, Michel Dänzer wrote:
>> On 22/05/17 11:04 PM, Lukas Wunner wrote:
>>> On Sun, May 21, 2017 at 09:31:09AM +0200, Nicolai Stange wrote:
>>>> On Thu, May 18 2017, Lukas Wunner wrote:
>>> [snip]
>>>>> Reported-by: Nicolai Stange <nicsta...@gmail.com>
>>>>> Fixes: 7ffb0ce31cf9 ("drm/radeon: Don't register Thunderbolt eGPU with 
>>>>> vga_switcheroo")
>>>>> Signed-off-by: Lukas Wunner <lu...@wunner.de>
>>>>> ---
>>>>>
>>>>> Awaiting a Tested-by: from Nicolai, but it's clear this is a bug and
>>>>> needs to be fixed, so sending out with a proper commit message now.
>>>>> The bug was only introduced to radeon, not amdgpu.
>>>>
>>>> Tested-by: Nicolai Stange <nicsta...@gmail.com>
>>>>
>>>> Thanks for the quick fix!
>>>>
>>>>> @Alex Deucher: I could push this to drm-misc-fixes but then it wouldn't
>>>>> land before -rc3 because Sean Paul has already sent out the -rc2 pull.
>>>>> I notice you haven't sent out a pull for -rc2 yet, so maybe you want to
>>>>> take it yourself?  Whichever you prefer.  Thanks & sorry for the breakage!
>>>
>>> I've learned this morning that Alex is on vacation.
>>
>> Christian König is standing in for Alex.
> 
> By his own account, he already has "all hands full replacing him [Alex]",
> explicitly asked Daniel to merge an amdgpu patch through drm-misc-next for
> this reason and lacks permission to update branches in Alex' repo on fdo:
> 
> "One lesson learned from the past week is that Alex needs to stop using
> his personal repository on fdo.
> We were asked a couple of times if I couldn't update a branch there from 
> different directions, which we obviously can't do."
> 
> https://lists.freedesktop.org/archives/dri-devel/2017-May/142376.html
> https://lists.freedesktop.org/archives/dri-devel/2017-May/142380.html

The important point being that Christian reviewed that patch and
explicitly asked Daniel to pick it up.


>>> I've pushed the patch to drm-misc-fixes so that the issue is fixed in
>>> 4.12-rc3.
>>
>> I don't think there was any particular need to bypass the normal radeon
>> tree for this. There was plenty of time for the fix to get into 4.12
>> final, even after Alex is back.
> 
> Well, it wouldn't be nice towards users affected by the same issue
> who may waste time with bisecting to just sit on a fix twiddling thumbs.

We all tend to think our latest fix is the most important one. :) But I
really don't see this one being special.


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


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

2017-05-22 Thread Michel Dänzer
On 22/05/17 11:04 PM, Lukas Wunner wrote:
> On Sun, May 21, 2017 at 09:31:09AM +0200, Nicolai Stange wrote:
>> On Thu, May 18 2017, Lukas Wunner wrote:
> [snip]
>>> Reported-by: Nicolai Stange <nicsta...@gmail.com>
>>> Fixes: 7ffb0ce31cf9 ("drm/radeon: Don't register Thunderbolt eGPU with 
>>> vga_switcheroo")
>>> Signed-off-by: Lukas Wunner <lu...@wunner.de>
>>> ---
>>>
>>> Awaiting a Tested-by: from Nicolai, but it's clear this is a bug and
>>> needs to be fixed, so sending out with a proper commit message now.
>>> The bug was only introduced to radeon, not amdgpu.
>>
>> Tested-by: Nicolai Stange <nicsta...@gmail.com>
>>
>> Thanks for the quick fix!
>>
>>> @Alex Deucher: I could push this to drm-misc-fixes but then it wouldn't
>>> land before -rc3 because Sean Paul has already sent out the -rc2 pull.
>>> I notice you haven't sent out a pull for -rc2 yet, so maybe you want to
>>> take it yourself?  Whichever you prefer.  Thanks & sorry for the breakage!
> 
> I've learned this morning that Alex is on vacation.

Christian König is standing in for Alex.

> I've pushed the patch to drm-misc-fixes so that the issue is fixed in
> 4.12-rc3.

I don't think there was any particular need to bypass the normal radeon
tree for this. There was plenty of time for the fix to get into 4.12
final, even after Alex is back.


-- 
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 0/3] drm/amdgpu: Tweaks for high pressure on CPU visible VRAM

2017-05-22 Thread Michel Dänzer
On 22/05/17 07:09 PM, Marek Olšák wrote:
> On Mon, May 22, 2017 at 12:00 PM, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 20/05/17 06:26 PM, Marek Olšák wrote:
>>> On May 20, 2017 3:26 AM, "Michel Dänzer" <mic...@daenzer.net
>>> <mailto:mic...@daenzer.net>> wrote:
>>>
>>> On 20/05/17 01:14 AM, Marek Olšák wrote:
>>> > Hi Michel,
>>> >
>>> > I've applied your series
>>>
>>> Thanks for testing it.
>>>
>>> > and it doesn't help with low Dirt Rally performance on Fiji. I see TTM
>>> > buffer moves at 800MB/s and many VRAM page faults.
>>>
>>> Did you see this:
>>>
>>> >> Note that there's only little if any improvement of the average
>>> framerate
>>> >> reported, but the minimum framerate as seen on the HUD goes from
>>> ~10 fps
>>> >> to ~17.
>>>
>>> I.e. it mostly affects the minimum framerate and smoothness for me
>>> as well.
>>>
>>>
>>> Without the series, I get 70 average fps. With the series, I get 30
>>> average fps. That might just be random bad luck. I don't know.
>>
>> Hmm, yeah, maybe that was just one of the random slowdowns you've been
>> talking about in other threads and on IRC?
>>
>> I can't reproduce any slowdown with these patches, even leaving visible
>> VRAM size at 256 MB.
> 
> The random slowdowns with Dirt Rally are only caused by the pressure
> on visible VRAM. This whole thread is about those random slowdowns.

No, this thread is about the scenario described in the cover letter of
this patch series.


> If you're saying "maybe it was just one of the random slowdowns", you're
> saying "maybe it was just the visible VRAM pressure". It's only
> random with Dirt Rally, which makes it difficult to believe statements
> such as "I can't reproduce any slowdown".

I could say the same thing about you seeing random slowdowns... I've
never seen that, I had to artificially limit the size of visible VRAM to
64 MB to make it significantly affect the benchmark result.

How many times do you need to run the benchmark on average to hit a
random slowdown? Which desktop environment and other X clients are
running during the benchmark? Which tab is active in the Steam window
while the benchmark runs?

In my case, it's only xfwm4, xterm and steam on the Dirt Rally page in
the library.


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


Re: [PATCH] drm/amd/display: Limit DCN to x86 arch

2017-05-23 Thread Michel Dänzer
On 23/05/17 10:37 PM, Harry Wentland wrote:
> On 2017-05-20 04:13 AM, Christian König wrote:
>> Am 19.05.2017 um 22:28 schrieb Harry Wentland:
>>> 
>>> I realize this is raising a lot of concern. I was concerned myself
>>> when I first saw this. Beside calling kernel_fpu_begin() and
>>> kernel_fpu_end() are there other things to watch out for?
>>
>> Yeah, especially setting "-msse" is rather questionable. As far as I
>> know on 64bit systems it is the default, but on 32bit systems that
>> could silently break some assumptions.
>>
>> Additional to that as far as I know "-msse" is just for optimization
>> and that isn't performance critical code, so why exactly do we need it?
> 
> Once we enable multi-plane code this code becomes performance critical
> as I believe it gets executed when resizing an underlay surface, such as
> a video player.

That should still only happen once per frame though, i.e. on the order
of 10s to 100s of times per second.


-- 
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


[PATCH xf86-video-amdgpu 1/1] Update URLs

2017-05-23 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

* Point to the amd-gfx mailing list
* Specify the component in all bugzilla URLs
* Use https:// for all HTML URLs

(Ported from radeon commit d80d01a73c2eaba2e3649b7bc0a3541b3ff782f6)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 README | 14 +++---
 configure.ac   |  2 +-
 man/amdgpu.man |  6 +++---
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/README b/README
index 98deab952..1a74a1281 100644
--- a/README
+++ b/README
@@ -1,25 +1,25 @@
 xf86-video-amdgpu - AMD Radeon video driver for the Xorg X server
 
-All questions regarding this software should be directed at the
-Xorg mailing list:
+Patches and questions regarding this software should be directed at the
+amd-gfx mailing list:
 
-http://lists.freedesktop.org/mailman/listinfo/xorg
+https://lists.freedesktop.org/mailman/listinfo/amd-gfx
 
 Please submit bug reports to the Xorg bugzilla:
 
-https://bugs.freedesktop.org/enter_bug.cgi?product=xorg
+
https://bugs.freedesktop.org/enter_bug.cgi?product=xorg=Driver/AMDgpu
 
 The master development code repository can be found at:
 
 git://anongit.freedesktop.org/git/xorg/driver/xf86-video-amdgpu
 
-http://cgit.freedesktop.org/xorg/driver/xf86-video-amdgpu
+https://cgit.freedesktop.org/xorg/driver/xf86-video-amdgpu
 
 For patch submission instructions, see:
 
-   http://www.x.org/wiki/Development/Documentation/SubmittingPatches
+https://www.x.org/wiki/Development/Documentation/SubmittingPatches
 
 For more information on the git code manager, see:
 
-http://wiki.x.org/wiki/GitPage
+https://wiki.x.org/wiki/GitPage
 
diff --git a/configure.ac b/configure.ac
index 9110f3545..26eb52ddc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -24,7 +24,7 @@
 AC_PREREQ([2.60])
 AC_INIT([xf86-video-amdgpu],
 [1.3.99],
-[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg],
+
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg=Driver/AMDgpu],
 [xf86-video-amdgpu])
 
 AC_CONFIG_SRCDIR([Makefile.am])
diff --git a/man/amdgpu.man b/man/amdgpu.man
index 13ffb7a27..d6904b818 100644
--- a/man/amdgpu.man
+++ b/man/amdgpu.man
@@ -115,17 +115,17 @@ __xservername__(__appmansuffix__), 
__xconfigfile__(__filemansuffix__), Xserver(_
 .IP " 1." 4
 Wiki page:
 .RS 4
-http://www.x.org/wiki/radeon
+https://www.x.org/wiki/radeon
 .RE
 .IP " 2." 4
 Overview about amdgpu development code:
 .RS 4
-http://cgit.freedesktop.org/xorg/driver/xf86-video-amdgpu/
+https://cgit.freedesktop.org/xorg/driver/xf86-video-amdgpu/
 .RE
 .IP " 3." 4
 Mailing list:
 .RS 4
-http://lists.freedesktop.org/mailman/listinfo/amd-gfx
+https://lists.freedesktop.org/mailman/listinfo/amd-gfx
 .RE
 .IP " 4." 4
 IRC channel:
-- 
2.11.0

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


Re: [PATCH 0/3] drm/amdgpu: Tweaks for high pressure on CPU visible VRAM

2017-05-24 Thread Michel Dänzer
On 24/05/17 08:27 PM, Christian König wrote:
> Am 24.05.2017 um 13:03 schrieb Marek Olšák:
>>>
>> I think the final solution (done in fault_reserve_notify) should be:
>> if (bo->num_cpu_page_faults++ > 20)
>> bo->preferred_domain = GTT_WC;

I agree something like that will probably be part of the solution, but I
doubt it's quite that simple or that it's the only thing that can be
improved.


> I more or less agree on that, but setting preferred_domain permanently
> to GTT_WC is what worries me a bit.
> 
> E.g. imagine you alt+tab from a game to your browser and back and the
> game runs way slower now because BOs are never moved back to VRAM.

Right, permanently moving a BO to GTT might itself cause performance to
drop down a cliff in some cases. It's possible that this is irrelevant
compared to excessive buffer migration for CPU access though.


> What we need is a global limit of number of bytes transfered per second
> for swap operations or something like that.
> 
> Or maybe a timeout which says when a BO was moved (either by swapping it
> out or by a CPU page fault) only move it back after +n jiffies or
> something like that.

I also feel like something like this will be more useful than the number
of CPU page faults per se. But I'm curious what Marek comes up with. :)


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


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

2017-05-22 Thread Michel Dänzer
On 22/05/17 10:54 AM, Michel Dänzer wrote:
> On 20/05/17 03:55 AM, Felix Kuehling wrote:
>> On 17-05-18 09:17 PM, Michel Dänzer wrote:
> 
>>>>> The default at this point should possibly still be for CIK GPUs to be
>>>>> driven by radeon, even if CONFIG_DRM_AMDGPU_CIK is enabled;
>>>> Alex and Christian seem to think otherwise.
>>> FWIW, on my AMD notebook (HP EliteBook 725 G2, Kaveri), suspend/resume
>>> with amdgpu results in a black screen (can reboot blindly); works fine
>>> with radeon.
>>
>> Has this problem been reported anywhere? Is anyone working on fixing it?
>> If it's not reported, it will never be fixed.
> 
> You're right, I'll file a bug report.

I get the same error lines as described in
https://bugs.freedesktop.org/100949, I'm trying to get clarification if
it's the same bug.


-- 
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 0/3] drm/amdgpu: Tweaks for high pressure on CPU visible VRAM

2017-05-25 Thread Michel Dänzer
On 25/05/17 08:24 PM, Marek Olšák wrote:
> On Thu, May 25, 2017 at 5:31 AM, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 24/05/17 08:27 PM, Christian König wrote:
>>> Am 24.05.2017 um 13:03 schrieb Marek Olšák:
>>>>>
>>>> I think the final solution (done in fault_reserve_notify) should be:
>>>> if (bo->num_cpu_page_faults++ > 20)
>>>> bo->preferred_domain = GTT_WC;
>>
>> I agree something like that will probably be part of the solution, but I
>> doubt it's quite that simple or that it's the only thing that can be
>> improved.
>>
>>
>>> I more or less agree on that, but setting preferred_domain permanently
>>> to GTT_WC is what worries me a bit.
>>>
>>> E.g. imagine you alt+tab from a game to your browser and back and the
>>> game runs way slower now because BOs are never moved back to VRAM.
>>
>> Right, permanently moving a BO to GTT might itself cause performance to
>> drop down a cliff in some cases. It's possible that this is irrelevant
>> compared to excessive buffer migration for CPU access though.
>>
>>
>>> What we need is a global limit of number of bytes transfered per second
>>> for swap operations or something like that.
>>>
>>> Or maybe a timeout which says when a BO was moved (either by swapping it
>>> out or by a CPU page fault) only move it back after +n jiffies or
>>> something like that.
>>
>> I also feel like something like this will be more useful than the number
>> of CPU page faults per se. But I'm curious what Marek comes up with. :)
> 
> I don't have any better idea at the moment. It looks like John Brooks
> has already solved this issue based on his IRC comments.

I don't think there's "the issue" with a single solution. None of John's
patches that I've tried so far help for the scenario described in the
cover letter of this series.


-- 
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 1/1] amdgpu: move asic id table to a separate file

2017-05-24 Thread Michel Dänzer
On 25/05/17 12:10 AM, Li, Samuel wrote:
>> There's no need for a separate repo upstream.  It's purely to aid
>> internal packaging.
> As a first step, we can put a snapshot in libdrm for now. External
> packaging  will face the same issue though ... the packagers need to
> put the ids in various versions of various distros.

They can take a snapshot from the libdrm repository the same way they
could from a separate repository. I don't see any issue there.


> We also likely need to automate the upstream update to save sometime
> for ourselves.

I'm skeptical that it can be automated, we'll see. :)


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


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

2017-05-18 Thread Michel Dänzer
On 19/05/17 04:35 AM, Felix Kuehling wrote:
> On 17-04-27 05:22 AM, Michel Dänzer wrote:
>> On 27/04/17 05:15 AM, Felix Kuehling wrote:
>>> Hi Michel,
>>>
>>> You said in an earlier email that it doesn't have to be convenient. With
>>> that in mind, I went back to a minimalistic solution that doesn't need
>>> any additional Kconfig options and only uses two module parameters (one
>>> in radeon, one in amdgpu).
>>>
>>> I'm attaching my WIP patches for reference (currently based on
>>> amd-kfd-staging). For an official review I'd rebase them on
>>> amd-staging-4.9, remove the #if-0-ed hack that didn't work out, and add
>>> the same for SI.
>>>
>>> Can everyone can agree that this is good enough?
>> Yes, something like this could be a good first step in the right
>> direction. Some comments:
>>
>> The radeon options should be available regardless of
>> CONFIG_DRM_AMDGPU_CIK/SI, or they won't be useful for amdgpu-pro / other
>> out-of-tree amdgpu builds.
> 
> Hmm, when an amdgpu-pro build is installed on a system with an older
> kernel, radeon won't have the module option at all. Unless the pro-build
> blacklists radeon, or replaces radeon with its own version, it will
> always have to be compiled without CIK support.
> 
> Therefore, I think my patch, meant for upstream, should not consider
> this case.

Obviously, out-of-tree module builds will have to continue doing what
they've been doing so far with kernels without your patch. I'm thinking
about making it easier for them with kernels which do have your patch.
At some point in the future, maybe the support for kernels without your
patch can be dropped entirely.


>> The default at this point should possibly still be for CIK GPUs to be
>> driven by radeon, even if CONFIG_DRM_AMDGPU_CIK is enabled;
> 
> Alex and Christian seem to think otherwise.

FWIW, on my AMD notebook (HP EliteBook 725 G2, Kaveri), suspend/resume
with amdgpu results in a black screen (can reboot blindly); works fine
with radeon.


-- 
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 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread Michel Dänzer
On 19/05/17 12:43 AM, John Brooks wrote:
> On Thu, May 18, 2017 at 06:08:09PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daen...@amd.com>
>>
>> In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
>> flag set to CPU visible VRAM with more force.
>>
>> For other BOs, this gives another chance to stay in VRAM if they
>> happened to lie in the CPU visible part and another BO needs to go
>> there.
>>
>> This should allow BOs to stay in VRAM longer in some cases.
>>
>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>

[...]

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 57789b860768..d5ed85026542 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
>> *bo,
>>  adev->mman.buffer_funcs_ring &&
>>  adev->mman.buffer_funcs_ring->ready == false) {
>>  amdgpu_ttm_placement_from_domain(abo, 
>> AMDGPU_GEM_DOMAIN_CPU);
>> +} else if (adev->mc.visible_vram_size < 
>> adev->mc.real_vram_size) {
>> +unsigned fpfn = adev->mc.visible_vram_size >> 
>> PAGE_SHIFT;
>> +struct drm_mm_node *node = bo->mem.mm_node;
>> +unsigned long pages_left;
>> +
>> +for (pages_left = bo->mem.num_pages;
>> + pages_left;
>> + pages_left -= node->size, node++) {
>> +if (node->start < fpfn)
>> +break;
>> +}
>> +
>> +if (!pages_left)
>> +goto gtt;
>> +
>> +/* Try evicting to the CPU inaccessible part of VRAM
>> + * first, but only set GTT as busy placement, so this
>> + * BO will be evicted to GTT rather than causing other
>> + * BOs to be evicted from VRAM
>> + */
>> +amdgpu_ttm_placement_from_domain(abo, 
>> AMDGPU_GEM_DOMAIN_VRAM |
>> + AMDGPU_GEM_DOMAIN_GTT);
>> +abo->placements[0].fpfn = fpfn;
>> +abo->placements[0].lpfn = 0;
>> +abo->placement.busy_placement = >placements[1];
> 
> Are you sure you want to hardcode the placements index? It'll be dependent on
> the order set up in amdgpu_ttm_placement_init.

Yes, see patch 1. Looping over the placements and testing their contents
is silly when we know exactly how they were set up. Or do you mean this
code shouldn't call amdgpu_ttm_placement_from_domain at all and just set
up the placements itself?


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


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

2017-05-18 Thread Michel Dänzer
On 07/04/17 07:20 PM, Joerg Roedel wrote:
> On Mon, Mar 27, 2017 at 11:47:07AM +0530, arindam.n...@amd.com wrote:
>> From: Arindam Nath <arindam.n...@amd.com>
>>
>> The idea behind flush queues is to defer the IOTLB flushing
>> for domains for which the mappings are no longer valid. We
>> add such domains in queue_add(), and when the queue size
>> reaches FLUSH_QUEUE_SIZE, we perform __queue_flush().
>>
>> Since we have already taken lock before __queue_flush()
>> is called, we need to make sure the IOTLB flushing is
>> performed as quickly as possible.
>>
>> In the current implementation, we perform IOTLB flushing
>> for all domains irrespective of which ones were actually
>> added in the flush queue initially. This can be quite
>> expensive especially for domains for which unmapping is
>> not required at this point of time.
>>
>> This patch makes use of domain information in
>> 'struct flush_queue_entry' to make sure we only flush
>> IOTLBs for domains who need it, skipping others.
>>
>> Signed-off-by: Arindam Nath <arindam.n...@amd.com>
>> ---
>>  drivers/iommu/amd_iommu.c | 15 ---
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 98940d1..6a9a048 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -2227,15 +2227,16 @@ static struct iommu_group 
>> *amd_iommu_device_group(struct device *dev)
>>  
>>  static void __queue_flush(struct flush_queue *queue)
>>  {
>> -struct protection_domain *domain;
>> -unsigned long flags;
>>  int idx;
>>  
>> -/* First flush TLB of all known domains */
>> -spin_lock_irqsave(_iommu_pd_lock, flags);
>> -list_for_each_entry(domain, _iommu_pd_list, list)
>> -domain_flush_tlb(domain);
>> -spin_unlock_irqrestore(_iommu_pd_lock, flags);
>> +/* First flush TLB of all domains which were added to flush queue */
>> +for (idx = 0; idx < queue->next; ++idx) {
>> +struct flush_queue_entry *entry;
>> +
>> +entry = queue->entries + idx;
>> +
>> +domain_flush_tlb(>dma_dom->domain);
>> +}
> 
> With this we will flush a domain every time we find one of its
> iova-addresses in the flush queue, so potentially we flush a domain
> multiple times per __queue_flush() call.
> 
> Its better to either add a flush-flag to the domains and evaluate that
> in __queue_flush or keep a list of domains to flush to make the flushing
> really more efficient.

Arindam, can you incorporate Joerg's feedback?

FWIW, looks like Carrizo systems are affected by this as well (see e.g.
https://bugs.freedesktop.org/show_bug.cgi?id=101029#c21), so it would be
good to land this fix in some form ASAP.


-- 
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 3/3] drm/amdgpu: Try evicting from CPU visible to invisible VRAM first

2017-05-18 Thread Michel Dänzer
On 19/05/17 10:43 AM, John Brooks wrote:
> On Fri, May 19, 2017 at 10:20:37AM +0900, Michel Dänzer wrote:
>> On 19/05/17 12:43 AM, John Brooks wrote:
>>> On Thu, May 18, 2017 at 06:08:09PM +0900, Michel Dänzer wrote:
>>>> From: Michel Dänzer <michel.daen...@amd.com>
>>>>
>>>> In exchange, move BOs with the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
>>>> flag set to CPU visible VRAM with more force.
>>>>
>>>> For other BOs, this gives another chance to stay in VRAM if they
>>>> happened to lie in the CPU visible part and another BO needs to go
>>>> there.
>>>>
>>>> This should allow BOs to stay in VRAM longer in some cases.
>>>>
>>>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
>>
>> [...]
>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 57789b860768..d5ed85026542 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -206,7 +206,34 @@ static void amdgpu_evict_flags(struct 
>>>> ttm_buffer_object *bo,
>>>>adev->mman.buffer_funcs_ring &&
>>>>adev->mman.buffer_funcs_ring->ready == false) {
>>>>amdgpu_ttm_placement_from_domain(abo, 
>>>> AMDGPU_GEM_DOMAIN_CPU);
>>>> +  } else if (adev->mc.visible_vram_size < 
>>>> adev->mc.real_vram_size) {
>>>> +  unsigned fpfn = adev->mc.visible_vram_size >> 
>>>> PAGE_SHIFT;
>>>> +  struct drm_mm_node *node = bo->mem.mm_node;
>>>> +  unsigned long pages_left;
>>>> +
>>>> +  for (pages_left = bo->mem.num_pages;
>>>> +   pages_left;
>>>> +   pages_left -= node->size, node++) {
>>>> +  if (node->start < fpfn)
>>>> +  break;
>>>> +  }
>>>> +
>>>> +  if (!pages_left)
>>>> +  goto gtt;
>>>> +
>>>> +  /* Try evicting to the CPU inaccessible part of VRAM
>>>> +   * first, but only set GTT as busy placement, so this
>>>> +   * BO will be evicted to GTT rather than causing other
>>>> +   * BOs to be evicted from VRAM
>>>> +   */
>>>> +  amdgpu_ttm_placement_from_domain(abo, 
>>>> AMDGPU_GEM_DOMAIN_VRAM |
>>>> +   AMDGPU_GEM_DOMAIN_GTT);
>>>> +  abo->placements[0].fpfn = fpfn;
>>>> +  abo->placements[0].lpfn = 0;
>>>> +  abo->placement.busy_placement = >placements[1];
>>>
>>> Are you sure you want to hardcode the placements index? It'll be dependent 
>>> on
>>> the order set up in amdgpu_ttm_placement_init.
>>
>> Yes, see patch 1. Looping over the placements and testing their contents
>> is silly when we know exactly how they were set up. 
> 
> Agreed
> 
>> Or do you mean this code shouldn't call amdgpu_ttm_placement_from_domain at
>> all and just set up the placements itself?
> 
> Calling amdgpu_ttm_placement_from_domain makes sense. I was just imagining a
> scenario where code like this that makes assumptions about the ordering of
> placements in the array would break silently if that order were changed, and
> you'd have to go about finding the places where integer literals were used to
> address specific placements.

Right, if we make changes to amdgpu_ttm_placement_init, we'll need to
audit and possibly update its callers. I think that's reasonable, though
others might disagree. :)

Thanks for your feedback.


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


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

2017-05-19 Thread Michel Dänzer
On 19/05/17 12:04 PM, John Brooks wrote:
> Set GTT as the busy placement for newly created BOs that have the
> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
> established BOs to be evicted from visible VRAM.

This is an interesting idea, but there are some issues with this patch.


> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 365883d..655718a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device 
> *adev,
>  #endif
>  
>   amdgpu_fill_placement_to_bo(bo, placement);
> +
> + /* This is a new BO that wants to be CPU-visible; set GTT as the busy
> +  * placement so that it does not evict established BOs from visible 
> VRAM.
> +  */
> + if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&

Should be something like

if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&

otherwise it would also match e.g. BOs with domain ==
AMDGPU_GEM_DOMAIN_GTT and the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
flag set (not that this makes sense, but there's nothing to prevent it).


> + flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> + bo->placement.num_placement = 1;
> + bo->placement.num_busy_placement = 1;
> + bo->placement.busy_placement = >placement.placement[1];
> + }

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

Does the patch still help for Dying Light if you fix this?

The patch as is doesn't seem to make any difference for my dirt-rally
test case.


> @@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>   memset(, 0,
>  (AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
>  
> +
> + /* New CPU-visible BOs will have GTT set as their busy placement */
> + if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
> + flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {

Make this

if ((domain & AMDGPU_GEM_DOMAIN_VRAM) &&
(flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)) {

to match the coding style.


-- 
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 0/3] drm/amdgpu: Tweaks for high pressure on CPU visible VRAM

2017-05-24 Thread Michel Dänzer
On 23/05/17 07:38 PM, Marek Olšák wrote:
> On Tue, May 23, 2017 at 2:45 AM, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 22/05/17 07:09 PM, Marek Olšák wrote:
>>> On Mon, May 22, 2017 at 12:00 PM, Michel Dänzer <mic...@daenzer.net> wrote:
>>>> On 20/05/17 06:26 PM, Marek Olšák wrote:
>>>>> On May 20, 2017 3:26 AM, "Michel Dänzer" <mic...@daenzer.net
>>>>> <mailto:mic...@daenzer.net>> wrote:
>>>>>
>>>>> On 20/05/17 01:14 AM, Marek Olšák wrote:
>>>>> > Hi Michel,
>>>>> >
>>>>> > I've applied your series
>>>>>
>>>>> Thanks for testing it.
>>>>>
>>>>> > and it doesn't help with low Dirt Rally performance on Fiji. I see 
>>>>> TTM
>>>>> > buffer moves at 800MB/s and many VRAM page faults.
>>>>>
>>>>> Did you see this:
>>>>>
>>>>> >> Note that there's only little if any improvement of the average
>>>>> framerate
>>>>> >> reported, but the minimum framerate as seen on the HUD goes from
>>>>> ~10 fps
>>>>> >> to ~17.
>>>>>
>>>>> I.e. it mostly affects the minimum framerate and smoothness for me
>>>>> as well.
>>>>>
>>>>>
>>>>> Without the series, I get 70 average fps. With the series, I get 30
>>>>> average fps. That might just be random bad luck. I don't know.
>>>>
>>>> Hmm, yeah, maybe that was just one of the random slowdowns you've been
>>>> talking about in other threads and on IRC?
>>>>
>>>> I can't reproduce any slowdown with these patches, even leaving visible
>>>> VRAM size at 256 MB.
>>>
>>> The random slowdowns with Dirt Rally are only caused by the pressure
>>> on visible VRAM. This whole thread is about those random slowdowns.
>>
>> No, this thread is about the scenario described in the cover letter of
>> this patch series.
>>
>>
>>> If you're saying "maybe it was just one of the random slowdowns", you're
>>> saying "maybe it was just the visible VRAM pressure". It's only
>>> random with Dirt Rally, which makes it difficult to believe statements
>>> such as "I can't reproduce any slowdown".
>>
>> I could say the same thing about you seeing random slowdowns... I've
>> never seen that, I had to artificially limit the size of visible VRAM to
>> 64 MB to make it significantly affect the benchmark result.
>>
>> How many times do you need to run the benchmark on average to hit a
>> random slowdown? Which desktop environment and other X clients are
>> running during the benchmark? Which tab is active in the Steam window
>> while the benchmark runs?
>>
>> In my case, it's only xfwm4, xterm and steam on the Dirt Rally page in
>> the library.
> 
> Ubuntu Unity, Steam small mode (there are no tabs), Ultra settings in
> Dirt Rally.
> 
> Every single time I run the game with this series, I get 700-1000MB/s
> of TTM BO moves. There doesn't seem to be any randomness.
> 
> It was better without this series. (meaning it was sometimes OK, sometimes 
> bad)

Thanks for the additional details. I presume that in the bad case there
are some BOs lying around in visible VRAM (e.g. from Unity), which
causes some of Dirt Rally's BOs to go back and forth between GTT on CPU
page faults and VRAM on GPU usage.

This means at least patch 2 goes out the window. I'll see if I can
salvage something out of patch 3.


-- 
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 4/4] drm/amdgpu: reset fpriv vram_lost_counter

2017-05-16 Thread Michel Dänzer
On 16/05/17 06:25 PM, Chunming Zhou wrote:
> Change-Id: I8eb6d7f558da05510e429d3bf1d48c8cec6c1977
> Signed-off-by: Chunming Zhou <david1.z...@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +++
>  include/uapi/drm/amdgpu_drm.h  | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bca1fb5..f3e7525 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2547,6 +2547,9 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, 
> struct drm_file *filp)
>   case AMDGPU_VM_OP_UNRESERVE_VMID:
>   amdgpu_vm_free_reserved_vmid(adev, >vm, AMDGPU_GFXHUB);
>   break;
> + case AMDGPU_VM_OP_RESET:
> + fpriv->vram_lost_counter = 
> atomic_read(>vram_lost_counter);
> + break;

How do you envision the UMDs using this? I can mostly think of them
calling this ioctl when a context is created or destroyed. But that
would also allow any other remaining contexts using the same DRM file
descriptor to use all ioctls again. So, I think there needs to be a
vram_lost_counter in struct amdgpu_ctx instead of in struct amdgpu_fpriv.

But then I'm not sure this ioctl will be useful... I guess the UMD could
try re-uploading the contents of crucial BOs (shader code, resource
descriptors etc.) for an existing context and then call this ioctl. How
about the GPUVM page tables? Will the kernel driver automatically
re-generate those as needed, or will the UMD also need to e.g. destroy
and re-create the VM mappings for all BOs before calling this ioctl?

It's hard to be sure whether that's workable for the UMD without at
least a working prototype...


-- 
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


[PATCH xf86-video-amdgpu] Use reference counting for tracking KMS framebuffer lifetimes

2017-05-29 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

References are held by the pixmaps corresponding to the FBs (so
the same KMS FB can be reused as long as the pixmap exists) and by the
CRTCs scanning out from them (so a KMS FB is only destroyed once it's
not being scanned out anymore, preventing intermittent black screens and
worse issues due to a CRTC turning off when it should be on).

v2:
* Only increase reference count in drmmode_fb_reference if it was sane
  before
* Make drmmode_fb_reference's indentation match the rest of
  drmmode_display.h

(Ported from radeon commit 55e513b978b2afc52b7cafc5bfcb0d1dc78d75f6)

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_kms.c  |  70 +++---
 src/amdgpu_pixmap.h   |  58 +++
 src/amdgpu_present.c  |   9 ---
 src/drmmode_display.c | 157 --
 src/drmmode_display.h |  42 --
 5 files changed, 219 insertions(+), 117 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index a418cf9d3..69d61943d 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -675,6 +675,18 @@ amdgpu_prime_scanout_flip_abort(xf86CrtcPtr crtc, void 
*event_data)
 }
 
 static void
+amdgpu_prime_scanout_flip_handler(xf86CrtcPtr crtc, uint32_t msc, uint64_t 
usec,
+ void *event_data)
+{
+   AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+   drmmode_crtc_private_ptr drmmode_crtc = event_data;
+
+   drmmode_fb_reference(pAMDGPUEnt->fd, _crtc->fb,
+drmmode_crtc->flip_pending);
+   amdgpu_prime_scanout_flip_abort(crtc, event_data);
+}
+
+static void
 amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
 {
ScreenPtr screen = ent->slave_dst->drawable.pScreen;
@@ -701,7 +713,8 @@ amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
drm_queue_seq = amdgpu_drm_queue_alloc(crtc,
   AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
   AMDGPU_DRM_QUEUE_ID_DEFAULT,
-  drmmode_crtc, NULL,
+  drmmode_crtc,
+  
amdgpu_prime_scanout_flip_handler,
   amdgpu_prime_scanout_flip_abort);
if (drm_queue_seq == AMDGPU_DRM_QUEUE_ERROR) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
@@ -709,8 +722,17 @@ amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
return;
}
 
+   drmmode_crtc->flip_pending =
+   amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
+   if (!drmmode_crtc->flip_pending) {
+   xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+  "Failed to get FB for PRIME flip.\n");
+   amdgpu_drm_abort_entry(drm_queue_seq);
+   return;
+   }
+
if (drmmode_page_flip_target_relative(pAMDGPUEnt, drmmode_crtc,
- 
drmmode_crtc->scanout[scanout_id].fb_id,
+ 
drmmode_crtc->flip_pending->handle,
  0, drm_queue_seq, 0) != 0) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING, "flip queue failed in 
%s: %s\n",
   __func__, strerror(errno));
@@ -720,7 +742,6 @@ amdgpu_prime_scanout_flip(PixmapDirtyUpdatePtr ent)
 
drmmode_crtc->scanout_id = scanout_id;
drmmode_crtc->scanout_update_pending = TRUE;
-   drmmode_crtc->flip_pending = TRUE;
 }
 
 static void
@@ -950,10 +971,14 @@ amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
 static void
 amdgpu_scanout_flip_abort(xf86CrtcPtr crtc, void *event_data)
 {
-   drmmode_crtc_private_ptr drmmode_crtc = event_data;
+   amdgpu_prime_scanout_flip_abort(crtc, event_data);
+}
 
-   drmmode_crtc->scanout_update_pending = FALSE;
-   drmmode_clear_pending_flip(crtc);
+static void
+amdgpu_scanout_flip_handler(xf86CrtcPtr crtc, uint32_t msc, uint64_t usec,
+   void *event_data)
+{
+   amdgpu_prime_scanout_flip_handler(crtc, msc, usec, event_data);
 }
 
 static void
@@ -977,7 +1002,8 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
drm_queue_seq = amdgpu_drm_queue_alloc(xf86_crtc,
   AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
   AMDGPU_DRM_QUEUE_ID_DEFAULT,
-  drmmode_crtc, NULL,
+  drmmode_crtc,
+  amdgpu_scanout_flip_handler,
   amdgpu_scanout_flip_abort);
if (drm_queue_seq == AMDGPU_DRM_QUEUE_ERROR) {
xf86DrvMs

Re: [PATCH libdrm v3 1/1] amdgpu: move asic id table to a separate file

2017-05-28 Thread Michel Dänzer
On 27/05/17 04:23 AM, Samuel Li wrote:
> 
> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids
> new file mode 100644
> index 000..1b00b60
> --- /dev/null
> +++ b/include/drm/amdgpu.ids
> @@ -0,0 +1,154 @@
> +1.0.0
> +6600,0,AMD Radeon HD 8600/8700M

This doesn't look like the current format we've settled on internally.
There are supposed to be tabs after the commas to align the columns.


-- 
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


[PATCH 2/3] drm/radeon: Make si_support and cik_support parameters always available

2017-05-29 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

This will allow amdgpu-pro / other out-of-tree amdgpu builds to make use
of these options for using the out-of-tree amdgpu driver instead of the
in-tree radeon driver in a clean way.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 drivers/gpu/drm/radeon/radeon.h | 5 -
 drivers/gpu/drm/radeon/radeon_drv.c | 4 
 drivers/gpu/drm/radeon/radeon_kms.c | 4 
 3 files changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index b0ce11ebc677..fb0c27be4ee6 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -116,13 +116,8 @@ extern int radeon_auxch;
 extern int radeon_mst;
 extern int radeon_uvd;
 extern int radeon_vce;
-
-#ifdef CONFIG_DRM_AMDGPU_SI
 extern int radeon_si_support;
-#endif
-#ifdef CONFIG_DRM_AMDGPU_CIK
 extern int radeon_cik_support;
-#endif
 
 /*
  * Copy from radeon_drv.h so we don't have to include both and have conflicting
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index c0c1b258d570..1cefadfe4a41 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -303,17 +303,13 @@ module_param_named(uvd, radeon_uvd, int, 0444);
 MODULE_PARM_DESC(vce, "vce enable/disable vce support (1 = enable, 0 = 
disable)");
 module_param_named(vce, radeon_vce, int, 0444);
 
-#ifdef CONFIG_DRM_AMDGPU_SI
 int radeon_si_support = 1;
 MODULE_PARM_DESC(si_support, "SI support (1 = enabled (default), 0 = 
disabled)");
 module_param_named(si_support, radeon_si_support, int, 0444);
-#endif
 
-#ifdef CONFIG_DRM_AMDGPU_CIK
 int radeon_cik_support = 0;
 MODULE_PARM_DESC(cik_support, "CIK support (1 = enabled, 0 = disabled 
(default))");
 module_param_named(cik_support, radeon_cik_support, int, 0444);
-#endif
 
 static struct pci_device_id pciidlist[] = {
radeon_PCI_IDS
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
b/drivers/gpu/drm/radeon/radeon_kms.c
index 09fc0e1137a1..baaddf989f6d 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -98,7 +98,6 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned 
long flags)
struct radeon_device *rdev;
int r, acpi_status;
 
-#ifdef CONFIG_DRM_AMDGPU_SI
if (!radeon_si_support) {
switch (flags & RADEON_FAMILY_MASK) {
case CHIP_TAHITI:
@@ -111,8 +110,6 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned 
long flags)
return -ENODEV;
}
}
-#endif
-#ifdef CONFIG_DRM_AMDGPU_CIK
if (!radeon_cik_support) {
switch (flags & RADEON_FAMILY_MASK) {
case CHIP_KAVERI:
@@ -128,7 +125,6 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned 
long flags)
return -ENODEV;
}
}
-#endif
 
rdev = kzalloc(sizeof(struct radeon_device), GFP_KERNEL);
if (rdev == NULL) {
-- 
2.11.0

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


[PATCH 3/3] drm/amdgpu/radeon: Use radeon by default for CIK GPUs

2017-05-29 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Even if CONFIG_DRM_AMDGPU_CIK is enabled.

There is no feature parity yet for CIK, in particular amdgpu doesn't
support HDMI/DisplayPort without DC.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Kconfig  | 8 
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 7 +--
 drivers/gpu/drm/radeon/radeon_drv.c | 4 ++--
 drivers/gpu/drm/radeon/radeon_kms.c | 5 +
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 8d36087fc186..e0121f8b436e 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -17,11 +17,11 @@ config DRM_AMDGPU_CIK
help
  Choose this option if you want to enable support for CIK asics.
 
- CIK is already supported in radeon. If you enable this option,
- support for CIK will be provided by amdgpu and disabled in
- radeon by default. Use module options to override this:
+ CIK is already supported in radeon. Support for SI in amdgpu
+ will be disabled by default and is still provided by radeon.
+ Use module options to override this:
 
- radeon.cik_support=1 amdgpu.cik_support=0
+ radeon.cik_support=0 amdgpu.cik_support=1
 
 config DRM_AMDGPU_USERPTR
bool "Always enable userptr write support"
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 76dea5fe620b..27599db7d630 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -240,8 +240,8 @@ module_param_named(si_support, amdgpu_si_support, int, 
0444);
 #endif
 
 #ifdef CONFIG_DRM_AMDGPU_CIK
-int amdgpu_cik_support = 1;
-MODULE_PARM_DESC(cik_support, "CIK support (1 = enabled (default), 0 = 
disabled)");
+int amdgpu_cik_support = 0;
+MODULE_PARM_DESC(cik_support, "CIK support (1 = enabled, 0 = disabled 
(default))");
 module_param_named(cik_support, amdgpu_cik_support, int, 0444);
 #endif
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index f4f77b99afeb..31901d2886d9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -98,7 +98,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned 
long flags)
dev_info(dev->dev,
 "SI support provided by radeon.\n");
dev_info(dev->dev,
-   "Use radeon.si_support=0 amdgpu.si_support=1 to override.\n"
+"Use radeon.si_support=0 amdgpu.si_support=1 
to override.\n"
);
return -ENODEV;
}
@@ -113,7 +113,10 @@ int amdgpu_driver_load_kms(struct drm_device *dev, 
unsigned long flags)
case CHIP_KABINI:
case CHIP_MULLINS:
dev_info(dev->dev,
-"CIK support disabled by module param\n");
+"CIK support provided by radeon.\n");
+   dev_info(dev->dev,
+"Use radeon.cik_support=0 amdgpu.cik_support=1 
to override.\n"
+   );
return -ENODEV;
}
}
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index 1cefadfe4a41..853ef118a735 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -307,8 +307,8 @@ int radeon_si_support = 1;
 MODULE_PARM_DESC(si_support, "SI support (1 = enabled (default), 0 = 
disabled)");
 module_param_named(si_support, radeon_si_support, int, 0444);
 
-int radeon_cik_support = 0;
-MODULE_PARM_DESC(cik_support, "CIK support (1 = enabled, 0 = disabled 
(default))");
+int radeon_cik_support = 1;
+MODULE_PARM_DESC(cik_support, "CIK support (1 = enabled (default), 0 = 
disabled)");
 module_param_named(cik_support, radeon_cik_support, int, 0444);
 
 static struct pci_device_id pciidlist[] = {
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
b/drivers/gpu/drm/radeon/radeon_kms.c
index baaddf989f6d..8f2579772c34 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -118,10 +118,7 @@ int radeon_driver_load_kms(struct drm_device *dev, 
unsigned long flags)
case CHIP_KABINI:
case CHIP_MULLINS:
dev_info(dev->dev,
-"CIK support provided by amdgpu.\n");
-   dev_info(dev->dev,
-   "Use radeon.cik_support=1 amdgpu.cik_support=0 

[PATCH 1/3] drm/amdgpu: Really leave SI support disabled by default

2017-05-29 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

The default option value didn't match the help text and intention.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---

Maybe this can be squashed into the commit adding this option when it
goes upstream.

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 17ecc2542af5..76dea5fe620b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -234,7 +234,7 @@ MODULE_PARM_DESC(param_buf_per_se, "the size of Off-Chip 
Pramater Cache per Shad
 module_param_named(param_buf_per_se, amdgpu_param_buf_per_se, int, 0444);
 
 #ifdef CONFIG_DRM_AMDGPU_SI
-int amdgpu_si_support = 1;
+int amdgpu_si_support = 0;
 MODULE_PARM_DESC(si_support, "SI support (1 = enabled, 0 = disabled 
(default))");
 module_param_named(si_support, amdgpu_si_support, int, 0444);
 #endif
-- 
2.11.0

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


Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

2017-05-29 Thread Michel Dänzer
On 30/05/17 06:16 AM, Samuel Li wrote:
> From: Xiaojie Yuan <xiaojie.y...@amd.com>

I took a closer look and noticed some details (and some non-details
about the amdgpu.ids file at the end).


> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
> new file mode 100644
> index 000..a43ca33
> --- /dev/null
> +++ b/amdgpu/amdgpu_asic_id.c

[...]

> +#include "xf86drm.h"
> +#include "amdgpu_drm.h"

Should be

#include 
#include 

since these header files are not located in the same directory as
amdgpu_asic_id.c.


> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
> +{
> + char *buf, *saveptr;
> + char *s_did;
> + char *s_rid;
> + char *s_name;
> + char *endptr;
> + int r = 0;

This function could be simplified slightly by initializing r = -EINVAL
here and only setting it to 0 just before the out label.


> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> +{

[...]

> + /* 1st valid line is file version */
> + while ((n = getline(, , fp)) != -1) {
> + /* trim trailing newline */
> + if (line[n - 1] == '\n')
> + line[n - 1] = '\0';

Should probably increment line_num here, otherwise the line number in
the error message below might be confusing.


> + fprintf(stderr, "Invalid format: %s: line %d: %s\n",
> + AMDGPU_ASIC_ID_TABLE, line_num, line);

The second line should be indented to align with the opening parenthesis.


> + if (table_size >= table_max_size) {
> + /* double table size */
> + table_max_size *= 2;
> + asic_id_table = realloc(asic_id_table, table_max_size *
> + sizeof(struct amdgpu_asic_id));

Ditto.


> + /* end of table */
> + id = asic_id_table + table_size;
> + memset(id, 0, sizeof(struct amdgpu_asic_id));

Might also want to realloc asic_id_table according to the final table
size, to avoid wasting memory.


> + if (r && asic_id_table) {
> + while (table_size--) {
> + id = asic_id_table + table_size;
> + if (id->marketing_name !=  NULL)
> + free(id->marketing_name);

free(NULL) works fine (and parse_one_line returns an error for
id->marketing_name == NULL anyway), so this can be simplified to

free(id->marketing_name);


> @@ -140,6 +140,13 @@ static void 
> amdgpu_device_free_internal(amdgpu_device_handle dev)
>   close(dev->fd);
>   if ((dev->flink_fd >= 0) && (dev->fd != dev->flink_fd))
>   close(dev->flink_fd);
> + if (dev->asic_ids) {
> + for (id = dev->asic_ids; id->did; id++) {
> + if (id->marketing_name !=  NULL)
> + free(id->marketing_name);
> + }

Ditto, this can be simplified to

for (id = dev->asic_ids; id->did; id++)
free(id->marketing_name);


> @@ -267,6 +274,11 @@ int amdgpu_device_initialize(int fd,
>   amdgpu_vamgr_init(>vamgr_32, start, max,
> dev->dev_info.virtual_address_alignment);
>  
> + r = amdgpu_parse_asic_ids(>asic_ids);
> + if (r)
> + fprintf(stderr, "%s: Can not parse asic ids, 0x%x.",
> + __func__, r);

"Cannot parse ASIC IDs"

Also, there should be curly braces around a multi-line statement.


> @@ -297,13 +309,15 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
>  
>  const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
>  {
> - const struct amdgpu_asic_id_table_t *t = amdgpu_asic_id_table;
> + const struct amdgpu_asic_id *id;
> +
> + if (!dev->asic_ids)
> + return NULL;
>  
> - while (t->did) {
> - if ((t->did == dev->info.asic_id) &&
> - (t->rid == dev->info.pci_rev_id))
> - return t->marketing_name;
> - t++;
> + for (id = dev->asic_ids; id->did; id++) {
> + if ((id->did == dev->info.asic_id) &&
> + (id->rid == dev->info.pci_rev_id))

The last line is indented incorrectly, should be 2 tabs and 4 spaces.


> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids
> new file mode 100644
> index 000..6d6b944
> --- /dev/null
> +++ b/include/drm/amdgpu.ids

I think the path of this file in the repository should be
amdgpu/amdgpu.ids rather than include/drm/amdgpu.ids.

Re: [PATCH 3/3] drm/amdgpu/radeon: Use radeon by default for CIK GPUs

2017-05-29 Thread Michel Dänzer
On 29/05/17 11:24 PM, Kai Wasserbäch wrote:
> Hey Michel,
> Michel Dänzer wrote on 29.05.2017 11:20:
>> From: Michel Dänzer <michel.daen...@amd.com>
>>
>> Even if CONFIG_DRM_AMDGPU_CIK is enabled.
>>
>> There is no feature parity yet for CIK, in particular amdgpu doesn't
>> support HDMI/DisplayPort without DC.
> 
> that can't be correct.

Indeed, I meant "HDMI/DisplayPort audio", sorry for the confusion. Will
be fixed in v2.


>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
>> b/drivers/gpu/drm/amd/amdgpu/Kconfig
>> index 8d36087fc186..e0121f8b436e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>> @@ -17,11 +17,11 @@ config DRM_AMDGPU_CIK
>>  help
>>Choose this option if you want to enable support for CIK asics.
>>  
>> -  CIK is already supported in radeon. If you enable this option,
>> -  support for CIK will be provided by amdgpu and disabled in
>> -  radeon by default. Use module options to override this:
>> +  CIK is already supported in radeon. Support for SI in amdgpu
>> +  will be disabled by default and is still provided by radeon.
>> +  Use module options to override this:
>>  
>> -  radeon.cik_support=1 amdgpu.cik_support=0
>> +  radeon.cik_support=0 amdgpu.cik_support=1
> 
> What happens if I have radeon blacklisted? Do I still need to set this
> additional flag?

Yes, you still need to set amdgpu.cik_support=1.

> If so I'd kindly request to at least fall back to amdgpu if somebody
> blacklisted radeon (or didn't compile it).

I'm afraid that's not possible, at least I don't know how it could be done.

The intention is that these options provide a cleaner way than
blacklisting for choosing between the drivers, so the options should be
used instead of blacklisting.


> Maybe issue a warning that not all features are supported yet.

The drivers print a message when they're ignoring a GPU due to these
options.


-- 
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 1/3] drm/amdgpu: Really leave SI support disabled by default

2017-05-29 Thread Michel Dänzer
On 30/05/17 02:18 AM, Christian König wrote:
> Am 29.05.2017 um 11:20 schrieb Michel Dänzer:
>> From: Michel Dänzer <michel.daen...@amd.com>
>>
>> The default option value didn't match the help text and intention.
>>
>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
> 
> I'm still unsure about the last one. The feature parity is a good
> argument but on the other hand we want people to use amdgpu for CIK
> these days, don't we?

We want to make it easy for people to test amdgpu on CIK, which is what
the options added by Felix are for. IMO we should not flip the default
(upstream) before there is feature parity.


> Anyway Reviewed-by: Christian König <christian.koe...@amd.com>.

Thanks, I assume that applies to the whole series?


-- 
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 3/3] drm/amdgpu/radeon: Use radeon by default for CIK GPUs

2017-05-29 Thread Michel Dänzer
On 29/05/17 06:20 PM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
> 
> Even if CONFIG_DRM_AMDGPU_CIK is enabled.
> 
> There is no feature parity yet for CIK, in particular amdgpu doesn't
> support HDMI/DisplayPort without DC.
> 
> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>

[...]

> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
> b/drivers/gpu/drm/amd/amdgpu/Kconfig
> index 8d36087fc186..e0121f8b436e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> @@ -17,11 +17,11 @@ config DRM_AMDGPU_CIK
>   help
> Choose this option if you want to enable support for CIK asics.
>  
> -   CIK is already supported in radeon. If you enable this option,
> -   support for CIK will be provided by amdgpu and disabled in
> -   radeon by default. Use module options to override this:
> +   CIK is already supported in radeon. Support for SI in amdgpu

Consider this "SI" typo fixed in v2.


-- 
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 2/3] drm/amdgpu: Remove two ! operations in an if condition

2017-05-30 Thread Michel Dänzer
On 31/05/17 06:47 AM, Alex Xie wrote:
>  Make the code easier to understand.
> 
> Signed-off-by: Alex Xie <alexbin@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 7a323f9..9fdeb82 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -680,9 +680,11 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring 
> *ring,
>  
>   if (amdgpu_vm_had_gpu_reset(adev, id))
>   return true;
> - if (!vm_flush_needed && !gds_switch_needed)
> +
> + if (vm_flush_needed || gds_switch_needed)
> + return true;
> + else
>   return false;

This can be further simplified to

    return vm_flush_needed || gds_switch_needed;

With that,

Reviewed-by: Michel Dänzer <michel.daen...@amd.com>


-- 
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 3/3] drm/amdgpu/radeon: Use radeon by default for CIK GPUs

2017-05-30 Thread Michel Dänzer
On 30/05/17 06:17 PM, Grazvydas Ignotas wrote:
> On Tue, May 30, 2017 at 6:30 AM, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 29/05/17 06:20 PM, Michel Dänzer wrote:
>>> From: Michel Dänzer <michel.daen...@amd.com>
>>>
>>> Even if CONFIG_DRM_AMDGPU_CIK is enabled.
>>>
>>> There is no feature parity yet for CIK, in particular amdgpu doesn't
>>> support HDMI/DisplayPort without DC.
>>>
>>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
>>
>> [...]
>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
>>> b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> index 8d36087fc186..e0121f8b436e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> @@ -17,11 +17,11 @@ config DRM_AMDGPU_CIK
>>>   help
>>> Choose this option if you want to enable support for CIK asics.
>>>
>>> -   CIK is already supported in radeon. If you enable this option,
>>> -   support for CIK will be provided by amdgpu and disabled in
>>> -   radeon by default. Use module options to override this:
>>> +   CIK is already supported in radeon. Support for SI in amdgpu
>>
>> Consider this "SI" typo fixed in v2.
> 
> While you are here, what about adding the full codenames here? You
> can't expect every user configuring the kernel to know what SI/CIK
> means, it's not even documented in
> https://www.x.org/wiki/RadeonFeature/ or wikipedia, while the long
> codenames are.

That's out of scope for this series, and I definitely won't get around
to doing that before next week. Anybody feel free to beat me to it.


-- 
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 libdrm v4 1/1] amdgpu: move asic id table to a separate file

2017-05-30 Thread Michel Dänzer
On 31/05/17 07:31 AM, Li, Samuel wrote:
> From: Michel Dänzer [mailto:mic...@daenzer.net] 
>> On 30/05/17 06:16 AM, Samuel Li wrote:
>> 
>>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
>>> file mode 100644 index 000..a43ca33
>>> --- /dev/null
>>> +++ b/amdgpu/amdgpu_asic_id.c
>> 
>> [...]
>> 
>>> +#include "xf86drm.h"
>>> +#include "amdgpu_drm.h"
>> 
>> Should be
>> 
>> #include 
>> #include 
>> 
>> since these header files are not located in the same directory as
>> amdgpu_asic_id.c.
> 
>  [Sam] Actually, "" is used to include programmer-defined header files,
> and <>  is used for files pre-designated by the compiler/IDE.

The only difference between the two is that #include "" first looks for
the header file in the same directory where the file containing the
#include directive (not necessarily the same as the original *.c file
passed to the compiler/preprocessor) is located, after that it looks in
the same paths in the same order as <>. So "" only really makes sense
when the header file is in the same directory as the file including it.


>>> @@ -267,6 +274,11 @@ int amdgpu_device_initialize(int fd,
>>> amdgpu_vamgr_init(>vamgr_32, start, max,
>>>   dev->dev_info.virtual_address_alignment);
>>>  
>>> +   r = amdgpu_parse_asic_ids(>asic_ids);
>>> +   if (r)
>>> +   fprintf(stderr, "%s: Can not parse asic ids, 0x%x.",
>>> +   __func__, r);
>> 
>> "Cannot parse ASIC IDs"
>> 
>> Also, there should be curly braces around a multi-line statement.
> 
> [Sam] Can be done. However, it is still a single statement. Does it matter?

It might not be strictly required, but I think it does make the code
clearer in this case.


>>> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids
>>> new file mode 100644
>>> index 000..6d6b944
>>> --- /dev/null
>>> +++ b/include/drm/amdgpu.ids
>> 
>> I think the path of this file in the repository should be
>> amdgpu/amdgpu.ids rather than include/drm/amdgpu.ids.
> 
> [Sam] The file is going to be shared with radeon.

We can cross that bridge when we get there. Meanwhile, it's not a header
file and not installed under $prefix/include/, so it doesn't belong in
include/.


>>> @@ -0,0 +1,170 @@
>>> +# List of AMDGPU ID's
>> 
>> This should say "IDs" instead of "ID's".
>> 
>> 
>>> +67FF,  CF, 67FF:CF
>>> +67FF,  EF, 67FF:EF
>> 
>> There should be no such dummy entries in the file. If it's useful,
>> amdgpu_get_marketing_name can return a dummy string based on the PCI ID
>> and revision when there's no matching entry in the file.
> 
> [Sam] I forwarded another thread to you.

Please make your argument explicitly, for the benefit of non-AMD readers
of the amd-gfx list.

Anyway, I don't think that invalidates what I wrote, and Alex seems to
agree. "67FF:CF" isn't a marketing name, so there should be no such
entries in this file. It's not necessary anyway; assuming it's useful
for amdgpu_get_marketing_name to return such "names", it can generate
them on the fly when there is no matching entry in the file.

Ideally the issues above should be fixed in the original file we get
from marketing (?), but meanwhile / failing that we should fix them up
(and can easily with Git).


-- 
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 libdrm v4 1/1] amdgpu: move asic id table to a separate file

2017-06-01 Thread Michel Dänzer
On 01/06/17 11:27 PM, Li, Samuel wrote:
>> If amdgpu.ids living in the amdgpu directory prevents it from being used by 
>> libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
>> "data".
>>> README is also located in this directory.
>> Not the same thing. It documents things about the header files, and doesn't 
>> get installed anywhere.
> I think that is a personal preference.

If you're referring to naming the new directory "data", that's just a
suggestion. Another possibility is to simply put it in the toplevel
directory.

What I wrote about include/drm/README is easily verifiable fact.


>>> My preference is to pass the names only, not to audit from a coder's 
>>> view ...
>> That's not how we do things.
> It is a data file, not really a part of code.

There is nothing magic about it. It's subject to the review process just
like any other file in the repository.

BTW, if you put the file outside of the amdgpu directory, you should
send the patch to the dri-devel mailing list for review as well.


> It shall be your preference to decide how much time you would like to
> spend in auditing the names :)

It shouldn't take as much time as we've spent talking about it in this
thread. :}


-- 
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 libdrm v6 1/1] amdgpu: move asic id table to a separate file

2017-06-07 Thread Michel Dänzer
On 07/06/17 08:12 PM, Emil Velikov wrote:
> On 7 June 2017 at 09:40, Michel Dänzer <mic...@daenzer.net> wrote:
>> On 06/06/17 10:43 PM, Emil Velikov wrote:
>>> On 31 May 2017 at 21:22, Samuel Li <samuel...@amd.com> wrote:
>>>
>>>> --- /dev/null
>>>> +++ b/amdgpu/amdgpu_asic_id.c
>>>
>>>> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
>>>> +{
>>>> +   char *buf, *saveptr;
>>>> +   char *s_did;
>>>> +   char *s_rid;
>>>> +   char *s_name;
>>>> +   char *endptr;
>>>> +   int r = 0;
>>>> +
>>>> +   buf = strdup(line);
>>> You don't need the extra strdup here if you use strchr over strtok.
>>
>> Beware that without strdup here, amdgpu_parse_asic_ids must set line =
>> NULL after table_line++, so that getline allocates a new buffer for the
>> next line.
>>
> A simple "line = NULL" will lead to a memory leak, AFAICT.
>
> In either case, I'm a bit baffled how that is affected by the
> presence/lack of strdup?
> We don't alter or reuse the backing storage only
> strcmp/isblank/strtol/strdup it.

Oh, I missed that id->marketing_name is strdup'd again.

Anyway, it's probably better not to change the logic too much at this
point, other than anything needed to fix immediate bugs. It can always
be improved with follow-up patches.


-- 
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: amd-gfx Digest, Vol 13, Issue 29

2017-06-06 Thread Michel Dänzer

Hi Alex,


> As I work more for open source driver, this digest mode becomes more
> annoying.
> 
> I didn't even know that amd-gfx can send individual emails to me. As a
> result, I did not know how to ask  correct question about my puzzling. I
> did ask around once.
> 
> Now I wondered how I enabled the digest mode. I think we should disable
> this option. I am thinking there are other victims of this digest mode.

Digest mode is disabled by default, it has to be enabled explicitly by
the subscriber. I don't want to disable digest mode completely for the
list, it can be useful for people who just want to read the list, not
post to it.


-- 
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 libdrm v6 1/1] amdgpu: move asic id table to a separate file

2017-06-07 Thread Michel Dänzer
On 06/06/17 10:43 PM, Emil Velikov wrote:
> On 31 May 2017 at 21:22, Samuel Li <samuel...@amd.com> wrote:
> 
>> --- /dev/null
>> +++ b/amdgpu/amdgpu_asic_id.c
> 
>> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
>> +{
>> +   char *buf, *saveptr;
>> +   char *s_did;
>> +   char *s_rid;
>> +   char *s_name;
>> +   char *endptr;
>> +   int r = 0;
>> +
>> +   buf = strdup(line);
> You don't need the extra strdup here if you use strchr over strtok.

Beware that without strdup here, amdgpu_parse_asic_ids must set line =
NULL after table_line++, so that getline allocates a new buffer for the
next line.


>> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>> +{
> 
>> +   /* 1st valid line is file version */
>> +   while ((n = getline(, , fp)) != -1) {
>> +   /* trim trailing newline */
>> +   if (line[n - 1] == '\n')
>> +   line[n - 1] = '\0';
> Why do we need this - afaict none of the parsing code cares if we have
> \n or not?

The \n has to be removed somehow, otherwise it ends up as part of the
marketing name returned to the application.


>> +   /* end of table */
>> +   id = asic_id_table + table_size;
>> +   memset(id, 0, sizeof(struct amdgpu_asic_id));
> Here one clears the sentinel, which is needed as we hit realloc above, 
> correct?
> 
>> +   asic_id_table = realloc(asic_id_table, (table_size+1) *
>> +   sizeof(struct amdgpu_asic_id));
> But why do we realloc again?

I asked for that, in order not to waste memory for unused table entries.


>> +free:
>> +   free(line);
>> +
>> +   if (r && asic_id_table) {
>> +   while (table_size--) {
>> +   id = asic_id_table + table_size;
>> +   free(id->marketing_name);
>> +   }
>> +   free(asic_id_table);
>> +   asic_id_table = NULL;
>> +   }
>> +close:
>> +   fclose(fp);
>> +
>> +   *p_asic_id_table = asic_id_table;
>> +
> Please don't entwine the error path with the normal one.
> 
> Setting *p_asic_id_table (or any user provided pointer) when the
> function fails is bad design.

I don't really see the issue with that; it's fine for the only caller of
this function.


-- 
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


[PATCH xf86-video-amdgpu] Improve AMDGPUPreInitAccel_KMS log messages

2017-06-07 Thread Michel Dänzer
From: Michel Dänzer <michel.daen...@amd.com>

Now it should always be clear in the log file why acceleration isn't
enabled.

Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
 src/amdgpu_glamor.c |  3 ---
 src/amdgpu_kms.c| 23 +--
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/amdgpu_glamor.c b/src/amdgpu_glamor.c
index 5583cd382..197592aa0 100644
--- a/src/amdgpu_glamor.c
+++ b/src/amdgpu_glamor.c
@@ -81,9 +81,6 @@ Bool amdgpu_glamor_pre_init(ScrnInfoPtr scrn)
pointer glamor_module;
CARD32 version;
 
-   if (!info->dri2.available)
-   return FALSE;
-
if (scrn->depth < 24) {
xf86DrvMsg(scrn->scrnIndex, X_ERROR,
   "glamor requires depth >= 24, disabling.\n");
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 69d61943d..784f7388a 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1191,19 +1191,22 @@ static Bool AMDGPUPreInitAccel_KMS(ScrnInfoPtr pScrn)
 
if (info->dri2.available)
info->gbm = gbm_create_device(pAMDGPUEnt->fd);
-   if (info->gbm == NULL)
-   info->dri2.available = FALSE;
 
-   if (use_glamor &&
-   amdgpu_glamor_pre_init(pScrn))
-   return TRUE;
-
-   if (info->dri2.available)
-   return TRUE;
+   if (info->gbm) {
+   if (!use_glamor ||
+   amdgpu_glamor_pre_init(pScrn))
+   return TRUE;
+   } else {
+   xf86DrvMsg(pScrn->scrnIndex, X_WARNING,
+  "gbm_create_device returned NULL, using "
+  "ShadowFB\n");
+   }
+   } else {
+   xf86DrvMsg(pScrn->scrnIndex, X_CONFIG,
+  "GPU acceleration disabled, using ShadowFB\n");
}
 
-   xf86DrvMsg(pScrn->scrnIndex, X_INFO,
-  "GPU accel disabled or not working, using shadowfb for 
KMS\n");
+   info->dri2.available = FALSE;
info->shadow_fb = TRUE;
if (!xf86LoadSubModule(pScrn, "shadow"))
info->shadow_fb = FALSE;
-- 
2.11.0

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


Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

2017-06-01 Thread Michel Dänzer
On 01/06/17 12:32 AM, Li, Samuel wrote:
>> From: Michel Dänzer [mailto:mic...@daenzer.net] 
>> On 31/05/17 07:31 AM, Li, Samuel wrote:
>>> From: Michel Dänzer [mailto:mic...@daenzer.net]
>>>> On 30/05/17 06:16 AM, Samuel Li wrote:
>>>>
>>>>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new 
>>>>> file mode 100644 index 000..a43ca33
>>>>> --- /dev/null
>>>>> +++ b/amdgpu/amdgpu_asic_id.c
>>>>
>>>> [...]
>>>>
>>>>> +#include "xf86drm.h"
>>>>> +#include "amdgpu_drm.h"
>>>>
>>>> Should be
>>>>
>>>> #include 
>>>> #include 
>>>>
>>>> since these header files are not located in the same directory as 
>>>> amdgpu_asic_id.c.
>>>
>>>  [Sam] Actually, "" is used to include programmer-defined header 
>>> files, and <>  is used for files pre-designated by the compiler/IDE.
>> 
>> The only difference between the two is that #include "" first looks for the 
>> header file in the same directory where the file containing the #include 
>> directive (not necessarily the same as the original *.c file passed to the 
>> compiler/preprocessor) is located, after that it looks in the same paths in 
>> the same order as <>. So "" only really makes sense when the header file is 
>> in the same directory as the file including it.
> 
> [Sam] Please see here
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

Thanks, I didn't know different search paths can apply with "" vs <>.
One learns something new every day. :) You can ignore the above then.


>>>>> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids new 
>>>>> file mode 100644 index 000..6d6b944
>>>>> --- /dev/null
>>>>> +++ b/include/drm/amdgpu.ids
>>>>
>>>> I think the path of this file in the repository should be 
>>>> amdgpu/amdgpu.ids rather than include/drm/amdgpu.ids.
>>>
>>> [Sam] The file is going to be shared with radeon.
>> 
>> We can cross that bridge when we get there. Meanwhile, it's not a header 
>> file and not installed under $prefix/include/, so it doesn't belong in 
>> include/.
> [Sam] I am planning to do it right after this.

If amdgpu.ids living in the amdgpu directory prevents it from being used
by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
"data".

> README is also located in this directory.

Not the same thing. It documents things about the header files, and
doesn't get installed anywhere.


>>>>> @@ -0,0 +1,170 @@
>>>>> +# List of AMDGPU ID's
>>>>
>>>> This should say "IDs" instead of "ID's".
>>>>
>>>>
>>>>> +67FF,CF, 67FF:CF
>>>>> +67FF,EF, 67FF:EF
>>>>
>>>> There should be no such dummy entries in the file. If it's useful, 
>>>> amdgpu_get_marketing_name can return a dummy string based on the PCI 
>>>> ID and revision when there's no matching entry in the file.
>>>
>>> [Sam] I forwarded another thread to you.
>> 
>> Please make your argument explicitly, for the benefit of non-AMD readers of 
>> the amd-gfx list.
>> Anyway, I don't think that invalidates what I wrote, and Alex seems to 
>> agree. "67FF:CF" isn't a marketing name, so there should be no such entries 
>> in this file. It's not necessary anyway; assuming it's useful for 
>> amdgpu_get_marketing_name to return such "names", it can generate them on 
>> the fly when there is no matching entry in the file.
>> Ideally the issues above should be fixed in the original file we get from 
>> marketing (?), but meanwhile / failing that we should fix them up (and can 
>> easily with Git).
> 
> [Sam] Essentially marketing names are defined by Marketing. They are
> complicated as I can imagine. If you have questions regarding the
> names, the thread I forwarded has the contact you can use.
> IIRC, the hex format in marketing names has been used for very long
> time.

It's obviously not a "marketing name" but some kind of placeholder. We
don't need those in libdrm.

> My preference is to pass the names only, not to audit from a coder's
> view ...

That's not how we do things.


-- 
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


<    1   2   3   4   5   6   7   8   9   10   >