From 7beee68f8d8617557990a5f860d9d1f99c4ba009 Mon Sep 17 00:00:00 2001 From: Anton Kindestam Date: Sat, 2 Jun 2018 12:53:54 +0200 Subject: context_drm_egl: Fix CRTC setup and release code when using atomic The previous code did not save enough information about the old state, and could end up changing what plane the fbcon:s FB got attached to, or in worse case causing a blank screen (observed in some multi-screen setups on Sandy Bridge). In addition refactor the handling of drmModeModeInfo property blobs to not leak, as well as enable reuse of already created blobs. --- video/out/drm_atomic.c | 144 +++++++++++++++++++++++++++++++++++++ video/out/drm_atomic.h | 42 +++++++++++ video/out/drm_common.c | 8 +-- video/out/drm_common.h | 2 +- video/out/opengl/context_drm_egl.c | 56 ++++++--------- video/out/vo_drm.c | 6 +- 6 files changed, 217 insertions(+), 41 deletions(-) diff --git a/video/out/drm_atomic.c b/video/out/drm_atomic.c index 8e68cdb074..5c6b3bbe21 100644 --- a/video/out/drm_atomic.c +++ b/video/out/drm_atomic.c @@ -290,9 +290,153 @@ fail: void drm_atomic_destroy_context(struct drm_atomic_context *ctx) { + drm_mode_destroy_blob(ctx->fd, &ctx->old_state.crtc.mode); drm_object_free(ctx->crtc); drm_object_free(ctx->connector); drm_object_free(ctx->osd_plane); drm_object_free(ctx->video_plane); talloc_free(ctx); } + +static bool drm_atomic_save_plane_state(struct drm_object *plane, + struct drm_atomic_plane_state *plane_state) +{ + bool ret = true; + + if (0 > drm_object_get_property(plane, "FB_ID", &plane_state->fb_id)) + ret = false; + if (0 > drm_object_get_property(plane, "CRTC_ID", &plane_state->crtc_id)) + ret = false; + if (0 > drm_object_get_property(plane, "SRC_X", &plane_state->src_x)) + ret = false; + if (0 > drm_object_get_property(plane, "SRC_Y", &plane_state->src_y)) + ret = false; + if (0 > drm_object_get_property(plane, "SRC_W", &plane_state->src_w)) + ret = false; + if (0 > drm_object_get_property(plane, "SRC_H", &plane_state->src_h)) + ret = false; + if (0 > drm_object_get_property(plane, "CRTC_X", &plane_state->crtc_x)) + ret = false; + if (0 > drm_object_get_property(plane, "CRTC_Y", &plane_state->crtc_y)) + ret = false; + if (0 > drm_object_get_property(plane, "CRTC_W", &plane_state->crtc_w)) + ret = false; + if (0 > drm_object_get_property(plane, "CRTC_H", &plane_state->crtc_h)) + ret = false; + // ZPOS might not exist, so ignore whether or not this succeeds + drm_object_get_property(plane, "ZPOS", &plane_state->zpos); + + return ret; +} + +static bool drm_atomic_restore_plane_state(drmModeAtomicReq *request, + struct drm_object *plane, + const struct drm_atomic_plane_state *plane_state) +{ + bool ret = true; + + if (0 > drm_object_set_property(request, plane, "FB_ID", plane_state->fb_id)) + ret = false; + if (0 > drm_object_set_property(request, plane, "CRTC_ID", plane_state->crtc_id)) + ret = false; + if (0 > drm_object_set_property(request, plane, "SRC_X", plane_state->src_x)) + ret = false; + if (0 > drm_object_set_property(request, plane, "SRC_Y", plane_state->src_y)) + ret = false; + if (0 > drm_object_set_property(request, plane, "SRC_W", plane_state->src_w)) + ret = false; + if (0 > drm_object_set_property(request, plane, "SRC_H", plane_state->src_h)) + ret = false; + if (0 > drm_object_set_property(request, plane, "CRTC_X", plane_state->crtc_x)) + ret = false; + if (0 > drm_object_set_property(request, plane, "CRTC_Y", plane_state->crtc_y)) + ret = false; + if (0 > drm_object_set_property(request, plane, "CRTC_W", plane_state->crtc_w)) + ret = false; + if (0 > drm_object_set_property(request, plane, "CRTC_H", plane_state->crtc_h)) + ret = false; + // ZPOS might not exist, so ignore whether or not this succeeds + drm_object_set_property(request, plane, "ZPOS", plane_state->zpos); + + return ret; +} + +bool drm_atomic_save_old_state(struct drm_atomic_context *ctx) +{ + if (ctx->old_state.saved) + return false; + + bool ret = true; + + drmModeCrtc *crtc = drmModeGetCrtc(ctx->fd, ctx->crtc->id); + if (crtc == NULL) + return false; + ctx->old_state.crtc.mode.mode = crtc->mode; + drmModeFreeCrtc(crtc); + + if (0 > drm_object_get_property(ctx->crtc, "ACTIVE", &ctx->old_state.crtc.active)) + ret = false; + + if (0 > drm_object_get_property(ctx->connector, "CRTC_ID", &ctx->old_state.connector.crtc_id)) + ret = false; + + if (!drm_atomic_save_plane_state(ctx->osd_plane, &ctx->old_state.osd_plane)) + ret = false; + if (!drm_atomic_save_plane_state(ctx->video_plane, &ctx->old_state.video_plane)) + ret = false; + + ctx->old_state.saved = true; + + return ret; +} + +bool drm_atomic_restore_old_state(drmModeAtomicReqPtr request, struct drm_atomic_context *ctx) +{ + if (!ctx->old_state.saved) + return false; + + bool ret = true; + + if (0 > drm_object_set_property(request, ctx->connector, "CRTC_ID", ctx->old_state.connector.crtc_id)) + ret = false; + + if (!drm_mode_ensure_blob(ctx->fd, &ctx->old_state.crtc.mode)) + ret = false; + if (0 > drm_object_set_property(request, ctx->crtc, "MODE_ID", ctx->old_state.crtc.mode.blob_id)) + ret = false; + if (0 > drm_object_set_property(request, ctx->crtc, "ACTIVE", ctx->old_state.crtc.active)) + ret = false; + + if (!drm_atomic_restore_plane_state(request, ctx->osd_plane, &ctx->old_state.osd_plane)) + ret = false; + if (!drm_atomic_restore_plane_state(request, ctx->video_plane, &ctx->old_state.video_plane)) + ret = false; + + ctx->old_state.saved = false; + + return ret; +} + +bool drm_mode_ensure_blob(int fd, struct drm_mode *mode) +{ + int ret = 0; + + if (!mode->blob_id) { + ret = drmModeCreatePropertyBlob(fd, &mode->mode, sizeof(drmModeModeInfo), + &mode->blob_id); + } + + return (ret == 0); +} + +bool drm_mode_destroy_blob(int fd, struct drm_mode *mode) +{ + int ret = 0; + + if (mode->blob_id) { + ret = drmModeDestroyPropertyBlob(fd, mode->blob_id); + mode->blob_id = 0; + } + + return (ret == 0); +} diff --git a/video/out/drm_atomic.h b/video/out/drm_atomic.h index 01246baf36..cd0252a767 100644 --- a/video/out/drm_atomic.h +++ b/video/out/drm_atomic.h @@ -19,11 +19,45 @@ #define MP_DRMATOMIC_H #include +#include #include #include #include "common/msg.h" +struct drm_mode { + drmModeModeInfo mode; + uint32_t blob_id; +}; + +struct drm_atomic_plane_state { + uint64_t fb_id; + uint64_t crtc_id; + uint64_t src_x; + uint64_t src_y; + uint64_t src_w; + uint64_t src_h; + uint64_t crtc_x; + uint64_t crtc_y; + uint64_t crtc_w; + uint64_t crtc_h; + uint64_t zpos; +}; + +// Used to store the restore state for VT switching and uninit +struct drm_atomic_state { + bool saved; + struct { + uint64_t crtc_id; + } connector; + struct { + struct drm_mode mode; + uint64_t active; + } crtc; + struct drm_atomic_plane_state osd_plane; + struct drm_atomic_plane_state video_plane; +}; + struct drm_object { int fd; uint32_t id; @@ -41,6 +75,8 @@ struct drm_atomic_context { struct drm_object *video_plane; drmModeAtomicReq *request; + + struct drm_atomic_state old_state; }; @@ -56,4 +92,10 @@ struct drm_atomic_context *drm_atomic_create_context(struct mp_log *log, int fd, int osd_plane_id, int video_plane_id); void drm_atomic_destroy_context(struct drm_atomic_context *ctx); +bool drm_atomic_save_old_state(struct drm_atomic_context *ctx); +bool drm_atomic_restore_old_state(drmModeAtomicReq *request, struct drm_atomic_context *ctx); + +bool drm_mode_ensure_blob(int fd, struct drm_mode *mode); +bool drm_mode_destroy_blob(int fd, struct drm_mode *mode); + #endif // MP_DRMATOMIC_H diff --git a/video/out/drm_common.c b/video/out/drm_common.c index df06744fbe..aa3d0998c4 100644 --- a/video/out/drm_common.c +++ b/video/out/drm_common.c @@ -237,7 +237,7 @@ static bool setup_mode(struct kms *kms, int mode_id) return false; } - kms->mode = kms->connector->modes[mode_id]; + kms->mode.mode = kms->connector->modes[mode_id]; return true; } @@ -281,7 +281,7 @@ struct kms *kms_create(struct mp_log *log, const char *connector_spec, .fd = open_card(card_no), .connector = NULL, .encoder = NULL, - .mode = { 0 }, + .mode = {{0}}, .crtc_id = -1, .card_no = card_no, }; @@ -324,7 +324,6 @@ struct kms *kms_create(struct mp_log *log, const char *connector_spec, } } - drmModeFreeResources(res); return kms; @@ -341,6 +340,7 @@ void kms_destroy(struct kms *kms) { if (!kms) return; + drm_mode_destroy_blob(kms->fd, &kms->mode); if (kms->connector) { drmModeFreeConnector(kms->connector); kms->connector = NULL; @@ -425,7 +425,7 @@ void kms_show_available_cards_and_connectors(struct mp_log *log) double kms_get_display_fps(const struct kms *kms) { - return mode_get_Hz(&kms->mode); + return mode_get_Hz(&kms->mode.mode); } int drm_validate_connector_opt(struct mp_log *log, const struct m_option *opt, diff --git a/video/out/drm_common.h b/video/out/drm_common.h index 4d1e2a9e3f..3f144102ec 100644 --- a/video/out/drm_common.h +++ b/video/out/drm_common.h @@ -32,7 +32,7 @@ struct kms { int fd; drmModeConnector *connector; drmModeEncoder *encoder; - drmModeModeInfo mode; + struct drm_mode mode; uint32_t crtc_id; int card_no; struct drm_atomic_context *atomic_context; diff --git a/video/out/opengl/context_drm_egl.c b/video/out/opengl/context_drm_egl.c index c101a5b809..52774d8170 100644 --- a/video/out/opengl/context_drm_egl.c +++ b/video/out/opengl/context_drm_egl.c @@ -252,6 +252,10 @@ static bool crtc_setup_atomic(struct ra_ctx *ctx) struct priv *p = ctx->priv; struct drm_atomic_context *atomic_ctx = p->kms->atomic_context; + if (!drm_atomic_save_old_state(atomic_ctx)) { + MP_WARN(ctx->vo, "Failed to save old DRM atomic state\n"); + } + drmModeAtomicReqPtr request = drmModeAtomicAlloc(); if (!request) { MP_ERR(ctx->vo, "Failed to allocate drm atomic request\n"); @@ -263,14 +267,11 @@ static bool crtc_setup_atomic(struct ra_ctx *ctx) return false; } - uint32_t blob_id = 0; - if (drmModeCreatePropertyBlob(p->kms->fd, &p->kms->mode, sizeof(drmModeModeInfo), - &blob_id) != 0) { + if (!drm_mode_ensure_blob(p->kms->fd, &p->kms->mode)) { MP_ERR(ctx->vo, "Failed to create DRM mode blob\n"); - blob_id = 0; goto err; } - if (drm_object_set_property(request, atomic_ctx->crtc, "MODE_ID", blob_id) < 0) { + if (drm_object_set_property(request, atomic_ctx->crtc, "MODE_ID", p->kms->mode.blob_id) < 0) { MP_ERR(ctx->vo, "Could not set MODE_ID on crtc\n"); goto err; } @@ -287,8 +288,8 @@ static bool crtc_setup_atomic(struct ra_ctx *ctx) drm_object_set_property(request, atomic_ctx->osd_plane, "SRC_H", p->osd_size.height << 16); drm_object_set_property(request, atomic_ctx->osd_plane, "CRTC_X", 0); drm_object_set_property(request, atomic_ctx->osd_plane, "CRTC_Y", 0); - drm_object_set_property(request, atomic_ctx->osd_plane, "CRTC_W", p->kms->mode.hdisplay); - drm_object_set_property(request, atomic_ctx->osd_plane, "CRTC_H", p->kms->mode.vdisplay); + drm_object_set_property(request, atomic_ctx->osd_plane, "CRTC_W", p->kms->mode.mode.hdisplay); + drm_object_set_property(request, atomic_ctx->osd_plane, "CRTC_H", p->kms->mode.mode.vdisplay); int ret = drmModeAtomicCommit(p->kms->fd, request, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); if (ret) @@ -298,8 +299,6 @@ static bool crtc_setup_atomic(struct ra_ctx *ctx) return ret == 0; err: - if (blob_id) - drmModeDestroyPropertyBlob(p->kms->fd, blob_id); drmModeAtomicFree(request); return false; } @@ -314,17 +313,10 @@ static bool crtc_release_atomic(struct ra_ctx *ctx) MP_ERR(ctx->vo, "Failed to allocate drm atomic request\n"); return false; } - drm_object_set_property(request, atomic_ctx->connector, "CRTC_ID", p->old_crtc->crtc_id); - uint32_t blob_id; - if (drmModeCreatePropertyBlob(p->kms->fd, &p->old_crtc->mode, sizeof(drmModeModeInfo), - &blob_id) != 0) { - MP_ERR(ctx->vo, "Failed to create DRM mode blob\n"); - goto err; + if (!drm_atomic_restore_old_state(request, atomic_ctx)) { + MP_WARN(ctx->vo, "Got error while restoring old state\n"); } - drm_object_set_property(request, atomic_ctx->crtc, "MODE_ID", blob_id); - drm_object_set_property(request, atomic_ctx->crtc, "ACTIVE", 1); - drm_object_set_property(request, atomic_ctx->osd_plane, "FB_ID", p->old_crtc->buffer_id); int ret = drmModeAtomicCommit(p->kms->fd, request, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); @@ -333,10 +325,6 @@ static bool crtc_release_atomic(struct ra_ctx *ctx) drmModeAtomicFree(request); return ret == 0; - - err: - drmModeAtomicFree(request); - return false; } static bool crtc_setup(struct ra_ctx *ctx) @@ -344,16 +332,16 @@ static bool crtc_setup(struct ra_ctx *ctx) struct priv *p = ctx->priv; if (p->active) return true; - p->old_crtc = drmModeGetCrtc(p->kms->fd, p->kms->crtc_id); if (p->kms->atomic_context) { int ret = crtc_setup_atomic(ctx); p->active = true; return ret; } else { + p->old_crtc = drmModeGetCrtc(p->kms->fd, p->kms->crtc_id); int ret = drmModeSetCrtc(p->kms->fd, p->kms->crtc_id, p->fb->id, 0, 0, &p->kms->connector->connector_id, 1, - &p->kms->mode); + &p->kms->mode.mode); p->active = true; return ret == 0; } @@ -376,19 +364,21 @@ static void crtc_release(struct ra_ctx *ctx) } } - if (p->old_crtc) { - if (p->kms->atomic_context) { + if (p->kms->atomic_context) { + if (p->kms->atomic_context->old_state.saved) { if (!crtc_release_atomic(ctx)) MP_ERR(ctx->vo, "Failed to restore previous mode\n"); - } else { + } + } else { + if (p->old_crtc) { drmModeSetCrtc(p->kms->fd, p->old_crtc->crtc_id, p->old_crtc->buffer_id, p->old_crtc->x, p->old_crtc->y, &p->kms->connector->connector_id, 1, &p->old_crtc->mode); + drmModeFreeCrtc(p->old_crtc); + p->old_crtc = NULL; } - drmModeFreeCrtc(p->old_crtc); - p->old_crtc = NULL; } } @@ -606,13 +596,13 @@ static bool drm_egl_init(struct ra_ctx *ctx) p->osd_size.width = ctx->vo->opts->drm_opts->drm_osd_size.w; p->osd_size.height = ctx->vo->opts->drm_opts->drm_osd_size.h; } else { - p->osd_size.width = p->kms->mode.hdisplay; - p->osd_size.height = p->kms->mode.vdisplay; + p->osd_size.width = p->kms->mode.mode.hdisplay; + p->osd_size.height = p->kms->mode.mode.vdisplay; MP_WARN(ctx, "Setting OSD size is only available with DRM atomic, defaulting to screen resolution\n"); } } else { - p->osd_size.width = p->kms->mode.hdisplay; - p->osd_size.height = p->kms->mode.vdisplay; + p->osd_size.width = p->kms->mode.mode.hdisplay; + p->osd_size.height = p->kms->mode.mode.vdisplay; } uint32_t argb_format; diff --git a/video/out/vo_drm.c b/video/out/vo_drm.c index ed41d2aab0..7f5290110f 100644 --- a/video/out/vo_drm.c +++ b/video/out/vo_drm.c @@ -153,8 +153,8 @@ static bool fb_setup_double_buffering(struct vo *vo) p->front_buf = 0; for (unsigned int i = 0; i < 2; i++) { - p->bufs[i].width = p->kms->mode.hdisplay; - p->bufs[i].height = p->kms->mode.vdisplay; + p->bufs[i].width = p->kms->mode.mode.hdisplay; + p->bufs[i].height = p->kms->mode.mode.vdisplay; } for (unsigned int i = 0; i < BUF_COUNT; i++) { @@ -186,7 +186,7 @@ static bool crtc_setup(struct vo *vo) int ret = drmModeSetCrtc(p->kms->fd, p->kms->crtc_id, p->bufs[MOD(p->front_buf - 1, BUF_COUNT)].fb, 0, 0, &p->kms->connector->connector_id, 1, - &p->kms->mode); + &p->kms->mode.mode); p->active = true; return ret == 0; } -- cgit v1.2.3