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

[Boris Brezillon <boris.brezillon@collabora.com>]

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,
>  };
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel