[Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Chris Wilson]

Quoting Lukas Wunner (2017-11-26 11:49:19)
> On Sat, Nov 25, 2017 at 07:41:55PM +0000, Chris Wilson wrote:
> > @@ -697,10 +697,8 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
> >  
> >       /* Due to peculiar init order wrt to hpd handling this is separate. */
> >       if (drm_fb_helper_initial_config(&ifbdev->helper,
> > -                                      ifbdev->preferred_bpp)) {
> > +                                      ifbdev->preferred_bpp))
> >               intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
> > -             intel_fbdev_fini(to_i915(ifbdev->helper.dev));
> > -     }
> >  }
> 
> Hm, the race at hand would be solved by the intel_fbdev_sync() below,
> or am I missing something?  Still wondering why it's necessary to
> leave the fbdev around...

The race is solved, but if we do free ifbdev, we can't dereference
ifbdev prior to the sync; and we store the async cookie inside ifbdev.
Bleugh. Catch 22.

> 
> 
> > @@ -800,7 +798,11 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
> >  {
> >       struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> >  
> > -     if (ifbdev)
> > +     if (!ifbdev)
> > +             return;
> > +
> > +     intel_fbdev_sync(ifbdev);
> > +     if (ifbdev->vma)
> >               drm_fb_helper_hotplug_event(&ifbdev->helper);
> >  }
> 
> This hunk looks good, as you note the synchronization was already there
> but had to be reverted because I failed to notice that a "+ 1" needs to
> be added to the cookie.  You did a much better job than me understanding
> how the async API works with 43cee314345a.
> 
> However the "if (ifbdev->vma)" looks a bit fishy, ifbdev could be NULL
> (e.g. if BIOS fb was too small but intelfb_alloc() failed) so I think
> this might lead to a null pointer deref.  Does it make a difference
> if we check for ifbdev versus ifbdev->vma?  I also notice that you
> added a check for ifbdev->vma with 15727ed0d944 but Daniel later
> removed it with 88be58be886f.

We know that ifbdev is non-NULL and can't become NULL until fini. So
after the sync point, we want to ask the question of whether the config
was successful, for that I used to use ->fb which now replaced by ->vma.
 
> I guess a check *is* necessary here because fbdev initialization might
> have failed, but I'd just check for "if (ifbdev)".

ifbdev = i915->fbdev;
if (ifbdev)
	intel_fbdev_sync(ifbdev);
ifbdev = i915->fbdev;
if (ifbdev)
	drm_fb_helper_hotplug_event(&ifbdev->helper);

See the problem with randomly freeing ifbdev partway through and only
using a fence?
-Chris