[PATCH] drm/rockchip: Add AFBC support
Brian Starkey
Brian.Starkey at arm.com
Wed Sep 25 09:39:15 UTC 2019
Hi Andrzej,
Thanks for the patch, it's nice to see another AFBC implementation
coming in.
For future versions, could you please Cc ayan.halder at arm.com? It would
have been nice to have someone @arm.com on patches which use/impact
Arm modifiers. Sadly I don't know how to make get_maintainer.pl help
with that.
Some more comments below.
On Mon, Sep 23, 2019 at 02:20:13PM +0200, Andrzej Pietrasiewicz wrote:
> From: Ezequiel Garcia <ezequiel at collabora.com>
>
> AFBC is a proprietary lossless image compression protocol and format.
> It helps reduce memory bandwidth of the graphics pipeline operations.
> This, in turn, improves power efficiency.
>
> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> [locking improvements]
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> [squashing the above, commit message and Rockchip AFBC modifier]
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p at collabora.com>
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27 ++++++
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 94 ++++++++++++++++++++-
> drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 12 +++
> drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 18 ++++
> include/uapi/drm/drm_fourcc.h | 3 +
> 5 files changed, 151 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 64ca87cf6d50..5178939a9c29 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -24,6 +24,27 @@ static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> .dirty = drm_atomic_helper_dirtyfb,
> };
>
> +static int
> +rockchip_verify_afbc_mod(struct drm_device *dev,
> + const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> + if (mode_cmd->modifier[0] &
> + ~DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP)) {
> + DRM_DEV_ERROR(dev->dev,
> + "Unsupported format modifier 0x%llx\n",
> + mode_cmd->modifier[0]);
> + return -EINVAL;
> + }
> +
> + if (mode_cmd->width > 2560) {
> + DRM_DEV_ERROR(dev->dev,
> + "Unsupported width %d\n", mode_cmd->width);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static struct drm_framebuffer *
> rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd,
> struct drm_gem_object **obj, unsigned int num_planes)
> @@ -32,6 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
> int ret;
> int i;
>
> + if (mode_cmd->modifier[0]) {
> + ret = rockchip_verify_afbc_mod(dev, mode_cmd);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> +
> fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> if (!fb)
> return ERR_PTR(-ENOMEM);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 21b68eea46cc..50bf214d21da 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -46,6 +46,13 @@
> vop_reg_set(vop, &win->phy->scl->ext->name, \
> win->base, ~0, v, #name)
>
> +#define VOP_AFBC_SET(x, name, v) \
> + do { \
> + if (vop->data->afbc) \
> + vop_reg_set(vop, &vop->data->afbc->name, \
> + 0, ~0, v, #name); \
> + } while (0)
> +
> #define VOP_WIN_YUV2YUV_SET(vop, win_yuv2yuv, name, v) \
> do { \
> if (win_yuv2yuv && win_yuv2yuv->name.mask) \
> @@ -123,6 +130,8 @@ struct vop {
> struct drm_device *drm_dev;
> bool is_enabled;
>
> + struct vop_win *afbc_win;
> +
> struct completion dsp_hold_completion;
>
> /* protected by dev->event_lock */
> @@ -245,6 +254,30 @@ static bool has_rb_swapped(uint32_t format)
> }
> }
>
> +#define AFBC_FMT_RGB565 0x0
> +#define AFBC_FMT_U8U8U8U8 0x5
> +#define AFBC_FMT_U8U8U8 0x4
> +
> +static int vop_convert_afbc_format(uint32_t format)
> +{
It would be great if you are able to follow the guidance Arm published
here: https://www.kernel.org/doc/html/latest/gpu/afbc.html, which will
help ensure interoperability and compatibility between different
devices/drivers. Hopefully it doesn't limit some use-cases for you -
if it does, let's discuss them.
Specifically, please take a look at the format list there. Some of
your formats below are not on the preferred interop list:
> + switch (format) {
> + case DRM_FORMAT_XRGB8888:
XRGB8888: Not preferred, as encoding the X channel is a waste of bits
> + case DRM_FORMAT_ARGB8888:
ARGB8888: Not preferred as the channel order prevents YTR
> + case DRM_FORMAT_XBGR8888:
XBGR8888: Not preferred, as encoding the X channel is a waste of bits
> + case DRM_FORMAT_ABGR8888:
> + return AFBC_FMT_U8U8U8U8;
> + case DRM_FORMAT_RGB888:
RGB888: Not preferred as the channel order prevents YTR
> + case DRM_FORMAT_BGR888:
> + return AFBC_FMT_U8U8U8;
> + case DRM_FORMAT_RGB565:
RGB565: Not preferred as the channel order prevents YTR
> + case DRM_FORMAT_BGR565:
> + return AFBC_FMT_RGB565;
> + default:
> + DRM_ERROR("unsupported afbc format[%08x]\n", format);
> + return -EINVAL;
> + }
> +}
> +
> static enum vop_data_format vop_convert_format(uint32_t format)
> {
> switch (format) {
> @@ -587,10 +620,16 @@ static int vop_enable(struct drm_crtc *crtc)
>
> vop_win_disable(vop, win);
> }
> - spin_unlock(&vop->reg_lock);
> +
> + if (vop->data->afbc) {
> + VOP_AFBC_SET(vop, enable, 0);
> + vop->afbc_win = NULL;
> + }
>
> vop_cfg_done(vop);
>
> + spin_unlock(&vop->reg_lock);
> +
> /*
> * At here, vop clock & iommu is enable, R/W vop regs would be safe.
> */
> @@ -719,6 +758,32 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
> return -EINVAL;
> }
>
> + if (fb->modifier & DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP)) {
> + struct vop *vop = to_vop(crtc);
> +
> + if (!vop->data->afbc) {
> + DRM_ERROR("VOP does not support AFBC\n");
> + return -EINVAL;
> + }
> +
> + ret = vop_convert_afbc_format(fb->format->format);
> + if (ret < 0)
> + return ret;
> +
> + if (state->src.x1 || state->src.y1) {
> + DRM_ERROR("AFBC does not support offset display\n");
> + DRM_ERROR("xpos=%d, ypos=%d, offset=%d\n",
> + state->src.x1, state->src.y1, fb->offsets[0]);
> + return -EINVAL;
> + }
> +
> + if (state->rotation && state->rotation != DRM_MODE_ROTATE_0) {
> + DRM_ERROR("AFBC does not support rotation\n");
> + DRM_ERROR("rotation=%d\n", state->rotation);
> + return -EINVAL;
> + }
It may be a good idea to check your framebuffer size, as the required
framebuffer size is different for AFBC. You can refer to mali-dp for
the calculations - perhaps even share the code.
> + }
> +
> return 0;
> }
>
> @@ -732,6 +797,9 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
> if (!old_state->crtc)
> return;
>
> + if (vop->afbc_win == vop_win)
> + vop->afbc_win = NULL;
> +
> spin_lock(&vop->reg_lock);
>
> vop_win_disable(vop, win);
> @@ -774,6 +842,9 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
> if (WARN_ON(!vop->is_enabled))
> return;
>
> + if (vop->afbc_win == vop_win)
> + vop->afbc_win = NULL;
> +
> if (!state->visible) {
> vop_plane_atomic_disable(plane, old_state);
> return;
> @@ -808,6 +879,20 @@ 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_ROCKCHIP)) {
> + int afbc_format = vop_convert_afbc_format(fb->format->format);
> +
> + VOP_AFBC_SET(vop, format, afbc_format | 1 << 4);
> + VOP_AFBC_SET(vop, hreg_block_split, 0);
> + VOP_AFBC_SET(vop, win_sel, win_index);
> + VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
> + VOP_AFBC_SET(vop, pic_size, act_info);
> +
> + vop->afbc_win = vop_win;
> +
> + pr_info("AFBC on plane %s\n", plane->name);
> + }
> +
> VOP_WIN_SET(vop, win, format, format);
> VOP_WIN_SET(vop, win, yrgb_vir, DIV_ROUND_UP(fb->pitches[0], 4));
> VOP_WIN_SET(vop, win, yrgb_mst, dma_addr);
> @@ -1163,6 +1248,7 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
>
> spin_lock(&vop->reg_lock);
>
> + VOP_AFBC_SET(vop, enable, vop->afbc_win ? 0x1 : 0);
> vop_cfg_done(vop);
>
> spin_unlock(&vop->reg_lock);
> @@ -1471,7 +1557,8 @@ static int vop_create_crtc(struct vop *vop)
> 0, &vop_plane_funcs,
> win_data->phy->data_formats,
> win_data->phy->nformats,
> - NULL, win_data->type, NULL);
> + win_data->phy->format_modifiers,
> + win_data->type, NULL);
> if (ret) {
> DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
> ret);
> @@ -1511,7 +1598,8 @@ static int vop_create_crtc(struct vop *vop)
> &vop_plane_funcs,
> win_data->phy->data_formats,
> win_data->phy->nformats,
> - NULL, win_data->type, NULL);
> + win_data->phy->format_modifiers,
> + win_data->type, NULL);
> if (ret) {
> DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
> ret);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 2149a889c29d..dc8b12025269 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -77,6 +77,16 @@ struct vop_misc {
> struct vop_reg global_regdone_en;
> };
>
> +struct vop_afbc {
> + struct vop_reg enable;
> + struct vop_reg win_sel;
> + struct vop_reg format;
> + struct vop_reg hreg_block_split;
> + struct vop_reg pic_size;
> + struct vop_reg hdr_ptr;
> + struct vop_reg rstn;
> +};
> +
> struct vop_intr {
> const int *intrs;
> uint32_t nintrs;
> @@ -128,6 +138,7 @@ struct vop_win_phy {
> const struct vop_scl_regs *scl;
> const uint32_t *data_formats;
> uint32_t nformats;
> + const uint64_t *format_modifiers;
>
> struct vop_reg enable;
> struct vop_reg gate;
> @@ -169,6 +180,7 @@ struct vop_data {
> const struct vop_output *output;
> const struct vop_win_yuv2yuv_data *win_yuv2yuv;
> const struct vop_win_data *win;
> + const struct vop_afbc *afbc;
> unsigned int win_size;
>
> #define VOP_FEATURE_OUTPUT_RGB10 BIT(0)
> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> index 7b9c74750f6d..e9ff0c43c396 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> @@ -30,6 +30,12 @@
> #define VOP_REG_MASK_SYNC(off, _mask, _shift) \
> _VOP_REG(off, _mask, _shift, true, false)
>
> +static const uint64_t format_modifiers_afbc[] = {
> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP),
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> +};
> +
> static const uint32_t formats_win_full[] = {
> DRM_FORMAT_XRGB8888,
> DRM_FORMAT_ARGB8888,
> @@ -667,6 +673,7 @@ static const struct vop_win_phy rk3368_win01_data = {
> .scl = &rk3288_win_full_scl,
> .data_formats = formats_win_full,
> .nformats = ARRAY_SIZE(formats_win_full),
> + .format_modifiers = format_modifiers_afbc,
I may have missed something, but don't you need to implement the
format_mod_supported hook on the plane to expose the modifiers to
userspace?
> .enable = VOP_REG(RK3368_WIN0_CTRL0, 0x1, 0),
> .format = VOP_REG(RK3368_WIN0_CTRL0, 0x7, 1),
> .rb_swap = VOP_REG(RK3368_WIN0_CTRL0, 0x1, 12),
> @@ -758,6 +765,16 @@ static const struct vop_data rk3366_vop = {
> .win_size = ARRAY_SIZE(rk3368_vop_win_data),
> };
>
> +static const struct vop_afbc rk3399_afbc = {
> + .rstn = VOP_REG(RK3399_AFBCD0_CTRL, 0x1, 3),
> + .enable = VOP_REG(RK3399_AFBCD0_CTRL, 0x1, 0),
> + .win_sel = VOP_REG(RK3399_AFBCD0_CTRL, 0x3, 1),
> + .format = VOP_REG(RK3399_AFBCD0_CTRL, 0x1f, 16),
> + .hreg_block_split = VOP_REG(RK3399_AFBCD0_CTRL, 0x1, 21),
> + .hdr_ptr = VOP_REG(RK3399_AFBCD0_HDR_PTR, 0xffffffff, 0),
> + .pic_size = VOP_REG(RK3399_AFBCD0_PIC_SIZE, 0xffffffff, 0),
> +};
> +
> static const struct vop_output rk3399_output = {
> .dp_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 16),
> .rgb_pin_pol = VOP_REG(RK3368_DSP_CTRL1, 0xf, 16),
> @@ -808,6 +825,7 @@ static const struct vop_data rk3399_vop_big = {
> .modeset = &rk3288_modeset,
> .output = &rk3399_output,
> .misc = &rk3368_misc,
> + .afbc = &rk3399_afbc,
> .win = rk3368_vop_win_data,
> .win_size = ARRAY_SIZE(rk3368_vop_win_data),
> .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data,
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 3feeaa3f987a..ba6caf06c824 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -742,6 +742,9 @@ extern "C" {
> */
> #define AFBC_FORMAT_MOD_BCH (1ULL << 11)
>
> +#define AFBC_FORMAT_MOD_ROCKCHIP \
> + (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE)
> +
As Neil said, this seems redundant. If you would like the convenience
macro, stick it in the Rockchip driver rather than UAPI.
Also, if your hardware is able to support YTR, I would recommend
enabling it.
Best regards,
-Brian
> /*
> * Allwinner tiled modifier
> *
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list