Add support for HDR10 #8617
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| #include <strings.h> | ||
| #include "sway/commands.h" | ||
| #include "sway/config.h" | ||
| #include "sway/output.h" | ||
| #include "util.h" | ||
|
|
||
| struct cmd_results *output_cmd_hdr(int argc, char **argv) { | ||
| if (!config->handler_context.output_config) { | ||
| return cmd_results_new(CMD_FAILURE, "Missing output config"); | ||
| } | ||
| if (argc == 0) { | ||
| return cmd_results_new(CMD_INVALID, "Missing hdr argument"); | ||
| } | ||
|
|
||
| bool current = false; | ||
| if (strcasecmp(argv[0], "toggle") == 0) { | ||
| const char *oc_name = config->handler_context.output_config->name; | ||
| if (strcmp(oc_name, "*") == 0) { | ||
| return cmd_results_new(CMD_INVALID, | ||
| "Cannot apply toggle to all outputs"); | ||
| } | ||
|
|
||
| struct sway_output *output = all_output_by_name_or_id(oc_name); | ||
| if (!output) { | ||
| return cmd_results_new(CMD_FAILURE, | ||
| "Cannot apply toggle to unknown output %s", oc_name); | ||
| } | ||
|
|
||
| current = output->hdr; | ||
| } | ||
|
|
||
| config->handler_context.output_config->hdr = parse_boolean(argv[0], current); | ||
|
|
||
| config->handler_context.leftovers.argc = argc - 1; | ||
| config->handler_context.leftovers.argv = argv + 1; | ||
| return NULL; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,7 @@ struct output_config *new_output_config(const char *name) { | |
| oc->color_transform = NULL; | ||
| oc->power = -1; | ||
| oc->allow_tearing = -1; | ||
| oc->hdr = -1; | ||
| return oc; | ||
| } | ||
|
|
||
|
|
@@ -154,6 +155,9 @@ static void supersede_output_config(struct output_config *dst, struct output_con | |
| if (src->allow_tearing != -1) { | ||
| dst->allow_tearing = -1; | ||
| } | ||
| if (src->hdr != -1) { | ||
| dst->hdr = -1; | ||
| } | ||
| } | ||
|
|
||
| // merge_output_config sets all fields in dst that were set in src | ||
|
|
@@ -229,6 +233,9 @@ static void merge_output_config(struct output_config *dst, struct output_config | |
| if (src->allow_tearing != -1) { | ||
| dst->allow_tearing = src->allow_tearing; | ||
| } | ||
| if (src->hdr != -1) { | ||
| dst->hdr = src->hdr; | ||
| } | ||
| } | ||
|
|
||
| void store_output_config(struct output_config *oc) { | ||
|
|
@@ -271,11 +278,11 @@ void store_output_config(struct output_config *oc) { | |
|
|
||
| sway_log(SWAY_DEBUG, "Config stored for output %s (enabled: %d) (%dx%d@%fHz " | ||
| "position %d,%d scale %f subpixel %s transform %d) (bg %s %s) (power %d) " | ||
| "(max render time: %d) (allow tearing: %d) (hdr: %d)", | ||
| oc->name, oc->enabled, oc->width, oc->height, oc->refresh_rate, | ||
| oc->x, oc->y, oc->scale, sway_wl_output_subpixel_to_string(oc->subpixel), | ||
| oc->transform, oc->background, oc->background_option, oc->power, | ||
| oc->max_render_time, oc->allow_tearing, oc->hdr); | ||
|
|
||
| // If the configuration was not merged into an existing configuration, add | ||
| // it to the list. Otherwise we're done with it and can free it. | ||
|
|
@@ -341,6 +348,45 @@ static void set_modeline(struct wlr_output *output, | |
| #endif | ||
| } | ||
|
|
||
| bool output_supports_hdr(struct wlr_output *output, const char **unsupported_reason_ptr) { | ||
| const char *unsupported_reason = NULL; | ||
| if (!(output->supported_primaries & WLR_COLOR_NAMED_PRIMARIES_BT2020)) { | ||
| unsupported_reason = "BT2020 primaries not supported by output"; | ||
| } else if (!(output->supported_transfer_functions & WLR_COLOR_TRANSFER_FUNCTION_ST2084_PQ)) { | ||
| unsupported_reason = "PQ transfer function not supported by output"; | ||
| } else if (!server.renderer->features.output_color_transform) { | ||
| unsupported_reason = "renderer doesn't support output color transforms"; | ||
| } | ||
| if (unsupported_reason_ptr != NULL) { | ||
| *unsupported_reason_ptr = unsupported_reason; | ||
| } | ||
| return unsupported_reason == NULL; | ||
| } | ||
|
|
||
| static void set_hdr(struct wlr_output *output, struct wlr_output_state *pending, bool enabled) { | ||
| const char *unsupported_reason = NULL; | ||
| if (!output_supports_hdr(output, &unsupported_reason)) { | ||
| sway_log(SWAY_ERROR, "Cannot enable HDR on output %s: %s", | ||
| output->name, unsupported_reason); | ||
| enabled = false; | ||
| } | ||
|
|
||
| if (!enabled) { | ||
| if (output->supported_primaries != 0 || output->supported_transfer_functions != 0) { | ||
| sway_log(SWAY_DEBUG, "Disabling HDR on output %s", output->name); | ||
| wlr_output_state_set_image_description(pending, NULL); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| sway_log(SWAY_DEBUG, "Enabling HDR on output %s", output->name); | ||
| const struct wlr_output_image_description image_desc = { | ||
| .primaries = WLR_COLOR_NAMED_PRIMARIES_BT2020, | ||
| .transfer_function = WLR_COLOR_TRANSFER_FUNCTION_ST2084_PQ, | ||
| }; | ||
| wlr_output_state_set_image_description(pending, &image_desc); | ||
| } | ||
|
|
||
| /* Some manufacturers hardcode the aspect-ratio of the output in the physical | ||
| * size field. */ | ||
| static bool phys_size_is_aspect_ratio(struct wlr_output *output) { | ||
|
|
@@ -415,6 +461,16 @@ static enum render_bit_depth bit_depth_from_format(uint32_t render_format) { | |
| return RENDER_BIT_DEPTH_DEFAULT; | ||
| } | ||
|
|
||
| static enum render_bit_depth get_config_render_bit_depth(const struct output_config *oc) { | ||
| if (oc && oc->render_bit_depth != RENDER_BIT_DEPTH_DEFAULT) { | ||
| return oc->render_bit_depth; | ||
| } | ||
| if (oc && oc->hdr == 1) { | ||
| return RENDER_BIT_DEPTH_10; | ||
| } | ||
| return RENDER_BIT_DEPTH_8; | ||
| } | ||
|
|
||
| static bool render_format_is_bgr(uint32_t fmt) { | ||
| return fmt == DRM_FORMAT_XBGR2101010 || fmt == DRM_FORMAT_XBGR8888; | ||
| } | ||
|
|
@@ -485,24 +541,29 @@ static void queue_output_config(struct output_config *oc, | |
| } | ||
| } | ||
|
|
||
| enum render_bit_depth render_bit_depth = get_config_render_bit_depth(oc); | ||
| if (render_bit_depth == RENDER_BIT_DEPTH_10 && | ||
| bit_depth_from_format(output->wlr_output->render_format) == render_bit_depth) { | ||
| // 10-bit was set successfully before, try to save some tests by reusing the format | ||
| wlr_output_state_set_render_format(pending, output->wlr_output->render_format); | ||
| } else if (render_bit_depth == RENDER_BIT_DEPTH_10) { | ||
| wlr_output_state_set_render_format(pending, DRM_FORMAT_XRGB2101010); | ||
| } else if (render_bit_depth == RENDER_BIT_DEPTH_6) { | ||
| wlr_output_state_set_render_format(pending, DRM_FORMAT_RGB565); | ||
| } else { | ||
| wlr_output_state_set_render_format(pending, DRM_FORMAT_XRGB8888); | ||
| } | ||
|
|
||
| bool hdr = oc && oc->hdr == 1; | ||
| if (hdr && oc->color_transform != NULL) { | ||
| sway_log(SWAY_ERROR, "Cannot HDR on output %s: output has an ICC profile set", wlr_output->name); | ||
|
There was a problem hiding this comment. I don't think we should log from here, Maybe we could log something about output capabilities (HDR and VRR for example) when we create the output. There was a problem hiding this comment. We already log for other errors, e.g. mode not found, invalid modeline etc. I think it's important to log something when the user tries to enable HDR but we can't. Logging on startup isn't enough IMHO. The current scheme only logs when the output configuration changes, which isn't too spammy and easy enough for users to fix if they want to (by only enabling HDR if the output supports it). There was a problem hiding this comment.
Multiple times on every change. I really don't like log noise. :/ I have slowly been chipping away at it (e.g., a630272), removing most bogus logging and collecting meaningful stuff in places like The HDR case is logically similar to the adaptive sync case where the setting is best effort and can be disabled by display, link or connector restrictions, in which case not enabling is not considered an error. I do think in that case that we need to clarify what the output capabilities are in the log, as we right now only communicate what we end up doing (with debug log level). There was a problem hiding this comment. I noticed the adaptive sync thing and was about to write a patch to add a log there. I find it pretty confusing that a command like There was a problem hiding this comment. This was the kind of INFO-level logging I was thinking of: #8784 I feel like that's clearer than error logging. We could consider also logging the final output state as INFO though, right now that's only debug. There was a problem hiding this comment. I don't think that kind of logging is very helpful to users TBH. There was a problem hiding this comment. I think it's far more helpful than sporadic error messages. Errors is something people try to fix as if something is wrong that has to be corrected, and there's nothing to tell if something isn't meant to work. It's much easier to know if something is supported and to troubleshoot if the first step is: Does it say "yes"? Both for users, those providing support and those writing various FAQs/wikis for self-help. But I won't hold up the feature for that further. There was a problem hiding this comment. If the config tries to enable HDR10 and HDR10 cannot be enabled, I think it's only fair to print an error. Anyways, it doesn't seem like we're going to come to an agreement here. |
||
| hdr = false; | ||
| } | ||
| set_hdr(wlr_output, pending, hdr); | ||
| } | ||
|
|
||
| static bool finalize_output_config(struct output_config *oc, struct sway_output *output, | ||
| const struct wlr_output_state *applied) { | ||
| if (output == root->fallback_output) { | ||
| return false; | ||
| } | ||
|
|
@@ -561,6 +622,7 @@ static bool finalize_output_config(struct output_config *oc, struct sway_output | |
|
|
||
| output->max_render_time = oc && oc->max_render_time > 0 ? oc->max_render_time : 0; | ||
| output->allow_tearing = oc && oc->allow_tearing > 0; | ||
| output->hdr = applied->image_description != NULL; | ||
|
|
||
| return true; | ||
| } | ||
|
|
@@ -785,10 +847,7 @@ static bool search_render_format(struct search_context *ctx, size_t output_idx) | |
|
|
||
| const struct wlr_drm_format_set *primary_formats = | ||
| wlr_output_get_primary_formats(wlr_output, server.allocator->buffer_caps); | ||
| enum render_bit_depth needed_bits = get_config_render_bit_depth(cfg->config); | ||
| for (size_t idx = 0; fmts[idx] != DRM_FORMAT_INVALID; idx++) { | ||
| enum render_bit_depth format_bits = bit_depth_from_format(fmts[idx]); | ||
| if (needed_bits < format_bits) { | ||
|
|
@@ -943,9 +1002,10 @@ static bool apply_resolved_output_configs(struct matched_output_config *configs, | |
|
|
||
| for (size_t idx = 0; idx < configs_len; idx++) { | ||
| struct matched_output_config *cfg = &configs[idx]; | ||
| struct wlr_backend_output_state *backend_state = &states[idx]; | ||
| sway_log(SWAY_DEBUG, "Finalizing config for %s", | ||
| cfg->output->wlr_output->name); | ||
| finalize_output_config(cfg->config, cfg->output, &backend_state->base); | ||
| } | ||
|
|
||
| // Output layout being applied in finalize_output_config can shift outputs | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defaults to 10-bit even if
set_hdrwould bail out on requesting HDR. Maybe we should flip things, setting HDR and changing the default based on whether HDR was set or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that sounds complicated to implement because
get_config_render_bit_depth()is called from elsewhere too, e.g.search_render_format(). Also I think it's better to keep things simple to make the render bit depth default only depend on the config alone rather than the renderer/output capabilities.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does feel a little bit surprising that
output * hdr on(where someone just wants to use HDR whenever available) would lead to all non-HDR displays having 10-bit render depth, especially as you cannot easily revert it (output * render_bit_depth 8would also impact HDR-capable displays), and as 10-bit on Vulkan would activate the blending buffer which currently triples VRAM use and degrades performance a bit.On the other hand I doubt anyone needs
output * hdr on, at least right now where HDR is very much hit and miss. We can probably just ignore this for now.Both uses would be able to take
wlr_output_stateas argument if one wanted to check if hdr is enabled to select default render bit depth.search_render_formatis called afterqueue_output_confighas filled out the state, and HDR is not currently part of the output config search so it stays set until we give up.