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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 04, 2015 at 01:05:35PM +0900, Inki Dae wrote:
> 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.

I think cursor should by default be the top layer (if the hw can do it).
Otherwise it will be really confusing to compositors I fear.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch