[PATCH] drm: Do not set connector->encoder in drivers
Daniel Vetter
daniel at ffwll.ch
Wed Jan 13 04:31:29 PST 2016
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 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?
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 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! /
> ---------------
> ¯\_(ツ)_/¯
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list