Re: [PATCH v2] drm/arm/malidp: Validate rotations for compressed/uncompressed framebuffers for each layer

[Ayan Halder <Ayan.Halder@arm.com>]

Hi Liviu,

On Fri, Sep 21, 2018 at 03:33:53PM +0100, Liviu Dudau wrote:
> From: Ayan Kumar Halder <ayan.halder@arm.com>
> 
> Add support for compressed framebuffers that are described using
> the framebuffer's modifier field. Mali DP uses the rotation memory for
> the decompressor of the format, so we need to check for space when
> the modifiers are present.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> [re-worded commit, rebased, cleaned up duplicated checks for
>  RGB888 and BGR888 and removed additional parameter for
>  rotmem_required function hook]
> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_crtc.c   | 28 ++++++++++++---------
>  drivers/gpu/drm/arm/malidp_hw.c     | 38 ++++++++++++-----------------
>  drivers/gpu/drm/arm/malidp_hw.h     |  7 ++++++
>  drivers/gpu/drm/arm/malidp_planes.c | 19 +++++++++++----
>  4 files changed, 53 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> index ef44202fb43f8..e1b72782848c3 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -348,19 +348,20 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  	/*
>  	 * check if there is enough rotation memory available for planes
> -	 * that need 90?? and 270?? rotation. Each plane has set its required
> -	 * memory size in the ->plane_check() callback, here we only make
> -	 * sure that the sums are less that the total usable memory.
> +	 * that need 90?? and 270?? rotion or planes that are compressed.
> +	 * Each plane has set its required memory size in the ->plane_check()
> +	 * callback, here we only make sure that the sums are less that the
> +	 * total usable memory.
>  	 *
>  	 * The rotation memory allocation algorithm (for each plane):
> -	 *  a. If no more rotated planes exist, all remaining rotate
> -	 *     memory in the bank is available for use by the plane.
> -	 *  b. If other rotated planes exist, and plane's layer ID is
> -	 *     DE_VIDEO1, it can use all the memory from first bank if
> -	 *     secondary rotation memory bank is available, otherwise it can
> +	 *  a. If no more rotated or compressed planes exist, all remaining
> +	 *     rotate memory in the bank is available for use by the plane.
> +	 *  b. If other rotated or compressed planes exist, and plane's
> +	 *     layer ID is DE_VIDEO1, it can use all the memory from first bank
> +	 *     if secondary rotation memory bank is available, otherwise it can
>  	 *     use up to half the bank's memory.
> -	 *  c. If other rotated planes exist, and plane's layer ID is not
> -	 *     DE_VIDEO1, it can use half of the available memory
> +	 *  c. If other rotated or compressed planes exist, and plane's layer ID
> +	 *     is not DE_VIDEO1, it can use half of the available memory.
>  	 *
>  	 * Note: this algorithm assumes that the order in which the planes are
>  	 * checked always has DE_VIDEO1 plane first in the list if it is
> @@ -372,7 +373,9 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  	/* first count the number of rotated planes */
>  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
> -		if (pstate->rotation & MALIDP_ROTATED_MASK)
> +		struct drm_framebuffer *fb = pstate->fb;
> +
> +		if ((pstate->rotation & MALIDP_ROTATED_MASK) || fb->modifier)
>  			rotated_planes++;
>  	}
>  
> @@ -388,8 +391,9 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
>  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
>  		struct malidp_plane *mp = to_malidp_plane(plane);
>  		struct malidp_plane_state *ms = to_malidp_plane_state(pstate);
> +		struct drm_framebuffer *fb = pstate->fb;
>  
> -		if (pstate->rotation & MALIDP_ROTATED_MASK) {
> +		if ((pstate->rotation & MALIDP_ROTATED_MASK) || fb->modifier) {
>  			/* process current plane */
>  			rotated_planes--;
>  
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index b33000634a4ee..5549092e6c36a 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -85,41 +85,43 @@ static const struct malidp_format_id malidp550_de_formats[] = {
>  
>  static const struct malidp_layer malidp500_layers[] = {
>  	/*  id, base address, fb pointer address base, stride offset,
> -		yuv2rgb matrix offset, mmu control register offset */
> +	    yuv2rgb matrix offset, mmu control register offset, rotation_features */
>  	{ DE_VIDEO1, MALIDP500_DE_LV_BASE, MALIDP500_DE_LV_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, 0 },
> +		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, 0, ROTATE_ANY },
>  	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, 0 },
> +		MALIDP_DE_LG_STRIDE, 0, 0, ROTATE_ANY },
>  	{ DE_GRAPHICS2, MALIDP500_DE_LG2_BASE, MALIDP500_DE_LG2_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, 0 },
> +		MALIDP_DE_LG_STRIDE, 0, 0, ROTATE_ANY },
>  };
>  
>  static const struct malidp_layer malidp550_layers[] = {
>  	/*  id, base address, fb pointer address base, stride offset,
> -		yuv2rgb matrix offset, mmu control register offset */
> +		yuv2rgb matrix offset, mmu control register offset, rotation_features */
>  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, 0 },
> +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, 0, ROTATE_ANY },
>  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, 0 },
> +		MALIDP_DE_LG_STRIDE, 0, 0, ROTATE_ANY },
>  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, 0 },
> +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, 0, ROTATE_ANY },
>  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> -		MALIDP550_DE_LS_R1_STRIDE, 0, 0 },
> +		MALIDP550_DE_LS_R1_STRIDE, 0, 0, ROTATE_NONE },
>  };
>  
>  static const struct malidp_layer malidp650_layers[] = {
>  	/*  id, base address, fb pointer address base, stride offset,
> -		yuv2rgb matrix offset, mmu control register offset */
> +		yuv2rgb matrix offset, mmu control register offset, rotation_features */
>  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
>  		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB,
> -		MALIDP650_DE_LV_MMU_CTRL },
> +		MALIDP650_DE_LV_MMU_CTRL, ROTATE_ANY },
>  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, MALIDP650_DE_LG_MMU_CTRL },
> +		MALIDP_DE_LG_STRIDE, 0, MALIDP650_DE_LG_MMU_CTRL,
> +		ROTATE_COMPRESSED },
>  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
>  		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB,
> -		MALIDP650_DE_LV_MMU_CTRL },
> +		MALIDP650_DE_LV_MMU_CTRL, ROTATE_ANY },
>  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> -		MALIDP550_DE_LS_R1_STRIDE, 0, MALIDP650_DE_LS_MMU_CTRL },
> +		MALIDP550_DE_LS_R1_STRIDE, 0, MALIDP650_DE_LS_MMU_CTRL,
> +		ROTATE_NONE },
>  };
>  
>  #define SE_N_SCALING_COEFFS	96
> @@ -314,10 +316,6 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
>  
>  static int malidp500_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt)
>  {
> -	/* RGB888 or BGR888 can't be rotated */
> -	if ((fmt == DRM_FORMAT_RGB888) || (fmt == DRM_FORMAT_BGR888))
> -		return -EINVAL;
> -
>  	/*
>  	 * Each layer needs enough rotation memory to fit 8 lines
>  	 * worth of pixel data. Required size is then:
> @@ -605,10 +603,6 @@ static int malidp550_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
>  {
>  	u32 bytes_per_col;
>  
> -	/* raw RGB888 or BGR888 can't be rotated */
> -	if ((fmt == DRM_FORMAT_RGB888) || (fmt == DRM_FORMAT_BGR888))
> -		return -EINVAL;
> -
>  	switch (fmt) {
>  	/* 8 lines at 4 bytes per pixel */
>  	case DRM_FORMAT_ARGB2101010:
> diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
> index 0d7f9ea0ade89..3ab133d49bbad 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.h
> +++ b/drivers/gpu/drm/arm/malidp_hw.h
> @@ -36,6 +36,12 @@ enum {
>  	SE_MEMWRITE = BIT(5),
>  };
>  
> +enum rotation_features {
> +	ROTATE_NONE,		/* does not support rotation at all */
> +	ROTATE_ANY,		/* supports rotation on any buffers */
> +	ROTATE_COMPRESSED,	/* supports rotation only on compressed buffers */
> +};
> +
>  struct malidp_format_id {
>  	u32 format;		/* DRM fourcc */
>  	u8 layer;		/* bitmask of layers supporting it */
> @@ -63,6 +69,7 @@ struct malidp_layer {
>  	u16 stride_offset;	/* offset to the first stride register. */
>  	s16 yuv2rgb_offset;	/* offset to the YUV->RGB matrix entries */
>  	u16 mmu_ctrl_offset;    /* offset to the MMU control register */
> +	enum rotation_features rot;	/* type of rotation supported */
>  };
>  
>  enum malidp_scaling_coeff_set {
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index c21b44effa5a7..36d2952774b22 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -433,11 +433,20 @@ static int malidp_de_plane_check(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	/* packed RGB888 / BGR888 can't be rotated or flipped */
> -	if (state->rotation != DRM_MODE_ROTATE_0 &&
> -	    (fb->format->format == DRM_FORMAT_RGB888 ||
> -	     fb->format->format == DRM_FORMAT_BGR888))
> -		return -EINVAL;
> +	/* validate the rotation constraints for each layer */
> +	if (state->rotation != DRM_MODE_ROTATE_0) {
> +		if (mp->layer->rot == ROTATE_NONE)
> +			return -EINVAL;
> +		if ((mp->layer->rot == ROTATE_COMPRESSED) && !!(fb->modifier))
This should be !(fb->modifier) because the driver should return EINVAL
when the layer supports only compressed rotation and no framebuffer
modifiers (which denotes compression) have been provided.
> +			return -EINVAL;
> +		/*
> +		 * packed RGB888 / BGR888 can't be rotated or flipped
> +		 * unless they are stored in a compressed way
> +		 */
> +		if ((fb->format->format == DRM_FORMAT_RGB888 ||
> +		     fb->format->format == DRM_FORMAT_BGR888) && !!(fb->modifier))
This should also be !(fb->modifier)
> +			return -EINVAL;
> +	}
>  
>  	ms->rotmem_size = 0;
>  	if (state->rotation & MALIDP_ROTATED_MASK) {
> -- 
> 2.18.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel