From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v3 07/10] drm/exynos: Remove custom FB helper deferred setup Date: Tue, 21 Mar 2017 12:17:40 +0100 Message-ID: <20170321111740.GC30407@ulmo.ba.sec> References: <20170321081358.27237-1-thierry.reding@gmail.com> <20170321081358.27237-8-thierry.reding@gmail.com> <06a46f58-57b3-77e5-7434-8787981f0478@samsung.com> <20170321102049.w6dqxhu6nuudga4j@phenom.ffwll.local> <69858798-3b38-25e5-7624-e831d3d2ea32@samsung.com> <20170321105339.GA30407@ulmo.ba.sec> <89f5ae20-b589-b006-b405-46813358a269@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1199912748==" Return-path: In-Reply-To: <89f5ae20-b589-b006-b405-46813358a269@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Andrzej Hajda Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1199912748== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0lnxQi9hkpPO77W3" Content-Disposition: inline --0lnxQi9hkpPO77W3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 21, 2017 at 12:13:26PM +0100, Andrzej Hajda wrote: > On 21.03.2017 11:53, Thierry Reding wrote: > > On Tue, Mar 21, 2017 at 11:42:21AM +0100, Andrzej Hajda wrote: > >> On 21.03.2017 11:20, Daniel Vetter wrote: > >>> On Tue, Mar 21, 2017 at 10:58:43AM +0100, Andrzej Hajda wrote: > >>>> On 21.03.2017 09:13, Thierry Reding wrote: > >>>>> From: Thierry Reding > >>>>> > >>>>> The FB helper core now supports deferred setup, so the driver's cus= tom > >>>>> implementation can be removed. > >>>>> > >>>>> Signed-off-by: Thierry Reding > >>>>> --- > >>>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ++++-- > >>>>> drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 -- > >>>>> 2 files changed, 4 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/= drm/exynos/exynos_drm_drv.c > >>>>> index 09d3c4c3c858..c5a37dda8d1b 100644 > >>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > >>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > >>>>> @@ -399,8 +399,9 @@ static int exynos_drm_bind(struct device *dev) > >>>>> /* init kms poll for handling hpd */ > >>>>> drm_kms_helper_poll_init(drm); > >>>>> =20 > >>>>> - /* force connectors detection */ > >>>>> - drm_helper_hpd_irq_event(drm); > >>>>> + ret =3D exynos_drm_fbdev_init(dev); > >>>>> + if (ret) > >>>>> + goto err_cleanup_poll; > >>>> It should be rather: > >>>> ret =3D exynos_drm_fbdev_init(drm); > >>>> > >>>> Even with this change it does not work, I will try to track down the > >>>> problem. > >> The solution was to remove custom equivalent of > >> drm_fb_helper_maybe_connected - exynos_drm_fbdev_is_anything_connected, > >> it is called earlier and it was a part of custom deferred fbdev. > > Cool, thanks for figuring that out. I can add that to this patch for v4. > > > >>> We might need the multi-stage fbdev setup here, i.e. init fbdev, run > >>> hpd_irq_event() (I suspect that kicks all the connectors to start > >>> probing), then initial_config for fbdev. > >>> > >>> Probably simplest way to achieve this is to keep the hpd_irq_event ca= ll, > >>> but place it _after_ the fbdev_init. > >>> -Daniel > >> I wonder if it wouldn't be sufficient to check if there is anything > >> connected, instead of checking if there is anything not-disconnected, = in > >> drm_fb_helper_maybe_connected. > > My recollection is that this had been discussed when I first sent this > > out. VGA outputs are somewhat of a special case in that you can't always > > properly detect that it's connected or not. So effectively we need to > > take into account the connector_status_unknown. There's also some more > > information about this in the kerneldoc for enum drm_connector_status. >=20 > OK, I forgot about it and assumed that it is only used for initial state > of connector. > Returning to Daniel's proposition about hpd_irq_event(), it seems > unnecessary then: before calling drm_fb_helper_maybe_connected > drm_fb_helper_probe_connector_modes is called, which calls fill_modes > callback which should perform connector probing anyway, am I right? Yes, that matches my understanding of the call sequence. Thierry --0lnxQi9hkpPO77W3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAljRC9QACgkQ3SOs138+ s6GgCw//Tj6gcGJsnz8S0v283YnC6Q/j7ZEQSmAW8hZewS+RUL4MgnESAVjRhUlp kaxThdizlj1sKWgPY0/wBiGLMFoRH/CHeAmEcq9qBh7JUPAH430qI+r1nzt5Vw2y qwD22Fvv3WFp+f9rDfMe7BAdxHlp8ts4vJ1BVA0QDcfOAJckExNtlzDLPg+MQqq0 xWuDM6Hrm862Js/eHpKvRoXXMeJ8xlmT/Tks6dUccyu33CgKK26AU7qHMIT4l2KF d/RcghKryCUTwcHlHCVlJNK61G2OHSAdr32A5PU2jQbrD7zbkcNEOB4ZR3Dxg/HN qkHe8BwQ9/NDmqEhc68Mi+zYQiHHXQJazd/5nuWNwr65k45o7LFOkDTVUKOZMWmd Gkvtpwwxrv85JDgZrqoAerMa0JANBxRyy0ydscgGTkTLEKr37RWFcqGfSZqSk3yY B4SyhflTv3U217Z0++ZUKIn7nYs4F7aFChIwLPoJnYJ5TwAkfJ+oUxAi72nFkYuP Ibc0tgccufnAKe2H9vIUtNz/4qJhoJZw6z4WZFTdrQIRWwiLw5iYblQVB+CmMKqP z2K7HfkC/QGJySGcFfDfRWkSZY2xxiv8Pt+SoYAWxGiS7U7lAG7j/onDY++nUKgN wlPiDxeKDnff/FCTvLV2J82E5CnEi6PBSa+osi5a1VW6pj7ADS4= =Y20W -----END PGP SIGNATURE----- --0lnxQi9hkpPO77W3-- --===============1199912748== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1199912748==--