[PATCHv3/RFC 4/4] drm/rockchip: Add support for afbc

[Ezequiel Garcia]

Hello Andrzej,

Thanks a lot for the patch.

Reviewed-by: Ezequiel Garcia <ezequiel at collabora.com>

On Thu, 2019-11-21 at 18:22 +0100, Andrzej Pietrasiewicz wrote:
> This patch adds support for afbc handling. afbc is a compressed format
> which reduces the necessary memory bandwidth.
> 
> Co-developed-by: Mark Yao <mark.yao at rock-chips.com>
> Signed-off-by: Mark Yao <mark.yao at rock-chips.com>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p at collabora.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  29 ++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 142 +++++++++++++++++++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  12 ++
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |  84 +++++++++++-
>  4 files changed, 263 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..7eaa3fdc03b2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -8,6 +8,7 @@
>  
>  #include <drm/drm.h>
>  #include <drm/drm_atomic.h>
> +#include <drm/drm_afbc.h>
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
> @@ -18,6 +19,8 @@
>  #include "rockchip_drm_fb.h"
>  #include "rockchip_drm_gem.h"
>  
> +#define ROCKCHIP_MAX_AFBC_WIDTH	2560
> +
>  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>  	.destroy       = drm_gem_fb_destroy,
>  	.create_handle = drm_gem_fb_create_handle,
> @@ -32,6 +35,32 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>  	int ret;
>  	int i;
>  
> +	if (drm_is_afbc(mode_cmd->modifier[0])) {
> +		struct drm_afbc afbc;
> +
> +		drm_afbc_get_parameters(mode_cmd, &afbc);
> +
> +		if (afbc.offset) {
> +			DRM_WARN("AFBC plane offset must be zero!\n");
> +
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (afbc.tile_w != 16 || afbc.tile_h != 16) {
> +			DRM_WARN("Unsupported afbc tile w/h [%d/%d]\n",
> +				 afbc.tile_w, afbc.tile_h);
> +

I think it's important to stick to using always "AFBC" or
always ", i.e. to be consisten in user messages.
Makes grepping easier.

[..]
> @@ -846,6 +960,23 @@ 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)) {

You check this modifier condition a few times, how about
having a helper for it?

> +		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));
> +		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 +1132,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,
>  };
>  
>  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
> @@ -1340,6 +1472,10 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	spin_lock(&vop->reg_lock);
>  
> +	/*
> +	 * Enable AFBC if there is some AFBC window, disable otherwise
> +	 */

Nitpick: no need for multi-line style comments, if the comment
is a single line. Also, you might want to end the comment with a stop.

Thanks!
Eze