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; + 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);