* [PATCH] drm/exynos: switch to universal plane API
@ 2014-09-18 13:17 Andrzej Hajda
2014-09-19 1:02 ` Joonyoung Shim
0 siblings, 1 reply; 6+ messages in thread
From: Andrzej Hajda @ 2014-09-18 13:17 UTC (permalink / raw)
To: Inki Dae
Cc: Andrzej Hajda, Joonyoung Shim, Seung-Woo Kim, Kyungmin Park,
David Airlie, Kukjin Kim, open list:DRM DRIVERS FOR E...,
moderated list:ARM/S5P EXYNOS AR...,
open list, drake, daniel, m.szyprowski
The patch replaces legacy functions
drm_plane_init() / drm_crtc_init() with
drm_universal_plane_init() and drm_crtc_init_with_planes().
It allows to replace fake primary plane with the real one.
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi Inki,
I have tested this patch with trats board, it looks OK.
As a side effect it should solve fb refcounting issues
reported by me and Daniel Drake.
Regards
Andrzej
---
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++++++++++++++---------------
drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++-
drivers/gpu/drm/exynos/exynos_drm_plane.c | 17 +++++----
drivers/gpu/drm/exynos/exynos_drm_plane.h | 3 +-
4 files changed, 47 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index b68e58f..d2f713e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -32,7 +32,6 @@ enum exynos_crtc_mode {
* Exynos specific crtc structure.
*
* @drm_crtc: crtc object.
- * @drm_plane: pointer of private plane object for this crtc
* @manager: the manager associated with this crtc
* @pipe: a crtc index created at load() with a new crtc object creation
* and the crtc object would be set to private->crtc array
@@ -46,7 +45,6 @@ enum exynos_crtc_mode {
*/
struct exynos_drm_crtc {
struct drm_crtc drm_crtc;
- struct drm_plane *plane;
struct exynos_drm_manager *manager;
unsigned int pipe;
unsigned int dpms;
@@ -94,12 +92,12 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
- exynos_plane_commit(exynos_crtc->plane);
+ exynos_plane_commit(crtc->primary);
if (manager->ops->commit)
manager->ops->commit(manager);
- exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
+ exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
}
static bool
@@ -123,10 +121,9 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
{
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
struct exynos_drm_manager *manager = exynos_crtc->manager;
- struct drm_plane *plane = exynos_crtc->plane;
+ struct drm_framebuffer *fb = crtc->primary->fb;
unsigned int crtc_w;
unsigned int crtc_h;
- int ret;
/*
* copy the mode data adjusted by mode_fixup() into crtc->mode
@@ -134,29 +131,21 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
*/
memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
- crtc_w = crtc->primary->fb->width - x;
- crtc_h = crtc->primary->fb->height - y;
+ crtc_w = fb->width - x;
+ crtc_h = fb->height - y;
if (manager->ops->mode_set)
manager->ops->mode_set(manager, &crtc->mode);
- ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
- x, y, crtc_w, crtc_h);
- if (ret)
- return ret;
-
- plane->crtc = crtc;
- plane->fb = crtc->primary->fb;
- drm_framebuffer_reference(plane->fb);
-
- return 0;
+ return exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
+ crtc_w, crtc_h, x, y, crtc_w, crtc_h);
}
static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
struct drm_framebuffer *old_fb)
{
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
- struct drm_plane *plane = exynos_crtc->plane;
+ struct drm_framebuffer *fb = crtc->primary->fb;
unsigned int crtc_w;
unsigned int crtc_h;
int ret;
@@ -167,11 +156,11 @@ static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
return -EPERM;
}
- crtc_w = crtc->primary->fb->width - x;
- crtc_h = crtc->primary->fb->height - y;
+ crtc_w = fb->width - x;
+ crtc_h = fb->height - y;
- ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
- x, y, crtc_w, crtc_h);
+ ret = exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
+ crtc_w, crtc_h, x, y, crtc_w, crtc_h);
if (ret)
return ret;
@@ -279,6 +268,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
private->crtc[exynos_crtc->pipe] = NULL;
+ crtc->primary->funcs->destroy(crtc->primary);
drm_crtc_cleanup(crtc);
kfree(exynos_crtc);
}
@@ -304,8 +294,7 @@ static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
exynos_drm_crtc_commit(crtc);
break;
case CRTC_MODE_BLANK:
- exynos_plane_dpms(exynos_crtc->plane,
- DRM_MODE_DPMS_OFF);
+ exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
break;
default:
break;
@@ -351,8 +340,10 @@ static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
{
struct exynos_drm_crtc *exynos_crtc;
+ struct drm_plane *plane;
struct exynos_drm_private *private = manager->drm_dev->dev_private;
struct drm_crtc *crtc;
+ int ret;
exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
if (!exynos_crtc)
@@ -364,11 +355,11 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
exynos_crtc->manager = manager;
exynos_crtc->pipe = manager->pipe;
- exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
- 1 << manager->pipe, true);
- if (!exynos_crtc->plane) {
- kfree(exynos_crtc);
- return -ENOMEM;
+ plane = exynos_plane_init(manager->drm_dev, 1 << manager->pipe,
+ DRM_PLANE_TYPE_PRIMARY);
+ if (IS_ERR(plane)) {
+ ret = PTR_ERR(plane);
+ goto err_plane;
}
manager->crtc = &exynos_crtc->drm_crtc;
@@ -376,12 +367,22 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
private->crtc[manager->pipe] = crtc;
- drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
+ ret = drm_crtc_init_with_planes(manager->drm_dev, crtc, plane, NULL,
+ &exynos_crtc_funcs);
+ if (ret < 0)
+ goto err_crtc;
+
drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
exynos_drm_crtc_attach_mode_property(crtc);
return 0;
+
+err_crtc:
+ plane->funcs->destroy(plane);
+err_plane:
+ kfree(exynos_crtc);
+ return ret;
}
int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 9b00e4e..a439452 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -86,8 +86,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
struct drm_plane *plane;
unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
- plane = exynos_plane_init(dev, possible_crtcs, false);
- if (!plane)
+ plane = exynos_plane_init(dev, possible_crtcs,
+ DRM_PLANE_TYPE_OVERLAY);
+ if (IS_ERR(plane))
goto err_mode_config_cleanup;
}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 8371cbd..15e37a0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -139,6 +139,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
overlay->crtc_x, overlay->crtc_y,
overlay->crtc_width, overlay->crtc_height);
+ plane->crtc = crtc;
+
exynos_drm_crtc_plane_mode_set(crtc, overlay);
return 0;
@@ -254,25 +256,26 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane)
}
struct drm_plane *exynos_plane_init(struct drm_device *dev,
- unsigned long possible_crtcs, bool priv)
+ unsigned long possible_crtcs,
+ enum drm_plane_type type)
{
struct exynos_plane *exynos_plane;
int err;
exynos_plane = kzalloc(sizeof(struct exynos_plane), GFP_KERNEL);
if (!exynos_plane)
- return NULL;
+ return ERR_PTR(-ENOMEM);
- err = drm_plane_init(dev, &exynos_plane->base, possible_crtcs,
- &exynos_plane_funcs, formats, ARRAY_SIZE(formats),
- priv);
+ err = drm_universal_plane_init(dev, &exynos_plane->base, possible_crtcs,
+ &exynos_plane_funcs, formats,
+ ARRAY_SIZE(formats), type);
if (err) {
DRM_ERROR("failed to initialize plane\n");
kfree(exynos_plane);
- return NULL;
+ return ERR_PTR(err);
}
- if (priv)
+ if (type == DRM_PLANE_TYPE_PRIMARY)
exynos_plane->overlay.zpos = DEFAULT_ZPOS;
else
exynos_plane_attach_zpos_property(&exynos_plane->base);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index 84d464c..0d1986b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -17,4 +17,5 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
void exynos_plane_commit(struct drm_plane *plane);
void exynos_plane_dpms(struct drm_plane *plane, int mode);
struct drm_plane *exynos_plane_init(struct drm_device *dev,
- unsigned long possible_crtcs, bool priv);
+ unsigned long possible_crtcs,
+ enum drm_plane_type type);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API
2014-09-18 13:17 [PATCH] drm/exynos: switch to universal plane API Andrzej Hajda
@ 2014-09-19 1:02 ` Joonyoung Shim
2014-09-19 10:54 ` Andrzej Hajda
0 siblings, 1 reply; 6+ messages in thread
From: Joonyoung Shim @ 2014-09-19 1:02 UTC (permalink / raw)
To: Andrzej Hajda, Inki Dae
Cc: Seung-Woo Kim, Kyungmin Park, David Airlie, Kukjin Kim,
open list:DRM DRIVERS FOR E...,
moderated list:ARM/S5P EXYNOS AR...,
open list, drake, daniel, m.szyprowski
Hi Andrzej,
On 09/18/2014 10:17 PM, Andrzej Hajda wrote:
> The patch replaces legacy functions
> drm_plane_init() / drm_crtc_init() with
> drm_universal_plane_init() and drm_crtc_init_with_planes().
> It allows to replace fake primary plane with the real one.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi Inki,
>
> I have tested this patch with trats board, it looks OK.
> As a side effect it should solve fb refcounting issues
> reported by me and Daniel Drake.
>
> Regards
> Andrzej
> ---
> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++++++++++++++---------------
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++-
> drivers/gpu/drm/exynos/exynos_drm_plane.c | 17 +++++----
> drivers/gpu/drm/exynos/exynos_drm_plane.h | 3 +-
> 4 files changed, 47 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b68e58f..d2f713e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -32,7 +32,6 @@ enum exynos_crtc_mode {
> * Exynos specific crtc structure.
> *
> * @drm_crtc: crtc object.
> - * @drm_plane: pointer of private plane object for this crtc
> * @manager: the manager associated with this crtc
> * @pipe: a crtc index created at load() with a new crtc object creation
> * and the crtc object would be set to private->crtc array
> @@ -46,7 +45,6 @@ enum exynos_crtc_mode {
> */
> struct exynos_drm_crtc {
> struct drm_crtc drm_crtc;
> - struct drm_plane *plane;
> struct exynos_drm_manager *manager;
> unsigned int pipe;
> unsigned int dpms;
> @@ -94,12 +92,12 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>
> exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>
> - exynos_plane_commit(exynos_crtc->plane);
> + exynos_plane_commit(crtc->primary);
>
> if (manager->ops->commit)
> manager->ops->commit(manager);
>
> - exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
> }
>
> static bool
> @@ -123,10 +121,9 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> {
> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> struct exynos_drm_manager *manager = exynos_crtc->manager;
> - struct drm_plane *plane = exynos_crtc->plane;
> + struct drm_framebuffer *fb = crtc->primary->fb;
> unsigned int crtc_w;
> unsigned int crtc_h;
> - int ret;
>
> /*
> * copy the mode data adjusted by mode_fixup() into crtc->mode
> @@ -134,29 +131,21 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> */
> memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
>
> - crtc_w = crtc->primary->fb->width - x;
> - crtc_h = crtc->primary->fb->height - y;
> + crtc_w = fb->width - x;
> + crtc_h = fb->height - y;
>
> if (manager->ops->mode_set)
> manager->ops->mode_set(manager, &crtc->mode);
>
> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
> - x, y, crtc_w, crtc_h);
> - if (ret)
> - return ret;
> -
> - plane->crtc = crtc;
> - plane->fb = crtc->primary->fb;
> - drm_framebuffer_reference(plane->fb);
> -
> - return 0;
> + return exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
> }
>
> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
> struct drm_framebuffer *old_fb)
> {
> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> - struct drm_plane *plane = exynos_crtc->plane;
> + struct drm_framebuffer *fb = crtc->primary->fb;
> unsigned int crtc_w;
> unsigned int crtc_h;
> int ret;
> @@ -167,11 +156,11 @@ static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
> return -EPERM;
> }
>
> - crtc_w = crtc->primary->fb->width - x;
> - crtc_h = crtc->primary->fb->height - y;
> + crtc_w = fb->width - x;
> + crtc_h = fb->height - y;
>
> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
> - x, y, crtc_w, crtc_h);
> + ret = exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
> if (ret)
> return ret;
>
> @@ -279,6 +268,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
>
> private->crtc[exynos_crtc->pipe] = NULL;
>
> + crtc->primary->funcs->destroy(crtc->primary);
This is unnecessary.
> drm_crtc_cleanup(crtc);
> kfree(exynos_crtc);
> }
> @@ -304,8 +294,7 @@ static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
> exynos_drm_crtc_commit(crtc);
> break;
> case CRTC_MODE_BLANK:
> - exynos_plane_dpms(exynos_crtc->plane,
> - DRM_MODE_DPMS_OFF);
> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
> break;
> default:
> break;
> @@ -351,8 +340,10 @@ static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
> int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
> {
> struct exynos_drm_crtc *exynos_crtc;
> + struct drm_plane *plane;
> struct exynos_drm_private *private = manager->drm_dev->dev_private;
> struct drm_crtc *crtc;
> + int ret;
>
> exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
> if (!exynos_crtc)
> @@ -364,11 +355,11 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
> exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
> exynos_crtc->manager = manager;
> exynos_crtc->pipe = manager->pipe;
> - exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
> - 1 << manager->pipe, true);
> - if (!exynos_crtc->plane) {
> - kfree(exynos_crtc);
> - return -ENOMEM;
> + plane = exynos_plane_init(manager->drm_dev, 1 << manager->pipe,
> + DRM_PLANE_TYPE_PRIMARY);
> + if (IS_ERR(plane)) {
> + ret = PTR_ERR(plane);
> + goto err_plane;
> }
>
> manager->crtc = &exynos_crtc->drm_crtc;
> @@ -376,12 +367,22 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>
> private->crtc[manager->pipe] = crtc;
>
> - drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
> + ret = drm_crtc_init_with_planes(manager->drm_dev, crtc, plane, NULL,
> + &exynos_crtc_funcs);
> + if (ret < 0)
> + goto err_crtc;
> +
> drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
>
> exynos_drm_crtc_attach_mode_property(crtc);
>
> return 0;
> +
> +err_crtc:
> + plane->funcs->destroy(plane);
> +err_plane:
> + kfree(exynos_crtc);
> + return ret;
> }
>
> int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9b00e4e..a439452 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -86,8 +86,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
> struct drm_plane *plane;
> unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
>
> - plane = exynos_plane_init(dev, possible_crtcs, false);
> - if (!plane)
> + plane = exynos_plane_init(dev, possible_crtcs,
> + DRM_PLANE_TYPE_OVERLAY);
> + if (IS_ERR(plane))
> goto err_mode_config_cleanup;
> }
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index 8371cbd..15e37a0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -139,6 +139,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
> overlay->crtc_x, overlay->crtc_y,
> overlay->crtc_width, overlay->crtc_height);
>
> + plane->crtc = crtc;
> +
OK, then we can remove same code from exynos_update_plane().
One more, plane->crtc is NULL before mode_set or setplane so it's
problem if call plane->funcs->destroy with plane->crtc == NULL.
We need checking plane->crtc is NULL in exynos_disable_plane().
> exynos_drm_crtc_plane_mode_set(crtc, overlay);
>
> return 0;
> @@ -254,25 +256,26 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane)
> }
>
> struct drm_plane *exynos_plane_init(struct drm_device *dev,
> - unsigned long possible_crtcs, bool priv)
> + unsigned long possible_crtcs,
> + enum drm_plane_type type)
> {
> struct exynos_plane *exynos_plane;
> int err;
>
> exynos_plane = kzalloc(sizeof(struct exynos_plane), GFP_KERNEL);
> if (!exynos_plane)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> - err = drm_plane_init(dev, &exynos_plane->base, possible_crtcs,
> - &exynos_plane_funcs, formats, ARRAY_SIZE(formats),
> - priv);
> + err = drm_universal_plane_init(dev, &exynos_plane->base, possible_crtcs,
> + &exynos_plane_funcs, formats,
> + ARRAY_SIZE(formats), type);
> if (err) {
> DRM_ERROR("failed to initialize plane\n");
> kfree(exynos_plane);
> - return NULL;
> + return ERR_PTR(err);
> }
>
> - if (priv)
> + if (type == DRM_PLANE_TYPE_PRIMARY)
> exynos_plane->overlay.zpos = DEFAULT_ZPOS;
> else
> exynos_plane_attach_zpos_property(&exynos_plane->base);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
> index 84d464c..0d1986b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
> @@ -17,4 +17,5 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
> void exynos_plane_commit(struct drm_plane *plane);
> void exynos_plane_dpms(struct drm_plane *plane, int mode);
> struct drm_plane *exynos_plane_init(struct drm_device *dev,
> - unsigned long possible_crtcs, bool priv);
> + unsigned long possible_crtcs,
> + enum drm_plane_type type);
>
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API
2014-09-19 1:02 ` Joonyoung Shim
@ 2014-09-19 10:54 ` Andrzej Hajda
2014-09-19 11:11 ` Joonyoung Shim
0 siblings, 1 reply; 6+ messages in thread
From: Andrzej Hajda @ 2014-09-19 10:54 UTC (permalink / raw)
To: Joonyoung Shim, Inki Dae
Cc: Seung-Woo Kim, Kyungmin Park, David Airlie, Kukjin Kim,
open list:DRM DRIVERS FOR E...,
moderated list:ARM/S5P EXYNOS AR...,
open list, drake, daniel, m.szyprowski
On 09/19/2014 03:02 AM, Joonyoung Shim wrote:
> Hi Andrzej,
>
> On 09/18/2014 10:17 PM, Andrzej Hajda wrote:
>> The patch replaces legacy functions
>> drm_plane_init() / drm_crtc_init() with
>> drm_universal_plane_init() and drm_crtc_init_with_planes().
>> It allows to replace fake primary plane with the real one.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> Hi Inki,
>>
>> I have tested this patch with trats board, it looks OK.
>> As a side effect it should solve fb refcounting issues
>> reported by me and Daniel Drake.
>>
>> Regards
>> Andrzej
>> ---
>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++++++++++++++---------------
>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++-
>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 17 +++++----
>> drivers/gpu/drm/exynos/exynos_drm_plane.h | 3 +-
>> 4 files changed, 47 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> index b68e58f..d2f713e 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> @@ -32,7 +32,6 @@ enum exynos_crtc_mode {
>> * Exynos specific crtc structure.
>> *
>> * @drm_crtc: crtc object.
>> - * @drm_plane: pointer of private plane object for this crtc
>> * @manager: the manager associated with this crtc
>> * @pipe: a crtc index created at load() with a new crtc object creation
>> * and the crtc object would be set to private->crtc array
>> @@ -46,7 +45,6 @@ enum exynos_crtc_mode {
>> */
>> struct exynos_drm_crtc {
>> struct drm_crtc drm_crtc;
>> - struct drm_plane *plane;
>> struct exynos_drm_manager *manager;
>> unsigned int pipe;
>> unsigned int dpms;
>> @@ -94,12 +92,12 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>>
>> exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>>
>> - exynos_plane_commit(exynos_crtc->plane);
>> + exynos_plane_commit(crtc->primary);
>>
>> if (manager->ops->commit)
>> manager->ops->commit(manager);
>>
>> - exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
>> }
>>
>> static bool
>> @@ -123,10 +121,9 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>> {
>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>> struct exynos_drm_manager *manager = exynos_crtc->manager;
>> - struct drm_plane *plane = exynos_crtc->plane;
>> + struct drm_framebuffer *fb = crtc->primary->fb;
>> unsigned int crtc_w;
>> unsigned int crtc_h;
>> - int ret;
>>
>> /*
>> * copy the mode data adjusted by mode_fixup() into crtc->mode
>> @@ -134,29 +131,21 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>> */
>> memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
>>
>> - crtc_w = crtc->primary->fb->width - x;
>> - crtc_h = crtc->primary->fb->height - y;
>> + crtc_w = fb->width - x;
>> + crtc_h = fb->height - y;
>>
>> if (manager->ops->mode_set)
>> manager->ops->mode_set(manager, &crtc->mode);
>>
>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>> - x, y, crtc_w, crtc_h);
>> - if (ret)
>> - return ret;
>> -
>> - plane->crtc = crtc;
>> - plane->fb = crtc->primary->fb;
>> - drm_framebuffer_reference(plane->fb);
>> -
>> - return 0;
>> + return exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>> }
>>
>> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>> struct drm_framebuffer *old_fb)
>> {
>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>> - struct drm_plane *plane = exynos_crtc->plane;
>> + struct drm_framebuffer *fb = crtc->primary->fb;
>> unsigned int crtc_w;
>> unsigned int crtc_h;
>> int ret;
>> @@ -167,11 +156,11 @@ static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>> return -EPERM;
>> }
>>
>> - crtc_w = crtc->primary->fb->width - x;
>> - crtc_h = crtc->primary->fb->height - y;
>> + crtc_w = fb->width - x;
>> + crtc_h = fb->height - y;
>>
>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>> - x, y, crtc_w, crtc_h);
>> + ret = exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>> if (ret)
>> return ret;
>>
>> @@ -279,6 +268,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
>>
>> private->crtc[exynos_crtc->pipe] = NULL;
>>
>> + crtc->primary->funcs->destroy(crtc->primary);
> This is unnecessary.
The question is how these object should be destroyed. In current code
crtc is destroyed in fimd_unbind and it is called before
drm_mode_config_cleanup
which destroys all planes.
In such case primary plane will stay with .crtc pointing to non-existing
crtc.
Maybe performing crtcs cleanup after planes cleanup is better solution???
>
>> drm_crtc_cleanup(crtc);
>> kfree(exynos_crtc);
>> }
>> @@ -304,8 +294,7 @@ static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
>> exynos_drm_crtc_commit(crtc);
>> break;
>> case CRTC_MODE_BLANK:
>> - exynos_plane_dpms(exynos_crtc->plane,
>> - DRM_MODE_DPMS_OFF);
>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
>> break;
>> default:
>> break;
>> @@ -351,8 +340,10 @@ static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
>> int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>> {
>> struct exynos_drm_crtc *exynos_crtc;
>> + struct drm_plane *plane;
>> struct exynos_drm_private *private = manager->drm_dev->dev_private;
>> struct drm_crtc *crtc;
>> + int ret;
>>
>> exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
>> if (!exynos_crtc)
>> @@ -364,11 +355,11 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>> exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
>> exynos_crtc->manager = manager;
>> exynos_crtc->pipe = manager->pipe;
>> - exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
>> - 1 << manager->pipe, true);
>> - if (!exynos_crtc->plane) {
>> - kfree(exynos_crtc);
>> - return -ENOMEM;
>> + plane = exynos_plane_init(manager->drm_dev, 1 << manager->pipe,
>> + DRM_PLANE_TYPE_PRIMARY);
>> + if (IS_ERR(plane)) {
>> + ret = PTR_ERR(plane);
>> + goto err_plane;
>> }
>>
>> manager->crtc = &exynos_crtc->drm_crtc;
>> @@ -376,12 +367,22 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>
>> private->crtc[manager->pipe] = crtc;
>>
>> - drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
>> + ret = drm_crtc_init_with_planes(manager->drm_dev, crtc, plane, NULL,
>> + &exynos_crtc_funcs);
>> + if (ret < 0)
>> + goto err_crtc;
>> +
>> drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
>>
>> exynos_drm_crtc_attach_mode_property(crtc);
>>
>> return 0;
>> +
>> +err_crtc:
>> + plane->funcs->destroy(plane);
>> +err_plane:
>> + kfree(exynos_crtc);
>> + return ret;
>> }
>>
>> int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 9b00e4e..a439452 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -86,8 +86,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
>> struct drm_plane *plane;
>> unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
>>
>> - plane = exynos_plane_init(dev, possible_crtcs, false);
>> - if (!plane)
>> + plane = exynos_plane_init(dev, possible_crtcs,
>> + DRM_PLANE_TYPE_OVERLAY);
>> + if (IS_ERR(plane))
>> goto err_mode_config_cleanup;
>> }
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index 8371cbd..15e37a0 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -139,6 +139,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>> overlay->crtc_x, overlay->crtc_y,
>> overlay->crtc_width, overlay->crtc_height);
>>
>> + plane->crtc = crtc;
>> +
> OK, then we can remove same code from exynos_update_plane().
Right.
>
> One more, plane->crtc is NULL before mode_set or setplane so it's
> problem if call plane->funcs->destroy with plane->crtc == NULL.
> We need checking plane->crtc is NULL in exynos_disable_plane().
I can simply add checks, but why we allow the plane with NULL crtc to be
enabled?
Regards
Andrzej
>
>> exynos_drm_crtc_plane_mode_set(crtc, overlay);
>>
>> return 0;
>> @@ -254,25 +256,26 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane)
>> }
>>
>> struct drm_plane *exynos_plane_init(struct drm_device *dev,
>> - unsigned long possible_crtcs, bool priv)
>> + unsigned long possible_crtcs,
>> + enum drm_plane_type type)
>> {
>> struct exynos_plane *exynos_plane;
>> int err;
>>
>> exynos_plane = kzalloc(sizeof(struct exynos_plane), GFP_KERNEL);
>> if (!exynos_plane)
>> - return NULL;
>> + return ERR_PTR(-ENOMEM);
>>
>> - err = drm_plane_init(dev, &exynos_plane->base, possible_crtcs,
>> - &exynos_plane_funcs, formats, ARRAY_SIZE(formats),
>> - priv);
>> + err = drm_universal_plane_init(dev, &exynos_plane->base, possible_crtcs,
>> + &exynos_plane_funcs, formats,
>> + ARRAY_SIZE(formats), type);
>> if (err) {
>> DRM_ERROR("failed to initialize plane\n");
>> kfree(exynos_plane);
>> - return NULL;
>> + return ERR_PTR(err);
>> }
>>
>> - if (priv)
>> + if (type == DRM_PLANE_TYPE_PRIMARY)
>> exynos_plane->overlay.zpos = DEFAULT_ZPOS;
>> else
>> exynos_plane_attach_zpos_property(&exynos_plane->base);
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
>> index 84d464c..0d1986b 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
>> @@ -17,4 +17,5 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>> void exynos_plane_commit(struct drm_plane *plane);
>> void exynos_plane_dpms(struct drm_plane *plane, int mode);
>> struct drm_plane *exynos_plane_init(struct drm_device *dev,
>> - unsigned long possible_crtcs, bool priv);
>> + unsigned long possible_crtcs,
>> + enum drm_plane_type type);
>>
> Thanks.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API
2014-09-19 10:54 ` Andrzej Hajda
@ 2014-09-19 11:11 ` Joonyoung Shim
2014-09-19 11:29 ` Joonyoung Shim
2014-09-19 11:51 ` Andrzej Hajda
0 siblings, 2 replies; 6+ messages in thread
From: Joonyoung Shim @ 2014-09-19 11:11 UTC (permalink / raw)
To: Andrzej Hajda, Inki Dae
Cc: Seung-Woo Kim, Kyungmin Park, David Airlie, Kukjin Kim,
open list:DRM DRIVERS FOR E...,
moderated list:ARM/S5P EXYNOS AR...,
open list, drake, daniel, m.szyprowski
Hi,
On 09/19/2014 07:54 PM, Andrzej Hajda wrote:
> On 09/19/2014 03:02 AM, Joonyoung Shim wrote:
>> Hi Andrzej,
>>
>> On 09/18/2014 10:17 PM, Andrzej Hajda wrote:
>>> The patch replaces legacy functions
>>> drm_plane_init() / drm_crtc_init() with
>>> drm_universal_plane_init() and drm_crtc_init_with_planes().
>>> It allows to replace fake primary plane with the real one.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> Hi Inki,
>>>
>>> I have tested this patch with trats board, it looks OK.
>>> As a side effect it should solve fb refcounting issues
>>> reported by me and Daniel Drake.
>>>
>>> Regards
>>> Andrzej
>>> ---
>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++++++++++++++---------------
>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++-
>>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 17 +++++----
>>> drivers/gpu/drm/exynos/exynos_drm_plane.h | 3 +-
>>> 4 files changed, 47 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index b68e58f..d2f713e 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -32,7 +32,6 @@ enum exynos_crtc_mode {
>>> * Exynos specific crtc structure.
>>> *
>>> * @drm_crtc: crtc object.
>>> - * @drm_plane: pointer of private plane object for this crtc
>>> * @manager: the manager associated with this crtc
>>> * @pipe: a crtc index created at load() with a new crtc object creation
>>> * and the crtc object would be set to private->crtc array
>>> @@ -46,7 +45,6 @@ enum exynos_crtc_mode {
>>> */
>>> struct exynos_drm_crtc {
>>> struct drm_crtc drm_crtc;
>>> - struct drm_plane *plane;
>>> struct exynos_drm_manager *manager;
>>> unsigned int pipe;
>>> unsigned int dpms;
>>> @@ -94,12 +92,12 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>>>
>>> exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>>>
>>> - exynos_plane_commit(exynos_crtc->plane);
>>> + exynos_plane_commit(crtc->primary);
>>>
>>> if (manager->ops->commit)
>>> manager->ops->commit(manager);
>>>
>>> - exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
>>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
>>> }
>>>
>>> static bool
>>> @@ -123,10 +121,9 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>> {
>>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>> struct exynos_drm_manager *manager = exynos_crtc->manager;
>>> - struct drm_plane *plane = exynos_crtc->plane;
>>> + struct drm_framebuffer *fb = crtc->primary->fb;
>>> unsigned int crtc_w;
>>> unsigned int crtc_h;
>>> - int ret;
>>>
>>> /*
>>> * copy the mode data adjusted by mode_fixup() into crtc->mode
>>> @@ -134,29 +131,21 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>> */
>>> memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
>>>
>>> - crtc_w = crtc->primary->fb->width - x;
>>> - crtc_h = crtc->primary->fb->height - y;
>>> + crtc_w = fb->width - x;
>>> + crtc_h = fb->height - y;
>>>
>>> if (manager->ops->mode_set)
>>> manager->ops->mode_set(manager, &crtc->mode);
>>>
>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>> - x, y, crtc_w, crtc_h);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - plane->crtc = crtc;
>>> - plane->fb = crtc->primary->fb;
>>> - drm_framebuffer_reference(plane->fb);
>>> -
>>> - return 0;
>>> + return exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>> }
>>>
>>> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>> struct drm_framebuffer *old_fb)
>>> {
>>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>> - struct drm_plane *plane = exynos_crtc->plane;
>>> + struct drm_framebuffer *fb = crtc->primary->fb;
>>> unsigned int crtc_w;
>>> unsigned int crtc_h;
>>> int ret;
>>> @@ -167,11 +156,11 @@ static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>> return -EPERM;
>>> }
>>>
>>> - crtc_w = crtc->primary->fb->width - x;
>>> - crtc_h = crtc->primary->fb->height - y;
>>> + crtc_w = fb->width - x;
>>> + crtc_h = fb->height - y;
>>>
>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>> - x, y, crtc_w, crtc_h);
>>> + ret = exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>> if (ret)
>>> return ret;
>>>
>>> @@ -279,6 +268,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
>>>
>>> private->crtc[exynos_crtc->pipe] = NULL;
>>>
>>> + crtc->primary->funcs->destroy(crtc->primary);
>> This is unnecessary.
>
> The question is how these object should be destroyed. In current code
> crtc is destroyed in fimd_unbind and it is called before
> drm_mode_config_cleanup
> which destroys all planes.
> In such case primary plane will stay with .crtc pointing to non-existing
> crtc.
> Maybe performing crtcs cleanup after planes cleanup is better solution???
I think it's wrong to call destroy function of crtc in fimd_unbind, all
objects should be destroyed by drm_mode_config_cleanup except error
cases while initializing.
>
>>
>>> drm_crtc_cleanup(crtc);
>>> kfree(exynos_crtc);
>>> }
>>> @@ -304,8 +294,7 @@ static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
>>> exynos_drm_crtc_commit(crtc);
>>> break;
>>> case CRTC_MODE_BLANK:
>>> - exynos_plane_dpms(exynos_crtc->plane,
>>> - DRM_MODE_DPMS_OFF);
>>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
>>> break;
>>> default:
>>> break;
>>> @@ -351,8 +340,10 @@ static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
>>> int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>> {
>>> struct exynos_drm_crtc *exynos_crtc;
>>> + struct drm_plane *plane;
>>> struct exynos_drm_private *private = manager->drm_dev->dev_private;
>>> struct drm_crtc *crtc;
>>> + int ret;
>>>
>>> exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
>>> if (!exynos_crtc)
>>> @@ -364,11 +355,11 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>> exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
>>> exynos_crtc->manager = manager;
>>> exynos_crtc->pipe = manager->pipe;
>>> - exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
>>> - 1 << manager->pipe, true);
>>> - if (!exynos_crtc->plane) {
>>> - kfree(exynos_crtc);
>>> - return -ENOMEM;
>>> + plane = exynos_plane_init(manager->drm_dev, 1 << manager->pipe,
>>> + DRM_PLANE_TYPE_PRIMARY);
>>> + if (IS_ERR(plane)) {
>>> + ret = PTR_ERR(plane);
>>> + goto err_plane;
>>> }
>>>
>>> manager->crtc = &exynos_crtc->drm_crtc;
>>> @@ -376,12 +367,22 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>
>>> private->crtc[manager->pipe] = crtc;
>>>
>>> - drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
>>> + ret = drm_crtc_init_with_planes(manager->drm_dev, crtc, plane, NULL,
>>> + &exynos_crtc_funcs);
>>> + if (ret < 0)
>>> + goto err_crtc;
>>> +
>>> drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
>>>
>>> exynos_drm_crtc_attach_mode_property(crtc);
>>>
>>> return 0;
>>> +
>>> +err_crtc:
>>> + plane->funcs->destroy(plane);
>>> +err_plane:
>>> + kfree(exynos_crtc);
>>> + return ret;
>>> }
>>>
>>> int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> index 9b00e4e..a439452 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> @@ -86,8 +86,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
>>> struct drm_plane *plane;
>>> unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
>>>
>>> - plane = exynos_plane_init(dev, possible_crtcs, false);
>>> - if (!plane)
>>> + plane = exynos_plane_init(dev, possible_crtcs,
>>> + DRM_PLANE_TYPE_OVERLAY);
>>> + if (IS_ERR(plane))
>>> goto err_mode_config_cleanup;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> index 8371cbd..15e37a0 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> @@ -139,6 +139,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>>> overlay->crtc_x, overlay->crtc_y,
>>> overlay->crtc_width, overlay->crtc_height);
>>>
>>> + plane->crtc = crtc;
>>> +
>> OK, then we can remove same code from exynos_update_plane().
>
> Right.
>
>>
>> One more, plane->crtc is NULL before mode_set or setplane so it's
>> problem if call plane->funcs->destroy with plane->crtc == NULL.
>> We need checking plane->crtc is NULL in exynos_disable_plane().
>
> I can simply add checks, but why we allow the plane with NULL crtc to be
> enabled?
>
I mean plane disable case, not enable case.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API
2014-09-19 11:11 ` Joonyoung Shim
@ 2014-09-19 11:29 ` Joonyoung Shim
2014-09-19 11:51 ` Andrzej Hajda
1 sibling, 0 replies; 6+ messages in thread
From: Joonyoung Shim @ 2014-09-19 11:29 UTC (permalink / raw)
To: Andrzej Hajda, Inki Dae
Cc: moderated list:ARM/S5P EXYNOS AR...,
Seung-Woo Kim, open list, open list:DRM DRIVERS FOR E...,
drake, Kyungmin Park, Kukjin Kim, m.szyprowski
Hi,
On 09/19/2014 08:11 PM, Joonyoung Shim wrote:
> Hi,
>
> On 09/19/2014 07:54 PM, Andrzej Hajda wrote:
>> On 09/19/2014 03:02 AM, Joonyoung Shim wrote:
>>> Hi Andrzej,
>>>
>>> On 09/18/2014 10:17 PM, Andrzej Hajda wrote:
>>>> The patch replaces legacy functions
>>>> drm_plane_init() / drm_crtc_init() with
>>>> drm_universal_plane_init() and drm_crtc_init_with_planes().
>>>> It allows to replace fake primary plane with the real one.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> Hi Inki,
>>>>
>>>> I have tested this patch with trats board, it looks OK.
>>>> As a side effect it should solve fb refcounting issues
>>>> reported by me and Daniel Drake.
>>>>
>>>> Regards
>>>> Andrzej
>>>> ---
>>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++++++++++++++---------------
>>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++-
>>>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 17 +++++----
>>>> drivers/gpu/drm/exynos/exynos_drm_plane.h | 3 +-
>>>> 4 files changed, 47 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> index b68e58f..d2f713e 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> @@ -32,7 +32,6 @@ enum exynos_crtc_mode {
>>>> * Exynos specific crtc structure.
>>>> *
>>>> * @drm_crtc: crtc object.
>>>> - * @drm_plane: pointer of private plane object for this crtc
>>>> * @manager: the manager associated with this crtc
>>>> * @pipe: a crtc index created at load() with a new crtc object creation
>>>> * and the crtc object would be set to private->crtc array
>>>> @@ -46,7 +45,6 @@ enum exynos_crtc_mode {
>>>> */
>>>> struct exynos_drm_crtc {
>>>> struct drm_crtc drm_crtc;
>>>> - struct drm_plane *plane;
>>>> struct exynos_drm_manager *manager;
>>>> unsigned int pipe;
>>>> unsigned int dpms;
>>>> @@ -94,12 +92,12 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>>>>
>>>> exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>>>>
>>>> - exynos_plane_commit(exynos_crtc->plane);
>>>> + exynos_plane_commit(crtc->primary);
>>>>
>>>> if (manager->ops->commit)
>>>> manager->ops->commit(manager);
>>>>
>>>> - exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
>>>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
>>>> }
>>>>
>>>> static bool
>>>> @@ -123,10 +121,9 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>> {
>>>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>>> struct exynos_drm_manager *manager = exynos_crtc->manager;
>>>> - struct drm_plane *plane = exynos_crtc->plane;
>>>> + struct drm_framebuffer *fb = crtc->primary->fb;
>>>> unsigned int crtc_w;
>>>> unsigned int crtc_h;
>>>> - int ret;
>>>>
>>>> /*
>>>> * copy the mode data adjusted by mode_fixup() into crtc->mode
>>>> @@ -134,29 +131,21 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>> */
>>>> memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
>>>>
>>>> - crtc_w = crtc->primary->fb->width - x;
>>>> - crtc_h = crtc->primary->fb->height - y;
>>>> + crtc_w = fb->width - x;
>>>> + crtc_h = fb->height - y;
>>>>
>>>> if (manager->ops->mode_set)
>>>> manager->ops->mode_set(manager, &crtc->mode);
>>>>
>>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>>> - x, y, crtc_w, crtc_h);
>>>> - if (ret)
>>>> - return ret;
>>>> -
>>>> - plane->crtc = crtc;
>>>> - plane->fb = crtc->primary->fb;
>>>> - drm_framebuffer_reference(plane->fb);
>>>> -
>>>> - return 0;
>>>> + return exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>>> }
>>>>
>>>> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>>> struct drm_framebuffer *old_fb)
>>>> {
>>>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>>> - struct drm_plane *plane = exynos_crtc->plane;
>>>> + struct drm_framebuffer *fb = crtc->primary->fb;
>>>> unsigned int crtc_w;
>>>> unsigned int crtc_h;
>>>> int ret;
>>>> @@ -167,11 +156,11 @@ static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>>> return -EPERM;
>>>> }
>>>>
>>>> - crtc_w = crtc->primary->fb->width - x;
>>>> - crtc_h = crtc->primary->fb->height - y;
>>>> + crtc_w = fb->width - x;
>>>> + crtc_h = fb->height - y;
>>>>
>>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>>> - x, y, crtc_w, crtc_h);
>>>> + ret = exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> @@ -279,6 +268,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
>>>>
>>>> private->crtc[exynos_crtc->pipe] = NULL;
>>>>
>>>> + crtc->primary->funcs->destroy(crtc->primary);
>>> This is unnecessary.
>>
>> The question is how these object should be destroyed. In current code
>> crtc is destroyed in fimd_unbind and it is called before
>> drm_mode_config_cleanup
>> which destroys all planes.
>> In such case primary plane will stay with .crtc pointing to non-existing
>> crtc.
>> Maybe performing crtcs cleanup after planes cleanup is better solution???
>
> I think it's wrong to call destroy function of crtc in fimd_unbind, all
> objects should be destroyed by drm_mode_config_cleanup except error
> cases while initializing.
>
>>
>>>
>>>> drm_crtc_cleanup(crtc);
>>>> kfree(exynos_crtc);
>>>> }
>>>> @@ -304,8 +294,7 @@ static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
>>>> exynos_drm_crtc_commit(crtc);
>>>> break;
>>>> case CRTC_MODE_BLANK:
>>>> - exynos_plane_dpms(exynos_crtc->plane,
>>>> - DRM_MODE_DPMS_OFF);
>>>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
>>>> break;
>>>> default:
>>>> break;
>>>> @@ -351,8 +340,10 @@ static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
>>>> int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>> {
>>>> struct exynos_drm_crtc *exynos_crtc;
>>>> + struct drm_plane *plane;
>>>> struct exynos_drm_private *private = manager->drm_dev->dev_private;
>>>> struct drm_crtc *crtc;
>>>> + int ret;
>>>>
>>>> exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
>>>> if (!exynos_crtc)
>>>> @@ -364,11 +355,11 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>> exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
>>>> exynos_crtc->manager = manager;
>>>> exynos_crtc->pipe = manager->pipe;
>>>> - exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
>>>> - 1 << manager->pipe, true);
>>>> - if (!exynos_crtc->plane) {
>>>> - kfree(exynos_crtc);
>>>> - return -ENOMEM;
>>>> + plane = exynos_plane_init(manager->drm_dev, 1 << manager->pipe,
>>>> + DRM_PLANE_TYPE_PRIMARY);
>>>> + if (IS_ERR(plane)) {
>>>> + ret = PTR_ERR(plane);
>>>> + goto err_plane;
>>>> }
>>>>
>>>> manager->crtc = &exynos_crtc->drm_crtc;
>>>> @@ -376,12 +367,22 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>>
>>>> private->crtc[manager->pipe] = crtc;
>>>>
>>>> - drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
>>>> + ret = drm_crtc_init_with_planes(manager->drm_dev, crtc, plane, NULL,
>>>> + &exynos_crtc_funcs);
>>>> + if (ret < 0)
>>>> + goto err_crtc;
>>>> +
>>>> drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
>>>>
>>>> exynos_drm_crtc_attach_mode_property(crtc);
>>>>
>>>> return 0;
>>>> +
>>>> +err_crtc:
>>>> + plane->funcs->destroy(plane);
>>>> +err_plane:
>>>> + kfree(exynos_crtc);
>>>> + return ret;
>>>> }
>>>>
>>>> int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>> index 9b00e4e..a439452 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>> @@ -86,8 +86,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
>>>> struct drm_plane *plane;
>>>> unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
>>>>
>>>> - plane = exynos_plane_init(dev, possible_crtcs, false);
>>>> - if (!plane)
>>>> + plane = exynos_plane_init(dev, possible_crtcs,
>>>> + DRM_PLANE_TYPE_OVERLAY);
>>>> + if (IS_ERR(plane))
>>>> goto err_mode_config_cleanup;
>>>> }
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> index 8371cbd..15e37a0 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> @@ -139,6 +139,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>>>> overlay->crtc_x, overlay->crtc_y,
>>>> overlay->crtc_width, overlay->crtc_height);
>>>>
>>>> + plane->crtc = crtc;
>>>> +
>>> OK, then we can remove same code from exynos_update_plane().
>>
>> Right.
>>
>>>
>>> One more, plane->crtc is NULL before mode_set or setplane so it's
>>> problem if call plane->funcs->destroy with plane->crtc == NULL.
>>> We need checking plane->crtc is NULL in exynos_disable_plane().
>>
>> I can simply add checks, but why we allow the plane with NULL crtc to be
>> enabled?
>>
>
> I mean plane disable case, not enable case.
OK, i am missing plane DPMS, the plane DPMS is not ON before mode_set
and setplane so plane->crtc is not referenced while disable plane.
Please ignore this comment.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API
2014-09-19 11:11 ` Joonyoung Shim
2014-09-19 11:29 ` Joonyoung Shim
@ 2014-09-19 11:51 ` Andrzej Hajda
1 sibling, 0 replies; 6+ messages in thread
From: Andrzej Hajda @ 2014-09-19 11:51 UTC (permalink / raw)
To: Joonyoung Shim, Inki Dae
Cc: Seung-Woo Kim, Kyungmin Park, David Airlie, Kukjin Kim,
open list:DRM DRIVERS FOR E...,
moderated list:ARM/S5P EXYNOS AR...,
open list, drake, daniel, m.szyprowski
On 09/19/2014 01:11 PM, Joonyoung Shim wrote:
> Hi,
>
> On 09/19/2014 07:54 PM, Andrzej Hajda wrote:
>> On 09/19/2014 03:02 AM, Joonyoung Shim wrote:
>>> Hi Andrzej,
>>>
>>> On 09/18/2014 10:17 PM, Andrzej Hajda wrote:
>>>> The patch replaces legacy functions
>>>> drm_plane_init() / drm_crtc_init() with
>>>> drm_universal_plane_init() and drm_crtc_init_with_planes().
>>>> It allows to replace fake primary plane with the real one.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> Hi Inki,
>>>>
>>>> I have tested this patch with trats board, it looks OK.
>>>> As a side effect it should solve fb refcounting issues
>>>> reported by me and Daniel Drake.
>>>>
>>>> Regards
>>>> Andrzej
>>>> ---
>>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++++++++++++++---------------
>>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++-
>>>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 17 +++++----
>>>> drivers/gpu/drm/exynos/exynos_drm_plane.h | 3 +-
>>>> 4 files changed, 47 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> index b68e58f..d2f713e 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> @@ -32,7 +32,6 @@ enum exynos_crtc_mode {
>>>> * Exynos specific crtc structure.
>>>> *
>>>> * @drm_crtc: crtc object.
>>>> - * @drm_plane: pointer of private plane object for this crtc
>>>> * @manager: the manager associated with this crtc
>>>> * @pipe: a crtc index created at load() with a new crtc object creation
>>>> * and the crtc object would be set to private->crtc array
>>>> @@ -46,7 +45,6 @@ enum exynos_crtc_mode {
>>>> */
>>>> struct exynos_drm_crtc {
>>>> struct drm_crtc drm_crtc;
>>>> - struct drm_plane *plane;
>>>> struct exynos_drm_manager *manager;
>>>> unsigned int pipe;
>>>> unsigned int dpms;
>>>> @@ -94,12 +92,12 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>>>>
>>>> exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>>>>
>>>> - exynos_plane_commit(exynos_crtc->plane);
>>>> + exynos_plane_commit(crtc->primary);
>>>>
>>>> if (manager->ops->commit)
>>>> manager->ops->commit(manager);
>>>>
>>>> - exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
>>>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
>>>> }
>>>>
>>>> static bool
>>>> @@ -123,10 +121,9 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>> {
>>>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>>> struct exynos_drm_manager *manager = exynos_crtc->manager;
>>>> - struct drm_plane *plane = exynos_crtc->plane;
>>>> + struct drm_framebuffer *fb = crtc->primary->fb;
>>>> unsigned int crtc_w;
>>>> unsigned int crtc_h;
>>>> - int ret;
>>>>
>>>> /*
>>>> * copy the mode data adjusted by mode_fixup() into crtc->mode
>>>> @@ -134,29 +131,21 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>> */
>>>> memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
>>>>
>>>> - crtc_w = crtc->primary->fb->width - x;
>>>> - crtc_h = crtc->primary->fb->height - y;
>>>> + crtc_w = fb->width - x;
>>>> + crtc_h = fb->height - y;
>>>>
>>>> if (manager->ops->mode_set)
>>>> manager->ops->mode_set(manager, &crtc->mode);
>>>>
>>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>>> - x, y, crtc_w, crtc_h);
>>>> - if (ret)
>>>> - return ret;
>>>> -
>>>> - plane->crtc = crtc;
>>>> - plane->fb = crtc->primary->fb;
>>>> - drm_framebuffer_reference(plane->fb);
>>>> -
>>>> - return 0;
>>>> + return exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>>> }
>>>>
>>>> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>>> struct drm_framebuffer *old_fb)
>>>> {
>>>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>>> - struct drm_plane *plane = exynos_crtc->plane;
>>>> + struct drm_framebuffer *fb = crtc->primary->fb;
>>>> unsigned int crtc_w;
>>>> unsigned int crtc_h;
>>>> int ret;
>>>> @@ -167,11 +156,11 @@ static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y,
>>>> return -EPERM;
>>>> }
>>>>
>>>> - crtc_w = crtc->primary->fb->width - x;
>>>> - crtc_h = crtc->primary->fb->height - y;
>>>> + crtc_w = fb->width - x;
>>>> + crtc_h = fb->height - y;
>>>>
>>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h,
>>>> - x, y, crtc_w, crtc_h);
>>>> + ret = exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
>>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> @@ -279,6 +268,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
>>>>
>>>> private->crtc[exynos_crtc->pipe] = NULL;
>>>>
>>>> + crtc->primary->funcs->destroy(crtc->primary);
>>> This is unnecessary.
>> The question is how these object should be destroyed. In current code
>> crtc is destroyed in fimd_unbind and it is called before
>> drm_mode_config_cleanup
>> which destroys all planes.
>> In such case primary plane will stay with .crtc pointing to non-existing
>> crtc.
>> Maybe performing crtcs cleanup after planes cleanup is better solution???
> I think it's wrong to call destroy function of crtc in fimd_unbind, all
> objects should be destroyed by drm_mode_config_cleanup except error
> cases while initializing.
Yes, it looks better this way. As I lurked in other drm drivers only
exynos and imx
destroys their objects directly, not via drm_mode_config_cleanup.
>
>>>> drm_crtc_cleanup(crtc);
>>>> kfree(exynos_crtc);
>>>> }
>>>> @@ -304,8 +294,7 @@ static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
>>>> exynos_drm_crtc_commit(crtc);
>>>> break;
>>>> case CRTC_MODE_BLANK:
>>>> - exynos_plane_dpms(exynos_crtc->plane,
>>>> - DRM_MODE_DPMS_OFF);
>>>> + exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
>>>> break;
>>>> default:
>>>> break;
>>>> @@ -351,8 +340,10 @@ static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
>>>> int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>> {
>>>> struct exynos_drm_crtc *exynos_crtc;
>>>> + struct drm_plane *plane;
>>>> struct exynos_drm_private *private = manager->drm_dev->dev_private;
>>>> struct drm_crtc *crtc;
>>>> + int ret;
>>>>
>>>> exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
>>>> if (!exynos_crtc)
>>>> @@ -364,11 +355,11 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>> exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
>>>> exynos_crtc->manager = manager;
>>>> exynos_crtc->pipe = manager->pipe;
>>>> - exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
>>>> - 1 << manager->pipe, true);
>>>> - if (!exynos_crtc->plane) {
>>>> - kfree(exynos_crtc);
>>>> - return -ENOMEM;
>>>> + plane = exynos_plane_init(manager->drm_dev, 1 << manager->pipe,
>>>> + DRM_PLANE_TYPE_PRIMARY);
>>>> + if (IS_ERR(plane)) {
>>>> + ret = PTR_ERR(plane);
>>>> + goto err_plane;
>>>> }
>>>>
>>>> manager->crtc = &exynos_crtc->drm_crtc;
>>>> @@ -376,12 +367,22 @@ int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
>>>>
>>>> private->crtc[manager->pipe] = crtc;
>>>>
>>>> - drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
>>>> + ret = drm_crtc_init_with_planes(manager->drm_dev, crtc, plane, NULL,
>>>> + &exynos_crtc_funcs);
>>>> + if (ret < 0)
>>>> + goto err_crtc;
>>>> +
>>>> drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
>>>>
>>>> exynos_drm_crtc_attach_mode_property(crtc);
>>>>
>>>> return 0;
>>>> +
>>>> +err_crtc:
>>>> + plane->funcs->destroy(plane);
>>>> +err_plane:
>>>> + kfree(exynos_crtc);
>>>> + return ret;
>>>> }
>>>>
>>>> int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>> index 9b00e4e..a439452 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>> @@ -86,8 +86,9 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
>>>> struct drm_plane *plane;
>>>> unsigned long possible_crtcs = (1 << MAX_CRTC) - 1;
>>>>
>>>> - plane = exynos_plane_init(dev, possible_crtcs, false);
>>>> - if (!plane)
>>>> + plane = exynos_plane_init(dev, possible_crtcs,
>>>> + DRM_PLANE_TYPE_OVERLAY);
>>>> + if (IS_ERR(plane))
>>>> goto err_mode_config_cleanup;
>>>> }
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> index 8371cbd..15e37a0 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> @@ -139,6 +139,8 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
>>>> overlay->crtc_x, overlay->crtc_y,
>>>> overlay->crtc_width, overlay->crtc_height);
>>>>
>>>> + plane->crtc = crtc;
>>>> +
>>> OK, then we can remove same code from exynos_update_plane().
>> Right.
>>
>>> One more, plane->crtc is NULL before mode_set or setplane so it's
>>> problem if call plane->funcs->destroy with plane->crtc == NULL.
>>> We need checking plane->crtc is NULL in exynos_disable_plane().
>> I can simply add checks, but why we allow the plane with NULL crtc to be
>> enabled?
>>
> I mean plane disable case, not enable case.
exynos_plane_dpms(off) calls exynos_drm_crtc_plane_disable only if
exynos_plane->enabled
is true, so only if plane changes state from enabled to disabled
exynos_drm_crtc_plane_disable
is called. I guess it should not be allowed that plane have .enabled
true and .crtc NULL, if so
we do not need checks here.
Regards
Andrzej
>
> Thanks.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-19 11:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-18 13:17 [PATCH] drm/exynos: switch to universal plane API Andrzej Hajda
2014-09-19 1:02 ` Joonyoung Shim
2014-09-19 10:54 ` Andrzej Hajda
2014-09-19 11:11 ` Joonyoung Shim
2014-09-19 11:29 ` Joonyoung Shim
2014-09-19 11:51 ` Andrzej Hajda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).