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

[Deucher, Alexander]

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Michel Dänzer
> Sent: Monday, May 29, 2017 4:08 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH xf86-video-amdgpu] Use reference counting for tracking
> KMS framebuffer lifetimes
> 
> 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>

Reviewed-by: Alex Deucher <alexander.deuc...@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(cr

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

[Michel Dänzer]

From: Michel Dänzer 

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 
---
 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) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
@@ -985,8 +1011,17 @@ amdgpu_scanout_flip(ScreenPtr pScreen,