The previous wording could be understood by user-space evelopers as "a primary/cursor plane is only compatible with a single CRTC" [1].
Reword the planes description to make it clear the DRM-internal drm_crtc.primary and drm_crtc.cursor planes are for legacy uAPI.
[1]: https://github.com/swaywm/wlroots/pull/2333#discussion_r456788057
Signed-off-by: Simon Ser contact@emersion.fr Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Acked-by: Pekka Paalanen pekka.paalanen@collabora.com --- drivers/gpu/drm/drm_crtc.c | 3 +++ drivers/gpu/drm/drm_plane.c | 16 +++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8d19d258547f..a6336c7154d6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -256,6 +256,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc) * planes). For really simple hardware which has only 1 plane look at * drm_simple_display_pipe_init() instead. * + * The @primary and @cursor planes are only relevant for legacy uAPI, see + * &drm_crtc.primary and &drm_crtc.cursor. + * * Returns: * Zero on success, error code on failure. */ diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 385801dd21f9..5d33ca9f0032 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -49,14 +49,16 @@ * &struct drm_plane (possibly as part of a larger structure) and registers it * with a call to drm_universal_plane_init(). * - * Cursor and overlay planes are optional. All drivers should provide one - * primary plane per CRTC to avoid surprising userspace too much. See enum - * drm_plane_type for a more in-depth discussion of these special uapi-relevant - * plane types. Special planes are associated with their CRTC by calling - * drm_crtc_init_with_planes(). - * * The type of a plane is exposed in the immutable "type" enumeration property, - * which has one of the following values: "Overlay", "Primary", "Cursor". + * which has one of the following values: "Overlay", "Primary", "Cursor" (see + * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see + * &drm_plane.possible_crtcs. + * + * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core + * relies on the driver to set the primary and optionally the cursor plane used + * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All + * drivers should provide one primary plane per CRTC to avoid surprising legacy + * userspace too much. */
static unsigned int drm_num_planes(struct drm_device *dev)
If a primary or cursor plane is not compatible with a CRTC it's attached to via the legacy primary/cursor field, things will be broken for legacy user-space.
v4: use drm_crtc_mask instead of BIT (Ville)
Signed-off-by: Simon Ser contact@emersion.fr Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Acked-by: Pekka Paalanen pekka.paalanen@collabora.com Cc: Ville Syrjala ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_mode_config.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..1628c8b60d9a 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -625,6 +625,7 @@ static void validate_encoder_possible_crtcs(struct drm_encoder *encoder) void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; + struct drm_crtc *crtc;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return; @@ -636,4 +637,19 @@ void drm_mode_config_validate(struct drm_device *dev) validate_encoder_possible_clones(encoder); validate_encoder_possible_crtcs(encoder); } + + drm_for_each_crtc(crtc, dev) { + if (crtc->primary) { + WARN(!(crtc->primary->possible_crtcs & drm_crtc_mask(crtc)), + "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", + crtc->primary->base.id, crtc->primary->name, + crtc->base.id, crtc->name); + } + if (crtc->cursor) { + WARN(!(crtc->cursor->possible_crtcs & drm_crtc_mask(crtc)), + "Bogus cursor plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", + crtc->cursor->base.id, crtc->cursor->name, + crtc->base.id, crtc->name); + } + } }
If a CRTC is missing a legacy primary plane pointer, a lot of things will be broken for user-space: fbdev stops working and the entire legacy uAPI stops working.
Require all drivers to populate drm_crtc.primary to prevent these issues. Warn if it's NULL.
Signed-off-by: Simon Ser contact@emersion.fr Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Acked-by: Pekka Paalanen pekka.paalanen@collabora.com --- drivers/gpu/drm/drm_mode_config.c | 3 +++ drivers/gpu/drm/drm_plane.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 1628c8b60d9a..1e5da83fd2a8 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -639,6 +639,9 @@ void drm_mode_config_validate(struct drm_device *dev) }
drm_for_each_crtc(crtc, dev) { + WARN(!crtc->primary, "Missing primary plane on [CRTC:%d:%s]\n", + crtc->base.id, crtc->name); + if (crtc->primary) { WARN(!(crtc->primary->possible_crtcs & drm_crtc_mask(crtc)), "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 5d33ca9f0032..49b0a8b9ac02 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -57,7 +57,7 @@ * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core * relies on the driver to set the primary and optionally the cursor plane used * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All - * drivers should provide one primary plane per CRTC to avoid surprising legacy + * drivers must provide one primary plane per CRTC to avoid surprising legacy * userspace too much. */
User-space expects to be able to pick a primary plane for each CRTC exposed by the driver. Make sure this assumption holds in drm_mode_config_validate.
Use the legacy drm_crtc.primary field to check this, because it's simpler and we require drivers to set it anyways. Accumulate a set of primary planes which are already used for a CRTC in a bitmask. Error out if a primary plane is re-used.
v2: new patch
v3: - Use u64 instead of __u64 (Jani) - Use `unsigned int` instead of `unsigned` (Jani)
v4: - Use u32 instead of u64 for plane mask (Ville) - Use drm_plane_mask instead of BIT (Ville) - Fix typos (Ville)
Signed-off-by: Simon Ser contact@emersion.fr Reviewed-by: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Ville Syrjala ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_mode_config.c | 21 +++++++++++++++++++++ drivers/gpu/drm/drm_plane.c | 6 ++++++ 2 files changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 1e5da83fd2a8..7ffc0d6a8e2c 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -626,6 +626,9 @@ void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; struct drm_crtc *crtc; + struct drm_plane *plane; + u32 primary_with_crtc = 0, cursor_with_crtc = 0; + unsigned int num_primary = 0;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return; @@ -647,12 +650,30 @@ void drm_mode_config_validate(struct drm_device *dev) "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", crtc->primary->base.id, crtc->primary->name, crtc->base.id, crtc->name); + WARN(primary_with_crtc & drm_plane_mask(crtc->primary), + "Primary plane [PLANE:%d:%s] used for multiple CRTCs", + crtc->primary->base.id, crtc->primary->name); + primary_with_crtc |= drm_plane_mask(crtc->primary); } if (crtc->cursor) { WARN(!(crtc->cursor->possible_crtcs & drm_crtc_mask(crtc)), "Bogus cursor plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", crtc->cursor->base.id, crtc->cursor->name, crtc->base.id, crtc->name); + WARN(cursor_with_crtc & drm_plane_mask(crtc->cursor), + "Cursor plane [PLANE:%d:%s] used for multiple CRTCs", + crtc->cursor->base.id, crtc->cursor->name); + cursor_with_crtc |= drm_plane_mask(crtc->cursor); } } + + drm_for_each_plane(plane, dev) { + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { + num_primary++; + } + } + + WARN(num_primary != dev->mode_config.num_crtc, + "Must have as many primary planes as there are CRTCs, but have %u primary planes and %u CRTCs", + num_primary, dev->mode_config.num_crtc); } diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 49b0a8b9ac02..a1f4510efa83 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -54,6 +54,12 @@ * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see * &drm_plane.possible_crtcs. * + * Each CRTC must have a unique primary plane userspace can attach to enable + * the CRTC. In other words, userspace must be able to attach a different + * primary plane to each CRTC at the same time. Primary planes can still be + * compatible with multiple CRTCs. There must be exactly as many primary planes + * as there are CRTCs. + * * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core * relies on the driver to set the primary and optionally the cursor plane used * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All
dri-devel@lists.freedesktop.org
-
Simon Ser