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@xxxxxxxxxxx>
> >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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
