| Message ID | 1440510314-8633-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > > 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; >
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. 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; >> >
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
On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote: > 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 ... Note that at least for i915 we can't make it perfectly equal since i915 compiles out a few more things with FBDEV_EMULATION=n than just the stubs. Long-term we might want to push some of that into helpers too perhaps. -Daniel
On 08/26/2015 05:04 PM, Daniel Vetter wrote: > 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 ... Right, they would behave like the stub functions. But driver level fbdev functions would still be called. For example, with DRM_FBDEV_EMULATION not set, a stub intel_fbdev_init() would be called. With DRM_FBDEV_EMULATION set but the module param not set, the non-stub intel_fbdev_init() would still be called, allocating an empty ifbdev. The ifbdev->helper would be passed to the fb_helper funcs, but the module param check will bail us out immediately. So, the code flow isn't exactly the same as compared to when DRM_FBDEV_EMULATION is disabled. The end result should be the same. My only concern was whether other drivers might get confused with a non-NULL fb_helper pointer. It most likely shouldn't be, though. Archit > -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 >
On 08/26/2015 05:07 PM, Daniel Vetter wrote: > On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote: >> 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 ... > > Note that at least for i915 we can't make it perfectly equal since i915 > compiles out a few more things with FBDEV_EMULATION=n than just the stubs. > Long-term we might want to push some of that into helpers too perhaps. Ah, I missed looking at this mail. Yeah, that's what I wanted to mainly point out. It looks okay otherwise. Since the param is 'true' by default. Things should be okay for all drivers. If someone reports an issue with a driver with the above combination, we could fix it individually. I guess the next step now is to remove the custom config fbdev emulation options and module params from drivers that have those. After that, we could start with scary process of removing the CONFIG_FB and CONFIG_DRM_KMS_FB_HELPER from each driver. Archit
On Wed, Aug 26, 2015 at 05:59:14PM +0530, Archit Taneja wrote: > > > On 08/26/2015 05:07 PM, Daniel Vetter wrote: > >On Wed, Aug 26, 2015 at 01:34:58PM +0200, Daniel Vetter wrote: > >>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 ... > > > >Note that at least for i915 we can't make it perfectly equal since i915 > >compiles out a few more things with FBDEV_EMULATION=n than just the stubs. > >Long-term we might want to push some of that into helpers too perhaps. > > Ah, I missed looking at this mail. Yeah we had to go ahead with removing I915_FBDEV since it was causing trouble if you disable one but not the other. I think i915 is the only driver where this can happen though, the others with their own fbdev Kconfig option disable a lot less. > Yeah, that's what I wanted to mainly point out. It looks okay > otherwise. > > Since the param is 'true' by default. Things should be okay for all > drivers. If someone reports an issue with a driver with the above > combination, we could fix it individually. > > I guess the next step now is to remove the custom config fbdev > emulation options and module params from drivers that have > those. > > After that, we could start with scary process of removing the > CONFIG_FB and CONFIG_DRM_KMS_FB_HELPER from each driver. Yeah, definitely should do that for 4.4. I'll pull in this one here with your r-b too, thanks for the feedback. -Daniel
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;
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. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)