Re: [PATCH] drm: Do not set connector->encoder in drivers

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 16, 2015 at 06:19:53PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> An encoder is associated with a connector by the DRM core as a result of
> setting up a configuration. Drivers using the atomic or legacy helpers
> should never set up this link, even if it is a static one.
> 
> While at it, try to catch this kind of error in the future by adding a
> WARN_ON() in drm_mode_connector_attach_encoder(). Note that this doesn't
> cover all the cases, since drivers could set this up after attaching.
> Drivers that use the atomic helpers will get a warning later on, though,
> so hopefully the two combined cover enough to help people avoid this in
> the future.
> 
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: Mark yao <mark.yao@rock-chips.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Daniel, I didn't add your Reviewed-by because I included two more fixes
> for the same errors in the i.MX and shmobile drivers. But I suspect that
> you will want to pick this up into drm/misc anyway.
> 
>  drivers/gpu/drm/bridge/dw_hdmi.c          |  2 --
>  drivers/gpu/drm/drm_crtc.c                | 14 ++++++++++++++
>  drivers/gpu/drm/i2c/tda998x_drv.c         |  1 -
>  drivers/gpu/drm/imx/parallel-display.c    |  2 --
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |  2 --
>  5 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
> index 56de9f1c95fc..ffef392ccc13 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -1648,8 +1648,6 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
>  	drm_connector_init(drm, &hdmi->connector, &dw_hdmi_connector_funcs,
>  			   DRM_MODE_CONNECTOR_HDMIA);
>  
> -	hdmi->connector.encoder = encoder;
> -
>  	drm_mode_connector_attach_encoder(&hdmi->connector, encoder);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 24c5434abd1c..bc0693c63ca4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4983,6 +4983,20 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>  {
>  	int i;
>  
> +	/*
> +	 * In the past, drivers have attempted to model the static association
> +	 * of connector to encoder in simple connector/encoder devices using a
> +	 * direct assignment of connector->encoder = encoder. This connection
> +	 * is a logical one and the responsibility of the core, so drivers are
> +	 * expected not to mess with this.
> +	 *
> +	 * Note that the error return should've been enough here, but a large
> +	 * majority of drivers ignores the return value, so add in a big WARN
> +	 * to get people's attention.
> +	 */
> +	if (WARN_ON(connector->encoder))
> +		return -EINVAL;

I'd go with

	/*
	 * connector->encoder is dynamic and gets cleared to NULL when
	 * disabling an output, hence can't be used to set up a static
	 * connector -> encoder link. Warn about abuse.
	 */
	WARN_ON(connector->encoder);

No point failing and making driver writers life harder. With that bikeshed
this patch is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
>  		if (connector->encoder_ids[i] == 0) {
>  			connector->encoder_ids[i] = encoder->base.id;
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 896b6aaf8c4d..8b0a402190eb 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1453,7 +1453,6 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		goto err_sysfs;
>  
> -	priv->connector.encoder = &priv->encoder;
>  	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index b4deb9cf9d71..57b78226e392 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -200,8 +200,6 @@ static int imx_pd_register(struct drm_device *drm,
>  
>  	drm_mode_connector_attach_encoder(&imxpd->connector, &imxpd->encoder);
>  
> -	imxpd->connector.encoder = &imxpd->encoder;
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> index e9272b0a8592..cb72b35d85d1 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> @@ -739,8 +739,6 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
>  	if (ret < 0)
>  		goto err_backlight;
>  
> -	connector->encoder = encoder;
> -
>  	drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
>  	drm_object_property_set_value(&connector->base,
>  		sdev->ddev->mode_config.dpms_property, DRM_MODE_DPMS_OFF);
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel