Re: [RESEND 3/3] drm :fsl-dcu: Add multi layers support

[Stefan Agner <stefan@agner.ch>]

Hi Dongsheng,

On 2015-12-01 00:16, Dongsheng Wang wrote:
> From: Jianwei Wang <jianwei.wang.chn@gmail.com>
> 
> For DCU support atmost 16 layers(on ls1021a) or 64 layers(on vf610),
> add (total_layer - 1) overlay planes.
> 
> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
> Signed-off-by: Yi Meng <b56799@freescale.com>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> index a8932a8..7ed1a7e 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> @@ -235,7 +235,10 @@ static const u32 fsl_dcu_drm_plane_formats[] = {
>  
>  struct drm_plane *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>  {

It feels wrong to me to add multi layer support in a function called
fsl_dcu_drm_primary_create_plane...

I suggest to either rename the function, or better create a new function
for the overlay planes. The only shared variable is formats_size, which
is anyway statically determined by the compiler.

--
Stefan

> +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
>  	struct drm_plane *primary;
> +	struct drm_plane *overlay;
> +	unsigned int total_layer, formats_size, i;
>  	int ret;
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> @@ -244,11 +247,12 @@ struct drm_plane
> *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>  		return NULL;
>  	}
>  
> +	formats_size = ARRAY_SIZE(fsl_dcu_drm_plane_formats);
>  	/* possible_crtc's will be filled in later by crtc_init */
>  	ret = drm_universal_plane_init(dev, primary, 0,
>  				       &fsl_dcu_drm_plane_funcs,
>  				       fsl_dcu_drm_plane_formats,
> -				       ARRAY_SIZE(fsl_dcu_drm_plane_formats),
> +				       formats_size,
>  				       DRM_PLANE_TYPE_PRIMARY);
>  	if (ret) {
>  		kfree(primary);
> @@ -256,5 +260,26 @@ struct drm_plane
> *fsl_dcu_drm_primary_create_plane(struct drm_device *dev)
>  	}
>  	drm_plane_helper_add(primary, &fsl_dcu_drm_plane_helper_funcs);
>  
> +	total_layer = fsl_dev->soc->total_layer;
> +	for (i = 0; i < total_layer - 1; i++) {
> +		overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> +		if (!overlay) {
> +			DRM_DEBUG_KMS("Failed to allocate overlay plane\n");
> +			goto out;
> +		}
> +
> +		ret = drm_universal_plane_init(dev, overlay, 1,
> +					       &fsl_dcu_drm_plane_funcs,
> +					       fsl_dcu_drm_plane_formats,
> +					       formats_size,
> +					       DRM_PLANE_TYPE_OVERLAY);
> +		if (ret) {
> +			kfree(overlay);

You basically allow to initialize only some layers, which is probably
fine. But I think we should add at least an error message here... 

--
Stefan

> +			goto out;
> +		}
> +		drm_plane_helper_add(overlay, &fsl_dcu_drm_plane_helper_funcs);
> +	}
> +
> +out:
>  	return primary;
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel