<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae <<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">2013/10/23 Stéphane Marchesin <<a href="mailto:stephane.marchesin@gmail.com">stephane.marchesin@gmail.com</a>>: <div><div class="h5">> > > > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>> wrote: >> >> 2013/10/22 Sean Paul <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>>: >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>> wrote: >> >> >> >> >> >>> -----Original Message----- >> >>> From: Sean Paul [mailto:<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>] >> >>> 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 <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>> >> >>> wrote: >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>> >> >>> > wrote: >> >>> >> >> >>> >> >> >>> >>> -----Original Message----- >> >>> >>> From: Sean Paul [mailto:<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>] >> >>> >>> 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 <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>> >> >> wrote: >> >>> >>> > >> >>> >>> > >> >>> >>> >> -----Original Message----- >> >>> >>> >> From: Sean Paul [mailto:<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>] >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM >> >>> >>> >> To: <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a>; <a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a> >> >>> >>> >> Cc: <a href="mailto:airlied@linux.ie">airlied@linux.ie</a>; <a href="mailto:tomasz.figa@gmail.com">tomasz.figa@gmail.com</a>; >> >>> >>> >> <a href="mailto:marcheu@chromium.org">marcheu@chromium.org</a>; >> >>> 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 <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>> >> >>> >>> >> --- >> >>> >>> >> >> >>> >>> >> 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 >> >>> >> >>> <a href="https://github.com/crseanpaul/exynos-drm-next/commits/linux-next-exynos-" target="_blank">https://github.com/crseanpaul/exynos-drm-next/commits/linux-next-exynos-</a> >> >>> 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, </div></div>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. </blockquote><div> </div><div>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...</div> <div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> > 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. </div>So those drivers should be in drm/exynos. Isn't that you really mean those drivers should be driver/gpu/drm?</blockquote><div> </div><div>I don't understand this sentence, sorry.</div><div> </div><div><div>Stéphane </div></div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> If so, That would really be horrible. :( </blockquote><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> 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 <div class=""><div class="h5"> > > 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 >> > >> > (<a href="https://github.com/crseanpaul/exynos-drm-next/blob/linux-next-exynos-staging/drivers/gpu/drm/exynos/exynos_drm_encoder.c" target="_blank">https://github.com/crseanpaul/exynos-drm-next/blob/linux-next-exynos-staging/drivers/gpu/drm/exynos/exynos_drm_encoder.c</a>), >> > 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 >> > <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> >> > <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> >> _______________________________________________ >> dri-devel mailing list >> <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> >> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> > > > > _______________________________________________ > dri-devel mailing list > <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> > </div></div></blockquote></div> </div></div>