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

[Daniel Vetter <daniel@ffwll.ch>]

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@collabora.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel