Re: [PATCH 06/11] drm/msm/dpu: support cursor sspp hw blocks

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Cursor SSPP must be assigned to the last mixer stage, so we assign an
> immutable zpos property with a value higher than primary/overlay planes,
> to ensure it will always be on top.

The commit does more than is described in the commit message. Let's do 
it step by step. Please split into several patches. Also see below.

> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 19 ++++++++++++++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 26 +++++++++++++++++++++++---
>   2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 0e7a68714e9e1..6cce0f6cfcb01 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -738,13 +738,22 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   	for (i = 0; i < catalog->sspp_count; i++) {
>   		enum drm_plane_type type;
>   
> -		if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR))
> -			&& cursor_planes_idx < max_crtc_count)
> -			type = DRM_PLANE_TYPE_CURSOR;
> -		else if (primary_planes_idx < max_crtc_count)
> +		if (catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)) {
> +			if (cursor_planes_idx < max_crtc_count) {
> +				type = DRM_PLANE_TYPE_CURSOR;
> +			} else if (catalog->sspp[i].type == SSPP_TYPE_CURSOR) {
> +				/* Cursor SSPP can only be used in the last
> +				 * mixer stage, so it doesn't make sense to
> +				 * assign two of those to the same CRTC */
> +				continue;
> +			} else {
> +				type = DRM_PLANE_TYPE_OVERLAY;
> +			}
> +		} else if (primary_planes_idx < max_crtc_count) {
>   			type = DRM_PLANE_TYPE_PRIMARY;
> -		else
> +		} else {
>   			type = DRM_PLANE_TYPE_OVERLAY;
> +		}

Ack. I'm not sure how compositors will cope if we have two planes with 
immutable zpos set to the same value. Also I'd prefer to have this as a 
separate commit.

>   
>   		DPU_DEBUG("Create plane type %d with features %lx (cur %lx)\n",
>   			  type, catalog->sspp[i].features,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 128ecdc145260..5a7bb8543866c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -881,7 +881,14 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   	r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>   	r_pipe->sspp = NULL;
>   
> -	pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
> +	if (pipe_hw_caps->type == SSPP_TYPE_CURSOR) {
> +		/* enforce cursor sspp to use the last mixer stage */

I'd add here 'we know that it is the last plane in the stack because of 
zpos property ranges'

> +		pstate->stage = DPU_STAGE_BASE +
> +			pdpu->catalog->caps->max_mixer_blendstages;
> +	} else {
> +		pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
> +	}
> +
>   	if (pstate->stage > DPU_STAGE_BASE + pdpu->catalog->caps->max_mixer_blendstages) {
>   		DPU_ERROR("> %d plane mixer stages assigned\n",
>   			  pdpu->catalog->caps->max_mixer_blendstages);
> @@ -1463,6 +1470,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct dpu_kms *kms = to_dpu_kms(priv->kms);
>   	struct dpu_hw_sspp *pipe_hw;
> +	const uint64_t *format_modifiers;
>   	uint32_t num_formats;
>   	uint32_t supported_rotations;
>   	int ret = -EINVAL;
> @@ -1489,15 +1497,27 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>   	format_list = pipe_hw->cap->sblk->format_list;
>   	num_formats = pipe_hw->cap->sblk->num_formats;
>   
> +	if (pipe_hw->cap->type == SSPP_TYPE_CURSOR)
> +		format_modifiers = NULL;
> +	else
> +		format_modifiers = supported_format_modifiers;
> +
>   	ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs,
>   				format_list, num_formats,
> -				supported_format_modifiers, type, NULL);
> +				format_modifiers, type, NULL);


Separate commit please

>   	if (ret)
>   		goto clean_plane;
>   
>   	pdpu->catalog = kms->catalog;
>   
> -	ret = drm_plane_create_zpos_property(plane, 0, 0, DPU_ZPOS_MAX);
> +	if (pipe_hw->cap->type == SSPP_TYPE_CURSOR) {
> +		/* cursor SSPP can only be used in the last mixer stage,
> +		 * enforce it by maxing out the cursor plane zpos */
> +		ret = drm_plane_create_zpos_immutable_property(plane, DPU_ZPOS_MAX);
> +	} else {
> +		ret = drm_plane_create_zpos_property(plane, 0, 0, DPU_ZPOS_MAX - 1);
> +	}
> +
>   	if (ret)
>   		DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>   
> 

-- 
With best wishes
Dmitry