On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@xxxxxxxxxx> > > To implement hotplug detection in a race-free manner, drivers must call > drm_kms_helper_poll_init() before hotplug events can be triggered. Such > events can be triggered right after any of the encoders or connectors > are initialized. At the same time, if the drm_fb_helper_hotplug_event() > helper is used by a driver, then the poll helper requires some parts of > the FB helper to be initialized to prevent a crash. > > At the same time, drm_fb_helper_init() requires information that is not > necessarily available at such an early stage (number of CRTCs and > connectors), so it cannot be used yet. > > Add a new helper, drm_fb_helper_prepare(), that initializes the bare > minimum needed to allow drm_kms_helper_poll_init() to execute and any > subsequent hotplug events to be processed properly. > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> Some bikeshed on the kerneldoc below, with that addressed this is Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > --- > drivers/gpu/drm/armada/armada_fbdev.c | 2 +- > drivers/gpu/drm/ast/ast_fb.c | 4 ++- > drivers/gpu/drm/bochs/bochs_fbdev.c | 3 ++- > drivers/gpu/drm/cirrus/cirrus_fbdev.c | 4 ++- > drivers/gpu/drm/drm_fb_cma_helper.c | 3 ++- > drivers/gpu/drm/drm_fb_helper.c | 43 ++++++++++++++++++++++++------- > drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ++- > drivers/gpu/drm/gma500/framebuffer.c | 3 ++- > drivers/gpu/drm/i915/intel_fbdev.c | 3 ++- > drivers/gpu/drm/mgag200/mgag200_fb.c | 3 ++- > drivers/gpu/drm/msm/msm_fbdev.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_fbcon.c | 3 ++- > drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +- > drivers/gpu/drm/qxl/qxl_fb.c | 5 +++- > drivers/gpu/drm/radeon/radeon_fb.c | 4 ++- > drivers/gpu/drm/tegra/fb.c | 4 +-- > drivers/gpu/drm/udl/udl_fb.c | 3 ++- > include/drm/drm_fb_helper.h | 2 ++ > 18 files changed, 68 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c > index 21aa1261dba2..9437e11d5df1 100644 > --- a/drivers/gpu/drm/armada/armada_fbdev.c > +++ b/drivers/gpu/drm/armada/armada_fbdev.c > @@ -149,7 +149,7 @@ int armada_fbdev_init(struct drm_device *dev) > > priv->fbdev = fbh; > > - fbh->funcs = &armada_fb_helper_funcs; > + drm_fb_helper_prepare(dev, fbh, &armada_fb_helper_funcs); > > ret = drm_fb_helper_init(dev, fbh, 1, 1); > if (ret) { > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c > index 2113894e4ff8..cba45c774552 100644 > --- a/drivers/gpu/drm/ast/ast_fb.c > +++ b/drivers/gpu/drm/ast/ast_fb.c > @@ -328,8 +328,10 @@ int ast_fbdev_init(struct drm_device *dev) > return -ENOMEM; > > ast->fbdev = afbdev; > - afbdev->helper.funcs = &ast_fb_helper_funcs; > spin_lock_init(&afbdev->dirty_lock); > + > + drm_fb_helper_prepare(dev, &afbdev->helper, &ast_fb_helper_funcs); > + > ret = drm_fb_helper_init(dev, &afbdev->helper, > 1, 1); > if (ret) { > diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c b/drivers/gpu/drm/bochs/bochs_fbdev.c > index 17e5c17f2730..19cf3e9413b6 100644 > --- a/drivers/gpu/drm/bochs/bochs_fbdev.c > +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c > @@ -189,7 +189,8 @@ int bochs_fbdev_init(struct bochs_device *bochs) > { > int ret; > > - bochs->fb.helper.funcs = &bochs_fb_helper_funcs; > + drm_fb_helper_prepare(bochs->dev, &bochs->fb.helper, > + &bochs_fb_helper_funcs); > > ret = drm_fb_helper_init(bochs->dev, &bochs->fb.helper, > 1, 1); > diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c > index 2bd0291168e4..2a135f253e29 100644 > --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c > +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c > @@ -306,9 +306,11 @@ int cirrus_fbdev_init(struct cirrus_device *cdev) > return -ENOMEM; > > cdev->mode_info.gfbdev = gfbdev; > - gfbdev->helper.funcs = &cirrus_fb_helper_funcs; > spin_lock_init(&gfbdev->dirty_lock); > > + drm_fb_helper_prepare(cdev->dev, &gfbdev->helper, > + &cirrus_fb_helper_funcs); > + > ret = drm_fb_helper_init(cdev->dev, &gfbdev->helper, > cdev->num_crtc, CIRRUSFB_CONN_LIMIT); > if (ret) { > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index b74f9e58b69d..acbbd230e081 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -354,9 +354,10 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, > return ERR_PTR(-ENOMEM); > } > > - fbdev_cma->fb_helper.funcs = &drm_fb_cma_helper_funcs; > helper = &fbdev_cma->fb_helper; > > + drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs); > + > ret = drm_fb_helper_init(dev, helper, num_crtc, max_conn_count); > if (ret < 0) { > dev_err(dev->dev, "Failed to initialize drm fb helper.\n"); > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 80ce92ab91f9..7788f110fcbf 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -49,10 +49,11 @@ static LIST_HEAD(kernel_fb_helper_list); > * helper functions used by many drivers to implement the kernel mode setting > * interfaces. > * > - * Initialization is done as a three-step process with drm_fb_helper_init(), > - * drm_fb_helper_single_add_all_connectors() and drm_fb_helper_initial_config(). > - * Drivers with fancier requirements than the default behaviour can override the > - * second step with their own code. Teardown is done with drm_fb_helper_fini(). > + * Initialization is done as a four-step process with drm_fb_helper_prepare(), > + * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and > + * drm_fb_helper_initial_config(). Drivers with fancier requirements than the > + * default behaviour can override the second step with their own code. > + * Teardown is done with drm_fb_helper_fini(). > * > * At runtime drivers should restore the fbdev console by calling > * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They > @@ -502,6 +503,33 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper) > } > > /** > + * drm_fb_helper_prepare - setup a drm_fb_helper structure > + * @dev: DRM device > + * @helper: driver-allocated fbdev helper structure to set up > + * @funcs: pointer to structure of functions associate with this helper > + * > + * Sets up the bare minimum to make the framebuffer helper usable. This is > + * useful to implement race-free initialization of the polling helpers. In > + * that case a typical sequence would be: > + * > + * - call drm_fb_helper_prepare() > + * - set dev->mode_config.funcs This step is done in drm_fb_helper_prepare already. > + * - perform driver-specific setup > + * - call drm_kms_helper_poll_init() Maybe mention that after this you can start to handle hpd events using the probe helpers? > + * - call drm_fb_helper_init() > + * - call drm_fb_helper_single_add_all_connectors() > + * - call drm_fb_helper_initial_config() Does this list look sane in the generated DocBook? Afaik kerneldoc unfortunately lacks any form of markup, at least afaik :( > + */ > +void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, > + const struct drm_fb_helper_funcs *funcs) > +{ > + INIT_LIST_HEAD(&helper->kernel_fb_list); > + helper->funcs = funcs; > + helper->dev = dev; > +} > +EXPORT_SYMBOL(drm_fb_helper_prepare); > + > +/** > * drm_fb_helper_init - initialize a drm_fb_helper structure > * @dev: drm device > * @fb_helper: driver-allocated fbdev helper structure to initialize > @@ -513,8 +541,7 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper) > * nor register the fbdev. This is only done in drm_fb_helper_initial_config() > * to allow driver writes more control over the exact init sequence. > * > - * Drivers must set fb_helper->funcs before calling > - * drm_fb_helper_initial_config(). > + * Drivers must call drm_fb_helper_prepare() before calling this function. > * > * RETURNS: > * Zero if everything went ok, nonzero otherwise. > @@ -529,10 +556,6 @@ int drm_fb_helper_init(struct drm_device *dev, > if (!max_conn_count) > return -EINVAL; > > - fb_helper->dev = dev; > - > - INIT_LIST_HEAD(&fb_helper->kernel_fb_list); > - > fb_helper->crtc_info = kcalloc(crtc_count, sizeof(struct drm_fb_helper_crtc), GFP_KERNEL); > if (!fb_helper->crtc_info) > return -ENOMEM; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > index 7ccf04917f47..46443971d517 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > @@ -274,7 +274,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev) > return -ENOMEM; > > private->fb_helper = helper = &fbdev->drm_fb_helper; > - helper->funcs = &exynos_drm_fb_helper_funcs; > + > + drm_fb_helper_prepare(dev, helper, &exynos_drm_fb_helper_funcs); > > num_crtc = dev->mode_config.num_crtc; > > diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c > index 76e4d777d01d..d0dd3bea8aa5 100644 > --- a/drivers/gpu/drm/gma500/framebuffer.c > +++ b/drivers/gpu/drm/gma500/framebuffer.c > @@ -600,7 +600,8 @@ int psb_fbdev_init(struct drm_device *dev) > } > > dev_priv->fbdev = fbdev; > - fbdev->psb_fb_helper.funcs = &psb_fb_helper_funcs; > + > + drm_fb_helper_prepare(dev, &fbdev->psb_fb_helper, &psb_fb_helper_funcs); > > drm_fb_helper_init(dev, &fbdev->psb_fb_helper, dev_priv->ops->crtcs, > INTELFB_CONN_LIMIT); > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 336a89f2549e..e7dd21029060 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -600,7 +600,8 @@ int intel_fbdev_init(struct drm_device *dev) > if (ifbdev == NULL) > return -ENOMEM; > > - ifbdev->helper.funcs = &intel_fb_helper_funcs; > + drm_fb_helper_probe(dev, &ifbdev->helper, &intel_fb_helper_funcs); > + > if (!intel_fbdev_init_bios(dev, ifbdev)) > ifbdev->preferred_bpp = 32; > > diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c > index a4319aba9180..5451dc58eff1 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_fb.c > +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c > @@ -293,9 +293,10 @@ int mgag200_fbdev_init(struct mga_device *mdev) > return -ENOMEM; > > mdev->mfbdev = mfbdev; > - mfbdev->helper.funcs = &mga_fb_helper_funcs; > spin_lock_init(&mfbdev->dirty_lock); > > + drm_fb_helper_prepare(mdev->dev, &mfbdev->helper, &mga_fb_helper_funcs); > + > ret = drm_fb_helper_init(mdev->dev, &mfbdev->helper, > mdev->num_crtc, MGAG200FB_CONN_LIMIT); > if (ret) > diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c > index 841665862f2f..745b1be4acf0 100644 > --- a/drivers/gpu/drm/msm/msm_fbdev.c > +++ b/drivers/gpu/drm/msm/msm_fbdev.c > @@ -200,7 +200,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev) > > helper = &fbdev->base; > > - helper->funcs = &msm_fb_helper_funcs; > + drm_fb_helper_prepare(dev, helper, &msm_fb_helper_funcs); > > ret = drm_fb_helper_init(dev, helper, > priv->num_crtcs, priv->num_connectors); > diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c > index 8e9c07b7fc89..afe706a20f97 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c > @@ -464,7 +464,8 @@ nouveau_fbcon_init(struct drm_device *dev) > > fbcon->dev = dev; > drm->fbcon = fbcon; > - fbcon->helper.funcs = &nouveau_fbcon_helper_funcs; > + > + drm_fb_helper_prepare(dev, &fbcon->helper, &nouveau_fbcon_helper_funcs); > > ret = drm_fb_helper_init(dev, &fbcon->helper, > dev->mode_config.num_crtc, 4); > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c > index 4cb12083eb12..8436c6857cda 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c > @@ -325,7 +325,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) > > helper = &fbdev->base; > > - helper->funcs = &omap_fb_helper_funcs; > + drm_fb_helper_prepare(dev, helper, &omap_fb_helper_funcs); > > ret = drm_fb_helper_init(dev, helper, > priv->num_crtcs, priv->num_connectors); > diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c > index cf89614c72be..df567888bb1e 100644 > --- a/drivers/gpu/drm/qxl/qxl_fb.c > +++ b/drivers/gpu/drm/qxl/qxl_fb.c > @@ -676,9 +676,12 @@ int qxl_fbdev_init(struct qxl_device *qdev) > > qfbdev->qdev = qdev; > qdev->mode_info.qfbdev = qfbdev; > - qfbdev->helper.funcs = &qxl_fb_helper_funcs; > spin_lock_init(&qfbdev->delayed_ops_lock); > INIT_LIST_HEAD(&qfbdev->delayed_ops); > + > + drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper, > + &qxl_fb_helper_funcs); > + > ret = drm_fb_helper_init(qdev->ddev, &qfbdev->helper, > qxl_num_crtc /* num_crtc - QXL supports just 1 */, > QXLFB_CONN_LIMIT); > diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c > index ad97afdbc4c7..db598d712901 100644 > --- a/drivers/gpu/drm/radeon/radeon_fb.c > +++ b/drivers/gpu/drm/radeon/radeon_fb.c > @@ -353,7 +353,9 @@ int radeon_fbdev_init(struct radeon_device *rdev) > > rfbdev->rdev = rdev; > rdev->mode_info.rfbdev = rfbdev; > - rfbdev->helper.funcs = &radeon_fb_helper_funcs; > + > + drm_fb_helper_prepare(rdev->ddev, &rfbdev->helper, > + &radeon_fb_helper_funcs); > > ret = drm_fb_helper_init(rdev->ddev, &rfbdev->helper, > rdev->num_crtc, > diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c > index 75bed72a9755..2e3758542c89 100644 > --- a/drivers/gpu/drm/tegra/fb.c > +++ b/drivers/gpu/drm/tegra/fb.c > @@ -276,7 +276,6 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm, > unsigned int num_crtc, > unsigned int max_connectors) > { > - struct drm_fb_helper *helper; > struct tegra_fbdev *fbdev; > int err; > > @@ -286,8 +285,7 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm, > return ERR_PTR(-ENOMEM); > } > > - fbdev->base.funcs = &tegra_fb_helper_funcs; > - helper = &fbdev->base; > + drm_fb_helper_prepare(drm, &fbdev->base, &tegra_fb_helper_funcs); > > err = drm_fb_helper_init(drm, &fbdev->base, num_crtc, max_connectors); > if (err < 0) { > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c > index 0647c8cc368b..d1da339843ca 100644 > --- a/drivers/gpu/drm/udl/udl_fb.c > +++ b/drivers/gpu/drm/udl/udl_fb.c > @@ -583,7 +583,8 @@ int udl_fbdev_init(struct drm_device *dev) > return -ENOMEM; > > udl->fbdev = ufbdev; > - ufbdev->helper.funcs = &udl_fb_helper_funcs; > + > + drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs); > > ret = drm_fb_helper_init(dev, &ufbdev->helper, > 1, 1); > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index e8c8eeb3a253..9b83c66a5f13 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -97,6 +97,8 @@ struct drm_fb_helper { > bool delayed_hotplug; > }; > > +void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, > + const struct drm_fb_helper_funcs *funcs); > int drm_fb_helper_init(struct drm_device *dev, > struct drm_fb_helper *helper, int crtc_count, > int max_conn); > -- > 1.9.2 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html