[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 at 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 at arm.linux.org.uk>
> > Cc: Philipp Zabel <p.zabel at pengutronix.de>
> > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Cc: Liviu Dudau <Liviu.Dudau at arm.com>
> > Cc: Mark yao <mark.yao at rock-chips.com>
> > Signed-off-by: Thierry Reding <treding at 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 at 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 at 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!  /
  ---------------
    ¯\_(ツ)_/¯