* [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec
@ 2018-07-27 16:04 Michel Dänzer
[not found] ` <20180727160452.18212-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Michel Dänzer @ 2018-07-27 16:04 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply related [flat|nested] 7+ messages in thread[parent not found: <20180727160452.18212-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org>]
* Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec [not found] ` <20180727160452.18212-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org> @ 2018-07-30 18:12 ` Alex Deucher [not found] ` <CADnq5_M+3OYaT1KdAZzqNv9uc-2ULQDots4CigjxMLLA2u3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-08-10 7:06 ` Johannes Hirte 1 sibling, 1 reply; 7+ messages in thread From: Alex Deucher @ 2018-07-30 18:12 UTC (permalink / raw) To: Michel Dänzer; +Cc: amd-gfx list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CADnq5_M+3OYaT1KdAZzqNv9uc-2ULQDots4CigjxMLLA2u3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec [not found] ` <CADnq5_M+3OYaT1KdAZzqNv9uc-2ULQDots4CigjxMLLA2u3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2018-07-31 8:48 ` Michel Dänzer [not found] ` <39de6fce-5abc-cb0a-0ac9-2ac03d0fe415-otUistvHUpPR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 7+ messages in thread From: Michel Dänzer @ 2018-07-31 8:48 UTC (permalink / raw) To: Alex Deucher; +Cc: amd-gfx list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <39de6fce-5abc-cb0a-0ac9-2ac03d0fe415-otUistvHUpPR7s880joybQ@public.gmane.org>]
* Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec [not found] ` <39de6fce-5abc-cb0a-0ac9-2ac03d0fe415-otUistvHUpPR7s880joybQ@public.gmane.org> @ 2018-07-31 15:01 ` Alex Deucher 0 siblings, 0 replies; 7+ messages in thread From: Alex Deucher @ 2018-07-31 15:01 UTC (permalink / raw) To: Michel Dänzer; +Cc: amd-gfx list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec [not found] ` <20180727160452.18212-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org> 2018-07-30 18:12 ` Alex Deucher @ 2018-08-10 7:06 ` Johannes Hirte 2018-08-16 14:38 ` Michel Dänzer 1 sibling, 1 reply; 7+ messages in thread From: Johannes Hirte @ 2018-08-10 7:06 UTC (permalink / raw) To: Michel Dänzer; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec 2018-08-10 7:06 ` Johannes Hirte @ 2018-08-16 14:38 ` Michel Dänzer [not found] ` <1705f9ac-9824-a0f7-0556-6c3f9006bed6-otUistvHUpPR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 7+ messages in thread From: Michel Dänzer @ 2018-08-16 14:38 UTC (permalink / raw) To: Johannes Hirte; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <1705f9ac-9824-a0f7-0556-6c3f9006bed6-otUistvHUpPR7s880joybQ@public.gmane.org>]
* Re: [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec [not found] ` <1705f9ac-9824-a0f7-0556-6c3f9006bed6-otUistvHUpPR7s880joybQ@public.gmane.org> @ 2018-08-17 12:22 ` Johannes Hirte 0 siblings, 0 replies; 7+ messages in thread From: Johannes Hirte @ 2018-08-17 12:22 UTC (permalink / raw) To: Michel Dänzer; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-17 12:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-27 16:04 [PATCH xf86-video-amdgpu] Store FB for each CRTC in drmmode_flipdata_rec Michel Dänzer
[not found] ` <20180727160452.18212-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org>
2018-07-30 18:12 ` Alex Deucher
[not found] ` <CADnq5_M+3OYaT1KdAZzqNv9uc-2ULQDots4CigjxMLLA2u3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-07-31 8:48 ` Michel Dänzer
[not found] ` <39de6fce-5abc-cb0a-0ac9-2ac03d0fe415-otUistvHUpPR7s880joybQ@public.gmane.org>
2018-07-31 15:01 ` Alex Deucher
2018-08-10 7:06 ` Johannes Hirte
2018-08-16 14:38 ` Michel Dänzer
[not found] ` <1705f9ac-9824-a0f7-0556-6c3f9006bed6-otUistvHUpPR7s880joybQ@public.gmane.org>
2018-08-17 12:22 ` Johannes Hirte
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.