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>
