Re: [PATCH 2/2] drm/rockchip: Add support for afbc

[Daniel Stone]

Hi Andrzej,

On Fri, 11 Oct 2019 at 12:18, Andrzej Pietrasiewicz
<andrzej.p@xxxxxxxxxxxxx> wrote:
> @@ -32,6 +35,46 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>         int ret;
>         int i;
>
> +       if (mode_cmd->modifier[0]) {
> +               const struct drm_format_info *info;
> +               int bpp;
> +
> +               if (mode_cmd->modifier[0] &

Use != here, not &~.

> +       case DRM_FORMAT_BGR888:
> +               return AFBC_FMT_U8U8U8;
> +       case DRM_FORMAT_RGB565:
> +       case DRM_FORMAT_BGR565:
> +               return AFBC_FMT_RGB565;
> +       default:
> +               DRM_ERROR("unsupported afbc format[%08x]\n", format);

This should not be reachable, because you shouldn't be able to create
a framebuffer with an unsupported format/modifier combination.

> +static bool rockchip_mod_supported(struct drm_plane *plane,
> +                                  u32 format, u64 modifier)
> +{
> +       const struct drm_format_info *info;
> +
> +       if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
> +               return false;
> +
> +       if (modifier == DRM_FORMAT_MOD_LINEAR)
> +               return true;
> +
> +       if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(

Again use != here.

> +                               AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> +                               AFBC_FORMAT_MOD_SPARSE
> +                       )
> +       ) {
> +               DRM_DEBUG_KMS("Unsupported format modifer 0x%llx\n", modifier);
> +
> +               return false;
> +       }
> +
> +       info = drm_format_info(format);
> +       if (info->num_planes != 1) {
> +               DRM_DEBUG_KMS("AFBC buffers expect one plane\n");
> +               return false;
> +       }

This is where you should reject unsupported format + AFBC
combinations. Doing it here prevents it from ever being advertised to
userspace in the first place.

> @@ -808,6 +919,24 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>
>         spin_lock(&vop->reg_lock);
>
> +       if (fb->modifier == DRM_FORMAT_MOD_ARM_AFBC(
> +               AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE)) {
> +               int afbc_format = vop_convert_afbc_format(fb->format->format);
> +
> +               VOP_AFBC_SET(vop, format, afbc_format | 1 << 4);

I assume the (1 << 4) programs the 16x16 block size. Can we support
other block sizes?

> +               VOP_AFBC_SET(vop, hreg_block_split, 0);

Does this mean we can also support AFBC_FORMAT_MOD_SPLIT?

> +               VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
> +               VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
> +               VOP_AFBC_SET(vop, pic_size, act_info);
> +
> +               /*
> +                * The window being udated becomes the AFBC window
> +                */
> +               vop->afbc_win = vop_win;
> +
> +               pr_info("AFBC on plane %s\n", plane->name);

Please do not use pr_info() here. Userspace should not be able to
trigger logging, apart from DRM_DEBUG.

> +static const uint64_t format_modifiers_win_full[] = {
> +       DRM_FORMAT_MOD_NONE,

*DRM_FORMAT_MOD_LINEAR

Cheers,
Daniel