From ab72b0d5c6f914dc931ae2555e7c0e427598e827 Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Thu, 9 Feb 2023 12:33:11 +0100 Subject: [PATCH] Stop re-using a previous configuration when FB attributes change If FB attributes change, we might be able to take advantage of a plane we couldn't use before. Closes: https://gitlab.freedesktop.org/emersion/libliftoff/-/issues/71 --- alloc.c | 50 ++++++++++++++++++++++++++++++++++++----- include/private.h | 4 ++++ layer.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 6 deletions(-) diff --git a/alloc.c b/alloc.c index fcc54ae..35bf315 100644 --- a/alloc.c +++ b/alloc.c @@ -517,6 +517,18 @@ apply_current(struct liftoff_device *device, drmModeAtomicReq *req) return 0; } +static bool +fb_info_needs_realloc(const drmModeFB2 *a, const drmModeFB2 *b) +{ + if (a->width != b->width || a->height != b->height || + a->pixel_format != b->pixel_format || a->modifier != b->modifier) + return true; + + /* TODO: consider checking pitch and offset? */ + + return false; +} + static bool layer_needs_realloc(struct liftoff_layer *layer) { @@ -529,21 +541,30 @@ layer_needs_realloc(struct liftoff_layer *layer) for (i = 0; i < layer->props_len; i++) { prop = &layer->props[i]; - if (prop->value == prop->prev_value) { - continue; - } /* If FB_ID changes from non-zero to zero, we don't need to * display this layer anymore, so we may be able to re-use its * plane for another layer. If FB_ID changes from zero to * non-zero, we might be able to find a plane for this layer. - * If FB_ID changes from non-zero to non-zero, we can try to - * re-use the previous allocation. */ + * If FB_ID changes from non-zero to non-zero and the FB + * attributes didn't change, we can try to re-use the previous + * allocation. */ if (strcmp(prop->name, "FB_ID") == 0) { if (prop->value == 0 || prop->prev_value == 0) { return true; } - /* TODO: check format/modifier is the same? */ + + if (fb_info_needs_realloc(&layer->fb_info, + &layer->prev_fb_info)) { + return true; + } + + continue; + } + + /* For all properties except FB_ID, we can skip realloc if the + * value didn't change. */ + if (prop->value == prop->prev_value) { continue; } @@ -639,6 +660,22 @@ update_layers_priority(struct liftoff_device *device) } } +static void +update_layers_fb_info(struct liftoff_output *output) +{ + struct liftoff_layer *layer; + + /* We don't know what the library user did in-between + * liftoff_output_apply() calls. They might've removed the FB and + * re-created a completely different one which happens to have the same + * FB ID. */ + liftoff_list_for_each(layer, &output->layers, link) { + memset(&layer->fb_info, 0, sizeof(layer->fb_info)); + layer_cache_fb_info(layer); + /* TODO: propagate error? */ + } +} + static void log_reuse(struct liftoff_output *output) { @@ -697,6 +734,7 @@ liftoff_output_apply(struct liftoff_output *output, drmModeAtomicReq *req, device = output->device; update_layers_priority(device); + update_layers_fb_info(output); ret = reuse_previous_alloc(output, req, flags); if (ret == 0) { diff --git a/include/private.h b/include/private.h index 4b237d0..9a7cd83 100644 --- a/include/private.h +++ b/include/private.h @@ -51,6 +51,7 @@ struct liftoff_layer { int current_priority, pending_priority; /* prop added or force_composition changed */ bool changed; + drmModeFB2 fb_info, prev_fb_info; /* cached FB info */ }; struct liftoff_layer_property { @@ -107,6 +108,9 @@ layer_has_fb(struct liftoff_layer *layer); bool layer_is_visible(struct liftoff_layer *layer); +int +layer_cache_fb_info(struct liftoff_layer *layer); + int plane_apply(struct liftoff_plane *plane, struct liftoff_layer *layer, drmModeAtomicReq *req); diff --git a/layer.c b/layer.c index 2e14b2a..34e1d14 100644 --- a/layer.c +++ b/layer.c @@ -1,6 +1,7 @@ #include #include #include +#include #include "private.h" struct liftoff_layer * @@ -178,6 +179,7 @@ layer_mark_clean(struct liftoff_layer *layer) size_t i; layer->changed = false; + layer->prev_fb_info = layer->fb_info; for (i = 0; i < layer->props_len; i++) { layer->props[i].prev_value = layer->props[i].value; @@ -239,3 +241,58 @@ layer_is_visible(struct liftoff_layer *layer) return layer_has_fb(layer); } } + +int +layer_cache_fb_info(struct liftoff_layer *layer) +{ + struct liftoff_layer_property *fb_id_prop; + drmModeFB2 *fb_info; + size_t i, j, num_planes; + int ret; + + fb_id_prop = layer_get_property(layer, "FB_ID"); + if (fb_id_prop == NULL || fb_id_prop->value == 0) { + memset(&layer->fb_info, 0, sizeof(layer->fb_info)); + return 0; + } + + if (layer->fb_info.fb_id == fb_id_prop->value) { + return 0; + } + + fb_info = drmModeGetFB2(layer->output->device->drm_fd, fb_id_prop->value); + if (fb_info == NULL) { + if (errno == EINVAL) { + return 0; /* old kernel */ + } + return -errno; + } + + /* drmModeGetFB2() always creates new GEM handles -- close these, we + * won't use them and we don't want to leak them */ + num_planes = sizeof(fb_info->handles) / sizeof(fb_info->handles[0]); + for (i = 0; i < num_planes; i++) { + if (fb_info->handles[i] == 0) { + continue; + } + + ret = drmCloseBufferHandle(layer->output->device->drm_fd, + fb_info->handles[i]); + if (ret != 0) { + liftoff_log_errno(LIFTOFF_ERROR, "drmCloseBufferHandle"); + continue; + } + + /* Make sure we don't double-close a handle */ + for (j = i + 1; j < num_planes; j++) { + if (fb_info->handles[j] == fb_info->handles[i]) { + fb_info->handles[j] = 0; + } + } + fb_info->handles[i] = 0; + } + + layer->fb_info = *fb_info; + drmModeFreeFB2(fb_info); + return 0; +}