2013/10/23 Stéphane Marchesin <stephane.marchesin@xxxxxxxxx>: > > > > On Tue, Oct 22, 2013 at 9:15 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >> >> 2013/10/23 Stéphane Marchesin <stephane.marchesin@xxxxxxxxx>: >> > >> > >> > >> > On Tue, Oct 22, 2013 at 8:38 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >> >> >> >> 2013/10/23 Stéphane Marchesin <stephane.marchesin@xxxxxxxxx>: >> >> > >> >> > >> >> > >> >> > On Tue, Oct 22, 2013 at 7:28 PM, Inki Dae <inki.dae@xxxxxxxxxxx> >> >> > wrote: >> >> >> >> >> >> 2013/10/22 Sean Paul <seanpaul@xxxxxxxxxxxx>: >> >> >> > On Tue, Oct 22, 2013 at 1:30 AM, Inki Dae <inki.dae@xxxxxxxxxxx> >> >> >> > wrote: >> >> >> >> >> >> >> >> >> >> >> >>> -----Original Message----- >> >> >> >>> From: Sean Paul [mailto:seanpaul@xxxxxxxxxxxx] >> >> >> >>> 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@xxxxxxxxxxxx> >> >> >> >>> wrote: >> >> >> >>> > On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae >> >> >> >>> > <inki.dae@xxxxxxxxxxx> >> >> >> >>> > wrote: >> >> >> >>> >> >> >> >> >>> >> >> >> >> >>> >>> -----Original Message----- >> >> >> >>> >>> From: Sean Paul [mailto:seanpaul@xxxxxxxxxxxx] >> >> >> >>> >>> 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@xxxxxxxxxxx> >> >> >> >> wrote: >> >> >> >>> >>> > >> >> >> >>> >>> > >> >> >> >>> >>> >> -----Original Message----- >> >> >> >>> >>> >> From: Sean Paul [mailto:seanpaul@xxxxxxxxxxxx] >> >> >> >>> >>> >> Sent: Thursday, October 17, 2013 4:27 AM >> >> >> >>> >>> >> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; inki.dae@xxxxxxxxxxx >> >> >> >>> >>> >> Cc: airlied@xxxxxxxx; tomasz.figa@xxxxxxxxx; >> >> >> >>> >>> >> marcheu@xxxxxxxxxxxx; >> >> >> >>> 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@xxxxxxxxxxxx> >> >> >> >>> >>> >> --- >> >> >> >>> >>> >> >> >> >> >>> >>> >> 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... >> > >> >> Noooooo. It's not to use a different framework. It's to use a wrapper >> instead. > > > Ok, if you want to call it a wrapper, then what is the point of doing this > wrapping given that it prevents a proper drm-style implementation? > I already commented. That is for only Exynos drm framework has dependency of drm framework directly, and Exynos drm based drivers include only Exynos drm headers. > >> >> > >> >> >> >> >> >> > 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. >> >> Sorry, again, you mean Exynos drm based drivers should be in >> drivers/gpu/drm, not drivers/gpu/drm/exynos? >> > Is the exynos drm useful in its current shape at all? My recommendation > would be to fork off a real drm driver in gpu/drm/exynos with the current > code as a base. > Yes as of now. of course, There could be a better way. However, I don't want for Exynos drm based drivers have dependency of drm framework directly. Thanks for your opinions. Inki Dae > Stéphane > > >> >> Thanks, >> Inki Dae >> >> > >> > 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@xxxxxxxxxxxxxxxxxxxxx >> >> >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> >> _______________________________________________ >> >> >> dri-devel mailing list >> >> >> dri-devel@xxxxxxxxxxxxxxxxxxxxx >> >> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> > >> >> > >> >> > >> >> > _______________________________________________ >> >> > dri-devel mailing list >> >> > dri-devel@xxxxxxxxxxxxxxxxxxxxx >> >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> > >> > >> > >> > >> > _______________________________________________ >> > dri-devel mailing list >> > dri-devel@xxxxxxxxxxxxxxxxxxxxx >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > > > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
