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