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);
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.
I'm pretty sure that when I started out writing armada-drm, pre-initialising the connector's encoder was required, otherwise DRM would oops. Has something changed which makes a NULL pointer there at initialisation always safe?
On Mon, Nov 16, 2015 at 05:46:46PM +0000, Russell King - ARM Linux wrote:
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.
I'm pretty sure that when I started out writing armada-drm, pre-initialising the connector's encoder was required, otherwise DRM would oops. Has something changed which makes a NULL pointer there at initialisation always safe?
The drm core/helpers have always cleared drm_connector->encoder when disabling an output (see drm_crtc_helper_disable). So if you have indeed Oopsed because of this your driver would have likely oopsed after a modeset too. drm_connector->encoder was always just the dynamic association (but I think I've seen some implementation of ->best_encoder which got this wrong and just used drm_connector->encoder). -Daniel
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;- 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);
For the drivers/gpu/drm/drm_crtc.c and drivers/gpu/drm/i2c/tda998x_drv.c combination, together with an atomic modesetting enabled KMS driver (HDLCD):
Tested-by: Liviu Dudau Liviu.Dudau@arm.com
Many thanks, Liviu
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
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Nov 17, 2015 at 10:29:56AM +0000, Liviu Dudau wrote:
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.
Thierry,
I've been using your patch for weeks and I found it very useful as it removes an ugly WARN() in the kernel boot log. However, it doesn't seem to have been included in any upstream trees. Do you have any plans to push it to Daniel?
Best regards, Liviu
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);
For the drivers/gpu/drm/drm_crtc.c and drivers/gpu/drm/i2c/tda998x_drv.c combination, together with an atomic modesetting enabled KMS driver (HDLCD):
Tested-by: Liviu Dudau Liviu.Dudau@arm.com
Many thanks, Liviu
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
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jan 13, 2016 at 11:47:20AM +0000, Liviu Dudau wrote:
On Tue, Nov 17, 2015 at 10:29:56AM +0000, Liviu Dudau wrote:
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.
Thierry,
I've been using your patch for weeks and I found it very useful as it removes an ugly WARN() in the kernel boot log. However, it doesn't seem to have been included in any upstream trees. Do you have any plans to push it to Daniel?
There's been a serious lack of acks from driver maintainers, that's why I stalled on this one. Applied to drm-misc now. -Daniel
Best regards, Liviu
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);
For the drivers/gpu/drm/drm_crtc.c and drivers/gpu/drm/i2c/tda998x_drv.c combination, together with an atomic modesetting enabled KMS driver (HDLCD):
Tested-by: Liviu Dudau Liviu.Dudau@arm.com
Many thanks, Liviu
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
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
| I would like to | | fix the world, | | but they're not | | giving me the | \ source code! /
¯\_(ツ)_/¯
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
On Mon, 16 Nov 2015, Thierry Reding thierry.reding@gmail.com wrote:
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.
Not to block this patch in any way, but really this kind of stuff should end up in the struct drm_connector kernel-doc. Although it's already a monster.
BR, Jani.
On Tue, Nov 17, 2015 at 04:58:25PM +0200, Jani Nikula wrote:
On Mon, 16 Nov 2015, Thierry Reding thierry.reding@gmail.com wrote:
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.
Not to block this patch in any way, but really this kind of stuff should end up in the struct drm_connector kernel-doc. Although it's already a monster.
In 4.4 we can split up the kerneldoc for structures into per-member comments, which was added exactly to handle monsters like this. Also, with the per-member comment layout you can do full paragraphs to explain tricky bits like this separately and are no longer limited to the single (continuated if needed) line we have right now. -Daniel
On Tuesday 17 November 2015 16:58:25 Jani Nikula wrote:
On Mon, 16 Nov 2015, Thierry Reding thierry.reding@gmail.com wrote:
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.
Not to block this patch in any way, but really this kind of stuff should end up in the struct drm_connector kernel-doc. Although it's already a monster.
I was about to mention the same, so I'll second. And v4.4 is here, so we can deal with the monster :-)
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
Laurent Pinchart -
Liviu Dudau -
Russell King - ARM Linux -
Thierry Reding