Re: [PATCH 1/3] drm/fbdev: Return -EBUSY when oopsing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jul 28, 2015 at 11:55 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Jul 28, 2015 at 10:30:08AM -0400, Rob Clark wrote:
>> 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..
>
> Well there's still the kgdb stuff, but we might want to nuke that too.
> Also fbdev assumes (at least afaik) that the mmap is always valid and it
> just writes to it. At least that's why we have the fbdev_set_suspend call
> to in suspend/resume to tell it that now it's really no longer cool to
> draw fbcon. But userspace doesn't have anything like that I think.

we couldn't put_pages but we could unpin from iommu/gtt/etc and unpin
the pages so they could be moved by mm at least.. but letting 'em get
swapped out, probably not..

BR,
-R

>> anyways, lgtm.. for the series:
>>
>> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
>
> Thanks for the review, all merged.
> -Daniel
>
>>
>> > 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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux