From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:34091 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755361AbcK2R5H (ORCPT ); Tue, 29 Nov 2016 12:57:07 -0500 From: Laurent Pinchart To: Archit Taneja Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Boris Brezillon , Jingoo Han , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Stefan Agner , Alison Wang , Xinliang Liu , Rongrong Zou , Xinwei Kong , Chen Feng , Philipp Zabel , CK Hu , Rob Clark , Benjamin Gaignard , Vincent Abriou , Maxime Ripard Subject: Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code Date: Tue, 29 Nov 2016 19:57:20 +0200 Message-ID: <3126195.dNH0p7KEJE@avalon> In-Reply-To: <94e6b51f-71bf-8734-a843-e236ea1e8ff1@codeaurora.org> References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1480410283-28698-4-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <94e6b51f-71bf-8734-a843-e236ea1e8ff1@codeaurora.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: Hi Archit, On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote: > On 11/29/2016 02:34 PM, Laurent Pinchart wrote: > > Instead of linking encoders and bridges in every driver (and getting it > > wrong half of the time, as many drivers forget to set the drm_bridge > > encoder pointer), do so in core code. The drm_bridge_attach() function > > needs the encoder and optional previous bridge to perform that task, > > update all the callers. > > > > Signed-off-by: Laurent Pinchart > > > > --- > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 4 +- > > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 4 +- > > drivers/gpu/drm/bridge/dw-hdmi.c | 3 +- > > drivers/gpu/drm/drm_bridge.c | 46 ++++++++++++----- > > drivers/gpu/drm/drm_simple_kms_helper.c | 4 +- > > drivers/gpu/drm/exynos/exynos_dp.c | 5 +-- > > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 6 +-- > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 5 +-- > > drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 5 +-- > > drivers/gpu/drm/imx/imx-ldb.c | 6 +-- > > drivers/gpu/drm/imx/parallel-display.c | 4 +- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 8 ++-- > > drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++--------- > > drivers/gpu/drm/mediatek/mtk_hdmi.c | 11 +++--- > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 17 +++++--- > > drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +- > > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- > > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c | 5 +-- > > drivers/gpu/drm/sti/sti_dvo.c | 3 +- > > drivers/gpu/drm/sti/sti_hda.c | 3 +- > > drivers/gpu/drm/sti/sti_hdmi.c | 3 +- > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 13 +++--- > > include/drm/drm_bridge.h | 3 +- > > 23 files changed, 83 insertions(+), 103 deletions(-) [snip] > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index 0ee052b7c21a..850bd6509ef1 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c [snip] > > @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge) > > EXPORT_SYMBOL(drm_bridge_remove); > > > > /** > > - * drm_bridge_attach - associate given bridge to our DRM device > > + * drm_bridge_attach - attach the bridge to an encoder's chain > > * > > - * @dev: DRM device > > - * @bridge: bridge control structure > > + * @encoder: DRM encoder > > + * @bridge: bridge to attach > > + * @previous: previous bridge in the chain (optional) > > * > > - * Called by a kms driver to link one of our encoder/bridge to the given > > - * bridge. > > + * Called by a kms driver to link the bridge to an encoder's chain. The > > previous > > + * argument specifies the previous bridge in the chain. If NULL, the > > bridge is > > + * linked directly at the encoder's output. Otherwise it is linked at the > > + * previous bridge's output. > > * > > - * Note that setting up links between the bridge and our encoder/bridge > > - * objects needs to be handled by the kms driver itself. > > + * If non-NULL the previous bridge must be already attached by a call to > > this > > + * function. > > * > > * RETURNS: > > * Zero on success, error code on failure > > */ > > -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge) > > +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge > > *bridge, > > + struct drm_bridge *previous) > > { > > - if (!dev || !bridge) > > + int ret; > > + > > + if (!encoder || !bridge) > > + return -EINVAL; > > I think we could derive previous from the encoder itself. Something like: > > previous = encoder->bridge; > while (previous && previous->next) > previous = previous->next; That's a very good point. It would however prevent us from catching drivers that attach bridges in the wrong order, which the !previous->dev currently allows us to do (and it should be turned into a WARN_ON as Daniel proposed). I'm fine losing that ability, as your proposal makes the API simpler. I'll let you decide, which option do you prefer ? > > + > > + if (previous && (!previous->dev || previous->encoder != encoder)) > > return -EINVAL; > > > > if (bridge->dev) > > return -EBUSY; > > > > - bridge->dev = dev; > > + bridge->dev = encoder->dev; > > + bridge->encoder = encoder; > > + > > + if (bridge->funcs->attach) { > > + ret = bridge->funcs->attach(bridge); > > + if (ret < 0) { > > + bridge->dev = NULL; > > + bridge->encoder = NULL; > > + return ret; > > + } > > + } > > > > - if (bridge->funcs->attach) > > - return bridge->funcs->attach(bridge); > > + if (previous) > > + previous->next = bridge; > > + else > > + encoder->bridge = bridge; > > > > return 0; > > } > > -- Regards, Laurent Pinchart