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_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...
@@ -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.
I guess a check *is* necessary here because fbdev initialization might have failed, but I'd just check for "if (ifbdev)".
Thanks & have a pleasant Sunday afternoon.
Lukas