From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751104AbcEJGyE (ORCPT ); Tue, 10 May 2016 02:54:04 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:37702 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750950AbcEJGyB (ORCPT ); Tue, 10 May 2016 02:54:01 -0400 Date: Tue, 10 May 2016 08:53:59 +0200 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: Daniel Vetter , dri-devel@lists.freedesktop.org, treding@nvidia.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional Message-ID: <20160510065359.GF27098@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org, treding@nvidia.com, linux-kernel@vger.kernel.org References: <1462454674-2246-1-git-send-email-noralf@tronnes.org> <1462454674-2246-3-git-send-email-noralf@tronnes.org> <20160505162359.GL1286@phenom.ffwll.local> <9791de12-06cf-1192-9ac4-5892644a60c6@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <9791de12-06cf-1192-9ac4-5892644a60c6@tronnes.org> X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >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