[PATCH] drm/msm/dpu: enable cursor plane on dpu

Sean Paul seanpaul at chromium.org
Fri Aug 3 15:37:56 UTC 2018


On Thu, Aug 02, 2018 at 04:24:44PM +0530, Sravanthi Kollukuduru wrote:
> Reserve DMA pipe for cursor plane and attach it to the
> crtc during the initialization.
> 
> Signed-off-by: Sravanthi Kollukuduru <skolluku at codeaurora.org>

Hi Sravanthi,
Thanks for sending this. I have a few comments, mostly based off my own cursor
patch [1] I posted last week. It's mostly the same as what you have here, but
takes a couple different things into consideration.

[1]-
https://gitlab.freedesktop.org/seanpaul/dpu-staging/commit/fde9f11f8f3be3ceaacfd0751e250a3c03476a8c

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c       |  5 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h       |  4 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 53 +++++++++++---------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 35 +++++++++++------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c      |  9 +----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h      |  4 +-
>  6 files changed, 55 insertions(+), 55 deletions(-)
> 

/snip

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 8d4678d..dc647ee 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -531,12 +531,13 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  {
>  	struct drm_device *dev;
>  	struct drm_plane *primary_planes[MAX_PLANES], *plane;
> +	struct drm_plane *cursor_planes[MAX_PLANES] = { NULL };
>  	struct drm_crtc *crtc;
>  
>  	struct msm_drm_private *priv;
>  	struct dpu_mdss_cfg *catalog;
>  
> -	int primary_planes_idx = 0, i, ret;
> +	int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>  	int max_crtc_count;
>  
>  	if (!dpu_kms || !dpu_kms->dev || !dpu_kms->dev->dev) {
> @@ -556,16 +557,24 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  
>  	max_crtc_count = min(catalog->mixer_count, priv->num_encoders);
>  
> -	/* Create the planes */
> +	/* Create the planes, keeping track of one primary/cursor per crtc */
>  	for (i = 0; i < catalog->sspp_count; i++) {
> -		bool primary = true;
> -
> -		if (catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)
> -			|| primary_planes_idx >= max_crtc_count)
> -			primary = false;
> -
> -		plane = dpu_plane_init(dev, catalog->sspp[i].id, primary,
> -				(1UL << max_crtc_count) - 1, 0);
> +		enum drm_plane_type type;
> +
> +		if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR))
> +			&& cursor_planes_idx < max_crtc_count)

You don't need to check that the index is less than max_crtc_count, it'll fit in
the array regardless.

> +			type = DRM_PLANE_TYPE_CURSOR;
> +		else if (primary_planes_idx < max_crtc_count)
> +			type = DRM_PLANE_TYPE_PRIMARY;
> +		else
> +			type = DRM_PLANE_TYPE_OVERLAY;
> +
> +		DPU_DEBUG("Create plane type %d with features %lx (cur %lx)\n",
> +			  type, catalog->sspp[i].features,
> +			  catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR));
> +
> +		plane = dpu_plane_init(dev, catalog->sspp[i].id, type,
> +				       (1UL << max_crtc_count) - 1, 0);
>  		if (IS_ERR(plane)) {
>  			DPU_ERROR("dpu_plane_init failed\n");
>  			ret = PTR_ERR(plane);
> @@ -573,7 +582,9 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  		}
>  		priv->planes[priv->num_planes++] = plane;
>  
> -		if (primary)
> +		if (type == DRM_PLANE_TYPE_CURSOR)
> +			cursor_planes[cursor_planes_idx++] = plane;
> +		else if (type == DRM_PLANE_TYPE_PRIMARY)
>  			primary_planes[primary_planes_idx++] = plane;
>  	}
>  
> @@ -581,7 +592,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  
>  	/* Create one CRTC per encoder */
>  	for (i = 0; i < max_crtc_count; i++) {
> -		crtc = dpu_crtc_init(dev, primary_planes[i]);
> +		crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);

Here you assume that there is at least one cursor per crtc. That might not be
the case. Check out how I handle this in my patch with max_cursor_planes, it's
a bit more safe than this.



>  		if (IS_ERR(crtc)) {
>  			ret = PTR_ERR(crtc);
>  			goto fail;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index b640e39..efdf9b2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1827,7 +1827,7 @@ bool is_dpu_plane_virtual(struct drm_plane *plane)
>  
>  /* initialize plane */
>  struct drm_plane *dpu_plane_init(struct drm_device *dev,
> -		uint32_t pipe, bool primary_plane,
> +		uint32_t pipe, enum drm_plane_type type,
>  		unsigned long possible_crtcs, u32 master_plane_id)
>  {
>  	struct drm_plane *plane = NULL, *master_plane = NULL;
> @@ -1835,7 +1835,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>  	struct dpu_plane *pdpu;
>  	struct msm_drm_private *priv;
>  	struct dpu_kms *kms;
> -	enum drm_plane_type type;
>  	int zpos_max = DPU_ZPOS_MAX;
>  	int ret = -EINVAL;
>  
> @@ -1916,12 +1915,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>  		goto clean_sspp;
>  	}
>  
> -	if (pdpu->features & BIT(DPU_SSPP_CURSOR))
> -		type = DRM_PLANE_TYPE_CURSOR;
> -	else if (primary_plane)
> -		type = DRM_PLANE_TYPE_PRIMARY;
> -	else
> -		type = DRM_PLANE_TYPE_OVERLAY;
>  	ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs,
>  				pdpu->formats, pdpu->nformats,
>  				NULL, type, NULL);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> index f6fe6dd..7fed0b6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> @@ -122,7 +122,7 @@ void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl,
>   * dpu_plane_init - create new dpu plane for the given pipe
>   * @dev:   Pointer to DRM device
>   * @pipe:  dpu hardware pipe identifier
> - * @primary_plane: true if this pipe is primary plane for crtc
> + * @type:  Plane type - PRIMARY/OVERLAY/CURSOR
>   * @possible_crtcs: bitmask of crtc that can be attached to the given pipe
>   * @master_plane_id: primary plane id of a multirect pipe. 0 value passed for
>   *                   a regular plane initialization. A non-zero primary plane
> @@ -130,7 +130,7 @@ void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl,
>   *
>   */
>  struct drm_plane *dpu_plane_init(struct drm_device *dev,
> -		uint32_t pipe, bool primary_plane,
> +		uint32_t pipe, enum drm_plane_type type,
>  		unsigned long possible_crtcs, u32 master_plane_id);
>  
>  /**
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS


More information about the dri-devel mailing list