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 at 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
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
>>
