Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 09, 2016 at 09:19:22PM +0200, Noralf Trønnes wrote:
> 
> Den 05.05.2016 18:23, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
> >>Make drm_encoder_helper_funcs and it's functions optional to avoid
> >>having dummy functions.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >Please also update the kerneldoc and mention there that the enable/disable
> >hooks are optional. You can build the hmtl docs using
> 
> AFAICT the kerneldoc already says that they are optional for atomic helpers:
> 
> struct drm_encoder_helper_funcs {
> [...]
>         /**
>          * @disable:
> [...]
>          * This hook is used both by legacy CRTC helpers and atomic helpers.
>          * Atomic drivers don't need to implement it if there's no need to
>          * disable anything at the encoder level. To ensure that runtime PM
> [...]
>         /**
>          * @enable:
> [...]
>          * This hook is used only by atomic helpers, for symmetry with
> @disable.
>          * Atomic drivers don't need to implement it if there's no need to
>          * enable anything at the encoder level. To ensure that runtime PM
> handling

Indeed, docs not matching the code. But with your patch here will soon ;-)
I'll apply this one to drm-misc.
-Daniel

> 
> Noralf.
> 
> >$ make htmldocs
> >
> >to check the result. The vtables are documented in
> >include/drm/drm_helper_vtables.h
> >
> >>---
> >>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
> >>  drivers/gpu/drm/drm_crtc_helper.c   | 41 +++++++++++++++++++++++++++++--------
> >Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
> >part. Old drivers should converted to atomic, new drivers should just use
> >atomic. No need to touch that code at all.
> >-Daniel
> >
> >>  2 files changed, 42 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>index 92e11a2..03ea049 100644
> >>--- a/drivers/gpu/drm/drm_atomic_helper.c
> >>+++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>@@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >>  			continue;
> >>  		funcs = encoder->helper_private;
> >>+		if (!funcs)
> >>+			continue;
> >>  		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
> >>  				 encoder->base.id, encoder->name);
> >>@@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >>  			funcs->prepare(encoder);
> >>  		else if (funcs->disable)
> >>  			funcs->disable(encoder);
> >>-		else
> >>+		else if (funcs->dpms)
> >>  			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> >>  		drm_bridge_post_disable(encoder->bridge);
> >>@@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> >>  		encoder = connector->state->best_encoder;
> >>  		funcs = encoder->helper_private;
> >>+		if (!funcs)
> >>+			continue;
> >>+
> >>  		new_crtc_state = connector->state->crtc->state;
> >>  		mode = &new_crtc_state->mode;
> >>  		adjusted_mode = &new_crtc_state->adjusted_mode;
> >>@@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >>  		encoder = connector->state->best_encoder;
> >>  		funcs = encoder->helper_private;
> >>+		if (!funcs)
> >>+			continue;
> >>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> >>  				 encoder->base.id, encoder->name);
> >>@@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >>  		if (funcs->enable)
> >>  			funcs->enable(encoder);
> >>-		else
> >>+		else if (funcs->commit)
> >>  			funcs->commit(encoder);
> >>  		drm_bridge_enable(encoder->bridge);
> >>diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >>index 66ca313..6e6ab38 100644
> >>--- a/drivers/gpu/drm/drm_crtc_helper.c
> >>+++ b/drivers/gpu/drm/drm_crtc_helper.c
> >>@@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
> >>  {
> >>  	const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
> >>+	if (!encoder_funcs)
> >>+		return;
> >>+
> >>  	drm_bridge_disable(encoder->bridge);
> >>  	if (encoder_funcs->disable)
> >>  		(*encoder_funcs->disable)(encoder);
> >>-	else
> >>+	else if (encoder_funcs->dpms)
> >>  		(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
> >>  	drm_bridge_post_disable(encoder->bridge);
> >>@@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
> >>  	drm_for_each_encoder(encoder, dev) {
> >>  		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		/* Disable unused encoders */
> >>  		if (encoder->crtc == NULL)
> >>  			drm_encoder_disable(encoder);
> >>@@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		ret = drm_bridge_mode_fixup(encoder->bridge,
> >>  			mode, adjusted_mode);
> >>  		if (!ret) {
> >>@@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		drm_bridge_disable(encoder->bridge);
> >>-		encoder_funcs = encoder->helper_private;
> >>  		/* Disable the encoders as the first thing we do. */
> >>-		encoder_funcs->prepare(encoder);
> >>+		if (encoder_funcs->prepare)
> >>+			encoder_funcs->prepare(encoder);
> >>  		drm_bridge_post_disable(encoder->bridge);
> >>  	}
> >>@@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
> >>  			encoder->base.id, encoder->name,
> >>  			mode->base.id, mode->name);
> >>-		encoder_funcs = encoder->helper_private;
> >>-		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> >>+		if (encoder_funcs->mode_set)
> >>+			encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> >>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> >>  	}
> >>@@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		drm_bridge_pre_enable(encoder->bridge);
> >>-		encoder_funcs = encoder->helper_private;
> >>-		encoder_funcs->commit(encoder);
> >>+		if (encoder_funcs->commit)
> >>+			encoder_funcs->commit(encoder);
> >>  		drm_bridge_enable(encoder->bridge);
> >>  	}
> >>@@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
> >>  	struct drm_bridge *bridge = encoder->bridge;
> >>  	const struct drm_encoder_helper_funcs *encoder_funcs;
> >>+	encoder_funcs = encoder->helper_private;
> >>+	if (!encoder_funcs)
> >>+		return;
> >>+
> >>  	if (mode == DRM_MODE_DPMS_ON)
> >>  		drm_bridge_pre_enable(bridge);
> >>  	else
> >>  		drm_bridge_disable(bridge);
> >>-	encoder_funcs = encoder->helper_private;
> >>  	if (encoder_funcs->dpms)
> >>  		encoder_funcs->dpms(encoder, mode);
> >>-- 
> >>2.2.2
> >>
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch