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 at 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 at 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
