Re: [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Lukas Wunner <lukas@wunner.de>]

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