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
