Re: [PATCH v5 7/7] drm/simpledrm: add fbdev fallback support

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Sat, Sep 3, 2016 at 7:15 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 03.09.2016 14:04, skrev Noralf Trønnes:
>>
>>
>> Den 02.09.2016 10:22, skrev David Herrmann:
>>>
>>> Create a simple fbdev device during SimpleDRM setup so legacy user-space
>>> and fbcon can use it.
>>>
>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>
>>
>> [...]
>>
>>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>>> b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>>
>>
>> [...]
>>
>>> +void sdrm_fbdev_bind(struct sdrm_device *sdrm)
>>> +{
>>> +    struct drm_fb_helper *fbdev;
>>> +    int r;
>>> +
>>> +    fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>>> +    if (!fbdev)
>>> +        return;
>>> +
>>> +    drm_fb_helper_prepare(sdrm->ddev, fbdev, &sdrm_fbdev_funcs);
>>> +
>>> +    r = drm_fb_helper_init(sdrm->ddev, fbdev, 1, 1);
>>> +    if (r < 0)
>>> +        goto error;
>>> +
>>> +    r = drm_fb_helper_single_add_all_connectors(fbdev);
>>> +    if (r < 0)
>>> +        goto error;
>>> +
>>> +    r = drm_fb_helper_initial_config(fbdev,
>>> +                sdrm->ddev->mode_config.preferred_depth);
>>> +    if (r < 0)
>>> +        goto error;
>>> +
>>> +    if (!fbdev->fbdev)
>>> +        goto error;
>>> +
>>> +    sdrm->fbdev = fbdev;
>>> +    return;
>>> +
>>> +error:
>>> +    drm_fb_helper_fini(fbdev);
>>> +    kfree(fbdev);
>>> +}
>>> +
>>> +void sdrm_fbdev_unbind(struct sdrm_device *sdrm)
>>> +{
>>> +    struct drm_fb_helper *fbdev = sdrm->fbdev;
>>> +
>>> +    if (!fbdev)
>>> +        return;
>>> +
>>> +    sdrm->fbdev = NULL;
>>> +    drm_fb_helper_unregister_fbi(fbdev);
>>> +    cancel_work_sync(&fbdev->dirty_work);
>>> +    drm_fb_helper_release_fbi(fbdev);
>>> +    drm_framebuffer_unreference(fbdev->fb);
>>
>>
>> I get a warning that there are still fb's left during unbind:
>>
>> [   48.666003] WARNING: CPU: 0 PID: 716 at drivers/gpu/drm/drm_crtc.c:3855
>> drm_mode_config_cleanup+0x180/0x1f4 [drm]
>>
>> This worked:
>>
>> -       drm_framebuffer_unreference(fbdev->fb);
>> +       drm_framebuffer_unregister_private(fbdev->fb);
>> +       drm_framebuffer_cleanup(fbdev->fb);
>>
>
> Well not quite, this doesn't free the bo, so maybe this:
>
> -       drm_framebuffer_unreference(fbdev->fb);
> +       drm_framebuffer_unregister_private(fbdev->fb);
> +       sdrm_fb_destroy(fbdev->fb);
>
> IIRC the reason ref count doesn't drop to zero, had something to do with
> multiple
> fb_set_par calls taking reference but not being dropped later.

So I don't like this at all. If we leak references, we should fix the
ref-leak or properly document why this is fine to do. Daniel, what is
the exact reason we have unregister_private()? Maybe I should try and
trace the ref-leak.

Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel