Re: [PATCH v4 5/5] drm: simpledrm: honour remove_conflicting_framebuffers()

[Noralf TrÃnnes]

Den 23.08.2016 20:01, skrev Daniel Vetter:
> On Tue, Aug 23, 2016 at 7:52 PM, Noralf TrÃnnes <noralf@xxxxxxxxxxx> wrote:
> > > > +static int sdrm_fbdev_event_notify(struct notifier_block *self,
> > > > +                                  unsigned long action, void *data)
> > > > +{
> > > > +       struct sdrm_device *sdrm;
> > > > +       struct fb_event *event = data;
> > > > +       struct fb_info *info = event->info;
> > > > +       struct drm_fb_helper *fb_helper = info->par;
> > > > +
> > > > +       if (action != FB_EVENT_FB_UNREGISTERED)
> > > > +               return NOTIFY_DONE;
> > > > +
> > > > +       if (!fb_helper || !fb_helper->dev || fb_helper->fbdev != info)
> > > > +               return NOTIFY_DONE;
> > > > +
> > > > +       sdrm = fb_helper->dev->dev_private;
> > > > +
> > > > +       if (sdrm && sdrm->fb_helper == fb_helper)
> > > > +
> > > > platform_device_del(to_platform_device(fb_helper->dev->dev));
> > > > +
> > > > +       return NOTIFY_DONE;
> > > > +}
> > > One problem this leaves behind is that registering of the new fbdev driver
> > > is too late - by that point we've already set up the entire driver,
> > > including modeset. If fbdev meanwhile does a dpms off or something like
> > > that all hell will break loose.
> >
> > I don't understand how fbdev registration comes into play here. Drivers call
> > remove_conflicting_framebuffers very early so simpledrm is gone by the time
> > they register anything.
> >
> > For simpledrm, fbdev doing blank/unblank is a no-op since fb_ops.fb_blank
> > is not implemented. So a fb_blank() just results in fbcon doing a
> > software blank.
> Maybe my scenario wasn't entirely clear:
> - prereq: fbdev emulation in drm is disabled
> 1. simpledrm loads and sets up the firmware fb
> 2. real driver loads, first calls
> drm_fb_helper_remove_conflicting_framebuffer. Nothing happens because
> CONFIG_FB=n.
> 3. real driver start loading, remapping the gart and what not else
> 4. something is drawn using fbcon, simplerdrm writes through the now
> invalid mapping
> -> BOOM

Yes CONFIG_FB=n is not covered, that's the drawback mentioned in the 
Kconfig.
However, who uses simpledrm without fbdev/fbcon when it shall be handed over
to a real hw-driver?
But yes, it's not a good stop gap solution, I like my other idea much 
better.

> You have code to listen to the framebuffer registration notifier, but
> I think even that happens way too late. Or at least I didn't spot any
> code in remove_conflicting_framebuffers which would call down into
> that notifier. Or maybe I entirely misunderstand your code ...

remove_conflicting_framebuffers unregisters the fbdev framebuffer.
sdrm_fbdev_event_notify detects that the framebuffer is being unregistered,
and deletes the platform device.

Some details:

do_remove_conflicting_framebuffers():
        for (i = 0 ; i < FB_MAX; i++) {
[...]
                        printk(KERN_INFO "fb: switching to %s from %s\n",
                               name, registered_fb[i]->fix.id);
                        ret = do_unregister_framebuffer(registered_fb[i]);

do_unregister_framebuffer(): short version
{
    console_lock();

        event.info = fb_info;
        ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
        unlock_fb_info(fb_info);
        console_unlock();

        pm_vt_switch_unregister(fb_info->dev);

        unlink_framebuffer(fb_info);

        registered_fb[i] = NULL;
        num_registered_fb--;

        event.info = fb_info;
        console_lock();
        fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
/* at this point simpledrm has been deleted */
        console_unlock();

        /* this may free fb info */
        put_fb_info(fb_info);
        return 0;
}

> Wrt fixing: Just adding it to the recently added stub is of course
> also a working solution.

I actually like this better, because it's so straightforward and easy
to understand. The notifier solution is very convoluted and easy to
mess up. And it runs with the console lock held...

> -Daniel
>
> PS: Can you pls review the 2 patches I submitted with you on cc? I
> won't merge my own patches without proper review, so without that done
> they're stuck.

Ok.

I'll test the simple-helper plane patch tomorrow.
In simpledrm I first tried to send the vblank event only in pipe.update(),
but I had to do it in enable() and disable() as well to make that vblank
timeout go away.


Noralf.