Re: [PATCH] drm/exynos: add cursor plane support

[Inki Dae <inki.dae@samsung.com>]

Hi Gustavo,

I had already a review but I didn't give any comment to you. Sorry about
that. This patch looks good to me but one thing isn't clear. Below is my
comment.


On 2015년 09월 04일 05:14, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Set one of the planes for each crtc driver as a cursor plane enabled
> window managers to fully work on exynos.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  9 ++-------
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  8 ++------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  3 +++
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  8 ++------
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     | 18 +++++++++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_plane.h     |  5 ++---
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      |  9 ++-------
>  drivers/gpu/drm/exynos/exynos_mixer.c         | 10 +++-------
>  8 files changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index b3c7307..79b2b22 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -33,7 +33,6 @@ struct decon_context {
>  	struct exynos_drm_plane		planes[WINDOWS_NR];
>  	void __iomem			*addr;
>  	struct clk			*clks[6];
> -	unsigned int			default_win;
>  	unsigned long			irq_flags;
>  	int				pipe;
>  	bool				suspended;
> @@ -493,7 +492,6 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  	struct drm_device *drm_dev = data;
>  	struct exynos_drm_private *priv = drm_dev->dev_private;
>  	struct exynos_drm_plane *exynos_plane;
> -	enum drm_plane_type type;
>  	unsigned int zpos;
>  	int ret;
>  
> @@ -501,16 +499,14 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  	ctx->pipe = priv->pipe++;
>  
>  	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
> -		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
> -							DRM_PLANE_TYPE_OVERLAY;
>  		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
> -				1 << ctx->pipe, type, decon_formats,
> +				1 << ctx->pipe, decon_formats,
>  				ARRAY_SIZE(decon_formats), zpos);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	exynos_plane = &ctx->planes[ctx->default_win];
> +	exynos_plane = &ctx->planes[DEFAULT_WIN];
>  	ctx->crtc = exynos_drm_crtc_create(drm_dev, &exynos_plane->base,
>  					ctx->pipe, EXYNOS_DISPLAY_TYPE_LCD,
>  					&decon_crtc_ops, ctx);
> @@ -607,7 +603,6 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ctx->default_win = 0;
>  	ctx->suspended = true;
>  	ctx->dev = dev;
>  	if (of_get_child_by_name(dev->of_node, "i80-if-timings"))
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index cbdb78e..f3826dc 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -52,7 +52,6 @@ struct decon_context {
>  	struct clk			*eclk;
>  	struct clk			*vclk;
>  	void __iomem			*regs;
> -	unsigned int			default_win;
>  	unsigned long			irq_flags;
>  	bool				i80_if;
>  	bool				suspended;
> @@ -691,7 +690,6 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  	struct decon_context *ctx = dev_get_drvdata(dev);
>  	struct drm_device *drm_dev = data;
>  	struct exynos_drm_plane *exynos_plane;
> -	enum drm_plane_type type;
>  	unsigned int zpos;
>  	int ret;
>  
> @@ -702,16 +700,14 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  	}
>  
>  	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
> -		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
> -						DRM_PLANE_TYPE_OVERLAY;
>  		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
> -					1 << ctx->pipe, type, decon_formats,
> +					1 << ctx->pipe, decon_formats,
>  					ARRAY_SIZE(decon_formats), zpos);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	exynos_plane = &ctx->planes[ctx->default_win];
> +	exynos_plane = &ctx->planes[DEFAULT_WIN];
>  	ctx->crtc = exynos_drm_crtc_create(drm_dev, &exynos_plane->base,
>  					   ctx->pipe, EXYNOS_DISPLAY_TYPE_LCD,
>  					   &decon_crtc_ops, ctx);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index b7ba21d..cc56c3d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -22,6 +22,9 @@
>  #define MAX_PLANE	5
>  #define MAX_FB_BUFFER	4
>  
> +#define DEFAULT_WIN	0
> +#define CURSOR_WIN	1

You fixed overlay number for cursor with 1. However, Display controllers
of Exynos SoC have fixed overlay priority like this,

win 4 > win 3 > win 2 > win 1 > win 0 so if we fix the overlay number
for cursor and we use another overlay - which has a higher layer than
cursor - to display caption or UI then the we couldn't see the cursor on
screen without additional work such as color key or alpha channel.

So before that, you need to check how many overlays can be used
according to Exynos SoC versions, and which way is better - one is for
top layer to be fixed for cursor, other is for one given layer to be
fixed by user-space for cursor.

Thanks,
Inki Dae

> +