On Fri, Jul 27, 2018 at 12:04 PM, Michel Dänzer <michel at daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer at 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 at 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 at amd.com>
Alex
> } drmmode_flipdata_rec, *drmmode_flipdata_ptr;
>
> struct drmmode_fb {
> --
> 2.18.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
