* [PATCH 3/3] drm/vc4: Add support for SAND modifier.
2018-03-16 22:04 [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier Eric Anholt
2018-03-16 22:04 ` [PATCH 2/3] drm/vc4: Add missing formats to vc4_format_mod_supported() Eric Anholt
@ 2018-03-16 22:04 ` Eric Anholt
2018-03-19 16:33 ` [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier Ville Syrjälä
2018-05-22 16:31 ` Ville Syrjälä
3 siblings, 0 replies; 5+ messages in thread
From: Eric Anholt @ 2018-03-16 22:04 UTC (permalink / raw)
To: dri-devel
Cc: Dave Stevenson, Boris Brezillon, Maxime Ripard, linux-kernel,
Daniel Stone
From: Dave Stevenson <dave.stevenson@raspberrypi.org>
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 <dave.stevenson@raspberrypi.org>
Signed-off-by: Eric Anholt <eric@anholt.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
---
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(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index ba00c14f989a..06ec6fb08fe4 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -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
};
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index 4af3e29d076a..fb8af19f12bc 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -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
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index e04613d30a13..64bf67abff7e 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -384,6 +384,23 @@ extern "C" {
#define DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_THIRTYTWO_GOB \
fourcc_mod_code(NVIDIA, 0x15)
+/*
+ * Some Broadcom modifiers take parameters, for example the number of
+ * vertical lines in the image. Reserve the lower 32 bits for modifier
+ * type, and the next 24 bits for parameters. Top 8 bits are the
+ * vendor code.
+ */
+#define __fourcc_mod_broadcom_param_shift 8
+#define __fourcc_mod_broadcom_param_bits 48
+#define fourcc_mod_broadcom_code(val, params) \
+ fourcc_mod_code(BROADCOM, ((((__u64)params) << __fourcc_mod_broadcom_param_shift) | val))
+#define fourcc_mod_broadcom_param(m) \
+ ((int)(((m) >> __fourcc_mod_broadcom_param_shift) & \
+ ((1ULL << __fourcc_mod_broadcom_param_bits) - 1)))
+#define fourcc_mod_broadcom_mod(m) \
+ ((m) & ~(((1ULL << __fourcc_mod_broadcom_param_bits) - 1) << \
+ __fourcc_mod_broadcom_param_shift))
+
/*
* Broadcom VC4 "T" format
*
@@ -405,6 +422,48 @@ extern "C" {
*/
#define DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED fourcc_mod_code(BROADCOM, 1)
+/*
+ * Broadcom SAND format
+ *
+ * This is the native format that the H.264 codec block uses. For VC4
+ * HVS, it is only valid for H.264 (NV12/21) and RGBA modes.
+ *
+ * The image can be considered to be split into columns, and the
+ * columns are placed consecutively into memory. The width of those
+ * columns can be either 32, 64, 128, or 256 pixels, but in practice
+ * only 128 pixel columns are used.
+ *
+ * The pitch between the start of each column is set to optimally
+ * switch between SDRAM banks. This is passed as the number of lines
+ * of column width in the modifier (we can't use the stride value due
+ * to various core checks that look at it , so you should set the
+ * stride to width*cpp).
+ *
+ * Note that the column height for this format modifier is the same
+ * for all of the planes, assuming that each column contains both Y
+ * and UV. Some SAND-using hardware stores UV in a separate tiled
+ * image from Y to reduce the column height, which is not supported
+ * with these modifiers.
+ */
+
+#define DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(v) \
+ fourcc_mod_broadcom_code(2, v)
+#define DRM_FORMAT_MOD_BROADCOM_SAND64_COL_HEIGHT(v) \
+ fourcc_mod_broadcom_code(3, v)
+#define DRM_FORMAT_MOD_BROADCOM_SAND128_COL_HEIGHT(v) \
+ fourcc_mod_broadcom_code(4, v)
+#define DRM_FORMAT_MOD_BROADCOM_SAND256_COL_HEIGHT(v) \
+ fourcc_mod_broadcom_code(5, v)
+
+#define DRM_FORMAT_MOD_BROADCOM_SAND32 \
+ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0)
+#define DRM_FORMAT_MOD_BROADCOM_SAND64 \
+ DRM_FORMAT_MOD_BROADCOM_SAND64_COL_HEIGHT(0)
+#define DRM_FORMAT_MOD_BROADCOM_SAND128 \
+ DRM_FORMAT_MOD_BROADCOM_SAND128_COL_HEIGHT(0)
+#define DRM_FORMAT_MOD_BROADCOM_SAND256 \
+ DRM_FORMAT_MOD_BROADCOM_SAND256_COL_HEIGHT(0)
+
#if defined(__cplusplus)
}
#endif
--
2.16.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier.
2018-03-16 22:04 [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier Eric Anholt
2018-03-16 22:04 ` [PATCH 2/3] drm/vc4: Add missing formats to vc4_format_mod_supported() Eric Anholt
2018-03-16 22:04 ` [PATCH 3/3] drm/vc4: Add support for SAND modifier Eric Anholt
@ 2018-03-19 16:33 ` Ville Syrjälä
2018-05-22 16:31 ` Ville Syrjälä
3 siblings, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2018-03-19 16:33 UTC (permalink / raw)
To: Eric Anholt; +Cc: linux-kernel, dri-devel
On Fri, Mar 16, 2018 at 03:04:33PM -0700, Eric Anholt wrote:
> 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 <eric@anholt.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/drm_plane.c | 23 ++++++++++++-----------
> include/drm/drm_plane.h | 5 ++++-
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 46fbd019a337..5bb501f1aae8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -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;
> }
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a48b1c3..6b1b51645f75 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -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.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier.
2018-03-16 22:04 [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier Eric Anholt
` (2 preceding siblings ...)
2018-03-19 16:33 ` [PATCH 1/3] drm: Trust format_mod_supported() when it OKs a plane modifier Ville Syrjälä
@ 2018-05-22 16:31 ` Ville Syrjälä
3 siblings, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2018-05-22 16:31 UTC (permalink / raw)
To: Eric Anholt; +Cc: linux-kernel, dri-devel
On Fri, Mar 16, 2018 at 03:04:33PM -0700, Eric Anholt wrote:
> 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 <eric@anholt.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
With https://patchwork.freedesktop.org/series/40207/ this should be
safe for i915, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/drm_plane.c | 23 ++++++++++++-----------
> include/drm/drm_plane.h | 5 ++++-
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 46fbd019a337..5bb501f1aae8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -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;
> }
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a48b1c3..6b1b51645f75 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -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:
> *
> --
> 2.16.2
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread