[PATCH RFC 02/19] drm: Support custom encoder/bridge enable/disable sequences officially

[Boris Brezillon]

On Wed, 21 Aug 2019 10:21:07 +0200
Daniel Vetter <daniel at ffwll.ch> 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.