From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: RE: [PATCH 12/23] drm/exynos: Split manager/display/subdrv Date: Sat, 12 Oct 2013 13:30:32 +0900 Message-ID: <019401cec703$c9c6c780$5d545680$%dae@samsung.com> References: <1381451436-10089-1-git-send-email-seanpaul@chromium.org> <1381451436-10089-13-git-send-email-seanpaul@chromium.org> <019301cec6fd$fde74220$f9b5c660$%dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mailout4.samsung.com (mailout4.samsung.com [203.254.224.34]) by gabe.freedesktop.org (Postfix) with ESMTP id 014D3E5F12 for ; Fri, 11 Oct 2013 21:30:34 -0700 (PDT) Received: from epcpsbgr4.samsung.com (u144.gpu120.samsung.co.kr [203.254.230.144]) by mailout4.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MUJ00E8FGIWP780@mailout4.samsung.com> for dri-devel@lists.freedesktop.org; Sat, 12 Oct 2013 13:30:32 +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 mailing list' List-Id: dri-devel@lists.freedesktop.org > -----Original Message----- > From: Sean Paul [mailto:seanpaul@chromium.org] > Sent: Saturday, October 12, 2013 1:20 PM > To: Inki Dae > Cc: DRI mailing list; St=E9phane Marchesin; Dave Airlie; Tomasz Figa > Subject: Re: [PATCH 12/23] drm/exynos: Split manager/display/subdrv > = > On Fri, Oct 11, 2013 at 11:49 PM, Inki Dae wrote: > > > > > >> -----Original Message----- > >> From: Sean Paul [mailto:seanpaul@chromium.org] > >> Sent: Saturday, October 12, 2013 11:07 AM > >> To: Inki Dae > >> Cc: DRI mailing list; St=E9phane Marchesin; Dave Airlie; Tomasz Figa > >> Subject: Re: [PATCH 12/23] drm/exynos: Split manager/display/subdrv > >> > >> On Fri, Oct 11, 2013 at 8:42 PM, Inki Dae wrote: > >> >> static int exynos_drm_create_enc_conn(struct drm_device *dev, > >> >> - struct exynos_drm_subdrv > > *subdrv) > >> >> + struct exynos_drm_display > > *display) > >> >> { > >> >> struct drm_encoder *encoder; > >> >> struct drm_connector *connector; > >> >> + struct exynos_drm_manager *manager; > >> >> int ret; > >> >> + unsigned long possible_crtcs =3D 0; > >> >> > >> >> - subdrv->manager->dev =3D subdrv->dev; > >> >> + /* Find possible crtcs for this display */ > >> >> + list_for_each_entry(manager, &exynos_drm_manager_list, list) > >> >> + if (manager->type =3D=3D display->type) > >> >> + possible_crtcs |=3D 1 << manager->pipe; > >> >> > >> >> /* create and initialize a encoder for this sub driver. */ > >> >> - encoder =3D exynos_drm_encoder_create(dev, subdrv->manager, > >> >> - (1 << MAX_CRTC) - 1); > >> >> + encoder =3D exynos_drm_encoder_create(dev, display, > >> possible_crtcs); > >> >> if (!encoder) { > >> >> DRM_ERROR("failed to create encoder\n"); > >> >> return -EFAULT; > >> >> } > >> >> - subdrv->encoder =3D encoder; > >> >> + display->encoder =3D encoder; > >> >> > >> >> - if (subdrv->manager->display_ops->type =3D=3D > >> EXYNOS_DISPLAY_TYPE_LCD) { > >> >> + if (display->type =3D=3D EXYNOS_DISPLAY_TYPE_LCD) { > >> >> ret =3D exynos_drm_attach_lcd_bridge(dev, encoder) > >> > > >> > Plz, _do_not_base_ on top of your lvds bridge patch set. As I > >> > mentioned before, the tricky codes are not good. For this, let's find > >> > a better way after your refactoring patch set are reviewed enough, > and > >> > if merged. > >> > > >> > >> [+adding back the people you removed from CC] > >> > > > > Hm.. I didn't really remove the people from CC. See the your original > > email. :) > > > > > >> I'm happy to change how that works, but I've yet to find a better > >> solution. Instead of just dismissing something as "tricky codes", can > >> you suggest a concrete solution that is better than what I've got? > >> > > > > I think I mentioned already at previous email. First of all, let's have > a > > thorough review to your refactoring patch set, and then let's find a > better > > way. > > > > I believe that we can find a better way with your refactoring patch set: > now > > we can implement encoder and connector directly. > > > = > The code will look the exact same, it'll just be in the dp driver > instead of drm_core (since drm_core will not create > encoders/connectors). Adding the ptn driver in the interim allows me > to boot exynos5250-snow with linux-next, so I can test my changes. > = > = > > > >> I'm also a bit confused as to why you think it's so tricky, it's a > > > > First, That was your comment, "Right, so this is kind of tricky". And > also > > it seems like tricky codes to me. > > > = > Right, it was a tricky problem (for reasons I've stated multiple > times), but the solution is dead easy to understand. > = > = > >> synchronous initialization call to a driver; that's about as simple as > >> it gets. > > > > As you mentioned before, ptn3460 driver is not depended on SoC, and can > be > > used for other SoC. Let you try to think about lcd class and backlight > > class. I think we need a interfacing layer between common driver and > > specific something for connecting SoC specific DRM framework. > > > = > Laurent mentioned that he wanted to come up with a way to model > graphics hardware in dt. He could then build a notifier when the > various pieces were successfully probed, and drm would only load once > all of the pieces were ready. > = > I think that would be a nice long-term solution, however let's not let > the perfect be the enemy of the good. If you can simplify how this > works, I'm all for it. However, I don't see any other way at the > moment and I don't think it's productive to just wait around for > something better to come along. > = Sean, let's have reviews to your refactoring patch set first. And then I will merge your lvds bridge patch if we cannot find a better way. That would not take much time. > Sean > = > = > >> > >> Sean > >> > >> > >> > >> >> if (ret) > >> >> return 0; > >> >> @@ -91,7 +98,7 @@ static int exynos_drm_create_enc_conn(struct > >> drm_device *dev, > >> >> goto err_destroy_encoder; > >> >> } > >> >> > >> >> - subdrv->connector =3D connector; > >> >> + display->connector =3D connector; > >> >> > >> >> return 0; > >> >> > >> >> @@ -100,21 +107,6 @@ err_destroy_encoder: > >> >> return ret; > >> >> } > >> >> > >