Re: [PATCH] drm/fb-helper: Fix hpd vs. initial config races

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux