-----Original Message----- From: Sean Paul [mailto:seanpaul@chromium.org] 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@chromium.org wrote:
On Thu, Oct 17, 2013 at 10:31 PM, Inki Dae inki.dae@samsung.com wrote:
-----Original Message----- From: Sean Paul [mailto:seanpaul@chromium.org] 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@samsung.com
wrote:
-----Original Message----- From: Sean Paul [mailto:seanpaul@chromium.org] Sent: Thursday, October 17, 2013 4:27 AM To: dri-devel@lists.freedesktop.org; inki.dae@samsung.com Cc: airlied@linux.ie; tomasz.figa@gmail.com; marcheu@chromium.org;
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@chromium.org
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? :) 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?
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 doesnt 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.
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