| Message ID | 20220504114808.1578304-1-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/fbdev: print error in case drm_fb_helper_initial_config fails | expand |
On Wed, 04 May 2022, Andrzej Hajda <andrzej.hajda@intel.com> wrote: > On some configurations drm_fb_helper_initial_config sometimes fails. > Logging error value should help debugging such issues. > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbdev.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > index 221336178991f0..557c7f15ac22a9 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -539,11 +539,16 @@ int intel_fbdev_init(struct drm_device *dev) > static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) > { > struct intel_fbdev *ifbdev = data; > + int ret; > > /* Due to peculiar init order wrt to hpd handling this is separate. */ > - if (drm_fb_helper_initial_config(&ifbdev->helper, > - ifbdev->preferred_bpp)) > - intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); > + ret = drm_fb_helper_initial_config(&ifbdev->helper, > + ifbdev->preferred_bpp); > + if (!ret) > + return; If this patch is intended for merging, I'd prefer keeping the failure path indented within if (ret). > + drm_err(ifbdev->helper.dev, "failed to set initial configuration: %pe\n", > + ERR_PTR(ret)); I'm leaning towards preferring "%s", errname(ret) over "%pe", ERR_PTR(ret) because everyone knows what %s means and using ERR_PTR() seems odd when it's really a plain int. BR, Jani. > + intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); > } > > void intel_fbdev_initial_config_async(struct drm_device *dev)
Hi Jani, On 05.05.2022 20:37, Jani Nikula wrote: > On Wed, 04 May 2022, Andrzej Hajda <andrzej.hajda@intel.com> wrote: >> On some configurations drm_fb_helper_initial_config sometimes fails. >> Logging error value should help debugging such issues. >> >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_fbdev.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c >> index 221336178991f0..557c7f15ac22a9 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c >> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c >> @@ -539,11 +539,16 @@ int intel_fbdev_init(struct drm_device *dev) >> static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) >> { >> struct intel_fbdev *ifbdev = data; >> + int ret; >> >> /* Due to peculiar init order wrt to hpd handling this is separate. */ >> - if (drm_fb_helper_initial_config(&ifbdev->helper, >> - ifbdev->preferred_bpp)) >> - intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); >> + ret = drm_fb_helper_initial_config(&ifbdev->helper, >> + ifbdev->preferred_bpp); >> + if (!ret) >> + return; > If this patch is intended for merging, I'd prefer keeping the failure > path indented within if (ret). > >> + drm_err(ifbdev->helper.dev, "failed to set initial configuration: %pe\n", >> + ERR_PTR(ret)); > I'm leaning towards preferring "%s", errname(ret) over "%pe", > ERR_PTR(ret) because everyone knows what %s means and using ERR_PTR() > seems odd when it's really a plain int. > > BR, > Jani. Apparently this patch didn't help in catching the bug, so no big feelings about it. Anyway I think I have found the issue I was looking for: hpd poll worker could be run during driver removal after console unregistration causing re-registration of console, which is not removed later leaving dangling pointers. If you could take a look at the patch [1], it would be nice :) [1]: https://patchwork.freedesktop.org/series/103621/#rev1 Regards Andrzej > >> + intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); >> } >> >> void intel_fbdev_initial_config_async(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index 221336178991f0..557c7f15ac22a9 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -539,11 +539,16 @@ int intel_fbdev_init(struct drm_device *dev) static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) { struct intel_fbdev *ifbdev = data; + int ret; /* Due to peculiar init order wrt to hpd handling this is separate. */ - if (drm_fb_helper_initial_config(&ifbdev->helper, - ifbdev->preferred_bpp)) - intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); + ret = drm_fb_helper_initial_config(&ifbdev->helper, + ifbdev->preferred_bpp); + if (!ret) + return; + drm_err(ifbdev->helper.dev, "failed to set initial configuration: %pe\n", + ERR_PTR(ret)); + intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); } void intel_fbdev_initial_config_async(struct drm_device *dev)
On some configurations drm_fb_helper_initial_config sometimes fails. Logging error value should help debugging such issues. Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> --- drivers/gpu/drm/i915/display/intel_fbdev.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)