Re: [PATCH 0/8] drm/fb-helper: Use drm_file to get a dumb framebuffer

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

Den 15.09.2017 18.37, skrev Noralf Trønnes:
>
> Den 15.09.2017 00.29, skrev Laurent Pinchart:
>> Hi Noralf,
>>
>> On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
>>> Den 13.09.2017 07.09, skrev Laurent Pinchart:
>>>> On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
>>>>> Hi,
>>>>>
>>>>> I want to start out by saying that this patchset is low priority 
>>>>> for me
>>>>> and if no one has interest or time to review this, that is just 
>>>>> fine. I
>>>>> was in the flow and just typed it out.
>>>>>
>>>>> This patchset adds a way for fbdev emulation code to create a
>>>>> framebuffer that is backed by a dumb buffer. drm_fb_helper gets a
>>>>> drm_file to hang the objects on, drm_framebuffer_create_dumb() 
>>>>> creates
>>>>> the framebuffer and drm_fb_helper_fini() destroys it.
>>>>> I have verified that all cma drivers supports dumb buffers, so
>>>>> converting the library should be fine for all.
>>>> Stupid question, what does this give us ? The series makes the call 
>>>> stack
>>>> more complex (up to a point where I'm getting trouble just 
>>>> following it),
>>>> what's the advantage that offsets that ?
>>> The short answer is that it avoids the need for special fbdev
>>> _with_funcs() functions in libraries like cma for framebuffers with the
>>> dirty callback set.
>>>
>>> I'm suprised that you think this more complex, I find it more coherent
>>> when fbdev userspace is doing things more like DRM userspace, like
>>> hanging the objects on a drm_file and cleaning up the objects when 
>>> done.
>> Maybe moving to a new code base gave me the wrong feeling that the 
>> result is
>> more complex. The current implementation is certainly not simple.
>>
>>> The longer and more diffuse answer is that it's annoying me that many
>>> drivers carry the burden of fbdev emulation just to get a console!
>> I totally agree with that. Ideally I'd like to remove 100% of 
>> fbdev-related
>> code from drivers. This includes
>>
>> - initialization and cleanup of fbdev helpers
>> - fbdev restore in last_close()
>> - forwarding of hotplug events to fbdev compatibility layer
>>
>> In practice we'll likely need to keep one initialization function (or a
>> drm_driver flag) to let drivers opt-in to fbdev compatibility, but 
>> the rest
>> should go away. Or maybe we could even enable fbdev compatibility in all
>> drivers unconditionally.
>
> Interesting idea, maybe something like this:
>

I just remembered that Daniel said something about the helpers shouldn't
bleed into the core, which my suggestion does :-/


>  struct drm_device {
> +     struct drm_fb_helper *fbdev;
>  };
>
>  void drm_lastclose(struct drm_device * dev)
>  {
>      DRM_DEBUG("\n");
>
>      if (dev->driver->lastclose)
>          dev->driver->lastclose(dev);
> +    else if (dev->fbdev)
> +        drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev);
>      DRM_DEBUG("driver lastclose completed\n");
>
>      if (drm_core_check_feature(dev, DRIVER_LEGACY))
>          drm_legacy_dev_reinit(dev);
>  }
>
>  void drm_kms_helper_hotplug_event(struct drm_device *dev)
>  {
>      /* send a uevent + call fbdev */
>      drm_sysfs_hotplug_event(dev);
>      if (dev->mode_config.funcs->output_poll_changed)
>          dev->mode_config.funcs->output_poll_changed(dev);
> +    else if (dev->fbdev)
> +        drm_fb_helper_hotplug_event(dev->fbdev);
>  }
>
>  void drm_dev_unregister(struct drm_device *dev)
>  {
>     struct drm_map_list *r_list, *list_temp;
>
> +    if (dev->fbdev)
> +        drm_fb_helper_unregister_fbi(dev->fbdev);
> +
>      drm_lastclose(dev);
>  ...
>  }
>
>  static void drm_dev_release(struct kref *ref)
>  {
>      struct drm_device *dev = container_of(ref, struct drm_device, ref);
>
> +    drm_fb_helper_simple_fini(dev);
> +
>      if (dev->driver->release) {
>          dev->driver->release(dev);
>      } else {
>          drm_dev_fini(dev);
>          kfree(dev);
>      }
>  }
>
> int driver_probe()
> {
>     ret = drm_fb_helper_simple_init(dev, ...);
>     if (ret)
>         DRM_WARN("Oh well fbdev init failed, but don't let that stop 
> us\n");
> }
>
> I've slightly adjusted some code I have in the pipeline:
>
> /**
>  * drm_fb_helper_simple_init - Simple fbdev emulation init
>  * @dev: DRM device
>  * @bpp_sel: bpp value to use for the framebuffer configuration
>  * @max_conn: max connector count
>  * @funcs: pointer to structure of functions associated with this helper
>  *
>  * Simple fbdev emulation initialization. This function allocates a
>  * &drm_fb_helper structure and calls:
>  * drm_fb_helper_prepare(), drm_fb_helper_init(),
>  * drm_fb_helper_single_add_all_connectors() and
>  * drm_fb_helper_initial_config().
>  *
>  * fbdev deferred I/O users should use drm_fb_helper_defio_init().
>  *
>  * Returns:
>  * Zero on success, negative error code on failure.
>  */
> int drm_fb_helper_simple_init(struct drm_device *dev, int bpp_sel, int 
> max_conn,
>                   const struct drm_fb_helper_funcs *funcs)
> {
>     struct drm_fb_helper *fb_helper;
>     int ret;
>
>     fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
>     if (!fb_helper)
>         return -ENOMEM;
>
>     drm_fb_helper_prepare(dev, fb_helper, funcs);
>
>     ret = drm_fb_helper_init(dev, fb_helper, max_conn);
>     if (ret < 0) {
>         DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
>         goto err_drm_fb_helper_free;
>     }
>
>     ret = drm_fb_helper_single_add_all_connectors(fb_helper);
>     if (ret < 0) {
>         DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
>         goto err_drm_fb_helper_fini;
>
>     }
>
>     ret = drm_fb_helper_initial_config(fb_helper, bpp_sel);
>     if (ret < 0) {
>         DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
>         goto err_drm_fb_helper_fini;
>     }
>
>     dev->fbdev = fb_helper;
>
>     return 0;
>
> err_drm_fb_helper_fini:
>     drm_fb_helper_fini(fb_helper);
> err_drm_fb_helper_free:
>     kfree(fb_helper);
>
>     return ret;
> }
> EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);
>
> /**
>  * drm_fb_helper_simple_fini - Simple fbdev cleanup
>  * @dev: DRM device
>  *
>  * Simple fbdev emulation cleanup. This function unregisters fbdev if 
> it is not
>  * done, cleans up deferred IO if necessary, removes framebuffer, 
> finalizes
>  * @fb_helper and frees the structure.
>  */
> void drm_fb_helper_simple_fini(struct drm_device *dev)
> {
>     struct drm_fb_helper *fb_helper = dev->fbdev;
>     struct fb_ops *fbops = NULL;
>     struct fb_info *info;
>
>     if (!fb_helper)
>         return;
>
>     info = fb_helper->fbdev;
>
>     /* Make sure it hasn't been unregistered already */
>     if (info && info->dev)
>         drm_fb_helper_unregister_fbi(fb_helper);
>
>     if (info && info->fbdefio) {
>         fb_deferred_io_cleanup(info);
>         kfree(info->fbdefio);
>         info->fbdefio = NULL;
>         fbops = info->fbops;
>     }
>
>     drm_fb_helper_fini(fb_helper);
>     kfree(fbops);
>
>     if (fb_helper->fb)
>         drm_framebuffer_remove(fb_helper->fb);
>
>     kfree(fb_helper);
> }
> EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);
>
> <snip>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx