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).
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.
> 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
