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

[Thierry Reding]

On Tue, Apr 22, 2014 at 10:26:37AM +0200, Daniel Vetter wrote:
> 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 at ffwll.ch>
> > > 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 at nvidia.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > ---
> > >  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 at nvidia.com>
> > 
> > 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?

It would be essentially this:

void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
			   const struct drm_fb_helper_funcs *funcs)
{
	helper->funcs = funcs;
	helper->dev = dev;
}

So I wonder if that's still what we want or whether drivers should
simply be doing that manually if they need to. Having a function for it
gives us a place to document things, though, and perhaps at some point
we'll have to extend this, so it may be a good idea after all, even if
it's just the two lines currently.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140422/b398c072/attachment.sig>