Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191011111813.20851-1-andrzej.p@collabora.com> <20191011111813.20851-3-andrzej.p@collabora.com>
In-Reply-To: <20191011111813.20851-3-andrzej.p@collabora.com>
From:   Daniel Stone <daniel@fooishbar.org>
Date:   Fri, 11 Oct 2019 12:59:02 +0100
Message-ID: <CAPj87rNb=Sa2Hg8KZK5ioocdv0BMj9c3NHK2v4UZibMmw2DdGA@mail.gmail.com>
Subject: Re: [PATCH 2/2] drm/rockchip: Add support for afbc
To:     Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc:     dri-devel <dri-devel@lists.freedesktop.org>,
        Sandy Huang <hjc@rock-chips.com>, kernel@collabora.com,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Liviu Dudau <liviu.dudau@arm.com>,
        Brian Starkey <brian.starkey@arm.com>,
        =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip <linux-rockchip@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Andrzej,

On Fri, 11 Oct 2019 at 12:18, Andrzej Pietrasiewicz
<andrzej.p@collabora.com> 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