Re: [PATCHv5 34/34] drm/rockchip: Add support for afbc

On Tue, 17 Dec 2019 15:50:20 +0100 Andrzej Pietrasiewicz andrzej.p@collabora.com wrote:

static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {

• .fb_create = drm_gem_fb_create_with_dirty,

• .fb_create = rockchip_fb_create, .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit,

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index d04b3492bdac..ffe1e4b9a9ca 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -91,9 +91,21 @@ #define VOP_WIN_TO_INDEX(vop_win) \ ((vop_win) - (vop_win)->vop->win)

+#define VOP_AFBC_SET(vop, name, v) \

• do { \

• if ((vop)->data->afbc) \
  

• 	vop_reg_set((vop), &(vop)->data->afbc->name, 0, ~0, v, #name); \
  

• } while (0)

#define to_vop(x) container_of(x, struct vop, crtc) #define to_vop_win(x) container_of(x, struct vop_win, base)

+#define AFBC_FMT_RGB565 0x0 +#define AFBC_FMT_U8U8U8U8 0x5 +#define AFBC_FMT_U8U8U8 0x4

+#define AFBC_TILE_16x16 BIT(4)

/*

• The coefficients of the following matrix are all fixed points.
• The format is S2.10 for the 3x3 part of the matrix, and S9.12 for the offsets.

@@ -166,6 +178,7 @@ struct vop { /* optional internal rgb encoder */ struct rockchip_rgb *rgb;

• struct vop_win *afbc_win;

Looks like something that should be in the VOP specialized crtc_state. Also, do we really need a vop_win pointer or can it be replaced by an index directly?

struct vop_win win[]; };

@@ -274,6 +287,29 @@ static enum vop_data_format vop_convert_format(uint32_t format) } }

+static int vop_convert_afbc_format(uint32_t format) +{

• switch (format) {
• case DRM_FORMAT_XRGB8888:
• case DRM_FORMAT_ARGB8888:
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_ABGR8888:

• return AFBC_FMT_U8U8U8U8;
  

• case DRM_FORMAT_RGB888:
• case DRM_FORMAT_BGR888:

• return AFBC_FMT_U8U8U8;
  

• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_BGR565:

• return AFBC_FMT_RGB565;
  

• /* either of the below should not be reachable */
• default:

• DRM_WARN_ONCE("unsupported AFBC format[%08x]\n", format);
  

• return -EINVAL;
  

• }
• return -EINVAL;

+}

static uint16_t scl_vop_cal_scale(enum scale_mode mode, uint32_t src, uint32_t dst, bool is_horizontal, int vsu_mode, int *vskiplines) @@ -598,6 +634,15 @@ static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) vop_win_disable(vop, vop_win); } }

• if (vop->data->afbc) {

• /*
  

•  * Disable AFBC and forget there was a vop window with AFBC
  

•  */
  

• VOP_AFBC_SET(vop, enable, 0);
  

• vop->afbc_win = NULL;
  

• }

• spin_unlock(&vop->reg_lock);

  vop_cfg_done(vop);

@@ -710,6 +755,37 @@ static void vop_plane_destroy(struct drm_plane *plane) drm_plane_cleanup(plane); }

+static bool rockchip_afbc(u64 modifier) +{

• return modifier == DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE);

+}

+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 (!rockchip_afbc(modifier)) {

• 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;
  

• }

Should we have a call to vop_convert_afbc_format() to make sure AFBC can be applied to this specific format?

• return true;

+}

static int vop_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) { @@ -719,10 +795,8 @@ static int vop_plane_atomic_check(struct drm_plane *plane, struct vop_win *vop_win = to_vop_win(plane); const struct vop_win_data *win = vop_win->data; int ret;

• int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :

• 			DRM_PLANE_HELPER_NO_SCALING;
  

• int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :

• 			DRM_PLANE_HELPER_NO_SCALING;
  

• int min_scale = win->phy->scl ? FRAC_16_16(1, 8) : DRM_PLANE_HELPER_NO_SCALING;
• int max_scale = win->phy->scl ? FRAC_16_16(8, 1) : DRM_PLANE_HELPER_NO_SCALING;

Looks like those coding-style fixes have nothing to do in this patch :).

if (!crtc || !fb) return 0; @@ -731,9 +805,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane, if (WARN_ON(!crtc_state)) return -EINVAL;

• ret = drm_atomic_helper_check_plane_state(state, crtc_state,

• 				  min_scale, max_scale,
  

• 				  true, true);
  

• ret = drm_atomic_helper_check_plane_state(state, crtc_state, min_scale, max_scale, true, true);

Same here, plus, following the 80 char rule when it doesn't hurt readability is a good thing I think.

if (ret) return ret;

@@ -758,6 +830,31 @@ static int vop_plane_atomic_check(struct drm_plane *plane, return -EINVAL; }

• if (rockchip_afbc(fb->modifier)) {

• 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]);
  

Why splitting that in 2 error messages?

• 	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);
  

Ditto.

• 	return -EINVAL;
  

• }
  

• }
• return 0;

}

@@ -773,6 +870,11 @@ static void vop_plane_atomic_disable(struct drm_plane *plane, spin_lock(&vop->reg_lock);

vop_win_disable(vop, vop_win);

• /*

• * Forget about the AFBC window if it is being disabled
  

• */
  

• if (vop_win == vop->afbc_win)

• vop->afbc_win = NULL;
  
  

  spin_unlock(&vop->reg_lock);

} @@ -812,6 +914,13 @@ static void vop_plane_atomic_update(struct drm_plane *plane, if (WARN_ON(!vop->is_enabled)) return;

• /*

• * If updating the AFBC window then assume that
  

• * after the update there will be no AFBC window.
  

• */
  

• if (vop_win == vop->afbc_win)

• vop->afbc_win = NULL;
  

• if (!state->visible) { vop_plane_atomic_disable(plane, old_state); return;

@@ -846,6 +955,21 @@ static void vop_plane_atomic_update(struct drm_plane *plane,

spin_lock(&vop->reg_lock);

• if (rockchip_afbc(fb->modifier)) {

• int afbc_format = vop_convert_afbc_format(fb->format->format);
  

• VOP_AFBC_SET(vop, format, afbc_format | AFBC_TILE_16x16);
  

• VOP_AFBC_SET(vop, hreg_block_split, 0);
  

• VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
  

IIUC, only one plane can use AFBC at a given time, but you don't seem to check the value of vop->afbc_win in your atomic_check. What happens if the user tries to enable this modifier on 2+ planes?

• 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;
  

• }
• 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);

@@ -1001,6 +1125,7 @@ static const struct drm_plane_funcs vop_plane_funcs = { .reset = drm_atomic_helper_plane_reset, .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,

• .format_mod_supported = rockchip_mod_supported,

};