On Thu, May 04, 2017 at 07:44:08AM +0200, Daniel Vetter wrote: > On Wed, May 3, 2017 at 6:17 PM, Eric Anholt <eric@xxxxxxxxxx> wrote: > > Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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 display > >>> >>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). > >>> >>> > >>> >>> There are kms drivers that use drm_panel, but don't have simple stub > >>> >>> connectors that wrap around a drm_panel. They have more complicated > >>> >>> connector ops, and may call drm_panel_prepare() and related functions > >>> >>> a bit differently. We won't be able to use drm_panel_bridge for those > >>> >>> drivers. > >>> >>> > >>> >>> For msm, we check whether the DSI encoder is connected directly to a > >>> >>> panel or an external bridge. If it's connected to an external bridge, > >>> >>> we skip the creation of the stub connector, and rely on the external > >>> >>> 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 unconditionally > >>> >> creating the panel bridge is probably even simpler, after all bridges > >>> >> are supposed to be chainable. I guess there's always going to be drivers > >>> >> 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_panel > >>> >> 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 panel > >>> >> drivers. > >>> > > >>> > As I've just explained in another reply, I don't see the point in doing > >>> > 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 nodes, it > >>> > makes sense to instantiate an object for them that can be handled by 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 definitely > >>> 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 introduced I > >> could have told you that converting all drivers was an impossible task ;-) > > 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 :-) > > >> Jokes aside, I believe it might be possible to implement something simple. 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 step use > >> drm_bridge as that base structure. We would only need to embed a drm_bridge > >> instance in drm_panel and add a few connector-related operations to the bridge > >> ops structure. As existing bridge drivers wouldn't need to provide those 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. > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
