Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config/output: support DRM_FORMAT_ARGB8888 #8344

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

sdirkwinkel
Copy link
Contributor

Some display output hardware [1] doesn't support any of the current formats, but works with ARGB8888. Fall back to it if available.

[1] https://github.com/torvalds/linux/blob/196145c606d0f816fd3926483cb1ff87e09c2c0b/drivers/gpu/drm/xlnx/zynqmp_disp.c#L313

Some display output hardware [1] doesn't support any of the current
formats, but works with ARGB8888. Fall back to it if available.

[1] https://github.com/torvalds/linux/blob/196145c606d0f816fd3926483cb1ff87e09c2c0b/drivers/gpu/drm/xlnx/zynqmp_disp.c#L313

Signed-off-by: Steffen Dirkwinkel <[email protected]>
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, sounds good to me!

@kennylevinsen
Copy link
Member

I would kind of have expected a display device that supports ARGB to also be able to disable blending and do XRGB as well, but oh well. Having "extra" render formats on the list just means that when the formats are supported but lighting up the monitor is not possible, we end up wasting a bit more time allocating buffers for bogus tests. Buffer-less commit tests would be fantastic...

LGTM!

@kennylevinsen kennylevinsen merged commit f957c7e into swaywm:master Sep 13, 2024
3 checks passed
@sdirkwinkel
Copy link
Contributor Author

@kennylevinsen @emersion
Sorry, maybe I should have written this in the original description:

I also got it working by setting the default here (maybe it could be set to a format the hardware actually supports?):
https://salsa.debian.org/swaywm-team/wlroots/-/blob/debian/bookworm/types/output/output.c?ref_type=heads#L388
I tried something like this in wlroots first (with older sway, haven't tried yet with current sway):

diff --git a/types/output/render.c b/types/output/render.c
index 985b93a9..cc547fcf 100644
--- a/types/output/render.c
+++ b/types/output/render.c
@@ -17,6 +17,12 @@ bool wlr_output_init_render(struct wlr_output
*output,
        assert(output->allocator == NULL && allocator != NULL);
        assert(output->renderer == NULL && renderer != NULL);
 
+       const struct wlr_drm_format_set *display_formats =
wlr_output_get_primary_formats(output, allocator->buffer_caps);
+
+       if(display_formats->formats[0] != NULL) {
+               output->render_format = display_formats->formats[0]-
>format;
+       }
        uint32_t backend_caps = backend_get_buffer_caps(output-
>backend);
        uint32_t renderer_caps =
renderer_get_render_buffer_caps(renderer);

I didn't end up submitting that, because sway already iterates over format.
Could wlr_output_get_primary_formats be used in sway to avoid unnecessary tries?

@kennylevinsen
Copy link
Member

kennylevinsen commented Sep 13, 2024

Could wlr_output_get_primary_formats be used in sway to avoid unnecessary tries?

We already use wlr_output_get_primary_formats to skip unsupported formats:

sway/sway/config/output.c

Lines 795 to 805 in f957c7e

const struct wlr_drm_format_set *primary_formats =
wlr_output_get_primary_formats(wlr_output, WLR_BUFFER_CAP_DMABUF);
bool need_10bit = cfg->config && cfg->config->render_bit_depth == RENDER_BIT_DEPTH_10;
for (size_t idx = 0; fmts[idx] != DRM_FORMAT_INVALID; idx++) {
if (!need_10bit && render_format_is_10bit(fmts[idx])) {
continue;
}
if (!wlr_drm_format_set_get(primary_formats, fmts[idx])) {
// This is not a supported format for this output
continue;
}

The "problem" is that many GPUs support all the formats on the list, so if an output is failing to light up for some non-format reason (e.g., bandwidth or CRTC limit), we end up allocating and testing all those buffer types to no avail. This was already the case before this patch, with the 10-bit BGR test being quite bogus on most GPUs.

IMO, not something we need to worry about yet. If this ends up being enough of an issue (e.g., if the list grows longer or people experience noticeable delays), maybe we could add a heuristic to only try alternate format variations if the primary isn't supported to reduce the number of tested formats (e.g., BGR only if RGB isn't supported, ARGB only if XRGB isn't supported).

The "proper" fix is kernel support for doing a commit test for a buffer format without an actual buffer allocation, in which case we could just cycle through and test all supported formats without worry.

@kennylevinsen
Copy link
Member

Made a draft of the heuristic approach here for the heck of it: #8345

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants