[PATCH v2 12/26] drm/exynos: Split manager/display/subdrv

[Inki Dae]

2013/10/23 Stéphane Marchesin <stephane.marchesin at gmail.com>:
>
>
>
> On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae <inki.dae at samsung.com> wrote:
>>
>> 2013/10/22 Sean Paul <seanpaul at chromium.org>:
>> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae <inki.dae at samsung.com> wrote:
>> >>
>> >>
>> >>> -----Original Message-----
>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> Sent: Tuesday, October 22, 2013 6:18 AM
>> >>> To: Inki Dae
>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; Stéphane Marchesin
>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv
>> >>>
>> >>> On Mon, Oct 21, 2013 at 10:46 AM, Sean Paul <seanpaul at chromium.org>
>> >>> wrote:
>> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae <inki.dae at samsung.com>
>> >>> > wrote:
>> >>> >>
>> >>> >>
>> >>> >>> -----Original Message-----
>> >>> >>> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> >>> Sent: Thursday, October 17, 2013 11:37 PM
>> >>> >>> To: Inki Dae
>> >>> >>> Cc: dri-devel; Dave Airlie; Tomasz Figa; Stéphane Marchesin
>> >>> >>> Subject: Re: [PATCH v2 12/26] drm/exynos: Split
>> >>> >>> manager/display/subdrv
>> >>> >>>
>> >>> >>> On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae <inki.dae at samsung.com>
>> >> wrote:
>> >>> >>> >
>> >>> >>> >
>> >>> >>> >> -----Original Message-----
>> >>> >>> >> From: Sean Paul [mailto:seanpaul at chromium.org]
>> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM
>> >>> >>> >> To: dri-devel at lists.freedesktop.org; inki.dae at samsung.com
>> >>> >>> >> Cc: airlied at linux.ie; tomasz.figa at gmail.com;
>> >>> >>> >> marcheu at chromium.org;
>> >>> Sean
>> >>> >>> >> Paul
>> >>> >>> >> Subject: [PATCH v2 12/26] drm/exynos: Split
>> >>> >>> >> manager/display/subdrv
>> >>> >>> >>
>> >>> >>> >> This patch splits display and manager from subdrv. The result
>> >>> >>> >> is
>> >>> that
>> >>> >>> >> crtc functions can directly call into manager callbacks and
>> >>> >>> >> encoder
>> >>> >>> >> functions can directly call into display callbacks. This will
>> >>> >>> >> allow
>> >>> >>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi &
>> >>> fimd/dp
>> >>> >>> >> with common code.
>> >>> >>> >>
>> >>> >>> >> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>> >>> >>> >> ---
>> >>> >>> >>
>> >>> >>> >> Changes in v2:
>> >>> >>> >>       - Pass display into display_ops instead of context
>> >>> >>> >
>> >>> >>> > Sorry but it seems like more reasonable to pass device object
>> >>> >>> > into
>> >>> >>> > display_ops and manager_ops.
>> >>> >>> >
>> >>> >>>
>> >>> >>>
>> >>> >>> So you've changed your mind from when you said the following?
>> >>> >>>
>> >>> >>> >>> manager->ops->xxx(manager, ...);
>> >>> >>> >>> display->ops->xxx(display, ...);
>> >>> >>> >>>
>> >>> >>> >>> Agree.
>> >>> >>>
>> >>> >>
>> >>> >>
>> >>> >> True. Before that, My comment was to pass device object into
>> >>> display_ops and
>> >>> >> manager_ops, and then you said the good solution is to pass manager
>> >>> >> and
>> >>> >> display to each driver. At that time, I thought no matter how the
>> >>> callback
>> >>> >> is called if the framework doesn't call callbacks of each driver
>> >>> >> with
>> >>> ctx.
>> >>> >> So I agreed.
>> >>> >>
>> >>> >>
>> >>> >>> It would have been nice if you had changed your mind *before* I
>> >>> >>> reworked everything. At any rate, I think it's still the right
>> >>> >>> thing
>> >>> >>> to do.
>> >>> >>
>> >>> >> Really sorry about that. And I will add new patch for it so you
>> >>> >> don't
>> >>> need
>> >>> >> to concern about that.
>> >>> >>
>> >>> >>>
>> >>> >>>
>> >>> >>> > I'm not sure but display_ops could be implemented in other
>> >>> >>> > framework
>> >>> >>> based
>> >>> >>> > driver such as CDF based lcd panel driver. So if you pass
>> >>> >>> > display -
>> >>> it's
>> >>> >>> > specific to exynos drm framework - into display_ops, the other
>> >>> framework
>> >>> >>> > based driver should include specific exynos drm header.
>> >>> >>> >
>> >>> >>>
>> >>> >>> AFAIK, CDF will not land in its current separate-from-drm form, we
>> >>> >>> don't need to worry about this. Furthermore, these ops should just
>> >>> >>> go
>> >>> >>> away and become drm_crtc/drm_encoder/drm_connector funcs anyways.
>> >>> >>>
>> >>> >>
>> >>> >> Can you assure the display_ops never implemented in other framework
>> >>> based
>> >>> >> driver, not CDF? At any rate, I think all possibilities should be
>> >>> opened.
>> >>> >>
>> >>> >
>> >>> > I don't think we should let an RFC framework make the code more
>> >>> > complicated for unclear benefit. By removing manager/display
>> >>> > entirely,
>> >>> > we can get rid of a *lot* of other code that is basically just
>> >>> > plumbing drm hooks (exynos_drm_connector is a good example).
>> >>> >
>> >>>
>> >>> I hacked this up today to prove it out. Check out the top 5 commits in
>> >>>
>> >>> https://github.com/crseanpaul/exynos-drm-next/commits/linux-next-exynos-
>> >>> staging.
>> >>> It removes exynos_drm_connector in favor of just implementing
>> >>> drm_connector directly. This same treatment should be done for
>> >>> exynos_drm_encoder and exynos_drm_crtc, but I didn't get around to
>> >>> doing this.
>> >>>
>> >>> As you can see, it cuts out a lot of code and removes an entire
>> >>> abstraction layer. Much nicer :)
>> >>>
>> >>
>> >> It seems that you implements connector in each device driver. Can't
>> >> they be
>> >> combined as common spot, exynos_connector, again to avoid codes from
>> >> duplicated? :)
>> >
>> > There's nothing of substance being duplicated.
>>
>> Not true. xxx_create_connector is duplicated.
>>
>> > In fact, by getting rid
>> > of the exynos_drm_connector layer, we deleted 150 lines. If you really
>> > take a look at exynos_drm_connector, it's not doing anything useful.
>>
>> No, That is for each driver has no any dependency of drm framework.
>>
>> > All it does is translate the drm callbacks into display callbacks, so
>> > I think it's much better to just implement the drm callbacks directly.
>> >
>>
>> No, It has strongly dependency of drm framework. Assume that we
>> implemented the drm callbacks directly, and then some features are
>> added to drm framework, drm_connector side. At this time, we will have
>> to take care of each device driver according to the change. That is
>> really not good. Why device drivers should have dependency of drm
>> framework? Just to reduce line counts?
>
>
>
> You seem to miss the point here and elsewhere in the discussion.
> drm/exynos is a drm driver, and as such it should use the drm
> framework,

Hm.. you seem to miss something. Exynos drm based drivers are based on
exynos drm framework, not drm framework directly. So I mean that
Exynos drm framework based drivers should include only Exynos drm
headers, _not drm header_ directly.

> especially if this reduces the line count and the code
> complexity (as is the case for this patch series). If you don't want
> to maintain a drm driver, it simply should be moved away from drm/,
> and it should be replaced by a real drm driver in my opinion.

So those drivers should be in drm/exynos. Isn't that you really mean
those drivers should be driver/gpu/drm? If so, That would really be
horrible. :(

Please, know that only Exynos drm framework, _not device drivers_, has
all dependencies of drm framework, and also I know that other ARM
based drm drivers are using same way.

Thanks,
Inki Dae

>
> Stéphane
>
>>
>>
>> > There are a bunch of real bugs that we've found as a result of having
>> > these abstraction layers. Take, for example, dpms. Before this
>> > patchset, dpms for fimd was being tracked separately in fimd driver,
>> > exynos_drm_encoder, exynos_drm_crtc, and exynos_drm_connector.
>> > Furthermore, during suspend, only fimd driver's dpms state was
>> > updated, so the others were incorrect. There was also this weird
>> > gymnastics that had to happen when dpms was changed in the encoder
>> > since it had to walk up to the connector level to change its dpms
>> > state. If fimd just directly implemented
>> > drm_crtc/drm_encoder/drm_connector (before dp was moved in), this
>> > problem wouldn't exist. The same goes for HDMI/mixer.
>> >
>>
>> That is a issue we should take care of by using the independent layer.
>> Then, aren't you take care of that well with the re-factoring patch
>> set? :)  It seems that you are outside real point.
>>
>> > Take a look at exynos_drm_encoder.c  in my tree
>> >
>> > (https://github.com/crseanpaul/exynos-drm-next/blob/linux-next-exynos-staging/drivers/gpu/drm/exynos/exynos_drm_encoder.c),
>> > what does it do that's useful to abstract? All that it does is just
>> > call display ops, it's completely useless. The same is true for
>> > exynos_drm_connector, it's just dead weight. There is some useful
>> > stuff in exynos_drm_crtc for page flipping, that would be better
>> > served as a helper library, though.
>> >
>> >> The abstraction layer you mentioned also means a common spot.
>> >> Another one, you patch also makes each sub driver have strongly
>> >> dependency
>> >> of drm framework. So how we can support existing backlight and lcd
>> >> class
>> >> based lcd panel drivers if the connector is implemented in each device
>> >> driver later?  the drm header files should be included in
>> >> drivers/video/backlight/xxx_lcd.c?
>> >>
>> >
>> > drm_bridge or drm_panel seem like good candidates for this.
>> >
>>
>> Yes, exynos_drm_display could be replaced with drm_panel later if the
>> drm_panel can be merged to mainline.
>>
>> >
>> >> And, I will introduce a new framework to support existing lcd panel
>> >> drivers
>> >> and display bus drivers soon; as of now for Exynos drm, and the
>> >> framework is
>> >> being tested internally. With this framework, encoder and connector
>> >> will be
>> >> created when lcd panel or display bus driver such as eDP is probed: it
>> >> doesn’t really need to create encoder and connector in advance if lcd
>> >> panel
>> >> or display bus driver isn't probed yet. Regardless of crtc, and encoder
>> >> and
>> >> connector creation order, when last one is created, crtc and connector
>> >> will
>> >> be connected each other. And exynos_drm_display could be implemented in
>> >> other frameworks if we have common structure for display device driver.
>> >> And
>> >> also the framework will support lvds driver according to Linux device
>> >> driver
>> >> model.
>> >>
>> >
>> > I don't really follow what you're trying to do here, but I think we
>> > should be moving in the direction of fewer abstractions in the exynos
>> > driver, not more :)
>> >
>>
>> Not abstraction layer, just a bridge for connecting crtc and its
>> corresponding encoder/connector, and lvds regardless of creation
>> order, and for connecting drm connector and other framework based
>> display ops such as drm_panel later.
>>
>> > Sean
>> >
>> >
>> >
>> >> Thanks,
>> >> Inki Dae
>> >>
>> >>> Sean
>> >>>
>> >>> >>>
>> >>> >>> > And another one, the patch 6 passes manager object to
>> >>> >>> > manager_ops,
>> >>> and
>> >>> >>> for
>> >>> >>> > this, you made the manager object to be set to driver data;
>> >>> >>> > platform_set_drvdata(pdev, &manager). That isn't reasonable.
>> >>> Generally,
>> >>> >>> > driver_data would point to device driver's context object.
>> >>> >>> >
>> >>> >>>
>> >>> >>> I'm not sure why this isn't reasonable, but it's a moot point. The
>> >>> >>> driver data is only used up until we get rid of the pm ops, it
>> >>> >>> needn't
>> >>> >>> be set at all once things go through dpms.
>> >>> >>>
>> >>> >>
>> >>> >> Generally, device drivers can call its own internal functions, and
>> >>> >> they
>> >>> will
>> >>> >> call that functions with ctx. However, if you set manager to
>> >>> driver_data
>> >>> >> then that functions should be called with manager object and also
>> >>> internally
>> >>> >> that functions should get ctx from the manager object. What is the
>> >>> purpose
>> >>> >> of manager? Do you think it's reasonable?
>> >>> >>
>> >>> >
>> >>> > So, to avoid setting the manager as the drvdata, we could implement
>> >>> > something like fimd_dpms_ctx(ctx, mode) that takes ctx and the
>> >>> > manager
>> >>> > callback calls it fimd_dpms(mgr, mode) { ctx = mgr->ctx;
>> >>> > fimd_dpms_ctx(ctx, mode); }. Alternatively, you can save a pointer
>> >>> > to
>> >>> > mgr in ctx, but that creates a circular link between the two. IMO,
>> >>> > both of those solutions suck :)
>> >>> >
>> >>> > I'd much rather just set drvdata to the manager and call the hook
>> >>> > directly. Like I said earlier, this is just a temporary step since
>> >>> > we
>> >>> > remove these pm ops later in the patch series.
>> >>> >
>> >>> > Sean
>> >>> >
>> >>> >
>> >>> >> Anyway, I'd like to say really sorry about inconvenient again. So I
>> >>> will fix
>> >>> >> it.
>> >>> >>
>> >>> >> Thanks,
>> >>> >> Inki Dae
>> >>> >>
>> >>> >>> Sean
>> >>> >>>
>> >>> >>>
>> >>> >>> > Thanks,
>> >>> >>> > Inki Dae
>> >>> >>> >
>> >>> >>
>> >>
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>