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 :) 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
