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

[Stéphane Marchesin]

On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae <inki.dae at samsung.com> wrote:

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

Well, I think everyone sees that exynos is different. But my point still
remains: why is the exynos driver in drm/ if it wants to use a different
framework? Right now it is blocking work on a proper drm driver...



>
> > 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?


I don't understand this sentence, sorry.

Stéphane



> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131022/722b4c30/attachment-0001.html>