Re: [PATCH 7/7] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

[Neil Armstrong]

Hi Sam,

On 14/10/2021 20:15, Sam Ravnborg wrote:
> Hi Neil,
> 
> with include order fixed and the comment below considered:
> Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> 
> 	Sam
> 
> 
> On Thu, Oct 14, 2021 at 05:26:06PM +0200, Neil Armstrong wrote:
>> Drop the local connector and move all callback to bridge funcs in order
>> to leverage the generic CVBS diplay connector.
>>
>> This will also permit adding custom cvbs connectors for ADC based HPD
>> detection on some Amlogic SoC based boards.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/meson/meson_encoder_cvbs.c | 178 +++++++++------------
>>  1 file changed, 79 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>> index 01024c5f610c..0b974667cf55 100644
>> --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>> +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>> @@ -13,6 +13,9 @@
>>  #include <linux/of_graph.h>
>>  
>>  #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +#include <drm/drm_bridge.h>
>> +#include <drm/drm_bridge_connector.h>
>>  #include <drm/drm_device.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_probe_helper.h>
> alphabetic order.
> 
>> @@ -30,14 +33,14 @@
>>  
>>  struct meson_encoder_cvbs {
>>  	struct drm_encoder	encoder;
>> -	struct drm_connector	connector;
>> +	struct drm_bridge	bridge;
>> +	struct drm_bridge	*next_bridge;
>> +	struct drm_connector	*connector;
> Maybe drop this - see later.
> 
>>  	struct meson_drm	*priv;
>>  };
>> -#define encoder_to_meson_encoder_cvbs(x) \
>> -	container_of(x, struct meson_encoder_cvbs, encoder)
>>  
>> -#define connector_to_meson_encoder_cvbs(x) \
>> -	container_of(x, struct meson_encoder_cvbs, connector)
>> +#define bridge_to_meson_encoder_cvbs(x) \
>> +	container_of(x, struct meson_encoder_cvbs, bridge)
>>  
>>  /* Supported Modes */
>>  
>> @@ -81,21 +84,18 @@ meson_cvbs_get_mode(const struct drm_display_mode *req_mode)
>>  	return NULL;
>>  }
>>  
>> -/* Connector */
>> -
>> -static void meson_cvbs_connector_destroy(struct drm_connector *connector)
>> +static int meson_encoder_cvbs_attach(struct drm_bridge *bridge,
>> +				     enum drm_bridge_attach_flags flags)
>>  {
>> -	drm_connector_cleanup(connector);
>> -}
>> +	struct meson_encoder_cvbs *meson_encoder_cvbs =
>> +					bridge_to_meson_encoder_cvbs(bridge);
>>  
>> -static enum drm_connector_status
>> -meson_cvbs_connector_detect(struct drm_connector *connector, bool force)
>> -{
>> -	/* FIXME: Add load-detect or jack-detect if possible */
>> -	return connector_status_connected;
>> +	return drm_bridge_attach(bridge->encoder, meson_encoder_cvbs->next_bridge,
>> +				 &meson_encoder_cvbs->bridge, flags);
>>  }
>>  
>> -static int meson_cvbs_connector_get_modes(struct drm_connector *connector)
>> +static int meson_encoder_cvbs_get_modes(struct drm_bridge *bridge,
>> +					struct drm_connector *connector)
>>  {
>>  	struct drm_device *dev = connector->dev;
>>  	struct drm_display_mode *mode;
>> @@ -116,40 +116,18 @@ static int meson_cvbs_connector_get_modes(struct drm_connector *connector)
>>  	return i;
>>  }
>>  
>> -static int meson_cvbs_connector_mode_valid(struct drm_connector *connector,
>> -					   struct drm_display_mode *mode)
>> +static int meson_encoder_cvbs_mode_valid(struct drm_bridge *bridge,
>> +					const struct drm_display_info *display_info,
>> +					const struct drm_display_mode *mode)
>>  {
>> -	/* Validate the modes added in get_modes */
>> -	return MODE_OK;
>> -}
>> -
>> -static const struct drm_connector_funcs meson_cvbs_connector_funcs = {
>> -	.detect			= meson_cvbs_connector_detect,
>> -	.fill_modes		= drm_helper_probe_single_connector_modes,
>> -	.destroy		= meson_cvbs_connector_destroy,
>> -	.reset			= drm_atomic_helper_connector_reset,
>> -	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
>> -	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>> -};
>> -
>> -static const
>> -struct drm_connector_helper_funcs meson_cvbs_connector_helper_funcs = {
>> -	.get_modes	= meson_cvbs_connector_get_modes,
>> -	.mode_valid	= meson_cvbs_connector_mode_valid,
>> -};
>> +	if (meson_cvbs_get_mode(mode))
>> +		return MODE_OK;
>>  
>> -/* Encoder */
>> -
>> -static void meson_encoder_cvbs_encoder_destroy(struct drm_encoder *encoder)
>> -{
>> -	drm_encoder_cleanup(encoder);
>> +	return MODE_BAD;
>>  }
>>  
>> -static const struct drm_encoder_funcs meson_encoder_cvbs_encoder_funcs = {
>> -	.destroy        = meson_encoder_cvbs_encoder_destroy,
>> -};
>> -
>> -static int meson_encoder_cvbs_encoder_atomic_check(struct drm_encoder *encoder,
>> +static int meson_encoder_cvbs_atomic_check(struct drm_bridge *bridge,
>> +					struct drm_bridge_state *bridge_state,
>>  					struct drm_crtc_state *crtc_state,
>>  					struct drm_connector_state *conn_state)
>>  {
>> @@ -159,10 +137,10 @@ static int meson_encoder_cvbs_encoder_atomic_check(struct drm_encoder *encoder,
>>  	return -EINVAL;
>>  }
>>  
>> -static void meson_encoder_cvbs_encoder_disable(struct drm_encoder *encoder)
>> +static void meson_encoder_cvbs_disable(struct drm_bridge *bridge)
>>  {
>>  	struct meson_encoder_cvbs *meson_encoder_cvbs =
>> -					encoder_to_meson_encoder_cvbs(encoder);
>> +					bridge_to_meson_encoder_cvbs(bridge);
>>  	struct meson_drm *priv = meson_encoder_cvbs->priv;
>>  
>>  	/* Disable CVBS VDAC */
>> @@ -175,10 +153,10 @@ static void meson_encoder_cvbs_encoder_disable(struct drm_encoder *encoder)
>>  	}
>>  }
>>  
>> -static void meson_encoder_cvbs_encoder_enable(struct drm_encoder *encoder)
>> +static void meson_encoder_cvbs_enable(struct drm_bridge *bridge)
>>  {
>>  	struct meson_encoder_cvbs *meson_encoder_cvbs =
>> -					encoder_to_meson_encoder_cvbs(encoder);
>> +					bridge_to_meson_encoder_cvbs(bridge);
>>  	struct meson_drm *priv = meson_encoder_cvbs->priv;
>>  
>>  	/* VDAC0 source is not from ATV */
>> @@ -198,13 +176,13 @@ static void meson_encoder_cvbs_encoder_enable(struct drm_encoder *encoder)
>>  	}
>>  }
>>  
>> -static void meson_encoder_cvbs_encoder_mode_set(struct drm_encoder *encoder,
>> -				   struct drm_display_mode *mode,
>> -				   struct drm_display_mode *adjusted_mode)
>> +static void meson_encoder_cvbs_mode_set(struct drm_bridge *bridge,
>> +				   const struct drm_display_mode *mode,
>> +				   const struct drm_display_mode *adjusted_mode)
>>  {
>>  	const struct meson_cvbs_mode *meson_mode = meson_cvbs_get_mode(mode);
>>  	struct meson_encoder_cvbs *meson_encoder_cvbs =
>> -					encoder_to_meson_encoder_cvbs(encoder);
>> +					bridge_to_meson_encoder_cvbs(bridge);
>>  	struct meson_drm *priv = meson_encoder_cvbs->priv;
>>  
>>  	if (meson_mode) {
>> @@ -218,76 +196,78 @@ static void meson_encoder_cvbs_encoder_mode_set(struct drm_encoder *encoder,
>>  	}
>>  }
>>  
>> -static const struct drm_encoder_helper_funcs
>> -				meson_encoder_cvbs_encoder_helper_funcs = {
>> -	.atomic_check	= meson_encoder_cvbs_encoder_atomic_check,
>> -	.disable	= meson_encoder_cvbs_encoder_disable,
>> -	.enable		= meson_encoder_cvbs_encoder_enable,
>> -	.mode_set	= meson_encoder_cvbs_encoder_mode_set,
>> +static const struct drm_bridge_funcs meson_encoder_cvbs_bridge_funcs = {
>> +	.attach = meson_encoder_cvbs_attach,
>> +	.enable	= meson_encoder_cvbs_enable,
>> +	.disable = meson_encoder_cvbs_disable,
>> +	.mode_valid = meson_encoder_cvbs_mode_valid,
>> +	.mode_set = meson_encoder_cvbs_mode_set,
>> +	.get_modes = meson_encoder_cvbs_get_modes,
>> +	.atomic_check = meson_encoder_cvbs_atomic_check,
>>  };
>>  
>> -static bool meson_encoder_cvbs_connector_is_available(struct meson_drm *priv)
>> -{
>> -	struct device_node *remote;
>> -
>> -	remote = of_graph_get_remote_node(priv->dev->of_node, 0, 0);
>> -	if (!remote)
>> -		return false;
>> -
>> -	of_node_put(remote);
>> -	return true;
>> -}
>> -
>>  int meson_encoder_cvbs_init(struct meson_drm *priv)
>>  {
>>  	struct drm_device *drm = priv->drm;
>>  	struct meson_encoder_cvbs *meson_encoder_cvbs;
>> -	struct drm_connector *connector;
>> -	struct drm_encoder *encoder;
>> +	struct device_node *remote;
>>  	int ret;
>>  
>> -	if (!meson_encoder_cvbs_connector_is_available(priv)) {
>> +	meson_encoder_cvbs = devm_kzalloc(priv->dev, sizeof(*meson_encoder_cvbs), GFP_KERNEL);
>> +	if (!meson_encoder_cvbs)
>> +		return -ENOMEM;
>> +
>> +	/* CVBS Connector Bridge */
>> +	remote = of_graph_get_remote_node(priv->dev->of_node, 0, 0);
>> +	if (!remote) {
>>  		dev_info(drm->dev, "CVBS Output connector not available\n");
>>  		return 0;
>>  	}
>>  
>> -	meson_encoder_cvbs = devm_kzalloc(priv->dev, sizeof(*meson_encoder_cvbs),
>> -				       GFP_KERNEL);
>> -	if (!meson_encoder_cvbs)
>> -		return -ENOMEM;
>> +	meson_encoder_cvbs->next_bridge = of_drm_find_bridge(remote);
>> +	if (!meson_encoder_cvbs->next_bridge) {
>> +		dev_err(priv->dev, "Failed to find CVBS Connector bridge\n");
>> +		return -EPROBE_DEFER;
>> +	}
>>  
>> -	meson_encoder_cvbs->priv = priv;
>> -	encoder = &meson_encoder_cvbs->encoder;
>> -	connector = &meson_encoder_cvbs->connector;
>> +	/* CVBS Encoder Bridge */
>> +	meson_encoder_cvbs->bridge.funcs = &meson_encoder_cvbs_bridge_funcs;
>> +	meson_encoder_cvbs->bridge.of_node = priv->dev->of_node;
>> +	meson_encoder_cvbs->bridge.type = DRM_MODE_CONNECTOR_Composite;
>> +	meson_encoder_cvbs->bridge.ops = DRM_BRIDGE_OP_MODES;
>> +	meson_encoder_cvbs->bridge.interlace_allowed = true;
>>  
>> -	/* Connector */
>> +	drm_bridge_add(&meson_encoder_cvbs->bridge);
>>  
>> -	drm_connector_helper_add(connector,
>> -				 &meson_cvbs_connector_helper_funcs);
>> +	meson_encoder_cvbs->priv = priv;
>>  
>> -	ret = drm_connector_init(drm, connector, &meson_cvbs_connector_funcs,
>> -				 DRM_MODE_CONNECTOR_Composite);
>> +	/* Encoder */
>> +	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_cvbs->encoder,
>> +				      DRM_MODE_ENCODER_TVDAC);
>>  	if (ret) {
>> -		dev_err(priv->dev, "Failed to init CVBS connector\n");
>> +		dev_err(priv->dev, "Failed to init CVBS encoder: %d\n", ret);
>>  		return ret;
>>  	}
>>  
>> -	connector->interlace_allowed = 1;
>> +	meson_encoder_cvbs->encoder.possible_crtcs = BIT(0);
>>  
>> -	/* Encoder */
>> -
>> -	drm_encoder_helper_add(encoder, &meson_encoder_cvbs_encoder_helper_funcs);
>> -
>> -	ret = drm_encoder_init(drm, encoder, &meson_encoder_cvbs_encoder_funcs,
>> -			       DRM_MODE_ENCODER_TVDAC, "meson_encoder_cvbs");
>> +	/* Attach CVBS Encoder Bridge to Encoder */
>> +	ret = drm_bridge_attach(&meson_encoder_cvbs->encoder, &meson_encoder_cvbs->bridge, NULL,
>> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>  	if (ret) {
>> -		dev_err(priv->dev, "Failed to init CVBS encoder\n");
>> +		dev_err(priv->dev, "Failed to attach bridge: %d\n", ret);
>>  		return ret;
>>  	}
>>  
>> -	encoder->possible_crtcs = BIT(0);
>> -
>> -	drm_connector_attach_encoder(connector, encoder);
>> +	/* Initialize & attach Bridge Connector */
>> +	meson_encoder_cvbs->connector = drm_bridge_connector_init(priv->drm,
>> +							&meson_encoder_cvbs->encoder);
> I did not see other uses of meson_encoder_cvbs->connector, so if I am
> right a local variable can be used and the member dropped.

You're right, I'll drop this.

Thanks
Neil

> 
>> +	if (IS_ERR(meson_encoder_cvbs->connector)) {
>> +		dev_err(priv->dev, "Unable to create CVBS bridge connector\n");
>> +		return PTR_ERR(meson_encoder_cvbs->connector);
>> +	}
>> +	drm_connector_attach_encoder(meson_encoder_cvbs->connector,
>> +				     &meson_encoder_cvbs->encoder);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.25.1