2013/10/23 Sean Paul <seanpaul@xxxxxxxxxxxx>: > On Wed, Oct 23, 2013 at 1:42 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >> 2013/10/23 Sean Paul <seanpaul@xxxxxxxxxxxx>: >>> On Wed, Oct 23, 2013 at 12:48 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >>>> 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. >>>> >>> >>> Just to satisfy my curiosity, do you actually have something that uses >>> these drivers outside of drm? >>> >> >> Never no. >> >> >>> So I think we've reached somewhat of an impasse. I'd like to move the >>> driver towards a proper drm driver (ie: no exynos >>> framework/wrapper/whatever), you'd like to keep things separate. So >>> should we create a new exynos driver drivers/drm/gpu/exynos5 to house >>> the drm driver? >> >> Did you check out other drm drivers using similar way? And If so, the >> other drm framework based drivers should be moved? > > No, I haven't looked at other ARM drivers. It's besides the point, Look at omapdrm, nouveau, and radeon drm drivers. > really, that's not a compelling argument. For the exynos driver, in > particular, it doesn't make sense to wrapping everything in > exynos_drm_* That is really not thing to make sense or not. If you think this wrapping doesn't make sense, then can you argue about that the above drivers; omapdrm, nouveau, and radeon don't also make sense? That would definitely be just their style. If this design has some problem so the above drm maintainers also want to change their design, then I will merge your patch set. > >> What is the problem >> that Exynos drm based drivers include only Exynos drm headers, not drm >> framework header directly? Is that really a big issue? I _cannot_ >> understand your behavior. > > The problem is that it makes things unnecessarily complex for no benefit. > > For example, that you wanted to implement > connector_funcs->set_property() just in the hdmi driver. This requires > that you add a new display_op in exynos_drm_drv.h, then you implement > the connector_func callback in exynos_drm_connector to check if the op > is implemented then call it, and then you implement the set_property > hook in the hdmi driver. If you didn't have the wrapper, you just > implement the hook in the hdmi driver. > As I mentioned earlier, display_ops is needed to have no any dependency of drm framework directly like below, DRM Framework | Exynos DRM Framework / | \ Real device drivers In particular, in case of ARM based DRM drivers with separated devices, I still tend to think it's better design than that device drivers implement the connector callbacks directly, but I will try to consider what is the better way. Anyway, can you fix the build error to vidi module? I cannot merge your re-factoring patch set to exynos-drm-next. Thanks, Inki Dae > Sean > > >> Anyway, if there are any reasons that Exynos >> drm based drivers should necessary include drm framework header >> directly, I will agree it. >> >> Thanks, >> Inki Dae >> >>> >>> Sean >>> >>> >>>> 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 >>> _______________________________________________ >>> 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
