From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752203AbdEDMft (ORCPT ); Thu, 4 May 2017 08:35:49 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:35889 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750832AbdEDMfm (ORCPT ); Thu, 4 May 2017 08:35:42 -0400 Date: Thu, 4 May 2017 14:35:39 +0200 From: Thierry Reding To: Daniel Vetter Cc: Eric Anholt , Linux Kernel Mailing List , dri-devel , Yannick Fertre , Laurent Pinchart , Thierry Reding Subject: Re: [PATCH 1/2] drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge. Message-ID: <20170504123539.GA2031@ulmo.ba.sec> References: <20170427163601.7313-1-eric@anholt.net> <2556350.BYUa4rMt48@avalon> <20170503142856.bmazihqoj6rgvbwq@phenom.ffwll.local> <2463758.9HmFMyJbcQ@avalon> <87tw51zzhp.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="liOOAslEiF7prFVr" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --liOOAslEiF7prFVr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, May 04, 2017 at 07:44:08AM +0200, Daniel Vetter wrote: > On Wed, May 3, 2017 at 6:17 PM, Eric Anholt wrote: > > Laurent Pinchart writes: > > > >> Hi Daniel, > >> > >> On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote: > >>> On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: > >>> > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: > >>> >> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > >>> >>> +panel/bridge reviewers. > >>> >>> > >>> >>> This does make things much cleaner, but it seems a bit strange to > >>> >>> create a drm_bridge when there isn't really a HW bridge in the di= splay > >>> >>> chain (i.e, when the DSI encoder is directly connected to a DSI p= anel). > >>> >>> > >>> >>> There are kms drivers that use drm_panel, but don't have simple s= tub > >>> >>> connectors that wrap around a drm_panel. They have more complicat= ed > >>> >>> connector ops, and may call drm_panel_prepare() and related funct= ions > >>> >>> a bit differently. We won't be able to use drm_panel_bridge for t= hose > >>> >>> drivers. > >>> >>> > >>> >>> For msm, we check whether the DSI encoder is connected directly t= o a > >>> >>> panel or an external bridge. If it's connected to an external bri= dge, > >>> >>> we skip the creation of the stub connector, and rely on the exter= nal > >>> >>> bridge driver to create the connector: > >>> >>> > >>> >>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.= c#L22 > >>> >>> 7 > >>> >>> > >>> >>> The msm solution isn't very neat, but it avoids the need to create > >>> >>> another bridge to glue things together. > >>> >> > >>> >> Since I suggested this, yes I like it. And I think just unconditio= nally > >>> >> creating the panel bridge is probably even simpler, after all brid= ges > >>> >> are supposed to be chainable. I guess there's always going to be d= rivers > >>> >> where we need special handling, but I'm kinda hoping that for most= cases > >>> >> simply plugging in a panel bridge is all that's need to glue drm_p= anel > >>> >> support into a driver. The simple pipe helpers do support bridges,= and > >>> >> part of the goal there very much was to make it easy to glue in pa= nel > >>> >> drivers. > >>> > > >>> > As I've just explained in another reply, I don't see the point in d= oing > >>> > this when we can instead refactor the bridge and panel operations to > >>> > expose a common base object that will then be easy to handle in core > >>> > code. This isn't just for panels, as connectors should have DT node= s, it > >>> > makes sense to instantiate an object for them that can be handled b= y the > >>> > DRM core, without having to push connector handling in all bridge > >>> > drivers. > >>> > >>> Imo you're aiming too high. We have 21 bridge drivers and 11 panel > >>> drivers. Asking someone to refactor them all (plus all the callers and > >>> everything) means it won't happen. At least I personally will definit= ely > >>> not block a contribution on this happening, that's a totally outsized > >>> demand. > >> > >> I think you're aiming too low. When the atomic update API was introduc= ed I > >> could have told you that converting all drivers was an impossible task= ;-) >=20 > We still have non-atomic drivers. We will have non-atomic drivers for > a _very_ long time. Heck, we still have non-kms drivers in drm. And > especially with atomic we've started out with some helpers to glue the > new world into the old one, not by refactoring existing interfaces. > That's somewhat starting to happen now, 2 years and 20+ drivers later. > You've just proven my point I think :-) >=20 > >> Jokes aside, I believe it might be possible to implement something sim= ple. I'm > >> flexible about the naming, so instead of creating a new base structure= and > >> refactor drm_bridge and drm_panel to embed it, we could as a first ste= p use > >> drm_bridge as that base structure. We would only need to embed a drm_b= ridge > >> instance in drm_panel and add a few connector-related operations to th= e bridge > >> ops structure. As existing bridge drivers wouldn't need to provide tho= se new > >> ops, they wouldn't need to be touched. > > > > I think drm_bridge itself should be the base structure, as it is today. > > It's got the entrypoints we want, connected in at the right level in the > > modeset chain. You could call it the "drm_display_chain_link" or > > something, but "bridge" is short and I don't think renaming is worth the > > trouble. This helper just makes it easy to make one of these display > > chain links for the endpoint of your display chain when it's a > > drm_panel. > > > > One of the followons I'd like to see after this is to make a helper that > > drivers get to use that does the "grab a bridge? no? grab a panel? > > Ok? wrap it in a bridge" pattern. Then the drm_panel/drm_connector are > > an implementation detail of whatever is at the end of the chain, and not > > something that the middle of the chain needs to know about. > > > > The alternative to that that I see would be to have panel drivers call > > this code to advertise themselves through the list of bridges. >=20 > Yeah, I do believe that Eric's series here is actual the right (first, > among a lot more) steps into the direction of unifying panels and > bridges. Atm, if you want to have this shiny new unified world you get > to deal with n bridges, m panels and bunch of callers for each. With > Eric's work here we can get the n:m out of this refactoring problem by > making panels looks like bridges for everyone outside at least. And > then, once that part is done (we might, or might not need to clarify > something in the bridge interface), we can then push that bridge > interface down, or at least embed it into drm_panel and unify them. > But at that point it's only a 1:m issue (hopefully at least). I'm not sure I understand what the goal here is, or what you think we'd gain. Bridges and panels are fundamentally different things. One sits between an encoder and a connector, the other sits behind the connector. The only reason the interfaces have some resemblance is because they are fairly simple things that just need a two step initialization and two step deinitialization. Why would you want to unify them? I think that complicates things because you'd have to special case depending on whether the "bridge" is behind an encoder or behind a connector. That said, I think this helper makes sense for cases where the standard handling is used and where we otherwise would be using a tightly coupled encoder/connector pair. Thierry --liOOAslEiF7prFVr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlkLIBMACgkQ3SOs138+ s6HwRBAAr8/rs+oCOGJhkiBvGP3xYeQwgqvr7769NMPYsPWmJOse353Wzzd8/CMl /40IbrjvsoHQwcp0nR/bQgTlFROuOQVgG6jW9qsZRmFhiKLE88DmsvYdiVz2SRfd v/eca2ifjFH7/XzW0UgzREoXh0W9R8EgaNhDRVTxz/dX4aH8XDOIGhX8yfPQ+6WU Iv6FnoykQmyPhDK/w7egTUloTlrRtFb/mr4sD92UExmhYTtTzwJGRUCHaYozRRdW ZjpiGSuP689sinrCr0XVHPJgiVQ5T6sB67YXgl/Srk9IbjCXPWOvj4TU6LFAh/TY 8PxAOrD/mcDM5iidUGsTL2zVfq3t1B9lB3vw8WM2mrbNjvjMDxwznMGmCrc/W64D 4NLoIQlr5hUPHECZ1V/c5BS/gP0YcvM8gbM5Ana+hxEi259DkiLdlEOJ8oNcDRh6 INTsn1zx75cYLRK7QMd4xZi2P40zfoPd5+zAMvbtDUUCyd8uNMO9iEDZfaDwGlJl ZNF32QqMr6bsy0O3UlpDabvmhTVY8LphBHO5PrRzIJZrrKsQie5dCI0ajwqlqZ2q hrpngv1bCs6kBKLG7pXq/ui0ycyL3ffXCte0oHK1e8WJNMmxK+/4S0uBKkt7HwJi Fpm/EDj9JiZCoAHq258yBwEI7nxerMSkC9hXDDrsg/zbK+2nAS4= =K4HR -----END PGP SIGNATURE----- --liOOAslEiF7prFVr--