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