Message-ID: <ea92f925-7778-477b-aeab-604407260de8@ideasonboard.com>
Date: Tue, 27 May 2025 10:38:20 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Jayesh Choudhary <j-choudhary@...com>
Cc: jonas@...boo.se, jernej.skrabec@...il.com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 airlied@...il.com, simona@...ll.ch, lumag@...nel.org, jani.nikula@...el.com,
 andy.yan@...k-chips.com, mordan@...ras.ru, linux@...blig.org,
 viro@...iv.linux.org.uk, yamonkar@...ence.com, sjakhade@...ence.com,
 quentin.schulz@...e-electrons.com, jsarha@...com,
 linux-kernel@...r.kernel.org, devarsht@...com, dianders@...omium.org,
 andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org,
 Laurent.pinchart@...asonboard.com, dri-devel@...ts.freedesktop.org,
 alexander.stein@...tq-group.com
Subject: Re: [RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core:
 Remove legacy support for connector initialisation in bridge

Hi,

On 21/05/2025 10:32, Jayesh Choudhary wrote:
> Now that we have DBANC framework, remove the connector initialisation code
> as that piece of code is not called if DRM_BRIDGE_ATTACH_NO_CONNECTOR flag
> is used. Only TI K3 platforms consume this driver and tidss (their display
> controller) has this flag set. So this legacy support can be dropped.
> 

Why is the series RFC? Does it not work? Is there something here you're
not comfortable with?

> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>  1 file changed, 25 insertions(+), 161 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index b431e7efd1f0..66bd916c2fe9 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -1444,56 +1444,6 @@ static const struct drm_edid *cdns_mhdp_edid_read(struct cdns_mhdp_device *mhdp,
>  	return drm_edid_read_custom(connector, cdns_mhdp_get_edid_block, mhdp);
>  }
>  
> -static int cdns_mhdp_get_modes(struct drm_connector *connector)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(connector);
> -	const struct drm_edid *drm_edid;
> -	int num_modes;
> -
> -	if (!mhdp->plugged)
> -		return 0;
> -
> -	drm_edid = cdns_mhdp_edid_read(mhdp, connector);
> -
> -	drm_edid_connector_update(connector, drm_edid);
> -
> -	if (!drm_edid) {
> -		dev_err(mhdp->dev, "Failed to read EDID\n");
> -		return 0;
> -	}
> -
> -	num_modes = drm_edid_connector_add_modes(connector);
> -	drm_edid_free(drm_edid);
> -
> -	/*
> -	 * HACK: Warn about unsupported display formats until we deal
> -	 *       with them correctly.
> -	 */
> -	if (connector->display_info.color_formats &&
> -	    !(connector->display_info.color_formats &
> -	      mhdp->display_fmt.color_format))
> -		dev_warn(mhdp->dev,
> -			 "%s: No supported color_format found (0x%08x)\n",
> -			__func__, connector->display_info.color_formats);
> -
> -	if (connector->display_info.bpc &&
> -	    connector->display_info.bpc < mhdp->display_fmt.bpc)
> -		dev_warn(mhdp->dev, "%s: Display bpc only %d < %d\n",
> -			 __func__, connector->display_info.bpc,
> -			 mhdp->display_fmt.bpc);
> -
> -	return num_modes;
> -}
> -
> -static int cdns_mhdp_connector_detect(struct drm_connector *conn,
> -				      struct drm_modeset_acquire_ctx *ctx,
> -				      bool force)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
> -
> -	return cdns_mhdp_detect(mhdp);
> -}
> -
>  static u32 cdns_mhdp_get_bpp(struct cdns_mhdp_display_fmt *fmt)
>  {
>  	u32 bpp;
> @@ -1547,114 +1497,6 @@ bool cdns_mhdp_bandwidth_ok(struct cdns_mhdp_device *mhdp,
>  	return true;
>  }
>  
> -static
> -enum drm_mode_status cdns_mhdp_mode_valid(struct drm_connector *conn,
> -					  const struct drm_display_mode *mode)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
> -
> -	mutex_lock(&mhdp->link_mutex);
> -
> -	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> -				    mhdp->link.rate)) {
> -		mutex_unlock(&mhdp->link_mutex);
> -		return MODE_CLOCK_HIGH;
> -	}
> -
> -	mutex_unlock(&mhdp->link_mutex);
> -	return MODE_OK;
> -}
> -
> -static int cdns_mhdp_connector_atomic_check(struct drm_connector *conn,
> -					    struct drm_atomic_state *state)
> -{
> -	struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
> -	struct drm_connector_state *old_state, *new_state;
> -	struct drm_crtc_state *crtc_state;
> -	u64 old_cp, new_cp;
> -
> -	if (!mhdp->hdcp_supported)
> -		return 0;
> -
> -	old_state = drm_atomic_get_old_connector_state(state, conn);
> -	new_state = drm_atomic_get_new_connector_state(state, conn);
> -	old_cp = old_state->content_protection;
> -	new_cp = new_state->content_protection;
> -
> -	if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
> -	    new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> -		new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		goto mode_changed;
> -	}
> -
> -	if (!new_state->crtc) {
> -		if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
> -			new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> -		return 0;
> -	}
> -
> -	if (old_cp == new_cp ||
> -	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> -	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
> -		return 0;
> -
> -mode_changed:
> -	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> -	crtc_state->mode_changed = true;
> -
> -	return 0;
> -}
> -
> -static const struct drm_connector_helper_funcs cdns_mhdp_conn_helper_funcs = {
> -	.detect_ctx = cdns_mhdp_connector_detect,
> -	.get_modes = cdns_mhdp_get_modes,
> -	.mode_valid = cdns_mhdp_mode_valid,
> -	.atomic_check = cdns_mhdp_connector_atomic_check,
> -};
> -
> -static const struct drm_connector_funcs cdns_mhdp_conn_funcs = {
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.destroy = drm_connector_cleanup,
> -};
> -
> -static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
> -{
> -	u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
> -	struct drm_connector *conn = &mhdp->connector;
> -	struct drm_bridge *bridge = &mhdp->bridge;
> -	int ret;
> -
> -	conn->polled = DRM_CONNECTOR_POLL_HPD;
> -
> -	ret = drm_connector_init(bridge->dev, conn, &cdns_mhdp_conn_funcs,
> -				 DRM_MODE_CONNECTOR_DisplayPort);
> -	if (ret) {
> -		dev_err(mhdp->dev, "Failed to initialize connector with drm\n");
> -		return ret;
> -	}
> -
> -	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
> -
> -	ret = drm_display_info_set_bus_formats(&conn->display_info,
> -					       &bus_format, 1);
> -	if (ret)
> -		return ret;
> -
> -	ret = drm_connector_attach_encoder(conn, bridge->encoder);
> -	if (ret) {
> -		dev_err(mhdp->dev, "Failed to attach connector to encoder\n");
> -		return ret;
> -	}
> -
> -	if (mhdp->hdcp_supported)
> -		ret = drm_connector_attach_content_protection_property(conn, true);
> -
> -	return ret;
> -}
> -
>  static int cdns_mhdp_attach(struct drm_bridge *bridge,
>  			    struct drm_encoder *encoder,
>  			    enum drm_bridge_attach_flags flags)
> @@ -1671,9 +1513,11 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
>  		return ret;
>  
>  	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> -		ret = cdns_mhdp_connector_init(mhdp);
> -		if (ret)
> -			goto aux_unregister;
> +		ret = -EINVAL;
> +		dev_err(mhdp->dev,
> +			"Connector initialisation not supported in bridge_attach %d\n",
> +			ret);
> +		goto aux_unregister;
>  	}
>  
>  	spin_lock(&mhdp->start_lock);
> @@ -2158,6 +2002,25 @@ static const struct drm_edid *cdns_mhdp_bridge_edid_read(struct drm_bridge *brid
>  	return cdns_mhdp_edid_read(mhdp, connector);
>  }
>  
> +static enum drm_mode_status
> +cdns_mhdp_bridge_mode_valid(struct drm_bridge *bridge,
> +			    const struct drm_display_info *info,
> +			    const struct drm_display_mode *mode)
> +{
> +	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> +
> +	mutex_lock(&mhdp->link_mutex);
> +
> +	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> +				    mhdp->link.rate)) {
> +		mutex_unlock(&mhdp->link_mutex);
> +		return MODE_CLOCK_HIGH;
> +	}
> +
> +	mutex_unlock(&mhdp->link_mutex);
> +	return MODE_OK;
> +}
> +
>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.atomic_enable = cdns_mhdp_atomic_enable,
>  	.atomic_disable = cdns_mhdp_atomic_disable,
> @@ -2172,6 +2035,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.edid_read = cdns_mhdp_bridge_edid_read,
>  	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
>  	.hpd_disable = cdns_mhdp_bridge_hpd_disable,
> +	.mode_valid = cdns_mhdp_bridge_mode_valid,
>  };
>  
>  static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp, bool *hpd_pulse)

Why do you need to add bridge mode_valid() when removing the legacy
non-DRM_BRIDGE_ATTACH_NO_CONNECTOR code?

 Tomi

Powered by blists - more mailing lists