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

[Archit Taneja <architt@codeaurora.org>]

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
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel