Hi,
I have a number of comments below. But if you want, you can add
Acked-by: Thomas Zimmermann tzimmermann@suse.de
Am 15.12.21 um 10:17 schrieb Maxime Ripard:
From: Dave Stevenson dave.stevenson@raspberrypi.com
The P030 format, used with the DRM_FORMAT_MOD_BROADCOM_SAND128 modifier, is a format output by the video decoder on the BCM2711.
Add native support to the KMS planes for that format.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_plane.c | 127 ++++++++++++++++++++++++-------- 1 file changed, 96 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index ac761c683663..022cd12f561e 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -33,6 +33,7 @@ static const struct hvs_format { u32 hvs; /* HVS_FORMAT_* */ u32 pixel_order; u32 pixel_order_hvs5;
- bool hvs5_only; } hvs_formats[] = { { .drm = DRM_FORMAT_XRGB8888,
@@ -128,6 +129,12 @@ static const struct hvs_format { .hvs = HVS_PIXEL_FORMAT_YCBCR_YUV422_2PLANE, .pixel_order = HVS_PIXEL_ORDER_XYCRCB, },
{
.drm = DRM_FORMAT_P030,.hvs = HVS_PIXEL_FORMAT_YCBCR_10BIT,.pixel_order = HVS_PIXEL_ORDER_XYCBCR,.hvs5_only = true,}, };
static const struct hvs_format *vc4_get_hvs_format(u32 drm_format)
@@ -762,47 +769,90 @@ static int vc4_plane_mode_set(struct drm_plane *plane, case DRM_FORMAT_MOD_BROADCOM_SAND128: case DRM_FORMAT_MOD_BROADCOM_SAND256: { uint32_t param = fourcc_mod_broadcom_param(fb->modifier);
u32 tile_w, tile, x_off, pix_per_tile;hvs_format = HVS_PIXEL_FORMAT_H264;switch (base_format_mod) {case DRM_FORMAT_MOD_BROADCOM_SAND64:tiling = SCALER_CTL0_TILING_64B;tile_w = 64;break;case DRM_FORMAT_MOD_BROADCOM_SAND128:tiling = SCALER_CTL0_TILING_128B;tile_w = 128;break;case DRM_FORMAT_MOD_BROADCOM_SAND256:tiling = SCALER_CTL0_TILING_256B_OR_T;tile_w = 256;break;default:break;}if (param > SCALER_TILE_HEIGHT_MASK) {
DRM_DEBUG_KMS("SAND height too large (%d)\n", param);
DRM_DEBUG_KMS("SAND height too large (%d)\n",param);
Should be good for the 100-character limit.
return -EINVAL; }
pix_per_tile = tile_w / fb->format->cpp[0];tile = vc4_state->src_x / pix_per_tile;x_off = vc4_state->src_x % pix_per_tile;
if (fb->format->format == DRM_FORMAT_P030) {hvs_format = HVS_PIXEL_FORMAT_YCBCR_10BIT;tiling = SCALER_CTL0_TILING_128B;} else {hvs_format = HVS_PIXEL_FORMAT_H264;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:return -EINVAL;
It's not atomic modesetting? I'm asking because the code returns errno codes in several places.
}}/* Adjust the base pointer to the first pixel to be scanned
- out.
** For P030, y_ptr [31:4] is the 128bit word for the start pixel* y_ptr [3:0] is the pixel (0-11) contained within that 128bit* word that should be taken as the first pixel.* Ditto uv_ptr [31:4] vs [3:0], however [3:0] contains the* element within the 128bit word, eg for pixel 3 the value* should be 6.*/ for (i = 0; i < num_planes; i++) {
u32 tile_w, tile, x_off, pix_per_tile;if (fb->format->format == DRM_FORMAT_P030) {/** Spec says: bits [31:4] of the given address* should point to the 128-bit word containing* the desired starting pixel, and bits[3:0]* should be between 0 and 11, indicating which* of the 12-pixels in that 128-bit word is the* first pixel to be used*/u32 remaining_pixels = vc4_state->src_x % 96;u32 aligned = remaining_pixels / 12;u32 last_bits = remaining_pixels % 12;x_off = aligned * 16 + last_bits;tile_w = 128;pix_per_tile = 96;} else {switch (base_format_mod) {case DRM_FORMAT_MOD_BROADCOM_SAND64:tile_w = 64;break;case DRM_FORMAT_MOD_BROADCOM_SAND128:tile_w = 128;break;case DRM_FORMAT_MOD_BROADCOM_SAND256:tile_w = 256;break;default:return -EINVAL;}pix_per_tile = tile_w / fb->format->cpp[0];x_off = (vc4_state->src_x % pix_per_tile) /(i ? h_subsample : 1) *fb->format->cpp[i];}tile = vc4_state->src_x / pix_per_tile;
It's hard to read. If you can put some of these switches into helpers, it might be worth it.
vc4_state->offsets[i] += param * tile_w * tile; vc4_state->offsets[i] += src_y / (i ? v_subsample : 1) * tile_w;
vc4_state->offsets[i] += x_off /(i ? h_subsample : 1) *fb->format->cpp[i];
vc4_state->offsets[i] += x_off & ~(i ? 1 : 0);}
pitch0 = VC4_SET_FIELD(param, SCALER_TILE_HEIGHT);
@@ -955,7 +1005,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
/* Pitch word 1/2 */ for (i = 1; i < num_planes; i++) {
if (hvs_format != HVS_PIXEL_FORMAT_H264) {
if (hvs_format != HVS_PIXEL_FORMAT_H264 &&hvs_format != HVS_PIXEL_FORMAT_YCBCR_10BIT) {
This branch's condition looks like is could be a little helper.
vc4_dlist_write(vc4_state, VC4_SET_FIELD(fb->pitches[i], SCALER_SRC_PITCH));@@ -1315,6 +1366,13 @@ static bool vc4_format_mod_supported(struct drm_plane *plane, default: return false; }
- case DRM_FORMAT_P030:
switch (fourcc_mod_broadcom_mod(modifier)) {case DRM_FORMAT_MOD_BROADCOM_SAND128:return true;default:return false;}@@ -1347,8 +1405,11 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev, struct drm_plane *plane = NULL; struct vc4_plane *vc4_plane; u32 formats[ARRAY_SIZE(hvs_formats)];
- int num_formats = 0; int ret = 0; unsigned i;
- bool hvs5 = of_device_is_compatible(dev->dev->of_node,
"brcm,bcm2711-vc5");
Maybe also a little helper function?
static const uint64_t modifiers[] = { DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED, DRM_FORMAT_MOD_BROADCOM_SAND128, @@ -1363,13 +1424,17 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev, if (!vc4_plane) return ERR_PTR(-ENOMEM);
- for (i = 0; i < ARRAY_SIZE(hvs_formats); i++)
formats[i] = hvs_formats[i].drm;
- for (i = 0; i < ARRAY_SIZE(hvs_formats); i++) {
if (!hvs_formats[i].hvs5_only || hvs5) {formats[num_formats] = hvs_formats[i].drm;num_formats++;}- }
With this loop, could num_formats ever be 0?
Best regards Thomas
plane = &vc4_plane->base; ret = drm_universal_plane_init(dev, plane, 0, &vc4_plane_funcs,
formats, ARRAY_SIZE(formats),
formats, num_formats, modifiers, type, NULL);