GNOME Bugzilla – Bug 785779
KMS: Support tiled/compressed buffers
Last modified: 2018-01-24 03:46:36 UTC
Using zwp_linux_dmabuf_v1 lets us take in client buffers allocated with modifiers, e.g. tiled/compressed. However, the scanout surface allocated for KMS is still unmodified. This patchset takes advantage of the IN_FORMATS modifier-advertisement available in excruciatingly recent kernels (will be released in 4.14), to find out which modifiers are supported for scanout. We then pass this to a new GBM surface-creation API (take list of supported modifiers and pick one), and use new multi-plane/modifier-aware GBM API to extract the information from the BO and feed it back into the kernel. This lets us have compressed scanout surfaces, for memory bandwidth victory. \o/ The first patch is a bit hokey, with the local struct definition for the blobs. We could either keep that as a local definition, or wait until ~next week when I can update & release a new libdrm which will include that in drm_mode.h. Hopefully the rest is OK, though the open-coded g_array_intersect() in the last patch makes me extremely unhappy. I've also tried to not botch the coding style this time. Let's see how well that's worked out.
Created attachment 356866 [details] [review] monitor-manager-kms: Add parsing for IN_FORMATS property The KMS IN_FORMATS blob property contains a structure defining which format/modifier combinations are supported for each plane. Use this to extract a list of acceptable modifiers to use for the primary plane for XRGB8888, so we can ask EGL to allocate tiled/compressed buffers for scanout when available.
Created attachment 356867 [details] [review] renderer-native: Use drmModeAddFB2 where available drmModeAddFB2 allows specifying multiple planes, as well as directly specifying the format, rather than relying on a depth/bpp -> format mapping.
Created attachment 356868 [details] [review] renderer-native: Use modifier-aware GBM API Newer versions of GBM support buffer modifiers, including multi-plane buffers. Use this new API to explicitly pull the information from GBM, and feed it to drmModeAddFB2WithModifiers.
Created attachment 356869 [details] [review] renderer-native: Create GBM surfaces with modifiers Now that we have the list of supported modifiers from the monitor manager (via the CRTCs to the primary planes), we can use this to inform EGL it can use those modifiers to allocate the GBM surface with. Doing so allows us to use tiling and compression for our scanout surfaces.
Given the recent merge of multi-gpu support, the patches needed some work to be rebased on top of master. Any testing with multi-gpu hardware would be welcome.
Created attachment 363121 [details] [review] monitor-manager-kms: Add parsing for IN_FORMATS property The KMS IN_FORMATS blob property contains a structure defining which format/modifier combinations are supported for each plane. Use this to extract a list of acceptable modifiers to use for the primary plane for XRGB8888, so we can ask EGL to allocate tiled/compressed buffers for scanout when available.
Created attachment 363122 [details] [review] renderer-native: Use drmModeAddFB2 where available drmModeAddFB2 allows specifying multiple planes, as well as directly specifying the format, rather than relying on a depth/bpp -> format mapping.
Created attachment 363123 [details] [review] renderer-native: Use modifier-aware GBM API Newer versions of GBM support buffer modifiers, including multi-plane buffers. Use this new API to explicitly pull the information from GBM, and feed it to drmModeAddFB2WithModifiers.
Created attachment 363124 [details] [review] renderer-native: Create GBM surfaces with modifiers Now that we have the list of supported modifiers from the monitor manager (via the CRTCs to the primary planes), we can use this to inform EGL it can use those modifiers to allocate the GBM surface with. Doing so allows us to use tiling and compression for our scanout surfaces.
Review of attachment 363121 [details] [review]: ::: src/backends/native/meta-crtc-kms.c @@ +182,3 @@ +{ + return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset); +} These two functions needs to have their coding style fixed. @@ +187,3 @@ +parse_formats (MetaCrtc *crtc, + int kms_fd, + uint32_t blob_id) This should be: static void parse_formats (MetaCrtc *crtc, int kms_fd, uint32_t blob_id) @@ +406,3 @@ + MetaCrtcKms *crtc_kms = crtc->driver_private; + + g_array_free (crtc_kms->modifiers_xrgb8888, TRUE); Needs a NULL check, as AFAICS it's not guaranteed that crtc_kms->modifiers_xrgb8888 will be non-NULL.
Review of attachment 363122 [details] [review]: lgtm
Review of attachment 363123 [details] [review]: lgtm (just one seemingly misplaced include) ::: src/backends/native/meta-crtc-kms.c @@ +27,3 @@ #include "backends/native/meta-gpu-kms.h" +#include <drm_fourcc.h> Looks missplaced.
Review of attachment 363124 [details] [review]: ::: src/backends/native/meta-renderer-native-gles3.c @@ +69,3 @@ + * will fail. + * https://bugs.freedesktop.org/show_bug.cgi?id=76188 + */ Would be good to have this in the commit message. @@ +94,3 @@ + attribs[atti++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT; + attribs[atti++] = modifiers[0] >> 32; + } accidental indentation slip (here and two more places below) ::: src/backends/native/meta-renderer-native.c @@ +1641,3 @@ +static GArray * +get_supported_modifiers (CoglOnscreen *onscreen, + uint32_t format) nit: coding style @@ +1673,3 @@ + renderer_gpu_data = meta_renderer_native_get_gpu_data (renderer_native, + META_GPU_KMS (gpu)); + if (cogl_renderer_egl->platform != renderer_gpu_data && When is this not true? @@ +1679,3 @@ + goto error; + + for (c = meta_gpu_get_crtcs (gpu); c; c = c->next) nit: iterators are usually "l" or "k", etc, or "l_crtc" if "l" is already taken/ambigous. @@ +1683,3 @@ + MetaCrtc *crtc = c->data; + + if (logical_monitor && crtc->logical_monitor != logical_monitor) You early out on !logical_monitor so that check is redundant (here and below) @@ +1719,3 @@ + { + uint64_t modifier = g_array_index (base_mods, uint64_t, i); + gboolean found_everywhere = true; s/true/TRUE s/false/FALSE a bit here and there below @@ +1758,3 @@ + + if (found_everywhere) + g_array_append_val (modifiers, modifier); nit: indentation @@ +1769,3 @@ + return modifiers; + +error: Is this really an error? You go here even for cases where the EGL extension is missing, or where the & of all the mods of all the crtcs on an onscreen are nil, which is not really an error, just unfortunate, right? @@ +1841,3 @@ + width, height, format, + (uint64_t *) modifiers->data, + modifiers->len); Have you tested the secondary-gpu paths with this? What is the equivalent to GBM_BO_USE_LINEAR here? Or is it that what the & of the modifiers for all CRTCs does indirectly (i.e. it disables tiling modifiers and what not, which IIRC is what USE_LINEAR does)
Marking the pre-meta-gpu patches as obsolete.
(In reply to Jonas Ådahl from comment #13) > Review of attachment 363124 [details] [review] [review]: > > @@ +1673,3 @@ > + renderer_gpu_data = meta_renderer_native_get_gpu_data > (renderer_native, > + META_GPU_KMS > (gpu)); > + if (cogl_renderer_egl->platform != renderer_gpu_data && > > When is this not true? When the GPU is a secondary one (there is a similar test in should_surface_be_sharable()). > @@ +1841,3 @@ > + width, height, format, > + (uint64_t *) modifiers->data, > + modifiers->len); > > Have you tested the secondary-gpu paths with this? What is the equivalent to > GBM_BO_USE_LINEAR here? Or is it that what the & of the modifiers for all > CRTCs does indirectly (i.e. it disables tiling modifiers and what not, which > IIRC is what USE_LINEAR does) I couldn't test the secondary-gpu path given I don't have the setup to do so. The equivalent to GBM_BO_USE_LINEAR is DRM_FORMAT_MOD_LINEAR. Indeed, what we do is to get the set of common modifiers between all involved GPUs/CRTCs so we only use a format supported everywhere. If there is at least one common modifier (most likely DRM_FORMAT_MOD_LINEAR) then the surface is created using gbm_surface_create_with_modifiers. If not, it's using the same path than before and set the GBM_BO_USE_LINEAR flag. (It's important not to call gbm_surface_create_with_modifiers with an empty modifier list though, as there is no guarantee that MOD_LINEAR will be used)
Created attachment 365613 [details] [review] monitor-manager-kms: Add parsing for IN_FORMATS property
Created attachment 365614 [details] [review] renderer-native: Use modifier-aware GBM API
Created attachment 365615 [details] [review] renderer-native: Create GBM surfaces with modifiers
(In reply to Louis-Francis Ratté-Boulianne from comment #15) > (In reply to Jonas Ådahl from comment #13) > > Review of attachment 363124 [details] [review] [review] [review]: > > > > @@ +1673,3 @@ > > + renderer_gpu_data = meta_renderer_native_get_gpu_data > > (renderer_native, > > + META_GPU_KMS > > (gpu)); > > + if (cogl_renderer_egl->platform != renderer_gpu_data && > > > > When is this not true? > > When the GPU is a secondary one (there is a similar test in > should_surface_be_sharable()). Ah, it's a "is_secondary_gpu()" check. I guess I should blame myself for making that somewhat unreadable :P > > > > @@ +1841,3 @@ > > + width, height, format, > > + (uint64_t *) modifiers->data, > > + modifiers->len); > > > > Have you tested the secondary-gpu paths with this? What is the equivalent to > > GBM_BO_USE_LINEAR here? Or is it that what the & of the modifiers for all > > CRTCs does indirectly (i.e. it disables tiling modifiers and what not, which > > IIRC is what USE_LINEAR does) > > I couldn't test the secondary-gpu path given I don't have the setup to do > so. > The equivalent to GBM_BO_USE_LINEAR is DRM_FORMAT_MOD_LINEAR. > > Indeed, what we do is to get the set of common modifiers between all > involved GPUs/CRTCs so we only use a format supported everywhere. > If there is at least one common modifier (most likely DRM_FORMAT_MOD_LINEAR) > then the surface is created using gbm_surface_create_with_modifiers. > If not, it's using the same path than before and set the GBM_BO_USE_LINEAR > flag. > > (It's important not to call gbm_surface_create_with_modifiers with an empty > modifier list though, as there is no guarantee that MOD_LINEAR will be used) I guess this makes sense then. Wouldn't hurt with some testing if you know anybody who might have access to such hardware as it's very inconvenient for me to get access to such hardware as well.
Review of attachment 365613 [details] [review]: Still looks good, but I still have style nits in the same places as before. ::: src/backends/native/meta-crtc-kms.c @@ +173,3 @@ +static inline uint32_t * +formats_ptr(struct drm_format_modifier_blob *blob) still style nits left: space between formats_ptr and ( same for the next function
Review of attachment 365614 [details] [review]: lgtm
Review of attachment 365615 [details] [review]: lgtm, just some minor nits. ::: src/backends/native/meta-renderer-native.c @@ +1743,3 @@ + for (m = 0; m < crtc_mods->len; m++) + { + uint64_t local_mod = g_array_index (crtc_mods, uint64_t, m); Empty line after declarations. @@ +1828,3 @@ EGLSurface new_egl_surface; + uint32_t format = GBM_FORMAT_XRGB8888; + GArray *modifiers = NULL; No need to initialize here. @@ +1857,3 @@ + flags); + } + Double newline
(In reply to Jonas Ådahl from comment #19) > I guess this makes sense then. Wouldn't hurt with some testing if you know > anybody who might have access to such hardware as it's very inconvenient for > me to get access to such hardware as well. Unfortunately, I don't know anyone with the appropriate hardware. Do you think it should be feasible to push the patch upstream even without that? How was multi-gpu tested for the rest of Mutter?
(In reply to Louis-Francis Ratté-Boulianne from comment #23) > (In reply to Jonas Ådahl from comment #19) > > I guess this makes sense then. Wouldn't hurt with some testing if you know > > anybody who might have access to such hardware as it's very inconvenient for > > me to get access to such hardware as well. > > Unfortunately, I don't know anyone with the appropriate hardware. Do you > think it should be feasible to push the patch upstream even without that? > How was multi-gpu tested for the rest of Mutter? It was tested by me on borrowed hardware. I think we should still land things, because I see nothing wrong with the patches as is. Hopefully (but not definitely) I'll get a chance to play with the relevant hardware again before 3.28 is done, otherwise we'll have to rely on brave beta testers.
Review of attachment 365613 [details] [review]: ::: src/backends/native/meta-crtc-kms.c @@ +236,3 @@ + * 64 formats. */ + if (xrgb_idx < modifiers[i].offset || + This should be + 63 rather than + 64.
Created attachment 366695 [details] [review] monitor-manager-kms: Add parsing for IN_FORMATS property
Created attachment 366696 [details] [review] renderer-native: Create GBM surfaces with modifiers
Jonas, any update on getting these landed? If Mutter is branched, probably best to get them out there early ...
Pushed after fixing compilation error in one of the patches.