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