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.
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.
Cheers, Daniel
>
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
