drmmode: refactor set_flip/blit error paths and page flip me... [chromiumos/third_party/xf86-video-armsoc : master]

[John Sheu (Gerrit)]

John Sheu has posted comments on this change.

Change subject: drmmode: refactor set_flip/blit error paths and page flip
messaging
......................................................................


Patch Set 1: Code-Review+2

Looking good, thanks!

--
To view, visit https://chromium-review.googlesource.com/190692
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/xf86-video-armsoc
Gerrit-Branch: master
Gerrit-Owner: Daniel Kurtz <djk...@chromium.org>
Gerrit-Reviewer: Dominik Behr <db...@chromium.org>
Gerrit-Reviewer: John Sheu <sh...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>
Gerrit-HasComments: No

[Dominik Behr (Gerrit)]

Dominik Behr has posted comments on this change.

Change subject: drmmode: refactor set_flip/blit error paths and page flip
messaging
......................................................................


Patch Set 1: Code-Review+2

[Daniel Kurtz (Gerrit)]

Hello John Sheu, Dominik Behr,

I'd like you to reexamine a change. Please visit

https://chromium-review.googlesource.com/190692

to look at the new patch set (#2).

Change subject: drmmode: refactor set_flip/blit error paths and page flip
messaging
......................................................................

drmmode: refactor set_flip/blit error paths and page flip messaging

Refactor "set_crtc for flip mode" and "set_crtc for blit mode" into a
set of helper functions, since how we set them is the same when
transitioning modes and the unwind fallback case when setting to blit/flip
mode fails.

Also, fix up error message in drmmode_page_flip, add a debug message, and
reduce the scope of some variables.

Signed-off-by: Daniel Kurtz <djk...@chromium.org>

BUG=chrome-os-partner:26387
TEST=connect Peach Pi to Optoma projector, switch between extended
desktop and mirror mode multiple times. It should not hang.
Change

Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265
---
M src/drmmode_display.c
1 file changed, 84 insertions(+), 57 deletions(-)

--
To view, visit https://chromium-review.googlesource.com/190692
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265
Gerrit-PatchSet: 2

[Daniel Kurtz (Gerrit)]

Hello John Sheu, Dominik Behr,

I'd like you to reexamine a change. Please visit

https://chromium-review.googlesource.com/190692

to look at the new patch set (#3).

Change subject: drmmode: refactor set_flip/blit error paths and page flip
messaging
......................................................................

drmmode: refactor set_flip/blit error paths and page flip messaging

Refactor "set_crtc for flip mode" and "set_crtc for blit mode" into a
set of helper functions, since how we set them is the same when
transitioning modes and the unwind fallback case when setting to blit/flip
mode fails.

Also, fix up error message in drmmode_page_flip, add a debug message, and
reduce the scope of some variables.

Signed-off-by: Daniel Kurtz <djk...@chromium.org>

BUG=chrome-os-partner:26387
TEST=connect Peach Pi to Optoma projector, switch between extended
desktop and mirror mode multiple times. It should not hang.
Change

Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265
---
M src/drmmode_display.c
1 file changed, 84 insertions(+), 57 deletions(-)


--
To view, visit https://chromium-review.googlesource.com/190692
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265

Gerrit-PatchSet: 3

[Daniel Kurtz (Gerrit)]

Daniel Kurtz has posted comments on this change.

Change subject: drmmode: refactor set_flip/blit error paths and page flip
messaging
......................................................................

Patch Set 3: Code-Review+2 Commit-Queue+1 Verified+1

Carry-forward dbehr +2 sheu +2

--
To view, visit https://chromium-review.googlesource.com/190692
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/xf86-video-armsoc
Gerrit-Branch: master
Gerrit-Owner: Daniel Kurtz <djk...@chromium.org>

Gerrit-Reviewer: Daniel Kurtz <djk...@chromium.org>

Gerrit-Reviewer: Dominik Behr <db...@chromium.org>
Gerrit-Reviewer: John Sheu <sh...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>

Gerrit-HasComments: No

[chrome-internal-fetch (Gerrit)]

chrome-internal-fetch has submitted this change and it was merged.

Change subject: drmmode: refactor set_flip/blit error paths and page flip
messaging
......................................................................


drmmode: refactor set_flip/blit error paths and page flip messaging

Refactor "set_crtc for flip mode" and "set_crtc for blit mode" into a
set of helper functions, since how we set them is the same when
transitioning modes and the unwind fallback case when setting to blit/flip
mode fails.

Also, fix up error message in drmmode_page_flip, add a debug message, and
reduce the scope of some variables.

Signed-off-by: Daniel Kurtz <djk...@chromium.org>

BUG=chrome-os-partner:26387
TEST=connect Peach Pi to Optoma projector, switch between extended
desktop and mirror mode multiple times. It should not hang.
Change

Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265

Reviewed-on: https://chromium-review.googlesource.com/190692
Reviewed-by: Daniel Kurtz <djk...@chromium.org>
Commit-Queue: Daniel Kurtz <djk...@chromium.org>
Tested-by: Daniel Kurtz <djk...@chromium.org>

---
M src/drmmode_display.c
1 file changed, 84 insertions(+), 57 deletions(-)

Approvals:
Daniel Kurtz: Looks good to me, approved; Ready; Verified



diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index e55cfbe..ad54744 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -500,12 +500,55 @@
return 0;
}

+static Bool drmmode_set_blit_crtc(ScrnInfoPtr pScrn, xf86CrtcPtr crtc)
+{
+ OMAPPtr pOMAP = OMAPPTR(pScrn);
+ int rc;
+
+ if (!crtc->enabled)
+ return TRUE;
+
+ /* Note: drmmode_set_crtc returns <0 on error, not a Bool */
+ rc = drmmode_set_crtc(pScrn, crtc, pOMAP->scanout, crtc->x, crtc->y);
+ if (rc) {
+ ERROR_MSG("[CRTC:%u] set root scanout failed",
+ drmmode_crtc_id(crtc));
+ drmmode_set_crtc_off(crtc);
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+static Bool drmmode_set_flip_crtc(ScrnInfoPtr pScrn, xf86CrtcPtr crtc)
+{
+ OMAPPtr pOMAP = OMAPPTR(pScrn);
+ OMAPScanoutPtr scanout;
+ int rc;
+
+ if (!crtc->enabled)
+ return TRUE;
+
+ scanout = drmmode_scanout_from_crtc(pOMAP->scanouts, crtc);
+ if (!scanout)
+ return TRUE;
+
+ /* Note: drmmode_set_crtc returns <0 on error, not a Bool */
+ rc = drmmode_set_crtc(pScrn, crtc, scanout->bo, 0, 0);
+ if (rc) {
+ ERROR_MSG("[CRTC:%u] set per-crtc scanout failed",
+ drmmode_crtc_id(crtc));
+ drmmode_set_crtc_off(crtc);
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
Bool drmmode_set_blit_mode(ScrnInfoPtr pScrn)
{
OMAPPtr pOMAP = OMAPPTR(pScrn);
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
- xf86CrtcPtr crtc;
- OMAPScanoutPtr scanout;
int ret, i;

if (pOMAP->flip_mode == OMAP_FLIP_DISABLED)
@@ -517,7 +560,7 @@

/* Only copy if source is valid. */
for (i = 0; i < MAX_SCANOUTS; i++) {
- scanout = &pOMAP->scanouts[i];
+ OMAPScanoutPtr scanout = &pOMAP->scanouts[i];

if (!scanout->bo)
continue;
@@ -532,40 +575,31 @@
}
}
for (i = 0; i < xf86_config->num_crtc; i++) {
- crtc = xf86_config->crtc[i];
-
- if (!crtc->enabled)
- continue;
-
- ret = drmmode_set_crtc(pScrn, crtc, pOMAP->scanout, crtc->x,
- crtc->y);
- if (ret) {
- ERROR_MSG("Set crtc to scanout failed");
- drmmode_set_crtc_off(crtc);
- /* try restoring other CRTCs to previous state*/
- while (--i >= 0) {
- crtc = xf86_config->crtc[i];
- if (!crtc->enabled)
- continue;
- scanout = drmmode_scanout_from_crtc(
- pOMAP->scanouts, crtc);
- if (!scanout)
- continue;
- drmmode_set_crtc(pScrn, crtc, scanout->bo, 0, 0);
- }
- return FALSE;
+ xf86CrtcPtr crtc = xf86_config->crtc[i];
+ if (!drmmode_set_blit_crtc(pScrn, crtc)) {
+ ERROR_MSG("[CRTC:%u] could not set blit mode",
+ drmmode_crtc_id(crtc));
+ goto unwind;
}
}
pOMAP->flip_mode = OMAP_FLIP_DISABLED;
return TRUE;
+
+unwind:
+ /* try restoring already transitioned CRTCs back to flip mode */
+ while (--i >= 0) {
+ xf86CrtcPtr crtc = xf86_config->crtc[i];
+ if (!drmmode_set_flip_crtc(pScrn, crtc))
+ ERROR_MSG("[CRTC:%u] could not restore flip mode",
+ drmmode_crtc_id(crtc));
+ }
+ return FALSE;
}

Bool drmmode_set_flip_mode(ScrnInfoPtr pScrn)
{
OMAPPtr pOMAP = OMAPPTR(pScrn);
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
- xf86CrtcPtr crtc;
- OMAPScanoutPtr scanout;
int ret, i;

if (pOMAP->flip_mode == OMAP_FLIP_ENABLED)
@@ -573,7 +607,7 @@

/* Only copy if destination is invalid. */
for (i = 0; i < MAX_SCANOUTS; i++) {
- scanout = &pOMAP->scanouts[i];
+ OMAPScanoutPtr scanout = &pOMAP->scanouts[i];

if (!scanout->bo)
continue;
@@ -591,32 +625,25 @@
}

for (i = 0; i < xf86_config->num_crtc; i++) {
- crtc = xf86_config->crtc[i];
-
- if (!crtc->enabled)
- continue;
-
- scanout = drmmode_scanout_from_crtc(pOMAP->scanouts, crtc);
- if (!scanout)
- continue;
-
- ret = drmmode_set_crtc(pScrn, crtc, scanout->bo, 0, 0);
- if (ret) {
- ERROR_MSG("Set crtc to crtc scanout failed");
- drmmode_set_crtc_off(crtc);
- /* try to restoring other CRTCs to previous state*/
- while (--i >= 0) {
- crtc = xf86_config->crtc[i];
- if (!crtc->enabled)
- continue;
- drmmode_set_crtc(pScrn, crtc, pOMAP->scanout,
- crtc->x, crtc->y);
- }
- return FALSE;
+ xf86CrtcPtr crtc = xf86_config->crtc[i];
+ if (!drmmode_set_flip_crtc(pScrn, crtc)) {
+ ERROR_MSG("[CRTC:%u] could not set flip mode",
+ drmmode_crtc_id(crtc));
+ goto unwind;
}
}
pOMAP->flip_mode = OMAP_FLIP_ENABLED;
return TRUE;
+
+unwind:
+ /* try restoring already transitioned CRTCs back to blit mode */
+ while (--i >= 0) {
+ xf86CrtcPtr crtc = xf86_config->crtc[i];
+ if (!drmmode_set_blit_crtc(pScrn, crtc))
+ ERROR_MSG("[CRTC:%u] could not restore blit mode",
+ drmmode_crtc_id(crtc));
+ }
+ return FALSE;
}

static Bool drmmode_update_scanouts(ScrnInfoPtr pScrn)
@@ -1716,8 +1743,6 @@
ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen);
OMAPPtr pOMAP = OMAPPTR(pScrn);
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
- xf86CrtcPtr crtc;
- drmmode_crtc_private_ptr drmmode_crtc;
int ret, i;
unsigned int flags = 0;

@@ -1728,8 +1753,9 @@
/* Flip all crtc's that match this drawable's position and size */
*num_flipped = 0;
for (i = 0; i < xf86_config->num_crtc; i++) {
- crtc = xf86_config->crtc[i];
- drmmode_crtc = crtc->driver_private;
+ xf86CrtcPtr crtc = xf86_config->crtc[i];
+ uint32_t crtc_id = drmmode_crtc_id(crtc);
+
if (!crtc->enabled)
continue;

@@ -1738,11 +1764,12 @@
crtc->mode.VDisplay != draw->height)
continue;

- ret = drmModePageFlip(pOMAP->drmFD,
- drmmode_crtc->mode_crtc->crtc_id, fb_id, flags,
+ DEBUG_MSG("[CRTC:%u] [FB:%u]", crtc_id, fb_id);
+ ret = drmModePageFlip(pOMAP->drmFD, crtc_id, fb_id, flags,
priv);
if (ret) {
- ERROR_MSG("flip queue failed: %s crtc:%d", strerror(errno), i);
+ ERROR_MSG("[CRTC:%u] [FB:%u] page flip failed: %s",
+ crtc_id, fb_id, strerror(errno));
return ret;
}
(*num_flipped)++;

--
To view, visit https://chromium-review.googlesource.com/190692
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iefabc7374fec3b50bdcfed0ecad1f2b6a8863265
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/xf86-video-armsoc
Gerrit-Branch: master
Gerrit-Owner: Daniel Kurtz <djk...@chromium.org>
Gerrit-Reviewer: Daniel Kurtz <djk...@chromium.org>
Gerrit-Reviewer: Dominik Behr <db...@chromium.org>
Gerrit-Reviewer: John Sheu <sh...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>

Gerrit-Reviewer: chrome-internal-fetch <chrome-int...@google.com>