* [PATCH] drm/exynos: switch to universal plane API @ 2014-09-18 13:17 ` Andrzej Hajda 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
* [PATCH] drm/exynos: switch to universal plane API @ 2014-09-18 13:17 ` Andrzej Hajda 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API 2014-09-18 13:17 ` Andrzej Hajda (?) @ 2014-09-19 1:02 ` Joonyoung Shim 2014-09-19 10:54 ` Andrzej Hajda -1 siblings, 1 reply; 9+ 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] 9+ 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; 9+ 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] 9+ 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 0 siblings, 0 replies; 9+ messages in thread From: Joonyoung Shim @ 2014-09-19 11:11 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 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] 9+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API @ 2014-09-19 11:11 ` Joonyoung Shim 0 siblings, 0 replies; 9+ 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] 9+ 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 -1 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API 2014-09-19 11:11 ` Joonyoung Shim @ 2014-09-19 11:51 ` Andrzej Hajda -1 siblings, 0 replies; 9+ messages in thread From: Andrzej Hajda @ 2014-09-19 11:51 UTC (permalink / raw) To: Joonyoung Shim, 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 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] 9+ messages in thread
* Re: [PATCH] drm/exynos: switch to universal plane API @ 2014-09-19 11:51 ` Andrzej Hajda 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2014-09-19 11:51 UTC | newest] Thread overview: 9+ 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-18 13:17 ` 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:11 ` Joonyoung Shim 2014-09-19 11:29 ` Joonyoung Shim 2014-09-19 11:51 ` Andrzej Hajda 2014-09-19 11:51 ` Andrzej Hajda
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.