kms window: fix implicit modifier support

 - don't attempt to iterate over modified pixel formats if not supported by the plane
 - add some documentation to the way we choose the allowed modifiers
 - fix `supported_modified_formats_blob` documentation in modesetting.h
 - fix logging for `gbm_surface_create` errors

Author: ardera

src/egl_gbm_render_surface.c

@@ -168,7 +168,7 @@ static int egl_gbm_render_surface_init(
         );
         if (gbm_surface == NULL) {
             ok = errno;
-            LOG_ERROR("Couldn't create GBM surface for rendering. gbm_surface_create_with_modifiers: %s\n", strerror(ok));
+            LOG_ERROR("Couldn't create GBM surface for rendering. gbm_surface_create: %s\n", strerror(ok));
             return ok;
         }
     }
@@ -187,10 +187,10 @@ static int egl_gbm_render_surface_init(
         }
     }
 
-    // EGLAttribKHR is defined by EGL_KHR_cl_event2.
-    #ifndef EGL_KHR_cl_event2
-        #error "EGL header definitions for extension EGL_KHR_cl_event2 are required."
-    #endif
+// EGLAttribKHR is defined by EGL_KHR_cl_event2.
+#ifndef EGL_KHR_cl_event2
+    #error "EGL header definitions for extension EGL_KHR_cl_event2 are required."
+#endif
 
     static const EGLAttribKHR surface_attribs[] = {
         /* EGL_GL_COLORSPACE, GL_LINEAR / GL_SRGB */
@@ -454,26 +454,28 @@ static int egl_gbm_render_surface_present_kms(struct surface *s, const struct fl
     TRACER_BEGIN(egl_surface->surface.tracer, "kms_req_builder_push_fb_layer");
     ok = kms_req_builder_push_fb_layer(
         builder,
-        &(const struct kms_fb_layer){ .drm_fb_id = fb_id,
-                                      .format = pixel_format,
-                                      .has_modifier = gbm_bo_get_modifier(bo) != DRM_FORMAT_MOD_INVALID,
-                                      .modifier = gbm_bo_get_modifier(bo),
-
-                                      .dst_x = (int32_t) props->aa_rect.offset.x,
-                                      .dst_y = (int32_t) props->aa_rect.offset.y,
-                                      .dst_w = (uint32_t) props->aa_rect.size.x,
-                                      .dst_h = (uint32_t) props->aa_rect.size.y,
-
-                                      .src_x = 0,
-                                      .src_y = 0,
-                                      .src_w = DOUBLE_TO_FP1616_ROUNDED(egl_surface->render_surface.size.x),
-                                      .src_h = DOUBLE_TO_FP1616_ROUNDED(egl_surface->render_surface.size.y),
-
-                                      .has_rotation = false,
-                                      .rotation = PLANE_TRANSFORM_ROTATE_0,
-
-                                      .has_in_fence_fd = false,
-                                      .in_fence_fd = 0, },
+        &(const struct kms_fb_layer){
+            .drm_fb_id = fb_id,
+            .format = pixel_format,
+            .has_modifier = gbm_bo_get_modifier(bo) != DRM_FORMAT_MOD_INVALID,
+            .modifier = gbm_bo_get_modifier(bo),
+
+            .dst_x = (int32_t) props->aa_rect.offset.x,
+            .dst_y = (int32_t) props->aa_rect.offset.y,
+            .dst_w = (uint32_t) props->aa_rect.size.x,
+            .dst_h = (uint32_t) props->aa_rect.size.y,
+
+            .src_x = 0,
+            .src_y = 0,
+            .src_w = DOUBLE_TO_FP1616_ROUNDED(egl_surface->render_surface.size.x),
+            .src_h = DOUBLE_TO_FP1616_ROUNDED(egl_surface->render_surface.size.y),
+
+            .has_rotation = false,
+            .rotation = PLANE_TRANSFORM_ROTATE_0,
+
+            .has_in_fence_fd = false,
+            .in_fence_fd = 0,
+        },
         on_release_layer,
         NULL,
         locked_fb_ref(egl_surface->locked_front_fb)

src/modesetting.h

@@ -410,7 +410,7 @@ struct drm_plane {
     uint32_t id;
 
     /// @brief Bitmap of the indexes of the CRTCs that this plane can be scanned out on.
-    /// 
+    ///
     /// I.e. if bit 0 is set, this plane can be scanned out on the CRTC with index 0.
     /// if bit 0 is not set, this plane can not be scanned out on that CRTC.
     uint32_t possible_crtcs;
@@ -438,7 +438,7 @@ struct drm_plane {
     /// @brief The minimum and maximum possible zpos
     ///
     /// Only valid if @ref has_zpos is true.
-    /// 
+    ///
     /// If @ref has_hardcoded_zpos is true, min_zpos should equal max_zpos.
     int64_t min_zpos, max_zpos;
 
@@ -494,9 +494,8 @@ struct drm_plane {
     ///
     /// DRM_FORMAT_MOD_LINEAR is supported for most (but not all pixel formats).
     /// There are some format & modifier pairs that may be faster to scanout by the GPU.
-    /// Might be NULL if the plane didn't specify an IN_FORMATS property.
-    //struct modified_format *supported_modified_formats;
-
+    ///
+    /// Is NULL if the plane didn't specify an IN_FORMATS property.
     struct drm_format_modifier_blob *supported_modified_formats_blob;
 
     /// @brief Whether this plane has a mutable alpha property we can set.
@@ -527,7 +526,7 @@ struct drm_plane {
         uint32_t src_x, src_y, src_w, src_h;
 
         /// @brief The committed destination rect, on the CRTC.
-        /// 
+        ///
         /// Only valid when using atomic modesetting.
         uint32_t crtc_x, crtc_y, crtc_w, crtc_h;
 
@@ -537,22 +536,22 @@ struct drm_plane {
         int64_t zpos;
 
         /// @brief The committed plane rotation.
-        /// 
+        ///
         /// Only valid if @ref drm_plane.has_rotation is true.
         drm_plane_transform_t rotation;
-        
+
         /// @brief The committed alpha property.
         ///
         /// Only valid if @ref drm_plane.has_alpha is true.
         uint16_t alpha;
-        
+
         /// @brief  The committed blend mode.
         ///
         /// Only valid if @ref drm_plane.has_blend_mode is true.
         enum drm_blend_mode blend_mode;
-        
+
         /// @brief If false, we don't know about the committed format.
-        /// 
+        ///
         /// This can be false on debian buster for example, because we don't
         /// have drmModeGetFB2 here, which is required for querying the pixel
         /// format of a framebuffer. When a plane is associated with our own
@@ -578,28 +577,30 @@ struct drm_plane {
 /**
  * @brief Callback that will be called on each iteration of
  * @ref drm_plane_foreach_modified_format.
- * 
+ *
  * Should return true if looping should continue. False if iterating should be
  * stopped.
- * 
+ *
  * @param plane The plane that was passed to @ref drm_plane_foreach_modified_format.
  * @param index The index of the pixel format. Is incremented for each call of the callback.
  * @param pixel_format The pixel format.
  * @param modifier The modifier of this pixel format.
  * @param userdata Userdata that was passed to @ref drm_plane_foreach_modified_format.
  */
-typedef bool (*drm_plane_modified_format_callback_t)(struct drm_plane *plane, int index, enum pixfmt pixel_format, uint64_t modifier, void *userdata);
+typedef bool (*drm_plane_modified_format_callback_t)(
+    struct drm_plane *plane,
+    int index,
+    enum pixfmt pixel_format,
+    uint64_t modifier,
+    void *userdata
+);
 
 /**
  * @brief Iterates over every supported pixel-format & modifier pair.
- * 
+ *
  * See @ref drm_plane_modified_format_callback_t for documentation on the callback.
  */
-void drm_plane_foreach_modified_format(
-    struct drm_plane *plane,
-    drm_plane_modified_format_callback_t callback,
-    void *userdata
-);
+void drm_plane_foreach_modified_format(struct drm_plane *plane, drm_plane_modified_format_callback_t callback, void *userdata);
 
 struct drmdev;
 struct _drmModeModeInfo;

src/window.c

@@ -27,13 +27,13 @@
 #include "util/collection.h"
 
 #ifdef HAVE_EGL_GLES2
-    #include "gl_renderer.h"
     #include "egl_gbm_render_surface.h"
+    #include "gl_renderer.h"
 #endif
 
 #ifdef HAVE_VULKAN
-    #include "vk_renderer.h"
     #include "vk_gbm_render_surface.h"
+    #include "vk_renderer.h"
 #endif
 
 FILE_DESCR("native windows")
@@ -1273,7 +1273,13 @@ static int kms_window_push_composition(struct window *window, struct fl_layer_co
     return ok;
 }
 
-static bool count_modifiers_for_pixel_format(UNUSED struct drm_plane *plane, UNUSED int index, enum pixfmt pixel_format, UNUSED uint64_t modifier, void *userdata) {
+static bool count_modifiers_for_pixel_format(
+    UNUSED struct drm_plane *plane,
+    UNUSED int index,
+    enum pixfmt pixel_format,
+    UNUSED uint64_t modifier,
+    void *userdata
+) {
     struct {
         enum pixfmt format;
         uint64_t *modifiers;
@@ -1288,7 +1294,13 @@ static bool count_modifiers_for_pixel_format(UNUSED struct drm_plane *plane, UNU
     return true;
 }
 
-static bool extract_modifiers_for_pixel_format(UNUSED struct drm_plane *plane, UNUSED int index, enum pixfmt pixel_format, uint64_t modifier, void *userdata) {
+static bool extract_modifiers_for_pixel_format(
+    UNUSED struct drm_plane *plane,
+    UNUSED int index,
+    enum pixfmt pixel_format,
+    uint64_t modifier,
+    void *userdata
+) {
     struct {
         enum pixfmt format;
         uint64_t *modifiers;
@@ -1330,14 +1342,25 @@ static struct render_surface *kms_window_get_render_surface_internal(struct wind
 
     // For now just set the supported modifiers for the first plane that supports this pixel format
     // as the allowed modifiers.
+    /// TODO: Find a way to rank pixel formats, maybe by number of planes that support them for scanout.
     {
         struct drm_plane *plane;
         for_each_plane_in_drmdev(window->kms.drmdev, plane) {
             if (!(plane->possible_crtcs & window->kms.crtc->bitmask)) {
+                // Only query planes that are possible to connect to the CRTC we're using.
                 continue;
             }
 
             if (plane->type != kPrimary_DrmPlaneType && plane->type != kOverlay_DrmPlaneType) {
+                // We explicitly only look for primary and overlay planes.
+                continue;
+            }
+
+            if (plane->supports_modifiers == NULL) {
+                // The plane does not have an IN_FORMATS property and does not support
+                // explicit modifiers.
+                //
+                // Calling drm_plane_foreach_modified_format below will segfault.
                 continue;
             }
 
@@ -1353,12 +1376,14 @@ static struct render_surface *kms_window_get_render_surface_internal(struct wind
                 .index = 0,
             };
 
+            // First, count the allowed modifiers for this pixel format.
             drm_plane_foreach_modified_format(plane, count_modifiers_for_pixel_format, &context);
 
             n_allowed_modifiers = context.n_modifiers;
             allowed_modifiers = calloc(n_allowed_modifiers, sizeof(*context.modifiers));
             context.modifiers = allowed_modifiers;
 
+            // Next, fill context.modifiers with the allowed modifiers.
             drm_plane_foreach_modified_format(plane, extract_modifiers_for_pixel_format, &context);
             break;
         }
@@ -1367,10 +1392,10 @@ static struct render_surface *kms_window_get_render_surface_internal(struct wind
     if (window->renderer_type == kOpenGL_RendererType) {
         // opengl
 #ifdef HAVE_EGL_GLES2
-        // EGL_NO_CONFIG_KHR is defined by EGL_KHR_no_config_context.
-        #ifndef EGL_KHR_no_config_context
-            #error "EGL header definitions for extension EGL_KHR_no_config_context are required."
-        #endif
+    // EGL_NO_CONFIG_KHR is defined by EGL_KHR_no_config_context.
+    #ifndef EGL_KHR_no_config_context
+        #error "EGL header definitions for extension EGL_KHR_no_config_context are required."
+    #endif
 
         struct egl_gbm_render_surface *egl_surface = egl_gbm_render_surface_new_with_egl_config(
             window->tracer,
@@ -1388,7 +1413,7 @@ static struct render_surface *kms_window_get_render_surface_internal(struct wind
         } else {
             render_surface = CAST_RENDER_SURFACE(egl_surface);
         }
-        
+
 #else
         UNREACHABLE();
 #endif