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

[Noralf TrÃnnes]

Den 23.08.2016 14:41, skrev Daniel Vetter:
> On Mon, Aug 22, 2016 at 10:25:25PM +0200, Noralf Trønnes wrote:
> > There is currently no non-fbdev mechanism in place to kick out
> > simpledrm when the real hw-driver is probed. As a stop gap until
> > that is in place, honour remove_conflicting_framebuffers() and
> > delete the simple-framebuffer platform device when it's called.
> >
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > ---

<snip>

> > diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> > index c6596ad..7c6db2c 100644
> > --- a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> > +++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> > @@ -44,6 +44,20 @@ static int sdrm_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >   	return -ENODEV;
> >   }
> >
> > +/*
> > + * Releasing has to be done outside the notifier callchain when we're
> > + * kicked out, since do_unregister_framebuffer() calls put_fb_info()
> > + * after the notifier has run.
> > + * Open code drm_fb_helper_release_fbi() since fb_helper is freed at
> > + * this point when kicked out.
> > + */
> > +static void sdrm_fbdev_fb_destroy(struct fb_info *info)
> > +{
> > +	if (info->cmap.len)
> > +		fb_dealloc_cmap(&info->cmap);
> > +	framebuffer_release(info);
> > +}
> > +
> >   static struct fb_ops sdrm_fbdev_ops = {
> >   	.owner		= THIS_MODULE,
> >   	.fb_fillrect	= drm_fb_helper_sys_fillrect,
> > @@ -53,6 +67,7 @@ static struct fb_ops sdrm_fbdev_ops = {
> >   	.fb_set_par	= drm_fb_helper_set_par,
> >   	.fb_setcmap	= drm_fb_helper_setcmap,
> >   	.fb_mmap	= sdrm_fb_mmap,
> > +	.fb_destroy	= sdrm_fbdev_fb_destroy,
> >   };
> >
> >   static int sdrm_fbdev_create(struct drm_fb_helper *helper,
> > @@ -110,6 +125,9 @@ static int sdrm_fbdev_create(struct drm_fb_helper *helper,
> >   	fbi->screen_base = obj->vmapping;
> >   	fbi->fix.smem_len = sdrm->fb_size;
> >
> > +	fbi->apertures->ranges[0].base = sdrm->fb_base;
> > +	fbi->apertures->ranges[0].size = sdrm->fb_size;
> > +
> >   	return 0;
> >
> >   err_fbi_release:
> > @@ -188,9 +206,13 @@ void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> >   	sdrm->fb_helper = NULL;
> >   	fbdev = to_sdrm_fbdev(fb_helper);
> >
> > -	drm_fb_helper_unregister_fbi(fb_helper);
> > +	/* it might have been kicked out */
> > +	if (registered_fb[fbdev->fb_helper.fbdev->node])
> > +		drm_fb_helper_unregister_fbi(fb_helper);
> > +
> > +	/* freeing fb_info is done in fb_ops.fb_destroy() */
> > +
> >   	cancel_work_sync(&fb_helper->dirty_work);
> > -	drm_fb_helper_release_fbi(fb_helper);
> >
> >   	drm_framebuffer_unregister_private(fb_helper->fb);
> >   	drm_framebuffer_cleanup(fb_helper->fb);
> > @@ -199,3 +221,39 @@ void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> >   	drm_fb_helper_fini(fb_helper);
> >   	kfree(fbdev);
> >   }
> > +
> > +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.

> I think the only option is to add a new notifier chain for fbdev removal,
> called from remove_conflicting_framebuffers (even for CONFIG_FB=n, so need
> a fallback in core/fb_notify.c like with the other notifier I think). That
> would at least keep things working if fbdev is entirely disabled. Or have
> I entirely misunderstood how this works?

How about extending the new wrapper you just added. Something like this:

static int find_simpledrm_cb(struct device *dev, void *data)
{
    struct platform_device *pdev = to_platform_device(dev);
    char *name = data;

    DRM_INFO("Switching to %s from simpledrm\n", name);
    platform_device_del(pdev);

    return 0;
}

/* not sure where this function should go */
void drm_remove_simpledrm(const char *name)
{
    struct device_driver *drv;

    drv = driver_find("simpledrm", &platform_bus_type);
    if (drv)
        driver_for_each_device(drv, NULL, name, find_simpledrm_cb);

    /* or we can just unregister the driver instead */
    if (drv) {
        struct platform_driver *pdrv = to_platform_driver(drv);

        DRM_INFO("simpledrm is being removed by %s\n", name);
        platform_driver_unregister(pdrv);
    }
}

static inline int
drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a,
                          const char *name, bool primary)
{
    int ret;

    ret = remove_conflicting_framebuffers(a, name, primary);
    drm_remove_simpledrm(name);

    return ret;
}
#else
static inline int
drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a,
                          const char *name, bool primary)
{
    drm_remove_simpledrm(name);

    return 0;
}

This removes simpledrm unconditionally without looking at apertures_struct
though. Not sure if that is needed?


Noralf.

> Oh and another one, which we learned the hard way with i915: We probably
> need to get rid of vgacon too. And again we need to do that _before_ we
> start touching the hardware. I think simpledrm probably needs that too.
> But since simpledrm doesn't know on which pci device it sits it probably
> won't know if the same device is decoding vga signals, so no one knows how
> that is supposed to work :( One thing at the time probably.
> -Daniel
>
> > +
> > +static struct notifier_block sdrm_fbdev_event_notifier = {
> > +	.notifier_call  = sdrm_fbdev_event_notify,
> > +};
> > +
> > +void sdrm_fbdev_kickout_init(void)
> > +{
> > +	fb_register_client(&sdrm_fbdev_event_notifier);
> > +}
> > +
> > +void sdrm_fbdev_kickout_exit(void)
> > +{
> > +	fb_unregister_client(&sdrm_fbdev_event_notifier);
> > +}
> > --
> > 2.8.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel