Re: [PATCH 09/14] drm/cirrus: use universal plane interfaces for primary plane

From: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
To: Varad Gautam <varadgautam@gmail.com>
Cc: Varad Gautam <varad.gautam@collabora.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 09/14] drm/cirrus: use universal plane interfaces for primary plane
Date: Mon, 04 Sep 2017 08:14:41 -0300	[thread overview]
Message-ID: <87efrmsoke.fsf@dilma.collabora.co.uk> (raw)
In-Reply-To: <20170818154919.18607-10-varadgautam@gmail.com> (Varad Gautam's message of "Fri, 18 Aug 2017 21:19:14 +0530")

Varad Gautam <varadgautam@gmail.com> writes:

> From: Varad Gautam <varad.gautam@collabora.com>
>
> cirrus exposes one legacy primary plane tied to the crtc. convert this to
> use the universal planes interface in preparation for atomic.
>
> Signed-off-by: Varad Gautam <varad.gautam@collabora.com>
> ---
>  drivers/gpu/drm/cirrus/cirrus_mode.c | 44 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
> index 6032978a2797..2994dd391850 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_mode.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
> @@ -358,12 +358,29 @@ static const struct drm_crtc_helper_funcs cirrus_helper_funcs = {
>  	.load_lut = cirrus_crtc_load_lut,
>  };
>  
> +static const uint32_t cirrus_plane_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_RGB565,
> +};
> +
> +static const struct drm_plane_funcs cirrus_plane_funcs = {
> +	.update_plane	= drm_primary_helper_update,
> +	.disable_plane	= drm_primary_helper_disable,
> +	.destroy	= drm_primary_helper_destroy,
> +};
> +
> +static const struct drm_plane_helper_funcs cirrus_plane_helper_funcs = {
> +};
> +
>  /* CRTC setup */
>  static void cirrus_crtc_init(struct drm_device *dev)
>  {
>  	struct cirrus_device *cdev = dev->dev_private;
>  	struct cirrus_crtc *cirrus_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	cirrus_crtc = kzalloc(sizeof(struct cirrus_crtc) +
>  			      (CIRRUSFB_CONN_LIMIT * sizeof(struct drm_connector *)),
> @@ -372,8 +389,23 @@ 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 = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		goto cleanup_crtc;
>  
> +	drm_plane_helper_add(primary, &cirrus_plane_helper_funcs);
> +	ret = drm_universal_plane_init(dev, primary, 1,
> +				       &cirrus_plane_funcs,
> +				       cirrus_plane_formats,
> +				       ARRAY_SIZE(cirrus_plane_formats),
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret)
> +		goto cleanup;

If this fails early enough, you can't call drm_plane_cleanup, otherwise
you'll hit an oops.

Otherwise, looks good.  with the above change, please add:

Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>

> +
> +	ret = drm_crtc_init_with_planes(dev, &cirrus_crtc->base, primary, NULL,
> +					&cirrus_crtc_funcs, NULL);
> +	if (ret)
> +		goto cleanup;
>  	drm_mode_crtc_set_gamma_size(&cirrus_crtc->base, CIRRUS_LUT_SIZE);
>  	cdev->mode_info.crtc = cirrus_crtc;
>  
> @@ -384,6 +416,14 @@ static void cirrus_crtc_init(struct drm_device *dev)
>  	}
>  
>  	drm_crtc_helper_add(&cirrus_crtc->base, &cirrus_helper_funcs);
> +	return;
> +
> +cleanup:
> +	drm_plane_cleanup(primary);
> +	kfree(primary);
> +cleanup_crtc:
> +	kfree(cirrus_crtc);
> +	return;
>  }
>  
>  /** Sets the color ramps on behalf of fbcon */

-- 
Gabriel Krisman Bertazi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel