drm: Trust format_mod_supported() when it OKs a plane modifier.

PATCH In review Eric Anholt 2 participants 5 messages Mar 16, 2018 – May 22, 2018

Subsystems (3)

DRM drivers DRM misc DRM drivers for VC4

lore

Share & tools

lore

Eric Anholt

author

2018-03-16 22:04 UTC #0 lore raw

PATCH 1/3 drm: Trust format_mod_supported() when it OKs a plane modifier.

For parameterized modifiers (Broadcom's SAND and UIF), we need to allow the parameter fields to be filled in, while exposing only the variant of the modifier with the parameter unfilled in the internal arrays and the format blob.

Signed-off-by Eric Anholt <[email protected]>

Cc Ville Syrjälä <[email protected]>

---
 drivers/gpu/drm/drm_plane.c | 23 ++++++++++++-----------
 include/drm/drm_plane.h     |  5 ++++-
 2 files changed, 16 insertions(+), 12 deletions(-)

drivers/gpu/drm/drm_plane.c +12 ~~-11~~

index 46fbd019a337..5bb501f1aae8 100644
@@ -561,19 +561,20 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (i == plane->format_count)
 		return -EINVAL;
 
-	if (!plane->modifier_count)
-		return 0;
+	if (plane->funcs->format_mod_supported) {
+		if (!plane->funcs->format_mod_supported(plane, format, modifier))
+			return -EINVAL;
+	} else {
+		if (!plane->modifier_count)
+			return 0;
 
-	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
-			break;
+		for (i = 0; i < plane->modifier_count; i++) {
+			if (modifier == plane->modifiers[i])
+				break;
+		}
+		if (i == plane->modifier_count)
+			return -EINVAL;
 	}
-	if (i == plane->modifier_count)
-		return -EINVAL;
-
-	if (plane->funcs->format_mod_supported &&
-	    !plane->funcs->format_mod_supported(plane, format, modifier))
-		return -EINVAL;
 
 	return 0;
 }

include/drm/drm_plane.h +4 -1

index f7bf4a48b1c3..6b1b51645f75 100644
@@ -420,7 +420,10 @@ struct drm_plane_funcs {
 	 * This optional hook is used for the DRM to determine if the given
 	 * format/modifier combination is valid for the plane. This allows the
 	 * DRM to generate the correct format bitmask (which formats apply to
-	 * which modifier).
+	 * which modifier), and to valdiate modifiers at atomic_check time.
+	 *
+	 * If not present, then any modifier in the plane's modifier
+	 * list is allowed with any of the plane's formats.
 	 *
 	 * Returns:
 	 *

Signature · 1 line

2.16.2

Eric Anholt

author

2018-03-16 22:04 UTC #1 lore raw

PATCH 2/3 drm/vc4: Add missing formats to vc4_format_mod_supported().

Daniel's format_mod_supported() patch predated Dave's for NV21/61, and I didn't catch that when rebasing. This is a problem since the formats are now getting validated before being passed to the driver's atomic hooks.

Signed-off-by Eric Anholt <[email protected]>

Cc Daniel Stone <[email protected]>

Cc Dave Stevenson <[email protected]>

Fixes 423ad7b3cbd1 ("drm/vc4: Advertise supported modifiers for planes")

---
 drivers/gpu/drm/vc4/vc4_plane.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/vc4/vc4_plane.c +2 -0

index ce39390be389..ba00c14f989a 100644
@@ -884,7 +884,9 @@ static bool vc4_format_mod_supported(struct drm_plane *plane,
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
 	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV21:
 	case DRM_FORMAT_NV16:
+	case DRM_FORMAT_NV61:
 	default:
 		return (modifier == DRM_FORMAT_MOD_LINEAR);
 	}

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Eric Anholt

author

2018-03-16 22:04 UTC #2 lore raw

PATCH 3/3 drm/vc4: Add support for SAND modifier.

In reply to Eric Anholt

From: Dave Stevenson <[email protected]>

This is the format generated by VC4's H.264 engine, and preferred by the ISP as well. By displaying SAND buffers directly, we can avoid needing to use the ISP to rewrite the SAND H.264 output to linear before display.

This is a joint effort by Dave Stevenson (who wrote the initial patch and DRM demo) and Eric Anholt (drm_fourcc.h generalization, safety checks, RGBA support).

v2: Make the parameter macro give all of the middle 48 bits (suggested
by Daniels). Fix fourcc_mod_broadcom_mod()'s bits/shift being swapped. Mark NV12/21 as supported, not YUV420.

Signed-off-by Dave Stevenson <[email protected]>

Signed-off-by Eric Anholt <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Daniel Stone <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Maxime Ripard <[email protected]>

---
 drivers/gpu/drm/vc4/vc4_plane.c | 84 +++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/vc4/vc4_regs.h  |  6 +++
 include/uapi/drm/drm_fourcc.h   | 59 +++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 7 deletions(-)

drivers/gpu/drm/vc4/vc4_plane.c +77 -7

index ba00c14f989a..06ec6fb08fe4 100644
@@ -466,11 +466,13 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	struct drm_framebuffer *fb = state->fb;
 	u32 ctl0_offset = vc4_state->dlist_count;
 	const struct hvs_format *format = vc4_get_hvs_format(fb->format->format);
+	u64 base_format_mod = fourcc_mod_broadcom_mod(fb->modifier);
 	int num_planes = drm_format_num_planes(format->drm);
 	bool covers_screen;
 	u32 scl0, scl1, pitch0;
 	u32 lbm_size, tiling;
 	unsigned long irqflags;
+	u32 hvs_format = format->hvs;
 	int ret, i;
 
 	ret = vc4_plane_setup_clipping_and_scaling(state);

@@ -510,7 +512,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		scl1 = vc4_get_scl_field(state, 0);
 	}
 
-	switch (fb->modifier) {
+	switch (base_format_mod) {
 	case DRM_FORMAT_MOD_LINEAR:
 		tiling = SCALER_CTL0_TILING_LINEAR;
 		pitch0 = VC4_SET_FIELD(fb->pitches[0], SCALER_SRC_PITCH);

@@ -533,6 +535,49 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		break;
 	}
 
+	case DRM_FORMAT_MOD_BROADCOM_SAND64:
+	case DRM_FORMAT_MOD_BROADCOM_SAND128:
+	case DRM_FORMAT_MOD_BROADCOM_SAND256: {
+		uint32_t param = fourcc_mod_broadcom_param(fb->modifier);
+
+		/* Column-based NV12 or RGBA.
+		 */
+		if (fb->format->num_planes > 1) {
+			if (hvs_format != HVS_PIXEL_FORMAT_YCBCR_YUV420_2PLANE) {
+				DRM_DEBUG_KMS("SAND format only valid for NV12/21");
+				return -EINVAL;
+			}
+			hvs_format = HVS_PIXEL_FORMAT_H264;
+		} else {
+			if (base_format_mod == DRM_FORMAT_MOD_BROADCOM_SAND256) {
+				DRM_DEBUG_KMS("SAND256 format only valid for H.264");
+				return -EINVAL;
+			}
+		}
+
+		switch (base_format_mod) {
+		case DRM_FORMAT_MOD_BROADCOM_SAND64:
+			tiling = SCALER_CTL0_TILING_64B;
+			break;
+		case DRM_FORMAT_MOD_BROADCOM_SAND128:
+			tiling = SCALER_CTL0_TILING_128B;
+			break;
+		case DRM_FORMAT_MOD_BROADCOM_SAND256:
+			tiling = SCALER_CTL0_TILING_256B_OR_T;
+			break;
+		default:
+			break;
+		}
+
+		if (param > SCALER_TILE_HEIGHT_MASK) {
+			DRM_DEBUG_KMS("SAND height too large (%d)\n", param);
+			return -EINVAL;
+		}
+
+		pitch0 = VC4_SET_FIELD(param, SCALER_TILE_HEIGHT);
+		break;
+	}
+
 	default:
 		DRM_DEBUG_KMS("Unsupported FB tiling flag 0x%16llx",
 			      (long long)fb->modifier);

@@ -543,7 +588,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	vc4_dlist_write(vc4_state,
 			SCALER_CTL0_VALID |
 			(format->pixel_order << SCALER_CTL0_ORDER_SHIFT) |
-			(format->hvs << SCALER_CTL0_PIXEL_FORMAT_SHIFT) |
+			(hvs_format << SCALER_CTL0_PIXEL_FORMAT_SHIFT) |
 			VC4_SET_FIELD(tiling, SCALER_CTL0_TILING) |
 			(vc4_state->is_unity ? SCALER_CTL0_UNITY : 0) |
 			VC4_SET_FIELD(scl0, SCALER_CTL0_SCL0) |

@@ -597,8 +642,13 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 
 	/* Pitch word 1/2 */
 	for (i = 1; i < num_planes; i++) {
-		vc4_dlist_write(vc4_state,
-				VC4_SET_FIELD(fb->pitches[i], SCALER_SRC_PITCH));
+		if (hvs_format != HVS_PIXEL_FORMAT_H264) {
+			vc4_dlist_write(vc4_state,
+					VC4_SET_FIELD(fb->pitches[i],
+						      SCALER_SRC_PITCH));
+		} else {
+			vc4_dlist_write(vc4_state, pitch0);
+		}
 	}
 
 	/* Colorspace conversion words */

@@ -878,13 +928,30 @@ static bool vc4_format_mod_supported(struct drm_plane *plane,
 	case DRM_FORMAT_BGR565:
 	case DRM_FORMAT_ARGB1555:
 	case DRM_FORMAT_XRGB1555:
-		return true;
+		switch (fourcc_mod_broadcom_mod(modifier)) {
+		case DRM_FORMAT_MOD_LINEAR:
+		case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
+		case DRM_FORMAT_MOD_BROADCOM_SAND64:
+		case DRM_FORMAT_MOD_BROADCOM_SAND128:
+			return true;
+		default:
+			return false;
+		}
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV21:
+		switch (fourcc_mod_broadcom_mod(modifier)) {
+		case DRM_FORMAT_MOD_LINEAR:
+		case DRM_FORMAT_MOD_BROADCOM_SAND64:
+		case DRM_FORMAT_MOD_BROADCOM_SAND128:
+		case DRM_FORMAT_MOD_BROADCOM_SAND256:
+			return true;
+		default:
+			return false;
+		}
 	case DRM_FORMAT_YUV422:
 	case DRM_FORMAT_YVU422:
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
 	case DRM_FORMAT_NV16:
 	case DRM_FORMAT_NV61:
 	default:

@@ -914,6 +981,9 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
 	unsigned i;
 	static const uint64_t modifiers[] = {
 		DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
+		DRM_FORMAT_MOD_BROADCOM_SAND128,
+		DRM_FORMAT_MOD_BROADCOM_SAND64,
+		DRM_FORMAT_MOD_BROADCOM_SAND256,
 		DRM_FORMAT_MOD_LINEAR,
 		DRM_FORMAT_MOD_INVALID
 	};

drivers/gpu/drm/vc4/vc4_regs.h +6 -0

index 4af3e29d076a..fb8af19f12bc 100644
@@ -1030,6 +1030,12 @@ enum hvs_pixel_format {
 #define SCALER_SRC_PITCH_MASK			VC4_MASK(15, 0)
 #define SCALER_SRC_PITCH_SHIFT			0
 
+/* PITCH0/1/2 fields for tiled (SAND). */
+#define SCALER_TILE_SKIP_0_MASK			VC4_MASK(18, 16)
+#define SCALER_TILE_SKIP_0_SHIFT		16
+#define SCALER_TILE_HEIGHT_MASK			VC4_MASK(15, 0)
+#define SCALER_TILE_HEIGHT_SHIFT		0
+
 /* PITCH0 fields for T-tiled. */
 #define SCALER_PITCH0_TILE_WIDTH_L_MASK		VC4_MASK(22, 16)
 #define SCALER_PITCH0_TILE_WIDTH_L_SHIFT	16

include/uapi/drm/drm_fourcc.h +59 -0

71 lines not on this page · raw message

Signature · 6 lines

2.16.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2018-03-19 16:33 UTC #3 lore raw

In reply to Eric Anholt

Quoted 53 lines · Eric Anholt

Signed-off-by Eric Anholt <[email protected]>

Cc Ville Syrjälä <[email protected]>

---
 drivers/gpu/drm/drm_plane.c | 23 ++++++++++++-----------
 include/drm/drm_plane.h     |  5 ++++-
 2 files changed, 16 insertions(+), 12 deletions(-)

drivers/gpu/drm/drm_plane.c +12 ~~-11~~

index 46fbd019a337..5bb501f1aae8 100644
@@ -561,19 +561,20 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (i == plane->format_count)
 		return -EINVAL;
 
-	if (!plane->modifier_count)
-		return 0;
+	if (plane->funcs->format_mod_supported) {
+		if (!plane->funcs->format_mod_supported(plane, format, modifier))
+			return -EINVAL;
+	} else {
+		if (!plane->modifier_count)
+			return 0;
 
-	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
-			break;
+		for (i = 0; i < plane->modifier_count; i++) {
+			if (modifier == plane->modifiers[i])
+				break;
+		}
+		if (i == plane->modifier_count)
+			return -EINVAL;
 	}
-	if (i == plane->modifier_count)
-		return -EINVAL;
-
-	if (plane->funcs->format_mod_supported &&
-	    !plane->funcs->format_mod_supported(plane, format, modifier))
-		return -EINVAL;
 
 	return 0;
 }

include/drm/drm_plane.h +0 -0

index f7bf4a48b1c3..6b1b51645f75 100644

include/drm/drm_plane.h struct drm_plane_funcs { · line 426

Quoted 8 lines

include/drm/drm_plane.h +4 -1

@@ -420,7 +420,10 @@ struct drm_plane_funcs {
 	 * This optional hook is used for the DRM to determine if the given
 	 * format/modifier combination is valid for the plane. This allows the
 	 * DRM to generate the correct format bitmask (which formats apply to
-	 * which modifier).
+	 * which modifier), and to valdiate modifiers at atomic_check time.
+	 *
+	 * If not present, then any modifier in the plane's modifier
+	 * list is allowed with any of the plane's formats.

This certainly avoids the vfunc profileration that using the modifiers list causes. So yeah I do like it from that angle.

It does promote .format_mod_supported() to a more prominent role. My original idea for .format_mod_supported() was that it could be very lightweight; just reject the exceptional cases and assume everything else is fine. With the change of role I think the docs should make it very clear that _any_ modifier can be passed to this function directly from userspace. The driver must handle that gracefully and without spewing any non-debug stuff to dmesg.

Also I definitely need to change i915. Currently we're using the modifier list as the primary means by which we filter out unsupported modifiers for planes, and .format_mod_support() just serves an supporting role further removing any format+mod combination that isn't valid. So I need to make .format_mod_supported() the lead actor here and the modifier list just serves as a starting point which we filter down to something we can report via the IN_FORMATS blobifier.

Hmm. And now the question becomes should we make .format_mod_supported() take over the role of validating the pixel format as well from the plane's format list? It would certainly be more in line with the approach we're now going to take with modifiers.

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

Reviewed-by on v1 1/3

2018-05-22 16:31 UTC #4 lore raw

In reply to Eric Anholt

Quoted 7 lines · Eric Anholt

For parameterized modifiers (Broadcom's SAND and UIF), we need to allow the parameter fields to be filled in, while exposing only the variant of the modifier with the parameter unfilled in the internal arrays and the format blob.
Signed-off-by Eric Anholt <[email protected]>
Cc Ville Syrjälä <[email protected]>

With https://patchwork.freedesktop.org/series/40207/ this should be safe for i915, so

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 59 lines

---
 drivers/gpu/drm/drm_plane.c | 23 ++++++++++++-----------
 include/drm/drm_plane.h     |  5 ++++-
 2 files changed, 16 insertions(+), 12 deletions(-)

drivers/gpu/drm/drm_plane.c +12 ~~-11~~

index 46fbd019a337..5bb501f1aae8 100644
@@ -561,19 +561,20 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (i == plane->format_count)
 		return -EINVAL;
 
-	if (!plane->modifier_count)
-		return 0;
+	if (plane->funcs->format_mod_supported) {
+		if (!plane->funcs->format_mod_supported(plane, format, modifier))
+			return -EINVAL;
+	} else {
+		if (!plane->modifier_count)
+			return 0;
 
-	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
-			break;
+		for (i = 0; i < plane->modifier_count; i++) {
+			if (modifier == plane->modifiers[i])
+				break;
+		}
+		if (i == plane->modifier_count)
+			return -EINVAL;
 	}
-	if (i == plane->modifier_count)
-		return -EINVAL;
-
-	if (plane->funcs->format_mod_supported &&
-	    !plane->funcs->format_mod_supported(plane, format, modifier))
-		return -EINVAL;
 
 	return 0;
 }

include/drm/drm_plane.h +4 -1

index f7bf4a48b1c3..6b1b51645f75 100644
@@ -420,7 +420,10 @@ struct drm_plane_funcs {
 	 * This optional hook is used for the DRM to determine if the given
 	 * format/modifier combination is valid for the plane. This allows the
 	 * DRM to generate the correct format bitmask (which formats apply to
-	 * which modifier).
+	 * which modifier), and to valdiate modifiers at atomic_check time.
+	 *
+	 * If not present, then any modifier in the plane's modifier
+	 * list is allowed with any of the plane's formats.
 	 *
 	 * Returns:
 	 *

Signature · 1 line

2.16.2

Signature · 6 lines

Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel