[PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

We were only storing the FB provided by the client, but on CRTCs with
TearFree enabled, we use a separate FB. This could cause
drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
could result in a hang when waiting for the pending flip to complete. We
were trying to avoid that by always clearing drmmode_crtc->flip_pending
when TearFree is enabled, but that wasn't reliable, because
drmmode_crtc->tear_free can already be FALSE at this point when
disabling TearFree.

Now that we're keeping track of each CRTC's flip FB separately,
drmmode_flip_handler can reliably clear flip_pending, and we no longer
need the TearFree hack.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 92f58c157..e58e15d7b 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -3051,17 +3051,21 @@ drmmode_flip_abort(xf86CrtcPtr crtc, void *event_data)
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
 	drmmode_flipdata_ptr flipdata = event_data;
+	int crtc_id = drmmode_get_crtc_id(crtc);
+	struct drmmode_fb **fb = &flipdata->fb[crtc_id];
+
+	if (drmmode_crtc->flip_pending == *fb) {
+		drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->flip_pending,
+				     NULL);
+	}
+	drmmode_fb_reference(pAMDGPUEnt->fd, fb, NULL);
 
 	if (--flipdata->flip_count == 0) {
 		if (!flipdata->fe_crtc)
 			flipdata->fe_crtc = crtc;
 		flipdata->abort(flipdata->fe_crtc, flipdata->event_data);
-		drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb, NULL);
 		free(flipdata);
 	}
-
-	drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->flip_pending,
-			     NULL);
 }
 
 static void
@@ -3070,6 +3074,8 @@ drmmode_flip_handler(xf86CrtcPtr crtc, uint32_t frame, uint64_t usec, void *even
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	drmmode_flipdata_ptr flipdata = event_data;
+	int crtc_id = drmmode_get_crtc_id(crtc);
+	struct drmmode_fb **fb = &flipdata->fb[crtc_id];
 
 	/* Is this the event whose info shall be delivered to higher level? */
 	if (crtc == flipdata->fe_crtc) {
@@ -3078,13 +3084,12 @@ drmmode_flip_handler(xf86CrtcPtr crtc, uint32_t frame, uint64_t usec, void *even
 		flipdata->fe_usec = usec;
 	}
 
-	drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb,
-			     flipdata->fb);
-	if (drmmode_crtc->tear_free ||
-	    drmmode_crtc->flip_pending == flipdata->fb) {
+	if (drmmode_crtc->flip_pending == *fb) {
 		drmmode_fb_reference(pAMDGPUEnt->fd,
 				     &drmmode_crtc->flip_pending, NULL);
 	}
+	drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb, *fb);
+	drmmode_fb_reference(pAMDGPUEnt->fd, fb, NULL);
 
 	if (--flipdata->flip_count == 0) {
 		/* Deliver MSC & UST from reference/current CRTC to flip event
@@ -3096,7 +3101,6 @@ 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, &flipdata->fb, NULL);
 		free(flipdata);
 	}
 }
@@ -3875,21 +3879,22 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
 	xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
 	xf86CrtcPtr crtc = NULL;
 	drmmode_crtc_private_ptr drmmode_crtc = config->crtc[0]->driver_private;
-	int i;
 	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;
+	int i = 0;
 
-	flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
+	flipdata = calloc(1, sizeof(*flipdata) + config->num_crtc *
+			  sizeof(flipdata->fb[0]));
 	if (!flipdata) {
 		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
 			   "flip queue: data alloc failed.\n");
 		goto error;
 	}
 
-	drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb,
-			     amdgpu_pixmap_get_fb(new_front));
-	if (!flipdata->fb) {
+	fb = amdgpu_pixmap_get_fb(new_front);
+	if (!fb) {
 		ErrorF("Failed to get FB for flip\n");
 		goto error;
 	}
@@ -3910,8 +3915,6 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
 	flipdata->fe_crtc = ref_crtc;
 
 	for (i = 0; i < config->num_crtc; i++) {
-		struct drmmode_fb *fb = flipdata->fb;
-
 		crtc = config->crtc[i];
 		drmmode_crtc = crtc->driver_private;
 
@@ -3947,8 +3950,9 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
 				goto next;
 			}
 
-			fb = amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
-			if (!fb) {
+			drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb[i],
+					     amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap));
+			if (!flipdata->fb[i]) {
 				ErrorF("Failed to get FB for TearFree flip\n");
 				goto error;
 			}
@@ -3963,6 +3967,8 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
 				amdgpu_drm_abort_entry(drmmode_crtc->scanout_update_pending);
 				drmmode_crtc->scanout_update_pending = 0;
 			}
+		} else {
+			drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb[i], fb);
 		}
 
 		if (crtc == ref_crtc) {
@@ -3988,8 +3994,8 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
 		}
 
 	next:
-		drmmode_fb_reference(pAMDGPUEnt->fd,
-				     &drmmode_crtc->flip_pending, fb);
+		drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->flip_pending,
+				     flipdata->fb[i]);
 		drm_queue_seq = 0;
 	}
 
@@ -4007,7 +4013,6 @@ error:
 		drmmode_flip_abort(crtc, flipdata);
 	else {
 		abort(NULL, data);
-		drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb, NULL);
 		free(flipdata);
 	}
 
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 8b949f79d..5618c6b40 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -74,7 +74,6 @@ typedef struct {
 } drmmode_rec, *drmmode_ptr;
 
 typedef struct {
-	struct drmmode_fb *fb;
 	void *event_data;
 	int flip_count;
 	unsigned int fe_frame;
@@ -82,6 +81,7 @@ typedef struct {
 	xf86CrtcPtr fe_crtc;
 	amdgpu_drm_handler_proc handler;
 	amdgpu_drm_abort_proc abort;
+	struct drmmode_fb *fb[0];
 } drmmode_flipdata_rec, *drmmode_flipdata_ptr;
 
 struct drmmode_fb {
-- 
2.18.0

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

Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Alex Deucher]

On Fri, Jul 27, 2018 at 12:04 PM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> We were only storing the FB provided by the client, but on CRTCs with
> TearFree enabled, we use a separate FB. This could cause
> drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
> could result in a hang when waiting for the pending flip to complete. We
> were trying to avoid that by always clearing drmmode_crtc->flip_pending
> when TearFree is enabled, but that wasn't reliable, because
> drmmode_crtc->tear_free can already be FALSE at this point when
> disabling TearFree.
>
> Now that we're keeping track of each CRTC's flip FB separately,
> drmmode_flip_handler can reliably clear flip_pending, and we no longer
> need the TearFree hack.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  src/drmmode_display.c | 47 ++++++++++++++++++++++++-------------------
>  src/drmmode_display.h |  2 +-
>  2 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 92f58c157..e58e15d7b 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -3051,17 +3051,21 @@ drmmode_flip_abort(xf86CrtcPtr crtc, void *event_data)
>         drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>         AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
>         drmmode_flipdata_ptr flipdata = event_data;
> +       int crtc_id = drmmode_get_crtc_id(crtc);
> +       struct drmmode_fb **fb = &flipdata->fb[crtc_id];
> +
> +       if (drmmode_crtc->flip_pending == *fb) {
> +               drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->flip_pending,
> +                                    NULL);
> +       }
> +       drmmode_fb_reference(pAMDGPUEnt->fd, fb, NULL);
>
>         if (--flipdata->flip_count == 0) {
>                 if (!flipdata->fe_crtc)
>                         flipdata->fe_crtc = crtc;
>                 flipdata->abort(flipdata->fe_crtc, flipdata->event_data);
> -               drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb, NULL);
>                 free(flipdata);
>         }
> -
> -       drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->flip_pending,
> -                            NULL);
>  }
>
>  static void
> @@ -3070,6 +3074,8 @@ drmmode_flip_handler(xf86CrtcPtr crtc, uint32_t frame, uint64_t usec, void *even
>         AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
>         drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>         drmmode_flipdata_ptr flipdata = event_data;
> +       int crtc_id = drmmode_get_crtc_id(crtc);
> +       struct drmmode_fb **fb = &flipdata->fb[crtc_id];
>
>         /* Is this the event whose info shall be delivered to higher level? */
>         if (crtc == flipdata->fe_crtc) {
> @@ -3078,13 +3084,12 @@ drmmode_flip_handler(xf86CrtcPtr crtc, uint32_t frame, uint64_t usec, void *even
>                 flipdata->fe_usec = usec;
>         }
>
> -       drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb,
> -                            flipdata->fb);
> -       if (drmmode_crtc->tear_free ||
> -           drmmode_crtc->flip_pending == flipdata->fb) {
> +       if (drmmode_crtc->flip_pending == *fb) {
>                 drmmode_fb_reference(pAMDGPUEnt->fd,
>                                      &drmmode_crtc->flip_pending, NULL);
>         }
> +       drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb, *fb);
> +       drmmode_fb_reference(pAMDGPUEnt->fd, fb, NULL);
>
>         if (--flipdata->flip_count == 0) {
>                 /* Deliver MSC & UST from reference/current CRTC to flip event
> @@ -3096,7 +3101,6 @@ 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, &flipdata->fb, NULL);
>                 free(flipdata);
>         }
>  }
> @@ -3875,21 +3879,22 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>         xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
>         xf86CrtcPtr crtc = NULL;
>         drmmode_crtc_private_ptr drmmode_crtc = config->crtc[0]->driver_private;
> -       int i;
>         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;
> +       int i = 0;
>
> -       flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
> +       flipdata = calloc(1, sizeof(*flipdata) + config->num_crtc *
> +                         sizeof(flipdata->fb[0]));
>         if (!flipdata) {
>                 xf86DrvMsg(scrn->scrnIndex, X_WARNING,
>                            "flip queue: data alloc failed.\n");
>                 goto error;
>         }
>
> -       drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb,
> -                            amdgpu_pixmap_get_fb(new_front));
> -       if (!flipdata->fb) {
> +       fb = amdgpu_pixmap_get_fb(new_front);
> +       if (!fb) {
>                 ErrorF("Failed to get FB for flip\n");
>                 goto error;
>         }
> @@ -3910,8 +3915,6 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>         flipdata->fe_crtc = ref_crtc;
>
>         for (i = 0; i < config->num_crtc; i++) {
> -               struct drmmode_fb *fb = flipdata->fb;
> -
>                 crtc = config->crtc[i];
>                 drmmode_crtc = crtc->driver_private;
>
> @@ -3947,8 +3950,9 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>                                 goto next;
>                         }
>
> -                       fb = amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
> -                       if (!fb) {
> +                       drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb[i],
> +                                            amdgpu_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap));
> +                       if (!flipdata->fb[i]) {
>                                 ErrorF("Failed to get FB for TearFree flip\n");
>                                 goto error;
>                         }
> @@ -3963,6 +3967,8 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>                                 amdgpu_drm_abort_entry(drmmode_crtc->scanout_update_pending);
>                                 drmmode_crtc->scanout_update_pending = 0;
>                         }
> +               } else {
> +                       drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb[i], fb);
>                 }
>
>                 if (crtc == ref_crtc) {
> @@ -3988,8 +3994,8 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>                 }
>
>         next:
> -               drmmode_fb_reference(pAMDGPUEnt->fd,
> -                                    &drmmode_crtc->flip_pending, fb);
> +               drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->flip_pending,
> +                                    flipdata->fb[i]);
>                 drm_queue_seq = 0;
>         }
>
> @@ -4007,7 +4013,6 @@ error:
>                 drmmode_flip_abort(crtc, flipdata);
>         else {
>                 abort(NULL, data);
> -               drmmode_fb_reference(pAMDGPUEnt->fd, &flipdata->fb, NULL);
>                 free(flipdata);
>         }
>
> diff --git a/src/drmmode_display.h b/src/drmmode_display.h
> index 8b949f79d..5618c6b40 100644
> --- a/src/drmmode_display.h
> +++ b/src/drmmode_display.h
> @@ -74,7 +74,6 @@ typedef struct {
>  } drmmode_rec, *drmmode_ptr;
>
>  typedef struct {
> -       struct drmmode_fb *fb;
>         void *event_data;
>         int flip_count;
>         unsigned int fe_frame;
> @@ -82,6 +81,7 @@ typedef struct {
>         xf86CrtcPtr fe_crtc;
>         amdgpu_drm_handler_proc handler;
>         amdgpu_drm_abort_proc abort;
> +       struct drmmode_fb *fb[0];

Don't some compilers have problems with zero sized arrays?  Other than
that looks good to me:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Alex

>  } drmmode_flipdata_rec, *drmmode_flipdata_ptr;
>
>  struct drmmode_fb {
> --
> 2.18.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Michel Dänzer]

On 2018-07-30 08:12 PM, Alex Deucher wrote:
> On Fri, Jul 27, 2018 at 12:04 PM, Michel Dänzer <michel@daenzer.net> wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> We were only storing the FB provided by the client, but on CRTCs with
>> TearFree enabled, we use a separate FB. This could cause
>> drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
>> could result in a hang when waiting for the pending flip to complete. We
>> were trying to avoid that by always clearing drmmode_crtc->flip_pending
>> when TearFree is enabled, but that wasn't reliable, because
>> drmmode_crtc->tear_free can already be FALSE at this point when
>> disabling TearFree.
>>
>> Now that we're keeping track of each CRTC's flip FB separately,
>> drmmode_flip_handler can reliably clear flip_pending, and we no longer
>> need the TearFree hack.
>>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>
>> [...]
>>
>> @@ -82,6 +81,7 @@ typedef struct {
>>         xf86CrtcPtr fe_crtc;
>>         amdgpu_drm_handler_proc handler;
>>         amdgpu_drm_abort_proc abort;
>> +       struct drmmode_fb *fb[0];
> 
> Don't some compilers have problems with zero sized arrays?

Are you thinking of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b7d245009e734588e553092f5c0b0bd788b3a55
? Per https://bugs.freedesktop.org/show_bug.cgi?id=66932#c24 , the
problem there was variable sized arrays being declared as [1], which
obviously can't work.

Declaring a [0] array at the end of a struct is a GCC extension, see
https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html#Zero-Length . clang
seems to handle this fine as well.

An alternative would be using the C99 syntax "[];" instead, but I think
the above is fine in this case.


> Other than that looks good to me:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

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

Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Alex Deucher]

On Tue, Jul 31, 2018 at 4:48 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-07-30 08:12 PM, Alex Deucher wrote:
>> On Fri, Jul 27, 2018 at 12:04 PM, Michel Dänzer <michel@daenzer.net> wrote:
>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> We were only storing the FB provided by the client, but on CRTCs with
>>> TearFree enabled, we use a separate FB. This could cause
>>> drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
>>> could result in a hang when waiting for the pending flip to complete. We
>>> were trying to avoid that by always clearing drmmode_crtc->flip_pending
>>> when TearFree is enabled, but that wasn't reliable, because
>>> drmmode_crtc->tear_free can already be FALSE at this point when
>>> disabling TearFree.
>>>
>>> Now that we're keeping track of each CRTC's flip FB separately,
>>> drmmode_flip_handler can reliably clear flip_pending, and we no longer
>>> need the TearFree hack.
>>>
>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> [...]
>>>
>>> @@ -82,6 +81,7 @@ typedef struct {
>>>         xf86CrtcPtr fe_crtc;
>>>         amdgpu_drm_handler_proc handler;
>>>         amdgpu_drm_abort_proc abort;
>>> +       struct drmmode_fb *fb[0];
>>
>> Don't some compilers have problems with zero sized arrays?
>
> Are you thinking of
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b7d245009e734588e553092f5c0b0bd788b3a55
> ? Per https://bugs.freedesktop.org/show_bug.cgi?id=66932#c24 , the
> problem there was variable sized arrays being declared as [1], which
> obviously can't work.
>
> Declaring a [0] array at the end of a struct is a GCC extension, see
> https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html#Zero-Length . clang
> seems to handle this fine as well.
>
> An alternative would be using the C99 syntax "[];" instead, but I think
> the above is fine in this case.

Yup, that was the issue I was thinking of.  No other concerns.

Alex


>
>
>> Other than that looks good to me:
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> 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

Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Johannes Hirte]

On 2018 Jul 27, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> We were only storing the FB provided by the client, but on CRTCs with
> TearFree enabled, we use a separate FB. This could cause
> drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
> could result in a hang when waiting for the pending flip to complete. We
> were trying to avoid that by always clearing drmmode_crtc->flip_pending
> when TearFree is enabled, but that wasn't reliable, because
> drmmode_crtc->tear_free can already be FALSE at this point when
> disabling TearFree.
> 
> Now that we're keeping track of each CRTC's flip FB separately,
> drmmode_flip_handler can reliably clear flip_pending, and we no longer
> need the TearFree hack.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Since this change I get a black screen when login into KDE Plasma. I
have to switch to linux console and back for getting the X11 screen.
Additional the Xorg.log is spammed with:

[   189.744] (WW) AMDGPU(0): get vblank counter failed: Invalid argument
[   189.828] (WW) AMDGPU(0): flip queue failed in amdgpu_scanout_flip: Device or resource busy, TearFree inactive until next modeset
[   189.828] (WW) AMDGPU(0): drmmode_wait_vblank failed for scanout update: Invalid argument
[   189.828] (WW) AMDGPU(0): drmmode_wait_vblank failed for scanout update: Invalid argument

The "flip queue failed" message appears only once, the other two are
much more often.

System is a Carrizo A10-8700B, kernel 4.17.13 + this patch:
https://bugzilla.kernel.org/attachment.cgi?id=276173


-- 
Regards,
  Johannes

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

[PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Michel Dänzer]

On 2018-08-10 09:06 AM, Johannes Hirte wrote:
> On 2018 Jul 27, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> We were only storing the FB provided by the client, but on CRTCs with
>> TearFree enabled, we use a separate FB. This could cause
>> drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
>> could result in a hang when waiting for the pending flip to complete. We
>> were trying to avoid that by always clearing drmmode_crtc->flip_pending
>> when TearFree is enabled, but that wasn't reliable, because
>> drmmode_crtc->tear_free can already be FALSE at this point when
>> disabling TearFree.
>>
>> Now that we're keeping track of each CRTC's flip FB separately,
>> drmmode_flip_handler can reliably clear flip_pending, and we no longer
>> need the TearFree hack.
>>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> 
> Since this change I get a black screen when login into KDE Plasma. I
> have to switch to linux console and back for getting the X11 screen.
> Additional the Xorg.log is spammed with:
> 
> [   189.744] (WW) AMDGPU(0): get vblank counter failed: Invalid argument
> [   189.828] (WW) AMDGPU(0): flip queue failed in amdgpu_scanout_flip: Device or resource busy, TearFree inactive until next modeset
> [   189.828] (WW) AMDGPU(0): drmmode_wait_vblank failed for scanout update: Invalid argument
> [   189.828] (WW) AMDGPU(0): drmmode_wait_vblank failed for scanout update: Invalid argument
> 
> The "flip queue failed" message appears only once, the other two are
> much more often.
> 
> System is a Carrizo A10-8700B, kernel 4.17.13 + this patch:
> https://bugzilla.kernel.org/attachment.cgi?id=276173

Does https://patchwork.freedesktop.org/patch/244860/ fix 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 xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec

[Johannes Hirte]

On 2018 Aug 16, Michel Dänzer wrote:
> On 2018-08-10 09:06 AM, Johannes Hirte wrote:
> > On 2018 Jul 27, Michel Dänzer wrote:
> >> From: Michel Dänzer <michel.daenzer@amd.com>
> >>
> >> We were only storing the FB provided by the client, but on CRTCs with
> >> TearFree enabled, we use a separate FB. This could cause
> >> drmmode_flip_handler to fail to clear drmmode_crtc->flip_pending, which
> >> could result in a hang when waiting for the pending flip to complete. We
> >> were trying to avoid that by always clearing drmmode_crtc->flip_pending
> >> when TearFree is enabled, but that wasn't reliable, because
> >> drmmode_crtc->tear_free can already be FALSE at this point when
> >> disabling TearFree.
> >>
> >> Now that we're keeping track of each CRTC's flip FB separately,
> >> drmmode_flip_handler can reliably clear flip_pending, and we no longer
> >> need the TearFree hack.
> >>
> >> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > Since this change I get a black screen when login into KDE Plasma. I
> > have to switch to linux console and back for getting the X11 screen.
> > Additional the Xorg.log is spammed with:
> > 
> > [   189.744] (WW) AMDGPU(0): get vblank counter failed: Invalid argument
> > [   189.828] (WW) AMDGPU(0): flip queue failed in amdgpu_scanout_flip: Device or resource busy, TearFree inactive until next modeset
> > [   189.828] (WW) AMDGPU(0): drmmode_wait_vblank failed for scanout update: Invalid argument
> > [   189.828] (WW) AMDGPU(0): drmmode_wait_vblank failed for scanout update: Invalid argument
> > 
> > The "flip queue failed" message appears only once, the other two are
> > much more often.
> > 
> > System is a Carrizo A10-8700B, kernel 4.17.13 + this patch:
> > https://bugzilla.kernel.org/attachment.cgi?id=276173
> 
> Does https://patchwork.freedesktop.org/patch/244860/ fix it?
> 
Yes, this fixed it.

-- 
Regards,
  Johannes

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