Re: [PATCH 5.15 247/779] drm/meson: encoder_hdmi: switch to bridge DRM_BRIDGE_ATTACH_NO_CONNECTOR

[Stefan Agner]

On 2022-09-12 18:08, Stefan Agner wrote:
> On 2022-08-15 19:58, Greg Kroah-Hartman wrote:
>> From: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>
>> [ Upstream commit 0af5e0b41110e2da872030395231ab19c45be931 ]
>>
>> This implements the necessary change to no more use the embedded
>> connector in dw-hdmi and use the dedicated bridge connector driver
>> by passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to the bridge attach call.
>>
>> The necessary connector properties are added to handle the same
>> functionalities as the embedded dw-hdmi connector, i.e. the HDR
>> metadata, the CEC notifier & other flags.
>>
>> The dw-hdmi output_port is set to 1 in order to look for a connector
>> next bridge in order to get DRM_BRIDGE_ATTACH_NO_CONNECTOR working.
> 
> HDMI on ODROID-N2+ was working with v5.15.60, and stopped working with
> v5.15.61. Reverting this commit (and two dependent refcount leak) to be
> the culprit. Reverting just the refcount leaks is not enough to get HDMI
> working, so I assume it is this commit.
> 
> I haven't investigated much beyond that, maybe its simple a case of a
> missing kernel configuration? DRM_DISPLAY_CONNECTOR is compiled, and the
> module display_connector is loaded, so that part seemed to have worked.
> 
> Any ideas welcome.
> 
> FWIW, I track the issue in the HAOS tracker at
> https://github.com/home-assistant/operating-system/issues/2120.

It seems that backporting commit 7cd70656d128 ("drm/bridge:
display-connector: implement bus fmts callbacks") fixes the problem
without reverting this commit.

@Greg, can we backport this commit as well?

--
Stefan

> 
> --
> Stefan
> 
>>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>> Acked-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
>> Link:
>> https://patchwork.freedesktop.org/patch/msgid/20211020123947.2585572-5-narmstrong@xxxxxxxxxxxx
>> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/meson/Kconfig              |  2 +
>>  drivers/gpu/drm/meson/meson_dw_hdmi.c      |  1 +
>>  drivers/gpu/drm/meson/meson_encoder_hdmi.c | 81 +++++++++++++++++++++-
>>  3 files changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig
>> index 9f9281dd49f8..a4e1ed96e5e8 100644
>> --- a/drivers/gpu/drm/meson/Kconfig
>> +++ b/drivers/gpu/drm/meson/Kconfig
>> @@ -6,9 +6,11 @@ config DRM_MESON
>>  	select DRM_KMS_HELPER
>>  	select DRM_KMS_CMA_HELPER
>>  	select DRM_GEM_CMA_HELPER
>> +	select DRM_DISPLAY_CONNECTOR
>>  	select VIDEOMODE_HELPERS
>>  	select REGMAP_MMIO
>>  	select MESON_CANVAS
>> +	select CEC_CORE if CEC_NOTIFIER
>>
>>  config DRM_MESON_DW_HDMI
>>  	tristate "HDMI Synopsys Controller support for Amlogic Meson Display"
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index fb540a503efe..5cd2b2ebbbd3 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -803,6 +803,7 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>>  	dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
>>  	dw_plat_data->ycbcr_420_allowed = true;
>>  	dw_plat_data->disable_cec = true;
>> +	dw_plat_data->output_port = 1;
>>
>>  	if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
>>  	    dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
>> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
>> b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
>> index db332fa4cd54..5e306de6f485 100644
>> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
>> @@ -14,8 +14,11 @@
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/reset.h>
>>
>> +#include <media/cec-notifier.h>
>> +
>>  #include <drm/drm_atomic_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>
>> @@ -34,8 +37,10 @@ struct meson_encoder_hdmi {
>>  	struct drm_encoder encoder;
>>  	struct drm_bridge bridge;
>>  	struct drm_bridge *next_bridge;
>> +	struct drm_connector *connector;
>>  	struct meson_drm *priv;
>>  	unsigned long output_bus_fmt;
>> +	struct cec_notifier *cec_notifier;
>>  };
>>
>>  #define bridge_to_meson_encoder_hdmi(x) \
>> @@ -50,6 +55,14 @@ static int meson_encoder_hdmi_attach(struct
>> drm_bridge *bridge,
>>  				 &encoder_hdmi->bridge, flags);
>>  }
>>
>> +static void meson_encoder_hdmi_detach(struct drm_bridge *bridge)
>> +{
>> +	struct meson_encoder_hdmi *encoder_hdmi =
>> bridge_to_meson_encoder_hdmi(bridge);
>> +
>> +	cec_notifier_conn_unregister(encoder_hdmi->cec_notifier);
>> +	encoder_hdmi->cec_notifier = NULL;
>> +}
>> +
>>  static void meson_encoder_hdmi_set_vclk(struct meson_encoder_hdmi
>> *encoder_hdmi,
>>  					const struct drm_display_mode *mode)
>>  {
>> @@ -298,9 +311,30 @@ static int meson_encoder_hdmi_atomic_check(struct
>> drm_bridge *bridge,
>>  	return 0;
>>  }
>>
>> +static void meson_encoder_hdmi_hpd_notify(struct drm_bridge *bridge,
>> +					  enum drm_connector_status status)
>> +{
>> +	struct meson_encoder_hdmi *encoder_hdmi =
>> bridge_to_meson_encoder_hdmi(bridge);
>> +	struct edid *edid;
>> +
>> +	if (!encoder_hdmi->cec_notifier)
>> +		return;
>> +
>> +	if (status == connector_status_connected) {
>> +		edid = drm_bridge_get_edid(encoder_hdmi->next_bridge,
>> encoder_hdmi->connector);
>> +		if (!edid)
>> +			return;
>> +
>> +		cec_notifier_set_phys_addr_from_edid(encoder_hdmi->cec_notifier, edid);
>> +	} else
>> +		cec_notifier_phys_addr_invalidate(encoder_hdmi->cec_notifier);
>> +}
>> +
>>  static const struct drm_bridge_funcs meson_encoder_hdmi_bridge_funcs = {
>>  	.attach = meson_encoder_hdmi_attach,
>> +	.detach = meson_encoder_hdmi_detach,
>>  	.mode_valid = meson_encoder_hdmi_mode_valid,
>> +	.hpd_notify = meson_encoder_hdmi_hpd_notify,
>>  	.atomic_enable = meson_encoder_hdmi_atomic_enable,
>>  	.atomic_disable = meson_encoder_hdmi_atomic_disable,
>>  	.atomic_get_input_bus_fmts = meson_encoder_hdmi_get_inp_bus_fmts,
>> @@ -313,6 +347,7 @@ static const struct drm_bridge_funcs
>> meson_encoder_hdmi_bridge_funcs = {
>>  int meson_encoder_hdmi_init(struct meson_drm *priv)
>>  {
>>  	struct meson_encoder_hdmi *meson_encoder_hdmi;
>> +	struct platform_device *pdev;
>>  	struct device_node *remote;
>>  	int ret;
>>
>> @@ -337,6 +372,7 @@ int meson_encoder_hdmi_init(struct meson_drm *priv)
>>  	meson_encoder_hdmi->bridge.funcs = &meson_encoder_hdmi_bridge_funcs;
>>  	meson_encoder_hdmi->bridge.of_node = priv->dev->of_node;
>>  	meson_encoder_hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>> +	meson_encoder_hdmi->bridge.interlace_allowed = true;
>>
>>  	drm_bridge_add(&meson_encoder_hdmi->bridge);
>>
>> @@ -353,17 +389,58 @@ int meson_encoder_hdmi_init(struct meson_drm *priv)
>>  	meson_encoder_hdmi->encoder.possible_crtcs = BIT(0);
>>
>>  	/* Attach HDMI Encoder Bridge to Encoder */
>> -	ret = drm_bridge_attach(&meson_encoder_hdmi->encoder,
>> &meson_encoder_hdmi->bridge, NULL, 0);
>> +	ret = drm_bridge_attach(&meson_encoder_hdmi->encoder,
>> &meson_encoder_hdmi->bridge, NULL,
>> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>  	if (ret) {
>>  		dev_err(priv->dev, "Failed to attach bridge: %d\n", ret);
>>  		return ret;
>>  	}
>>
>> +	/* Initialize & attach Bridge Connector */
>> +	meson_encoder_hdmi->connector = drm_bridge_connector_init(priv->drm,
>> +							&meson_encoder_hdmi->encoder);
>> +	if (IS_ERR(meson_encoder_hdmi->connector)) {
>> +		dev_err(priv->dev, "Unable to create HDMI bridge connector\n");
>> +		return PTR_ERR(meson_encoder_hdmi->connector);
>> +	}
>> +	drm_connector_attach_encoder(meson_encoder_hdmi->connector,
>> +				     &meson_encoder_hdmi->encoder);
>> +
>>  	/*
>>  	 * We should have now in place:
>> -	 * encoder->[hdmi encoder bridge]->[dw-hdmi bridge]->[dw-hdmi connector]
>> +	 * encoder->[hdmi encoder bridge]->[dw-hdmi bridge]->[display
>> connector bridge]->[display connector]
>>  	 */
>>
>> +	/*
>> +	 * drm_connector_attach_max_bpc_property() requires the
>> +	 * connector to have a state.
>> +	 */
>> +	drm_atomic_helper_connector_reset(meson_encoder_hdmi->connector);
>> +
>> +	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXL) ||
>> +	    meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) ||
>> +	    meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>> +		drm_connector_attach_hdr_output_metadata_property(meson_encoder_hdmi->connector);
>> +
>> +	drm_connector_attach_max_bpc_property(meson_encoder_hdmi->connector, 8, 8);
>> +
>> +	/* Handle this here until handled by drm_bridge_connector_init() */
>> +	meson_encoder_hdmi->connector->ycbcr_420_allowed = true;
>> +
>> +	pdev = of_find_device_by_node(remote);
>> +	if (pdev) {
>> +		struct cec_connector_info conn_info;
>> +		struct cec_notifier *notifier;
>> +
>> +		cec_fill_conn_info_from_drm(&conn_info, meson_encoder_hdmi->connector);
>> +
>> +		notifier = cec_notifier_conn_register(&pdev->dev, NULL, &conn_info);
>> +		if (!notifier)
>> +			return -ENOMEM;
>> +
>> +		meson_encoder_hdmi->cec_notifier = notifier;
>> +	}
>> +
>>  	dev_dbg(priv->dev, "HDMI encoder initialized\n");
>>
>>  	return 0;