On Wed, 21 Aug 2019 10:21:07 +0200 Daniel Vetter <daniel@xxxxxxxx> wrote: > On Tue, Aug 20, 2019 at 10:05:05PM +0300, Laurent Pinchart wrote: > > Hi Boris, > > > > Thank you for the patch. > > > > On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote: > > > The VC4 and Exynos DSI encoder drivers need a custom enable/disable > > > sequence and manually set encoder->bridge to NULL to prevent the core > > > from calling the bridge hooks. > > > > > > Let's provide a more official way to support custom bridge/encoder > > > enable/disable sequences. > > > > So, while I'm not opposed to this, I think it's a bit of a hack, and I'd > > like to share my vision of how I'd like to see this being implemented in > > the more distant future (and thus possibly on top of this change). > > I think it's also counter to how the atomic helpers are meant to be used. > I mean you're adding a pile new hooks here all for essentially not having > to type a few lines of helper code. If you look around at big drivers > (i915, amdgpu, nouveau) _none_ of them use the modesets_enables/disables > helpers. Exactly for reasons like this where you need a custom sequence. > > So proper recommendation is to just type your own, you'll be happier. > > > It has been established for quite some time now that exposing > > drm_encoder to userspace was likely a mistake, and that it should have > > stayed a kernel-only object. With the generalised usage of drm_bridge, I > > would go one step further: drm_encoder shouldn't matter for DRM/KMS > > drivers at all. > > > > drm_bridge has been introduced to model chained encoders, where the > > second (and subsequent) encoders couldn't easily be supported in a > > standard and reusable way with just drm_encoder. A bridge (with the > > exception of the panel bridge) is just an encoder. It shouldn't matter > > whether encoders are internal to the display controller, separate IPs in > > the SoC or external components, they could all be modelled as bridges. > > We do have bridges for DSI encoder IPs, and DSI (and other) bridges > > potentially need similar custom enable/disable sequences. I would thus > > ideally like to see the VC4 and Exynos DSI encoders being implemented as > > bridges, with support for custom enable/disable sequences in bridges, > > and drop support for custom enable/disable sequences in drm_encoder. > > > > Does this make sense to you ? While I would love to see this being > > implemented right away, it may be too much work as a prerequisite for > > this bus format negotiation series, so I don't want to reject this > > patch. I would however like to already make sure we agree on the way > > forward for the next steps. > > Yeah the other bit is that bridges are supposed to be some kind of > standard. I do wonder (I haven't looked at the series) whether the problem > here is that encoders don't have their own full set of pre/post enable > hooks like bridges (essentially that's what you're adding here), or > whether the idea behind pre/post enable/disable hooks is not good enough. It's just that we lack the pre/post enable hooks at the encoder level. But I'm addressing this limitation slightly differently in the v2 I'm working on. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
