On Tue, Jul 28, 2015 at 7:18 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> Trying to do anything with kms drivers when oopsing has become a
> failing proposition. But since we can end up in the fbdev code simply
> due to the console unblanking that's done unconditionally just
> removing our panic handler isn't enough. We need to block all fbdev
> callbacks when oopsing.
>
> There was already one in the blank handler, but it failed silently.
> That makes it impossible for drivers (like i915) who subclass these
> functions to figure this out.
>
> Instead consistently return -EBUSY so that everyone knows that we
> really don't want to be bothered right now. This also allows us to
> remove a pile of FIXMEs from the i915 fbdev code (since due to the
> failure code they now won't attempt to grab dangerous locks any more).
I guess drivers that were keeping fbdev buffer pinned just for opps'
can stop doing that now..
anyways, lgtm.. for the series:
Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Dave Airlie <airlied@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 24 ++++++++++++------------
> drivers/gpu/drm/i915/intel_fbdev.c | 21 ---------------------
> 2 files changed, 12 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 73f90f7e2f74..77f570a470da 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -491,14 +491,6 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> int i, j;
>
> /*
> - * fbdev->blank can be called from irq context in case of a panic.
> - * Since we already have our own special panic handler which will
> - * restore the fbdev console mode completely, just bail out early.
> - */
> - if (oops_in_progress)
> - return;
> -
> - /*
> * For each CRTC in this fb, turn the connectors on/off.
> */
> drm_modeset_lock_all(dev);
> @@ -531,6 +523,9 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> */
> int drm_fb_helper_blank(int blank, struct fb_info *info)
> {
> + if (oops_in_progress)
> + return -EBUSY;
> +
> switch (blank) {
> /* Display: On; HSync: On, VSync: On */
> case FB_BLANK_UNBLANK:
> @@ -755,9 +750,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> int i, j, rc = 0;
> int start;
>
> - if (__drm_modeset_lock_all(dev, !!oops_in_progress)) {
> + if (oops_in_progress)
> return -EBUSY;
> - }
> +
> + drm_modeset_lock_all(dev);
> if (!drm_fb_helper_is_bound(fb_helper)) {
> drm_modeset_unlock_all(dev);
> return -EBUSY;
> @@ -906,6 +902,9 @@ int drm_fb_helper_set_par(struct fb_info *info)
> struct drm_fb_helper *fb_helper = info->par;
> struct fb_var_screeninfo *var = &info->var;
>
> + if (oops_in_progress)
> + return -EBUSY;
> +
> if (var->pixclock != 0) {
> DRM_ERROR("PIXEL CLOCK SET\n");
> return -EINVAL;
> @@ -931,9 +930,10 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
> int ret = 0;
> int i;
>
> - if (__drm_modeset_lock_all(dev, !!oops_in_progress)) {
> + if (oops_in_progress)
> return -EBUSY;
> - }
> +
> + drm_modeset_lock_all(dev);
> if (!drm_fb_helper_is_bound(fb_helper)) {
> drm_modeset_unlock_all(dev);
> return -EBUSY;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index eef54feb7545..b763f24e20ef 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -55,13 +55,6 @@ static int intel_fbdev_set_par(struct fb_info *info)
> ret = drm_fb_helper_set_par(info);
>
> if (ret == 0) {
> - /*
> - * FIXME: fbdev presumes that all callbacks also work from
> - * atomic contexts and relies on that for emergency oops
> - * printing. KMS totally doesn't do that and the locking here is
> - * by far not the only place this goes wrong. Ignore this for
> - * now until we solve this for real.
> - */
> mutex_lock(&fb_helper->dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> mutex_unlock(&fb_helper->dev->struct_mutex);
> @@ -80,13 +73,6 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
> ret = drm_fb_helper_blank(blank, info);
>
> if (ret == 0) {
> - /*
> - * FIXME: fbdev presumes that all callbacks also work from
> - * atomic contexts and relies on that for emergency oops
> - * printing. KMS totally doesn't do that and the locking here is
> - * by far not the only place this goes wrong. Ignore this for
> - * now until we solve this for real.
> - */
> mutex_lock(&fb_helper->dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> mutex_unlock(&fb_helper->dev->struct_mutex);
> @@ -106,13 +92,6 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
> ret = drm_fb_helper_pan_display(var, info);
>
> if (ret == 0) {
> - /*
> - * FIXME: fbdev presumes that all callbacks also work from
> - * atomic contexts and relies on that for emergency oops
> - * printing. KMS totally doesn't do that and the locking here is
> - * by far not the only place this goes wrong. Ignore this for
> - * now until we solve this for real.
> - */
> mutex_lock(&fb_helper->dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> mutex_unlock(&fb_helper->dev->struct_mutex);
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
