[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPj87rNb=Sa2Hg8KZK5ioocdv0BMj9c3NHK2v4UZibMmw2DdGA@mail.gmail.com>
Date: Fri, 11 Oct 2019 12:59:02 +0100
From: Daniel Stone <daniel@...ishbar.org>
To: Andrzej Pietrasiewicz <andrzej.p@...labora.com>
Cc: dri-devel <dri-devel@...ts.freedesktop.org>,
Sandy Huang <hjc@...k-chips.com>, kernel@...labora.com,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Liviu Dudau <liviu.dudau@....com>,
Brian Starkey <brian.starkey@....com>,
Heiko Stübner <heiko@...ech.de>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip <linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH 2/2] drm/rockchip: Add support for afbc
Hi Andrzej,
On Fri, 11 Oct 2019 at 12:18, Andrzej Pietrasiewicz
<andrzej.p@...labora.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
Powered by blists - more mailing lists