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