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

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

[Thierry Reding]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

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?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Liviu Dudau]

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
-- 
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

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

[Daniel Vetter]

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

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

[Jani Nikula]

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.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
-- 
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

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

[Liviu Dudau]

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
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

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

[Laurent Pinchart]

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 :-)


-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel