On Tue, Oct 22, 2013 at 10: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. > I said nothing *of substance*, all of the other drm drivers call drm_connector_init for each display type. >> 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. > I didn't think this was desirable :) >> 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? > Assuming someone adds something to drm_connector, it's their responsibility to bring all other drivers forward with that change. Device drivers should have a dependency on drm framework because that's the API that everyone on this list has agreed to use. Why invent something else? What's the point of having independent hooks that almost identically mirror the drm framework? >> 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. > Right, I took out the connector and encoder dpms state, but it still exists in crtc. I was trying to point out the class of issues that comes from this type of shim. >> 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. > Well, exynos_drm_display can be replaced today by drm_encoder/drm_connector. >> >>> 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. > I guess we see things differently. It seems like you want to do a bunch of stuff that is outside of the drm framework, I assumed that you wanted this to be a proper drm driver. Not surprisingly, I'm with Stephane on this, I'd like for exynos to be a first-class drm driver, and that can't happen with these homebrew interfaces. Sean >> 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
