| Message ID | 152403998526.27610.17785499460259643269@mail.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-04-18 at 09:26 +0100, Chris Wilson wrote: > Quoting Souza, Jose (2018-04-18 01:07:16) > > On Wed, 2018-04-18 at 00:44 +0100, Chris Wilson wrote: > > > Quoting José Roberto de Souza (2018-04-17 23:34:18) > > > > If the initial fbdev > > > > configuration(intel_fbdev_initial_config()) > > > > runs and > > > > there still no sink connected it will cause > > > > drm_fb_helper_initial_config() to return 0 as no error > > > > happened(but > > > > internally the return is -EAGAIN). > > > > Because no framebuffer was allocated, when a sink is connected > > > > intel_fbdev_output_poll_changed() will not execute > > > > drm_fb_helper_hotplug_event() that would trigger another try to > > > > do > > > > the > > > > initial fbdev configuration. > > > > > > > > So here creating a dummy framebuffer of 800x600, so > > > > drm_fb_helper_hotplug_event() will be executed and fbdev can be > > > > properly > > > > setup when a sink is connected, if needed the dummy framebuffer > > > > will be > > > > freed and a new with the proper size will be allocated. > > > > > > > > This issue also happens when a MST DP sink is connected since > > > > boot, > > > > as > > > > the MST topology is discovered in parallel if > > > > intel_fbdev_initial_config() > > > > is executed before the first sink MST is discovered it will > > > > cause > > > > this > > > > same issue. > > > > > > > > This is a follow up patch of > > > > https://patchwork.freedesktop.org/patch/196089/ > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158 > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425 > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_fbdev.c | 24 > > > > +++++++++++++++++++++++- > > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > > index 7d41d139341b..773577d39782 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > > @@ -696,14 +696,36 @@ int intel_fbdev_init(struct drm_device > > > > *dev) > > > > return 0; > > > > } > > > > > > > > +static void intel_fbdev_dummy_fb_create(struct intel_fbdev > > > > *ifbdev) > > > > +{ > > > > + struct drm_fb_helper_surface_size sizes; > > > > + > > > > + sizes.fb_width = 800; > > > > + sizes.fb_height = 600; > > > > + sizes.surface_width = sizes.fb_width; > > > > + sizes.surface_height = sizes.fb_height; > > > > + sizes.surface_bpp = 32; > > > > + sizes.surface_depth = 24; > > > > + > > > > + if (intelfb_create(&ifbdev->helper, &sizes)) > > > > + DRM_ERROR("Unable to create dummy framebufer"); > > > > +} > > > > + > > > > static void intel_fbdev_initial_config(void *data, > > > > async_cookie_t > > > > cookie) > > > > { > > > > struct intel_fbdev *ifbdev = data; > > > > > > > > /* 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)); > > > > > > > > + return; > > > > + } > > > > + > > > > + mutex_lock(&ifbdev->helper.lock); > > > > + if (!ifbdev->vma) > > > > + intel_fbdev_dummy_fb_create(ifbdev); > > > > + mutex_unlock(&ifbdev->helper.lock); > > > > } > > > > > > Did you try > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > index 65a3313723c9..4120c635742d 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -493,6 +493,7 @@ static bool intel_fb_initial_config(struct > > > drm_fb_helper *fb_helper, > > > bail: > > > DRM_DEBUG_KMS("Not using firmware > > > configuration\n"); > > > memcpy(enabled, save_enabled, count); > > > + fb_helper->deferred_setup = true; > > > > This is too earlier to set deferred_setup when > > intel_fb_initial_config() fails, > > __drm_fb_helper_initial_config_and_unlock() will then try to > > probe/create a fb but as there is no sink connected it will fail > > too. > > > > Also __drm_fb_helper_initial_config_and_unlock() is already setting > > deferred_setup if both attempts fails. > > So... > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index 7d41d139341b..5f138a03dd2a 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct > drm_device *dev) > return; > > intel_fbdev_sync(ifbdev); > - if (ifbdev->vma) > + if (ifbdev->vma || ifbdev->helper->deferred_setup) > drm_fb_helper_hotplug_event(&ifbdev->helper); > } That would work but according to your comment here: https://patchwork.f reedesktop.org/patch/196089/ that would lead to a crash in some case. > > ? > -Chris
Quoting Souza, Jose (2018-04-18 17:42:47) > On Wed, 2018-04-18 at 09:26 +0100, Chris Wilson wrote: > > Quoting Souza, Jose (2018-04-18 01:07:16) > > > On Wed, 2018-04-18 at 00:44 +0100, Chris Wilson wrote: > > > > Quoting José Roberto de Souza (2018-04-17 23:34:18) > > > > > If the initial fbdev > > > > > configuration(intel_fbdev_initial_config()) > > > > > runs and > > > > > there still no sink connected it will cause > > > > > drm_fb_helper_initial_config() to return 0 as no error > > > > > happened(but > > > > > internally the return is -EAGAIN). > > > > > Because no framebuffer was allocated, when a sink is connected > > > > > intel_fbdev_output_poll_changed() will not execute > > > > > drm_fb_helper_hotplug_event() that would trigger another try to > > > > > do > > > > > the > > > > > initial fbdev configuration. > > > > > > > > > > So here creating a dummy framebuffer of 800x600, so > > > > > drm_fb_helper_hotplug_event() will be executed and fbdev can be > > > > > properly > > > > > setup when a sink is connected, if needed the dummy framebuffer > > > > > will be > > > > > freed and a new with the proper size will be allocated. > > > > > > > > > > This issue also happens when a MST DP sink is connected since > > > > > boot, > > > > > as > > > > > the MST topology is discovered in parallel if > > > > > intel_fbdev_initial_config() > > > > > is executed before the first sink MST is discovered it will > > > > > cause > > > > > this > > > > > same issue. > > > > > > > > > > This is a follow up patch of > > > > > https://patchwork.freedesktop.org/patch/196089/ > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158 > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425 > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_fbdev.c | 24 > > > > > +++++++++++++++++++++++- > > > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > > > index 7d41d139341b..773577d39782 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > > > @@ -696,14 +696,36 @@ int intel_fbdev_init(struct drm_device > > > > > *dev) > > > > > return 0; > > > > > } > > > > > > > > > > +static void intel_fbdev_dummy_fb_create(struct intel_fbdev > > > > > *ifbdev) > > > > > +{ > > > > > + struct drm_fb_helper_surface_size sizes; > > > > > + > > > > > + sizes.fb_width = 800; > > > > > + sizes.fb_height = 600; > > > > > + sizes.surface_width = sizes.fb_width; > > > > > + sizes.surface_height = sizes.fb_height; > > > > > + sizes.surface_bpp = 32; > > > > > + sizes.surface_depth = 24; > > > > > + > > > > > + if (intelfb_create(&ifbdev->helper, &sizes)) > > > > > + DRM_ERROR("Unable to create dummy framebufer"); > > > > > +} > > > > > + > > > > > static void intel_fbdev_initial_config(void *data, > > > > > async_cookie_t > > > > > cookie) > > > > > { > > > > > struct intel_fbdev *ifbdev = data; > > > > > > > > > > /* 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)); > > > > > > > > > > + return; > > > > > + } > > > > > + > > > > > + mutex_lock(&ifbdev->helper.lock); > > > > > + if (!ifbdev->vma) > > > > > + intel_fbdev_dummy_fb_create(ifbdev); > > > > > + mutex_unlock(&ifbdev->helper.lock); > > > > > } > > > > > > > > Did you try > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > > index 65a3313723c9..4120c635742d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > > @@ -493,6 +493,7 @@ static bool intel_fb_initial_config(struct > > > > drm_fb_helper *fb_helper, > > > > bail: > > > > DRM_DEBUG_KMS("Not using firmware > > > > configuration\n"); > > > > memcpy(enabled, save_enabled, count); > > > > + fb_helper->deferred_setup = true; > > > > > > This is too earlier to set deferred_setup when > > > intel_fb_initial_config() fails, > > > __drm_fb_helper_initial_config_and_unlock() will then try to > > > probe/create a fb but as there is no sink connected it will fail > > > too. > > > > > > Also __drm_fb_helper_initial_config_and_unlock() is already setting > > > deferred_setup if both attempts fails. > > > > So... > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > b/drivers/gpu/drm/i915/intel_fbdev.c > > index 7d41d139341b..5f138a03dd2a 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct > > drm_device *dev) > > return; > > > > intel_fbdev_sync(ifbdev); > > - if (ifbdev->vma) > > + if (ifbdev->vma || ifbdev->helper->deferred_setup) > > drm_fb_helper_hotplug_event(&ifbdev->helper); > > } > > That would work but according to your comment here: https://patchwork.f > reedesktop.org/patch/196089/ that would lead to a crash in some case. In the deferred_setup case, won't it go to intelfb_create() and after that point if we still have no ifdev->vma we won't hit any other pathway? Just because it's been historically buggy (at least 2 instances now of having to apply protection here), it may have been fixed... Unlikely, but maybe it all finally works. -Chris
On Wed, 2018-04-18 at 17:55 +0100, Chris Wilson wrote: > Quoting Souza, Jose (2018-04-18 17:42:47) > > On Wed, 2018-04-18 at 09:26 +0100, Chris Wilson wrote: > > > Quoting Souza, Jose (2018-04-18 01:07:16) > > > > On Wed, 2018-04-18 at 00:44 +0100, Chris Wilson wrote: > > > > > Quoting José Roberto de Souza (2018-04-17 23:34:18) > > > > > > If the initial fbdev > > > > > > configuration(intel_fbdev_initial_config()) > > > > > > runs and > > > > > > there still no sink connected it will cause > > > > > > drm_fb_helper_initial_config() to return 0 as no error > > > > > > happened(but > > > > > > internally the return is -EAGAIN). > > > > > > Because no framebuffer was allocated, when a sink is > > > > > > connected > > > > > > intel_fbdev_output_poll_changed() will not execute > > > > > > drm_fb_helper_hotplug_event() that would trigger another > > > > > > try to > > > > > > do > > > > > > the > > > > > > initial fbdev configuration. > > > > > > > > > > > > So here creating a dummy framebuffer of 800x600, so > > > > > > drm_fb_helper_hotplug_event() will be executed and fbdev > > > > > > can be > > > > > > properly > > > > > > setup when a sink is connected, if needed the dummy > > > > > > framebuffer > > > > > > will be > > > > > > freed and a new with the proper size will be allocated. > > > > > > > > > > > > This issue also happens when a MST DP sink is connected > > > > > > since > > > > > > boot, > > > > > > as > > > > > > the MST topology is discovered in parallel if > > > > > > intel_fbdev_initial_config() > > > > > > is executed before the first sink MST is discovered it will > > > > > > cause > > > > > > this > > > > > > same issue. > > > > > > > > > > > > This is a follow up patch of > > > > > > https://patchwork.freedesktop.org/patch/196089/ > > > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1041 > > > > > > 58 > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1044 > > > > > > 25 > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_fbdev.c | 24 > > > > > > +++++++++++++++++++++++- > > > > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > > > > index 7d41d139341b..773577d39782 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > > > > @@ -696,14 +696,36 @@ int intel_fbdev_init(struct > > > > > > drm_device > > > > > > *dev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +static void intel_fbdev_dummy_fb_create(struct intel_fbdev > > > > > > *ifbdev) > > > > > > +{ > > > > > > + struct drm_fb_helper_surface_size sizes; > > > > > > + > > > > > > + sizes.fb_width = 800; > > > > > > + sizes.fb_height = 600; > > > > > > + sizes.surface_width = sizes.fb_width; > > > > > > + sizes.surface_height = sizes.fb_height; > > > > > > + sizes.surface_bpp = 32; > > > > > > + sizes.surface_depth = 24; > > > > > > + > > > > > > + if (intelfb_create(&ifbdev->helper, &sizes)) > > > > > > + DRM_ERROR("Unable to create dummy > > > > > > framebufer"); > > > > > > +} > > > > > > + > > > > > > static void intel_fbdev_initial_config(void *data, > > > > > > async_cookie_t > > > > > > cookie) > > > > > > { > > > > > > struct intel_fbdev *ifbdev = data; > > > > > > > > > > > > /* 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)); > > > > > > > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + mutex_lock(&ifbdev->helper.lock); > > > > > > + if (!ifbdev->vma) > > > > > > + intel_fbdev_dummy_fb_create(ifbdev); > > > > > > + mutex_unlock(&ifbdev->helper.lock); > > > > > > } > > > > > > > > > > Did you try > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > > > index 65a3313723c9..4120c635742d 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > > > @@ -493,6 +493,7 @@ static bool > > > > > intel_fb_initial_config(struct > > > > > drm_fb_helper *fb_helper, > > > > > bail: > > > > > DRM_DEBUG_KMS("Not using firmware > > > > > configuration\n"); > > > > > memcpy(enabled, save_enabled, count); > > > > > + fb_helper->deferred_setup = true; > > > > > > > > This is too earlier to set deferred_setup when > > > > intel_fb_initial_config() fails, > > > > __drm_fb_helper_initial_config_and_unlock() will then try to > > > > probe/create a fb but as there is no sink connected it will > > > > fail > > > > too. > > > > > > > > Also __drm_fb_helper_initial_config_and_unlock() is already > > > > setting > > > > deferred_setup if both attempts fails. > > > > > > So... > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > index 7d41d139341b..5f138a03dd2a 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct > > > drm_device *dev) > > > return; > > > > > > intel_fbdev_sync(ifbdev); > > > - if (ifbdev->vma) > > > + if (ifbdev->vma || ifbdev->helper->deferred_setup) > > > drm_fb_helper_hotplug_event(&ifbdev->helper); > > > } > > > > That would work but according to your comment here: https://patchwo > > rk.f > > reedesktop.org/patch/196089/ that would lead to a crash in some > > case. > > In the deferred_setup case, won't it go to intelfb_create() and after > that point if we still have no ifdev->vma we won't hit any other > pathway? It will, okay I will just run some tests and send it adding you as 'Signed-off-by:' that is okay? Thanks > > Just because it's been historically buggy (at least 2 instances now > of > having to apply protection here), it may have been fixed... Unlikely, > but maybe it all finally works. > -Chris
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 7d41d139341b..5f138a03dd2a 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev) return; intel_fbdev_sync(ifbdev); - if (ifbdev->vma) + if (ifbdev->vma || ifbdev->helper->deferred_setup) drm_fb_helper_hotplug_event(&ifbdev->helper); }