Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Archit Taneja <architt@codeaurora.org>]

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 <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  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(-)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 6119b5085501..e7799b6ee829 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -230,9 +230,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev,
>  	of_node_put(np);
>
>  	if (bridge) {
> -		output->encoder.bridge = bridge;
> -		bridge->encoder = &output->encoder;
> -		ret = drm_bridge_attach(dev, bridge);
> +		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
>  		if (!ret)
>  			return 0;
>  	}
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6e0447f329a2..1835f1fdad19 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1227,12 +1227,10 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev,
>
>  	dp->bridge = bridge;
>
> -	dp->encoder->bridge = bridge;
>  	bridge->driver_private = dp;
> -	bridge->encoder = dp->encoder;
>  	bridge->funcs = &analogix_dp_bridge_funcs;
>
> -	ret = drm_bridge_attach(drm_dev, bridge);
> +	ret = drm_bridge_attach(dp->encoder, bridge, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to attach drm bridge\n");
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index b71088dab268..432e0e3fff72 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1841,13 +1841,12 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
>  	hdmi->bridge = bridge;
>  	bridge->driver_private = hdmi;
>  	bridge->funcs = &dw_hdmi_bridge_funcs;
> -	ret = drm_bridge_attach(drm, bridge);
> +	ret = drm_bridge_attach(encoder, bridge, NULL);
>  	if (ret) {
>  		DRM_ERROR("Failed to initialize bridge with drm\n");
>  		return -EINVAL;
>  	}
>
> -	encoder->bridge = bridge;
>  	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>
>  	drm_connector_helper_add(&hdmi->connector,
> 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
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_encoder.h>
>
>  /**
>   * DOC: overview
> @@ -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;
		
> +
> +	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;
>  }

<snip>

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project