Re: [PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 26, 2015 at 02:14:37PM +0530, Archit Taneja wrote:
> 
> 
> On 08/26/2015 10:42 AM, Archit Taneja wrote:
> >
> >
> >On 08/25/2015 07:15 PM, Daniel Vetter wrote:
> >>Faster than recompiling.
> >>
> >>Note that restore_fbdev_mode_unlocked is a bit special and the only
> >>one which returns an error code when fbdev isn't there - i915 needs
> >>that one to not fall over with some additional fbcon related restore
> >>code. Everyone else just ignores the return value or only prints a
> >>DRM_DEBUG level message.
> >
> >Reviewed-by:Archit Taneja <architt@codeaurora.org>
> 
> 
> With the module param, and the drivers should see the following state(
> based on the truth table below):
> 
> module param | config option
>    true      |    true       -> real helper funcs called, driver
>                                 allocated drm_fb_helper is correctly
>                                 populated.
> 
>    false     |    true       -> real helper funcs called, but bailed
>                                 out early, driver allocated
>                                 drm_fb_helper is non-NULL, but we won't
>                                 end up using it.

Hm I tried to give drivers the same semantics here as with the stub
functions. Where did I screw up? The goal really was to match the end
result for drivers ...
-Daniel

>     X        |    false      -> stub functions called, drm_fb_helper is
>                                 NULL
> 
> I wonder if the 2nd combination could leave space for errors in other
> drivers. Drivers tend to check whether the fb_helper struct is NULL
> or not, and make decisions based on that. If the decisions are to
> just call a drm_fb_helper function, then we're okay. If they do something
> more than that, then we could have an issue.
> 
> I'll prepare the remainder of 'Step 2' part of the series over this,
> and we can test to check if we don't hit any corner cases.

> 
> Archit
> 
> >
> >>
> >>Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>---
> >>  drivers/gpu/drm/drm_fb_helper.c | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c
> >>b/drivers/gpu/drm/drm_fb_helper.c
> >>index 83aacb0cc9df..8259dec1de1f 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -39,6 +39,11 @@
> >>  #include <drm/drm_fb_helper.h>
> >>  #include <drm/drm_crtc_helper.h>
> >>
> >>+static bool drm_fbdev_emulation = true;
> >>+module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >>+MODULE_PARM_DESC(fbdev_emulation,
> >>+         "Enable legacy fbdev emulation [default=true]");
> >>+
> >>  static LIST_HEAD(kernel_fb_helper_list);
> >>
> >>  /**
> >>@@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
> >>drm_fb_helper *fb_helper)
> >>      struct drm_connector *connector;
> >>      int i;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      mutex_lock(&dev->mode_config.mutex);
> >>      drm_for_each_connector(connector, dev) {
> >>          struct drm_fb_helper_connector *fb_helper_connector;
> >>@@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct
> >>drm_fb_helper *fb_helper, struct drm_
> >>      struct drm_fb_helper_connector **temp;
> >>      struct drm_fb_helper_connector *fb_helper_connector;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> >>      if (fb_helper->connector_count + 1 >
> >>fb_helper->connector_info_alloc_count) {
> >>          temp = krealloc(fb_helper->connector_info, sizeof(struct
> >>drm_fb_helper_connector *) * (fb_helper->connector_count + 1),
> >>GFP_KERNEL);
> >>@@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct
> >>drm_fb_helper *fb_helper,
> >>      struct drm_fb_helper_connector *fb_helper_connector;
> >>      int i, j;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
> >>
> >>      for (i = 0; i < fb_helper->connector_count; i++) {
> >>@@ -375,6 +389,9 @@ int
> >>drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> >>*fb_helper)
> >>      bool do_delayed
> >>      int ret;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return -ENODEV;
> >>+
> >>      drm_modeset_lock_all(dev);
> >>      ret = restore_fbdev_mode(fb_helper);
> >>
> >>@@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
> >>      struct drm_crtc *crtc;
> >>      int i;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      if (!max_conn_count)
> >>          return -EINVAL;
> >>
> >>@@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
> >>
> >>  void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> >>  {
> >>+    if (!drm_fbdev_emulation)
> >>+        return;
> >>+
> >>      if (!list_empty(&fb_helper->kernel_fb_list)) {
> >>          list_del(&fb_helper->kernel_fb_list);
> >>          if (list_empty(&kernel_fb_helper_list)) {
> >>@@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct
> >>drm_fb_helper *fb_helper, int bpp_sel)
> >>      struct drm_device *dev = fb_helper->dev;
> >>      int count = 0;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      mutex_lock(&dev->mode_config.mutex);
> >>      count = drm_fb_helper_probe_connector_modes(fb_helper,
> >>                              dev->mode_config.max_width,
> >>@@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct
> >>drm_fb_helper *fb_helper)
> >>      struct drm_device *dev = fb_helper->dev;
> >>      u32 max_width, max_height;
> >>
> >>+    if (!drm_fbdev_emulation)
> >>+        return 0;
> >>+
> >>      mutex_lock(&fb_helper->dev->mode_config.mutex);
> >>      if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> >>          fb_helper->delayed_hotplug = true;
> >>
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx