Re: [PATCH] drm/exynos: switch to universal plane API

From: Andrzej Hajda
Date: Fri Sep 19 2014 - 07:51:33 EST

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@xxxxxxxxxxx>
>>>> ---
>>>> 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.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/