[PATCH v4 2/3] drm/vc4: plane: Add support for DRM_FORMAT_P030
Thomas Zimmermann
tzimmermann at suse.de
Wed Dec 15 15:11:50 UTC 2021
Hi,
I have a number of comments below. But if you want, you can add
Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
Am 15.12.21 um 10:17 schrieb Maxime Ripard:
> From: Dave Stevenson <dave.stevenson at 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 at raspberrypi.com>
> Signed-off-by: Maxime Ripard <maxime at 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;
> + }
> case DRM_FORMAT_RGBX1010102:
> case DRM_FORMAT_BGRX1010102:
> case DRM_FORMAT_RGBA1010102:
> @@ -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);
> if (ret)
> return ERR_PTR(ret);
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20211215/a5125e0d/attachment-0001.sig>
More information about the dri-devel
mailing list