Hi Thomas,
Thanks for your review
On Wed, Dec 15, 2021 at 04:11:50PM +0100, Thomas Zimmermann wrote:
> Hi,
>
> I have a number of comments below. But if you want, you can add
>
> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Thanks :)
> Am 15.12.21 um 10:17 schrieb Maxime Ripard:
> > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> > 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.
This is atomic modesetting, but we're allowed to return an error at this
point :)
The function name is a bit confusing but it's mostly due to how the
hardware operates. We don't have the usual register set for the
composition but instead we have a list of hardware descriptors that will
describe the next frame.
The driver builds it here, at atomic_check time, and then copy it to the
hardware at atomic_commit time. So even though it's called
vc4_plane_mode_set, and does the operations needed to setup the
composition properly, we're still in atomic_check at this point.
> > + }
> > + }
> > /* 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.
Indeed. The whole function would need some though, is it ok if I send a
subsequent patch to fix it after merging this one?
> > 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?
It shouldn't no, unless the older generation didn't define any format,
which is highly unlikely.
Maxime
Attachment:
signature.asc
Description: PGP signature
