Re: [PATCH] drm/cirrus: add plane setup

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 04, 2019 at 12:01:31PM +0100, Gerd Hoffmann wrote:
> Commit "f4bd542bca drm/fb-helper: Scale back depth to supported maximum"
> uncovered a bug in the cirrus driver.  It must create its own primary
> plane, using the correct format list, depending on the bpp module
> parameter, so it is consistent with mode_config->preferred_depth.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/cirrus/cirrus_mode.c | 67 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
> index a830e70fc0..2e966a22c5 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_mode.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
> @@ -360,10 +360,67 @@ static const struct drm_crtc_helper_funcs cirrus_helper_funcs = {
>  };
>  
>  /* CRTC setup */
> +static const uint32_t cirrus_formats_16[] = {
> +	DRM_FORMAT_RGB565,
> +};
> +
> +static const uint32_t cirrus_formats_24[] = {
> +	DRM_FORMAT_RGB888,
> +};
> +
> +static const uint32_t cirrus_formats_32[] = {
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_ARGB8888,
> +};

I'd include the lower-res formats too here, without those you limit
userspace that looks at this stuff to only these formats. Not that this is
really important for cirrus ... So 24bits would include 16, and 32 would
include 24 and 16. With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
> +static struct drm_plane *cirrus_primary_plane(struct drm_device *dev)
> +{
> +	const uint32_t *formats;
> +	uint32_t nformats;
> +	struct drm_plane *primary;
> +	int ret;
> +
> +	switch (cirrus_bpp) {
> +	case 16:
> +		formats = cirrus_formats_16;
> +		nformats = ARRAY_SIZE(cirrus_formats_16);
> +		break;
> +	case 24:
> +		formats = cirrus_formats_24;
> +		nformats = ARRAY_SIZE(cirrus_formats_24);
> +		break;
> +	case 32:
> +		formats = cirrus_formats_32;
> +		nformats = ARRAY_SIZE(cirrus_formats_32);
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL) {
> +		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
> +		return NULL;
> +	}
> +
> +	ret = drm_universal_plane_init(dev, primary, 0,
> +				       &drm_primary_helper_funcs,
> +				       formats, nformats,
> +				       NULL,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret) {
> +		kfree(primary);
> +		primary = NULL;
> +	}
> +
> +	return primary;
> +}
> +
>  static void cirrus_crtc_init(struct drm_device *dev)
>  {
>  	struct cirrus_device *cdev = dev->dev_private;
>  	struct cirrus_crtc *cirrus_crtc;
> +	struct drm_plane *primary;
>  
>  	cirrus_crtc = kzalloc(sizeof(struct cirrus_crtc) +
>  			      (CIRRUSFB_CONN_LIMIT * sizeof(struct drm_connector *)),
> @@ -372,7 +429,15 @@ static void cirrus_crtc_init(struct drm_device *dev)
>  	if (cirrus_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &cirrus_crtc->base, &cirrus_crtc_funcs);
> +	primary = cirrus_primary_plane(dev);
> +	if (primary == NULL) {
> +		kfree(cirrus_crtc);
> +		return;
> +	}
> +
> +	drm_crtc_init_with_planes(dev, &cirrus_crtc->base,
> +				  primary, NULL,
> +				  &cirrus_crtc_funcs, NULL);
>  
>  	drm_mode_crtc_set_gamma_size(&cirrus_crtc->base, CIRRUS_LUT_SIZE);
>  	cdev->mode_info.crtc = cirrus_crtc;
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch