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

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

On 2015년 09월 04일 16:19, Daniel Vetter wrote:
> 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.

I also prefer first one.

Thanks,
Inki Dae

> -Daniel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel