Re: [PATCH 2/3] drm/vkms: add support for multiple overlay planes

[Melissa Wen]

On 12/13, José Expósito wrote:
> Add a new module parameter to allow to set the number of overlay planes
> to create. Set it to 1 by default in order to keep the "enable_overlay"
> backwards compatible.
> 
Hi José,

in general, lgtm. However, I think we need some limits for this number
of planes. I would suggest to just expand the enable_overlay option to
expose a predefined number of planes and to avoid passing "crazy"
numbers by another module option.
Afaik, we are also limited to 32, as you can see in this commit:

2a8d3eac3d6e1
drm: Warn if plane/crtc/encoder/connector index exceeds our 32bit bitmasks

I don't have a strong opinion on an exact/practical number. I took a
quick look at other drivers and exposing 8 planes seems reasonable to
me. Also, changing this number in the future would be pretty
straightfoward.

Thanks,

Melissa

> Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c    | 5 +++++
>  drivers/gpu/drm/vkms/vkms_drv.h    | 1 +
>  drivers/gpu/drm/vkms/vkms_output.c | 9 ++++++---
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 0ffe5f0e33f7..bb98f6c6c561 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -51,6 +51,10 @@ static bool enable_overlay;
>  module_param_named(enable_overlay, enable_overlay, bool, 0444);
>  MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
>  
> +static unsigned int num_overlay_planes = 1;
> +module_param_named(num_overlay_planes, num_overlay_planes, uint, 0444);
> +MODULE_PARM_DESC(num_overlay_planes, "Number of overlay planes to create");
> +
>  DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>  
>  static void vkms_release(struct drm_device *dev)
> @@ -229,6 +233,7 @@ static int __init vkms_init(void)
>  	config->cursor = enable_cursor;
>  	config->writeback = enable_writeback;
>  	config->overlay = enable_overlay;
> +	config->num_overlay_planes = num_overlay_planes;
>  
>  	return vkms_create(config);
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index d48c23d40ce5..33bdf717e3cd 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -97,6 +97,7 @@ struct vkms_config {
>  	bool writeback;
>  	bool cursor;
>  	bool overlay;
> +	unsigned int num_overlay_planes;
>  	/* only set when instantiated */
>  	struct vkms_device *dev;
>  };
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 2e805b2d36ae..6f26998fdb7e 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -57,15 +57,18 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  	struct vkms_plane *primary, *cursor = NULL;
>  	int ret;
>  	int writeback;
> +	unsigned int n;
>  
>  	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
>  	if (IS_ERR(primary))
>  		return PTR_ERR(primary);
>  
>  	if (vkmsdev->config->overlay) {
> -		ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> -		if (ret)
> -			return ret;
> +		for (n = 0; n < vkmsdev->config->num_overlay_planes; n++) {
> +			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	if (vkmsdev->config->cursor) {
> -- 
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature