On Thu, Apr 17, 2014 at 10:38:17AM +0200, Thierry Reding wrote:
> On Wed, Apr 16, 2014 at 04:45:21PM +0200, Daniel Vetter wrote:
> > Some drivers need to be able to have a perfect race-free fbcon setup.
> > Current drivers only enable hotplug processing after the call to
> > drm_fb_helper_initial_config which leaves a tiny but important race.
> >
> > This race is especially noticable on embedded platforms where the
> > driver itself enables the voltage for the hdmi output, since only then
> > will monitors (after a bit of delay, as usual) respond by asserting
> > the hpd pin.
> >
> > Most of the infrastructure is already there with the split-out
> > drm_fb_helper_init. And drm_fb_helper_initial_config already has all
> > the required locking to handle concurrent hpd events since
> >
> > commit 53f1904bced78d7c00f5d874c662ec3ac85d0f9f
> > Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Date: Thu Mar 20 14:26:35 2014 +0100
> >
> > drm/fb-helper: improve drm_fb_helper_initial_config locking
> >
> > The only missing bit is making drm_fb_helper_hotplug_event save
> > against concurrent calls of drm_fb_helper_initial_config. The only
> > unprotected bit is the check for fb_helper->fb.
> >
> > With that drivers can first initialize the fb helper, then enabel
> > hotplug processing and then set up the initial config all in a
> > completely race-free manner. Update kerneldoc and convert i915 as a
> > proof of concept.
> >
> > Feature requested by Thierry since his tegra driver atm reliably boots
> > slowly enough to misses the hotplug event for an external hdmi screen,
> > but also reliably boots to quickly for the hpd pin to be asserted when
> > the fb helper calls into the hdmi ->detect function.
> >
> > Cc: Thierry Reding <treding@xxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 11 +++++------
> > drivers/gpu/drm/i915/i915_dma.c | 3 ---
> > drivers/gpu/drm/i915/i915_drv.c | 2 --
> > drivers/gpu/drm/i915/i915_drv.h | 1 -
> > drivers/gpu/drm/i915/i915_irq.c | 4 ----
> > 5 files changed, 5 insertions(+), 16 deletions(-)
>
> The FB helper parts:
>
> Tested-by: Thierry Reding <treding@xxxxxxxxxx>
>
> But I have one comment below...
>
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> [...]
> > - * Note that the driver must ensure that this is only called _after_ the fb has
> > - * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
> > + * Note that drivers may call this even before calling
> > + * drm_fb_helper_initial_config but only aftert drm_fb_helper_init. This allows
>
> I don't think the requirement is that strict. To elaborate: on Tegra we
> cannot call drm_fb_helper_init() because the number of CRTCs and
> connectors isn't known this early. We determine that dynamically after
> all sub-devices have been initialized. So instead of calling
> drm_fb_helper_init() before drm_kms_helper_poll_init(), I did something
> more minimal (see attached patch). It's kind of difficult to tell
> because of the context, but tegra_drm_fb_prepare() sets up the mode
> config and functions and allocate memory for the FB helper and sets the
> FB helper functions.
>
> This may not be enough for all drivers, but on Tegra the implementation
> of .output_poll_changed() simply calls drm_fb_helper_hotplug_event(),
> which will work fine with just that rudimentary initialization.
Hm yeah I think this should be sufficient, too. It would be good to
extract this minimal initialization into a new drm_fb_helper_prepare
function and update the kerneldoc a bit more. Maybe as a patch on top of
mine?
Then we could merge this all as an early tegra-next pull to Dave.
-Daniel
>
> Thierry
> From ea394150524c8b54ee4131ad830bf5beb6b1056e Mon Sep 17 00:00:00 2001
> From: Thierry Reding <treding@xxxxxxxxxx>
> Date: Thu, 17 Apr 2014 10:02:17 +0200
> Subject: [PATCH] drm/tegra: Implement race-free hotplug detection
>
> A race condition currently exists on Tegra, where it can happen that a
> monitor attached via HDMI isn't detected during the initial FB helper
> setup, but the hotplug event happens too early to be processed by the
> poll helpers because they haven't been initialized yet. This happens
> because on some boards the HDMI driver can control the regulator that
> supplies the +5V pin on the HDMI connector. Therefore depending on the
> timing between the initialization of the HDMI driver and the rest of
> DRM, it's possible that the monitor returns the hotplug signal right
> within the window where we would miss it.
>
> Unfortunately, drm_kms_helper_poll_init() will wreak havoc when called
> before at least some parts of the FB helpers have been set up.
>
> This commit fixes this by splitting out the minimum of initialization
> required to make drm_kms_helper_poll_init() work into a separate
> function that can be called early. It is then safe to move all of the
> poll helper initialization to an earlier point in time (before the
> HDMI output driver has a chance to enable the +5V supply). That way if
> the hotplug signal is returned before the initial FB helper setup, the
> monitor will be forcefully detected at that point, and if the hotplug
> signal is returned after that it will be properly handled by the poll
> helpers.
>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> drivers/gpu/drm/tegra/drm.c | 8 ++++++--
> drivers/gpu/drm/tegra/drm.h | 1 +
> drivers/gpu/drm/tegra/fb.c | 50 ++++++++++++++++++++++++++++++---------------
> 3 files changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 09ee77923d67..d492c2f12ca8 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -41,6 +41,12 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
>
> drm_mode_config_init(drm);
>
> + err = tegra_drm_fb_prepare(drm);
> + if (err < 0)
> + return err;
> +
> + drm_kms_helper_poll_init(drm);
> +
> err = host1x_device_init(device);
> if (err < 0)
> return err;
> @@ -60,8 +66,6 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
> if (err < 0)
> return err;
>
> - drm_kms_helper_poll_init(drm);
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 784fd5c77441..d100f706d818 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -284,6 +284,7 @@ struct tegra_bo *tegra_fb_get_plane(struct drm_framebuffer *framebuffer,
> unsigned int index);
> bool tegra_fb_is_bottom_up(struct drm_framebuffer *framebuffer);
> bool tegra_fb_is_tiled(struct drm_framebuffer *framebuffer);
> +int tegra_drm_fb_prepare(struct drm_device *drm);
> int tegra_drm_fb_init(struct drm_device *drm);
> void tegra_drm_fb_exit(struct drm_device *drm);
> #ifdef CONFIG_DRM_TEGRA_FBDEV
> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index f7fca09d4921..2d7c589b550a 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -271,14 +271,9 @@ static struct drm_fb_helper_funcs tegra_fb_helper_funcs = {
> .fb_probe = tegra_fbdev_probe,
> };
>
> -static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm,
> - unsigned int preferred_bpp,
> - unsigned int num_crtc,
> - unsigned int max_connectors)
> +static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm)
> {
> - struct drm_fb_helper *helper;
> struct tegra_fbdev *fbdev;
> - int err;
>
> fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
> if (!fbdev) {
> @@ -287,12 +282,23 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm,
> }
>
> fbdev->base.funcs = &tegra_fb_helper_funcs;
> - helper = &fbdev->base;
> + fbdev->base.dev = drm;
> +
> + return fbdev;
> +}
> +
> +static int tegra_fbdev_init(struct tegra_fbdev *fbdev,
> + unsigned int preferred_bpp,
> + unsigned int num_crtc,
> + unsigned int max_connectors)
> +{
> + struct drm_device *drm = fbdev->base.dev;
> + int err;
>
> err = drm_fb_helper_init(drm, &fbdev->base, num_crtc, max_connectors);
> if (err < 0) {
> dev_err(drm->dev, "failed to initialize DRM FB helper\n");
> - goto free;
> + return err;
> }
>
> err = drm_fb_helper_single_add_all_connectors(&fbdev->base);
> @@ -301,21 +307,17 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm,
> goto fini;
> }
>
> - drm_helper_disable_unused_functions(drm);
> -
> err = drm_fb_helper_initial_config(&fbdev->base, preferred_bpp);
> if (err < 0) {
> dev_err(drm->dev, "failed to set initial configuration\n");
> goto fini;
> }
>
> - return fbdev;
> + return 0;
>
> fini:
> drm_fb_helper_fini(&fbdev->base);
> -free:
> - kfree(fbdev);
> - return ERR_PTR(err);
> + return err;
> }
>
> static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
> @@ -369,7 +371,7 @@ static const struct drm_mode_config_funcs tegra_drm_mode_funcs = {
> #endif
> };
>
> -int tegra_drm_fb_init(struct drm_device *drm)
> +int tegra_drm_fb_prepare(struct drm_device *drm)
> {
> #ifdef CONFIG_DRM_TEGRA_FBDEV
> struct tegra_drm *tegra = drm->dev_private;
> @@ -384,8 +386,7 @@ int tegra_drm_fb_init(struct drm_device *drm)
> drm->mode_config.funcs = &tegra_drm_mode_funcs;
>
> #ifdef CONFIG_DRM_TEGRA_FBDEV
> - tegra->fbdev = tegra_fbdev_create(drm, 32, drm->mode_config.num_crtc,
> - drm->mode_config.num_connector);
> + tegra->fbdev = tegra_fbdev_create(drm);
> if (IS_ERR(tegra->fbdev))
> return PTR_ERR(tegra->fbdev);
> #endif
> @@ -393,6 +394,21 @@ int tegra_drm_fb_init(struct drm_device *drm)
> return 0;
> }
>
> +int tegra_drm_fb_init(struct drm_device *drm)
> +{
> +#ifdef CONFIG_DRM_TEGRA_FBDEV
> + struct tegra_drm *tegra = drm->dev_private;
> + int err;
> +
> + err = tegra_fbdev_init(tegra->fbdev, 32, drm->mode_config.num_crtc,
> + drm->mode_config.num_connector);
> + if (err < 0)
> + return err;
> +#endif
> +
> + return 0;
> +}
> +
> void tegra_drm_fb_exit(struct drm_device *drm)
> {
> #ifdef CONFIG_DRM_TEGRA_FBDEV
> --
> 1.9.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
