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

["Noralf Trønnes" <noralf@tronnes.org>]

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@tronnes.org>
>> ---

<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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel