Hi Daniel,
On Wed, Aug 21, 2019 at 10:21:07AM +0200, Daniel Vetter wrote:
> On Tue, Aug 20, 2019 at 10:05:05PM +0300, Laurent Pinchart wrote:
> > 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 a mostly-good idea :-) DSI is a bit more troublesome as it often
requires enable/disable sequences that are a bit more entangled between
the source and the sink. That's why the omapdrm driver originally
controlled enabling from sink to source, it would locate the bridge (or
the omapdrm internal equivalent of bridge to be more accurate) at the
end of the chain, and enable it. The bridge would then be responsible
for forwarding the enable call to its source, broken in several
fine-grained calls for DSI. That allowed a DSI sink to implement
completely custom enable/disable sequences, and enable the source
accordingly (usually by first enabling clocks, then enabling data
transmission but without actual display data, and finally enabling
transfer of display data). The cost was a more complex enable/disable
implementation for each bridge.
I think the pre/post enable/disable operations we have now are mostly
good, but for some links they need to be manually controlled. Of course
I can't rule out a new display bus in the future that will shatter this
down to pieces by requiring more complex enable/disable sequences.
> Anyway, this here is imo not the right thing to do. If setting
> encoder->bridge to NULL doesn't work for you then just type your own
> enable/disable implementations. There's explicitly no requirement to use
> all parts of atomic helpers, au contraire, it's explicitly discouraged.
>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++---
> > > drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++-----------
> > > include/drm/drm_encoder.h | 9 +++++++
> > > 3 files changed, 42 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 4706439fb490..15ea61877122 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1030,7 +1030,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > > * Each encoder has at most one connector (since we always steal
> > > * it away), so we won't call disable hooks twice.
> > > */
> > > - drm_atomic_bridge_disable(encoder->bridge, old_state);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_atomic_bridge_disable(encoder->bridge, old_state);
> > >
> > > /* Right function depends upon target state. */
> > > if (funcs) {
> > > @@ -1044,7 +1045,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > > funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > > }
> > >
> > > - drm_atomic_bridge_post_disable(encoder->bridge, old_state);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_atomic_bridge_post_disable(encoder->bridge,
> > > + old_state);
> > > }
> > >
> > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> > > @@ -1342,7 +1345,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> > > * Each encoder has at most one connector (since we always steal
> > > * it away), so we won't call enable hooks twice.
> > > */
> > > - drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
> > >
> > > if (funcs) {
> > > if (funcs->atomic_enable)
> > > @@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> > > funcs->commit(encoder);
> > > }
> > >
> > > - drm_atomic_bridge_enable(encoder->bridge, old_state);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_atomic_bridge_enable(encoder->bridge, old_state);
> > > }
> > >
> > > drm_atomic_helper_commit_writebacks(dev, old_state);
> > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > > index fa3694836c22..87dae37fec12 100644
> > > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > > @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder)
> > > if (!encoder_funcs)
> > > return;
> > >
> > > - drm_bridge_disable(encoder->bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_bridge_disable(encoder->bridge);
> > >
> > > if (encoder_funcs->disable)
> > > (*encoder_funcs->disable)(encoder);
> > > else if (encoder_funcs->dpms)
> > > (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
> > >
> > > - drm_bridge_post_disable(encoder->bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_bridge_post_disable(encoder->bridge);
> > > }
> > >
> > > static void __drm_helper_disable_unused_functions(struct drm_device *dev)
> > > @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> > > if (!encoder_funcs)
> > > continue;
> > >
> > > - drm_bridge_disable(encoder->bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_bridge_disable(encoder->bridge);
> > >
> > > /* Disable the encoders as the first thing we do. */
> > > if (encoder_funcs->prepare)
> > > encoder_funcs->prepare(encoder);
> > >
> > > - drm_bridge_post_disable(encoder->bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_bridge_post_disable(encoder->bridge);
> > > }
> > >
> > > drm_crtc_prepare_encoders(dev);
> > > @@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> > > if (!encoder_funcs)
> > > continue;
> > >
> > > - drm_bridge_pre_enable(encoder->bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_bridge_pre_enable(encoder->bridge);
> > >
> > > if (encoder_funcs->commit)
> > > encoder_funcs->commit(encoder);
> > >
> > > - drm_bridge_enable(encoder->bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq)
> > > + drm_bridge_enable(encoder->bridge);
> > > }
> > >
> > > /* Calculate and store various constants which
> > > @@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
> > > if (!encoder_funcs)
> > > return;
> > >
> > > - if (mode == DRM_MODE_DPMS_ON)
> > > - drm_bridge_pre_enable(bridge);
> > > - else
> > > - drm_bridge_disable(bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq) {
> > > + if (mode == DRM_MODE_DPMS_ON)
> > > + drm_bridge_pre_enable(bridge);
> > > + else
> > > + drm_bridge_disable(bridge);
> > > + }
> > >
> > > if (encoder_funcs->dpms)
> > > encoder_funcs->dpms(encoder, mode);
> > >
> > > - if (mode == DRM_MODE_DPMS_ON)
> > > - drm_bridge_enable(bridge);
> > > - else
> > > - drm_bridge_post_disable(bridge);
> > > + if (!encoder->custom_bridge_enable_disable_seq) {
> > > + if (mode == DRM_MODE_DPMS_ON)
> > > + drm_bridge_enable(bridge);
> > > + else
> > > + drm_bridge_post_disable(bridge);
> > > + }
> > > }
> > >
> > > static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc)
> > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > index 70cfca03d812..d986ff1197ce 100644
> > > --- a/include/drm/drm_encoder.h
> > > +++ b/include/drm/drm_encoder.h
> > > @@ -175,6 +175,15 @@ struct drm_encoder {
> > > struct drm_bridge *bridge;
> > > const struct drm_encoder_funcs *funcs;
> > > const struct drm_encoder_helper_funcs *helper_private;
> > > +
> > > + /**
> > > + * @custom_bridge_enable_disable_seq: Set to true if the encoder needs
> > > + * a custom bridge/encoder enable/disable sequence. In that case the
> > > + * driver is responsible for calling the
> > > + * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}()
> > > + * functions as part of its encoder enable/disable handling.
> > > + */
> > > + uint32_t custom_bridge_enable_disable_seq : 1;
> > > };
> > >
> > > #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
