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...
> @@ -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