From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: RE: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv Date: Fri, 18 Oct 2013 11:31:39 +0900 Message-ID: <016f01cecbaa$2cc4fff0$864effd0$%dae@samsung.com> References: <1381951616-12548-1-git-send-email-seanpaul@chromium.org> <1381951616-12548-13-git-send-email-seanpaul@chromium.org> <00fb01cecb11$db9cec20$92d6c460$%dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mailout1.samsung.com (mailout1.samsung.com [203.254.224.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 89249E5F28 for ; Thu, 17 Oct 2013 19:31:41 -0700 (PDT) Received: from epcpsbgr1.samsung.com (u141.gpu120.samsung.co.kr [203.254.230.141]) by mailout1.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MUU00MO3F0OB3J0@mailout1.samsung.com> for dri-devel@lists.freedesktop.org; Fri, 18 Oct 2013 11:31:40 +0900 (KST) In-reply-to: Content-language: ko List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: 'Sean Paul' Cc: =?ISO-8859-1?Q?'St=E9phane_Marchesin'?= , 'dri-devel' List-Id: dri-devel@lists.freedesktop.org > -----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=E9phane Marchesin > Subject: Re: [PATCH v2 12/26] drm/exynos: Split manager/display/subdrv > = > On Thu, Oct 17, 2013 at 4:21 AM, Inki Dae 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 > >> --- > >> > >> 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. > = > > 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? Anyway, I'd like to say really sorry about inconvenient again. So I will fix it. = Thanks, Inki Dae > Sean > = > = > > Thanks, > > Inki Dae > >